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.

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

Serialize submodels as unions without erroring #626

Closed
wants to merge 1 commit into from

Conversation

dmontagu
Copy link
Collaborator

This resolves the issue discussed in pydantic/pydantic#5839. But I'm not sure it's the right way to handle it. Looking for feedback before fixing tests/etc.

@@ -200,7 +200,7 @@ impl TypeSerializer for GeneralFieldsSerializer {
let value = infer_to_python(value, next_include, next_exclude, &extra)?;
output_dict.set_item(key, value)?;
} else if extra.check.enabled() {
return Err(PydanticSerializationUnexpectedValue::new_err(None));
// return Err(PydanticSerializationUnexpectedValue::new_err(None));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this line is erroring if there is an unexpected extra field, and we shouldn't do that if we want to be able to serialize subclasses (which might have extra fields).

@@ -110,7 +110,7 @@ fn has_extra(schema: &PyDict, config: Option<&PyDict>) -> PyResult<bool> {
impl ModelSerializer {
fn allow_value(&self, value: &PyAny, extra: &Extra) -> PyResult<bool> {
match extra.check {
SerCheck::Strict => Ok(value.get_type().is(self.class.as_ref(value.py()))),
SerCheck::Strict => value.is_instance(self.class.as_ref(value.py())),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think in strict mode we allow subclasses during validation (we had some discussion about this and a controversial twitter poll). I am thinking the same logic should imply that we allow subclasses during serialization as well.

@codspeed-hq
Copy link

codspeed-hq bot commented May 24, 2023

CodSpeed Performance Report

Merging #626 serialize-submodels (5c63b51) will not alter performances.

Summary

🔥 0 improvements
❌ 0 regressions
✅ 120 untouched benchmarks

🆕 0 new benchmarks
⁉️ 0 dropped benchmarks

@samuelcolvin
Copy link
Member

replaced by #632

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants