Spec inconsistent when dealing with nullable variables

[eapache]

Consider the following trivial schema:

type QueryRoot {
  field(argument: Int!): Int!
}

And the following query against this schema:

query foo($variable: Int = 10) {
  field(argument: $variable)
}

Even though $variable is nullable, the fact that it has a default value explicitly allows it to be used for argument which is non-null (https://facebook.github.io/graphql/#sec-All-Variable-Usages-are-Allowed).

Now consider what happens if I pass an explicit null for the value of $variable (e.g. { "variable" => null" }). This is in the spec as being semantically different from not providing the variable at all (https://facebook.github.io/graphql/#sec-Null-Value) so it would not be correct to use the default value. Variable validation is run against the actual type of the variable which is nullable, so it must pass (https://facebook.github.io/graphql/#sec-Coercing-Variable-Values). But the variable usage is also already validated, since it doesn't take into account the actual variable value when it's run.

This is clearly wrong (you end up with a null value used for a non-null argument) but it is not clear to me what part of the validation process is supposed to fail here?

cc @rmosolgo

[theorygeek]

I agree, this is a very odd interaction between default values for variables, and explicit/implicit null values. An implicit null value causes the default value to be used, but an explicit null value does not.

It seems to me that:

• The variable's actual type should be validated against the field's expected type, irrespective of whether or not the variable has a default value.
• If the variable has a default value, then an implicit null should always be allowed (the application would observe it as having the default value).
• If the variable is not nullable, then an explicit null should always be rejected.

Alternatively, the explicit null could cause the application to observe the variable's default value. But this seems confusing, I would venture that most engineers expect explicit null's to override default values.

But still, it seems quite odd to me that these two inputs would be treated differently by the server:

{ "variable": null }

{  }

In the end, I think that GraphQL was more predictable when we did not try to accommodate the implicit/explicit distinction. These kinds of things are very transport-centric, but the spec has always purported to be transport-agnostic. If it were up to me, I think I would revert the addition of the null keyword to the spec, and establish it as a best practice that implementers not try to differentiate implicit and explicit null's.

But, short of that, it seems like we can certainly clarify the expectations here. Also, maybe the spec should use the term "undefined" instead of "implicit" null's. (For example, "If value is undefined" rather than "If value does not exist").

[eapache]

The variable's actual type should be validated against the field's expected type, irrespective of whether or not the variable has a default value.

I believe this is a result of the spec conflating "required" and "non-nullable" - it is specified in https://facebook.github.io/graphql/#sec-Variable-Default-Values-Are-Correctly-Typed that "If variableType is non‐null it cannot have a default value", presumably because if it is non-null the user is required to specify a value so the default is useless. Now that we have explicit nulls this conflation no longer makes sense.

Alternatively, the explicit null could cause the application to observe the variable's default value. But this seems confusing, I would venture that most engineers expect explicit null's to override default values.

That was my expectation as well, and at least in the ruby implementation it does override the default value, resulting in a bug where the the field is resolved with a non-null variable set to null.

---

Without having worked through all the implications fully, the solution I think makes the most sense is:

• Separate nullability from requiredness by allowing non-null variables/arguments/input-fields to have default values.
• Remove the clause about treating default-valued-variables as non-null.

The result of this would be that my original example query would be malformed because it's trying to use $variable: Int for an argument of type Int!. The ambiguity can be resolved by making $variable of type Int! as well (without losing the default value), making the validation error clear if somebody provides { "variable": null }.

[leebyron]

But still, it seems quite odd to me that these two inputs would be treated differently by the server: { "variable": null }, { }

These need to be treated differently since null is an explicit value.

In the end, I think that GraphQL was more predictable when we did not try to accommodate the implicit/explicit distinction.

I agree with you. Allowing null to be an explicit value which is treated differently from implicit absence resulted in a lot of additional complexity. In exchange having this semantic different allows for a few arguably-important use cases (like deletion of fields in CRUD-style mutations).

That ship has sailed though.

I attempted to resolve the confusion around these use cases in #418, even though I agree there are still corner cases where it is possible to create confusion - at least it should not result in clearly wrong outcomes like the one originally pointed out in this issue.

Please open new issues if you spot anything not covered in #418 related to this!