-
-
Notifications
You must be signed in to change notification settings - Fork 768
🐛 Fix Field alias support for Pydantic v2 with backward compatibility #1577
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
base: main
Are you sure you want to change the base?
Conversation
This commit fixes Field(alias="...") functionality in Pydantic v2 while maintaining full backward compatibility with Pydantic v1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea looks good to me!
A couple of more things:
- I think it would be nice to use
alias
as a default value for DB column. It can then be overridden bysa_column=Column("column_name")
- We should probably test how it works with alias generators specified in model's config (is it supported? - UPD: YES)
PS: @svlandeg, sorry, I only noticed you self-assigned this PR when I was about to press "Submit review".. Hope you don't mind I submitted this review)
def post_init_field_info(field_info: FieldInfo) -> None: | ||
return None | ||
if IS_PYDANTIC_V2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is declared inside the if IS_PYDANTIC_V2:
block. So, this condition looks not necessary here.
Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added for backward compatibility with Pydantic v1.
Without this, Pydantic tests v1 would fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked it with Python 3.11 and Pydantic 1.10.22 - tests passed.
Could you please check it one more time?
def post_init_field_info(field_info: FieldInfo) -> None:
if field_info.alias and not field_info.validation_alias:
field_info.validation_alias = field_info.alias
if field_info.alias and not field_info.serialization_alias:
field_info.serialization_alias = field_info.alias
# Models | ||
|
||
|
||
class PydanticUser(BaseModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should think whether we want to keep parameterizing tests with Pydantic models.
Initially I parametrized tests with both, Pydantic and SQLModel models to show the difference in behavior between SQLModel and Pydantic models.
If it's useful to keep this parametrization in repo? 🤔
Tests would be a bit simpler without it, but on the other hand this way we ensure it works the same as with Pydantic model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think it’s useful to keep the parametrization for now, since it acts as a kind of integration test between the two libraries.
*, | ||
default_factory: Optional[NoArgAnyCallable] = None, | ||
alias: Optional[str] = None, | ||
validation_alias: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pydantic's validation_alias
type annotation is wider (validation_alias: str | AliasPath | AliasChoices | None
), but I think it's fine if we only support str
for now and extend it later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong preference on this.
Co-authored-by: Motov Yurii <[email protected]>
Of course not! I tentatively assign PRs when I want to review them in the near future, just to keep track of them, but sometimes other tasks come in between 🙈 Feel free to review whatever you want! |
@YuriiMotov @svlandeg I’ve addressed all the comments from the review. Let me know if there’s anything else you’d like me to focus on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good! We are almost there)
I added a few comments.
Tested it a bit more an found a use case that is broken with this PR applied.
This is the workaround people use for now. Although it's not documented and it's a misuse of schema_extra
, I think it would be nice to support passing validation_alias
and serialization_alias
via schema_extra
, and show a warning message in such cases.
from sqlmodel import SQLModel, Field
class MyModel(SQLModel):
f: str = Field(schema_extra={"validation_alias": "f_alias"})
m = MyModel.model_validate({"f_alias": "asd"})
assert m.f == "asd"
Also, we may want to add more tests (just ideas):
alias
+validation_alias
=validation_alias
takes precedencealias
+serialization_alias
=serialization_alias
takes precedencealias_generator
in model configalias_generator
in model config +alias
for field =alias
takes precedence- passing
validation_alias
andserialization_alias
viaschema_extra
def test_serialization_alias_runtimeerror_pydantic_v1(): | ||
if VERSION.startswith("2."): | ||
pytest.skip("Only relevant for Pydantic v1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_serialization_alias_runtimeerror_pydantic_v1(): | |
if VERSION.startswith("2."): | |
pytest.skip("Only relevant for Pydantic v1") | |
@needs_pydanticv1 | |
def test_serialization_alias_runtimeerror_pydantic_v1(): |
def test_validation_alias_runtimeerror_pydantic_v1(): | ||
if VERSION.startswith("2."): | ||
pytest.skip("Only relevant for Pydantic v1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_validation_alias_runtimeerror_pydantic_v1(): | |
if VERSION.startswith("2."): | |
pytest.skip("Only relevant for Pydantic v1") | |
@needs_pydanticv1 | |
def test_validation_alias_runtimeerror_pydantic_v1(): |
from pydantic import Field as PField | ||
from sqlmodel import Field, SQLModel | ||
|
||
from tests.conftest import needs_pydanticv2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from tests.conftest import needs_pydanticv2 | |
from tests.conftest import needs_pydanticv1, needs_pydanticv2 |
Description
This PR fixes the Field(alias="...") functionality that was broken in Pydantic v2 while maintaining full backward compatibility with Pydantic v1.
Problem
In Pydantic v2, the
alias
parameter in SQLModel'sField
function was not working correctly for validation and serialization. The issue was that Pydantic v2 requires explicitvalidation_alias
andserialization_alias
parameters, but SQLModel wasn't automatically propagating thealias
value to these parameters.Solution
Changes Made:
Enhanced Field function:
validation_alias
andserialization_alias
parametersAutomatic alias propagation:
post_init_field_info
in_compat.py
to automatically setvalidation_alias
andserialization_alias
fromalias
when not explicitly provided (Pydantic v2 only)Backward compatibility:
Comprehensive testing:
Breaking Changes
None. This is purely a bug fix with backward compatibility maintained.
Closes
Fixes #1536