Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom Exception Handler #317

Closed
namnm opened this issue Oct 31, 2018 · 12 comments
Closed

Custom Exception Handler #317

namnm opened this issue Oct 31, 2018 · 12 comments
Assignees
Milestone

Comments

@namnm
Copy link

namnm commented Oct 31, 2018

Is your feature request related to a problem? Please describe.
Thanks for the great package. Now I want to response the error based on some custom exceptions I threw.

Describe the solution you'd like
It should expose the default spec-compliant formatError function as: https://github.com/graphql/graphql-js/blob/master/src/error/formatError.js
Then it should allow to pass a custom formatError function to handle the error in the schema constructor.

@michaelstaib
Copy link
Member

So, what do you want to achieve? Do you want to have a function that gets called whenever there is an exception and be able to format it? Or, do you want to be able to ensure that implementations of IQueryError are formatted correctly. In general the GraphQLError and GraphQLFormatted error do separate the concrete error from the serialization format.

@michaelstaib michaelstaib added the ❓ question This issue is a question about feature of Hot Chocolate. label Oct 31, 2018
@michaelstaib
Copy link
Member

Sorry did not read your comment thoroughly enough. So, you basically want to be able to catch errors and reformat them. I think that is a good idea. Since I am rendering custom exceptions useless at the moment because I did not want to expose any compromising information of the server.

Let me have a look into how we can do that.

@michaelstaib michaelstaib added enhancement 🔍 investigate Indicates that an issue or pull request needs more information. and removed ❓ question This issue is a question about feature of Hot Chocolate. labels Oct 31, 2018
@michaelstaib michaelstaib added this to the 0.6.0 milestone Oct 31, 2018
@michaelstaib michaelstaib changed the title Feature Request: Custom error handler Custom Exception Handler Oct 31, 2018
@michaelstaib
Copy link
Member

Ok, I have looked at the implementation and we will include it with the next release. It might not be the next preview build so.

On the schema you will be able to add exception formatter like the following:

Schema.Create(c => 
{
    // handle all exception that are not explicitly handled.
    c.FormatException(exceptionContext => new QueryError("foo"));
   
    // handle specific exceptions, this will override the default handler.
    c.FormatException<FooException>(exceptionContext => new QueryError("foo"));
});

I will also create an interface IExceptionHandler that can be implemented and registered like the following as an alternative:

Schema.Create(c => 
{
    // handle all exception that are not explicitly handled.
    c.FormatException<MyHandler>();
   
    // handle specific exceptions, this will override the default handler.
    c.FormatException<FooException, MyHandler>();
});

The exception context will carry the thrown exception and also context data that was available when the exception was thrown like the field, object etc....

Would that meet your needs?

@michaelstaib michaelstaib removed the 🔍 investigate Indicates that an issue or pull request needs more information. label Oct 31, 2018
@michaelstaib michaelstaib self-assigned this Oct 31, 2018
@namnm
Copy link
Author

namnm commented Nov 1, 2018

Yes, your idea has basically covered the case. And I think we need to expose the default error handler too.

public class MyExceptionHandler
{
    public QueryError HandleException(ExceptionContext ctx)
    {
        // We need to expose the default exception handler function as a static helper or something
        // So we can get the default QueryError and modify it instead of manually craft a new one
        var res = DefaultExceptionHandler.HandleException(ctx);
        // In this default QueryError it already has the context info such as path, line number...
        // We only want to modify the error message though
        // TODO modify res here based on the exception
        //
        return res;
    }
}

@michaelstaib
Copy link
Member

OK, I will add that.

@michaelstaib
Copy link
Member

#319

@michaelstaib
Copy link
Member

michaelstaib commented Nov 27, 2018

Hi,

the exception filter will now look like the following:

public interface IErrorFilter
{
        IError OnError(IError error, Exception exception);
}

The exception formatter will not registered with the schema but with the dependency injection....
We are currently separating non schema relevant code from the schema .... execution services now are just registered with the solution....

You are able to add multiple exception filters which are called in the order of their registration.

so basically:

services.AddSingleton< IErrorFilter, MyFilter>()

Important: The exception can be null.

@michaelstaib
Copy link
Member

The exception context is now not any more needed since each filter gets the exception and the error from the former filter .... so each handler could also add more details or remove details.....

@michaelstaib
Copy link
Member

  • Finalize implementation
  • Tests

@michaelstaib
Copy link
Member

michaelstaib commented Dec 20, 2018

Where can errors happen?

Basically errors can be provided as QueryException or as IError.

An error or exception can occur in the execution pipeline itself which could be handled in the ExceptionMiddleware.

Moreover, exceptions could occur when execution a field resolver pipeline. Also, the result of the field resolver pipeline could be an IError.

  • Check if the field resolver exception are already serialized into an IError
  • Check how the validation errors are handled

Ideally we can grab errors in a maximum of two or three places.

This was referenced Dec 21, 2018
@michaelstaib
Copy link
Member

#420

@michaelstaib
Copy link
Member

We have completed this one and will include it in 0.7.0-preview.19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants