-
Notifications
You must be signed in to change notification settings - Fork 769
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
Remove promise based debug middleware #1388
Remove promise based debug middleware #1388
Conversation
Potentially fixes #1387 |
Hello @jaw9c, this is great! |
Thanks @firaskafri - looks like the CI is broken as the tests pass locally. Just looked at them and appears to be an import error with the examples folder - https://github.com/graphql-python/graphene-django/actions/workflows/tests.yml |
9a464fb
to
2441c4f
Compare
2441c4f
to
e850ec2
Compare
@firaskafri Updated the PR, tests now get run correctly - was a problem with the python path not being set 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not that familiar with this part of the codebase but the changes look fine to me.
Thank you @jkimbo 🙏 |
The exception handling by the debug middleware is broken. If using the toolbar middleware, then any exception raised in a resolver gets consumed and replaced with a cryptic
Thread pool
related error.The root cause is that the graphql core lib expects the result of the call to exception instance rather than a rejected promise.
When the graphql core lib tries to resolve the promise as a normal result it results in the cryptic error.
I've also removed the reference to promises as they didn't seem to be helping anything (But please correct the PR if I'm missing something)