-
Notifications
You must be signed in to change notification settings - Fork 823
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
graphql-core coerce_string used instead of graphene.String #1392
Labels
Comments
I believe this may be the cause of an issue I'm experiencing. I'm new to GraphQL and graphene, and am trying to do a fix for a developer that won't be available again for several months. graphene.String is used for several cases (e.g. serial number) that can occasionally be all digits. In this case, the involved mutations fail. Getting this fixed is pretty high on my list. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Note: for support questions, please use stackoverflow. This repository's issues are reserved for feature requests and bug reports.
Not 100% sure this is a bug, but it seems not quite right.
graphene.String
has the followingcoerce_string
method:https://github.com/graphql-python/graphene/blob/master/graphene/types/scalars.py#L144
This should mean that any value that is resolved via a
graphene.String
field gets cast tostr
In contrast,
graphql-core
has thiscoerce_string
method:https://github.com/graphql-python/graphql-core/blob/a2d52df815f81a62649ee637999fc241af1e67a3/src/graphql/type/scalars.py#L176
...note that in graphql-core any value which passes an
isinstance(val, str)
check will not get cast tostr
.I noticed this because I am using pydantic
HttpUrl
type in my parent object, the value is anHttpUrl
instance (which passes anisinstance(val, str)
check).I noticed in my test cases that the
HttpUrl
value passes untouched through thegraphene.test.Client.execute
flow.Looking at the graphene src this was puzzling because it seemed like it should get cast down to a plain string.
Stepping through with ipdb, I could see in
complete_value
method:...so we have a graphql-core field rather than a graphene one, which explains why the cast to
str
didn't happen.I am guessing it may be due to this code:
graphene/graphene/types/schema.py
Line 148 in f039af2
Which raises the question... are the methods on
graphene.String
and friends just dead code?Or if they're still used in some other circumstance, should they maybe delegate to corresponding graphql-core methods for sake of consistency, rather than having their own subtly different implementation?
If I change my type def to:
...then I get the graphene casting behaviour, again I think because we fall thru this check
graphene/graphene/types/schema.py
Line 148 in f039af2
In the end this behaviour didn't break anything for me, but it's probably possible to imagine a scenario where it would.
The text was updated successfully, but these errors were encountered: