Skip to content

Make validating assignment work properly with allowed extra #766

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

Merged
merged 4 commits into from
Jul 13, 2023
Merged

Make validating assignment work properly with allowed extra #766

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
85589a4
Make validating assignment work properly with allowed extra
dmontagu Jul 12, 2023
c91609a
Undo clippy-related changes
dmontagu Jul 13, 2023
37fa39f
Update _pydantic_core.pyi
dmontagu Jul 13, 2023
d39c591
Merge main
dmontagu Jul 13, 2023
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
5 changes: 4 additions & 1 deletion python/pydantic_core/_pydantic_core.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,10 @@ class SchemaValidator:
strict: bool | None = None,
from_attributes: bool | None = None,
context: 'dict[str, Any] | None' = None,
) -> dict[str, Any]: ...
) -> dict[str, Any] | tuple[dict[str, Any], dict[str, Any] | None, set[str]]:
"""
ModelValidator and ModelFieldsValidator will return a tuple of (fields data, extra data, fields set)
"""
def get_default_value(self, *, strict: bool | None = None, context: Any = None) -> Some | None: ...

_IncEx: TypeAlias = set[int] | set[str] | dict[int, _IncEx] | dict[str, _IncEx] | None
Expand Down
25 changes: 17 additions & 8 deletions src/validators/model.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,32 +183,41 @@ impl Validator for ModelValidator {
Ok(model.into_py(py))
};
}
let dict: &PyDict = model.getattr(intern!(py, DUNDER_DICT))?.downcast()?;
let old_dict: &PyDict = model.getattr(intern!(py, DUNDER_DICT))?.downcast()?;

let new_dict = dict.copy()?;
new_dict.set_item(field_name, field_value)?;
let input_dict = old_dict.copy()?;
let old_extra: Option<&PyDict> = model.getattr(intern!(py, DUNDER_MODEL_EXTRA_KEY))?.downcast().ok();
if let Some(old_extra) = old_extra {
input_dict.update(old_extra.as_mapping())?;
}
input_dict.set_item(field_name, field_value)?;

let output = self.validator.validate_assignment(
py,
new_dict,
input_dict,
field_name,
field_value,
extra,
definitions,
recursion_guard,
)?;

let (output, _, updated_fields_set): (&PyDict, &PyAny, &PySet) = output.extract(py)?;
let (validated_dict, validated_extra, validated_fields_set): (&PyDict, &PyAny, &PySet) = output.extract(py)?;

if let Ok(fields_set) = model.getattr(intern!(py, DUNDER_FIELDS_SET_KEY)) {
let fields_set: &PySet = fields_set.downcast()?;
for field_name in updated_fields_set {
for field_name in validated_fields_set {
fields_set.add(field_name)?;
}
}
let output = output.to_object(py);

force_setattr(py, model, intern!(py, DUNDER_DICT), output)?;
force_setattr(py, model, intern!(py, DUNDER_DICT), validated_dict.to_object(py))?;
force_setattr(
py,
model,
intern!(py, DUNDER_MODEL_EXTRA_KEY),
validated_extra.to_object(py),
)?;
Ok(model.into_py(py))
}

Expand Down
26 changes: 21 additions & 5 deletions src/validators/model_fields.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,13 +302,13 @@ impl Validator for ModelFieldsValidator {
) -> ValResult<'data, PyObject> {
let dict: &PyDict = obj.downcast()?;

let ok = |output: PyObject| {
let get_updated_dict = |output: PyObject| {
dict.set_item(field_name, output)?;
Ok(dict.to_object(py))
Ok(dict)
};

let prepare_result = |result: ValResult<'data, PyObject>| match result {
Ok(output) => ok(output),
Ok(output) => get_updated_dict(output),
Err(ValError::LineErrors(line_errors)) => {
let errors = line_errors
.into_iter()
Expand Down Expand Up @@ -358,7 +358,7 @@ impl Validator for ModelFieldsValidator {
Some(ref validator) => {
prepare_result(validator.validate(py, field_value, &extra, definitions, recursion_guard))
}
None => ok(field_value.to_object(py)),
None => get_updated_dict(field_value.to_object(py)),
},
ExtraBehavior::Forbid | ExtraBehavior::Ignore => {
return Err(ValError::new_with_loc(
Expand All @@ -372,8 +372,24 @@ impl Validator for ModelFieldsValidator {
}
}?;

let new_extra = match &self.extra_behavior {
ExtraBehavior::Allow => {
let non_extra_data = PyDict::new(py);
self.fields.iter().for_each(|f| {
let popped_value = PyAny::get_item(new_data, &f.name).unwrap();
new_data.del_item(&f.name).unwrap();
non_extra_data.set_item(&f.name, popped_value).unwrap();
});
let new_extra = new_data.copy()?;
new_data.clear();
new_data.update(non_extra_data.as_mapping())?;
new_extra.to_object(py)
}
_ => py.None(),
};
Comment on lines +375 to +389
Copy link
Collaborator Author

@dmontagu dmontagu Jul 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I feel there must be a better way to achieve this — @davidhewitt maybe you have some suggestions?

The idea — I'm trying to take new_data, which will have all key-value pairs for fields and extra, and split them into two dicts — new_data which has only field values, and new_extra which has everything else.

Copy link
Member

Choose a reason for hiding this comment

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

This looks pretty sound to me on the whole

Copy link
Contributor

Choose a reason for hiding this comment

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

You're modifying new_data so I don't think you need to create two new dicts. The challenge seems to be that you only have the list of known fields, so you are forced to remove them from new_data.

How about swapping the binding over like this:

Suggested change
let new_extra = match &self.extra_behavior {
ExtraBehavior::Allow => {
let non_extra_data = PyDict::new(py);
self.fields.iter().for_each(|f| {
let popped_value = PyAny::get_item(new_data, &f.name).unwrap();
new_data.del_item(&f.name).unwrap();
non_extra_data.set_item(&f.name, popped_value).unwrap();
});
let new_extra = new_data.copy()?;
new_data.clear();
new_data.update(non_extra_data.as_mapping())?;
new_extra.to_object(py)
}
_ => py.None(),
};
let (new_data, new_extra) = match &self.extra_behavior {
ExtraBehavior::Allow => {
// Move non-extra keys out of new_data, leaving just the extra in new_data
let non_extra_data = PyDict::new(py);
for field in &self.fields
let popped_value = PyAny::get_item(new_data, &field.name).unwrap();
new_data.del_item(&f.name).unwrap();
non_extra_data.set_item(&f.name, popped_value).unwrap();
}
(non_extra_data, new_data.to_object())
}
// FIXME do you need to throw if `new_data` contains any extra keys?
_ => (new_data, py.None()),
};

Copy link
Collaborator Author

@dmontagu dmontagu Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think we check previously for any possible source of extra keys (and error), so I think we can ignore that potential issue here. So this looks good and we can drop the comment.

Copy link
Collaborator Author

@dmontagu dmontagu Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just kidding, this breaks some tests because we currently assume in some places that validate_assignment does an in-place modification to the fields dict, but this makes it become the extra dict. I'm not sure what the ideal behavior is here but I am inclined to leave as it was before for now, and we can revisit this in a separate PR if/when it seems worthwhile.


let fields_set: &PySet = PySet::new(py, &[field_name.to_string()])?;
Ok((new_data, py.None(), fields_set.to_object(py)).to_object(py))
Ok((new_data.to_object(py), new_extra, fields_set.to_object(py)).to_object(py))
Copy link
Contributor

Choose a reason for hiding this comment

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

The return value of validate_assignment in _pydantic_core.pyi is dict[str, Any], but here we have a 3-tuple.

a) Should we change the return value of validate_assignment in _pydantic_core.pyi?
b) Should we change the definition of validate_assignment in trait Validator to return (PyObject, PyObject, PyObject)? This will enforce all the Rust implementation to behave correctly and avoid needing to go in and out of a Python tuple until we get to the top level.

EDIT based on what I see in the tests, I think the answer to both of these is "yes".

Copy link
Member

Choose a reason for hiding this comment

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

Should we change the definition of validate_assignment in trait Validator to return (PyObject, PyObject, PyObject)? This will enforce all the Rust implementation to behave correctly and avoid needing to go in and out of a Python tuple until we get to the top level.

Some historical perspective: validate_assignment and validate used to be the same thing, there was just a boolean flag being thrown around internally to differentiate the "mode". So the fact that the return is a PyObject instead of a tuple of PyObject is likely just an artifact of that original implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This validate_assignment is also used for dataclasses, and I think we need it to be a dict[str, Any] for that case. I updated the type hint to be a union. I agree this probably deserves some cleanup but I think it's not as straightforward as "always return a tuple".

}

fn different_strict_behavior(
Expand Down
4 changes: 4 additions & 0 deletions tests/validators/test_function.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,9 @@ class Model:
__slots__ = '__dict__', '__pydantic_fields_set__', '__pydantic_extra__', '__pydantic_private__'
field_a: str

def __init__(self):
self.__pydantic_extra__ = None # this attribute must be present for validate_assignment

v = SchemaValidator(
core_schema.no_info_after_validator_function(
f,
Expand All @@ -474,6 +477,7 @@ class Model:
assert m.field_a == 'test'
assert m.__pydantic_fields_set__ == {'field_a'}
assert m.__dict__ == {'field_a': 'test', 'more': 'foobar'}
assert m.__pydantic_extra__ is None

m2 = Model()
m2.field_a = 'test'
Expand Down
8 changes: 7 additions & 1 deletion tests/validators/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,6 +943,9 @@ class MyModel:
field_a: str
field_b: int

def __init__(self):
self.__pydantic_extra__ = None

v = SchemaValidator(
{
'type': 'model',
Expand Down Expand Up @@ -1019,7 +1022,10 @@ def func(x, info):

def test_validate_assignment_no_fields_set():
class MyModel:
__slots__ = ('__dict__',)
__slots__ = ('__dict__', '__pydantic_extra__')

def __init__(self):
self.__pydantic_extra__ = None

v = SchemaValidator(
{
Expand Down
16 changes: 9 additions & 7 deletions tests/validators/test_model_fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,8 +347,8 @@ def test_validate_assignment_allow_extra():
assert v.validate_python({'field_a': 'test'}) == ({'field_a': 'test'}, {}, {'field_a'})

assert v.validate_assignment({'field_a': 'test'}, 'other_field', 456) == (
{'field_a': 'test', 'other_field': 456},
None,
{'field_a': 'test'},
{'other_field': 456},
{'other_field'},
)
Comment on lines 349 to 353
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on this it looks like the type annotation in _pydantic_core.pyi needs to be updated to a 3-tuple.


Expand All @@ -364,8 +364,8 @@ def test_validate_assignment_allow_extra_validate():
)

assert v.validate_assignment({'field_a': 'test'}, 'other_field', '456') == (
{'field_a': 'test', 'other_field': 456},
None,
{'field_a': 'test'},
{'other_field': 456},
{'other_field'},
)

Expand Down Expand Up @@ -1682,10 +1682,12 @@ def test_extra_behavior_allow(
assert fields_set == {'f', 'extra_field'}

v.validate_assignment(m, 'f', 'y')
assert m['f'] == 'y'
assert m == {'f': 'y'}

v.validate_assignment(m, 'not_f', '123')
assert m['not_f'] == expected_extra_value
new_m, new_model_extra, new_fields_set = v.validate_assignment({**m, **model_extra}, 'not_f', '123')
assert new_m == {'f': 'y'}
assert new_model_extra == {'extra_field': expected_extra_value, 'not_f': expected_extra_value}
assert new_fields_set == {'not_f'}


@pytest.mark.parametrize(
Expand Down