From a8336ebfa519a2b77f7e5d6ec283bf92335c4838 Mon Sep 17 00:00:00 2001 From: Brett Date: Tue, 18 Jul 2023 10:05:14 -0400 Subject: [PATCH 1/8] 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 53cb4711d..0d5dcc863 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 @@ -606,6 +607,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 12546d87c..01fa03b43 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 @@ -188,9 +189,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)): From 99157c9ee4825ad60cf7a5733b753ce0d38dc6fe Mon Sep 17 00:00:00 2001 From: Brett Date: Mon, 31 Jul 2023 11:20:44 -0400 Subject: [PATCH 2/8] raise Exception if Converter.select_tag receives no tags --- asdf/_tests/test_extension.py | 6 ++---- asdf/extension/_converter.py | 29 +++++++++-------------------- 2 files changed, 11 insertions(+), 24 deletions(-) diff --git a/asdf/_tests/test_extension.py b/asdf/_tests/test_extension.py index 0d5dcc863..05ad73b73 100644 --- a/asdf/_tests/test_extension.py +++ b/asdf/_tests/test_extension.py @@ -1,6 +1,5 @@ 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 @@ -610,8 +609,7 @@ def test_converter_proxy(): 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. + associated with the Converter should result in a failure to convert. """ class Foo: @@ -635,7 +633,7 @@ class FooExtension(Extension): tree = {"obj": Foo()} with config_context() as cfg: cfg.add_extension(FooExtension()) - with pytest.raises(RepresenterError, match=r"cannot represent an object"): + with pytest.raises(RuntimeError, match=r"Converter.select_tag was called with no supported tags.*"): roundtrip_object(tree) diff --git a/asdf/extension/_converter.py b/asdf/extension/_converter.py index 01fa03b43..8e2eaef79 100644 --- a/asdf/extension/_converter.py +++ b/asdf/extension/_converter.py @@ -3,7 +3,6 @@ types. Will eventually replace the `asdf.types` module. """ import abc -import inspect from asdf.util import get_class_name, uri_match @@ -83,6 +82,12 @@ def select_tag(self, obj, tags, ctx): the result of ``to_yaml_tree`` will be used to look up the next converter for this object. """ + if not len(tags): + msg = ( + "Converter.select_tag was called with no supported tags. " + f"The Converter tags {self.tags} might not be supported by the Extension." + ) + raise RuntimeError(msg) return tags[0] @abc.abstractmethod @@ -189,25 +194,9 @@ def __init__(self, delegate, extension): self._types = [] - 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 + 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)): From fe4b8721f29e8dad53e56beeb34170235bd5e59b Mon Sep 17 00:00:00 2001 From: Brett Date: Wed, 2 Aug 2023 15:30:53 -0400 Subject: [PATCH 3/8] remove default select_tag --- asdf/_tests/test_extension.py | 16 ++------- asdf/extension/_converter.py | 57 +++++++++--------------------- docs/asdf/extending/converters.rst | 4 +-- 3 files changed, 21 insertions(+), 56 deletions(-) diff --git a/asdf/_tests/test_extension.py b/asdf/_tests/test_extension.py index 05ad73b73..07485fa20 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 @@ -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: @@ -633,7 +621,7 @@ class FooExtension(Extension): tree = {"obj": Foo()} with config_context() as cfg: cfg.add_extension(FooExtension()) - with pytest.raises(RuntimeError, match=r"Converter.select_tag was called with no supported tags.*"): + with pytest.raises(RepresenterError, match=r"cannot represent an object"): roundtrip_object(tree) diff --git a/asdf/extension/_converter.py b/asdf/extension/_converter.py index 8e2eaef79..d9a0a02a8 100644 --- a/asdf/extension/_converter.py +++ b/asdf/extension/_converter.py @@ -14,7 +14,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,41 +71,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. - """ - if not len(tags): - msg = ( - "Converter.select_tag was called with no supported tags. " - f"The Converter tags {self.tags} might not be supported by the Extension." - ) - raise RuntimeError(msg) - return tags[0] - @abc.abstractmethod def to_yaml_tree(self, obj, tag, ctx): """ @@ -186,10 +167,6 @@ def __init__(self, delegate, extension): msg = "Converter property 'tags' must contain str values" 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) - self._tags = sorted(relevant_tags) self._types = [] diff --git a/docs/asdf/extending/converters.rst b/docs/asdf/extending/converters.rst index 4e5d12c9c..84fdfe941 100644 --- a/docs/asdf/extending/converters.rst +++ b/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`` - 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 @@ -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 `. Blocks are stored sequentially within a From 0b71489c9e05c3bf80869f33895b0e482dee89e7 Mon Sep 17 00:00:00 2001 From: Brett Date: Wed, 2 Aug 2023 15:40:41 -0400 Subject: [PATCH 4/8] clean up some links --- docs/asdf/extending/converters.rst | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/asdf/extending/converters.rst b/docs/asdf/extending/converters.rst index 84fdfe941..15baa685f 100644 --- a/docs/asdf/extending/converters.rst +++ b/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`` - an optional method that accepts a complex Python object and a list +`Converter.select_tag` - 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` 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` 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 @@ -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 From b0361126bd4e0c3ef3448d52d66543e3de971474 Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 4 Aug 2023 16:43:40 -0400 Subject: [PATCH 5/8] re-add check for >1 tag without select_tag in ConverterProxy turn error into warning for Converter subclasses that support >1 tag Previously this was allowed because of the default select_tag provided by Converter. Removing this default breaks at least one downstream package (gwcs) which relied on this bug. To help ease the transitiono to a Converter that doesn't provide select_tag (and fixes the bug caused by this) this commit changes the error to a warning for any convert subclasses that support >1 tag but do not implement select_tag. This more closely matches the previous behavior but introduces a warning to help packages relying on this bug to know they should be updated. add warning filter for gwcs extension bug --- asdf/_tests/test_extension.py | 26 +++++++++++++++++++++++++- asdf/extension/_converter.py | 19 +++++++++++++++++++ pyproject.toml | 6 ++++++ tox.ini | 6 ++++-- 4 files changed, 54 insertions(+), 3 deletions(-) diff --git a/asdf/_tests/test_extension.py b/asdf/_tests/test_extension.py index 07485fa20..c45125d5a 100644 --- a/asdf/_tests/test_extension.py +++ b/asdf/_tests/test_extension.py @@ -5,7 +5,7 @@ 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, @@ -911,3 +911,27 @@ def from_yaml_tree(self, node, tag, ctx): obj = typ(42) with pytest.raises(TypeError, match=r"Conversion cycle detected"): roundtrip_object(obj) + + +def test_warning_for_default_select_tag(): + class Foo: + pass + + class FooConverter(Converter): + 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) + with config_context() as config: + with pytest.warns(AsdfWarning, match="Converter handles multiple tags"): + config.add_extension(extension) diff --git a/asdf/extension/_converter.py b/asdf/extension/_converter.py index d9a0a02a8..857cb02c4 100644 --- a/asdf/extension/_converter.py +++ b/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 @@ -167,6 +169,23 @@ def __init__(self, delegate, extension): msg = "Converter property 'tags' must contain str values" raise TypeError(msg) + if len(relevant_tags) > 1 and not hasattr(delegate, "select_tag"): + if isinstance(delegate, Converter): + # 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) self._types = [] diff --git a/pyproject.toml b/pyproject.toml index eef720ff3..033db9114 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -114,6 +114,12 @@ remote_data_strict = true filterwarnings = [ 'error', 'ignore:numpy.ndarray size changed:RuntimeWarning', + # gwcs 0.18.3 contains a bug, new asdf versions will correctly raise + # a warning for this bug. We're ignoring the warning to allow the extension + # to be loaded and used. This warning filter (and similar ones in tox.ini) + # can be removed when the bug is fixed in gwcs/asdf-wcs-schemas. + # See: https://github.com/spacetelescope/gwcs/pull/469 + 'ignore:asdf.extensions plugin from package gwcs==0.18.3:asdf.exceptions.AsdfWarning', ] # Configuration for pytest-doctestplus text_file_format = 'rst' diff --git a/tox.ini b/tox.ini index 47e1d5a6d..42c494872 100644 --- a/tox.ini +++ b/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" From 00e88ec2a6438686af0e551d1531e00ca486425c Mon Sep 17 00:00:00 2001 From: Brett Date: Fri, 18 Aug 2023 14:11:13 -0400 Subject: [PATCH 6/8] drop gwcs as test dependency --- pyproject.toml | 8 +------- requirements-dev.txt | 1 - 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 033db9114..28f281a6b 100644 --- a/pyproject.toml +++ b/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", @@ -114,12 +114,6 @@ remote_data_strict = true filterwarnings = [ 'error', 'ignore:numpy.ndarray size changed:RuntimeWarning', - # gwcs 0.18.3 contains a bug, new asdf versions will correctly raise - # a warning for this bug. We're ignoring the warning to allow the extension - # to be loaded and used. This warning filter (and similar ones in tox.ini) - # can be removed when the bug is fixed in gwcs/asdf-wcs-schemas. - # See: https://github.com/spacetelescope/gwcs/pull/469 - 'ignore:asdf.extensions plugin from package gwcs==0.18.3:asdf.exceptions.AsdfWarning', ] # Configuration for pytest-doctestplus text_file_format = 'rst' diff --git a/requirements-dev.txt b/requirements-dev.txt index 34a857c96..658447c10 100644 --- a/requirements-dev.txt +++ b/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 From c8e45adac8b2d695fae36b01b2888262d01a03be Mon Sep 17 00:00:00 2001 From: Brett Date: Tue, 22 Aug 2023 11:32:36 -0400 Subject: [PATCH 7/8] work around Converter virtual subclass support add test for warning or error when a converter supports multiple tags but does not implement select_tag --- asdf/_tests/test_extension.py | 11 ++++++++--- asdf/extension/_converter.py | 4 +++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/asdf/_tests/test_extension.py b/asdf/_tests/test_extension.py index c45125d5a..5f5559712 100644 --- a/asdf/_tests/test_extension.py +++ b/asdf/_tests/test_extension.py @@ -913,11 +913,14 @@ def from_yaml_tree(self, node, tag, ctx): roundtrip_object(obj) -def test_warning_for_default_select_tag(): +@pytest.mark.parametrize("is_subclass", [True, False]) +def test_warning_or_error_for_default_select_tag(is_subclass): class Foo: pass - class FooConverter(Converter): + ParentClass = Converter if is_subclass else object + + class FooConverter(ParentClass): tags = ["asdf://somewhere.org/tags/foo-*"] types = [Foo] @@ -932,6 +935,8 @@ def from_yaml_tree(self, node, tag, ctx): "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 pytest.warns(AsdfWarning, match="Converter handles multiple tags"): + with ctx_type(exception_class, match="Converter handles multiple tags"): config.add_extension(extension) diff --git a/asdf/extension/_converter.py b/asdf/extension/_converter.py index 857cb02c4..8116ea3d6 100644 --- a/asdf/extension/_converter.py +++ b/asdf/extension/_converter.py @@ -170,7 +170,9 @@ def __init__(self, delegate, extension): raise TypeError(msg) if len(relevant_tags) > 1 and not hasattr(delegate, "select_tag"): - if isinstance(delegate, Converter): + # we cannot use isinstance here because Converter supports + # virtual subclasses + if Converter in delegate.__class__.__bases__: # 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 From 7556138f1dbf865424779833431cd5cf385edc80 Mon Sep 17 00:00:00 2001 From: Brett Date: Mon, 11 Sep 2023 16:23:06 -0400 Subject: [PATCH 8/8] use __mro__ when checking for Converter subclasses The previous code only warned for converters (that support multiple tags but not select tag) that were direct subclasses of Converter because of the use of __bases__ instead of __mro__. --- asdf/_tests/test_extension.py | 10 +++++++++- asdf/extension/_converter.py | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/asdf/_tests/test_extension.py b/asdf/_tests/test_extension.py index 5f5559712..821760d7a 100644 --- a/asdf/_tests/test_extension.py +++ b/asdf/_tests/test_extension.py @@ -914,12 +914,20 @@ def from_yaml_tree(self, node, tag, ctx): @pytest.mark.parametrize("is_subclass", [True, False]) -def test_warning_or_error_for_default_select_tag(is_subclass): +@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] diff --git a/asdf/extension/_converter.py b/asdf/extension/_converter.py index 8116ea3d6..7bf3681d3 100644 --- a/asdf/extension/_converter.py +++ b/asdf/extension/_converter.py @@ -172,7 +172,7 @@ def __init__(self, delegate, extension): if len(relevant_tags) > 1 and not hasattr(delegate, "select_tag"): # we cannot use isinstance here because Converter supports # virtual subclasses - if Converter in delegate.__class__.__bases__: + 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