Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

returning null for a custom scalar's serialize #1057

Closed
bradzacher opened this issue Oct 6, 2017 · 2 comments
Closed

returning null for a custom scalar's serialize #1057

bradzacher opened this issue Oct 6, 2017 · 2 comments

Comments

@bradzacher
Copy link

I'm trying to build some custom types to create reusable type/value checking within my API.

The problem I've got is that if there's a problem with the serialization, there's no way to silently ignore the value.

For example, in mysql, a non-null DATETIME that's ignored at insert time is assigned the value 0000-00-00 00:00:00.
The mysql2 DB adapter automatically converts the db value to a JS date via the Date constructor, which for the zero'd value above gives an Invalid Date.

I was creating a custom date type with the serialize function as follows:

serialize: function(value : Date) {
    if (isNaN(value.getTime()) {
        return null
    }

    return customDateFormatter(value)     
},

The problem is that if you return null from the serialize function, graphql will automatically throw an error on your behalf (

const serializedResult = returnType.serialize(result);
if (isNullish(serializedResult)) {
throw new Error(
`Expected a value of type "${String(returnType)}" but ` +
`received: ${String(result)}`
);
}
).

In execute.js completeValue:

// If result value is null-ish (null, undefined, or NaN) then return null.
if (isNullish(result)) {
return null;
}

The internals will happily silently return null (and not call serialize) if the value that comes into the function is null, but there is no way to stop it throwing if serialize returns null/undefined.

Considering that I can happily throw a handled exception from within the serialize function, how come you chose to also automatically throw one for me?

I can think of a lot of use cases where you might want to silently ignore the fact that serialize returns null.

Is it possible to get a way to make graphql-js allow a serialize function to return null?

@bradzacher bradzacher changed the title returning null for a custom scalar returning null for a custom scalar's serialize Oct 6, 2017
@bradzacher
Copy link
Author

Reading through the graph-ql spec - in particular this section: https://facebook.github.io/graphql/October2016/#sec-Value-Completion

  1. If fieldType is a Scalar or Enum type:
    a. Return the result of “coercing” result, ensuring it is a legal value of fieldType, otherwise null.

It seems like the design of graphql-js goes against the spec in this instance.

@bradzacher
Copy link
Author

It's a bit fuzzy though, because this section: https://facebook.github.io/graphql/October2016/#sec-Scalars

A GraphQL server, when preparing a field of a given scalar type, must uphold the contract the scalar type describes, either by coercing the value or producing an error.
...
For example, a GraphQL server could be preparing a field with the scalar type Int ... If the server encounters some value that cannot be reasonably coerced to an Int, then it must raise a field error.

The first sentence doesn't really mention how to handle nulls, and when taken in conjunction with the above extract on value completion, it seems like throwing an error is optional.

leebyron added a commit that referenced this issue Dec 1, 2017
This changes the check for null/undefined to a check for undefined to determine if scalar serialization was successful or not, allowing `null` to be returned from serialize() without indicating error.

This is potentially breaking for any existing custom scalar which returned `null` from `serialize()` to indicate failure. To account for this change, it should either throw an error or return `undefined`.

Fixes #1057
Closes #1066
leebyron added a commit that referenced this issue Dec 1, 2017
This changes the check for null/undefined to a check for undefined to determine if scalar serialization was successful or not, allowing `null` to be returned from serialize() without indicating error.

This is potentially breaking for any existing custom scalar which returned `null` from `serialize()` to indicate failure. To account for this change, it should either throw an error or return `undefined`.

Fixes #1057
Closes #1066
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant