graphql-python / graphql-python/graphene

graphql-core coerce_string used instead of graphene.String

オープン
#1,392 コメント 2 件 リアクション 0 件 担当者 0 名 GitHub で見る
🐛 bug
主要言語
Python
スター
8.2k
フォーク
818
PR マージ指標
30日以内にマージされた PR はありません

説明

**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.

コントリビューションガイド

このリポジトリのコントリビューションガイドは索引されていません

調査の方向性

Start by tracing scalar handling through graphene/types/schema.py and compare graphene/types/scalars.py with graphql/type/scalars.py. Reproduce the HttpUrl case using graphene.test.Client.execute, then determine whether the intended fix is to preserve graphene.String coercion or align it with graphql-core. Done means the behavior and expected coercion are covered by a regression test.

索引モデルが issue の本文から書いたものです。

評価

技術スタック
graphql, python
領域
api, backend
issue の種類
バグ
難易度
4/5
見積もり時間
3〜5日
活発さ
停滞
明瞭さ
説明が足りない
初心者へのやさしさ
35/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。