PR feedback

Author: andystaples

durabletask/internal/type_discovery.py

@@ -10,7 +10,7 @@
 
 Discovery is intentionally conservative: it only returns an annotation when the
 active :class:`~durabletask.serialization.DataConverter` reports it as
-*reconstructable* via :meth:`DataConverter.is_reconstructable`. The default
+*reconstructable* via :meth:`DataConverter.can_reconstruct`. The default
 converter recognizes dataclasses, ``from_json()``-capable types, and ``Optional``
 / ``list`` hints wrapping them; a custom converter can recognize its own types
 (e.g. ``pydantic.BaseModel``). Primitive and unknown annotations resolve to

durabletask/serialization.py

@@ -403,7 +403,12 @@ def _invoke_from_json(hook: Any, value: Any, converter: DataConverter | None) ->
     > ``from_json`` must be a ``@classmethod`` or ``@staticmethod`` -- the hook
     > is resolved off the *type* (no instance exists yet during reconstruction).
     > A plain instance method would have ``self`` consume the value and is
-    > unsupported regardless of the converter-arity detection below.
+    > unsupported regardless of the converter detection below.
+    >
+    > The converter is passed positionally as the second argument, so a hook
+    > opts in only by naming that parameter exactly ``converter``. This reserved
+    > name avoids misreading an unrelated second parameter (e.g.
+    > ``from_json(cls, value, strict=False)``) as converter-aware.
     """
     if converter is not None and _hook_accepts_converter(hook):
         return hook(value, converter)
@@ -412,26 +417,27 @@ def _invoke_from_json(hook: Any, value: Any, converter: DataConverter | None) ->
 
 @functools.lru_cache(maxsize=2048)
 def _hook_accepts_converter(hook: Any) -> bool:
-    """Return True if a bound ``from_json`` hook can accept a second argument.
+    """Return True if a bound ``from_json`` hook opts in to receiving the converter.
 
     The hook is inspected as accessed off the type (``cls``/``self`` already
     bound), so a classmethod ``from_json(cls, value, converter)`` presents as
-    ``(value, converter)``. Results are cached because reconstruction runs on
-    hot paths; bound classmethods hash equal across attribute accesses, so the
-    cache stays effective and bounded.
+    ``(value, converter)``. A hook is treated as converter-aware only when its
+    second positional parameter is named exactly ``converter`` -- the reserved
+    name for this argument -- so an unrelated second parameter such as
+    ``strict=False`` is not mistaken for it. Results are cached because
+    reconstruction runs on hot paths; bound classmethods hash equal across
+    attribute accesses, so the cache stays effective and bounded.
     """
     try:
         sig = inspect.signature(hook)
     except (TypeError, ValueError):
         return False
-    positional = 0
-    for param in sig.parameters.values():
+    positional = [
+        param for param in sig.parameters.values()
         if param.kind in (inspect.Parameter.POSITIONAL_ONLY,
-                          inspect.Parameter.POSITIONAL_OR_KEYWORD):
-            positional += 1
-        elif param.kind is inspect.Parameter.VAR_POSITIONAL:
-            return True
-    return positional >= 2
+                          inspect.Parameter.POSITIONAL_OR_KEYWORD)
+    ]
+    return len(positional) >= 2 and positional[1].name == "converter"
 
 
 def _coerce_generic(value: Any, expected_type: Any, origin: Any,

examples/custom_data_converter/README.md

@@ -48,9 +48,19 @@ A `DataConverter` implements three methods:
 The converter in [src/converter.py](src/converter.py) recognizes
 `pydantic.BaseModel` subclasses and uses pydantic for them, **delegating
 everything else** to the default `JsonDataConverter`. This "handle my types,
-delegate the rest" shape is the recommended pattern for a real converter — it
+delegate the rest" shape is a good starting point for a real converter — it
 costs nothing for non-pydantic payloads.
 
+> [!NOTE]
+> To stay focused on the seam, this example only intercepts when a model is the
+> *top-level* type. A model nested inside another model (like `Order.items`)
+> round-trips because pydantic recurses on its own, but a model nested in a
+> *top-level generic* the SDK rebuilds directly — e.g. `return_type=list[OrderItem]`
+> or an input annotated `dict[str, OrderItem]` — is not intercepted and falls to
+> the default codec, which leaves the elements as raw dicts. A production
+> converter would recurse into such generics (for example via
+> `pydantic.TypeAdapter`).
+
 ## Inbound inputs: `can_reconstruct`
 
 There is one extra detail for reconstructing **inbound** orchestrator/activity

examples/custom_data_converter/src/converter.py

@@ -61,6 +61,18 @@ class PydanticDataConverter(DataConverter):
     (with full validation) via ``model_validate_json()`` / ``model_validate()``
     whenever the SDK supplies the model type. Every other value falls through to
     the default :class:`JsonDataConverter`.
+
+    > [!NOTE]
+    > For simplicity this converter only intercepts when a pydantic model is the
+    > *top-level* ``target_type``. A model nested **inside another model** (like
+    > ``Order.items: list[OrderItem]`` below) round-trips fine because pydantic
+    > handles that recursion itself. But a model nested in a *top-level generic*
+    > the SDK is asked to rebuild directly -- e.g. ``return_type=list[OrderItem]``
+    > or an input annotated ``dict[str, OrderItem]`` -- is **not** intercepted
+    > here; it falls to the default codec, which cannot construct a pydantic
+    > model from a positional ``dict`` and so leaves the elements as raw dicts. A
+    > production converter would recurse into such generics (for example via
+    > ``pydantic.TypeAdapter``); this example keeps the surface minimal.
     """
 
     def __init__(self) -> None:

tests/durabletask/test_serialization.py

@@ -488,6 +488,27 @@ def test_coerce_to_type_without_converter_calls_single_arg_hook():
     assert result == Widget("gear", 5)
 
 
+def test_from_json_hook_unrelated_second_param_is_not_treated_as_converter():
+    # A ``from_json`` whose second parameter is *not* named ``converter`` (here a
+    # ``strict`` flag with a default) must not be mistaken for a converter-aware
+    # hook -- otherwise the DataConverter would be bound to ``strict``.
+    from durabletask.serialization import JsonDataConverter
+
+    @dataclass
+    class Flagged:
+        label: str
+
+        @classmethod
+        def from_json(cls, value, strict=False):
+            # If the converter were passed here, ``strict`` would be truthy.
+            assert strict is False
+            return cls(value["label"])
+
+    conv = JsonDataConverter()
+    result = conv.deserialize('{"label": "ok"}', Flagged)
+    assert result == Flagged("ok")
+
+
 # ----- forward-reference resolution (Python 3.10 get_type_hints parity) -----
 #
 # On Python 3.10, ``typing.get_type_hints`` does not deep-resolve forward