From d77433bc297ff891c9cd0356ddd0abab391b1ee6 Mon Sep 17 00:00:00 2001 From: Vinayak Deshmuk <118991845+Vinayak19112003@users.noreply.github.com> Date: Tue, 22 Sep 2026 11:13:40 +0530 Subject: [PATCH] Honor converter hooks in make_dict_unstructure_fn make_dict_unstructure_fn ignored hooks registered on the converter (for example methods picked up by the use_class_methods strategy), so hook factories built on top of it could not compose with those customizations. When no customizations are requested, prefer the converter's own unstructure hook for the class over generating a new function. Factories already being evaluated for the class are skipped so lower-precedence factories get a chance; in-progress factory invocations are now tracked (thread-local) in FunctionDispatch.dispatch for this. Fixes #566. --- HISTORY.md | 2 + src/cattrs/dispatch.py | 49 +++++++++++++- src/cattrs/gen/__init__.py | 89 +++++++++++++++++++++++++- tests/strategies/test_class_methods.py | 41 +++++++++++- 4 files changed, 175 insertions(+), 6 deletions(-) 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"}