Repository: spacetelescope/asdf
Pull request #738 (2020-01-24 19:33:08)

Resolve external references in custom schemas
Previously, external references in custom schemas were not being resolved (see https://github.com/spacetelescope/asdf/issues/683).  This was clearly deliberate, though I can't think of a reason why that would be the desired behavior.  This PR changes the custom schema behavior to resolve external references, and fixes an issue in the tests that prevented the example custom schema from resolving correctly.

Resolves https://github.com/spacetelescope/asdf/issues/683

codecov[bot]: # [Codecov](https://codecov.io/gh/spacetelescope/asdf/pull/738?src=pr&el=h1) Report
> Merging [#738](https://codecov.io/gh/spacetelescope/asdf/pull/738?src=pr&el=desc) into [master](https://codecov.io/gh/spacetelescope/asdf/commit/0368a3d79ff8ed6fcfefffb2bef85e09c7d765ec?src=pr&el=desc) will **decrease** coverage by `0.26%`.
> The diff coverage is `62.5%`.

[![Impacted file tree graph](https://codecov.io/gh/spacetelescope/asdf/pull/738/graphs/tree.svg?width=650&token=uNpl2ecnHm&height=150&src=pr)](https://codecov.io/gh/spacetelescope/asdf/pull/738?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #738      +/-   ##
==========================================
- Coverage   93.31%   93.04%   -0.27%     
==========================================
  Files          39       39              
  Lines        4396     4387       -9     
==========================================
- Hits         4102     4082      -20     
- Misses        294      305      +11
```


| [Impacted Files](https://codecov.io/gh/spacetelescope/asdf/pull/738?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [asdf/asdf.py](https://codecov.io/gh/spacetelescope/asdf/pull/738/diff?src=pr&el=tree#diff-YXNkZi9hc2RmLnB5) | `93.84% <100%> (-0.02%)` | :arrow_down: |
| [asdf/schema.py](https://codecov.io/gh/spacetelescope/asdf/pull/738/diff?src=pr&el=tree#diff-YXNkZi9zY2hlbWEucHk=) | `88.75% <25%> (-4.48%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/spacetelescope/asdf/pull/738?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/spacetelescope/asdf/pull/738?src=pr&el=footer). Last update [0368a3d...0e5f4a9](https://codecov.io/gh/spacetelescope/asdf/pull/738?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index fcd1c345..2a1973fc 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -8,6 +8,9 @@
 
 - Remove unnecessary dependency on six. [#739]
 
+- Resolve external references in custom schemas, and deprecate
+  asdf.schema.load_custom_schema.  [#738]
+
 2.5.1 (2020-01-07)
 ------------------
 
diff --git a/asdf/asdf.py b/asdf/asdf.py
index 756a5916..d5eeff54 100644
--- a/asdf/asdf.py
+++ b/asdf/asdf.py
@@ -109,17 +109,15 @@ class AsdfFile(versioning.VersionedMixin):
             standard.
 
         """
+        self._extensions = []
+        self._extension_metadata = {}
+        self._process_extensions(extensions)
 
         if custom_schema is not None:
-            self._custom_schema = schema.load_custom_schema(custom_schema)
-            schema.check_schema(self._custom_schema)
+            self._custom_schema = schema.load_schema(custom_schema, self.resolver, True)
         else:
             self._custom_schema = None
 
-        self._extensions = []
-        self._extension_metadata = {}
-
-        self._process_extensions(extensions)
         self._ignore_version_mismatch = ignore_version_mismatch
         self._ignore_unrecognized_tag = ignore_unrecognized_tag
         self._ignore_implicit_conversion = ignore_implicit_conversion
diff --git a/asdf/schema.py b/asdf/schema.py
index fb181102..6665620c 100644
--- a/asdf/schema.py
+++ b/asdf/schema.py
@@ -346,23 +346,12 @@ HARDCODED_SCHEMA = {
 
 @lru_cache()
 def load_custom_schema(url):
-    # Avoid circular import
-    from .tags.core import AsdfObject
-    custom = load_schema(url, resolve_local_refs=True)
-    core = load_schema(AsdfObject.yaml_tag)
-
-    def update(d, u):
-        for k, v in u.items():
-            # Respect the property ordering of the core schema
-            if k == 'propertyOrder' and k in d:
-                d[k] = u[k] + d[k]
-            elif isinstance(v, Mapping):
-                d[k] = update(d.get(k, {}), v)
-            else:
-                d[k] = v
-        return d
-
-    return update(custom, core)
+    warnings.warn(
+        "The 'load_custom_schema(...)' function is deprecated. Use"
+        "'load_schema' instead.",
+        AsdfDeprecationWarning
+    )
+    return load_schema(url, resolve_references=True)
 
 
 def load_schema(url, resolver=None, resolve_references=False,
@@ -389,7 +378,14 @@ def load_schema(url, resolver=None, resolve_references=False,
         within the same schema. This will automatically be handled when passing
         `resolve_references=True`, but it may be desirable in some cases to
         control local reference resolution separately.
+        This parameter is deprecated.
     """
+    if resolve_local_refs is True:
+        warnings.warn(
+            "The 'resolve_local_refs' parameter is deprecated.",
+            AsdfDeprecationWarning
+        )
+
     if resolver is None:
         # We can't just set this as the default in load_schema's definition
         # because invoking get_default_resolver at import time leads to a circular import.
diff --git a/docs/asdf/features.rst b/docs/asdf/features.rst
index 0f4cb42c..3e99d9d5 100644
--- a/docs/asdf/features.rst
+++ b/docs/asdf/features.rst
@@ -82,7 +82,7 @@ implementations of ASDF as long as the proper extensions are available.
 .. toctree::
     :maxdepth: 2
 
-    using_extensions 
+    using_extensions
 
 .. _schema_validation:
 
@@ -122,10 +122,9 @@ expectations:
 
     %YAML 1.1
     ---
+    id: "http://example.com/schemas/your-custom-schema"
     $schema: "http://stsci.edu/schemas/yaml-schema/draft-01"
-    id: "http://stsci.edu/schemas/asdf/core/asdf-1.1.0"
 
-    tag: "tag:stsci.edu:asdf/core/asdf-1.1.0"
     type: object
     properties:
       image:
@@ -155,10 +154,7 @@ expectations:
 This schema restricts the kinds of files that will be accepted as valid to
 those that contain a top-level ``image`` property that is an ``ndarray``, and
 a top-level ``metadata`` property that contains information about the time the
-image was taken and the resolution of the image. Note that the schema uses the
-same ``id`` and ``tag`` as the `top-level core schema`_ from the ASDF Standard.
-This is because it is validating the file at the top level, but is imposing
-restrictions beyond what is normally required for an ASDF file.
+image was taken and the resolution of the image.
 
 In order to use this schema for a secondary validation pass, we pass the
 `custom_schema` argument to either `asdf.open` or the `AsdfFile` constructor.
@@ -178,6 +174,10 @@ Similarly, if we wished to use this schema when creating new files:
     new_af = asdf.AsdfFile(custom_schema='image_schema.yaml')
     ...
 
+If your custom schema is registered with ASDF in an extension, you may
+pass the schema URI (``http://example.com/schemas/your-custom-schema``, in this
+case) instead of a file path.
+
 .. _top-level core schema:
     https://github.com/spacetelescope/asdf-standard/blob/master/schemas/stsci.edu/asdf/core/asdf-1.1.0.yaml
 


Test patch:
diff --git a/asdf/tests/data/custom_schema_definitions.yaml b/asdf/tests/data/custom_schema_definitions.yaml
index 1e21db0d..7411983c 100644
--- a/asdf/tests/data/custom_schema_definitions.yaml
+++ b/asdf/tests/data/custom_schema_definitions.yaml
@@ -1,9 +1,7 @@
 %YAML 1.1
 ---
 $schema: "http://stsci.edu/schemas/yaml-schema/draft-01"
-id: "http://stsci.edu/schemas/asdf/core/asdf-1.1.0"
 
-tag: "tag:stsci.edu:asdf/core/asdf-1.1.0"
 type: object
 properties:
   thing:
diff --git a/asdf/tests/data/custom_schema_external_ref.yaml b/asdf/tests/data/custom_schema_external_ref.yaml
new file mode 100644
index 00000000..7734a360
--- /dev/null
+++ b/asdf/tests/data/custom_schema_external_ref.yaml
@@ -0,0 +1,17 @@
+%YAML 1.1
+---
+$schema: "http://stsci.edu/schemas/yaml-schema/draft-01"
+title: |
+  Custom schema with an external reference, used for testing.
+
+description: |
+  This schema is used to test custom schema validation with an external reference.
+
+type: object
+properties:
+  foo:
+    anyOf:
+      - $ref: "http://stsci.edu/schemas/asdf/core/software-1.0.0"
+
+required: [foo]
+additionalProperties: true
diff --git a/asdf/tests/test_schema.py b/asdf/tests/test_schema.py
index 2c234b68..1a265efe 100644
--- a/asdf/tests/test_schema.py
+++ b/asdf/tests/test_schema.py
@@ -2,7 +2,6 @@
 # -*- coding: utf-8 -*-
 
 import io
-import os
 import re
 import warnings
 
@@ -666,7 +665,7 @@ def test_assert_roundtrip_with_extension(tmpdir):
 
 def test_custom_validation_bad(tmpdir):
     custom_schema_path = helpers.get_test_data_path('custom_schema.yaml')
-    asdf_file = os.path.join(str(tmpdir), 'out.asdf')
+    asdf_file = str(tmpdir.join('out.asdf'))
 
     # This tree does not conform to the custom schema
     tree = {'stuff': 42, 'other_stuff': 'hello'}
@@ -677,22 +676,22 @@ def test_custom_validation_bad(tmpdir):
 
     # Creating file using custom schema should fail
     with pytest.raises(ValidationError):
-        with asdf.AsdfFile(tree, custom_schema=custom_schema_path) as ff:
+        with asdf.AsdfFile(tree, custom_schema=custom_schema_path):
             pass
 
     # Opening file without custom schema should pass
-    with asdf.open(asdf_file) as ff:
+    with asdf.open(asdf_file):
         pass
 
     # Opening file with custom schema should fail
     with pytest.raises(ValidationError):
-        with asdf.open(asdf_file, custom_schema=custom_schema_path) as ff:
+        with asdf.open(asdf_file, custom_schema=custom_schema_path):
             pass
 
 
 def test_custom_validation_good(tmpdir):
     custom_schema_path = helpers.get_test_data_path('custom_schema.yaml')
-    asdf_file = os.path.join(str(tmpdir), 'out.asdf')
+    asdf_file = str(tmpdir.join('out.asdf'))
 
     # This tree conforms to the custom schema
     tree = {
@@ -703,7 +702,7 @@ def test_custom_validation_good(tmpdir):
     with asdf.AsdfFile(tree, custom_schema=custom_schema_path) as ff:
         ff.write_to(asdf_file)
 
-    with asdf.open(asdf_file, custom_schema=custom_schema_path) as ff:
+    with asdf.open(asdf_file, custom_schema=custom_schema_path):
         pass
 
 
@@ -716,7 +715,7 @@ def test_custom_validation_pathlib(tmpdir):
     from pathlib import Path
 
     custom_schema_path = Path(helpers.get_test_data_path('custom_schema.yaml'))
-    asdf_file = os.path.join(str(tmpdir), 'out.asdf')
+    asdf_file = str(tmpdir.join('out.asdf'))
 
     # This tree conforms to the custom schema
     tree = {
@@ -727,13 +726,13 @@ def test_custom_validation_pathlib(tmpdir):
     with asdf.AsdfFile(tree, custom_schema=custom_schema_path) as ff:
         ff.write_to(asdf_file)
 
-    with asdf.open(asdf_file, custom_schema=custom_schema_path) as ff:
+    with asdf.open(asdf_file, custom_schema=custom_schema_path):
         pass
 
 
 def test_custom_validation_with_definitions_good(tmpdir):
     custom_schema_path = helpers.get_test_data_path('custom_schema_definitions.yaml')
-    asdf_file = os.path.join(str(tmpdir), 'out.asdf')
+    asdf_file = str(tmpdir.join('out.asdf'))
 
     # This tree conforms to the custom schema
     tree = {
@@ -743,13 +742,13 @@ def test_custom_validation_with_definitions_good(tmpdir):
     with asdf.AsdfFile(tree, custom_schema=custom_schema_path) as ff:
         ff.write_to(asdf_file)
 
-    with asdf.open(asdf_file, custom_schema=custom_schema_path) as ff:
+    with asdf.open(asdf_file, custom_schema=custom_schema_path):
         pass
 
 
 def test_custom_validation_with_definitions_bad(tmpdir):
     custom_schema_path = helpers.get_test_data_path('custom_schema_definitions.yaml')
-    asdf_file = os.path.join(str(tmpdir), 'out.asdf')
+    asdf_file = str(tmpdir.join('out.asdf'))
 
     # This tree does NOT conform to the custom schema
     tree = {
@@ -762,19 +761,77 @@ def test_custom_validation_with_definitions_bad(tmpdir):
 
     # Creating file with custom schema should fail
     with pytest.raises(ValidationError):
-        with asdf.AsdfFile(tree, custom_schema=custom_schema_path) as ff:
+        with asdf.AsdfFile(tree, custom_schema=custom_schema_path):
             pass
 
     # Opening file without custom schema should pass
-    with asdf.open(asdf_file) as ff:
+    with asdf.open(asdf_file):
         pass
 
     # Opening file with custom schema should fail
     with pytest.raises(ValidationError):
-        with asdf.open(asdf_file, custom_schema=custom_schema_path) as ff:
+        with asdf.open(asdf_file, custom_schema=custom_schema_path):
             pass
 
 
+def test_custom_validation_with_external_ref_good(tmpdir):
+    custom_schema_path = helpers.get_test_data_path('custom_schema_external_ref.yaml')
+    asdf_file = str(tmpdir.join('out.asdf'))
+
+    # This tree conforms to the custom schema
+    tree = {
+        'foo': asdf.tags.core.Software(name="Microsoft Windows", version="95")
+    }
+
+    with asdf.AsdfFile(tree, custom_schema=custom_schema_path) as ff:
+        ff.write_to(asdf_file)
+
+    with asdf.open(asdf_file, custom_schema=custom_schema_path):
+        pass
+
+
+def test_custom_validation_with_external_ref_bad(tmpdir):
+    custom_schema_path = helpers.get_test_data_path('custom_schema_external_ref.yaml')
+    asdf_file = str(tmpdir.join('out.asdf'))
+
+    # This tree does not conform to the custom schema
+    tree = {
+        'foo': False
+    }
+
+    # Creating file without custom schema should pass
+    with asdf.AsdfFile(tree) as ff:
+        ff.write_to(asdf_file)
+
+    # Creating file with custom schema should fail
+    with pytest.raises(ValidationError):
+        with asdf.AsdfFile(tree, custom_schema=custom_schema_path):
+            pass
+
+    # Opening file without custom schema should pass
+    with asdf.open(asdf_file):
+        pass
+
+    # Opening file with custom schema should fail
+    with pytest.raises(ValidationError):
+        with asdf.open(asdf_file, custom_schema=custom_schema_path):
+            pass
+
+
+def test_load_custom_schema_deprecated():
+    custom_schema_path = helpers.get_test_data_path('custom_schema.yaml')
+
+    with pytest.deprecated_call():
+        schema.load_custom_schema(custom_schema_path)
+
+
+def test_load_schema_resolve_local_refs_deprecated():
+    custom_schema_path = helpers.get_test_data_path('custom_schema_definitions.yaml')
+
+    with pytest.deprecated_call():
+        schema.load_schema(custom_schema_path, resolve_local_refs=True)
+
+
 def test_nonexistent_tag(tmpdir):
     """
     This tests the case where a node is tagged with a type that apparently