Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Clean up serialization warnings for invalid data in unions #1449

Merged
merged 4 commits into from
Sep 16, 2024
+51 −80
Merged

Clean up serialization warnings for invalid data in unions #1449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8544612
warn, don't error
sydney-runkle Sep 12, 2024
d6bc0da
progress on warnings
sydney-runkle Sep 13, 2024
ff26f78
sort of flood with warnings
sydney-runkle Sep 13, 2024
d24aad5
fix tests
sydney-runkle Sep 16, 2024
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
10 changes: 9 additions & 1 deletion src/serializers/extra.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,15 @@ impl CollectWarnings {
if value.is_none() {
Ok(())
} else if extra.check.enabled() {
Err(PydanticSerializationUnexpectedValue::new_err(None))
let type_name = value
.get_type()
.qualname()
.unwrap_or_else(|_| PyString::new_bound(value.py(), "<unknown python object>"));

let value_str = truncate_safe_repr(value, None);
Err(PydanticSerializationUnexpectedValue::new_err(Some(format!(
"Expected `{field_type}` but got `{type_name}` with value `{value_str}` - serialized value may not be as expected"
))))
Comment on lines +395 to +403
Copy link
Member Author

Choose a reason for hiding this comment

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

Admittedly is a bit redundant with existing code - would like to standardize how we raise serialization errors in pydantic-core for the v2.10 release :)

Copy link
Member Author

Choose a reason for hiding this comment

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

This case shows why we might want to do that - could probably minimize the duplicates here:

from typing import Annotated, FrozenSet, Union, Literal, List

from pydantic import BaseModel, PlainSerializer, IPvAnyAddress, Field
from ipaddress import IPv4Network

SortedFrozenNetworkSet = Annotated[
    FrozenSet[IPv4Network],
    PlainSerializer(
        lambda x: sorted(str(network) for network in x),
        return_type=FrozenSet[str],
    ),
]


class ModelA(BaseModel):
    type: Literal["A"]
    addresses: SortedFrozenNetworkSet


class ModelB(BaseModel):
    type: Literal["B"]
    hostname: str
    address: IPvAnyAddress


AnnotatedUnionType = Annotated[
    Union[ModelA, ModelB],
    Field(discriminator="type")
]


class CompositeModel(BaseModel):
    composite_field: AnnotatedUnionType


data = {"composite_field": {"type": "A", "addresses": ["192.168.1.0/24", "127.0.0.1"]}}
model = CompositeModel(**data)
print("Deserialized:", model.model_dump_json())

"""
PydanticSerializationUnexpectedValue: Expected `frozenset[str]` but got `list` with value `['127.0.0.1/32', '192.168.1.0/24']` - serialized value may not be as expected
PydanticSerializationUnexpectedValue: Expected `ModelB` but got `ModelA` with value `ModelA(type='A', addresse...twork('127.0.0.1/32')}))` - serialized value may not be as expected
Expected `frozenset[str]` but got `list` with value `['127.0.0.1/32', '192.168.1.0/24']` - serialized value may not be as expected
"""

} else {
self.fallback_warning(field_type, value);
Ok(())
Expand Down
100 changes: 29 additions & 71 deletions src/serializers/type_serializers/union.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ use std::borrow::Cow;
use crate::build_tools::py_schema_err;
use crate::common::union::{Discriminator, SMALL_UNION_THRESHOLD};
use crate::definitions::DefinitionsBuilder;
use crate::serializers::type_serializers::py_err_se_err;
use crate::tools::{truncate_safe_repr, SchemaDict};
use crate::PydanticSerializationUnexpectedValue;

use super::{
infer_json_key, infer_serialize, infer_to_python, BuildSerializer, CombinedSerializer, Extra, SerCheck,
Expand Down Expand Up @@ -77,7 +75,6 @@ fn to_python(
exclude: Option<&Bound<'_, PyAny>>,
extra: &Extra,
choices: &[CombinedSerializer],
name: &str,
retry_with_lax_check: bool,
) -> PyResult<PyObject> {
// try the serializers in left to right order with error_on fallback=true
Expand All @@ -88,22 +85,15 @@ fn to_python(
for comb_serializer in choices {
match comb_serializer.to_python(value, include, exclude, &new_extra) {
Ok(v) => return Ok(v),
Err(err) => match err.is_instance_of::<PydanticSerializationUnexpectedValue>(value.py()) {
true => (),
false => errors.push(err),
},
Err(err) => errors.push(err),
}
}

if retry_with_lax_check {
new_extra.check = SerCheck::Lax;
for comb_serializer in choices {
match comb_serializer.to_python(value, include, exclude, &new_extra) {
Ok(v) => return Ok(v),
Err(err) => match err.is_instance_of::<PydanticSerializationUnexpectedValue>(value.py()) {
true => (),
false => errors.push(err),
},
if let Ok(v) = comb_serializer.to_python(value, include, exclude, &new_extra) {
return Ok(v);
}
}
}
Expand All @@ -112,15 +102,13 @@ fn to_python(
extra.warnings.custom_warning(err.to_string());
}

extra.warnings.on_fallback_py(name, value, extra)?;
infer_to_python(value, include, exclude, extra)
}

fn json_key<'a>(
key: &'a Bound<'_, PyAny>,
extra: &Extra,
choices: &[CombinedSerializer],
name: &str,
retry_with_lax_check: bool,
) -> PyResult<Cow<'a, str>> {
let mut new_extra = extra.clone();
Expand All @@ -130,22 +118,15 @@ fn json_key<'a>(
for comb_serializer in choices {
match comb_serializer.json_key(key, &new_extra) {
Ok(v) => return Ok(v),
Err(err) => match err.is_instance_of::<PydanticSerializationUnexpectedValue>(key.py()) {
true => (),
false => errors.push(err),
},
Err(err) => errors.push(err),
}
}

if retry_with_lax_check {
new_extra.check = SerCheck::Lax;
for comb_serializer in choices {
match comb_serializer.json_key(key, &new_extra) {
Ok(v) => return Ok(v),
Err(err) => match err.is_instance_of::<PydanticSerializationUnexpectedValue>(key.py()) {
true => (),
false => errors.push(err),
},
if let Ok(v) = comb_serializer.json_key(key, &new_extra) {
return Ok(v);
}
}
}
Expand All @@ -154,7 +135,6 @@ fn json_key<'a>(
extra.warnings.custom_warning(err.to_string());
}

extra.warnings.on_fallback_py(name, key, extra)?;
infer_json_key(key, extra)
}

Expand All @@ -166,7 +146,6 @@ fn serde_serialize<S: serde::ser::Serializer>(
exclude: Option<&Bound<'_, PyAny>>,
extra: &Extra,
choices: &[CombinedSerializer],
name: &str,
retry_with_lax_check: bool,
) -> Result<S::Ok, S::Error> {
let py = value.py();
Expand All @@ -177,22 +156,15 @@ fn serde_serialize<S: serde::ser::Serializer>(
for comb_serializer in choices {
match comb_serializer.to_python(value, include, exclude, &new_extra) {
Ok(v) => return infer_serialize(v.bind(py), serializer, None, None, extra),
Err(err) => match err.is_instance_of::<PydanticSerializationUnexpectedValue>(py) {
true => (),
false => errors.push(err),
},
Err(err) => errors.push(err),
}
}

if retry_with_lax_check {
new_extra.check = SerCheck::Lax;
for comb_serializer in choices {
match comb_serializer.to_python(value, include, exclude, &new_extra) {
Ok(v) => return infer_serialize(v.bind(py), serializer, None, None, extra),
Err(err) => match err.is_instance_of::<PydanticSerializationUnexpectedValue>(py) {
true => (),
false => errors.push(err),
},
if let Ok(v) = comb_serializer.to_python(value, include, exclude, &new_extra) {
return infer_serialize(v.bind(py), serializer, None, None, extra);
}
}
}
Expand All @@ -201,7 +173,6 @@ fn serde_serialize<S: serde::ser::Serializer>(
extra.warnings.custom_warning(err.to_string());
}

extra.warnings.on_fallback_ser::<S>(name, value, extra)?;
infer_serialize(value, serializer, include, exclude, extra)
}

Expand All @@ -219,13 +190,12 @@ impl TypeSerializer for UnionSerializer {
exclude,
extra,
&self.choices,
self.get_name(),
self.retry_with_lax_check(),
)
}

fn json_key<'a>(&self, key: &'a Bound<'_, PyAny>, extra: &Extra) -> PyResult<Cow<'a, str>> {
json_key(key, extra, &self.choices, self.get_name(), self.retry_with_lax_check())
json_key(key, extra, &self.choices, self.retry_with_lax_check())
}

fn serde_serialize<S: serde::ser::Serializer>(
Expand All @@ -243,7 +213,6 @@ impl TypeSerializer for UnionSerializer {
exclude,
extra,
&self.choices,
self.get_name(),
self.retry_with_lax_check(),
)
}
Expand Down Expand Up @@ -313,8 +282,6 @@ impl TypeSerializer for TaggedUnionSerializer {
exclude: Option<&Bound<'_, PyAny>>,
extra: &Extra,
) -> PyResult<PyObject> {
let py = value.py();

let mut new_extra = extra.clone();
new_extra.check = SerCheck::Strict;

Expand All @@ -325,15 +292,14 @@ impl TypeSerializer for TaggedUnionSerializer {

match serializer.to_python(value, include, exclude, &new_extra) {
Ok(v) => return Ok(v),
Err(err) => match err.is_instance_of::<PydanticSerializationUnexpectedValue>(py) {
true => {
if self.retry_with_lax_check() {
new_extra.check = SerCheck::Lax;
return serializer.to_python(value, include, exclude, &new_extra);
Err(_) => {
if self.retry_with_lax_check() {
new_extra.check = SerCheck::Lax;
if let Ok(v) = serializer.to_python(value, include, exclude, &new_extra) {
return Ok(v);
}
}
false => return Err(err),
},
}
}
}
}
Expand All @@ -344,13 +310,11 @@ impl TypeSerializer for TaggedUnionSerializer {
exclude,
extra,
&self.choices,
self.get_name(),
self.retry_with_lax_check(),
)
}

fn json_key<'a>(&self, key: &'a Bound<'_, PyAny>, extra: &Extra) -> PyResult<Cow<'a, str>> {
let py = key.py();
let mut new_extra = extra.clone();
new_extra.check = SerCheck::Strict;

Expand All @@ -361,20 +325,19 @@ impl TypeSerializer for TaggedUnionSerializer {

match serializer.json_key(key, &new_extra) {
Ok(v) => return Ok(v),
Err(err) => match err.is_instance_of::<PydanticSerializationUnexpectedValue>(py) {
true => {
if self.retry_with_lax_check() {
new_extra.check = SerCheck::Lax;
return serializer.json_key(key, &new_extra);
Err(_) => {
if self.retry_with_lax_check() {
new_extra.check = SerCheck::Lax;
if let Ok(v) = serializer.json_key(key, &new_extra) {
return Ok(v);
}
}
false => return Err(err),
},
}
}
}
}

json_key(key, extra, &self.choices, self.get_name(), self.retry_with_lax_check())
json_key(key, extra, &self.choices, self.retry_with_lax_check())
}

fn serde_serialize<S: serde::ser::Serializer>(
Expand All @@ -396,18 +359,14 @@ impl TypeSerializer for TaggedUnionSerializer {

match selected_serializer.to_python(value, include, exclude, &new_extra) {
Ok(v) => return infer_serialize(v.bind(py), serializer, None, None, extra),
Err(err) => match err.is_instance_of::<PydanticSerializationUnexpectedValue>(py) {
true => {
if self.retry_with_lax_check() {
new_extra.check = SerCheck::Lax;
match selected_serializer.to_python(value, include, exclude, &new_extra) {
Ok(v) => return infer_serialize(v.bind(py), serializer, None, None, extra),
Err(err) => return Err(py_err_se_err(err)),
}
Err(_) => {
if self.retry_with_lax_check() {
new_extra.check = SerCheck::Lax;
if let Ok(v) = selected_serializer.to_python(value, include, exclude, &new_extra) {
return infer_serialize(v.bind(py), serializer, None, None, extra);
}
}
false => return Err(py_err_se_err(err)),
},
}
}
}
}
Expand All @@ -419,7 +378,6 @@ impl TypeSerializer for TaggedUnionSerializer {
exclude,
extra,
&self.choices,
self.get_name(),
self.retry_with_lax_check(),
)
}
Expand Down
5 changes: 1 addition & 4 deletions tests/serializers/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@

from pydantic_core import (
PydanticSerializationError,
PydanticSerializationUnexpectedValue,
SchemaSerializer,
SchemaValidator,
core_schema,
Expand Down Expand Up @@ -1150,8 +1149,6 @@ class BModel(BasicModel): ...
)
)

with pytest.raises(
PydanticSerializationUnexpectedValue, match='Expected 2 fields but got 1 for type `.*AModel` with value `.*`.+'
):
with pytest.warns(UserWarning, match='Expected 2 fields but got 1 for type `.*AModel` with value `.*`.+'):
value = BasicModel(root=AModel(type='a'))
s.to_python(value)
16 changes: 12 additions & 4 deletions tests/serializers/test_union.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import dataclasses
import json
import re
import uuid
import warnings
from decimal import Decimal
from typing import Any, ClassVar, Union

Expand Down Expand Up @@ -32,9 +32,17 @@ def test_union_bool_int(input_value, expected_value, bool_case_label, int_case_l

def test_union_error():
s = SchemaSerializer(core_schema.union_schema([core_schema.bool_schema(), core_schema.int_schema()]))
msg = "Expected `Union[bool, int]` but got `str` with value `'a string'` - serialized value may not be as expected"
with pytest.warns(UserWarning, match=re.escape(msg)):
assert s.to_python('a string') == 'a string'

messages = [
"Expected `bool` but got `str` with value `'a string'` - serialized value may not be as expected",
"Expected `int` but got `str` with value `'a string'` - serialized value may not be as expected",
]

with warnings.catch_warnings(record=True) as w:
warnings.simplefilter('always')
s.to_python('a string')
for m in messages:
assert m in str(w[0].message)


class ModelA:
Expand Down
Loading