Design proper error names

[rogierschouten]

GraphQL-JS throws a variety of errors in normal everyday use e.g. GraphQLErrors, TypeErrors, and even regular JavaScript Error objects. All of these can occur because the client supplies garbage values to the server.

However, on the server side, I want to make a distinction between internal server errors (e.g. assertions, which should crash the server) and client errors (which should return a good client error message to the client).

To accomplish this, I tried to write a good formatError() function to give to express-graphql.

I cannot get this right, because e.g. graphql throws TypeErrors, which are indistinguishable from a server-side programming bug that causes a NodeJS TypeError.

So the proposal would be to ensure that graphql client errors are distinguishable from server errors, e.g. by giving all errors proper values for their 'name' and 'code' properties. This is also the direction in which NodeJS is going at the moment, see https://nodejs.org/api/errors.html#errors_error_code

[rogierschouten]

@IvanGoncharov is more exploration needed than simply replacing all throw Foo by a proper throw GraphQLError? What is the bottleneck, is there any thinking I can help you with?

[IvanGoncharov]

@rogierschouten Sorry I didn't read carefully enough during initial triage.
I saw you mentioned name and code property and assumed it was the main topic of this issue. So we definitely want to have some mechanism for distinguishing different types of errors (parsing, validation, execution, etc.) and we discussed it on GraphQL WG and current consensus is that we need to add it in the GraphQL specification first and also expose them in the formatError.

About the main topic of this issue on using a mixture of Error subclasses, it is definitely something that we should address and it doesn't require any spec changes.

is more exploration needed than simply replacing all throw Foo by a proper throw GraphQLError?

I still think we need to distinguish between development errors, graphql-js own invariants, and errors triggered by clients. So I need to do some research on what other big libraries that extensively use callbacks are using e.g. React.

Currently, I'm trying to finish two high priority issues for 14.5.0 release but this issue will be next in my queue so will try to resolve it next week.

[rogierschouten]

I still think we need to distinguish between development errors, graphql-js own invariants, and errors triggered by clients.

I agree, the most important thing is being able to distinguish between the different classes of errors. Thx!

[hassenc]

we discussed it on GraphQL WG and current consensus is that we need to add it in the GraphQL specification first and also expose them in the formatError.

I am also encountering the same problem and would love to see the differentiation you are describing.
@IvanGoncharov Is there an Open Issue on the graphql-spec page ?

[IvanGoncharov]

@hassenc I'm not sure, but it should be something inside notes of previous meetings:
https://github.com/graphql/graphql-wg/tree/master/notes

[jacob-israel-turner]

Has there been any progress on this item lately? I have the same use-case as @rogierschouten - I'm formatting errors and trying to decide what is acceptable to expose to the client, and what should be masked. I use Apollo, and it seems that all errors from Apollo are wrapped by ApolloError, which makes it easy to filter - but the GraphQL errors seem to be all over the place. For now I'm doing a combination of checking if the error is a GraphQL error, and doing some text matching on the error message. Definitely not a great solution. Is there anything I can do to help out here?

[5achinJani]

It would be great if graphql-js can at least add the error codes to the known errors like for input validations error with code BAD_USER_INPUT.
https://github.com/graphql/graphql-js/blob/main/src/execution/values.ts

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

then it would make it easy for packages like Apollo to just check for code and return UserInputError.
Currently in Apollo there's check for string Variable "$${varName}" of non-null type "${varTypeStr}" must not be null. replaced with vars to catch such errors :
apollographql/apollo-server#5091
apollographql/apollo-server#5353

[yaacovCR]

I think the library has improved since the issue was created, so that any remaining thrown errors that could be GraphQLErrors could be raised individually. In terms of an error class hierarchy, it probably should be based on a spec change, tracking until we have a spec PR in #3593