-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 sequence like validator with strict True #8977
fix sequence like validator with strict True #8977
Conversation
CodSpeed Performance ReportMerging #8977 will degrade performances by 19.35%Comparing Summary
Benchmarks breakdown
|
please review |
Ugh, looks like this is a reflection of the issues we currently have with sequence type validation / schema building. It's odd that we don't even use the I'd prefer something like this, as a patch fix for now:
Even if we resolve this with this patch (or something similar), I'd still like to keep an eye on this issue as an example of what's wrong with our sequence schema building! Thanks for your help! Also, re your xfail test, I think it is desired to have model config take precedence over runtime serialization flags, so I think the behavior that you're seeing is expected. |
Ah actually, here's a more simple fix: diff --git a/pydantic/_internal/_std_types_schema.py b/pydantic/_internal/_std_types_schema.py
index 5c61d8f0..90ed7b6d 100644
--- a/pydantic/_internal/_std_types_schema.py
+++ b/pydantic/_internal/_std_types_schema.py
@@ -288,7 +288,7 @@ class SequenceValidator:
item_source_type: type[Any]
min_length: int | None = None
max_length: int | None = None
- strict: bool = False
+ strict: bool | None = None
def serialize_sequence_via_list(
self, v: Any, handler: core_schema.SerializerFunctionWrapHandler, info: core_schema.SerializationInfo |
Another issue here that I stumbled upon during my testing is that you can't apply |
Any chance you'll be able to update this in the next few days? We'll be doing a new release soon, and I want to make sure we can get your fix in :). |
@sydney-runkle hey hello!, sure, are you requesting this change |
I think you should be able to just implement this 1 line diff to fix the issue (but definitely keep your tests)!
|
Nice i will review it once i finish working!. Im super curious, why that line makes the magic |
The issue here is that if you have a model with Thus, defaulting to |
ad435e1
to
b9dfaf2
Compare
@sydney-runkle i pushed the one line change and it's failing, maybe i missed something? I still not understanding how the |
Hmm, that's odd. I thought I had it working with just the one liner. I'll pull down the code tomorrow morning and take a closer work. Thanks for your work on this!! |
It is failing on a test i added, so probably your fix is ok! Will review it tomorrow as well |
It's failing on an existing test. Line 4748 in e58134b
I think is because deque and Counter are being validated using list_schema here
So with the fix, passing deque to the test model fails because it is expecting a list in strict mode. So I think currently deque and Counter are not supported to be strict . Now i remember that i added an overwrite metadata['strict'] = False in my original commit, in that part of the code to avoid forcing strict mode for those types. I don't know if that is expected anyways, if that fixes the issue i can add that line again!
|
Currently debugging. I can't reproduce the test failure locally, for some reason... |
@pytest.mark.xfail( | ||
reason='strict=True in model_validate_json does not overwrite strict=False given in ConfigDict' | ||
'See issue: https://github.com/pydantic/pydantic/issues/8930' | ||
) | ||
def test_model_validate_list_strict() -> None: | ||
# FIXME: This change must be implemented in pydantic-core. The argument strict=True | ||
# in model_validate_json method is not overwriting the one set with ConfigDict(strict=False) | ||
# for sequence like types. See: https://github.com/pydantic/pydantic/issues/8930 | ||
|
||
class LaxModel(BaseModel): | ||
x: List[str] | ||
model_config = ConfigDict(strict=False) | ||
|
||
assert LaxModel.model_validate_json(json.dumps({'x': ('a', 'b', 'c')}), strict=None) == LaxModel(x=('a', 'b', 'c')) | ||
assert LaxModel.model_validate_json(json.dumps({'x': ('a', 'b', 'c')}), strict=False) == LaxModel(x=('a', 'b', 'c')) | ||
with pytest.raises(ValidationError) as exc_info: | ||
LaxModel.model_validate_json(json.dumps({'x': ('a', 'b', 'c')}), strict=True) | ||
assert exc_info.value.errors(include_url=False) == [ | ||
{'type': 'list_type', 'loc': ('x',), 'msg': 'Input should be a valid list', 'input': ('a', 'b', 'c')} | ||
] | ||
|
||
|
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.
@davidhewitt, interesting rust bug 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.
Going to chat with @davidhewitt about this PR tomorrow and will get back to you :).
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.
AHHHHH I can reproduce now. Dumb error on my end :(
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 this is actually an issue with the deque
schema generation, not with the solution we've implemented :).
You're right, we have to add that strict override back for the list, but I don't think we need the other change to the initialization of |
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 now, great work!
Acknowledged benchmarks don't truly represent a regression (this is just bc this was opened before we moved the benchmarks to 3.12) |
Fix #8930
Change Summary
This PR aims to solve #8930. Until now, when
strict=True
is passed in the config dict, only the type of the elements of the sequence fields is forced. Now with this change, both the type of the sequence and the items will be validated strictly. This validation is done perfectly inpydantic-core
, the problem was that this information was not sent through the json schema topydantic-core
for sequence like types. However, themodel_validate_json
method, from what I understand, needs to be fixed inpydantic-core
. An example for the last is shown below and also there is test marked withxfail
in this PRThe example has no error,
strict=True
inmodel_validate_json
should overwrite config dictstrict=False
(To be honest, I make this statement looking at the numerous examples in the tests, where it is seen that it overwrites)Related issue number
#8930
Checklist
Selected Reviewer: @dmontagu