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.

Sign up for GitHub

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

Jump to bottom

Bug: Remove use of assert for pydantic import flow control #3354

Closed
4 tasks
peterschutt opened this issue Apr 9, 2024 · 4 comments · Fixed by #3359
Closed
4 tasks

Bug: Remove use of assert for pydantic import flow control #3354

peterschutt opened this issue Apr 9, 2024 · 4 comments · Fixed by #3359
Labels
area/contrib This PR involves changes to the contrib (Deprecated) Bug 🐛 This is something that is not working as expected Good First Issue This is good for newcomers to take on Help Wanted 🆘 This is good for people to work on

Comments

@peterschutt
Copy link
Contributor

peterschutt commented Apr 9, 2024 •
edited by polar-sh bot

Description

It was a poor choice to use assertions for the Pydantic import version discrimination refactor in #3347, as they can be optimized away.

We should revert to raising some other exception type where this was applied.

litestar/litestar/contrib/pydantic/pydantic_dto_factory.py

Lines 26 to 37 in fe73848

try:
import pydantic as pydantic_v2
assert pydantic_v2.__version__.startswith("2.") # noqa: S101
from pydantic import ValidationError as ValidationErrorV2
from pydantic import v1 as pydantic_v1
from pydantic.v1 import ValidationError as ValidationErrorV1
ModelType: TypeAlias = "pydantic_v1.BaseModel | pydantic_v2.BaseModel"
except AssertionError:

URL to code causing the issue

No response

MCVE

# Your MCVE code here

Steps to reproduce

1. Go to '...'
2. Click on '....'
3. Scroll down to '....'
4. See error

Screenshots

"![SCREENSHOT_DESCRIPTION](SCREENSHOT_LINK.png)"

Logs

No response

Litestar Version

v2.8.2

Platform

  • Linux
  • Mac
  • Windows
  • Other (Please specify in the description above)

Note

While we are open for sponsoring on GitHub Sponsors and
OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.
Fund with Polar
@peterschutt peterschutt added Bug 🐛 This is something that is not working as expected Help Wanted 🆘 This is good for people to work on Good First Issue This is good for newcomers to take on area/contrib This PR involves changes to the contrib (Deprecated) labels Apr 9, 2024
@provinzkraut
Copy link
Member

Why not just raise an ImportError?

@peterschutt
Copy link
Contributor Author

Why not just raise an ImportError?

Yeah

peterschutt added a commit that referenced this issue Apr 9, 2024
@peterschutt
fix: Remove use of asserts for control flow.
460ecf6 
#3347 introduced a new pattern to differentiate between Pydantic v1 and v2 installs, however it relies on using `assert` which is an issue as can optimised away.

This PR changes the approach to manually throw an `ImportError` instead.

Closes #3354
@peterschutt peterschutt mentioned this issue Apr 9, 2024
Merged
cofin pushed a commit that referenced this issue Apr 9, 2024
@peterschutt
fix: Remove use of asserts for control flow. (#3359)
4495c65 
#3347 introduced a new pattern to differentiate between Pydantic v1 and v2 installs, however it relies on using `assert` which is an issue as can optimised away.

This PR changes the approach to manually throw an `ImportError` instead.

Closes #3354
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 9, 2024

This issue has been closed in #3359. The change will be included in the upcoming patch release.

peterschutt added a commit that referenced this issue Apr 11, 2024
@peterschutt
fix: Remove use of asserts for control flow. (#3359)
75c38fa 
#3347 introduced a new pattern to differentiate between Pydantic v1 and v2 installs, however it relies on using `assert` which is an issue as can optimised away.

This PR changes the approach to manually throw an `ImportError` instead.

Closes #3354

(cherry picked from commit 4495c65)
peterschutt added a commit that referenced this issue Apr 11, 2024
@peterschutt
fix: Remove use of asserts for control flow. (#3359)
bcc7270 
#3347 introduced a new pattern to differentiate between Pydantic v1 and v2 installs, however it relies on using `assert` which is an issue as can optimised away.

This PR changes the approach to manually throw an `ImportError` instead.

Closes #3354

(cherry picked from commit 4495c65)
peterschutt added a commit that referenced this issue May 1, 2024
@peterschutt
fix: Remove use of asserts for control flow. (#3359)
63bfb4b 
#3347 introduced a new pattern to differentiate between Pydantic v1 and v2 installs, however it relies on using `assert` which is an issue as can optimised away.

This PR changes the approach to manually throw an `ImportError` instead.

Closes #3354

(cherry picked from commit 4495c65)
peterschutt added a commit that referenced this issue May 2, 2024
@peterschutt
fix: Remove use of asserts for control flow. (#3359)
0b3ab83 
#3347 introduced a new pattern to differentiate between Pydantic v1 and v2 installs, however it relies on using `assert` which is an issue as can optimised away.

This PR changes the approach to manually throw an `ImportError` instead.

Closes #3354

(cherry picked from commit 4495c65)
@github-actions GitHub Actions
Copy link

github-actions bot commented May 6, 2024

A fix for this issue has been released in v2.8.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/contrib This PR involves changes to the contrib (Deprecated) Bug 🐛 This is something that is not working as expected Good First Issue This is good for newcomers to take on Help Wanted 🆘 This is good for people to work on
Projects
Overview
Status: Closed
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Remove use of asserts for control flow. litestar-org/litestar
2 participants
@peterschutt @provinzkraut