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

update codebase to support fastapi>=0.100.0 and pydantic>=2.0.0 #447

Merged
merged 23 commits into from
Sep 28, 2023

Conversation

johnybx
Copy link
Contributor

@johnybx johnybx commented Aug 21, 2023

Hi,
I updated codebase to support fastapi>=0.100.0 and pydantic>=2.0.0 ( this is breaking change ). I tried to keep same features and interface but when it comes to with_prefix function there were changes required. Currently with pydantic>=2.0.0 when you define attribute like in this example:

        class MainFilter(BaseModel):
            name: str
            number_filter: Optional[NumberFilter] = FilterDepends(with_prefix("number_filter", NumberFilter))

you get error:

pydantic_core._pydantic_core.ValidationError: 1 validation error for FilterWrapper
  address
    Input should be a valid dictionary or instance of NumberFilter [type=model_type, input_value=FilterWrapper(...), input_type=FilterDepends.<locals>.FilterWrapper]
  For further information visit https://errors.pydantic.dev/2.2/v/model_type

The only workaround I found (currently) is to update with_prefix to generate the wrapped model and annotation with PlainValidator.

TODO:

  • test with all supported python versions
  • update documentation
  • this is breaking change - should probably update major version instead of minor ?

@netlify
Copy link

netlify bot commented Aug 21, 2023

Deploy Preview for fastapi-filter ready!

Name Link
🔨 Latest commit a267eea
🔍 Latest deploy log https://app.netlify.com/sites/fastapi-filter/deploys/650b42510f84df0007198f9c
😎 Deploy Preview https://deploy-preview-447--fastapi-filter.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@johnybx johnybx marked this pull request as draft August 21, 2023 09:18
@arthurio
Copy link
Owner

@johnybx Thanks so much for putting time into this, I'll try to review/help as much as I can 👍🏻 And yeah a major version bump is the way to go.

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #447 (a267eea) into main (7f1f367) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #447   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          180       193   +13     
=========================================
+ Hits           180       193   +13     
Files Changed Coverage Δ
fastapi_filter/base/filter.py 100.00% <100.00%> (ø)
fastapi_filter/contrib/mongoengine/filter.py 100.00% <100.00%> (ø)
fastapi_filter/contrib/sqlalchemy/filter.py 100.00% <100.00%> (ø)

@johnybx
Copy link
Contributor Author

johnybx commented Sep 4, 2023

I came up with different approach which does not introduce breaking changes to how with_prefix is used. It would be nice if you could review the changes @arthurio. Thanks

@johnybx johnybx marked this pull request as ready for review September 6, 2023 14:54
Copy link
Owner

@arthurio arthurio left a comment

Choose a reason for hiding this comment

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

That look really great, thank you so much for all your work!
I just have a few nits but we can ship it after that 🚀

examples/fastapi_filter_mongoengine.py Show resolved Hide resolved
examples/fastapi_filter_mongoengine.py Outdated Show resolved Hide resolved
examples/fastapi_filter_sqlalchemy.py Outdated Show resolved Hide resolved
fastapi_filter/base/filter.py Outdated Show resolved Hide resolved
tests/test_mongoengine/test_order_by.py Show resolved Hide resolved
johnybx and others added 3 commits September 13, 2023 09:12
Co-authored-by: Arthur Rio <[email protected]>
Signed-off-by: johnybx <[email protected]>
Co-authored-by: Arthur Rio <[email protected]>
Signed-off-by: johnybx <[email protected]>
Co-authored-by: Arthur Rio <[email protected]>
Signed-off-by: johnybx <[email protected]>
@johnybx johnybx requested a review from arthurio September 14, 2023 06:46
Copy link
Owner

@arthurio arthurio left a comment

Choose a reason for hiding this comment

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

Just that one last question so that we avoid confusion for people hitting that edge case, otherwise looks like it's ready to go! 👍🏻

if len(annotation_args) == 1:
annotation = annotation_args[0]
# Not sure what to do if there is more then 1 value 🤔
# Do we need to handle Optional[Annotated[...]] ?
Copy link
Owner

Choose a reason for hiding this comment

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

We should raise an exception for now until we figure this out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left the comment here for future as a note that union of list type with other types is not supported just in case someone thought that for example type like list[str] | int | None should work with filter. At the same time I don't think that it is reasonable to support such types ( I mean transformation of list part to str split by comma in such complex types) without knowing real usecase. Also supporting such types would be problem in case of types like list[str] | str | None.

Raising exception here is not a good idea because the exception would be raised on any type which is union basically so for example int | float because this is union type and will have length 2 but at the same time it is valid type which we want to skip. Maybe I should just update comment to specifically mention that I am not sure what to do if the type is combination of list and other types like list[str] | str ? Because list filtering will not work in that case 🤔

What do you think ?

Copy link
Owner

Choose a reason for hiding this comment

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

@johnybx Do you have time to make that change or do you want me to do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arthurio sure I just wasn't sure if you are fine with what I wrote. I added note to code and also added small section about current limitations of union types in filters with list. Let me know if something should be changed 👍

fastapi_filter/base/filter.py Outdated Show resolved Hide resolved
fastapi_filter/base/filter.py Outdated Show resolved Hide resolved
examples/fastapi_filter_mongoengine.py Show resolved Hide resolved
if len(annotation_args) == 1:
annotation = annotation_args[0]
# Not sure what to do if there is more then 1 value 🤔
# Do we need to handle Optional[Annotated[...]] ?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left the comment here for future as a note that union of list type with other types is not supported just in case someone thought that for example type like list[str] | int | None should work with filter. At the same time I don't think that it is reasonable to support such types ( I mean transformation of list part to str split by comma in such complex types) without knowing real usecase. Also supporting such types would be problem in case of types like list[str] | str | None.

Raising exception here is not a good idea because the exception would be raised on any type which is union basically so for example int | float because this is union type and will have length 2 but at the same time it is valid type which we want to skip. Maybe I should just update comment to specifically mention that I am not sure what to do if the type is combination of list and other types like list[str] | str ? Because list filtering will not work in that case 🤔

What do you think ?

if len(annotation_args) == 1:
annotation = annotation_args[0]
# Not sure what to do if there is more then 1 value 🤔
# Do we need to handle Optional[Annotated[...]] ?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arthurio sure I just wasn't sure if you are fine with what I wrote. I added note to code and also added small section about current limitations of union types in filters with list. Let me know if something should be changed 👍

@johnybx
Copy link
Contributor Author

johnybx commented Sep 22, 2023

@arthurio sorry that it took so long but I just realized that pending label on comment does not mean that it is waiting for reviewer but rather that it is not submitted 🤦

@johnybx johnybx requested a review from arthurio September 28, 2023 12:53
Copy link
Owner

@arthurio arthurio left a comment

Choose a reason for hiding this comment

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

Let's do it! Thanks again for all the hard work, really appreciate it.

@arthurio arthurio merged commit 827d2e3 into arthurio:main Sep 28, 2023
16 checks passed
@LucasPickering
Copy link

@arthurio @johnybx Thank you so much for your work on this! Very excited to upgrade now

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.

3 participants