From 15d63f153d0f455a4f29b4ab8361f028f8220bd5 Mon Sep 17 00:00:00 2001
From: Brett <brettgraham@gmail.com>
Date: Tue, 18 Jul 2023 10:05:14 -0400
Subject: [PATCH] fix Converter type indexing

Prior to #1561 the types supported by a converter that
doesn't support any tags that are supported by the extnesion
were ignored. The modifications in that PR checked
if the converter implemented 'select_tag' and if not
ignored the types. However if the converter implemented
'select_tag' the converter types were indexed (as the converter
could return None from 'select_tag' and defer to a new
converter).

If a converter inherits from Converter it gains the
select_tag method from the parent Converter class
(which provides documentation of the method but doesn't
implement functionality that is otherwise covered by
ConverterProxy). This creates an issue when indexing
the converter tags/types if a converter is included
in an extension that doesn't list support for the
tags supported by the converter.

The changes included here check if 'select_tag'
is implemented by a subclass of Converter. If overwritten,
the types of the converter are indexed (to allow the
converter to defer conversion). However, if 'select_tag'
is merely inherited by Converter, the types supported by
the converter are ignored (agreeing with behavior prior to #1561).
---
 asdf/_tests/test_extension.py | 33 +++++++++++++++++++++++++++++++++
 asdf/extension/_converter.py  | 23 ++++++++++++++++++++---
 2 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/asdf/_tests/test_extension.py b/asdf/_tests/test_extension.py
index 6adc39a6b..59b37e311 100644
--- a/asdf/_tests/test_extension.py
+++ b/asdf/_tests/test_extension.py
@@ -1,5 +1,6 @@
 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
@@ -607,6 +608,38 @@ 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 not index the types listed for the
+    Converter.
+    """
+
+    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])
diff --git a/asdf/extension/_converter.py b/asdf/extension/_converter.py
index 4044f9d5c..80a3ea341 100644
--- a/asdf/extension/_converter.py
+++ b/asdf/extension/_converter.py
@@ -3,6 +3,7 @@
 types.  Will eventually replace the `asdf.types` module.
 """
 import abc
+import inspect
 
 from asdf.util import get_class_name, uri_match
 
@@ -211,9 +212,25 @@ def __init__(self, delegate, extension):
 
         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
+        if not len(self._tags):
+            # this converter supports no tags supported by the extension
+            # before indexing it's types we need to check select_tag
+
+            # check if it implements select_tag (and not just because it
+            # subclasses Converter)
+            if not hasattr(delegate, "select_tag"):
+                return
+
+            # find which class implements select_tag
+            for class_ in inspect.getmro(delegate.__class__):
+                if "select_tag" in class_.__dict__:
+                    if class_ is not Converter:
+                        # a non-Converter class implements select_tag so consider the types for this Converter
+                        break
+                    else:
+                        # this converter implements select_tag only because it inherits from Converter
+                        # don't index it's types
+                        return
 
         for typ in delegate.types:
             if isinstance(typ, (str, type)):