Repository: spacetelescope/asdf (python)

Issue:
usage of oneOf for schema validation
I am having trouble getting my schemas to validate when using `oneOf` structures. (running asdf=2.6.0, python=3.7)

Consider the following python/asdf example that I expected to work but somehow cannot implement.
```python
#python class for debugging
@dataclass
class OneOfClass:
    data: dict


class OneOfClassType(CustomType):
    name = "debug/oneOf_class"
    version = "1.0.0"
    types = [OneOfClass]

    @classmethod
    def to_tree(cls, node: OneOfClass, ctx):
        return node.data

    @classmethod
    def from_tree(cls, tree, ctx):
        obj = OneOfClass(tree)
        return obj
```
The schema I wanted to implement:
```yaml
%YAML 1.1
---
$schema: "http://stsci.edu/schemas/yaml-schema/draft-01"
id: "http://somewhere.com/schemas/debug/oneOf_class-1.0.0"
tag: "tag:somewhere.com:debug/oneOf_class-1.0.0"

title: |
  oneOf test schema
oneOf:
  - type: object
    properties:
      value:
        type: number
    required: [value]
    additionalProperties: false

  - type: object
    properties:
      value:
        type: string
    required: [value]
    additionalProperties: false
...
```

Now the following will fail:
```python
with asdf.AsdfFile(
    tree={"obj_a": OneOfClass({"value": "a string"})},
    extensions=[CustomExtension()],
) as ff:
    ff.write_to("oneOf.asdf")
```

```python
jsonschema.exceptions.ValidationError: {'value': 'a string'} is valid under each of OrderedDict([('type', 'object'), ('properties', OrderedDict([('value', OrderedDict([('type', 'string')]))])), ('required', ['value']), ('additionalProperties', False)]), OrderedDict([('type', 'object'), ('properties', OrderedDict([('value', OrderedDict([('type', 'number')]))])), ('required', ['value']), ('additionalProperties', False)])
```

If I only keep the string part of the schema as the single `oneOf` entry, it works. (same when trying to store a number, only works when the number part is the only `oneOf` list entry)
```yaml
oneOf:
#  - type: object
#    properties:
#      value:
#        type: number
#    required: [value]
#    additionalProperties: false

  - type: object
    properties:
      value:
        type: string
    required: [value]
    additionalProperties: false
...
```

When changing `oneOf` to `allOf` I get `jsonschema.exceptions.ValidationError: 'a string' is not of type 'number'`
Using `anyOf` in the original example works.

While I can get by using `anyOf` I would prefer `oneOf` for clarity and peace of mind. Any hints are appreciated.

Pull request:
Stop traversing oneOf and anyOf combiners when filling/removing defaults
This PR completely disables filling/remove defaults from subschemas within `oneOf` or `anyOf` combiners.  #809 demonstrates a serious bug in the defaults code that prevents use of `oneOf` in most realistic cases.  Given that there isn't a clear correct answer when choosing a default from among multiple `oneOf` subschemas, I'm inclined to ignore them when processing defaults.

I also propose that we do away with defaults for `anyOf`.  The way `anyOf` currently behaves is to always add a default from the first subschema.  If the object actually matches one of the subsequent subschemas, then we'll be adding a default from the wrong type!

With these changes, the remaining default rules are as follows, for a given missing property:

- If an object's own property definition has a default, use that
- Otherwise, if there is an `allOf` combiner, use the first default value available for that property among the combiner's subschemas.  The first listed subschema in the `allOf` array has highest precedence.
- Defaults from any other combiners are ignored

Milestoning this as 2.7.0, since it doesn't seem possible that anyone could be successfully using defaults with these combiners given the bugs.

Resolves #809 

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 3edd1b94..1f72c5d7 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -22,6 +22,9 @@
 
 - Drop support for jsonschema 2.x. [#807]
 
+- Stop traversing oneOf and anyOf combiners when filling
+  or removing default values. [#811]
+
 2.6.1 (unreleased)
 ------------------
 
diff --git a/asdf/schema.py b/asdf/schema.py
index 4aacbd2f..faa97f25 100644
--- a/asdf/schema.py
+++ b/asdf/schema.py
@@ -178,7 +178,7 @@ def validate_fill_default(validator, properties, instance, schema):
 
 
 FILL_DEFAULTS = util.HashableDict()
-for key in ('allOf', 'anyOf', 'oneOf', 'items'):
+for key in ('allOf', 'items'):
     FILL_DEFAULTS[key] = mvalidators.Draft4Validator.VALIDATORS[key]
 FILL_DEFAULTS['properties'] = validate_fill_default
 
@@ -198,7 +198,7 @@ def validate_remove_default(validator, properties, instance, schema):
 
 
 REMOVE_DEFAULTS = util.HashableDict()
-for key in ('allOf', 'anyOf', 'oneOf', 'items'):
+for key in ('allOf', 'items'):
     REMOVE_DEFAULTS[key] = mvalidators.Draft4Validator.VALIDATORS[key]
 REMOVE_DEFAULTS['properties'] = validate_remove_default
 


Test patch:
diff --git a/asdf/tests/data/default-1.0.0.yaml b/asdf/tests/data/default-1.0.0.yaml
index d5a1a601..51067a5a 100644
--- a/asdf/tests/data/default-1.0.0.yaml
+++ b/asdf/tests/data/default-1.0.0.yaml
@@ -12,4 +12,44 @@ properties:
     properties:
       c:
         type: integer
-        default: 82
\ No newline at end of file
+        default: 82
+  d:
+    allOf:
+      - type: object
+        properties:
+          e:
+            type: integer
+            default: 122
+      - type: object
+        properties:
+          f:
+            type: integer
+            default: 162
+  g:
+    anyOf:
+      - type: object
+        properties:
+          h:
+            type: integer
+            default: 202
+      - type: object
+        properties:
+          i:
+            type: integer
+            default: 242
+  j:
+    oneOf:
+      - type: object
+        properties:
+          k:
+            type: integer
+            default: 282
+        required: [k]
+        additionalProperties: false
+      - type: object
+        properties:
+          l:
+            type: integer
+            default: 322
+        required: [l]
+        additionalProperties: false
diff --git a/asdf/tests/data/one_of-1.0.0.yaml b/asdf/tests/data/one_of-1.0.0.yaml
new file mode 100644
index 00000000..5bc5e88d
--- /dev/null
+++ b/asdf/tests/data/one_of-1.0.0.yaml
@@ -0,0 +1,21 @@
+%YAML 1.1
+---
+$schema: "http://stsci.edu/schemas/yaml-schema/draft-01"
+id: "http://nowhere.org/schemas/custom/one_of-1.0.0"
+title: |
+  oneOf test schema
+oneOf:
+  - type: object
+    properties:
+      value:
+        type: number
+    required: [value]
+    additionalProperties: false
+
+  - type: object
+    properties:
+      value:
+        type: string
+    required: [value]
+    additionalProperties: false
+...
diff --git a/asdf/tests/test_schema.py b/asdf/tests/test_schema.py
index e146b576..6f5dc3d4 100644
--- a/asdf/tests/test_schema.py
+++ b/asdf/tests/test_schema.py
@@ -359,26 +359,82 @@ def test_fill_and_remove_defaults():
     yaml = """
 custom: !<tag:nowhere.org:custom/default-1.0.0>
   b: {}
+  d: {}
+  g: {}
+  j:
+    l: 362
     """
     buff = helpers.yaml_to_asdf(yaml)
     with asdf.open(buff, extensions=[DefaultTypeExtension()]) as ff:
         assert 'a' in ff.tree['custom']
         assert ff.tree['custom']['a'] == 42
         assert ff.tree['custom']['b']['c'] == 82
+        # allOf combiner should fill defaults from all subschemas:
+        assert ff.tree['custom']['d']['e'] == 122
+        assert ff.tree['custom']['d']['f'] == 162
+        # anyOf combiners should be ignored:
+        assert 'h' not in ff.tree['custom']['g']
+        assert 'i' not in ff.tree['custom']['g']
+        # oneOf combiners should be ignored:
+        assert 'k' not in ff.tree['custom']['j']
+        assert ff.tree['custom']['j']['l'] == 362
 
     buff.seek(0)
     with asdf.open(buff, extensions=[DefaultTypeExtension()],
                             do_not_fill_defaults=True) as ff:
         assert 'a' not in ff.tree['custom']
         assert 'c' not in ff.tree['custom']['b']
+        assert 'e' not in ff.tree['custom']['d']
+        assert 'f' not in ff.tree['custom']['d']
+        assert 'h' not in ff.tree['custom']['g']
+        assert 'i' not in ff.tree['custom']['g']
+        assert 'k' not in ff.tree['custom']['j']
+        assert ff.tree['custom']['j']['l'] == 362
         ff.fill_defaults()
         assert 'a' in ff.tree['custom']
         assert ff.tree['custom']['a'] == 42
         assert 'c' in ff.tree['custom']['b']
         assert ff.tree['custom']['b']['c'] == 82
+        assert ff.tree['custom']['b']['c'] == 82
+        assert ff.tree['custom']['d']['e'] == 122
+        assert ff.tree['custom']['d']['f'] == 162
+        assert 'h' not in ff.tree['custom']['g']
+        assert 'i' not in ff.tree['custom']['g']
+        assert 'k' not in ff.tree['custom']['j']
+        assert ff.tree['custom']['j']['l'] == 362
         ff.remove_defaults()
         assert 'a' not in ff.tree['custom']
         assert 'c' not in ff.tree['custom']['b']
+        assert 'e' not in ff.tree['custom']['d']
+        assert 'f' not in ff.tree['custom']['d']
+        assert 'h' not in ff.tree['custom']['g']
+        assert 'i' not in ff.tree['custom']['g']
+        assert 'k' not in ff.tree['custom']['j']
+        assert ff.tree['custom']['j']['l'] == 362
+
+
+def test_one_of():
+    """
+    Covers https://github.com/spacetelescope/asdf/issues/809
+    """
+    class OneOfType(dict, types.CustomType):
+        name = 'one_of'
+        organization = 'nowhere.org'
+        version = (1, 0, 0)
+        standard = 'custom'
+
+    class OneOfTypeExtension(CustomExtension):
+        @property
+        def types(self):
+            return [OneOfType]
+
+    yaml = """
+one_of: !<tag:nowhere.org:custom/one_of-1.0.0>
+  value: foo
+    """
+    buff = helpers.yaml_to_asdf(yaml)
+    with asdf.open(buff, extensions=[OneOfTypeExtension()]) as ff:
+        assert ff['one_of']['value'] == 'foo'
 
 
 def test_tag_reference_validation():