Merge pull request #1594 from braingram/default_select_tag

fix Converter type indexing for Converters not supported by Extension

asdf/_tests/test_extension.py

@@ -1,10 +1,11 @@
 import pytest
 from packaging.specifiers import SpecifierSet
+from yaml.representer import RepresenterError
 
 from asdf import AsdfFile, config_context
 from asdf._tests._helpers import assert_extension_correctness
 from asdf._types import CustomType
-from asdf.exceptions import AsdfDeprecationWarning, ValidationError
+from asdf.exceptions import AsdfDeprecationWarning, AsdfWarning, ValidationError
 from asdf.extension import (
     Compressor,
     Converter,
@@ -501,19 +502,6 @@ def from_yaml_tree(self, *args):
 
     assert issubclass(ConverterNoSubclass, Converter)
 
-    class ConverterWithSubclass(Converter):
-        tags = []
-        types = []
-
-        def to_yaml_tree(self, *args):
-            pass
-
-        def from_yaml_tree(self, *args):
-            pass
-
-    # Confirm the behavior of the default select_tag implementation
-    assert ConverterWithSubclass().select_tag(object(), ["tag1", "tag2"], object()) == "tag1"
-
 
 def test_converter_proxy():
     # Test the minimum set of converter methods:
@@ -606,6 +594,37 @@ def test_converter_proxy():
         ConverterProxy(MinimumConverter(tags=[extension.tags[0].tag_uri], types=[object()]), extension)
 
 
+def test_converter_subclass_with_no_supported_tags():
+    """
+    Adding a Converter to an Extension that doesn't list support for the tags
+    associated with the Converter should result in a failure to convert.
+    """
+
+    class Foo:
+        pass
+
+    class FooConverterWithSubclass(Converter):
+        tags = ["asdf://somewhere.org/tags/foo-1.0.0"]
+        types = [Foo]
+
+        def to_yaml_tree(self, *args):
+            pass
+
+        def from_yaml_tree(self, *args):
+            pass
+
+    class FooExtension(Extension):
+        tags = []
+        converters = [FooConverterWithSubclass()]
+        extension_uri = "asdf://somewhere.org/extensions/foo-1.0.0"
+
+    tree = {"obj": Foo()}
+    with config_context() as cfg:
+        cfg.add_extension(FooExtension())
+        with pytest.raises(RepresenterError, match=r"cannot represent an object"):
+            roundtrip_object(tree)
+
+
 def test_get_cached_asdf_extension_list():
     extension = LegacyExtension()
     extension_list = get_cached_asdf_extension_list([extension])
@@ -892,3 +911,40 @@ def from_yaml_tree(self, node, tag, ctx):
             obj = typ(42)
             with pytest.raises(TypeError, match=r"Conversion cycle detected"):
                 roundtrip_object(obj)
+
+
+@pytest.mark.parametrize("is_subclass", [True, False])
+@pytest.mark.parametrize("indirect", [True, False])
+def test_warning_or_error_for_default_select_tag(is_subclass, indirect):
+    class Foo:
+        pass
+
+    ParentClass = Converter if is_subclass else object
+
+    if indirect:
+
+        class IntermediateClass(ParentClass):
+            pass
+
+        ParentClass = IntermediateClass
+
+    class FooConverter(ParentClass):
+        tags = ["asdf://somewhere.org/tags/foo-*"]
+        types = [Foo]
+
+        def to_yaml_tree(self, obj, tag, ctx):
+            return {}
+
+        def from_yaml_tree(self, node, tag, ctx):
+            return Foo()
+
+    tags = [
+        "asdf://somewhere.org/tags/foo-1.0.0",
+        "asdf://somewhere.org/tags/foo-2.0.0",
+    ]
+    extension = FullExtension(converters=[FooConverter()], tags=tags)
+    ctx_type = pytest.warns if is_subclass else pytest.raises
+    exception_class = AsdfWarning if is_subclass else RuntimeError
+    with config_context() as config:
+        with ctx_type(exception_class, match="Converter handles multiple tags"):
+            config.add_extension(extension)

asdf/extension/_converter.py

@@ -3,7 +3,9 @@
 types.  Will eventually replace the `asdf.types` module.
 """
 import abc
+import warnings
 
+from asdf.exceptions import AsdfWarning
 from asdf.util import get_class_name, uri_match
 
 
@@ -14,7 +16,23 @@ class Converter(abc.ABC):
 
     Implementing classes must provide the `tags` and `types`
     properties and `to_yaml_tree` and `from_yaml_tree` methods.
-    The `select_tag` method is optional.
+    The ``select_tag`` method is optional.
+
+    If implemented, ``select_tag`` should accept 3 parameters
+
+        obj : object
+            Instance of the custom type being converted.  Guaranteed
+            to be an instance of one of the types listed in the
+            `types` property.
+        tags : list of str
+            List of active tags to choose from.  Guaranteed to match
+            one of the tag patterns listed in the 'tags' property.
+        ctx : asdf.asdf.SerializationContext
+            Context of the current serialization request.
+
+    and return a str, the selected tag (should be one of tags) or
+    `None` which will trigger the result of ``to_yaml_tree`` to be
+    used to look up the next converter for this object.
     """
 
     @classmethod
@@ -55,35 +73,6 @@ def types(self):
             If str, the fully qualified class name of the type.
         """
 
-    def select_tag(self, obj, tags, ctx):
-        """
-        Select the tag to use when converting an object to YAML.
-        Typically only one tag will be active in a given context, but
-        converters that map one type to many tags must provide logic
-        to choose the appropriate tag.
-
-        Parameters
-        ----------
-        obj : object
-            Instance of the custom type being converted.  Guaranteed
-            to be an instance of one of the types listed in the
-            `types` property.
-        tags : list of str
-            List of active tags to choose from.  Guaranteed to match
-            one of the tag patterns listed in the 'tags' property.
-        ctx : asdf.extension.SerializationContext
-            Context of the current serialization request.
-
-        Returns
-        -------
-        str or None
-            The selected tag.  Should be one of the tags passed
-            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]
-
     @abc.abstractmethod
     def to_yaml_tree(self, obj, tag, ctx):
         """
@@ -181,8 +170,23 @@ def __init__(self, delegate, extension):
                 raise TypeError(msg)
 
         if len(relevant_tags) > 1 and not hasattr(delegate, "select_tag"):
-            msg = "Converter handles multiple tags for this extension, but does not implement a select_tag method."
-            raise RuntimeError(msg)
+            # we cannot use isinstance here because Converter supports
+            # virtual subclasses
+            if Converter in delegate.__class__.__mro__:
+                # prior to asdf 3.0 Converter provided a default select_tag
+                # to provide backwards compatibility allow Converter subclasses
+                # to be registered with >1 tag but produce a warning
+                msg = (
+                    "Converter handles multiple tags for this extension, "
+                    "but does not implement a select_tag method. "
+                    "This previously worked because Converter subclasses inherited "
+                    "the now removed select_tag. This will be an error in a future "
+                    "version of asdf"
+                )
+                warnings.warn(msg, AsdfWarning)
+            else:
+                msg = "Converter handles multiple tags for this extension, but does not implement a select_tag method."
+                raise RuntimeError(msg)
 
         self._tags = sorted(relevant_tags)
 

docs/asdf/extending/converters.rst

@@ -51,7 +51,7 @@ situation.
 Additionally, the Converter interface includes a method that must be implemented
 when some logic is required to select the tag to assign to a ``to_yaml_tree`` result:
 
-`Converter.select_tag` - a method that accepts a complex Python object and a list
+`Converter.select_tag<Converter>` - an optional method that accepts a complex Python object and a list
 candidate tags and returns the tag that should be used to serialize the object.
 
 A simple example
@@ -137,7 +137,7 @@ Multiple tags
 
 Now say we want to map our one Rectangle class to one of two tags, either
 rectangle-1.0.0 or square-1.0.0.  We'll need to add square-1.0.0 to
-the converter's list of tags and implement a ``select_tag`` method:
+the converter's list of tags and implement a `select_tag<Converter>` method:
 
 .. code-block:: python
 
@@ -179,7 +179,7 @@ the converter's list of tags and implement a ``select_tag`` method:
 Deferring to another converter
 ==============================
 
-Converters only support the exact types listed in ``Converter.types``. When a
+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.
 
@@ -188,7 +188,7 @@ 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
+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
@@ -197,8 +197,8 @@ 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
+is triggered by returning ``None`` from `Converter.select_tag<Converter>` 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
@@ -357,7 +357,7 @@ storing data in ASDF blocks.
 
 For applications that require more flexibility,
 Converters can control block storage through use of the `asdf.extension.SerializationContext`
-provided as an argument to `Converter.to_yaml_tree` `Converter.from_yaml_tree` and `Converter.select_tag`.
+provided as an argument to `Converter.to_yaml_tree` `Converter.from_yaml_tree` and ``Converter.select_tag``.
 
 It is helpful to first review some details of how asdf
 :ref:`stores block <asdf-standard:block>`. Blocks are stored sequentially within a
@@ -414,14 +414,14 @@ A simple example of a Converter using block storage to store the ``payload`` for
 
 .. asdf:: block_converter_example.asdf
 
-During read, ``Converter.from_yaml_tree`` will be called. Within this method
+During read, `Converter.from_yaml_tree` will be called. Within this method
 the Converter can prepare to access a block by calling
 ``SerializationContext.get_block_data_callback``. This will return a function
 that when called will return the contents of the block (to support lazy
 loading without keeping a reference to the ``SerializationContext`` (which is meant
 to be a short lived and lightweight object).
 
-During write, ``Converter.to_yaml_tree`` will be called. The Converter can
+During write, `Converter.to_yaml_tree` will be called. The Converter can
 use ``SerializationContext.find_available_block_index`` to find the location of an
 available block for writing. The data to be written to the block can be provided
 as an ``ndarray`` or a callable function that will return a ``ndarray`` (note that

pyproject.toml

@@ -47,7 +47,7 @@ docs = [
 tests = [
   "astropy>=5.0.4",
   "fsspec[http]>=2022.8.2",
-  "gwcs>=0.18.3",
+  "asdf-astropy>=0.4.0",
   "lz4>=0.10",
   "psutil",
   "pytest>=6",

requirements-dev.txt

@@ -5,7 +5,6 @@ git+https://github.com/asdf-format/asdf-transform-schemas
 git+https://github.com/asdf-format/asdf-unit-schemas.git
 git+https://github.com/asdf-format/asdf-wcs-schemas
 git+https://github.com/astropy/astropy
-git+https://github.com/spacetelescope/gwcs
 #git+https://github.com/yaml/pyyaml.git
 # jsonschema 4.18 contains incompatible changes: https://github.com/asdf-format/asdf/issues/1485
 #git+https://github.com/python-jsonschema/jsonschema

tox.ini

@@ -243,7 +243,8 @@ commands_pre =
     pip install -r {env_tmp_dir}/requirements.txt
     pip freeze
 commands =
-    pytest roman_datamodels/tests
+    pytest roman_datamodels/tests \
+        -W "ignore:asdf.extensions plugin from package gwcs==0.18.3:asdf.exceptions.AsdfWarning"
 
 [testenv:weldx]
 change_dir = {env_tmp_dir}
@@ -288,4 +289,5 @@ commands_pre =
     pip install -r {env_tmp_dir}/requirements.txt
     pip freeze
 commands =
-    pytest dkist
+    pytest dkist \
+        -W "ignore:asdf.extensions plugin from package gwcs==0.18.3:asdf.exceptions.AsdfWarning"