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

Bugfix: Cast error message to string to handle proxy objects #175

Merged
merged 1 commit into from Jun 11, 2022

Conversation

jkimbo
Copy link
Member

@jkimbo jkimbo commented Jun 8, 2022

Some frameworks (Django as an example) use a lazy evaluated object to represent error messages (https://github.com/django/django/blob/49b470b9187b6be60e573fed08c8f4a87f133750/django/utils/functional.py#L87-L96). This breaks graphql-core when it tries to format the error as a string.

This PR fixes this bug by forcing the error message to a string when creating a GraphQLError object.

@jkimbo jkimbo requested a review from Cito as a code owner June 8, 2022 11:54
@jkimbo jkimbo force-pushed the cast-error-messages-to-string branch 2 times, most recently from 3a1aa29 to 6a9f820 Compare June 8, 2022 12:51
@jkimbo jkimbo force-pushed the cast-error-messages-to-string branch from 6a9f820 to a5450f0 Compare June 10, 2022 17:51
@jkimbo
Copy link
Member Author

jkimbo commented Jun 10, 2022

I changed my approach to this fix by forcing the error message to a string in the located_error function instead of the GraphQLError.__str__ method.

@Cito
Copy link
Member

Cito commented Jun 10, 2022

Thank you @jkimbo, will look into this later. I think #153 was something similar.

@Cito Cito merged commit 2545ff4 into main Jun 11, 2022
Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. In addabc0 I have simplified the test a bit and made it more similar to the other one in test_block_string.

@jkimbo
Copy link
Member Author

jkimbo commented Jun 12, 2022

Thanks @Cito

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.

None yet

2 participants