POC: custom error handlers #49

nicksrandall · 2017-01-10T23:07:01Z

This proof of concept will likely need changes before it can be merged but I hope it can serve as a starting point for a conversation around how we can elegantly handle errors in this library.

This change gives users an optional hook to handle errors. Access to this hook opens up the door for several use cases:

Send more information rich errors to the client (related to Ability to return errors with extra fields #37) like sending error codes or other data related to error.
Single place to filter out "non-user-safe" errors from being sent to the client.
Integrate/send errors created in my resolves or graphql to a logging/monitoring service.

One benefit of this approach is that it is non-breaking. Users are able to "opt-in" when needed.

nicksrandall · 2017-01-10T23:10:26Z

@jargv What are you're thoughts on this?

neelance · 2017-01-10T23:18:16Z

Schema.Exec returns the list of errors in the Errors field of its Response value. There is the chance to modify them before returning them to the client. Maybe the QueryError struct should have an additional ResolverError error field that contains the original error so you can post-process.

For adding custom fields to the errors I'd suggest creating a new error type that has QueryError embedded and a new Response type that allows those custom errors to be encoded as JSON. This should only require a few lines of code and seems like the most lightweight solution to me.

What do you think?

nicksrandall · 2017-01-11T00:01:18Z

@neelance I've made that change. What do you think?

neelance · 2017-01-11T01:28:30Z

Yes, that's the direction I wanted to go. But I think we should only set ResolverError to errors actually returned by a resolver. Otherwise it can be nil. No need to always wrap an error.

neelance · 2017-01-11T01:30:56Z

So I wouldn't even change the signature of Errorf but simply set the ResolverError field in the single instance where we call the resolver.

nicksrandall · 2017-01-11T17:15:48Z

Ok, I made that change. What do you think? Thanks again for your help!

nicksrandall · 2017-01-11T17:19:16Z

internal/exec/exec.go

-		r.addError(errors.Errorf("%s", err))
+		queryError := errors.Errorf("%s", err)
+		queryError.ResolverError = err
+		r.addError(queryError)
 		addResult(f.Alias, nil) // TODO handle non-nil


This may be a discussion for another thread but shouldn't this be addResult(f.Alias, result) if the handler was able to resolve a partial amount of data, shouldn't we still send that to client and allow them to figure out what to do with it?

The spec says that null should be returned on error: http://facebook.github.io/graphql/#sec-Errors-and-Non-Nullability

Actually the null should even propagate to the parent if the current field has a non-null type. This is not yet implemented.

Interesting. Ok! Thanks!

neelance · 2017-01-11T17:33:59Z

internal/exec/exec.go

@@ -554,7 +557,9 @@ func (e *fieldExec) execField(ctx context.Context, r *request, f *query.Field, r
 	result, err := e.execField2(spanCtx, r, f, resolver, span)

 	if err != nil {
-		r.addError(errors.Errorf("%s", err))
+		queryError := errors.Errorf("%s", err)
+		queryError.ResolverError = err


Nice and simple. ❤️

neelance · 2017-01-11T17:37:59Z

internal/exec/exec.go

@@ -303,6 +303,9 @@ func (r *request) addError(err *errors.QueryError) {
 func (r *request) handlePanic() {
 	if err := recover(); err != nil {
 		execErr := errors.Errorf("graphql: panic occured: %v", err)
+		if err, ok := err.(error); ok {
+			execErr.ResolverError = err


Hmm, is there a good scenario on why we should set the field on a panic? A panic should always be an unexpected internal error, so there should be nothing to tell the user except "internal error".

Right now the error message includes the panic's error value, but eventually I want to change it to only say "internal error" and not leak internals. However right now I don't want to do that yet because there are still some graphql-go error conditions that cause a panic and they would be hard to understand if the message was omitted.

My thought here is that this could help your use case of not leaking internals. The error message could say something like "internal error" but more information (not available to client) could be included on the ResolverError (maybe thats a bad name) which won't get sent to client because it wont be serialized to json.

Also, if something panics in a resolver, won't it eventually be caught by this func? I don't have a use case for this yet but I could see myself wanting to have access to the originating error?

I don't feel very strongly about this so if you'd like me to remove it then I will.

If you want access to the originating error to extract information from it, then the error is not unexpected and thus it does not qualify for a panic, right? ;-)

Thats true.

nicksrandall · 2017-01-11T20:37:17Z

I removed changes from the panic Handler. Are we good to merge now?

neelance · 2017-01-11T21:41:06Z

Yes, we are!

neelance · 2017-01-11T21:41:49Z

@nicksrandall Thanks for contributing!

F21 · 2017-05-02T00:40:33Z

Is there an example for using this?

felixfbecker · 2018-04-10T01:43:35Z

Is there an example of how to use this?

blasterpistol · 2018-04-10T06:26:19Z

we should write some docs 📝

felixfbecker · 2018-04-20T02:38:48Z

@DenisNeustroev in lieu of docs, do you have a minimal example? 🙇

jpascal · 2018-05-05T09:42:21Z

@DenisNeustroev I explored all code and I can't find way to provide custom errors to client. 😕

sanae10001 · 2018-05-07T02:41:46Z

@jpascal What about this?
https://gist.github.com/sanae10001/9ad4c2ebfe640751f7e3278f5c6eb02a

nicksrandall force-pushed the custom-error-handler branch from ec96144 to 6270227 Compare January 10, 2017 23:51

nicksrandall force-pushed the custom-error-handler branch 2 times, most recently from 3dd6c84 to 24b2045 Compare January 11, 2017 17:14

nicksrandall commented Jan 11, 2017

View reviewed changes

neelance reviewed Jan 11, 2017

View reviewed changes

Add ResolverError field to QueryError for post processing

ae8518b

nicksrandall force-pushed the custom-error-handler branch from 24b2045 to ae8518b Compare January 11, 2017 19:50

neelance merged commit eccedde into graph-gophers:master Jan 11, 2017

neelance mentioned this pull request Jan 11, 2017

Ability to return errors with extra fields #37

Closed

neelance mentioned this pull request Feb 1, 2017

return structured errors #52

Closed

neelance mentioned this pull request Mar 10, 2017

More info on errors #60

Closed

F21 mentioned this pull request May 11, 2017

How do I use custom error types? #85

Closed

gracenoah mentioned this pull request May 11, 2018

Add support for custom error handlers #207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POC: custom error handlers #49

POC: custom error handlers #49

nicksrandall commented Jan 10, 2017

nicksrandall commented Jan 10, 2017

neelance commented Jan 10, 2017

nicksrandall commented Jan 11, 2017

neelance commented Jan 11, 2017

neelance commented Jan 11, 2017

nicksrandall commented Jan 11, 2017

nicksrandall Jan 11, 2017

neelance Jan 11, 2017

nicksrandall Jan 11, 2017

neelance Jan 11, 2017

neelance Jan 11, 2017

nicksrandall Jan 11, 2017

neelance Jan 11, 2017

nicksrandall Jan 11, 2017

nicksrandall Jan 11, 2017

nicksrandall commented Jan 11, 2017

neelance commented Jan 11, 2017

neelance commented Jan 11, 2017

F21 commented May 2, 2017 •

edited

Loading

felixfbecker commented Apr 10, 2018

blasterpistol commented Apr 10, 2018

felixfbecker commented Apr 20, 2018

jpascal commented May 5, 2018 •

edited

Loading

sanae10001 commented May 7, 2018

POC: custom error handlers #49

POC: custom error handlers #49

Conversation

nicksrandall commented Jan 10, 2017

nicksrandall commented Jan 10, 2017

neelance commented Jan 10, 2017

nicksrandall commented Jan 11, 2017

neelance commented Jan 11, 2017

neelance commented Jan 11, 2017

nicksrandall commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicksrandall commented Jan 11, 2017

neelance commented Jan 11, 2017

neelance commented Jan 11, 2017

F21 commented May 2, 2017 • edited Loading

felixfbecker commented Apr 10, 2018

blasterpistol commented Apr 10, 2018

felixfbecker commented Apr 20, 2018

jpascal commented May 5, 2018 • edited Loading

sanae10001 commented May 7, 2018

F21 commented May 2, 2017 •

edited

Loading

jpascal commented May 5, 2018 •

edited

Loading