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

Remove promise based debug middleware #1388

Merged

Conversation

jaw9c
Copy link
Collaborator

@jaw9c jaw9c commented Feb 13, 2023

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)

@jaw9c
Copy link
Collaborator Author

jaw9c commented Feb 13, 2023

Potentially fixes #1387

@firaskafri
Copy link
Collaborator

Hello @jaw9c, this is great!
Please check the tests as they're failing

@jaw9c
Copy link
Collaborator Author

jaw9c commented Feb 20, 2023

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

@jaw9c jaw9c force-pushed the fix/remove-promise-based-middleware branch 4 times, most recently from 9a464fb to 2441c4f Compare February 27, 2023 15:38
@jaw9c jaw9c force-pushed the fix/remove-promise-based-middleware branch from 2441c4f to e850ec2 Compare February 27, 2023 15:42
@jaw9c
Copy link
Collaborator Author

jaw9c commented Feb 27, 2023

@firaskafri Updated the PR, tests now get run correctly - was a problem with the python path not being set 👍

Copy link
Member

@jkimbo jkimbo left a 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.

@firaskafri
Copy link
Collaborator

I'm not that familiar with this part of the codebase but the changes look fine to me.

Thank you @jkimbo 🙏

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

Successfully merging this pull request may close these issues.

3 participants