graphql-js
graphql-js copied to clipboard
Should a variable value of undefined be converted to null?
See https://github.com/ardatan/graphql-tools/issues/1453
Variables not present within the variableValues object are left alone (https://github.com/graphql/graphql-js/blob/master/src/execution/values.js#L103) but variables present but undefined are later coerced to null.
This may be by design?
@yaacovCR Excellent question 👍 Technically it's correct according to the spec:
Let hasValue be true if variableValues provides a value for the name variableName.
http://spec.graphql.org/draft/#sel-JANLLFCFFJCAACIB1nF
But pragmatically in JS we can treat undefined
as absent value.
However, we need a consistent approach to undefined
in this library and since undefined
returned from resolver is converted to null
we need to maintain the same rule in all other places that work with values.
Does this fully answer your question?
@yaacovCR Thinking more about this maybe we need to change this in the next major version.
Do you know how widespread it is to set allowUndefinedInResolve
to false
in user codebases?
https://github.com/ardatan/graphql-tools/blob/2b1b5e3e48f23a81b286c48134507b6666eb0ccc/packages/schema/src/makeExecutableSchema.ts#L51
I'm not sure I understand the correspondence you're making between conversion of undefined to null within the return value of resolvers and the issue here...
I would think passing a value of undefined in the variable values should be equivalent to not passing it at all and returning undefined in a resolver should be equivalent to not returning anything which should also be coerced to null...
but in regards to your question, I am not personally sure, others might know better, actually forgot that option existed
To use typescript terms, maybe, I think undefined and void should be treated the same within graphql-js...
@yaacovCR I think you right. But it's too late in my timezone to think clearly so I need to sleep with it.
I'm not sure I understand the correspondence you're making between conversion of undefined to null within the return value of resolvers and the issue here...
Because of this line undefined
as variable value is converted to null
and the same transformation happens for return value of resolvers. It's way better to have a single rule that undefined => null
for the entire library.
Lol, get some sleep, I am pretty sure as this is not caused an issue until now and there is an acceptable workaround in GraphQL tools already, that this is the least of all issues in terms of priority
This is a breaking change and we can't migrate to the latest version of graphql-tools
easily.
In some of our resolvers we pass the arguments/input directly to MongoDB as the filter object. null
has meaning in MongoDB (https://docs.mongodb.com/manual/tutorial/query-for-null-fields/) and some of our queries with advanced filters stopped working and quite a lot of unexpected results throughout our back-office that makes extensive use of filtering queries. ):
At the very least, there could be a breaking change note on v5. If a configuration option can be made for this, even better.
Should be fixed in v6 of graphql-tools, what version are you on?
I was updating to v5 because I'm using Apollo Server and wanted to get rid of the fork. I wasn't considering v6 because it installs too many packages that we don't use so I'd probably wait for the Apollo team to decide the way forward on that.
Actually, we could probably take the discussion offline as this is not really relevant to this issue...but in v6 you should be able to decrease bundle size over v5 by only including the packages you need, import from @graphql-tools/schema, @graphql-tools/wrap @graphql-tools/stitch for whatever functionality you need rather than from graphql-tools, although latter is supported for backwards compatibility.
This is actually one of the main improvements in v6!
If you are doing so already and bundle size has gone up, you should probably open an issue at https://github.com/ardatan/graphql-tools/issues, maybe more of the generic utility functions need to be moved elsewhere.
This could be considered for v17
Updating code site:
https://github.com/graphql/graphql-js/blob/76e47fcd7d432f515d5bb99e95af1851148a0c54/src/execution/values.ts#L186-L202
@IvanGoncharov et al --
Can this be considered for v17?
Actually, I think this case should not exist, and was due to improper use of getArgumentValues in client code within graphql-tools that has since been fixed. Perhaps the code could still be cleaned up, though.
https://github.com/graphql/graphql-js/pull/3811#discussion_r1097258865
I am seeing this problem with v16.6.0. A scalar argument submitted on a mutation as undefined
is converted to null
by the time it reaches the resolver. This makes it impossible to delineate the difference between an intentionally submitted null
and a completely left-out optional argument. Tracing this out, the value is correctly carried to the server as undefined
until it reaches the getArgumentValues()
call in execute.js. At this point the resulting args
variable has swapped out undefined
for null
and the original input fidelity is lost by the time the resolver is invoked. Is this a regression?