SPEC/BUG: Ambiguity with null variable values and default values

[leebyron]

This change corresponds to a spec proposal (graphql/graphql-spec#418) which solves an ambiguity in how variable values and default values behave with explicit null values, and changes validation rules to better define the behavior of default values. Otherwise, this ambiguity can allows for null values to appear in non-null argument values, which may result in unforeseen null-pointer-errors.

In summary this change includes:

• BREAKING: Removal of VariablesDefaultValueAllowed validation rule. All variables may now specify a default value.

• Change to VariablesInAllowedPosition rule to explicitly not allow a null default value when flowing into a non-null argument, and now allows optional (nullable) variables in non-null arguments that provide default values.

• Changes to ProvidedRequiredArguments rule (BREAKING: renamed from ProvidedNonNullArguments) to no longer require values to be provided to non-null arguments which provide a default value.

• Changes to getVariableValues() and getArgumentValues() to ensure a null value never flows into a non-null argument.

• Changes to valueFromAST() to ensure null variable values do not flow into non-null types.

• Adds to the TypeInfo API to allow referencing the expected default value at a given AST position.

---

This appears in three distinct but related issues:

VariablesInAllowedPosition validation rule

The explicit value null may be used as a default value for a variable, however VariablesInAllowedPositions allowed a nullable type with a default value to flow into an argument expecting a non-null type. This validation rule must explicitly not allow null default values to flow in this manner.

Coercing Variable Values

coerceVariableValues allows the explicit null value to be used over a default value, which can result in flowing a null value to a non-null argument when paired with the validation rule mentioned above. Instead a default value must be used even when an explicit null value is provided.

Coercing Argument Values

coerceArgumentValues allows the explicit null default value to be used before checking for a non-null type. This could inadvertently allow a null value into a non-null typed argument.

[IvanGoncharov]

isNullish checks that value !== value that is not a problem if variables always comes from JSON. But I see a potential problem with NaN if variables come from some binary protocol or created by JS.

BTW. Nothing prevents a user from creating custom scalar like DoubleIEEE754 that accepting NaN, Infinity, etc.

[leebyron]

That's fair. This could be a more general == null check

[IvanGoncharov]

I think it's confusing to get such error if you pass null:

Variable "${name}" of required type "${String(argType)}" was not provided.

I think it should have a special error message for this case, similar to how it's done for arguments:

`Variable "${name}" of non-null type "${String(argType)}" must not be null.'

[leebyron]

Great point, I agree

[leebyron]

Changes based on feedback:

This preserves the behavior of explicit null values taking precedence over default values, greatly reducing the scope of breaking changes.

This adds a change to argument coercion to enforce null checking on variable valuesand removes one validation rule: VariablesDefaultValueAllowed. This allows supplying default values to non-nullable variables, widening the gap between "required" and "nullable". It also changes the validation rules for arguments - allowing non-null arguments with default values to be omitted.

This preserves all existing behavior (with the critical exception of no longer allowing null values into non-null arguments) while allowing queries which were previously considered invalid to be valid.

[leebyron]

Updated to most recent version of spec proposal.

This is now a breaking change. The default validation rules are stricter, however with a configuration flag the previous lax behavior can be used which will ensure an existing service can support all existing incoming operations.

For example to continue to support existing queries after updating to the new version, replace:

graphql({ schema, source })

With:

graphql({ schema, source, options: {
  allowNullableVariablesInNonNullPositions: true
}})

Another more minor breaking change is that the final typeInfo argument to validate has moved positions. However very few should be reliant on this experimental arg.

[leebyron]

See graphql/graphql-spec#418 (comment) - after discussion, the breaking change is too costly at large, so I'll be updating this to account for that.

[leebyron]

Ok, I've updated this to match the latest proposal in the spec and just updated the description to be accurate to the final version of the change. At this point I believe this is ready to merge. I'll first wait for a bit for others to review and comment.