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

benjie · 2022-12-06T09:52:00Z

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:

Let coercedValues be an empty unordered Map. ✅
Let variablesDefinition be the variables defined by operation. ✅
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);
}

The text was updated successfully, but these errors were encountered:

mjmahone · 2022-12-15T04:15:32Z

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 · 2022-12-15T18:58:32Z

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 · 2022-12-15T19:05:39Z

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

benjie · 2022-12-15T19:12:09Z

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 · 2022-12-15T20:25:26Z

Related: #418

rivantsov · 2022-12-23T00:43:00Z

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

benjie linked a pull request Nov 9, 2023 that will close this issue

[RFC] List coercion algorithm #1058

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

benjie commented Dec 6, 2022 •

edited

Loading

mjmahone commented Dec 15, 2022 •

edited

Loading

benjie commented Dec 15, 2022

benjie commented Dec 15, 2022

benjie commented Dec 15, 2022

benjie commented Dec 15, 2022

rivantsov commented Dec 23, 2022

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

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

Comments

benjie commented Dec 6, 2022 • edited Loading

mjmahone commented Dec 15, 2022 • edited Loading

benjie commented Dec 15, 2022

benjie commented Dec 15, 2022

benjie commented Dec 15, 2022

benjie commented Dec 15, 2022

rivantsov commented Dec 23, 2022

benjie commented Dec 6, 2022 •

edited

Loading

mjmahone commented Dec 15, 2022 •

edited

Loading