-
Notifications
You must be signed in to change notification settings - Fork 306
Validate enum values against other possible forms of expected values #1502
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
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #1502 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1502 +/- ##
==========================================
- Coverage 90.21% 89.39% -0.83%
==========================================
Files 106 112 +6
Lines 16339 17966 +1627
Branches 36 40 +4
==========================================
+ Hits 14740 16060 +1320
- Misses 1592 1886 +294
- Partials 7 20 +13
... and 53 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
This issue lies mostly in validating JSON values. Take the case in the issue as an example: from enum import Enum
class A(Enum):
a = 1, 2
serialised = RootModel[A](A.a).model_dump_json()
parsed = RootModel[A].model_validate_json(serialised) # fail What happens here is that My initial commit does fix this issue by adding class Model(BaseModel):
x: int
@model_serializer
def ser_model(self):
return [self.x]
class A(Enum):
a = 2, 3
b = Model(x=1)
serialised = RootModel[A](A.b).model_dump_json() # [1]
parsed = RootModel[A].model_validate_json(serialised) # fail To fix this issue completely, we need to know the serialised form of all enum values and add them to the validator. My initial fix solves the problem because tuples are serialised as JSON arrays and casting tuples to lists covers this case. The problem for me now is that I don't know how to find the corresponding serialisers given a By the way, the following case is expected: class MyEnum(Enum):
a = 1, 2
b = [1, 2]
v = SchemaValidator(core_schema.enum_schema(MyEnum, list(MyEnum.__members__.values())))
assert v.validate_json('[1, 2]') is MyEnum.a Since both |
I think you might want to be looking in |
Hi @davidhewitt, I've finally come up with a seemingly generalisable solution, but I would like to check with you and see if I missed out anything. The idea is basically that we create a separate lookup in # root model with enum round trip
class Model(BaseModel):
x: int
@model_serializer
def ser_model(self):
return [self.x]
class A(Enum):
e = "1+2j"
a = 2, 3
b = complex(1, 2)
c = datetime.datetime.fromisoformat("2024-01-01T00:00:00Z")
d = Model(x=2)
f = float('inf')
for v in A.__members__.values():
RootModel[A].model_validate_json(RootModel[A](v).model_dump_json())
# base model with enum round trip
class M(BaseModel):
v: A
#model_config = ConfigDict(ser_json_inf_nan='strings', use_enum_values=True)
for v in A.__members__.values():
M.model_validate_json(M(v=v).model_dump_json()) I'm not quite sure about calling At the same time, I have some questions regarding implementations around pydantic_core._pydantic_core.ValidationError: 1 validation error for M
v
Input should be '1+2j', (2, 3), (1+2j), datetime.datetime(2024, 1, 1, 0, 0, tzinfo=datetime.timezone.utc), Model(x=2) or inf [type=enum, input_value='Infinity', input_type=str] because the serialized value in By the way, I'm still listing the original values in the error message instead of listing them in the JSON form because I'm not sure if showing the JSON form would be confusing. Sorry if this is not very clear because I'm in a bit of a rush, but please let me know what you think. Thank you! |
please review |
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.
Thanks for this, and sorry it took me so long to review here.
I think your solution with having a jsonified lookup is elegant and seems correct to me.
The issue with float validation comes from pydantic/pydantic#10037. Maybe I can do something to come up with a fix for that.
&ser_config.timedelta_mode.to_string(), | ||
&ser_config.bytes_mode.to_string(), | ||
&ser_config.inf_nan_mode.to_string(), |
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.
We should probably refactor to_jsonable_python
to take these values as enum members rather than strings, to avoid this round-trip.
let py_lookup = LiteralLookup::new(py, expected_py.into_iter())?; | ||
let json_lookup = LiteralLookup::new(py, expected_json.into_iter())?; |
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.
To minimize memory usage we should probably do something here to avoid having json_lookup
if the two lookups are identical.
Change Summary
Validate enum values against other possible forms of expected values, specifically tuples. The problem is actually bigger than what the initial commit solves. Please see my comment.
Related issue number
Fixes pydantic/pydantic#10629
Checklist
pydantic-core
(except for expected changes)Selected Reviewer: @sydney-runkle