Merge pull request asdf-format#1561 from braingram/select_tag_none

Allow converter deferral and move Reference to a converter

CHANGES.rst

@@ -13,6 +13,8 @@ The ASDF Standard is at v1.6.0
 - Drop Python 3.8 support [#1556]
 - Drop NumPy 1.20, 1.21 support [#1568]
 - Fix issue opening files that don't support ``fileno`` [#1557]
+- Allow Converters to defer conversion to other Converters
+  by returning ``None`` in ``Converter.select_tag`` [#1561]
 
 2.15.0 (2023-03-28)
 -------------------

asdf/_tests/test_extension.py

@@ -18,6 +18,7 @@
     get_cached_extension_manager,
 )
 from asdf.extension._legacy import BuiltinExtension, _AsdfExtension, get_cached_asdf_extension_list
+from asdf.testing.helpers import roundtrip_object
 
 
 def test_builtin_extension():
@@ -600,7 +601,10 @@ def test_converter_proxy():
 
     # Should fail because types must instances of type:
     with pytest.raises(TypeError, match=r"Converter property .* must contain str or type values"):
-        ConverterProxy(MinimumConverter(types=[object()]), extension)
+        # as the code will ignore types if no relevant tags are found
+        # include a tag from this extension to make sure the proxy considers
+        # the types
+        ConverterProxy(MinimumConverter(tags=[extension.tags[0].tag_uri], types=[object()]), extension)
 
 
 def test_get_cached_asdf_extension_list():
@@ -760,3 +764,132 @@ def test_validator():
             af["foo"] = "bar"
             with pytest.raises(ValidationError, match=r"Node was doomed to fail"):
                 af.validate()
+
+
+def test_converter_deferral():
+    class Bar:
+        def __init__(self, value):
+            self.value = value
+
+    class Foo(Bar):
+        pass
+
+    class Baz(Bar):
+        pass
+
+    class FooConverter:
+        tags = []
+        types = [Foo]
+
+        def select_tag(self, *args):
+            return None
+
+        def to_yaml_tree(self, obj, tag, ctx):
+            # convert Foo instance to Bar
+            return Bar(obj.value)
+
+        def from_yaml_tree(self, node, tag, ctx):
+            raise NotImplementedError()
+
+    class BarConverter:
+        tags = ["asdf://somewhere.org/tags/bar"]
+        types = [Bar]
+
+        def to_yaml_tree(self, obj, tag, ctx):
+            return {"value": obj.value}
+
+        def from_yaml_tree(self, node, tag, ctx):
+            return Bar(node["value"])
+
+    class BazConverter:
+        tags = []
+        types = [Baz]
+
+        def select_tag(self, *args):
+            return None
+
+        def to_yaml_tree(self, obj, tag, ctx):
+            return Foo(obj.value)
+
+        def from_yaml_tree(self, node, tag, ctx):
+            raise NotImplementedError()
+
+    extension = FullExtension(converters=[FooConverter(), BarConverter(), BazConverter()], tags=BarConverter.tags)
+    with config_context() as config:
+        config.add_extension(extension)
+
+        foo = Foo(26)
+        bar = Bar(42)
+        baz = Baz(720)
+
+        bar_rt = roundtrip_object(bar)
+        assert isinstance(bar_rt, Bar)
+        assert bar_rt.value == bar.value
+
+        foo_rt = roundtrip_object(foo)
+        assert isinstance(foo_rt, Bar)
+        assert foo_rt.value == foo.value
+
+        baz_rt = roundtrip_object(baz)
+        assert isinstance(baz_rt, Bar)
+        assert baz_rt.value == baz.value
+
+
+def test_converter_loop():
+    class Bar:
+        def __init__(self, value):
+            self.value = value
+
+    class Foo(Bar):
+        pass
+
+    class Baz(Bar):
+        pass
+
+    class FooConverter:
+        tags = []
+        types = [Foo]
+
+        def select_tag(self, *args):
+            return None
+
+        def to_yaml_tree(self, obj, tag, ctx):
+            return Bar(obj.value)
+
+        def from_yaml_tree(self, node, tag, ctx):
+            raise NotImplementedError()
+
+    class BarConverter:
+        tags = []
+        types = [Bar]
+
+        def select_tag(self, *args):
+            return None
+
+        def to_yaml_tree(self, obj, tag, ctx):
+            return Baz(obj.value)
+
+        def from_yaml_tree(self, node, tag, ctx):
+            raise NotImplementedError()
+
+    class BazConverter:
+        tags = []
+        types = [Baz]
+
+        def select_tag(self, *args):
+            return None
+
+        def to_yaml_tree(self, obj, tag, ctx):
+            return Foo(obj.value)
+
+        def from_yaml_tree(self, node, tag, ctx):
+            raise NotImplementedError()
+
+    extension = FullExtension(converters=[FooConverter(), BarConverter(), BazConverter()])
+    with config_context() as config:
+        config.add_extension(extension)
+
+        for typ in (Foo, Bar, Baz):
+            obj = typ(42)
+            with pytest.raises(TypeError, match=r"Conversion cycle detected"):
+                roundtrip_object(obj)

asdf/core/_converters/reference.py

@@ -0,0 +1,18 @@
+from asdf.extension import Converter
+
+
+class ReferenceConverter(Converter):
+    tags = []
+    types = ["asdf.reference.Reference"]
+
+    def to_yaml_tree(self, obj, tag, ctx):
+        from asdf.generic_io import relative_uri
+
+        uri = relative_uri(ctx.url, obj._uri) if ctx.url is not None else obj._uri
+        return {"$ref": uri}
+
+    def from_yaml_tree(self, node, tag, ctx):
+        raise NotImplementedError()
+
+    def select_tag(self, obj, tags, ctx):
+        return None

asdf/core/_extensions.py

@@ -3,6 +3,7 @@
 from ._converters.complex import ComplexConverter
 from ._converters.constant import ConstantConverter
 from ._converters.external_reference import ExternalArrayReferenceConverter
+from ._converters.reference import ReferenceConverter
 from ._converters.tree import (
     AsdfObjectConverter,
     ExtensionMetadataConverter,
@@ -21,6 +22,7 @@
     HistoryEntryConverter(),
     SoftwareConverter(),
     SubclassMetadataConverter(),
+    ReferenceConverter(),
 ]
 
 

asdf/extension/_converter.py

@@ -76,9 +76,11 @@ def select_tag(self, obj, tags, ctx):
 
         Returns
         -------
-        str
+        str or None
             The selected tag.  Should be one of the tags passed
-            to this method in the `tags` parameter.
+            to this method in the `tags` parameter.  If `None`
+            the result of ``to_yaml_tree`` will be used to look
+            up the next converter for this object.
         """
         return tags[0]
 
@@ -208,6 +210,11 @@ def __init__(self, delegate, extension):
         self._tags = sorted(relevant_tags)
 
         self._types = []
+
+        if not len(self._tags) and not hasattr(delegate, "select_tag"):
+            # this converter supports no tags so don't inspect the types
+            return
+
         for typ in delegate.types:
             if isinstance(typ, (str, type)):
                 self._types.append(typ)
@@ -252,8 +259,8 @@ def select_tag(self, obj, ctx):
 
         Returns
         -------
-        str
-            Selected tag.
+        str or None
+            Selected tag or `None` to defer conversion.
         """
         method = getattr(self._delegate, "select_tag", None)
         if method is None:

asdf/extension/_manager.py

@@ -33,22 +33,18 @@ def __init__(self, extensions):
                 if tag_def.tag_uri not in self._tag_defs_by_tag:
                     self._tag_defs_by_tag[tag_def.tag_uri] = tag_def
             for converter in extension.converters:
-                # If a converter's tags do not actually overlap with
-                # the extension tag list, then there's no reason to
-                # use it.
-                if len(converter.tags) > 0:
-                    for tag in converter.tags:
-                        if tag not in self._converters_by_tag:
-                            self._converters_by_tag[tag] = converter
-                    for typ in converter.types:
-                        if isinstance(typ, str):
-                            if typ not in self._converters_by_type:
-                                self._converters_by_type[typ] = converter
-                        else:
-                            type_class_name = get_class_name(typ, instance=False)
-                            if typ not in self._converters_by_type and type_class_name not in self._converters_by_type:
-                                self._converters_by_type[typ] = converter
-                                self._converters_by_type[type_class_name] = converter
+                for tag in converter.tags:
+                    if tag not in self._converters_by_tag:
+                        self._converters_by_tag[tag] = converter
+                for typ in converter.types:
+                    if isinstance(typ, str):
+                        if typ not in self._converters_by_type:
+                            self._converters_by_type[typ] = converter
+                    else:
+                        type_class_name = get_class_name(typ, instance=False)
+                        if typ not in self._converters_by_type and type_class_name not in self._converters_by_type:
+                            self._converters_by_type[typ] = converter
+                            self._converters_by_type[type_class_name] = converter
 
             validators.update(extension.validators)
 

asdf/reference.py

@@ -11,7 +11,7 @@
 
 import numpy as np
 
-from . import _types, generic_io, treeutil, util
+from . import generic_io, treeutil, util
 from .util import patched_urllib_parse
 
 __all__ = ["resolve_fragment", "Reference", "find_references", "resolve_references", "make_reference"]
@@ -42,9 +42,7 @@ def resolve_fragment(tree, pointer):
     return tree
 
 
-class Reference(_types._AsdfType):
-    yaml_tag = "tag:yaml.org,2002:map"
-
+class Reference:
     def __init__(self, uri, base_uri=None, asdffile=None, target=None):
         self._uri = uri
         if asdffile is not None:
@@ -105,15 +103,6 @@ def __call__(self, **kwargs):
     def __contains__(self, item):
         return item in self._get_target()
 
-    @classmethod
-    def to_tree(cls, data, ctx):
-        uri = generic_io.relative_uri(ctx.uri, data._uri) if ctx.uri is not None else data._uri
-        return {"$ref": uri}
-
-    @classmethod
-    def validate(cls, data):
-        pass
-
 
 def find_references(tree, ctx):
     """

asdf/yamlutil.py

@@ -222,6 +222,22 @@ def custom_tree_to_tagged_tree(tree, ctx, _serialization_context=None):
     def _convert_obj(obj):
         converter = extension_manager.get_converter_for_type(type(obj))
         tag = converter.select_tag(obj, _serialization_context)
+        converters = set()
+        # if select_tag returns None, converter.to_yaml_tree should return a new
+        # object which will be handled by a different converter
+        while tag is None:
+            converters.add(converter)
+            obj = converter.to_yaml_tree(obj, tag, _serialization_context)
+            try:
+                converter = extension_manager.get_converter_for_type(type(obj))
+            except KeyError:
+                # no converter supports this type, return it as-is
+                yield obj
+                return
+            if converter in converters:
+                msg = "Conversion cycle detected"
+                raise TypeError(msg)
+            tag = converter.select_tag(obj, _serialization_context)
         node = converter.to_yaml_tree(obj, tag, _serialization_context)
 
         if isinstance(node, GeneratorType):

docs/asdf/extending/converters.rst

@@ -174,6 +174,71 @@ the converter's list of tags and implement a ``select_tag`` method:
             else:
                 return Rectangle(node["width"], node["height"])
 
+.. _extending_converters_deferral:
+
+Deferring to another converter
+==============================
+
+Converters only support the exact types listed in ``Converter.types``. When a
+supported type is subclassed the extension will need to be updated to support
+the new subclass. There are a few options for supporting subclasses.
+
+If serialization of the subclass needs to differ from the superclass a new
+Converter, tag and schema should be defined.
+
+If the subclass can be treated the same as the superclass (specifically if
+subclass instances can be serialized as the superclass) then the subclass
+can be added to the existing ``Converter.types``. Note that adding the
+subclass to the supported types (without making other changes to the Converter)
+will result in subclass instances using the same tag as the superclass. This
+means that any instances created during deserialization will always
+be of the superclass (subclass instances will never be read from an ASDF file).
+
+Another option (useful when modifying the existing Converter is not
+convenient) is to define a Converter that does not tag the subclass instance
+being serialized and instead defers to the existing Converter. Deferral
+is triggered by returning ``None`` from ``Converter.select_tag`` and
+implementing ``Converter.to_yaml_tree`` to convert the subclass instance
+into an instance of the (supported) superclass.
+
+For example, using the example ``Rectangle`` class above, let's say we
+have another class, ``AspectRectangle``, that represents a rectangle as
+a height and aspect ratio. We know we never need to deserialize this
+class for our uses and are ok with always reading ``Rectangle`` instances
+after saving ``AspectRectangle`` instances. In this case we can
+define a Converter for ``AspectRectangle`` that converts instances
+to ``Rectangle`` and defers to the ``RectangleConverter``.
+
+.. code-block:: python
+
+    class AspectRectangle(Rectangle):
+        def __init__(self, height, ratio):
+            self.height = height
+            self.ratio = ratio
+
+        def get_area(self):
+            width = self.height * self.ratio
+            return width * self.height
+
+
+    class AspectRectangleConverter(Converter):
+        tags = []
+        types = [AspectRectangle]
+
+        def select_tag(self, obj, tags, ctx):
+            return None  # defer to a different Converter
+
+        def to_yaml_tree(self, obj, tag, ctx):
+            # convert the instance of AspectRectangle (obj) to
+            # a supported type (Rectangle)
+            return Rectangle(obj.height * obj.ratio, obj.height)
+
+        def from_yaml_tree(self, node, tag, ctx):
+            raise NotImplementedError()
+
+Just like a non-deferring Converter this Converter will need to be
+added to an Extension and registered with asdf.
+
 .. _extending_converters_reference_cycles:
 
 Reference cycles