graphene-pydantic icon indicating copy to clipboard operation
graphene-pydantic copied to clipboard

Pydantic compatibility features

Open dima-dmytruk23 opened this issue 2 years ago • 7 comments

issue - https://github.com/graphql-python/graphene-pydantic/issues/73

Python 3.9

https://github.com/graphql-python/graphene-pydantic/pull/75/files#diff-7a099366beb78ce7e417d3c73fef1dcb56772e9c163a7b0e9a4680cb29d7b1c1R193

Perhaps here you can somehow check that the field in the model is Optional and then use this class?

override_input_fields - Perhaps there is a better solution.

dima-dmytruk23 avatar Apr 24 '22 01:04 dima-dmytruk23

@dima-dmytruk23 in #73 where you brought this up, you mention a few specific cases.

I am having trouble understanding what you need, and why these code changes are necessary for this library. Can you please provide specific code examples that don't work without these changes? Please see some specific questions on each point below

  1. In mutations - those fields that were not passed - still come as None. As a result, when parsing a dictionary into a Type[BaseModel], these fields are also passed through and an entry is added to the database with these values. exclude_default, exclude_unset - don't work. It is not correct.

It seems perhaps that skip_defaults is the correct argument in Pydantic -- based on https://github.com/samuelcolvin/pydantic/commit/96e3e74262c8bf19c57ca0526337f29abd8e1fdb ? Can you provide an example of your code, which is not working as intended?

  1. If null comes from the client to the Int field - I get an error. The field must be nullable.

I tried this, and it seems to work fine for me if I mark the field as Optional[int] -- see https://gist.github.com/necaris/73c5d8bc47b304cdcd97f17c5c729412 , which is based on the example in the README. Can you provide an example of your model, which is not working?

  1. Pass empty strings as null in Output.

Does https://github.com/samuelcolvin/pydantic/discussions/2687 solve this? I don't think that turning empty strings into None should be the default behavior, so this seems like a better solution if it works for you.

necaris avatar Apr 24 '22 21:04 necaris

I tried this, and it seems to work fine for me if I mark the field as Optional[int] -- see https://gist.github.com/necaris/73c5d8bc47b304cdcd97f17c5c729412 , which is based on the example in the README. Can you provide an example of your model, which is not working?

Very strange. Now it works.

Does samuelcolvin/pydantic#2687 solve this? I don't think that turning empty strings into None should be the default behavior, so this seems like a better solution if it works for you.

Yes. It looks like it works. Thnx.

It seems perhaps that skip_defaults is the correct argument in Pydantic -- based on samuelcolvin/pydantic@96e3e74 ? Can you provide an example of your code, which is not working as intended?

Yes. i have tried this. skip_defaults (exclude_unset) - doesn't work. The frontend does not pass the name field, but it is still present in the mutation as None. Therefore, when I convert an input into a model, this field is written there. Here is an example of a model, schema, and query. https://gist.github.com/dima-dmytruk23/aaeba0fbc7a539c1f8bf3d0914fce580

dima-dmytruk23 avatar Apr 24 '22 23:04 dima-dmytruk23

@dima-dmytruk23 thank you for the example! I've been looking into it and from what I can see, the input query turns into a UserUpdateInput, which is when the default values are filled in to the dictionary. So then when your code passes the dictionary in to build the UserUpdate, it sets all the fields -- so exclude_unset doesn't exclude anything, since all the fields were in fact set.

Based on https://github.com/graphql/graphql-spec/issues/476 , there isn't actually anything in the specification that says that passing the default None values through in the dictionary is the wrong thing to do, so it's possible graphene doesn't implement that behavior. I am fairly sure it's not in graphene-pydantic, though, since that is only responsible for converting to the GrapheneInputObjectType.

I'll look a little further if I get the time.

necaris avatar Apr 25 '22 03:04 necaris

@necaris Thnx. I think it would be nice to use some kind of flag in the Schema, or metaclass, to indicate whether fields that were not passed from the client should be excluded.

dima-dmytruk23 avatar Apr 25 '22 13:04 dima-dmytruk23

@necaris It is possible to collect words from vars, which is passed from the input, but for some reason I don't like this solution. Also, I don't want to use a graphene or graphql-core` fork to substitute. So now I'm using this crutch solution.

dima-dmytruk23 avatar Apr 25 '22 13:04 dima-dmytruk23

@necaris Also, I can fix it if replace https://github.com/graphql-python/graphql-core/blob/main/src/graphql/utilities/coerce_input_value.py#L98-L112 to if field_name in input_value:

dima-dmytruk23 avatar Apr 25 '22 13:04 dima-dmytruk23

@dima-dmytruk23 it seems to me that if you are able to fix it with a change to graphql-core you should submit a PR there. As you said, if you used a flag on the Schema to check it, this should be an acceptable change.

necaris avatar Apr 25 '22 15:04 necaris

There hasn't been movement on this in 6 months, so I'm closing it as not an appropriate change for this library (rather than graphql-core). @dima-dmytruk23 if you have changes you'd like to make to the PR, or other reasons we should include this, please reopen!

necaris avatar Nov 02 '22 02:11 necaris

There hasn't been movement on this in 6 months, so I'm closing it as not an appropriate change for this library (rather than graphql-core). @dima-dmytruk23 if you have changes you'd like to make to the PR, or other reasons we should include this, please reopen!

It's not problem of graphql-core https://github.com/graphql-python/graphene-pydantic/issues/73#issuecomment-1120485571

dima-dmytruk23 avatar Nov 03 '22 12:11 dima-dmytruk23

@dima-dmytruk23 from what I saw from the graphql-core issue, though, this is occurring because of the behavior of pydantic (not graphene-pydantic, but upstream pydantic) which we don't want to work around here.

necaris avatar Nov 03 '22 21:11 necaris