python-attrs / cattrs

Composable custom class converters for attrs, dataclasses and friends.
https://catt.rs
MIT License
829 stars 113 forks source link

Enum misuse causes unhelpful error #601

Open AstraLuma opened 6 days ago

AstraLuma commented 6 days ago

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 commented 3 days ago

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.