Skip to content
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

Pydantic V2 Support #94

Merged
merged 25 commits into from
Jan 31, 2024
Merged

Conversation

mak626
Copy link
Contributor

@mak626 mak626 commented Nov 3, 2023

Fixes #87

Changelog

  • Dropped support for pydantic v1 and python < 3.8.
  • Added support for pydantic v2
    • Changed ModelField -> FieldInfo
    • pydantic.parse_obj_as(FooBar, data["listFooBars"]) was changed to new syntax TypeAdapter(FooBar).validate_python(data["createFooBar"])
  • Support for python11 Union using pipe syntax
  • Added support for bson ObjectId -> graphene.ID
  • Added support for dict -> JSONString
  • Added default is_type_of resolvers. This simplifies using UnionType as is_type_of need not be defined individually in the base types of Union

Pydantic V2 also changed the way we define fields. Please refer their doc Migration Guide. Some of the breaking changes include Optional[type] no longer has a default value None unless given explicitly.

@necaris
Copy link
Collaborator

necaris commented Nov 5, 2023

Thank you @mak626 for the PR! Obviously this changes the valid dependencies for graphene-pydantic -- I'm thinking we should:

  • create a maintenance branch 0.4.x which will retain support for Pydantic 1.x
  • create a 0.5.0 release branch, with an emphasized note in the changelog about dropping support for Pydantic 1 and Python < 3.10
  • merge this in to the 0.5.0 release branch and do any more polishing we need before we merge that to main

@dantheman39, do you agree?

@abhinand-c
Copy link
Contributor

@necaris Maybe a typo on your side, but just to clarify support for Python support for 3.7 and below is only dropped, as it's EOL. We can maintain support for Python >=3.8

@ported-pw
Copy link

Thank you for your work! Just commenting here to express my interest in seeing this get merged / released properly.

@ported-pw
Copy link

ported-pw commented Jan 12, 2024

I think I just found a bug in this @mak626 @abhinand-c:
If you define a field as a union of two or more types and make it optional, the field does not get marked as optional in the schema.
These cases do not work as expected:

  • TypeOne | TypeTwo | None
  • Optional[TypeOne | TypeTwo]
  • Optional[Union[TypeOne, TypeTwo]]
  • Union[TypeOne, TypeTwo] | None

@mak626
Copy link
Contributor Author

mak626 commented Jan 12, 2024

@ported-pw , pydantic v2 has changed the way we use fields

The is the current way to use the fields to achieve optional

image

Reference: https://docs.pydantic.dev/2.0/migration/

@ported-pw
Copy link

ported-pw commented Jan 12, 2024

Sorry @mak626 , maybe my examples were not clear enough, I'll extend them to proper fields:

class SomeModel(BaseModel):
  field_1: TypeOne | TypeTwo | None = None
  field_2: Optional[TypeOne | TypeTwo]  = None
  field_3: Optional[Union[TypeOne, TypeTwo]]  = None
  field_4: Union[TypeOne, TypeTwo] | None  = None

None of these would be marked optional in the GraphQL schema right now, while they are not required by Pydantic during model validation, leading to this error:
Cannot return null for non-nullable field SomeModel.field_1..
So with the current state of the code it is not possible to make a schema with an optional union type.

There does not seem to be a test case for this combination of explicit union + optional/union with None.

@mak626
Copy link
Contributor Author

mak626 commented Jan 12, 2024

Sorry @mak626 , maybe my examples were not clear enough, I'll extend them to proper fields:

class SomeModel(BaseModel):
  field_1: TypeOne | TypeTwo | None = None
  field_2: Optional[TypeOne | TypeTwo]  = None
  field_3: Optional[Union[TypeOne, TypeTwo]]  = None
  field_4: Union[TypeOne, TypeTwo] | None  = None

None of these would be marked optional in the GraphQL schema right now, while they are not required by Pydantic during model validation, leading to this error: Cannot return null for non-nullable field SomeModel.field_1.. So with the current state of the code it is not possible to make a schema with an optional union type.

There does not seem to be a test case for this combination of explicit union + optional/union with None.

Thanks, we missed this case. Have fixed this now.

@ported-pw
Copy link

Awesome, seems to work well now :)

@dantheman39
Copy link
Contributor

dantheman39 commented Jan 13, 2024

Thank you @mak626 for the PR! Obviously this changes the valid dependencies for graphene-pydantic -- I'm thinking we should:

* create a maintenance branch `0.4.x` which will retain support for Pydantic 1.x

* create a `0.5.0` release branch, with an emphasized note in the changelog about dropping support for Pydantic 1 and Python < 3.10

* merge this in to the `0.5.0` release branch and do any more polishing we need before we merge that to `main`

@dantheman39, do you agree?

@necaris @mak626 would a major version bump be more appropriate? Does this include breaking changes or am I mistaken?

But I agree with creating a maintenance branch, and also agree with submitting a follow-up cleanup PR. I have a bit of time coming up and would be happy to do that. Are you thinking we merge this as-is, then?

Copy link
Contributor

@dantheman39 dantheman39 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this contribution. I'm currently traveling but will try to find time soon to give a proper review.

Question: should any updates be made to the README?

I ran the test suite and it looks like CI is not passing. If you're able to take a look at why that will be great.

Thanks again!

@necaris
Copy link
Collaborator

necaris commented Jan 14, 2024

Does this include breaking changes or am I mistaken?

Yes, this would include a breaking change dropping support for Pydantic 1.x (since the changes are incompatible); I'm not against making a major version bump, although IMHO calling any part of this library 1.x (implying readiness for production) is premature... 😉

@mak626
Copy link
Contributor Author

mak626 commented Jan 15, 2024

Thank you @mak626 for the PR! Obviously this changes the valid dependencies for graphene-pydantic -- I'm thinking we should:

* create a maintenance branch `0.4.x` which will retain support for Pydantic 1.x

* create a `0.5.0` release branch, with an emphasized note in the changelog about dropping support for Pydantic 1 and Python < 3.10

* merge this in to the `0.5.0` release branch and do any more polishing we need before we merge that to `main`

@dantheman39, do you agree?

@necaris @mak626 would a major version bump be more appropriate? Does this include breaking changes or am I mistaken?

But I agree with creating a maintenance branch, and also agree with submitting a follow-up cleanup PR. I have a bit of time coming up and would be happy to do that. Are you thinking we merge this as-is, then?

Yes it includes breaking changes as pydantic v2 usage is a lot different from v1. Major version bump would be necessary.

Also this library would work for python versions >=3.8 not just >=3.10

@mak626
Copy link
Contributor Author

mak626 commented Jan 15, 2024

Thanks so much for this contribution. I'm currently traveling but will try to find time soon to give a proper review.

Question: should any updates be made to the README?

I ran the test suite and it looks like CI is not passing. If you're able to take a look at why that will be great.

Thanks again!

Yes I have fixed the tests now for other python versions. Was due to an import error for UnionType of python 10.

Readme can be updated to emphasise that there are breaking changes when switching from v1 to v2 along with this link https://docs.pydantic.dev/2.0/migration/

Other than that I think the way we use this library remains the same.

Copy link
Contributor

@dantheman39 dantheman39 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@dantheman39 dantheman39 merged commit 5d66766 into graphql-python:main Jan 31, 2024
13 checks passed
@mak626 mak626 deleted the feat/pydantic-v2 branch February 27, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No support for pydantic-2
6 participants