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

marshalling nested enums from python strings raises a TypeError #103

Closed
software-dov opened this issue Aug 17, 2020 · 4 comments
Closed
Assignees
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@software-dov
Copy link
Contributor

This requires marshaling from python structures.

Minimal reproduction:

import proto

class Zone(proto.Enum):
    LITTORAL = 1
    MESOPELAGIC = 2
    ABYSSAL = 3
    HADEAN = 4

class Squid(proto.Message):
    zone = proto.Field(Zone, number=1)

class Trawl(proto.Message):
    squids = proto.RepeatedField(Squid, number=1)   # Note: this indirection with the nested field
                                                    # is necessary to trigger the exception.
                                                    # Setting the field in an existing message accepts strings AND
                                                    # checks for valid variants.
                                                    # Similarly, constructing a message directly with a top level 
                                                    # enum field kwarg passed as a string is also handled correctly, i.e.
                                                    # s = Squid(zone="ABYSSAL")
                                                    # does NOT raise an exception.

t = Trawl(squids=[{"zone": "HADEAN"}])

generates

TypeError: 'HADEAN' has type str, but expected one of: int, long

This is related to #52, and most of the implementation will probably be shared.

@software-dov software-dov added P2 A nice-to-fix bug type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Aug 17, 2020
@software-dov
Copy link
Contributor Author

A workaround is to not use strings directly but instead to use Enum class attributes.
E.g., assuming the above example class definitions,

t = Trawl(squids=[{"zone": Zone.HADEAN}, {"zone": Zone.LITTORAL}])

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Aug 18, 2020
@software-dov software-dov removed the triage me I really want to be triaged. label Aug 18, 2020
@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Aug 18, 2020
@software-dov software-dov removed 🚨 🚨 This issue needs some love. triage me I really want to be triaged. labels Aug 27, 2020
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. triage me I really want to be triaged. labels Aug 27, 2020
@hkdevandla
Copy link
Member

Making this as p3 considering that there is a workaround. @software-dov , adjust this if you disagree. thanks!

@hkdevandla hkdevandla added priority: p3 Desirable enhancement or fix. May not be included in next release. and removed P2 A nice-to-fix bug 🚨 This issue needs some love. 🚨 triage me I really want to be triaged. labels Aug 28, 2020
@software-dov
Copy link
Contributor Author

There is a workaround, and the issue is fixed by #107

@software-dov
Copy link
Contributor Author

Fixe

@busunkim96 busunkim96 reopened this Sep 8, 2020
software-dov added a commit that referenced this issue Sep 10, 2020
For real, this time.
Third party, unwrapped enums no longer cause problems
if they are used to describe proto-plus fields.

Limitations:
* Third party enums cannot be passed by string name to a Field
* More scenarios require a module definition than previously
    * Defining a proto.Enum may cause a FileDescriptor to be added to the
       descriptor pool. The only way to defer until _all_ Enums and
       Messages in a module have been added is to provide a manifest or
       make the Enum a nested type of a Message.
       This mostly affects unit tests and interactive development.

Includes re-addition of stringy enums in JSON.
Includes re-addition of fix for #103

Release-As: 1.10.0-dev1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

4 participants