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(validation): also check root_validators when validate_assignment is on #1972

Merged

Conversation

PrettyWood
Copy link
Member

Change Summary

This is a proposal but I agree with @caldwellshane that root_validator should be considered as a regular validator. It should hence be triggered when validate_assignment is on.
This is not the case at the moment

Related issue number

closes #1971

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@codecov
Copy link

codecov bot commented Oct 6, 2020

Codecov Report

Merging #1972 into master will increase coverage by 0.15%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #1972      +/-   ##
===========================================
+ Coverage   99.84%   100.00%   +0.15%     
===========================================
  Files          42        21      -21     
  Lines        7850      3925    -3925     
  Branches     1576       788     -788     
===========================================
- Hits         7838      3925    -3913     
+ Misses         10         0      -10     
+ Partials        2         0       -2     
Impacted Files Coverage Δ
D:/a/pydantic/pydantic/pydantic/env_settings.py
D:/a/pydantic/pydantic/pydantic/networks.py
D:/a/pydantic/pydantic/pydantic/datetime_parse.py
D:/a/pydantic/pydantic/pydantic/errors.py
...:/a/pydantic/pydantic/pydantic/class_validators.py
D:/a/pydantic/pydantic/pydantic/fields.py
D:/a/pydantic/pydantic/pydantic/tools.py
D:/a/pydantic/pydantic/pydantic/mypy.py
D:/a/pydantic/pydantic/pydantic/error_wrappers.py
D:/a/pydantic/pydantic/pydantic/dataclasses.py
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8326f8...7e8cc8c. Read the comment docs.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise I'm good with this.

pydantic/main.py Outdated
@@ -366,6 +366,16 @@ def __setattr__(self, name, value):
elif self.__config__.validate_assignment:
known_field = self.__fields__.get(name, None)
if known_field:
new_values = {**self.__dict__, name: value}

for skip_on_failure, validator in self.__post_root_validators__:
Copy link
Member

Choose a reason for hiding this comment

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

this should be called after validating the field and not all skip_on_failure validators should be skipped.

Copy link
Member

Choose a reason for hiding this comment

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

so it's more like standard validation.

@PrettyWood PrettyWood force-pushed the validate-assignment-root-validator branch 2 times, most recently from a2effd0 to 7e8cc8c Compare October 9, 2020 22:24
@PrettyWood
Copy link
Member Author

PrettyWood commented Oct 10, 2020

@samuelcolvin You were right sorry I went too fast. I added both pre- and post-rootvalidators. The code is quite similar with validate_model but I feel like factorizing all this code would make things less explicit so I didn't go with the full DRY approach.

@samuelcolvin samuelcolvin merged commit 1736ea1 into pydantic:master Oct 25, 2020
@samuelcolvin
Copy link
Member

👍 thank you

@Bobronium Bobronium mentioned this pull request Oct 25, 2020
4 tasks
@caldwellshane
Copy link

🙏 thank you @PrettyWood and @samuelcolvin

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.

validate_assignment does not work with root_validator
3 participants