Field error from list arg with nullable variable entry (nullable=optional clash?)

[benjie]

Consider the following currently valid schema and query:

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

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

With the following variables:

{
  "number": null
}

Now following through 6.1.2 Coercing Variable Values:

1. Let coercedValues be an empty unordered Map. ✅
2. Let variablesDefinition be the variables defined by operation. ✅
3. For each variableDefinition in variablesDefinition: ✅
   1. Let variableName be the name of variableDefinition. ✅
   2. Let variableType be the expected type of variableDefinition. ✅
   3. Assert: IsInputType(variableType) must be true. ✅
   4. Let defaultValue be the default value for variableDefinition. ✅
   5. Let hasValue be true if variableValues provides a value for the name variableName. ✅
   6. Let value be the value provided in variableValues for the name variableName. ✅
   7. If hasValue is not true and defaultValue exists (including null):
      1. Add an entry to coercedValues named variableName with the value defaultValue.
   8. Otherwise if variableType is a Non-Nullable type, and either hasValue is not true or value is null, raise a request error.
   9. Otherwise if hasValue is true: ✅
      1. If value is null: ✅
         1. Add an entry to coercedValues named variableName with the value null. ✅

For the variable definition $number: Int = 3:

• variableType is Int (NOT Int!).
• defaultValue is 3
• hasValue is true since number is provided in variables (even though it's null)
• value is null

Thus coercedValues becomes { number: null }.

When it comes to executing the field, this results in CoerceArgumentValues() raising a field error at 5.j.iii.1 (since [1, null , 3] cannot be coerced to [Int!]!).

Had the query have been defined with ($number: Int! = 3) then this error could not have occurred; but we explicitly allow the nullable Int with default in IsVariableUsageAllowed(). Presumably this is so that we can allow a default value to apply in the list, except this is a ListValue so there cannot be a default value there.

We've discussed "optionality" versus "nullability" a few times, but I'd like to get the WG's read on this.

Reproduction:

const { graphqlSync, GraphQLSchema, GraphQLList, GraphQLNonNull, GraphQLInt, GraphQLObjectType, validateSchema } = require('graphql');

const Query = new GraphQLObjectType({
  name: 'Query',
  fields: {
    sum: {
      type: GraphQLInt,
      args: {
        numbers: {
          type: new GraphQLNonNull(new GraphQLList(new GraphQLNonNull(GraphQLInt))),
        }
      },
      resolve(_, args) {
        let total = 0;
        for (const n of args.numbers) {
          total += n;
        }
        return total;
      }
    }
  }
});

const schema = new GraphQLSchema({
  query: Query
});

const errors = validateSchema(schema);
if (errors.length > 0) {
  throw errors[0];
}

{
  console.log('Testing query with variables and null');
  const result = graphqlSync({
    schema,
    source: `query Q($number: Int = 3) {sum(numbers:[1,$number,3])}`,
    variableValues: {
      number: null
    }
  });
  console.dir(result);
}

[mjmahone]

Yes this issue is terrible! It’s impossible to get the client to do the right thing here, unless we switched to requiring non-nullable usages to be defined by explicitly non-nullable variables. I personally am of the opinion that allowing nullable-with-default variables to be used in non-nullable positions was a mistake.

As a note, FB’s servers are not spec compliant in this case, as we convert an explicitly null variable with a default value into the default value. So in the above request $number is coerced to 3, not null. We therefore at execution time never have an error, but we have a subtle behavioral bug: every variable with a non-null default value is in fact non-nullable! No matter how hard the client tries to send a null.

I.e. instead of

Otherwise if hasValue is true:
a. If value is null:
a. Add an entry to coercedValues named variableName with the value null.

we do
ix. Otherwise if hasValue is true:
a. If value is null:
a. Add an entry to coercedValues named variableName with defaultValue (which is null if there is no defaultValue).

I’m fairly certain the FB-server behavior is the root cause of this weird anti-pattern, but now it would require a version bump to fix it, I think. Alternatively we could make it explicit that $number: Int = 3 is always behaviorally identical to $number: Int! = 3.

[benjie]

This is already known in the spec: http://spec.graphql.org/draft/#sec-All-Variable-Usages-are-Allowed.Allowing-optional-variables-when-default-values-exist

In the example below, an optional variable $booleanArg is allowed to be used in the non-null argument (nonNullBooleanArg) because the variable provides a default value in the operation. This behavior is explicitly supported for compatibility with earlier editions of this specification. GraphQL authoring tools may wish to report this as a warning with the suggestion to replace Boolean with Boolean! to avoid ambiguity.
Note: The value null could still be provided to such a variable at runtime. A non-null argument must raise a field error if provided a null value.

[benjie]

Maybe we should add a note here? https://spec.graphql.org/draft/#note-65769

[benjie]

Testing query with variables and null
{
  errors: [
    GraphQLError: Argument "numbers" has invalid value [1, $number, 3].
        at getArgumentValues (/home/benjie/Dev/DELETEME/repro/node_modules/graphql/execution/values.js:256:13)
        at executeField (/home/benjie/Dev/DELETEME/repro/node_modules/graphql/execution/execute.js:472:48)
        at executeFields (/home/benjie/Dev/DELETEME/repro/node_modules/graphql/execution/execute.js:413:20)
        at executeOperation (/home/benjie/Dev/DELETEME/repro/node_modules/graphql/execution/execute.js:344:14)
        at execute (/home/benjie/Dev/DELETEME/repro/node_modules/graphql/execution/execute.js:136:20)
        at graphqlImpl (/home/benjie/Dev/DELETEME/repro/node_modules/graphql/graphql.js:86:31)
        at graphqlSync (/home/benjie/Dev/DELETEME/repro/node_modules/graphql/graphql.js:33:18)
        at Object.<anonymous> (/home/benjie/Dev/DELETEME/repro/script.js:35:18)
        at Module._compile (node:internal/modules/cjs/loader:1103:14)
        at Object.Module._extensions..js (node:internal/modules/cjs/loader:1155:10) {
      path: [Array],
      locations: [Array],
      extensions: [Object: null prototype] {}
    }
  ],
  data: [Object: null prototype] { sum: null }
}

[benjie]

Related: #418

[rivantsov]

(prev post deleted, sorry for distraction)
I do not see any way out of it, I think it remains runtime error, as it is now; it is not preventable by extra spec rules, null value is not missing value.