-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Move core schema generation logic for path types inside the GenerateSchema
class
#10846
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
Conversation
Deploying pydantic-docs with
|
Latest commit: |
b2b8f7e
|
Status: | ✅ Deploy successful! |
Preview URL: | https://b439c509.pydantic-docs.pages.dev |
Branch Preview URL: | https://perf-remove-prep-anns.pydantic-docs.pages.dev |
CodSpeed Performance ReportMerging #10846 will not alter performanceComparing Summary
|
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
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.
What does this tell us about our annotation application in general?
I think it reveals a limitation of our current annotation application. Because paths could be considered as a custom type (i.e. there's no core schema that represents this type, we currently build the validation logic with a custom after validator), we face the limitation of not having access to the annotated metadata as described here.
The current min/max_length_validator
function is not going to scale well in the future if we ever want to add support for min/max_length
for another type similar to Path
(and that type similarly doesn't have a core schema type for it).
I'll note that while this limitation shows up here with a type without a proper core schema for it, we also face weird behaviors with the following (and unlike Path
, int
has a core schema type):
def val(v):
print('val called')
return v
class Model(BaseModel):
a: Annotated[int, AfterValidator(val), Field(gt=0)]
Model(a=-1)
#> val called
#> ValidationError raised
- the after validator is called before the
gt
validation, which imo is not what should happen. - as a consequence, the
gt
constraint isn't represented in theint_schema()
, but instead thegreater_than_validator
Python fallback validator function is used.
On another note, maybe we should discuss how we validate "concrete" vs "abstract" types. For instance, for paths, we use pathlib.PurePath
for os.PathLike
annotations, but this is an opinionated choice and users could expect Path
to be used instead.
This relates to the behavior of Iterable
: we currently validate these by creating a ValidationIterator
instance, and people complained about it. Maybe we should have a way of customizing which "concrete" implementation should be used for "abstract" types?
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Path
types
Path
typesGenerateSchema
class
…Schema` class (pydantic#10846) Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Some (verbose) context: #10036
I'm trying to remove the use of
prepare_pydantic_annotations_for_known_type
internally. We call this function in order to to pull metadata off of types and inject said metadata's constraints into core schema. This is bad practice, as we generally advertise being religious about respecting metadata order when building schema, but we violate that principle here.Though these checks don't drag much on performance, they do significantly clutter our internal schema generation logic. They represent the old pattern (we have since deprecated) of supporting a
__pydantic_prepare_annotations__
method.For example, we used to pull string constraint data like
min_length
off of aPath
like annotation + apply that to the innercore_schema.str_schema()
orcore_schema.bytes_schema()
.This removes support for constraints like
max_length
,min_length
, andstrip_whitespace
or other string constraints onPath
types. Users should implement these constraints with custom validators. This should be advertised in the v2.11 changelog.