Skip to content

Commit

Permalink
fix Converter type indexing
Browse files Browse the repository at this point in the history
Prior to asdf-format#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 asdf-format#1561).
  • Loading branch information
braingram committed Aug 18, 2023
1 parent 53b87b4 commit 0956ef3
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 3 deletions.
33 changes: 33 additions & 0 deletions asdf/_tests/test_extension.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -599,6 +600,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])
Expand Down
23 changes: 20 additions & 3 deletions asdf/extension/_converter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)):
Expand Down

0 comments on commit 0956ef3

Please sign in to comment.