Skip to content
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

[Feature Request] Support (str, Enum) like StrEnum #676

Open
sayevsky opened this issue Oct 24, 2024 · 5 comments
Open

[Feature Request] Support (str, Enum) like StrEnum #676

sayevsky opened this issue Oct 24, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@sayevsky
Copy link

What are you really trying to do?

I'm trying to use enum class to pass it to activity as an input.

Describe the bug

Activity deserializes bytes to list of chars instead of given Enum

Minimal Reproduction

from enum import Enum

from temporalio.api.common.v1.message_pb2 import Payload
from temporalio.converter import JSONPlainPayloadConverter

class SomeEnum(str, Enum):
    ONE = "one"
    TWO = "two"



converter = JSONPlainPayloadConverter()
result = converter.from_payload(Payload(metadata={
    "key": b"encoding",
    "value": b"json/plain"},
    data=b"\"one\""),
    SomeEnum)
print(result)

Output:

['o', 'n', 'e']

Environment/Versions

  • OS and processor: [e.g. M1 Mac, x86 Windows, Linux]
  • M1 Mac, though any architecture impacted
  • Temporal Version: [e.g. 1.14.0?] and/or SDK version
    SDK version is 1.6.0
  • Are you using Docker or Kubernetes or building Temporal from source?
    No

Additional context

JSONPlainPayloadConverter deserializes IntEnum, StrEnum correctly, but it is more reliable to convert payload to any Enum

@sayevsky sayevsky added the bug Something isn't working label Oct 24, 2024
@cretz
Copy link
Member

cretz commented Oct 24, 2024

Per https://github.com/temporalio/sdk-python?tab=readme-ov-file#data-conversion and https://docs.temporal.io/develop/python/converters-and-encryption#payload-conversion, only StrEnum and IntEnum are supported directly. Can you extend from one of those?

@sayevsky
Copy link
Author

sayevsky commented Oct 25, 2024

Hello, @cretz, thank you for the quick follow up.
Is the decision by not supporting basic Enum intentional? Are there any known corner-cases that stop adding additional functionality?

I can extend, but it is seems, that I need to copy-paste the whole value_to_type into my project. That is because types could be generic and value_to_type traverse the type recursively.

@sayevsky
Copy link
Author

sayevsky commented Oct 25, 2024

As proposal:

...
# StrEnum, available in 3.11+
    if sys.version_info >= (3, 11):
        if inspect.isclass(hint) and issubclass(hint, StrEnum):
            if not isinstance(value, str):
                raise TypeError(
                    f"Cannot convert to enum {hint}, value not a string, value is {type(value)}"
                )
            return hint(value)
# proposal to add the code:
    if inspect.isclass(hint) and issubclass(hint, Enum):
        if not isinstance(value, str):
            raise TypeError(
                f"Cannot convert to enum {hint}, value not a string, value is {type(value)}"
            )
        return hint(value)

@cretz
Copy link
Member

cretz commented Oct 25, 2024

Is the decision by not supporting basic Enum intentional?

It is because there is no clear serialization of plain Enum. But there is for (str, Enum) together so we can probably support that combo (though we, and Python, suggest using StrEnum now if you can). Your proposal is close, but we probably also need to also check that it's a subclass of both str and Enum and not just Enum. We'll look into adding this, thanks!

@cretz cretz changed the title [Bug] Enum deserialization is wrong [Feature Request] Support (str, Enum) like StrEnum Oct 25, 2024
@cretz cretz added enhancement New feature or request and removed bug Something isn't working labels Oct 25, 2024
@sayevsky
Copy link
Author

Similar enhancement for (int, Enum) would be nice. And if both checks do not match, then throw Exception. Because current behaviour is not obvious.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants