-
Notifications
You must be signed in to change notification settings - Fork 309
fix various RootModel
serialization issues
#1836
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
let inner_value = self.get_inner_value(value, &model_extra)?; | ||
do_serialize(Some((&self.serializer, &inner_value)), &model_extra) | ||
} | ||
_ => do_serialize(None, &model_extra), |
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.
I wonder if it's a bug that the fallback uses the model_extra
? (This preserved existing behaviour.)
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.
Do you have a MRE that hits this code path? I'm having trouble understanding what is this logic for here.
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.
Looks like only field serializers care about the extra.model
, I'm going to tidy up and not worry.
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.
(this would be a case that warns about expecting a model and not getting it, and then somehow falling to type inference which calls a field serializer. I'm pretty sure that's impossible.)
/// apply any type inference. (`Model` serialization is strongly coupled with its child | ||
/// serializer, and in the few cases where `serialize_as_any` applies, it is handled here.) |
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.
/// apply any type inference. (`Model` serialization is strongly coupled with its child | |
/// serializer, and in the few cases where `serialize_as_any` applies, it is handled here.) | |
/// apply any type inference. (`Model` serialization is strongly coupled with its child | |
/// serializer, and in the few cases where `serialize_as_any` applies, it is handled here.) |
I wondered if it actually made sense to have the 'model'
core schema having an inner 'schema'
item, that should be of type 'model-fields'
although it is typed as any CoreSchema
in the TypedDict definitions 🤔
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.
It exists for "wrap" model serializers (and maybe "before"), in both of those cases the inference should not be used there either.
If I was improving this (maybe another day):
- I would make root model schema completely separate, it works so differently.
- model schema would directly take the fields, no
model_fields
schema - model schema would contain any model validators / serializers as special keys in the schema
This would also be much better for performance, the critical model pathways could then just produce the model values without having to go through CombinedValidator
/ CombinedSerializer
abstractions
CodSpeed Performance ReportMerging #1836 will not alter performanceComparing Summary
|
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
The failing integration test is not a concern IMO, the case currently raises an |
Change Summary
Adds more testing and fixes to two bugs with
RootModel
:field_serializer
not respected withserialize_as_any
in 2.12RootModel
no longer serializes incompatible types with a "root" field, e.g.Path
Related issue number
Xref pydantic/pydantic#12379
Fixes pydantic/pydantic#8963
Checklist
pydantic-core
(except for expected changes)