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

Fix unchanging field bug when validate assignment true #1232

Conversation

austinpgraham
Copy link
Contributor

Problem

This PR addresses a bug reported here, where if validate_assignment is set to True on a schema, then Pydantic will correctly validate the new values, but the schema object will not persist the new value.

RCA

This is caused by the fact that pydantic-core sets these new values on the object (once validated) directly on the object __ dict __. As it was, this means that the new value will be set onto the DjangoGetter object, rather than the schema object being validated.

Solution

Added an extra condition in the root validator of a Schema object that will call handler on the original values before the conversion to a DjangoGetter to ensure the new value is persisted properly on the original object. Comment is also updated to reflect this change.

Testing

Added assertions into an existing test that a reassigned value is persisted when validate_assignment = True, and an additional test to test for the lack of ValidationError when validate_assignment is False or None.

Open Questions

This seems odd in the following way: I understand the use of DjangoGetter of course to handle resolvable fields. However, it not behaving like a dictionary is causing some churn where we now have to validate the object twice in certain conditions just to ensure fields are persisted/validated properly. I don't know immediately if there's a better solution, or if there's background here I don't know. Welcome to feedback.

@austinpgraham
Copy link
Contributor Author

@vitalik potential fix to issue mentioned here: #1223

@vitalik vitalik merged commit b75ada8 into vitalik:master Jul 15, 2024
27 checks passed
@vitalik
Copy link
Owner

vitalik commented Jul 15, 2024

Thank you

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.

2 participants