eclipse / microprofile-graphql

microprofile-graphql
Apache License 2.0
100 stars 40 forks source link

Improve error handling #227

Closed t1 closed 4 years ago

t1 commented 4 years ago
  1. When a request is syntactically correct, but not semantically, a Client Error has to be raised from within the application; e.g. when a requested id can't be found. The spec is not clear about this.
  2. Technical wrapper exceptions like EJBException should be unwrapped.
  3. Add @GraphQlWhitelist to add the annotated exception to the whitelist. And for the symmetry's sake: @GraphQlBlacklist for the blacklist.
  4. Document the GraphQLException.ExceptionType in the spec as well as the intended semantic of the values in the enum. Add more business related exception types: NotFound, Forbidden, Invalid.
  5. Add an exception mapper mechanism, so the application can take full control.
  6. RFC-7807 is a very well designed standard for RESTful services. Maybe we can evolve the error handling by learning from there: a. Define a type extension field to contain a stable URI for the type of error, so clients can safely react on specific error types. b. Define an instance extension field to contain a URI (e.g. a UUID-URN) that can be found in the logs. c. If the message field is more like the title, a detail extension field could be added; or the other way around. d. If these fields work out well, we could even upstream them to the GraphQL standard, so they become non-extension fields.

Maybe some of these points should go into a separate issue, but the discussion could start here ;-)

phillip-kruger commented 4 years ago

Hi @t1 - I like this. The @GraphQlWhitelist and @GraphQlBlacklist is a good idea. We keep the config options as well for exceptions out of control of the developer. For wrapped exceptions (like EJBException) - how would we know what exceptions to unwrap ? Config ?

t1 commented 4 years ago

Everything should also be configurable, but I think the implementations should be smart enough to know about the most common use cases. Maybe it's also possible to look at all the causes and if they are whitelisted, they should be unwrapped?

phillip-kruger commented 4 years ago

@t1 also look at #172

t1 commented 4 years ago

I had seen #172... that's why I didn't add another point to add better support for partial results. This is even more important for the Client.

phillip-kruger commented 4 years ago

Cool ! :)

phillip-kruger commented 4 years ago

Can we break this up into smaller issues ?

t1 commented 4 years ago

Yeah, I will break it up. Actually I didn't expect that there was so litte general need for discussion ;-)

t1 commented 4 years ago

Split up into separate issues