Apollo-server: Wrong error code thrown for GraphQL validation failures in arguments using query variables

[jcfi]

Package Name/Version

Name: apollo-server

Version: 2.x.x. All versions from 2.9.7 through 2.0.0 have this bug.

Expected Behavior

Queries that fail due to errors in scalar parsing should return the error code GRAPHQL_VALIDATION_FAILED.

Actual Behavior

If an argument passed as a non-null query variable fails to validate, the server response has the code INTERNAL_SERVER_ERROR. This issue occurs with both built-in scalars and custom scalar types.

Reproduction

Sandbox Link

https://codesandbox.io/s/apollo-error-code-bug-ho6vr

Instructions

In the query field of the sandbox, enter

query($var:String!) {
  hello(name:$var)
}

And in the query variables section, enter

{
  "var": 2
}

The query above returns this response:

{
  "error": {
    "errors": [
      {
        "message": "Variable \"$var\" got invalid value 2; Expected type String. String cannot represent a non string value: 2",
        "locations": [
          {
            "line": 1,
            "column": 8
          }
        ],
        "extensions": {
          "code": "INTERNAL_SERVER_ERROR",
          "exception": {
            "stacktrace": [
              "TypeError: String cannot represent a non string value: 2",
              "    at GraphQLScalarType.coerceString [as parseValue] (/sandbox/node_modules/graphql/type/scalars.js:164:11)",
              "    at coerceInputValueImpl (/sandbox/node_modules/graphql/utilities/coerceInputValue.js:127:26)",
              "    at coerceInputValue (/sandbox/node_modules/graphql/utilities/coerceInputValue.js:37:10)",
              "    at _loop (/sandbox/node_modules/graphql/execution/values.js:107:69)",
              "    at coerceVariableValues (/sandbox/node_modules/graphql/execution/values.js:119:16)",
              "    at getVariableValues (/sandbox/node_modules/graphql/execution/values.js:48:19)",
              "    at buildExecutionContext (/sandbox/node_modules/graphql/execution/execute.js:184:61)",
              "    at executeImpl (/sandbox/node_modules/graphql/execution/execute.js:89:20)",
              "    at Object.execute (/sandbox/node_modules/graphql/execution/execute.js:64:35)",
              "    at /sandbox/node_modules/apollo-server-core/dist/requestPipeline.js:240:46"
            ]
          }
        }
      }
    ]
  }
}

Type errors in arguments sent without using query variables, on the other hand, give a more accurate error code. Compare the above to the result for this query:

query {
  hello(name:2)
}

It gives the response

{
  "error": {
    "errors": [
      {
        "message": "Expected type String, found 2.",
        "locations": [
          {
            "line": 2,
            "column": 15
          }
        ],
        "extensions": {
          "code": "GRAPHQL_VALIDATION_FAILED",
          "exception": {
            "stacktrace": [
              "GraphQLError: Expected type String, found 2.",
              "    at isValidScalar (/sandbox/node_modules/graphql/validation/rules/ValuesOfCorrectType.js:159:27)",
              "    at Object.IntValue (/sandbox/node_modules/graphql/validation/rules/ValuesOfCorrectType.js:116:14)",
              "    at Object.enter (/sandbox/node_modules/graphql/language/visitor.js:324:29)",
              "    at Object.enter (/sandbox/node_modules/graphql/language/visitor.js:375:25)",
              "    at visit (/sandbox/node_modules/graphql/language/visitor.js:242:26)",
              "    at Object.validate (/sandbox/node_modules/graphql/validation/validate.js:73:24)",
              "    at validate (/sandbox/node_modules/apollo-server-core/dist/requestPipeline.js:212:32)",
              "    at Object.<anonymous> (/sandbox/node_modules/apollo-server-core/dist/requestPipeline.js:125:42)",
              "    at Generator.next (<anonymous>)",
              "    at fulfilled (/sandbox/node_modules/apollo-server-core/dist/requestPipeline.js:5:58)"
            ]
          }
        }
      }
    ]
  }
}

which contains a much more useful error code for the client.

A similar problem was noted in several previous issues (#3361 #1410 #2204), but those issues were all linked to mutations, instead of queries.

[n1ru4l]

The hacky workaround would be to fix it inside formatError:

 if (
  error.extensions &&
   (error.message.startsWith(`Variable "`) ||
     error.extensions.code === "GRAPHQL_VALIDATION_FAILED")
 ) {
  error.extensions.code = "GRAPHQL_VALIDATION_FAILED";
  return error;
}

[Tydax]

Unfortunately, Apollo failing to validate the query variables results in a TypeError being thrown, which are also thrown if attempting to read a property from undefined .

I wish we could rely on something else than the message to check for the error type, as I personally believe that the former should be passed down to the client while the latter should be wrapped as an internal system error.

[fiznool]

Also related to #3304.

[jeromemeichelbeck]

I came across the same issue.
I already use a custom formatError() so I will fix it as n1ru4l suggested, but it's not ideal...

[mshwery]

Ah, we are running into this same issue after upgrading both graphql (which now has stricter coercion / validation) and upgrading apollo-server.

The big issue here is that there is no reliable way to differentiate graphql validation errors from actual internal errors that are unexpected. GraphQL validation errors are essentially user-error... unactionable, whereas other types of internal errors are unexpected and potentially actionable.

Agree that TypeErrors thrown during document validation should be categorized as such.

[EllaSharakanski]

Are there any news on this issue?

[glasser]

You're right: INTERNAL_SERVER_ERROR shouldn't be something that should show up due to normal user input.

I don't think GRAPHQL_VALIDATION_FAILED is correct here. "Validation" is a specific stage in GraphQL processing that compares an operation document to a schema. It's a stage that's completely cacheable for a given document/schema pair and does not depend on other inputs like variables.

I think we should catch errors thrown by execution and if they are this particular type, give them an appropriate non-internal error code.

[glasser]

Fixing this in #5091. I've chosen BAD_USER_INPUT as the error code. This error code already existed in Apollo Server but was never thrown by AS itself (rather, as a user you can choose to throw UserInputError in your resolvers).

[glasser]

I've released a prerelease with this fix, version 2.23.0-alpha.0. Try out the alpha and see if it works for you! Please provide any feedback in #5094.

[glasser]

This is released in Apollo Server 2.23.0.