Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/serializers/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ pub(super) fn py_err_se_err<T: ser::Error, E: fmt::Display>(py_error: E) -> T {
T::custom(py_error.to_string())
}

/// Wrapper type which allows convenient conversion between `PyErr` and `ser::Error` in `?` expressions.
pub(super) struct WrappedSerError<T: ser::Error>(pub T);

impl<T: ser::Error> From<PyErr> for WrappedSerError<T> {
fn from(py_err: PyErr) -> Self {
WrappedSerError(T::custom(py_err.to_string()))
}
}

#[pyclass(extends=PyValueError, module="pydantic_core._pydantic_core")]
#[derive(Debug, Clone)]
pub struct PythonSerializerError {
Expand Down
2 changes: 1 addition & 1 deletion src/serializers/type_serializers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ pub mod with_default;

use super::computed_fields::ComputedFields;
use super::config::utf8_py_error;
use super::errors::{py_err_se_err, PydanticSerializationError};
use super::errors::{py_err_se_err, PydanticSerializationError, WrappedSerError};
use super::extra::{Extra, ExtraOwned, SerCheck, SerMode};
use super::fields::{FieldsMode, GeneralFieldsSerializer, SerField};
use super::filter::{AnyFilter, SchemaFilter};
Expand Down
160 changes: 97 additions & 63 deletions src/serializers/type_serializers/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -138,17 +140,80 @@ 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),
fn allow_value(&self, value: &Bound<'_, PyAny>, check: SerCheck) -> PyResult<bool> {
match check {
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>, check: SerCheck) -> PyResult<bool> {
match 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.)
Comment on lines +164 to +165
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// 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 🤔

Copy link
Contributor Author

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

///
/// 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> {
match self.root_model {
true if self.allow_value_root_model(value, extra.check)? => {
let root_extra = Extra {
field_name: Some(ROOT_FIELD),
model: Some(value),
..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, extra.check)? => {
let model_extra = Extra {
model: Some(value),
..extra.clone()
};
let inner_value = self.get_inner_value(value, &model_extra)?;
do_serialize(Some((&self.serializer, &inner_value, &model_extra)))
}
_ => do_serialize(None),
}
}

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 {
Expand Down Expand Up @@ -184,38 +249,18 @@ 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| match resolved {
Some((serializer, value, extra)) => 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>> {
if self.allow_value(key, extra)? {
// FIXME: root model in json key position should serialize as inner value?
if self.allow_value(key, extra.check)? {
infer_json_key_known(ObType::PydanticSerializable, key, extra)
} else {
extra.warnings.on_fallback_py(&self.name, key, extra)?;
Expand All @@ -231,30 +276,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| match resolved {
Some((cs, value, extra)) => 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 {
Expand Down
54 changes: 32 additions & 22 deletions tests/serializers/test_model_root.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
import json
import os
import platform
from pathlib import Path
from typing import Any, Union

import pytest

try:
from functools import cached_property
except ImportError:
cached_property = None


from pydantic_core import SchemaSerializer, core_schema

from ..conftest import plain_repr
Expand Down Expand Up @@ -181,27 +177,41 @@ class Model(RootModel):
assert s.to_json(m) == b'[1,2,{"value":"abc"}]'


def test_construct_nested():
class RModel(RootModel):
def test_not_root_model():
# https://github.com/pydantic/pydantic/issues/8963

class RootModel:
root: int

class BModel(BaseModel):
value: RModel
v = RootModel()
v.root = '123'

s = SchemaSerializer(
core_schema.model_schema(
BModel,
core_schema.model_fields_schema(
{
'value': core_schema.model_field(
core_schema.model_schema(RModel, core_schema.int_schema(), root_model=True)
)
}
),
)
RootModel,
core_schema.str_schema(),
root_model=True,
),
)

m = BModel(value=42)
assert s.to_python(v) == '123'
assert s.to_json(v) == b'"123"'

# Path is chosen because it has a .root property
# which could look like a root model in bad implementations

if os.name == 'nt':
path_value = Path('C:\\a\\b')
path_bytes = b'"C:\\\\a\\\\b"' # fixme double escaping?
else:
path_value = Path('/a/b')
path_bytes = b'"/a/b"'

with pytest.warns(UserWarning, match=r'PydanticSerializationUnexpectedValue\(Expected `RootModel`'):
assert s.to_python(path_value) == path_value

with pytest.warns(UserWarning, match=r'PydanticSerializationUnexpectedValue\(Expected `RootModel`'):
assert s.to_json(path_value) == path_bytes

with pytest.raises(AttributeError, match="'int' object has no attribute 'root'"):
s.to_python(m)
assert s.to_python(path_value, warnings=False) == path_value
assert s.to_json(path_value, warnings=False) == path_bytes
Loading
Loading