Option to not catch exceptions

[reverie]

Please add an option to not catch all exceptions. It makes things needlessly difficult to debug, and contravenes the Zen of Python.

[KaySackey]

It would also be sufficient to document that this is occuring, because workarounds exist:

graphql-python/graphql-core#41

But you did already mention that here:

graphql-python/graphql-core#238

So I +1 you'd there since it seems like a better solution to me. I'm not exactly sure why the core is opinionated about catching all exceptions, but I imagine there must be a good reason even if it was a historical one that no longer applies.

[Cito]

Can you add some code to demonstrate the exact problem? I would like to check if the same problem exists in graphql-core/modern branch or graphql-core-next or if this is specific to graphql-core.

[reverie]

@Cito Ok, I made a demo project

https://github.com/reverie/graphq_core_demo

Where is this error happening?

[Cito]

@reverie Thanks. But you are using Graphene and Graphene-Django in this demo. The error catching could also happen at these levels, not down at graphql-core. We need an example that uses only graphql-core.

Also, your example shows that the error itself is caught. What you seem to be missing is the traceback.

[Cito]

So I took your example down to the graphql-core level:

from graphql import (
  GraphQLObjectType, GraphQLField, GraphQLString, GraphQLSchema, graphql)


def a():
  return b()

def b():
  return c()

def c():
  return d()

def d():
  return a + 3


def make_foo(*_args):
  return a()


schema = GraphQLSchema(
  query= GraphQLObjectType(
    name='RootQueryType',
    fields={
      'foo': GraphQLField(
        GraphQLString,
        resolver=make_foo
      )
    }
  )
)

result = graphql(schema, '{foo}')

raise result.errors[0]

What I observe is that the error is printed even twice, with the correct traceback showing funcion d() as causing the error . The traceback is printed twice because the error is raised and graphql-core also logs it to the standard output. I also checked with graphql-core-next, which does not log but also returns the proper traceback attached to the error.

So actually graphql-core isn't suppressing any error information. The issue is probably on the level of graphene-django. I would not expect it to return the traceback in the response, revealing that much server side info to any client would be a security issue. Showing just the error message here is the right thing. But it should show the traceback in the server log.

[Cito]

Ok, now I really got to the bottom of it. The above code uses a synchronous resolver. Let's change this to return a promise, like this:

def make_foo(*_args):
  return Promise(lambda resolve, reject: resolve(a()))

Now suddenly the traceback is not printed any more. And this is exactly what happens in graphene-django, because the django debug middleware returns promises.

In the case of promises, the error is not raised directly and we need the stack attribute of the error to see the traceback, which is not set properly in GraphQLLocatedError. This line is the problem:

    stack = original_error and getattr(original_error, "stack", None)

Normal Python errors like the one raised by the resolver do not have a stack attribute. We must use the __traceback__ attribute instead:

    stack = original_error and (
        getattr(original_error, "stack", None) or
        getattr(original_error, '__traceback__', None))

After modifying GraphQLLocatedError like this, the error traceback is now shown properly in the Django server log.

There's just one caveat here: The __traceback__ attribute is only available in Python 3. It would be much more difficult if not impossible to implement in Python 2. That's sad because graphql-core is supposed to work with Python 2 as well, but we probably need to accept it as a known bug.

[Cito]

@reverie can you check if #240 solves your problem? After applying the patch, you should see the location of the error in the console where you start your Django server.

[reverie]

@Cito Thanks for all your work on this! I tried the patch in #240 and it's a big improvement. I think it would help a lot of people if it were merged. That change does not directly solve this issue (#237), though: I don't want graphql-core to catch the exception in the first place. (Why does it do that?)

[Cito]

I don't want graphql-core to catch the exception in the first place. (Why does it do that?)

Because we don't want to halt the whole GraphQL server just because one user sent a failing query.

[reverie]

I don't want graphql-core to catch the exception in the first place. (Why does it do that?)

Because we don't want to halt the whole GraphQL server just because one user sent a failing query.

That makes sense! From Django's perspective graphql-core is just a library, though, and it would be helpful to support that perspective as well.

[Cito]

The idea of catching the errors and returning them as a list is pretty much baked in GraphQL per the specs (see here for example). That's why it is implemented that way in GraphQL.js which graphql-core replicates.

Note that it is possible to re-raise the errors that are returned by GraphQL with the same stacktrace if you really want that.

[KaySackey]

How about two modes. During development let errors optionally break out of graphene. This will allow normal usage of the debugger. At the moment, I have to place a debug break point inside the graphene codebase in order to get a live stack on exceptions.

The other option would be to log errors via standard python logging module, before returning the error list to the user. This would work seamlessly with any framework like Django. And it also lets end users hook up custom logging like Sentry, without having to special case graphene.

[Cito]

The logging happens already through the logging module. What you see on the console are actually log messages that you can send anywhere you want, configurable via the Django configuration.

If you really want to re-raise errors, then you need to do this on the level of Graphene-Django, because anything that Graphene throws is caught by Graphene-Django anway and wrapped into a ExecutionResult in execute_graphql_request(). One idea would be to use a custom GraphQLView that has a format_error method that throws or does some extra logging or filtering of error messages. Every error message is seen by that method.

[reverie]

@Cito That makes sense. Thank you for your help with this!