graphql-python / graphene

GraphQL framework for Python
http://graphene-python.org/
MIT License
8.07k stars 822 forks source link

Question: Document how to raise exceptions to get an "errors" list in response #1469

Closed khink closed 1 year ago

khink commented 1 year ago

First off, this is a question. I apologize for creating a ticket here. As suggested in the "bug report" issue template, i asked on StackOverflow first, but unfortunately there are no responses yet. I realize this does not give me the right to create an issue . I'm hoping my question might benefit the documentation in some way.

I don't know how to raise an exception in such a way that it results in an "errors" list in the response data.

In https://github.com/graphql-python/graphene/issues/1410#issuecomment-1085823075, @erikwrede wrote that we can just raise a GraphQLError. I don't think that's documented yet. All i found about GraphQLError usage is with regard to validators: https://docs.graphene-python.org/en/latest/execution/queryvalidation/#implementing-custom-validators There's nothing there that says that the response will have an "errors" list.

The reason is ask is that this used to work for us with older Graphene versions (graphene==3.0, graphene-django==3.0.0b7, graphql-core==3.1.7) but stopped working after we upgrade (graphene==3.1.1, graphene-django==3.0.0 and graphql-core==3.2.3).

This used to work:

class DeleteOrderlineMutation(graphene.Mutation):
    Output = graphene.ID()

    class Arguments:
        id = graphene.UUID()
        order_id = graphene.ID()

    @classmethod
    def mutate(cls, root, info, id, order_id):
        user = info.context.user
        order = Order.objects.for_user(user).get(pk=order_id)
        if not order.is_editable_by(user):
            raise GraphQLError(
                "Order mutation not allowed, Orderline can not be deleted."
            )

It would result in a response like this:

{
  ...
  "errors": [
      {"message": "Order mutation not allowed, Orderline can not be deleted.", ...}
  ]
}

However, after upgrade this gives

{
  'data': {
    'deleteOrderline': "<Promise at 0x7f899900d5b0 rejected with GraphQLError('Order mutation not allowed, Orderline can not be deleted.')>"
  }
}

I've read through the changelog of graphene and graphene-django. As this "broke" (for us) in graphene-django 3.0.0b8, i thought https://github.com/graphql-python/graphene-django/pull/1327 might have something to do with it. But then, that also requires a newer version of graphene.

Thanks for taking the time to read this.

erikwrede commented 1 year ago

I believe that this problem is caused by graphene-django rather than graphene. Usually, raising a graphql error should add an entry to the errors list for the specific resolver (identified by the path) rather than return a string. I'm usually on a non-django stack and it works exactly like you'd expect. I'll search through the code and get back to you!

erikwrede commented 1 year ago

hoi @khink, somehow my comment on this was not posted. It's been some time since then so I'll need to look again, sorry!

edwinvandeven commented 1 year ago

@erikwrede Would also be interested in your findings, I'm running into the same as @khink

payamt007 commented 1 year ago

I have same issue, please help!

Satcheel commented 1 year ago

+1 for this issue. I have been breaking my head for a day around this.

There is little to no documentation around error handling and things also seem to be broken. For queries, I get the above 'Promise returned the error' whereas for mutations no error or anything - just the fields are null.

Will appreciate a fix for the issue and would love to see some proper documentation with custom handlers etc.

erikwrede commented 1 year ago

Is anyone able to post a minimal reproduction as a gist (including django setup)? I'm no longer able to reproduce the behavior on a simple django app using the newest versions.

khink commented 1 year ago

Reproduced it with a minimal setup: https://github.com/khink/graphene-issue-1469/

erikwrede commented 1 year ago

@khink I see why my reproduction didn't work anymore. I had debug mode disabled. Setting Debug = False in settings.py fixes the error for me.

The problem is in the DjangoDebugMiddleware of graphene-django. Whenever Debug=True, it wraps all calls to resolvers (like your mutation resolver). On error, it uses the old promise library which graphql-core>2 no longer supports and does Promise.reject(error):

https://github.com/graphql-python/graphene-django/blob/d18cab8aa4b49a0314e7d26b877e88b7eb30a3d0/graphene_django/debug/middleware.py#L66-L69

https://github.com/graphql-python/graphene-django/blob/d18cab8aa4b49a0314e7d26b877e88b7eb30a3d0/graphene_django/debug/middleware.py#L23-L26

The correct way to handle exceptions in graphql core 3 is to raise them. Promises are treated like a valid response object. In your case, the promise can be converted to the ID type which is an implicit str, so the promise is converted to a string representation and treated like a response instead of an exception.

To solve the issue, you can try disabling the middleware or send a PR to the graphene-django team that removes the outdated promise code. I hope this helps, sorry it took me so long to get to it.

You can disable the middleware by removing "graphene_django.debug.DjangoDebugMiddleware" from settings.Middleware after initializing graphene-django. It is added here: https://github.com/graphql-python/graphene-django/blob/d18cab8aa4b49a0314e7d26b877e88b7eb30a3d0/graphene_django/settings.py#L48-L49

khink commented 1 year ago

Thanks, i'd never have found that. Thanks especially since the reason is not within this project. I'll think about whether i want to report this at graphene-django, for now i'm happy that i was able to get everything working.

Edit: There's already a ticket, i think https://github.com/graphql-python/graphene-django/issues/1367 covers exactly this.

erikwrede commented 1 year ago

Great to hear I could help! I've forwarded the issue to @firaskafri - hopefully it will be resolved soon! 🙂

firaskafri commented 1 year ago

Hello! Solved in https://github.com/graphql-python/graphene-django/pull/1388