RFC: Do error return values on resolvers make sense?

[neelance]

Error handling in GraphQL seems to not be clearly defined yet. I recently started thinking about how it relates to how we do error handling in Go. Go has the clear distinction between error values and panics. The scenario I'm looking at is GraphQL used to feed data to some UI, so the "user" is the person using that UI. Here is what I've come up with:

	expected	unexpected
in Go	returned error value	panic
root cause	operation failed in expected way	bug in the code
error payload	processed / handled	only read by dev, never processed
resolution	degrade gracefully or inform user	fix code
in GraphQL	null value or custom error field in schema	"errors" list

Given this I am wondering if returning error values from resolvers actually makes sense. If it is some unexpected error, then it should be a panic. If it is something expected like "not found" or "permission denied", then it would be best to express that in the schema itself, just like we have explicit error return values in Go.

The ask for custom errors has come up several times and by saying that any error with significant payload should go into the schema itself, we would completely avoid that situation. Also the errors would then have a properly declared type schema.

It is also noteworthy that by default Relay completely discards any response that has a non-empty errors list. So if you want to pass a non-fatal error to the user in a Relay UI, then you have to use a schema field.

What are you opinions on this? Am I completely nuts? Are there other scenarios in which error values on resolvers make more sense?

[neelance]

/cc @F21 @nicksrandall @acornejo @bsr203 @EmperorEarth

[bsr203]

I use relay at client side, and not sure how the change affect other clients like apollo. I am assuming by saying error as a schema filed, it means, error will be part of normal/success payload and it is users responsibility to detect it at client side. I just saw a good blog post about it. It make some sense to say expected error should be handled by the user, but some points to consider

The default behavior of relay is to look at "errors" to detect error, else it caches the values in the store. So, to inform the framework it was an error, user needs to implement custom network layer. The "DefaultNetworkLayer" is just for reference, as pointed out in this comment. It is not a big concern if you have a serious enough application as you may ended up extending "DefaultNetworkLayer" to detect auth failure and process custom errors, but all the use of this library with relay will be affected.

Does it make sense to set the custom error as data payload, but still use "errors" list to indicate an error. May not be easy to implement as I am not sure what kind of resolver to return in case of error.

How it affects users in general as the concept may be unique to this library.

Can we wait till graphql defines error behavior correctly. They say "however future versions of the spec may describe additional entries to errors.".

I currently json encode custom error and validation errors as part of msg field. It is not ideal, but should be ok for now.

[neelance]

Yeah, I'm not going to change this soon. I just wanted to start some discussion / gather some data.

[tonyghita]

Thanks for getting this discussion going @neelance!

If it is some unexpected error, then it should be a panic.

It seems only unexpected would be language-level errors made by the backend developer, like nil pointer dereferences. These are fair to panic on, but should not be propagated to the API consumer.

Ideally, the panic would be contained to the field where it occurred. The API response would be null for that field and there would be a corresponding entry in the errors array.

If it is something expected like "not found" or "permission denied", then it would be best to express that in the schema itself, just like we have explicit error return values in Go.

At the consumer's level, they should only care if the field which errored is retriable. That is, if the same request is made, there is a reasonable expectation that the error will not occur again (i.e. HTTP 5XX vs HTTP 4XX).

The ask for custom errors has come up several times and by saying that any error with significant payload should go into the schema itself, we would completely avoid that situation. Also the errors would then have a properly declared type schema.

This is an interesting, but I'm a little hesitant to go down this path. I think would increase the schema and response payload to a massive size, mostly occupied by error objects.

This could also double the client's work, as they'd have to check for errors on every object rather than iterate through the single array of errors.

It is also noteworthy that by default Relay completely discards any response that has a non-empty errors list. So if you want to pass a non-fatal error to the user in a Relay UI, then you have to use a schema field.

Yes that definitely makes the problem easier, but not everyone will use Relay.

I would like to be able to allow successfully resolved fields through to the response and give enough information to the client about failed fields so that they can retry for those fields when it makes sense.

Are there other scenarios in which error values on resolvers make more sense?

This concept is nice because it's very idiomatic to golang, so we are working with the language rather than fighting it.

Perhaps there is some way to define an extensible error interface that could satisfy everyone's needs around errors.

I think with the current solution that we are not far from my ideal solution, we are just missing a few concepts.

Side note on GraphQL errors:

I'm very excited about the PR to include the path field on errors in the spec (graphql/graphql-spec#230), as this makes the errors in the response error array incredibly useful. I think regardless of whether this spec gets merged or not, it would be a incredibly powerful functionality to include in this library.

[F21]

I haven't used relay with graphql on a production project yet. Most of my current work is using graphql for communication between microservices.

I found returning an error directly to be idiomatic and easy to use. The caller of the service would then see the error, log in and take appropriate action such as opening a circuit breaker. In this sense, returning an error allowed the implementation to be simple and generic. I've created a very simple and generic GraphQL client for my projects and it simply looks at the errors field to determine whether the request was successful and returns an error type otherwise.

Having said that, the requirements for communicating with the client and between different microservices are vastly different, so I can see some value in having custom error objects for the client-side. However, once (graphql/graphql-spec#230) gets into the spec, I think it will negate the need for custom error types for most use-cases.

[zmb3]

I rather enjoy the error return values. As Tony says, it feels very idiomatic.

I would also avoid panics unless absolutely necessary. In Go, an error is an error, expected or not. Panics should be reserved for truly exceptional cases or logic errors where there is really nothing the application can do to recover from.

[neelance]

Let's keep this as is for now.