get item type in get_annotation_from_field_info #909
Conversation
Codecov Report
@@ Coverage Diff @@
## master #909 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 16 16
Lines 2766 2791 +25
Branches 528 540 +12
=====================================
+ Hits 2766 2791 +25
Continue to review full report at Codecov.
|
| unused_constraints = [ | ||
| f for f in validation_attribute_to_schema_keyword if f not in attrs and getattr(field_info, f) is not None | ||
| ] | ||
| if unused_constraints: |
There was a problem hiding this comment.
@dmontagu @tiangolo I need your feedback/advice on this.
I think we should tell the user if they add constraints but they're not applied, however are there situations where a custom could set a constraint which would show in schema, but not be applied here? I couldn't find any in tests, but that doesn't mean there aren't any.
If so we might need a config argument or extra argument to Field() to skip this check.
There was a problem hiding this comment.
I can't think of any, but it feels a little strong to me to raise an error rather than a warning or having a flag to allow it or similar.
That said, specifying schema_extra would work as a way to add these to the schema without validation if you really wanted to right?
If that's right, I think this is fine, but maybe it would be worth calling this out in the error message?
More generally, I think it would be nice if it were easier to implement the constraint_func for the class and have pydantic pick it up, so you could create custom classes with custom validation as a function of the specified schema. But that is probably outside the scope of what should be handled now.
There was a problem hiding this comment.
More generally, I think it would be nice if it were easier to implement the constraint_func for the class and have pydantic pick it up, so you could create custom classes with custom validation as a function of the specified schema. But that is probably outside the scope of what should be handled now.
Surely that should be done via type annotations?
If that's right, I think this is fine, but maybe it would be worth calling this out in the error message?
Yes agreed.
There was a problem hiding this comment.
after the changes I've just made, I can't think of a scenario where constraints would be valid in schema, but not enforced.
To add an example to the docs and an explanation of the work around we need at least one example where this error would be incorrectly raised :-) done
There was a problem hiding this comment.
I don't have strong opinions about this.
I see how it could feel cumbersome if it suddenly raises errors for models that didn't before.
But at the same time, I can imagine someone expecting it to show validation errors for some data with constraints in Field that are not actually applied. And I can see how they could forget to check the warnings on the logs...
Something I can think of to mitigate these use cases is, in the docs, add an alternative with pure Field for those cases, I suggested it in the PR review.
Another idea that comes to mind is to show a warning first as @dmontagu suggests, and make it a deprecation warning. And in some next version, throw an error. Just an idea, not that I think that's necessary, both approaches seem fine to me.
pydantic/schema.py
Outdated
| elif issubclass(origin, Tuple): # type: ignore | ||
| for i, t in enumerate(args): | ||
| if t is Ellipsis: | ||
| return args[0] |
There was a problem hiding this comment.
What if it is a tuple of optionals? (The same goes for the various container types below.)
There was a problem hiding this comment.
yes, agreed this could all be way more powerful, but would become massively more complex. Hence the error above so at least you know when it does and doesn't work.
pydantic/schema.py
Outdated
| args: Tuple[Any, ...] = annotation.__args__ | ||
|
|
||
| if origin is Union: | ||
| if sum(a is not NoneType for a in args) == 1: # type: ignore |
There was a problem hiding this comment.
This feels kind of fragile, for example, what if you had Optional[Union[str, bytes]]? It seems like there is no reason the get_annotation_from_field_info function shouldn't be able to handle that, but this would reject it.
I'm wondering if maybe there is a way to refactor this pair of functions so that it is applied on a per-union-member basis? That might also make handling more robust for the container types below.
|
oh, after going through this, I ended up rewriting the entire thing. Type analysis should now work recursively. I along the way, I discovered:
|
samuelcolvin
force-pushed
the
field-validator-fix
branch
from
c47096a to
162dfd7
Compare
samuelcolvin
force-pushed
the
field-validator-fix
branch
from
162dfd7 to
f7a096b
Compare
* get item type in get_annotation_from_field_info, fix pydantic#779 * check constraints are enforced * add change * tests for phony constraints * rewrite get_annotation_from_field_info * fix tests * add constaint enforcement to docs * fix get_annotation_from_field_info coverage * fix linting * update docs as per @tiangolo suggestions
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Change Summary
modify
get_annotation_from_field_infoto look at sub-types of typing objects likeUnionorListand thus correctly apply validators.Related issue number
fix #779
Checklist
changes/<pull request or issue id>-<github username>.mdfile added describing change(see changes/README.md for details)