-
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
Changes from 2 commits
f332b8f
a2f3452
d86b0e1
5435229
2166b68
00200d2
6ada18b
309f09a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,14 +9,16 @@ use ahash::AHashMap; | |
use pyo3::IntoPyObjectExt; | ||
|
||
use super::{ | ||
infer_json_key, infer_json_key_known, infer_serialize, infer_to_python, py_err_se_err, BuildSerializer, | ||
CombinedSerializer, ComputedFields, Extra, FieldsMode, GeneralFieldsSerializer, ObType, SerCheck, SerField, | ||
TypeSerializer, | ||
infer_json_key, infer_json_key_known, infer_serialize, infer_to_python, BuildSerializer, CombinedSerializer, | ||
ComputedFields, Extra, FieldsMode, GeneralFieldsSerializer, ObType, SerCheck, SerField, TypeSerializer, | ||
WrappedSerError, | ||
}; | ||
use crate::build_tools::py_schema_err; | ||
use crate::build_tools::{py_schema_error_type, ExtraBehavior}; | ||
use crate::definitions::DefinitionsBuilder; | ||
use crate::serializers::errors::PydanticSerializationUnexpectedValue; | ||
use crate::serializers::type_serializers::any::AnySerializer; | ||
use crate::serializers::type_serializers::function::FunctionPlainSerializer; | ||
use crate::serializers::type_serializers::function::FunctionWrapSerializer; | ||
use crate::tools::SchemaDict; | ||
|
||
const ROOT_FIELD: &str = "root"; | ||
|
@@ -139,16 +141,77 @@ fn has_extra(schema: &Bound<'_, PyDict>, config: Option<&Bound<'_, PyDict>>) -> | |
|
||
impl ModelSerializer { | ||
fn allow_value(&self, value: &Bound<'_, PyAny>, extra: &Extra) -> PyResult<bool> { | ||
let class = self.class.bind(value.py()); | ||
match extra.check { | ||
SerCheck::Strict => Ok(value.get_type().is(class)), | ||
SerCheck::Lax => value.is_instance(class), | ||
SerCheck::Strict => Ok(value.get_type().is(&self.class)), | ||
SerCheck::Lax => value.is_instance(self.class.bind(value.py())), | ||
SerCheck::None => value.hasattr(intern!(value.py(), "__dict__")), | ||
} | ||
} | ||
|
||
fn allow_value_root_model(&self, value: &Bound<'_, PyAny>, extra: &Extra) -> PyResult<bool> { | ||
match extra.check { | ||
SerCheck::Strict => Ok(value.get_type().is(&self.class)), | ||
SerCheck::Lax | SerCheck::None => value.is_instance(self.class.bind(value.py())), | ||
} | ||
} | ||
|
||
/// Performs serialization for the model. This handles | ||
/// - compatibility checks | ||
/// - extracting the inner value for root models | ||
/// - applying `serialize_as_any` where needed | ||
/// | ||
/// `do_serialize` should be a function which performs the actual serialization, and should not | ||
/// 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.) | ||
/// | ||
/// If the value is not applicable, `do_serialize` will be called with `None` to indicate fallback | ||
/// behaviour should be used. | ||
fn serialize<T, E: From<PyErr>>( | ||
&self, | ||
value: &Bound<'_, PyAny>, | ||
extra: &Extra, | ||
do_serialize: impl FnOnce(Option<(&Arc<CombinedSerializer>, &Bound<'_, PyAny>)>, &Extra) -> Result<T, E>, | ||
) -> Result<T, E> { | ||
let model = Some(value); | ||
let model_extra = Extra { model, ..*extra }; | ||
|
||
match self.root_model { | ||
true if self.allow_value_root_model(value, &model_extra)? => { | ||
let root_extra = Extra { | ||
field_name: Some(ROOT_FIELD), | ||
..model_extra.clone() | ||
}; | ||
let root = value.getattr(intern!(value.py(), ROOT_FIELD))?; | ||
|
||
// for root models, `serialize_as_any` may apply unless a `field_serializer` is used | ||
let serializer = if root_extra.serialize_as_any | ||
&& !matches!( | ||
self.serializer.as_ref(), | ||
CombinedSerializer::Function(FunctionPlainSerializer { | ||
is_field_serializer: true, | ||
.. | ||
}) | CombinedSerializer::FunctionWrap(FunctionWrapSerializer { | ||
is_field_serializer: true, | ||
.. | ||
}), | ||
) { | ||
AnySerializer::get() | ||
} else { | ||
&self.serializer | ||
}; | ||
|
||
do_serialize(Some((serializer, &root)), &root_extra) | ||
} | ||
false if self.allow_value(value, &model_extra)? => { | ||
let inner_value = self.get_inner_value(value, &model_extra)?; | ||
do_serialize(Some((&self.serializer, &inner_value)), &model_extra) | ||
} | ||
_ => do_serialize(None, &model_extra), | ||
|
||
} | ||
} | ||
|
||
fn get_inner_value<'py>(&self, model: &Bound<'py, PyAny>, extra: &Extra) -> PyResult<Bound<'py, PyAny>> { | ||
let py = model.py(); | ||
let py: Python<'_> = model.py(); | ||
let mut attrs = model.getattr(intern!(py, "__dict__"))?.downcast_into::<PyDict>()?; | ||
|
||
if extra.exclude_unset { | ||
|
@@ -184,37 +247,17 @@ impl TypeSerializer for ModelSerializer { | |
exclude: Option<&Bound<'_, PyAny>>, | ||
extra: &Extra, | ||
) -> PyResult<Py<PyAny>> { | ||
let model = Some(value); | ||
|
||
let model_extra = Extra { model, ..*extra }; | ||
if self.root_model { | ||
let field_name = Some(ROOT_FIELD); | ||
let root_extra = Extra { | ||
field_name, | ||
..model_extra | ||
}; | ||
let py = value.py(); | ||
let root = value.getattr(intern!(py, ROOT_FIELD)).map_err(|original_err| { | ||
if root_extra.check.enabled() { | ||
PydanticSerializationUnexpectedValue::new_from_msg(None).to_py_err() | ||
} else { | ||
original_err | ||
} | ||
})?; | ||
self.serializer.to_python(&root, include, exclude, &root_extra) | ||
} else if self.allow_value(value, &model_extra)? { | ||
let inner_value = self.get_inner_value(value, &model_extra)?; | ||
// There is strong coupling between a model serializer and its child, we should | ||
// not fall back to type inference in the middle. | ||
self.serializer | ||
.to_python_no_infer(&inner_value, include, exclude, &model_extra) | ||
} else { | ||
extra.warnings.on_fallback_py(self.get_name(), value, &model_extra)?; | ||
infer_to_python(value, include, exclude, &model_extra) | ||
} | ||
self.serialize(value, extra, |resolved, extra| match resolved { | ||
Some((serializer, value)) => serializer.to_python_no_infer(value, include, exclude, extra), | ||
None => { | ||
extra.warnings.on_fallback_py(self.get_name(), value, extra)?; | ||
infer_to_python(value, include, exclude, extra) | ||
} | ||
}) | ||
} | ||
|
||
fn json_key<'a>(&self, key: &'a Bound<'_, PyAny>, extra: &Extra) -> PyResult<Cow<'a, str>> { | ||
// FIXME: root model in json key position should serialize as inner value? | ||
if self.allow_value(key, extra)? { | ||
infer_json_key_known(ObType::PydanticSerializable, key, extra) | ||
} else { | ||
|
@@ -231,30 +274,19 @@ impl TypeSerializer for ModelSerializer { | |
exclude: Option<&Bound<'_, PyAny>>, | ||
extra: &Extra, | ||
) -> Result<S::Ok, S::Error> { | ||
let model = Some(value); | ||
let model_extra = Extra { model, ..*extra }; | ||
if self.root_model { | ||
let field_name = Some(ROOT_FIELD); | ||
let root_extra = Extra { | ||
field_name, | ||
..model_extra | ||
}; | ||
let py = value.py(); | ||
let root = value.getattr(intern!(py, ROOT_FIELD)).map_err(py_err_se_err)?; | ||
self.serializer | ||
.serde_serialize(&root, serializer, include, exclude, &root_extra) | ||
} else if self.allow_value(value, &model_extra).map_err(py_err_se_err)? { | ||
let inner_value = self.get_inner_value(value, &model_extra).map_err(py_err_se_err)?; | ||
// There is strong coupling between a model serializer and its child, we should | ||
// not fall back to type inference in the midddle. | ||
self.serializer | ||
.serde_serialize_no_infer(&inner_value, serializer, include, exclude, &model_extra) | ||
} else { | ||
extra | ||
.warnings | ||
.on_fallback_ser::<S>(self.get_name(), value, &model_extra)?; | ||
infer_serialize(value, serializer, include, exclude, &model_extra) | ||
} | ||
self.serialize(value, extra, |resolved, extra| match resolved { | ||
Some((cs, value)) => cs | ||
.serde_serialize_no_infer(value, serializer, include, exclude, extra) | ||
.map_err(WrappedSerError), | ||
None => { | ||
extra | ||
.warnings | ||
.on_fallback_ser::<S>(self.get_name(), value, extra) | ||
.map_err(WrappedSerError)?; | ||
infer_serialize(value, serializer, include, exclude, extra).map_err(WrappedSerError) | ||
} | ||
}) | ||
.map_err(|e| e.0) | ||
} | ||
|
||
fn get_name(&self) -> &str { | ||
|
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 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 anyCoreSchema
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):
model_fields
schemaThis 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