Handle forward refs in base types and improve logic that determines the item or key/value type in the CollectionConverter and MappingConverter respectively (#52)

* fix: Fixed serde of types that have a parameterized generic base class. (First reported in NiklasRosenstein/pydoc-markdown#292)

* tests: Add a unit tests to demonstrate that deserializing a nested type cannot work.

* Properly infer item type for collections in the `CollectionConverter`, do not assume `Any` for un-parameterized collections.

* Improve logic to find item type of collection in `CollectionConverter`

* databind.core/: improvement: Work around highly nested error tracebacks in `Module.convert()` by expanding all converters returned by `Module.get_converter()`. Note that this means `Module.convert()` is no longer called if the module is a child of another module.

* databind.core/: improvement: Add `DelegateToClassmethodConverter(serialized_type)` parameter.

* databind.json/: improvement: The `MappingConverter` now does improved resolution of the key and value type just like the `CollectionConverter`; note that an unparameterized Mapping no longer has its key and value type fall back to `typing.Any`

* databind.json/: improvement: Use `ConversionError.expected()` factory function in `PlainDatatypeConverter`

* databind.json/: tests: Test `JsonConverter`

* Updated PR references in 5 changelogs.

skip-checks: true

---------

Co-authored-by: GitHub Action <[email protected]>

Author: NiklasRosenstein

databind.core/.changelog/_unreleased.toml

@@ -4,3 +4,17 @@ type = "fix"
 description = "Fixed serde of types that have a parameterized generic base class. (First reported in NiklasRosenstein/pydoc-markdown#292)"
 author = "[email protected]"
 pr = "https://github.com/NiklasRosenstein/python-databind/pull/51"
+
+[[entries]]
+id = "43fe920b-b6b9-4701-9403-bff9aba2e744"
+type = "improvement"
+description = "Work around highly nested error tracebacks in `Module.convert()` by expanding all converters returned by `Module.get_converter()`. Note that this means `Module.convert()` is no longer called if the module is a child of another module."
+author = "@NiklasRosenstein"
+pr = "https://github.com/NiklasRosenstein/python-databind/pull/52"
+
+[[entries]]
+id = "e2e3fbdf-b19c-443d-bc79-8560dba4b1b5"
+type = "improvement"
+description = "Add `DelegateToClassmethodConverter(serialized_type)` parameter."
+author = "@NiklasRosenstein"
+pr = "https://github.com/NiklasRosenstein/python-databind/pull/52"

databind.core/pyproject.toml

@@ -18,7 +18,7 @@ python = "^3.6.3"
 Deprecated = "^1.2.12"
 nr-date = "^2.0.0"
 nr-stream = "^1.0.0"
-typeapi = "^1.4.2"
+typeapi = "^2.0.1"
 typing-extensions = ">=3.10.0"
 
 [tool.poetry.dev-dependencies]

databind.core/src/databind/core/converter.py

@@ -71,7 +71,11 @@ def register(self, converter: Converter, first: bool = False) -> None:
             self.converters.append(converter)
 
     def get_converters(self, ctx: "Context") -> t.Iterator[Converter]:
-        yield from self.converters
+        for converter in self.converters:
+            if isinstance(converter, Module):
+                yield from converter.get_converters(ctx)
+            else:
+                yield converter
 
     def convert(self, ctx: "Context") -> t.Any:
         errors: t.List[t.Tuple[Converter, Exception]] = []
@@ -150,18 +154,29 @@ class DelegateToClassmethodConverter(Converter):
     scenario (e.g. such as de/serializing JSON with the #databind.json.settings.JsonConverter setting).
     """
 
-    def __init__(self, *, serialize: "str | None" = None, deserialize: "str | None" = None) -> None:
+    def __init__(
+        self,
+        serialized_type: t.Union[t.Type[t.Any], t.Tuple[t.Type[t.Any], ...], None] = None,
+        *,
+        serialize: "str | None" = None,
+        deserialize: "str | None" = None,
+    ) -> None:
+        self._serialized_type = serialized_type
         self._serialize = serialize
         self._deserialize = deserialize
 
     def serialize(self, ctx: "Context") -> t.Any:
         if self._serialize is None or not isinstance(ctx.datatype, ClassTypeHint):
             raise NotImplementedError
+        if not isinstance(ctx.value, ctx.datatype.type):
+            raise ConversionError.expected(self, ctx, ctx.datatype.type)
         method: t.Callable[[t.Any], t.Any] = getattr(ctx.datatype.type, self._serialize)
         return method(ctx.value)
 
     def deserialize(self, ctx: "Context") -> t.Any:
         if self._deserialize is None or not isinstance(ctx.datatype, ClassTypeHint):
             raise NotImplementedError
+        if self._serialized_type is not None and not isinstance(ctx.value, self._serialized_type):
+            raise ConversionError.expected(self, ctx, self._serialized_type)
         method: t.Callable[[t.Any], t.Any] = getattr(ctx.datatype.type, self._deserialize)
         return method(ctx.value)

databind.json/.changelog/_unreleased.toml

@@ -4,3 +4,38 @@ type = "tests"
 description = "Add a unit tests to demonstrate that deserializing a nested type cannot work."
 author = "@NiklasRosenstein"
 pr = "https://github.com/NiklasRosenstein/python-databind/pull/51"
+
+[[entries]]
+id = "89204ef2-6173-4d4e-b857-3af59db2de32"
+type = "fix"
+description = "Technically a breaking change, but any consumer who is relying on this behaviour probably does that implicitly and wants to change their code anyway. o_o -- The `CollectionConverter` no longer implicitly assumes `Any` as the item type if collection is not parameterized."
+author = "@NiklasRosenstein"
+pr = "https://github.com/NiklasRosenstein/python-databind/pull/52"
+
+[[entries]]
+id = "0b05e104-7012-4a03-a995-12ed05878f0b"
+type = "improvement"
+description = "The `CollectionConverter` now properly infers the item type from the types base classes"
+author = "@NiklasRosenstein"
+pr = "https://github.com/NiklasRosenstein/python-databind/pull/52"
+
+[[entries]]
+id = "ae147797-2857-4e6e-b7af-d2c6c1152e59"
+type = "improvement"
+description = "The `MappingConverter` now does improved resolution of the key and value type just like the `CollectionConverter`; note that an unparameterized Mapping no longer has its key and value type fall back to `typing.Any`"
+author = "@NiklasRosenstein"
+pr = "https://github.com/NiklasRosenstein/python-databind/pull/52"
+
+[[entries]]
+id = "08e63490-84b9-418d-8f26-39c770f17d59"
+type = "improvement"
+description = "Use `ConversionError.expected()` factory function in `PlainDatatypeConverter`"
+author = "@NiklasRosenstein"
+pr = "https://github.com/NiklasRosenstein/python-databind/pull/52"
+
+[[entries]]
+id = "78c6beda-22dd-419e-848f-5835ab554efd"
+type = "tests"
+description = "Test `JsonConverter`"
+author = "@NiklasRosenstein"
+pr = "https://github.com/NiklasRosenstein/python-databind/pull/52"

databind.json/pyproject.toml

@@ -17,7 +17,7 @@ Repository = "https://github.com/NiklasRosenstein/python-databind"
 python = "^3.6.3"
 "databind.core" = "^4.3.2"
 nr-date = "^2.0.0"
-typeapi = "^1.4.2"
+typeapi = "^2.0.1"
 typing-extensions = ">=3.10.0"
 
 [tool.poetry.dev-dependencies]

databind.json/src/databind/json/converters.py

@@ -127,13 +127,18 @@ def _length_check() -> None:
                     )
 
         else:
-            # There could be no arguments to the collection type, in which case we
-            # consider Any as the item type.
-            item_types_iterator = iter(lambda: datatype[0] if datatype.args else TypeHint(t.Any), None)
-            if isinstance(datatype, TupleTypeHint):
-                python_type = tuple
-            else:
-                python_type = datatype.type
+            candidates = set()
+            for current in datatype.recurse_bases():
+                if issubclass(current.type, t.Collection) and len(current.args) == 1:
+                    candidates.add(current.args[0])
+            if len(candidates) == 0:
+                raise ConversionError(self, ctx, f"could not find item type in {datatype}")
+            elif len(candidates) > 1:
+                raise ConversionError(self, ctx, f"found multiple item types in {datatype}: {candidates}")
+
+            item_type = TypeHint(next(iter(candidates)))
+            item_types_iterator = iter(lambda: item_type, None)
+            python_type = datatype.type
 
             def _length_check() -> None:
                 pass
@@ -317,13 +322,20 @@ def __init__(self, json_mapping_type: t.Type[t.Mapping[str, t.Any]] = dict) -> N
 
     def convert(self, ctx: Context) -> t.Any:
         datatype = _unwrap_annotated(ctx.datatype)
+
+        # Find the key and value types of the mapping.
         if not isinstance(datatype, ClassTypeHint) or not issubclass(datatype.type, t.Mapping):
             raise NotImplementedError
+        candidates = set()
+        for current in datatype.recurse_bases():
+            if issubclass(current.type, t.Mapping) and len(current.args) == 2:
+                candidates.add(current.args)
+        if len(candidates) == 0:
+            raise ConversionError(self, ctx, f"could not find key/value type in {datatype}")
+        elif len(candidates) > 1:
+            raise ConversionError(self, ctx, f"found multiple key/value types in {datatype}: {candidates}")
 
-        if not datatype.args:
-            key_type, value_type = t.Any, t.Any
-        else:
-            key_type, value_type = datatype.args
+        key_type, value_type = next(iter(candidates))
 
         if not isinstance(ctx.value, t.Mapping):
             raise ConversionError.expected(self, ctx, t.Mapping)
@@ -417,8 +429,7 @@ def convert(self, ctx: Context) -> t.Any:
         adapter = adapters.get((source_type, target_type))
 
         if adapter is None:
-            msg = f"unable to {ctx.direction.name.lower()} {source_type.__name__} -> {target_type.__name__}"
-            raise ConversionError(self, ctx, msg)
+            raise ConversionError.expected(self, ctx, target_type, source_type)
 
         try:
             return adapter(ctx.value)

databind.json/src/databind/json/module.py

@@ -55,6 +55,17 @@ def __init__(self) -> None:
         self.register(StringifyConverter(duration, duration.parse, name="JsonModule:nr.date.duration"), first=True)
         self.register(LiteralConverter())
 
+        self.register(JsonConverterSupport(), first=True)
+
+
+class JsonConverterSupport(Module):
+    """
+    Handles the JsonConverter setting.
+    """
+
+    def __init__(self) -> None:
+        super().__init__(__name__ + ".JsonConverterSupport")
+
     def get_converters(self, ctx: Context) -> Iterator[Converter]:
         converter_setting = ctx.get_setting(JsonConverter)
         if converter_setting is not None:

databind.json/src/databind/json/settings.py

@@ -56,5 +56,12 @@ def __init__(self, supplier: t.Union[ConverterSupplier, Converter]) -> None:
             self.supplier = supplier
 
     @staticmethod
-    def using_classmethods(*, serialize: "str | None" = None, deserialize: "str | None" = None) -> "JsonConverter":
-        return JsonConverter(DelegateToClassmethodConverter(serialize=serialize, deserialize=deserialize))
+    def using_classmethods(
+        serialized_type: t.Union[t.Type[t.Any], t.Tuple[t.Type[t.Any], ...], None] = None,
+        *,
+        serialize: "str | None" = None,
+        deserialize: "str | None" = None
+    ) -> "JsonConverter":
+        return JsonConverter(
+            DelegateToClassmethodConverter(serialized_type, serialize=serialize, deserialize=deserialize)
+        )

databind.json/src/databind/json/tests/converters_test.py

@@ -2,6 +2,7 @@
 import datetime
 import decimal
 import enum
+import sys
 import typing as t
 import uuid
 from collections import namedtuple
@@ -36,6 +37,7 @@
     StringifyConverter,
     UnionConverter,
 )
+from databind.json.module import JsonConverterSupport, JsonModule
 from databind.json.settings import JsonConverter
 
 
@@ -200,7 +202,11 @@ def test_stringify_converter(direction: Direction) -> None:
 @pytest.mark.parametrize("direction", (Direction.SERIALIZE, Direction.DESERIALIZE))
 def test_mapping_converter(direction: Direction) -> None:
     mapper = make_mapper([AnyConverter(), MappingConverter(), PlainDatatypeConverter()])
-    assert mapper.convert(direction, {"a": 1}, t.Mapping) == {"a": 1}
+
+    with pytest.raises(ConversionError) as excinfo:
+        mapper.convert(direction, {"a": 1}, t.Mapping)
+    assert str(excinfo.value).splitlines()[0] == "could not find key/value type in TypeHint(typing.Mapping)"
+
     assert mapper.convert(direction, {"a": 1}, t.Mapping[str, int]) == {"a": 1}
     assert mapper.convert(direction, {"a": 1}, t.MutableMapping[str, int]) == {"a": 1}
     assert mapper.convert(direction, {"a": 1}, t.Dict[str, int]) == {"a": 1}
@@ -226,10 +232,57 @@ class CustomDict(t.Dict[K, V]):
     #   assert mapper.convert(direction, {"a": 1}, FixedDict) == FixedDict({"a": 1})
 
 
+def test__MappingConverter__cannot_deserialize_dict_without_key_value_annotations() -> None:
+    mapper = make_mapper([MappingConverter()])
+    with pytest.raises(ConversionError) as excinfo:
+        mapper.deserialize({"a": 1, "b": 2}, dict)
+    assert str(excinfo.value).splitlines()[0] == "could not find key/value type in TypeHint(dict)"
+    with pytest.raises(ConversionError) as excinfo:
+        mapper.deserialize({"a": 1, "b": 2}, t.Dict)
+    assert str(excinfo.value).splitlines()[0] == "could not find key/value type in TypeHint(typing.Dict)"
+
+
+def test__MappingConverter__can_deserialize_dict_with_key_value_annotations() -> None:
+    mapper = make_mapper([PlainDatatypeConverter(), MappingConverter()])
+    if sys.version_info[:2] >= (3, 10):
+        assert mapper.deserialize({"a": 1, "b": 2}, dict[str, int]) == {"a": 1, "b": 2}
+    assert mapper.deserialize({"a": 1, "b": 2}, t.Dict[str, int]) == {"a": 1, "b": 2}
+
+
+def test__MappingConverter__can_serde_custom_key_type() -> None:
+    @JsonConverter.using_classmethods(str, serialize="__str__", deserialize="of")
+    @dataclasses.dataclass(frozen=True)
+    class MyKeyType:
+        a: str
+        b: str
+
+        def __str__(self) -> str:
+            return f"{self.a}/{self.b}"
+
+        @staticmethod
+        def of(v: str) -> "MyKeyType":
+            return MyKeyType(*v.split("/"))
+
+    json = {"a/b": 1, "b/c": 2}
+    python = {MyKeyType("a", "b"): 1, MyKeyType("b", "c"): 2}
+
+    for key, mapper in {
+        "Subset": make_mapper([PlainDatatypeConverter(), MappingConverter(), JsonConverterSupport()]),
+        "JsonModule": make_mapper([JsonModule()]),
+    }.items():
+        print(">", key)
+        assert mapper.deserialize(json, t.Mapping[MyKeyType, int]) == python
+        assert mapper.serialize(python, t.Mapping[MyKeyType, int]) == json
+
+
 @pytest.mark.parametrize("direction", (Direction.SERIALIZE, Direction.DESERIALIZE))
 def test_collection_converter(direction: Direction) -> None:
     mapper = make_mapper([AnyConverter(), CollectionConverter(), PlainDatatypeConverter()])
-    assert mapper.convert(direction, [1, 2, 3], t.Collection) == [1, 2, 3]
+
+    with pytest.raises(ConversionError) as excinfo:
+        mapper.convert(direction, [1, 2, 3], t.Collection)
+    assert str(excinfo.value).splitlines()[0] == "could not find item type in TypeHint(typing.Collection)"
+
     assert mapper.convert(direction, [1, 2, 3], t.Collection[int]) == [1, 2, 3]
     assert mapper.convert(direction, [1, 2, 3], t.MutableSequence[int]) == [1, 2, 3]
     assert mapper.convert(direction, [1, 2, 3], t.List[int]) == [1, 2, 3]
@@ -294,7 +347,7 @@ def test_union_converter_flat_plain_types_not_supported(direction: Direction) ->
     if direction == Direction.DESERIALIZE:
         with pytest.raises(ConversionError) as excinfo:
             assert mapper.convert(direction, {"type": "int", "int": 42}, th)
-        assert "unable to deserialize dict -> int" in str(excinfo.value)
+        assert "expected int, got dict instead" in str(excinfo.value)
     else:
         with pytest.raises(ConversionError) as excinfo:
             assert mapper.convert(direction, 42, th)
@@ -516,22 +569,28 @@ def test_deserialize_tuple() -> None:
 
     with pytest.raises(ConversionError) as excinfo:
         databind.json.load([1, 42], t.Tuple[int, str])
-    assert excinfo.value.message == "unable to deserialize int -> str"
+    assert excinfo.value.message == "expected str, got int instead"
 
     with pytest.raises(ConversionError) as excinfo:
         databind.json.load([1, 42, 3], t.Tuple[int, int])
     assert excinfo.value.message == "expected a tuple of length 2, found 3"
 
 
-def test__namedtuple() -> None:
-    # NOTE: Need the AnyConverter because the namedtuple is not a dataclass and we don't have type information
-    #       for the fields.
+def test__namedtuple__cannot_serde() -> None:
+    """
+    There is no type information for #collections.namedtuples.
+    """
+
     mapper = make_mapper([CollectionConverter(), PlainDatatypeConverter(), AnyConverter()])
 
     nt = namedtuple("nt", ["a", "b"])
 
-    assert mapper.serialize(nt(1, 2), nt) == [1, 2]
-    assert mapper.deserialize([1, 2], nt) == nt(1, 2)
+    with pytest.raises(ConversionError) as excinfo:
+        print(mapper.serialize(nt(1, 2), nt))
+    assert str(excinfo.value).splitlines()[0] == "could not find item type in TypeHint(converters_test.nt)"
+    with pytest.raises(ConversionError) as excinfo:
+        print(mapper.deserialize([1, 2], nt))
+    assert str(excinfo.value).splitlines()[0] == "could not find item type in TypeHint(converters_test.nt)"
 
 
 def test__typing_NamedTuple() -> None:
@@ -614,3 +673,43 @@ class MyPage(Page["MyPage"]):
     assert mapper.deserialize(payload, Page[Page[Page[Page]]]) == Page(  # type: ignore[type-arg]
         "root", [Page("child", [Page("grandchild", [])])]
     )
+
+
+def test__list__fails_without_type_parameter() -> None:
+    mapper = make_mapper([AnyConverter(), CollectionConverter()])
+    with pytest.raises(ConversionError) as excinfo:
+        mapper.deserialize([1, 2, 3], list)
+    assert str(excinfo.value).splitlines()[0] == "could not find item type in TypeHint(list)"
+    with pytest.raises(ConversionError) as excinfo:
+        mapper.deserialize([1, 2, 3], t.List)
+    assert str(excinfo.value).splitlines()[0] == "could not find item type in TypeHint(typing.List)"
+
+
+def test__list__subclass_items_deserialized_correctly() -> None:
+    class MyList(t.List[SpecificPage]):
+        pass
+
+    mapper = make_mapper([SchemaConverter(), AnyConverter(), CollectionConverter(), PlainDatatypeConverter()])
+    assert mapper.deserialize([{"name": "foo", "children": []}], t.List[SpecificPage]) == MyList(
+        [SpecificPage("foo", [])]
+    )
+    assert mapper.deserialize([{"name": "foo", "children": []}], MyList) == MyList([SpecificPage("foo", [])])
+
+
+def test__JsonConverter__using_classmethods_on_plain_class() -> None:
+    @JsonConverter.using_classmethods(str, serialize="__str__", deserialize="of")
+    class MyCls:
+        def __eq__(self, other: t.Any) -> bool:
+            return type(other) is MyCls
+
+        def __str__(self) -> str:
+            return "MyCls"
+
+        @classmethod
+        def of(cls, v: str) -> "MyCls":
+            assert v == "MyCls"
+            return cls()
+
+    mapper = make_mapper([JsonConverterSupport()])
+    assert mapper.serialize(MyCls(), MyCls) == "MyCls"
+    assert mapper.deserialize("MyCls", MyCls) == MyCls()