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

detailed_validation=False doesn't work for GenConverter instance #245

Closed
blthayer opened this issue Apr 5, 2022 · 4 comments · Fixed by #247
Closed

detailed_validation=False doesn't work for GenConverter instance #245

blthayer opened this issue Apr 5, 2022 · 4 comments · Fixed by #247

Comments

@blthayer
Copy link

blthayer commented Apr 5, 2022

  • cattrs version: 22.1.0
  • Python version: 3.9.11
  • Operating System: Ubuntu 20.04.4 LTS

Description

Using detailed_validation=False with a GenConverter breaks my application after upgrading to cattrs 22.1.0, but everything works swimingly with detailed_validation=True.

What I Did

Changed from:

CONVERTER = cattrs.GenConverter()

to:

CONVERTER = cattrs.GenConverter(detailed_validation=False)

At the moment, I don't have time to get a minimum reproducible example put together, and I can't share my code or full stack traces.

However, I can shore portions of it. It seems that the error occurs in the generated code that cattrs creates for my GenConverter instance. Specifically, it looks like the method signature for instantiating my attrs classes is different under detailed_validation=True vs. False, but I'm not 100% sure.

Here's where the error occurs in cattrs:

    def _structure_list(self, obj, cl):
        """Convert an iterable to a potentially generic list."""
        if is_bare(cl) or cl.__args__[0] is Any:
            res = [e for e in obj]
        else:
            elem_type = cl.__args__[0]
            handler = self._structure_func.dispatch(elem_type)
            if self.detailed_validation:
                errors = []
                res = []
                ix = 0  # Avoid `enumerate` for performance.
                for e in obj:
                    try:
                        res.append(handler(e, elem_type))
                    except Exception as e:
                        e.__note__ = f"Structuring {cl} @ index {ix}"
                        errors.append(e)
                    finally:
                        ix += 1
                if errors:
                    raise IterableValidationError(
                        f"While structuring {cl!r}", errors, cl
                    )
            else:
>               res = [handler(e, elem_type) for e in obj]

../path/to/my/environment/lib/python3.9/site-packages/cattrs/converters.py:474: 

The error message seems to relate to bad arguments to the class signature:

E     TypeError: __init__() takes 1 positional argument but 5 positional arguments (and 8 keyword-only arguments) were given

<cattrs generated structure REDACTED>:19: TypeError

If I find time, I'll try to get a minimal example together that I can share, but don't hold your breath :)

@Tinche
Copy link
Member

Tinche commented Apr 5, 2022

I added a few tests for structuring attrs classes but couldn't repro it, so I'll need some more information ;)

@blthayer
Copy link
Author

blthayer commented Apr 5, 2022

Okay, took longer than I would have hoped, but I have a MWE for you:

from typing import Optional, Union

import cattrs
from attrs import define, field

CONVERTER = cattrs.GenConverter(detailed_validation=False)


@define(auto_attribs=True, kw_only=True, frozen=False, hash=None, slots=True)
class classA:
    # Either "A" or "B"
    id_: str = field()


@define(auto_attribs=True, kw_only=True, frozen=False, hash=None, slots=True)
class classB(classA):
    # Not always present during structuring:
    other_field: Optional[str] = field(default=None)


AOrB = Union[classA, classB]


@define(auto_attribs=True, kw_only=True, frozen=False, hash=None, slots=True)
class Nested:
    a_or_b: AOrB = field()


def structure_a_or_b(dict_: dict, _):
    if dict_["id_"] == "A":
        cls = classA
    else:
        cls = classB

    return CONVERTER.structure(dict_, cls)


CONVERTER.register_structure_hook(AOrB, structure_a_or_b)


if __name__ == "__main__":
    a_dict = {"a_or_b": {"id_": "A"}}
    b_dict = {"a_or_b": {"id_": "B"}}
    a_inst = CONVERTER.structure(a_dict, Nested)
    b_inst = CONVERTER.structure(b_dict, Nested)

Output:

/path/to/my/env/bin/python /path/to/script/tmp.py
Traceback (most recent call last):
  File "/path/to/script/tmp.py", line 44, in <module>
    a_inst = CONVERTER.structure(a_dict, Nested)
  File "/path/to/my/env/lib/python3.9/site-packages/cattrs/converters.py", line 281, in structure
    return self._structure_func.dispatch(cl)(obj, cl)
  File "<cattrs generated structure __main__.Nested>", line 3, in structure_Nested
    __c_structure_a_or_b(o['a_or_b'], __c_type_a_or_b),
  File "/path/to/my/env/lib/python3.9/site-packages/cattrs/converters.py", line 536, in _structure_union
    return handler(obj, union)
  File "/path/to/script/tmp.py", line 35, in structure_a_or_b
    return CONVERTER.structure(dict_, cls)
  File "/path/to/my/env/bin/python/lib/python3.9/site-packages/cattrs/converters.py", line 281, in structure
    return self._structure_func.dispatch(cl)(obj, cl)
  File "<cattrs generated structure __main__.classA>", line 2, in structure_classA
    return __cl(
TypeError: __init__() takes 1 positional argument but 2 were given

Process finished with exit code 1

And if you toggle detailed_validation=True, there's no error.

@Tinche
Copy link
Member

Tinche commented Apr 5, 2022

Ah, the problem is that you're using kw_only=True. That's not officially supported, and the fact that it works when using detailed validation is a happy coincidence.

You're not the first person to come asking for this, so adding explicit support for kw_only is probably the first thing on my agenda for the next release. You can also take a look at #234 for tips on implementing your own hooks (that can support kw_only).

@blthayer
Copy link
Author

blthayer commented Apr 5, 2022

Thanks for getting back to me, and thanks for the reference! Looking forward to a future release with kw_only supported, thanks again!

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 a pull request may close this issue.

2 participants