Skip to content

Decode enums that mix in str or int and int enum dict keys - #1900

Open
RaphaelFakhri wants to merge 1 commit into
temporalio:mainfrom
RaphaelFakhri:fix-str-enum-decoding
Open

RaphaelFakhri wants to merge 1 commit into
temporalio:mainfrom
RaphaelFakhri:fix-str-enum-decoding

Conversation

@RaphaelFakhri

Copy link
Copy Markdown

What was changed

value_to_type now decodes any enum that mixes in str or int, not only StrEnum and IntEnum. It also decodes dict keys typed as an int enum.

  • A payload for class Color(str, Enum) decoded with a Color type hint fell through to the iterable branch, because str is iterable. "foo" became ["foo"] with no error. The value is now converted with Color("foo"), and a non-string value raises a TypeError.
  • class Level(int, Enum) raised Unserializable type during conversion. It now decodes like IntEnum.
  • dict[IntEnum, ...] failed to decode because JSON writes the key as the string "1", and the key path only converted strings for plain int and float key types. Int enum keys are now converted to int before 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_hints in tests/test_converter.py.

  • Without the source change: pytest tests/test_converter.py fails with Failed converting key '1' to type <enum 'SerializableEnum'>, and (str, Enum) decoding returns ['foo'].
  • With the change: pytest tests/test_converter.py tests/contrib/pydantic passes.
  • ruff check --select I and ruff format --check pass.

@RaphaelFakhri
RaphaelFakhri requested a review from a team as a code owner September 29, 2026 10:15
@CLAassistant

CLAassistant commented Sep 29, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@tconley1428

Copy link
Copy Markdown
Contributor

CLA needs to be signed in order to proceed.

@KingLizard1020 KingLizard1020 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 int before the enum lookup for int-mixin enums matches how JSON round-trips dict[IntEnum, ...].
  • Tests cover mixin int/str enums, dict keys, and a non-string rejection for the str-mixin case.

Suggestions / questions

  1. 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 an int() conversion.
  2. 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.
  3. 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.

@RaphaelFakhri

Copy link
Copy Markdown
Author

Signed accordingly

@tconley1428

Copy link
Copy Markdown
Contributor

Post release, the changelog entry needs to be in unreleased rather than the previous version.

@RaphaelFakhri
RaphaelFakhri force-pushed the fix-str-enum-decoding branch from 30a1697 to 15c95a3 Compare October 3, 2026 16:23

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants