Skip reusing wrap validators / serializers for prebuilt variants #1660
Conversation
| if matches!(schema_serializer.get().serializer, CombinedSerializer::FunctionWrap(_)) { | ||
| return Ok(None); | ||
| } |
There was a problem hiding this comment.
I'd rather we check against disallowed variants here rather than all allowed variants - it keeps things cleaner.
There was a problem hiding this comment.
Should we go further and check that schema's type matches the type of the serializer?
e.g. maybe
match &schema_serializer.get().serializer {
CombinedSerializer::Model(_) => // check schema[type] is model
... same for other expected types
}There was a problem hiding this comment.
I don't think so, because I think it would be acceptable to have a PlainSerialier, for example for a model?
There was a problem hiding this comment.
Could we perhaps write a test to demonstrate that case? (We can use the repr to show that a prebuilt serializer is used.)
There was a problem hiding this comment.
Absolutely, happy to!
There was a problem hiding this comment.
Done via 6efdfc9
CodSpeed Performance ReportMerging #1660 will not alter performanceComparing Summary
|
tests/test_prebuilt.py
Outdated
There was a problem hiding this comment.
These added tests fail on main because two 'modified' additions are made (double call of respective val / ser.
Reuse of wrap validators / serializers leads to some really odd behavior (double calls).
This PR fixes pydantic/pydantic#11505 and fixes #1651, where you can find some reproducible examples.
This bug was originally introduced by me in #1616 where we added reuse of schema validators and serializers to optimize memory in the case of reused models.
As discussed with DH, perhaps a cleaner approach going forward would be to use a new type of core schema to control reuse of schema validators and serializers. Though this avoids introducing edge case checks like this one in
pydantic-core, it just pushes the burden up topydantic, where things actually may be more complicated (ex - with schema cleaning + discriminator application or parametrized generics).Note: the failing integration tests are not related to this PR. I think it makes sense to add corresponding tests to
pydanticfor the realistic use case here - I'll do that when we bump the version after apydantic-corerelease.