graphql-core-legacy: Why sys.excepthook for all (even "control flow" errors) ?

Hi! I’ve got some testcases covering invalid mutations e.g mutating a non-existent object with an invalid UUIDv4. GraphQL and graphene handle these quite nicely (user gets a proper error message).

However, my stderr is still full of errors when I run my tests (that all pass). I did some digging with the debugger and found this line: https://github.com/graphql-python/graphql-core/blob/f8a6a8a27119f924c2fce863cca87c4219d8a12b/graphql/execution/base.py#L85

Why do we want to print everything to stderr?

manage.py test output

python manage.py test --settings=config.test_settings
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
..Traceback (most recent call last):
  File "/Users/elnygren/Work/sumpli/sumpli-core/ENV/lib/python3.6/site-packages/graphql/execution/executor.py", line 215, in complete_value_catching_error
    exe_context, return_type, field_asts, info, result)
  File "/Users/elnygren/Work/sumpli/sumpli-core/ENV/lib/python3.6/site-packages/graphql/execution/executor.py", line 268, in complete_value
    raise GraphQLLocatedError(field_asts, original_error=result)
graphql.error.located_error.GraphQLLocatedError: No Business matches the given query.
.Traceback (most recent call last):
  File "/Users/elnygren/Work/sumpli/sumpli-core/ENV/lib/python3.6/site-packages/graphql/execution/executor.py", line 215, in complete_value_catching_error
    exe_context, return_type, field_asts, info, result)
  File "/Users/elnygren/Work/sumpli/sumpli-core/ENV/lib/python3.6/site-packages/graphql/execution/executor.py", line 268, in complete_value
    raise GraphQLLocatedError(field_asts, original_error=result)
graphql.error.located_error.GraphQLLocatedError: 'foo-bar-xyz-abc' is not a valid UUID.
.....................................

Note that:

No Business matches the given query.

and

‘foo-bar-xyz-abc’ is not a valid UUID.

are the messages shown to the users.

Code

class LikeABusiness(graphene.Mutation):
    """Mutation for liking/following a Business."""
    class Arguments:
        """Arguments of the Mutation"""
        uuid = graphene.String(required=True)

    # output
    ok = graphene.Boolean()

    def mutate(self, info, uuid, *args, **kwargs):
        """Perform the data mutation."""
        try:
            # raises the "No Business matches the given query." message due to 404
            return LikeABusiness(
                ok=get_object_or_404(Business, uuid=uuid).like(info.context.user)
            )
        except ValidationError as e:
            # passes along the "'foo-bar-xyz-abc' is not a valid UUID." message
            raise Exception(e.messages[0])

About this issue

Original URL
State: closed
Created 7 years ago
Reactions: 11
Comments: 19 (5 by maintainers)

Commits related to this issue

Replaced sys.excepthook with logging.error Closes https://github.com/graphql-python/graphql-core/issues/142 — committed to CaselIT/graphql-core by CaselIT 7 years ago

Most upvoted comments

BTW, I would like to share my recent experience handling errors with graphq-java which I believe is the reference in the JVM world.

Error vs Exception

When using this library, I always try to separate what is an error of what is an exception. The framework itself sometimes mixes errors and exceptions, but I think there’s a simple rule that could be used from the developer point of view that could help when deciding what error policy to apply.

An error is just a data structure carrying data used to rendered as an error as a result of a given business logic. An error it’s not returned as result but added to the current execution context errors. That enables all use cases, even the rare use case of having a result with data and errors.
Whereas an exception is a non expected behavior that happens to be rendered as an error with the extra information of the exception’s stacktrace. When an exception is thrown, the current execution is interrupted and, by default you can see the stacktrace in the logs, because is a non expected behavior. The appealing part of exceptions is that you may think of exceptions as a way of returning an error every time you don’t want to return a result. And that’s not the best way of building your business logic. At least in the Java world an stacktrace has a given execution cost that a simple data structure doesn’t have. That’s why errors as data are better than throwing exceptions.

If you would like to handle certain situations and send an error back you should be sending an error which is an expected result of a certain business logic.

Error extensions

An error has pretty much the same structure of an exception

message
location
path

But it also has an extensions attribute that could be used to add extra information to the current error.

I hope this helps Mario

mariogarcia on Mar 18, 2018

Thans for everyone involved in the fix! Indeed, using logging is much better than sys.excepthook

@tlinhart as you can see from the PR, the logger is created as logger = logging.getLogger(__name__). The __name__ is relevant for deciding whether the logged entry will show up in your final errors logs (and/or console stdout/stderr). To learn how this is done, refer to any python logging module tutorial.

Essentially you can tell that a logger of certain level has a certain log level, format, output (eg. file or stdout) etc.

elnygren on Aug 26, 2018

@tlinhart it’s now open

CaselIT on Mar 17, 2018

Well in my case I think is still desirable to have a minimum traceability of authentication errors.

But yeah, I think you’re right, maybe there should be a specific type of error that graphql-core could propagate without being verbose along the way. Or maybe a flag in the GraphQLError type:

Special type:

Promise.reject(GraphQLUserError('message'))

or flag:

Promise.reject(GraphQLError(message='message', log=False))

mariogarcia on Dec 5, 2017