aggregate root validation errors #1586
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1586 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 3796 3815 +19
Branches 752 759 +7
=========================================
+ Hits 3796 3815 +19
Continue to review full report at Codecov.
|
|
Appreciate any suggestions about what documentation to update. It seems to me the behavior at present is not currently documented, and since the change brings root validators in line with the behavior of attribute validators, I'm having a hard time coming up with an intuitive way to highlight the new behavior. Is there a place where we call this behavior (aggregation of errors) out for attribute validators explicitly? Perhaps changing the wording to reflect all validators there would be the most impactful way to communicate the new behavior. |
|
I would add something to https://pydantic-docs.helpmanual.io/usage/validators/#root-validators Just a note ( |
|
I think this might be captured by changing the language " root validators by default will be called even if field validation fails; this behaviour can be changed by setting the skip_on_failure=True keyword argument to the validator." to " root validators by default will be called even if prior validation fails; this behaviour can be changed by setting the skip_on_failure=True keyword argument to the validator." The new language expands the scope of "things that can fail prior to a given root validator without preventing that root validator from running", from an original scope of field validators, to a new scope of field validators plus any other root validators that run prior to the root validator in question. It also correctly captures that That said if you think the note is worthwhile just for the sake of higher visibility, I'm happy to put one in addition, I can see that the language change I'm suggesting is subtle despite accuracy. To your last point, while the library doesn't do anything for a user in terms of tracking success or failure of prior root validators, there's technically nothing stopping someone from attaching extra data to the underlying values dictionary as they return updated dictionaries from their root validators, allowing subsequent validators to check for presence of these values as signal of success, or absence as signal of failure. From the view of pydantic, I'd say we're un-opinionated in this respect, it's not impossible but we're not helping you do it in any meaningful way. Open to your thoughts there and happy to include whatever language around this you might feel worthwhile, fwiw I wouldn't find it problematic to not mention at all given the passive role the library has with respect to this nuance. |
|
Honestly not a response I was expecting - I don't know if you intend hostility here, but this doesn't come off as "contributions welcome" to me |
|
Sorry it if it seemed hostile, it wasn't meant as such. Just that I'm dyslexic and long responses take me a long time to parse, as you can see from the volume of issues on pydantic, avoiding such time sinks is helpful. I actually skim-read your message above, but missed the part about duplication. That's my fault, but also kind of an example of what I was saying. I'll look into this more soon and let you know what I think. |
|
Thanks and sorry if I read wrong. I'll take your feedback to heart and continue striving for brevity without sacrificing precision. |
|
awesome, thank you. |
Change Summary
This change enables aggregation of multiple root validation errors.
It turns out deduplication of root validators is necessary in the metaclass, since it appears
__new__is called twice for generics, once at class definition, and another when type arguments are applied (seems like semantically python is creating an on-the-fly subclass with the type variables unified,) the second of which results in all root validators defined on the class itself appearing in a base for the type-arg-unified subclass as well as on the type-arg-unified subclass itself, hence duplicating all of them. The issue is caught by current generic tests, but was masked by the short-circuiting (duplication of root validators was not apparent because root validation was halted after the first failure.)Added test coverage to make sure multiple root validators can fail simultaneously.
Related issue number
#1571
Checklist
changes/<pull request or issue id>-<github username>.mdfile added describing change(see changes/README.md for details)