diff --git a/HISTORY.md b/HISTORY.md index f81acc7a..93552416 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -13,6 +13,8 @@ Our backwards-compatibility policy can be found [here](https://github.com/python ## NEXT (UNRELEASED) +- Fix {func}`make_dict_unstructure_fn ` ignoring hooks registered on the converter (like the methods picked up by the {func}`use_class_methods ` strategy) when no customizations are requested; hook factories built on top of it now compose with those hooks instead of silently dropping them. + ([#566](https://github.com/python-attrs/cattrs/issues/566)) - Fix heterogeneous tuples and `NamedTuple`s with a member type containing a quote in its `repr`, like `tuple[Literal["a"], int]`, crashing structuring code generation with `SyntaxError`; the index note is now embedded with `repr`. ([#777](https://github.com/python-attrs/cattrs/pull/777)) - Fix {func}`transform_error ` listing the extra keys of a `ForbiddenExtraKeysError` in set iteration order, which made the message differ between runs; the keys are now sorted, like the error's own `__str__` already sorts them. diff --git a/src/cattrs/dispatch.py b/src/cattrs/dispatch.py index f98dc51d..5a8ea10e 100644 --- a/src/cattrs/dispatch.py +++ b/src/cattrs/dispatch.py @@ -1,6 +1,7 @@ from __future__ import annotations from functools import lru_cache, singledispatch +from threading import local from typing import TYPE_CHECKING, Any, Callable, Generic, Literal, TypeVar from attrs import Factory, define @@ -27,6 +28,40 @@ class _DispatchNotFound: """A dummy object to help signify a dispatch not found.""" +_factory_state = local() +"""Thread-local state for tracking in-progress hook factory invocations.""" + + +def _in_progress_factories() -> set: + """The set of hook factories currently being evaluated, as `(id, type)` pairs. + + Used to detect re-entrant factory invocations for the same type; purely + observational, it does not change dispatching itself. + """ + try: + return _factory_state.in_progress + except AttributeError: + res = set() + _factory_state.in_progress = res + return res + + +def _factory_key(handler: Callable, typ: Any) -> tuple | None: + """A hashable key identifying a factory invocation, if possible.""" + key = (id(handler), typ) + try: + hash(key) + except TypeError: + return None + return key + + +def _is_factory_in_progress(handler: Callable, typ: Any) -> bool: + """Is `handler` currently being evaluated for `typ` on this thread?""" + key = _factory_key(handler, typ) + return key is not None and key in _in_progress_factories() + + @define class FunctionDispatch: """ @@ -71,9 +106,17 @@ def dispatch(self, typ: Any) -> Callable[..., Any] | None: continue if ch: if is_generator: - if takes_converter: - return handler(typ, self._converter) - return handler(typ) + key = _factory_key(handler, typ) + in_progress = _in_progress_factories() if key is not None else None + if in_progress is not None: + in_progress.add(key) + try: + if takes_converter: + return handler(typ, self._converter) + return handler(typ) + finally: + if in_progress is not None: + in_progress.discard(key) return handler return None diff --git a/src/cattrs/gen/__init__.py b/src/cattrs/gen/__init__.py index 43e1559f..4c891e9a 100644 --- a/src/cattrs/gen/__init__.py +++ b/src/cattrs/gen/__init__.py @@ -19,7 +19,7 @@ is_generic, ) from .._generics import deep_copy_with -from ..dispatch import UnstructureHook +from ..dispatch import UnstructureHook, _DispatchNotFound, _is_factory_in_progress from ..errors import ( AttributeValidationNote, ClassValidationError, @@ -251,6 +251,75 @@ def make_dict_unstructure_fn_from_attrs( return res +def _converter_unstructure_hook( + converter: BaseConverter, cl: type[T] +) -> Callable[[T], Any] | None: + """Get the converter's own unstructure hook for `cl`, if it has a custom one. + + "Custom" here means anything except the converter's default attrs + unstructuring. Hook factories which are already being evaluated for `cl` + (directly causing this call, for example a hook factory delegating to + `make_dict_unstructure_fn`) are skipped, giving lower-precedence factories + a chance; factories raising `RecursionError` are skipped as well. + + Used by `make_dict_unstructure_fn` so it composes with hooks registered on + the converter (for example methods picked up by + `cattrs.strategies.use_class_methods`, or hooks registered directly for + `cl`) instead of silently ignoring them. + """ + unstructure_func = converter._unstructure_func + + def is_default_attrs_hook(hook: Any) -> bool: + # `BaseConverter._unstructure_attrs` is the default attrs unstructuring; + # bound methods compare by `__self__` and `__func__`. + return hook == converter._unstructure_attrs + + # Hooks registered for (super)classes take precedence, mirroring dispatch. + try: + hook = unstructure_func._single_dispatch.dispatch(cl) + except Exception: # noqa: S110 + pass + else: + if hook is not _DispatchNotFound and not is_default_attrs_hook(hook): + return hook + + hook = unstructure_func._direct_dispatch.get(cl) + if hook is not None and not is_default_attrs_hook(hook): + return hook + + for ( + predicate, + handler, + is_generator, + takes_converter, + ) in unstructure_func._function_dispatch._handler_pairs: + try: + matches = predicate(cl) + except Exception: # noqa: S112 + continue + if not matches: + continue + if not is_generator: + if not is_default_attrs_hook(handler): + return handler + elif _is_factory_in_progress(handler, cl): + # The factory is already being evaluated for `cl` further up the + # stack; calling it again would just hit its own recursion guard + # (or recurse), so give lower-precedence factories a chance. + continue + else: + try: + hook = handler(cl, converter) if takes_converter else handler(cl) + except RecursionError: + # The factory recurses back into `make_dict_unstructure_fn`; + # skip it so factories with a lower precedence get a chance. + continue + if not is_default_attrs_hook(hook): + return hook + + return None + + def make_dict_unstructure_fn( cl: type[T], converter: BaseConverter, @@ -287,7 +356,8 @@ def make_dict_unstructure_fn( attrs = adapted_fields(origin or cl) # type: ignore mapping = {} - if _cattrs_use_alias == "from_converter": + use_alias_from_converter = _cattrs_use_alias == "from_converter" + if use_alias_from_converter: # BaseConverter doesn't have it so we're careful. _cattrs_use_alias = getattr(converter, "use_alias", False) if is_generic(cl): @@ -309,6 +379,21 @@ def make_dict_unstructure_fn( working_set.add(cl) try: + if ( + not kwargs + and not _cattrs_omit_if_default + and not _cattrs_include_init_false + and use_alias_from_converter + ): + # No customizations were requested, so the converter may have a + # better hook for `cl` than the one we would generate (for example + # a method picked up by `cattrs.strategies.use_class_methods`, or a + # hook registered directly for `cl`); prefer it so hook factories + # built on top of this function compose with those customizations + # instead of silently ignoring them. + custom_hook = _converter_unstructure_hook(converter, cl) + if custom_hook is not None: + return custom_hook return make_dict_unstructure_fn_from_attrs( attrs, cl, diff --git a/tests/strategies/test_class_methods.py b/tests/strategies/test_class_methods.py index a99b2a78..30df94fc 100644 --- a/tests/strategies/test_class_methods.py +++ b/tests/strategies/test_class_methods.py @@ -2,11 +2,12 @@ from typing import Union import pytest -from attrs import define +from attrs import define, has from hypothesis import given from hypothesis.strategies import integers from cattrs import BaseConverter +from cattrs.gen import make_dict_unstructure_fn from cattrs.strategies import use_class_methods @@ -120,3 +121,41 @@ def _unstructure(cls): converter.structure({"a": 1}, Bad) with pytest.raises(TypeError): converter.unstructure(Bad(1)) + + +def test_make_dict_unstructure_fn_honors_class_methods(converter: BaseConverter): + """`make_dict_unstructure_fn` should honor the converter's own hooks. + + A hook factory delegating to `make_dict_unstructure_fn` should compose with + the methods picked up by `use_class_methods` instead of ignoring them. + + See https://github.com/python-attrs/cattrs/issues/566. + """ + + @define + class A: + a: int + + def _unstructure(self): + return {"a": str(self.a)} + + use_class_methods(converter, None, "_unstructure") + + # The metamethod is picked up directly. + assert make_dict_unstructure_fn(A, converter)(A(1)) == {"a": "1"} + + def tag_attrs_hook_factory(cl): + base_hook = make_dict_unstructure_fn(cl, converter) + + def hook(inst): + unstructured = base_hook(inst) + unstructured["_type"] = type(inst).__name__ + return unstructured + + return hook + + # The tag factory takes precedence (registered last), but the metamethod + # should still be honored through `make_dict_unstructure_fn`. + converter.register_unstructure_hook_factory(has, tag_attrs_hook_factory) + + assert converter.unstructure(A(1)) == {"a": "1", "_type": "A"}