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

Enum misuse causes unhelpful error #601

Open
AstraLuma opened this issue Nov 20, 2024 · 1 comment
Open

Enum misuse causes unhelpful error #601

AstraLuma opened this issue Nov 20, 2024 · 1 comment

Comments

@AstraLuma
Copy link

  • cattrs version: 24.1.2
  • Python version: 3.11
  • Operating System: Linux

Description

Use an enum's value as the default of a class (instead of the enum reference) causes an unhelpful backtrace.

tbh, I'm not entirely sure what the fix here is, but based on how much effort I put into finding this mistake, I think there's room for some improvement in developer experience.

What I Did

This code:

class SiteFlavors(enum.Enum):
    SIMPLE = "simple"


@attrs.define
class Site():
    flavor: SiteFlavors = "simple"

attrs.unstructure(Site())

Causes this error:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/astraluma/code/teahouse/barista/.venv/lib/python3.12/site-packages/cattrs/converters.py", line 238, in unstructure
    return self._unstructure_func.dispatch(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<cattrs generated unstructure __main__.Site>", line 3, in unstructure_Site
  File "/home/astraluma/code/teahouse/barista/.venv/lib/python3.12/site-packages/cattrs/converters.py", line 359, in _unstructure_enum
    return obj.value
           ^^^^^^^^^
AttributeError: 'str' object has no attribute 'value'

which is obvious when reduced, but in my case, the usage was spread out between a definition, usage, and a wrapping library, so it looked more like:

  File "/app/src/barista/routes/sites.py", line 44, in delete_site
    await sitedb.attempt_delete(site)
  File "/usr/local/lib/python3.13/site-packages/chaise/__init__.py", line 441, in attempt_delete
    _, db, docid, etag = self._doc2blob(doc)
                         ~~~~~~~~~~~~~~^^^^^
  File "/usr/local/lib/python3.13/site-packages/chaise/__init__.py", line 308, in _doc2blob
    blob = self._session.loader().dumpj(doc)
  File "/usr/local/lib/python3.13/site-packages/chaise/__init__.py", line 129, in dumpj
    blob = self.dump_doc(doc)
  File "/usr/local/lib/python3.13/site-packages/chaise/attrs.py", line 194, in dump_doc
    return converter.unstructure(doc)
           ~~~~~~~~~~~~~~~~~~~~~^^^^^
  File "/usr/local/lib/python3.13/site-packages/cattrs/converters.py", line 238, in unstructure
    return self._unstructure_func.dispatch(
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        obj.__class__ if unstructure_as is None else unstructure_as
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    )(obj)
    ~^^^^^
  File "<cattrs generated unstructure barista.models.Site>", line 4, in unstructure_Site
    'flavor': __c_unstr_flavor(instance.flavor),
              ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.13/site-packages/cattrs/converters.py", line 359, in _unstructure_enum
    return obj.value
           ^^^^^^^^^
AttributeError: 'str' object has no attribute 'value'
@Tinche
Copy link
Member

Tinche commented Nov 23, 2024

Sorry you had a bad experience using cattrs!

Let me try organizing my thoughts.

The most immediate thing is that your code example isn't correct - a string is not a valid value for a field of an enum type.

One of the main ideas of cattrs is this concept of validation at the edges - we do work when data enters the system, and only then. Inside the system you're expected to use a type checker, unit tests or whatever other means of ensuring correctness. This assumption is why cattrs does next to no validation when unstructuring - it's assuming the data is correct and goes for speed. This is what's been biting you here; no explicit validation means bad error messages.

That said, some codebases might not be able to ensure this, or maybe they rely on libraries that can't ensure this, or something else. I wouldn't change the default behavior, but I think implementing a strategy, that can be applied to a converter and that would enable more validation when unstructuring, would be a good idea. Especially since users would be free to, for example, apply the strategy conditionally - maybe when running in debug mode, maybe when running unit tests, maybe when running in a staging environment, maybe always.

I think this would be a good addition to cattrs.

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

No branches or pull requests

2 participants