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

I'd like to change how errors are handled #10

Closed
miracle2k opened this issue Apr 19, 2018 · 3 comments
Closed

I'd like to change how errors are handled #10

miracle2k opened this issue Apr 19, 2018 · 3 comments

Comments

@miracle2k
Copy link

I strongly dislike the fact that this library seems to catch any and all exceptions, and returns them as an error response.

If I have a bug in my resolver code, this should be a 500 error, be tracked to Sentry, and the error message should not be exposed to the user.

If the user makes a mistake in their query, this should be a 400 error, with a clear message, and should not go to Sentry.

As it stands, this library (and thus all the integrations) do not seem to let me do this. I just get an error response, and I'd have to reverse engineer which class of error it is.

@miracle2k
Copy link
Author

It seems what is really happening is this:

  • graphql-core itself already catches all errors.
  • graphql-server-core return the result from graphql-core as-is.
  • sanic_graphql then calls again into graphql-server-core, namely the encode_execution_results helper. This one essentially returns the data for an HTTP response (status code and so on). It passes through a format_error callback.

I guess what I want to do is override this format_error callback, and raise an exception there.

class MyGraphQLView(GraphQLView):

    @staticmethod
    def format_error(error):
        # graphene likes to wrap all of the exceptions, unwrap
        if isinstance(error, GraphQLLocatedError):
            error = error.original_error

        # Is this an error we want to pass through?
        if isinstance(error, GraphQLError):
            return graphql.error.format_error(error)
        else:
            # Unknown error, crash
            raise error

@miracle2k
Copy link
Author

I think this is a bit confusing to achieve what I would perceive to be the "default" approach, but I am not sure if there is something actionable here right now from my point of view.

@kaos
Copy link

kaos commented Nov 12, 2020

Most Excellent. This ought to be in the docs.

However, that seems to apply to the now legacy version https://github.com/graphql-python/graphql-core-legacy as I don't find the GraphQLLocatedError in the new core, which seems to loose the original exception type, as they only use the str(e.message) part of it!?

Ah, they've double wrapped it, in some cases.
graphql-python/graphql-core#106

EDIT:

Here's my take on it for graphql-core v3:

from graphql_server.flask import GraphQLView
from graphql import GraphQLError


class MyGraphQLView (GraphQLView):
    '''Credit: [@miracle2k Michael Elsdörfer](https://github.com/miracle2k)
    [graphql-python/graphql-server#10](https://github.com/graphql-python/graphql-server/issues/10#issuecomment-382809784)

    Adapted by @kaos
    '''
    @staticmethod
    def format_error(error):
        root_cause = error
        while isinstance(root_cause, GraphQLError) and root_cause.original_error:
            # work-around for https://github.com/graphql-python/graphql-core/issues/106
            root_cause = root_cause.original_error

        if isinstance(root_cause, GraphQLError):
            return GraphQLView.format_error(error)

        # treat non graphql errors as internal server errors
        raise error from root_cause

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