Should a variable value of undefined be converted to null?

[yaacovCR]

See ardatan/graphql-tools#1453

[yaacovCR]

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?

[IvanGoncharov]

@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?

[IvanGoncharov]

@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

[yaacovCR]

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...

[yaacovCR]

but in regards to your question, I am not personally sure, others might know better, actually forgot that option existed

[yaacovCR]

To use typescript terms, maybe, I think undefined and void should be treated the same within graphql-js...

[IvanGoncharov]

@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.

[yaacovCR]

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

[alfaproject]

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.

[yaacovCR]

Should be fixed in v6 of graphql-tools, what version are you on?

[alfaproject]

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.

[yaacovCR]

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.

[yaacovCR]

This could be considered for v17

[yaacovCR]

Updating code site:

graphql-js/src/execution/values.ts

Lines 186 to 202 in 76e47fc

	const variableName = valueNode.name.value;
	if (
	variableValues == null ||
	!hasOwnProperty(variableValues, variableName)
	) {
	if (argDef.defaultValue !== undefined) {
	coercedValues[name] = argDef.defaultValue;
	} else if (isNonNullType(argType)) {
	throw new GraphQLError(
	`Argument "${name}" of required type "${inspect(argType)}" ` +
	`was provided the variable "$${variableName}" which was not provided a runtime value.`,
	{ nodes: valueNode },
	);
	}
	continue;
	}
	isNull = variableValues[variableName] == null;

@IvanGoncharov et al --

Can this be considered for v17?

[yaacovCR]

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.

[yaacovCR]

#3811 (comment)

[robross0606]

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?