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 failure to load SecretStr
from JSON (regression in v2.4)
#7729
Conversation
Deploying with Cloudflare Pages
|
Please review 😄 |
SecretStr
from JSON (regression in v2.4)
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.
Overall looks like the right fix to me, two quick thoughts:
tests/test_types.py
Outdated
f1 = Foobar(password='1234', empty_password='') | ||
f2 = Foobar.model_validate_json('{"password": "1234", "empty_password": ""}') | ||
|
||
for f in [f1, f2]: |
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.
It may be slightly nicer to turn this into a parameterized test rather than have one of f1
or f2
fail inside the loop
strict=True, | ||
custom_error_type=error_kind, | ||
), | ||
json_schema=json_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.
This might want to have a custom_error_schema
and use custom_error_type
in some form on this branch. Possibly worth testing the JSON validation error with the wrong type.
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.
(As discussed elsewhere,) I think given the specific format of the errors seems good (added to the test below), this isn't necessary.
Change Summary
Ensure that a
SecretStr
type can be populated from json.Related issue number
Fix #7720
Checklist
Selected Reviewer: @samuelcolvin