graphql-python / graphql-python/graphene

graphql-core coerce_string used instead of graphene.String

Offen
#1,392 2 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
🐛 bug
Vorherrschende Sprache
Python
Sterne
8.2k
Forks
818
PR-Merge-Kennzahlen
Keine gemergten PRs in 30 T.

Beschreibung

**Note: for support questions, please use stackoverflow**. This repository's issues are reserved for feature requests and bug reports.

* **What is the current behavior?**

Not 100% sure this is a bug, but it seems not quite right.

`graphene.String` has the following `coerce_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 to `str`

In contrast, `graphql-core` has this `coerce_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 to `str`.

I noticed this because I am using pydantic `HttpUrl` type in my parent object, the value is an `HttpUrl` instance (which passes an `isinstance(val, str)` check).

I noticed in my test cases that the `HttpUrl` value passes untouched through the `graphene.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:
```
ipdb> pp result
HttpUrl('https://example.com/test-url, scheme='https', host='example.com', tld='com', host_type='domain', path='test-url')
ipdb> return_type

ipdb> import inspect
ipdb> inspect.getsource(return_type.serialize)
'def coerce_string(value):\n # type: (Any) -> str\n if isinstance(value, string_types):\n return value\n\n if isinstance(value, bool):\n return u"true" if value else u"false"\n\n return text_type(value)\n'
ipdb> inspect.getsourcefile(return_type.serialize)
'/lib/python3.8/site-packages/graphql/type/scalars.py'
```
...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:
https://github.com/graphql-python/graphene/blob/f039af2810806ab42521426777b3a0d061b02802/graphene/types/schema.py#L148

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:
```python
import graphene

class HttpUrl(graphene.String):
pass

class MyParent(graphene.ObjectType):
some_url = HttpUrl(required=True)

```
...then I get the graphene casting behaviour, again I think because we fall thru this check https://github.com/graphql-python/graphene/blob/f039af2810806ab42521426777b3a0d061b02802/graphene/types/schema.py#L148 and don't substitute a graphql-core type.

In the end this behaviour didn't break anything for me, but it's probably possible to imagine a scenario where it would.

Beitragsleitfaden

Für dieses Repository ist kein Beitragsleitfaden indexiert

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.