graphql-dotnet: ExecutionError: Differentiate between client and server error.

Summary

As the author of a GraphQL.Net API, it would be very helpful to be able to differentiate between an ExecutionError where the Api-author screwed up as opposed to when the Api-client screwed up. Presently, one needs to hard-code checks to exception messages found in the GraphQL.Net source code, which is a brittle approach.

Basic example

Implement a FieldAsync<ListGraphType<...>> resolver and directly return your data from an async method; make the mistake of omitting the await keyword. ExecutionStrategy.SetArrayItemNodes() will raise an ExecutionError that says “User error: expected an IEnumerable list though did not find one.”

Motivation

The above error is obviously the Api-author’s fault, not the Api-client’s. As an Api-author, I’d like to have my Api clearly behave as if it’s my fault, not the client’s fault. I want to log that; I don’t want to log ExecutionErrors that are not my fault.

I feel like there are 2 options:

1. Use a non-ExecutionError

For Api-author errors, don’t use ExecutionError at all. Instead use a built-in Exception (like InvalidOperationException or ApplicationException). This way when the Api-author screws up, they handle it just like any other “unhandled” exception that comes out of GraphQL.Net.

Searching on ExecutionError in GraphQL.Net, there’s a number of other spots that raise “Api-author” level errors besides the above “Basic example”. E.g. “Only add root types.”, “Schema is not configured for mutations”, “A schema is required.”, “Must provide an enter or leave function.”, “Unhandled document definition”, etc.

2. Sub-class ExecutionError

If the ExecutionError was instead a new subclass that could be used to globally determine “It’s the Api-author’s fault”, then the GraphQL service has the opportunity to behave better.

But it feels like ExecutionError is not supposed to be used for things that aren’t problems caused by the Api-author. For example, there is already InvalidValueException, and that’s (appropriately) used when it’s an Api-client error (validation).

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 23 (13 by maintainers)

Most upvoted comments

@GrangerAts @Shane32 I carefully re-read this entire thread again. @GrangerAts I totally agree with you. Thanks to the latest PRs from @Shane32, I finally realized that the problem really exists. I see you closed #1658 so I close this issue as well in favor of linked PRs from @Shane32 in #1658. OK?

sungam3r on Aug 13, 2020

I’m not in favor of sub-classing, nor am I in favor of doing “outer/inner” exceptions that involve ExecutionError. What I want is GraphQL.Net to simply throw a non-ExecutionError exception when it’s the Api-author’s fault. It’s confusing to hide server-side errors in a client-side error object.

Here are some significant problems with putting server-side errors in a client-side object (the “code” approach):

GraphQL.Net already appropriately uses non-ExecutionError exceptions to indicate Api-author errors. (GraphQLTypeReference.cs, ExecutionStrategy.cs, and others use InvalidOperationException. GraphQLExtensions.cs and others use ArgumentException. HttpResponseStreamWriter.cs, DataLoaderContextExtensions.cs, ObjectGraphType.cs, and many more all use ArgumentNullException… the list goes on for 14 pages in a GitHub search.)
To be consistent, you’d have to change ALL of those to instead throw ExecutionError with a new “code”; that’s multiple breaking changes for everyone that uses GraphQL.Net.
Even if you did that, you still have .Net Core BCL functionality that throws regular exceptions. So then the official GraphQL.Net documentation is going to be something like this: Handle exceptions like normal, but if it’s an ExecutionError, go check the “code”; if that’s null/empty, then treat it like a normal exception, otherwise, you need to let it get serialized/emitted to the client.
Next, there’s the issue that there isn’t a GraphQL “working group” for standardization of error codes. The most I’ve found is this discussion/proposal/RFC that was opened less than 2 weeks ago. It’s presently just an idea; there’s no actionable proposal to discuss, evaluate, or submit. And, based on the current GraphQL spec’s revision cadence, it’s going to be multiple years before this even makes it into the next working-draft.

From your posture, it appears that devs using the GraphQL.Net library have no choice but to write a helper method in our code that will inspect the .Message of each ExecutionError object returned in ExecutionResult.Errors against a hard-coded list of strings we manually pulled from the GraphQL.Net source code. If they’re in the list, we change our server’s response to be the same as if there had been an un-handled exception during Gql execution. Again: very brittle. But on the bright side, when I search the code for "new ExecutionError", there’s only 2 pages of hits to consider. So now I’m off to write that hack.

GrangerAts on Apr 23, 2020