diff --git a/asdf/_tests/test_extension.py b/asdf/_tests/test_extension.py index 53cb4711d..821760d7a 100644 --- a/asdf/_tests/test_extension.py +++ b/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) diff --git a/asdf/extension/_converter.py b/asdf/extension/_converter.py index 12546d87c..7bf3681d3 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 @@ -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) diff --git a/docs/asdf/extending/converters.rst b/docs/asdf/extending/converters.rst index 4e5d12c9c..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` - 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 @@ -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 @@ -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 @@ -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 diff --git a/pyproject.toml b/pyproject.toml index eef720ff3..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", 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 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"