-
-
Notifications
You must be signed in to change notification settings - Fork 736
🐛 Ensure that type checks are executed when setting table=True
#1041
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
raw_self = self.model_copy() | ||
pydantic_validated_model = self.__pydantic_validator__.validate_python( | ||
data, | ||
self_instance=raw_self, |
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.
why was raw_self
necessary here?
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.
why was
raw_self
necessary here?
self.__pydantic_validator__.validate_python(
data,
self_instance=raw_self,
)
would update self, and something would go wrong on sqlmodel_table_construct
for the changed self. Maybe that's the reason that the original code dosen't validate when table=True
.
Actually, we need to get the pydantic-validated dict, and use this dict to update the original dict, and then construct tables by the updated-original dict.
Anyway, you can try self_instance=self
, and some unittests would fail.
Some related issues for this one:
However, I think this comment indicates that this is actually the desired behavior. |
I regard this action as a bug rather than a feature because |
@tiangolo Would you please look at this PR snd see whether this solution is OK? It passes all tests. |
Hey, bumping this because we urgently need it as well :) Thanks! |
Fortunately, there seems to be an easy solution.
|
table=True
Bumping this. I get @tiangolo 's intention, but it seems that things are naturally leaning in another direction. If nothing is wrong with this implementation, then I think it's a huge step forward. |
This works for type validations(as PR aims to fix) but it is not very helpful if you want to use field validators, please see the comment: #52 (comment) Just wanted to point it out if someone ends up here looking for how to work with field or model validators like me. |
Agree with this interpretation, but at the same time the code seems to explicitly try to avoid that behaviour (https://github.com/fastapi/sqlmodel/blob/main/sqlmodel/_compat.py#L564). Would love some clarity from the FastAPI folks on the intention here. This was definitely the most surprising behaviour I noticed using SQLModel. |
I found that when
table=True
is set, pydantic becomes invalid for usage likehero = Hero(name="Deadpond", secret_name="Dive Wilson", age="test")
. As the document saysyou get all of Pydantic's features, including automatic data validation, serialization, and documentation. You can use SQLModel in the same way you can use Pydantic.
, I thinktable=True
makes pydantic invalid is a bug, so I tried to fix it.I delete a test case because this case is incompatible with pydantic. I also add a test case to ensure my code works. For current code, my new test case would fail, but it would pass for my new code.