[RFC] List coercion algorithm

[benjie]

Fixes #1002.

Previously, list coercion does not detail what to do with variables at all, and that could lead to either a null pointer exception, or to double-coercion of the variable value if you're only following the spec.

Consider the following valid schema:

type Query {
  sum(numbers:[Int!]!): Int
}

and the query that is valid against this schema:

query Q ($number: Int = 3) {
  sum(numbers: [1, $number, 3])
}

NOTE: We're using the variable in a list item position!

If you issue this to the GraphQL server with variables {"number": null} then CoerceVariableValues will give you {"number": null} and when you fast-forward to CoerceArgumentValues you'll go in to 5.j.iii.1:

https://spec.graphql.org/draft/#sel-NANTHHCJFTDFBBCAACGB0yS

• Let {coercedValues} be an empty unordered Map. coercedValues = {}
• Let {argumentValues} be the argument values provided in {field}. argumentValues = { numbers: [1, $number, 3] }
• Let {fieldName} be the name of {field}. fieldName = 'sum'
• Let {argumentDefinitions} be the arguments defined by {objectType} for the
  field named {fieldName}. argumentDefinitions = { numbers: ... }
• For each {argumentDefinition} in {argumentDefinitions}:
  • Let {argumentName} be the name of {argumentDefinition}. argumentName = 'numbers'
  • Let {argumentType} be the expected type of {argumentDefinition}. argumentType = [Int!]!
  • Let {defaultValue} be the default value for {argumentDefinition}. defaultValue = undefined
  • Let {hasValue} be {true} if {argumentValues} provides a value for the name
    {argumentName}. hasValue = true
  • Let {argumentValue} be the value provided in {argumentValues} for the name
    {argumentName}. argumentValue = [1, $number, 3]
  • If {argumentValue} is a {Variable}: NOPE
    • Let {variableName} be the name of {argumentValue}.
    • Let {hasValue} be {true} if {variableValues} provides a value for the name
      {variableName}.
    • Let {value} be the value provided in {variableValues} for the name
      {variableName}.
  • Otherwise, let {value} be {argumentValue}. value = [1, $number, 3]
  • If {hasValue} is not {true} and {defaultValue} exists (including {null}): NOT TRIGGERED
    • Add an entry to {coercedValues} named {argumentName} with the value
      {defaultValue}.
  • Otherwise if {argumentType} is a Non-Nullable type, and either {hasValue} is
    not {true} or {value} is {null}, raise a field error. NOT TRIGGERED
  • Otherwise if {hasValue} is true: Yes, it is
    • If {value} is {null}: It is not, it is a list
      • Add an entry to {coercedValues} named {argumentName} with the value
        {null}.
    • Otherwise, if {argumentValue} is a {Variable}: It is not, it is a list
      • Add an entry to {coercedValues} named {argumentName} with the value
        {value}.
    • Otherwise: YES
      • If {value} cannot be coerced according to the input coercion rules of
        {argumentType}, raise a field error. TIME TO VISIT LIST COERCION
      • Let {coercedValue} be the result of coercing {value} according to the
        input coercion rules of {argumentType}.
      • Add an entry to {coercedValues} named {argumentName} with the value
        {coercedValue}.
• Return {coercedValues}.

Time to visit list coercion

We need to coerce the value [1, $number, 3] to the non-nullable type [Int!]!.

Step 1: handle the non-null. It's not null. Great!

Now we need to coerce the value [1, $number, 3] to the list type [Int!].

Here's what the spec says about input coercion for lists:

When expected as an input, list values are accepted only when each item in the list can be accepted by the list’s item type.

If the value passed as an input to a list type is not a list and not the null value, then the result of input coercion is a list of size one, where the single item value is the result of input coercion for the list’s item type on the provided value (note this may apply recursively for nested lists).

This allows inputs which accept one or many arguments (sometimes referred to as “var args”) to declare their input type as a list while for the common case of a single value, a client can just pass that value directly rather than constructing the list.

We have a list, so we only care about the bold line.

This line seems to miss a bunch of situations.

For example: if we were coercing to [Int] the value [1, $number, 3] with variables {} then is $number (which is undefined, since it wasn't provided in the variables) "accepted by the list's item type"? Really we must coerce this to null, but that doesn't seem to be detailed. In fact this entire section doesn't mention variables at all.

We're actually coercing to [Int!], so the question is: is $number accepted by the list's item type? $number itself is a variable, so...

---

I've attempted to solve this problem by being much more explicit about the input coercion for lists, inspired by the input coercion for input objects. I've also added a non-normative note highlighting the risk of a null variable being fed through into a non-nullable position, why that can occur (validation) and what we do about it (field error). I've also expanded the table with both variables and many more examples to cover many more edge cases.

[netlify]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	fba35d5
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/6748b3294008e900085893b4
😎 Deploy Preview	https://deploy-preview-1058--graphql-spec-draft.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[mjmahone]

This feels like it brings list coercion to the same (currently buggy due to nullable-with-default-values) state as all the other variable coercion so this feels right to me

[martinbonnin]

Is there a specific reason this doesn't use the "pre-coerced" variable values? (from CoerceVariableValues)

[benjie]

The reason was that this is essentially copied from the input coercion for input objects (but via an algorithm to make it clearer): https://spec.graphql.org/draft/#sec-Input-Objects.Input-Coercion

But it's a good question. I guess the reason is that coercedVariableValues is not explicitly made available in section 3 of the spec.

[martinbonnin]

I see... It may be an issue:

Ultra synthetic example:

type Query {
  a(b: [[Int]]): Int
}

Operation:

query Foo($c: [Int]) {
  a(b: [$c])
}

Runtime Variables:

{
  "c": 42
}

If we're saying the runtime value is the "value that is sent over the wire", we end up with b = [42] (incompatible) instead of b = [[42]] if we coerce the variable to a list first (I think?)
If we're saying the runtime value is the "pre-coerced" value, then there's no need to mention defaultValue

[JoviDeCroock]

That should work in GraphQL-JS as well from what I can tell

• We see that b expects a List https://github.com/graphql/graphql-js/blob/16.x.x/src/utilities/valueFromAST.ts#L77
• We see that b gets a list as an AST-Argument https://github.com/graphql/graphql-js/blob/16.x.x/src/utilities/valueFromAST.ts#L79
• We go into the for loop, the first element is a variable, we recurse https://github.com/graphql/graphql-js/blob/16.x.x/src/utilities/valueFromAST.ts#L90
• Inside of the recursion we see that this value is expected to be an array again https://github.com/graphql/graphql-js/blob/16.x.x/src/utilities/valueFromAST.ts#L77
• The received value is not a list, but a variable, we get the variable and return it in an array https://github.com/graphql/graphql-js/blob/16.x.x/src/utilities/valueFromAST.ts#L99-L103

[martinbonnin]

@JoviDeCroock I think it works in graphql-js because variables (here) are coerced already?

Because variable are coerced super early in validateExecutionArgs?

And because CoerceVariableValues is already taking care of variables default values, there's no need to be explicit about them here.

I think there is some value in making variableValues available in Section 3. I opened a PR on the PR here: benjie#2

A more formal version would define a CoerceXXXValue(value, type, variableValues) for every type but that's probably out of scope for this specific PR.

[martinbonnin]

Also, I think the last 2 steps of the code path is more like so?

• Inside of the recursion we see that this value is a variable https://github.com/graphql/graphql-js/blob/16.x.x/src/utilities/valueFromAST.ts#L49
• The variable value is returned as is and it's working fine because it was coerced to an array before: https://github.com/graphql/graphql-js/blob/16.x.x/src/utilities/valueFromAST.ts#L62

[JoviDeCroock]

That understanding is not what I'm saying though, notice how we return [result] rather than result? Meaning we do coerce into a list as that's the expected type of the argument, regardless of the variable type, so the variable coerced or not seems irrelevant. Or I am misunderstanding the point entirely

[martinbonnin]

On line 103 there's coercion indeed:

  if (isListType(type)) {
    // ....
    return [coercedValue]; // line 103
  } 

But my understanding is that line 103 is not reached because we should enter the branch in line 49 first?:

  if (valueNode.kind === Kind.VARIABLE) { // line 49
    // ..
    return variableValue;
  }

  // ...
  // not reached 
  if (isListType(type)) { 
  // ...

Not super familiar with the codebase so apologies if that's absolutely not how it works but it feels like it should work like so?

[JoviDeCroock]

This one is also in line with GraphQL JS we deeply traverse and perform the same logic as during normal coercion on each value we find deeply