Decode enums that mix in str or int and int enum dict keys - #1900
RaphaelFakhri wants to merge 1 commit into
Conversation
|
|
|
CLA needs to be signed in order to proceed. |
There was a problem hiding this comment.
AI Review, ignore
Thanks for tackling the silent mis-decode of (str, Enum) / (int, Enum) mixins — that fall-through into the iterable branch turning "foo" into ["foo"] is a nasty footgun, and aligning with the existing IntEnum/StrEnum paths makes sense.
What looks good
- Checking
issubclass(hint, Enum) and issubclass(hint, int|str)before the iterable branch correctly intercepts mixin enums. - Dict-key path: coercing JSON string keys to
intbefore the enum lookup for int-mixin enums matches how JSON round-tripsdict[IntEnum, ...]. - Tests cover mixin int/str enums, dict keys, and a non-string rejection for the str-mixin case.
Suggestions / questions
- Dict keys typed as str-mixin enums — JSON keys are already strings, so
{SerializableMixinStrEnum.FOO: 1}should work without a special branch (and your test covers it). Worth a one-line comment next to the int-enum key branch clarifying that str-mixin keys intentionally rely on the existing string path, so a future reader does not "fix" them into anint()conversion. fail(SerializableMixinIntEnum, "1")— you assert non-string rejection for the str mixin (fail(..., 5)), but not the symmetric case of a string payload for an int mixin. JSON sometimes delivers numbers as strings in odd converters; an explicit fail (or an intentional accept-via-int()if you want that) would lock the contract.- Order of int vs str branches — an enum that somehow mixes both (
class Weird(int, str, Enum)) would hit the int branch first. Extremely rare, but a brief comment that int is checked first would help.
Overall this looks correct and ready pending the small test/docs nits above.
|
Signed accordingly |
|
Post release, the changelog entry needs to be in unreleased rather than the previous version. |
30a1697 to
15c95a3
Compare
What was changed
value_to_typenow decodes any enum that mixes instrorint, not onlyStrEnumandIntEnum. It also decodesdictkeys typed as an int enum.class Color(str, Enum)decoded with aColortype hint fell through to the iterable branch, becausestris iterable."foo"became["foo"]with no error. The value is now converted withColor("foo"), and a non-string value raises aTypeError.class Level(int, Enum)raisedUnserializable type during conversion. It now decodes likeIntEnum.dict[IntEnum, ...]failed to decode because JSON writes the key as the string"1", and the key path only converted strings for plainintandfloatkey types. Int enum keys are now converted tointbefore the enum lookup.Updates the README type list and adds a CHANGELOG entry.
Why
Enums declared as
(str, Enum)are common and were the pattern StrEnum replaced. The maintainer agreed on the linked issue (#676) that this combination has a clear serialization. That issue was closed without a code change, and the current behavior silently returns the wrong type.Testing
Added cases to
test_json_type_hintsintests/test_converter.py.pytest tests/test_converter.pyfails withFailed converting key '1' to type <enum 'SerializableEnum'>, and(str, Enum)decoding returns['foo'].pytest tests/test_converter.py tests/contrib/pydanticpasses.ruff check --select Iandruff format --checkpass.