Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong error code thrown for GraphQL validation failures in arguments using query variables #3498

Closed
jcfi opened this issue Nov 11, 2019 · 12 comments · Fixed by #5091
Closed
Labels
🍐 error-handling Pertaining to error handling (or lack thereof), not just for just general errors/bugs.

Comments

@jcfi
Copy link

jcfi commented Nov 11, 2019

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
Copy link
Contributor

n1ru4l commented Nov 18, 2019

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
Copy link

Tydax commented Jan 22, 2020

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
Copy link

fiznool commented Apr 2, 2020

Also related to #3304.

@jeromemeichelbeck
Copy link

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
Copy link

mshwery commented Jun 20, 2020

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.

@abernix abernix added the 🍐 error-handling Pertaining to error handling (or lack thereof), not just for just general errors/bugs. label Dec 31, 2020
@EllaSharakanski
Copy link

Are there any news on this issue?

@glasser
Copy link
Member

glasser commented Apr 6, 2021

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 added a commit that referenced this issue Apr 7, 2021
This particular error can be trivially triggered by clients, so it doesn't make
sense to use `INTERNAL_SERVER_ERROR` for it. It seems like a good fit for
`BAD_USER_INPUT`, which previously was only used if you explicitly throw a
`UserInputError` in your app.

Fixes #3498.
@glasser
Copy link
Member

glasser commented Apr 7, 2021

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 added a commit that referenced this issue Apr 8, 2021
…5091)

This particular error can be trivially triggered by clients, so it doesn't make
sense to use `INTERNAL_SERVER_ERROR` for it. It seems like a good fit for
`BAD_USER_INPUT`, which previously was only used if you explicitly throw a
`UserInputError` in your app.

Fixes #3498.
@glasser
Copy link
Member

glasser commented Apr 9, 2021

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
Copy link
Member

glasser commented Apr 14, 2021

This is released in Apollo Server 2.23.0.

@iBlueDust
Copy link

Same thing happens when you feed null to a required variable.

Example

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

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

Variables:

{} /* or */ { "var": null }

Response:

{
  "error": {
    "errors": [
      {
        "message": "Variable \"$var\" of non-null type \"String!\" must not be null.",
        "locations": [
          {
            "line": 1,
            "column": 8
          }
        ],
        "extensions": {
          "code": "INTERNAL_SERVER_ERROR",
          "exception": {
            "stacktrace": [ /* ... */ ]
          }
        }
      }
    ]
  }
}

Expected BAD_USER_INPUT instead of INTERNAL_SERVER_ERROR

This is derived from OP's example since my own example has unnecessary implementation details that would only bloat this post.

@glasser
Copy link
Member

glasser commented Jun 22, 2021

@iBlueDust Thanks for the report and reproduction. I filed #5353 which would be a great first issue for anyone interested in contributing.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🍐 error-handling Pertaining to error handling (or lack thereof), not just for just general errors/bugs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants