Some variable-related errors should be BAD_USER_INPUT rather than INTERNAL_SERVER_ERROR

[glasser]

The function coerceVariableValues in https://github.com/graphql/graphql-js/blob/main/src/execution/values.ts can throw several errors if the provided variable values aren't good. These are all bad user input, not server bugs, so Apollo Server should throw them as UserInputError rather than internal server errors. Unfortunately graphql-js doesn't give a great way of differentiating these errors from errors in server code and so we treat them like internal server errors.

In #5091 we fixed one of these errors (Variable "$${varName}" got invalid value) to throw UserInputError but other errors like Variable "$${varName}" of required type "${varTypeStr}" was not provided and Variable "$${varName}" of non-null type "${varTypeStr}" must not be null still show up as internal server errors. See #3498 (comment) for a codesandbox that demonstrates both issues. We should make them both into UserInputError.

[glasser]

#5091 shows how to fix issues like this with tests. PRs welcome! (Note that right now before 3.0.0 has been released, release-3.0 is the active development branch, not main.)

[5achinJani]

I can take this up.

Unfortunately graphql-js doesn't give a great way of differentiating these errors from errors in server code and so we treat them like internal server errors.

I couldn’t find any issue in graphql-js related to providing a way to differentiate bad input user validation errors from other errors. Wouldn’t it be great if graphql-js provides us a code in extensions like :

      onError(
        new GraphQLError(
          `Variable "$${varName}" of non-null type "${varTypeStr}" must not be null.`,
          varDefNode,
          undefined,
          undefined,
          undefined,
          undefined,
          {code:"BAD_USER_INPUT"}
        ),

(I confirmed it by making this change and checking e.extensions.code == "BAD_USER_INPUT" and returning UserInputError in requestPipeline.js file. it works )
This would save us the effort of matching all the possible validation related error strings from this file :
https://github.com/graphql/graphql-js/blob/main/src/execution/values.ts
like :

• Variable "$${varName}" expected value of type "${varTypeStr}" which cannot be used as an input type.,
• Variable "$${varName}" of required type "${varTypeStr}" was not provided.,
• Variable "$${varName}" of non-null type "${varTypeStr}" must not be null.
• Variable "$${varName}" got invalid value + inspect(invalidValue); - we've already handled this.

And I'm not sure if we also need to handle the errors coming from getArgumentValues cause they too look like bad user input errors :

• Argument "${name}" of required type "${inspect(argType)}" was not provided.
• Argument "${name}" of required type "${inspect(argType)}" was provided the variable "$${variableName}" which was not provided a runtime value.
• Argument "${name}" of non-null type "${inspect(argType)}" must not be null.

Let me know you inputs on this.
And if we want to go with the #5091 's way to handle this then would it be okay if we check for static error sentences in the error string like :
For error string : "Variable "$${varName}" of non-null type "${varTypeStr}" must not be null"
Code. :

if(e.message.includes(`Variable`) && e.message.includes(`of non-null type`) && e.message.includes(`must not be null`)

Reason for doing this instead of fetching the varName and varTypeStr and replacing it and then matching the string it includes a bit of effort with interpolation like for varTypeStr it would be:

            e.message.startsWith(
              `Variable "$${e.nodes[0].variable.name.value}" of non-null type "${nodes[0].type.type.name.value}!" must not be null`,
            )

[glasser]

I think for the long term it would be great if graphql-js had more machine-parseable errors (presumably adding something on extensions); if you open an issue over there, link it here!

In the short term we should probably do something like #5091 so we can make progress. The main reason I feel comfortable with #5091 is that it does have a test so we will learn if new graphql-js versions change the error messages. I do also like how we link together the node from the error to the error message...

I don't think we have to worry about the errors in getArgumentValues because those cases should all be caught either in the validation step (which compares an operation to the schema without paying attention to variable values) or in getVariableValues:

• required argument not provided should be caught by validation
• required argument provided a variable without a value either means the variable was declared nullable (in which case validation should fail because you can't use a nullable variable for a required argument) or it was declared non-nullable (in which case getVariableValues should have failed)
• argument of non-null type must not be null means either a null value is passed in the operation itself (validation failure) or we pulled a null value out of the variables map (same problem as previous bullet)

Note that right now, active development is being done on release-3.0 rather than main (it should be merged in the next week or two).

[5achinJani]

Hey @glasser I've linked a PR do let me know if there are any changes to be made in it. :)