search

graphql-python / graphene-pydantic

Integrate GraphQL with your Pydantic models
Other
229 stars 44 forks source link

Pydantic compatibility features #75

Closed dima-dmytruk23 closed 2 years ago

dima-dmytruk23 commented 2 years ago

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.

necaris commented 2 years ago

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

dima-dmytruk23 commented 2 years ago

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

necaris commented 2 years ago

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

dima-dmytruk23 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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:

necaris commented 2 years ago

@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 commented 2 years ago

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!

dima-dmytruk23 commented 2 years ago

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

necaris commented 2 years ago

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