-
Notifications
You must be signed in to change notification settings - Fork 246
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
fix wrap
serializer breaking union serialization in presence of extra fields
#1530
Conversation
return Err(PydanticSerializationUnexpectedValue::new_err(None)); | ||
let type_name = field_extra.model_type_name(); | ||
return Err(PydanticSerializationUnexpectedValue::new_err(Some(format!( | ||
"Unexpected field `{key}`{for_type_name}", |
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 is a drive-by improvement to the warning which was necessary for me to debug.
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.
Reminds me of #1483
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.
Nice improvement though, thanks
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.
Should we go ahead and fix #1483 while we're at it 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.
Last comment here - we should do a quick search through the codebase to see if other (similar) warnings can be updated with this structure.
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 think I will punt on both of these and leave them for a later refactoring of serialization warnings.
@@ -778,3 +761,67 @@ class ModelB: | |||
model_b = ModelB(field=1) | |||
assert s.to_python(model_a) == {'field': 1, 'TAG': 'a'} | |||
assert s.to_python(model_b) == {'field': 1, 'TAG': 'b'} | |||
|
|||
|
|||
def test_union_model_wrap_serializer(): |
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.
Test fails on main
.
CodSpeed Performance ReportMerging #1530 will not alter performanceComparing Summary
|
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.
Looking good, just a few notes on the error wording. Thanks @davidhewitt !
}, | ||
}, | ||
}, | ||
core_schema.union_schema( |
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.
Much cleaner, thanks.
Change Summary
It turns out that
wrap
serializers can potentially need "retrying" in lax mode (as per the union logic), but this was not currently handled. This is necessary when a model contains unexpected extra fields; theStrict
mode will fail.Related issue number
Fixes pydantic/pydantic#10682
Checklist
pydantic-core
(except for expected changes)