Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for dynamic scope and keywords #1041

Merged
merged 1 commit into from
Dec 1, 2020

Conversation

jdesrosiers
Copy link
Member

This PR fixes errors in the spec for the dynamic and keywords and dynamic scope. The spec describes dynamic scope to only be resolvable when processing an instance. This wasn't true for the recursive keywords, and it isn't true for the dynamic keywords either. You need to know all the schemas and how they reference each other, but you don't need an instance. The vast majority of these changes cover that issue. The rest fixes minor typos and such.

The path from this root schema to any particular keyword (that
includes any "$ref" and "$dynamicRef" keywords that may have
been resolved) is considered the keyword's "validation path."
<cref>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the cref here because I'm mostly sure it's just left over from the recursive keywords and doesn't apply any more. At the least, it's not correct that, $dyanmicAnchor is only allowed in the root schema. If someone thinks there's still some value in this cref, let me know and we'll try to correct it instead of delete it.

Comment on lines -1507 to -1514
<cref>
Requiring both the initial and final URI fragment to be defined
by "$dynamicAnchor" ensures that the more common "$anchor"
never unexpectedly changes the dynamic resolution process
due to a naming conflict across resources. Users of
"$dynamicAnchor" are expected to be aware of the possibility
of such name collisions, while users of "$anchor" are not.
</cref>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this cref because it appears to be wrong. There's no way that an $anchor can change dynamic scope resolution and even if it could, it's not clear how bookending could possibly solve that problem. If anyone knows what this means, please share and I'll put it back in.

@Relequestual
Copy link
Member

Many thanks!

@Relequestual Relequestual merged commit 126ccfe into json-schema-org:master Dec 1, 2020
@karenetheridge
Copy link
Member

The spec describes dynamic scope to only be resolvable when processing an instance. This wasn't true for the recursive keywords, and it isn't true for the dynamic keywords either.

How is it not true? The dynamic path is affected by the instance data because it can change the path through applicator keywords, which can change which references are followed or not followed. Example:

{
  "$defs": {
    "yes": { ... something complicated containing $refs and $dynamicRefs... },
    "no": { ..something complicated containing other $refs and $dynamicRefs ... },
  },
  "if": { "const": "yes" },
  "then": { "$ref": "#/$defs/yes" },
  "else": { "$ref": "#/$defs/no" },
}

We cannot know the dynamic scope in a particular subschema without knowing how we got there, and how we got there depends on the evaluation result of the if/then/else applicator keywords.

@jdesrosiers
Copy link
Member Author

@karenetheridge If I'm wrong about this, it should be relatively easy to prove because it would mean my implementation has a bug. I resolve all URLs during the compile step before it has an instance. All we need is one example we can run on https://json-schema.hyperjump.io and get instant feedback. We can then use that example in the test suite as well.

@Relequestual
Copy link
Member

What $schema should it have?

@jdesrosiers
Copy link
Member Author

Sorry, I forgot to mention, use "$schema": "https://json-schema.org/draft/future/schema" on https://json-schema.hyperjump.io to get support for the dynamic keywords.

@karenetheridge
Copy link
Member

karenetheridge commented Dec 2, 2020

I implemented this first with recursiveRef, so I could test it locally (if others make affirmative noises I can add this to the 2019-09 test suite):

schema:

{
    "$id": "main.json",
    "$defs": {
        "inner": {
            "$id": "inner.json",
            "$recursiveAnchor": true,
            "title": "inner",
            "additionalProperties": {
                "$recursiveRef": "#"
            }
        }
    },
    "if": { "propertyNames": { "pattern": "^[a-m]" } },
    "then": {
        "title": "any type of node",
        "$id": "anyLeafNode.json",
        "$recursiveAnchor": true,
        "$ref": "main.json#/$defs/inner"
    },
    "else": {
        "title": "integer node",
        "$id": "integerNode.json",
        "$recursiveAnchor": true,
        "type": [ "object", "integer" ],
        "$ref": "main.json#/$defs/inner"
    }
}

passing data: { "alpha": 1.1 }

produces result:

{
  "annotations" : [
    {
      "absoluteKeywordLocation" : "inner.json#/title",
      "annotation" : "inner",
      "instanceLocation" : "/alpha",
      "keywordLocation" : "/then/$ref/additionalProperties/$recursiveRef/$ref/title"
    },
    {
      "absoluteKeywordLocation" : "anyLeafNode.json#/title",
      "annotation" : "any type of node",
      "instanceLocation" : "/alpha",
      "keywordLocation" : "/then/$ref/additionalProperties/$recursiveRef/title"
    },
    {
      "absoluteKeywordLocation" : "inner.json#/additionalProperties",
      "annotation" : [
        "alpha"
      ],
      "instanceLocation" : "",
      "keywordLocation" : "/then/$ref/additionalProperties"
    },
    {
      "absoluteKeywordLocation" : "inner.json#/title",
      "annotation" : "inner",
      "instanceLocation" : "",
      "keywordLocation" : "/then/$ref/title"
    },
    {
      "absoluteKeywordLocation" : "anyLeafNode.json#/title",
      "annotation" : "any type of node",
      "instanceLocation" : "",
      "keywordLocation" : "/then/title"
    }
  ],
  "valid" : true
}

failing data: { "november": 1.1 }

produces results:

{
  "errors" : [
    {
      "absoluteKeywordLocation" : "integerNode.json#/type",
      "error" : "wrong type (expected one of object, integer)",
      "instanceLocation" : "/november",
      "keywordLocation" : "/else/$ref/additionalProperties/$recursiveRef/type"
    },
    {
      "absoluteKeywordLocation" : "inner.json#/additionalProperties",
      "error" : "not all additional properties are valid",
      "instanceLocation" : "",
      "keywordLocation" : "/else/$ref/additionalProperties"
    },
    {
      "absoluteKeywordLocation" : "main.json#/else",
      "error" : "subschema is not valid",
      "instanceLocation" : "",
      "keywordLocation" : "/else"
    }
  ],
  "valid" : false
}

I believe the equivalent schema, in draft2020-12, would be:

{
    "$id": "main.json",
    "$defs": {
        "inner": {
            "$id": "inner.json",
            "$dynamicAnchor": "node",
            "title": "inner",
            "additionalProperties": {
                "$dynamicRef": "#node"
            }
        }
    },
    "if": { "propertyNames": { "pattern": "^[a-m]" } },
    "then": {
        "title": "any type of node",
        "$id": "anyLeafNode.json",
        "$dynamicAnchor": "node",
        "$ref": "main.json#/$defs/inner"
    },
    "else": {
        "title": "integer node",
        "$id": "integerNode.json",
        "$dynamicAnchor": "node",
        "type": [ "object", "integer" ],
        "$ref": "main.json#/$defs/inner"
    }
}

Before runtime, we cannot know the destination of the $dynamicRef because we do not know the path by which we got here, and anywhere in the higher scope could have declared a $dynamicAnchor -- and indeed both of the possible source paths (/then or /else) have one.

https://json-schema.hyperjump.io/ thinks both of those data instances pass against the schema. I believe the second one should not (similarly to the 2019-09 version of the schema with $recursiveRef).

@jdesrosiers
Copy link
Member Author

Well, this is embarrassing, but it seems you found a bug not related to this discussion. https://json-schema.hyperjump.io doesn't give the right result for this schema either and there are no branches.

{
    "$id": "main.json",
    "$defs": {
        "inner": {
            "$id": "inner.json",
            "$dynamicAnchor": "node",
            "title": "inner",
            "additionalProperties": {
                "$dynamicRef": "#node"
            }
        }
    },
    "allOf": [{
        "title": "integer node",
        "$id": "integerNode.json",
        "$dynamicAnchor": "node",
        "type": [ "object", "integer" ],
        "$ref": "main.json#/$defs/inner"
    }]
}

I think the problem is with the mutually recursive references. I'm working to understand the problem better so I can either fix it quickly or find a workaround so we can see if the schema exposes a bug related to this discussion.

karenetheridge added a commit to karenetheridge/JSON-Schema-Modern that referenced this pull request Dec 3, 2020
@jdesrosiers
Copy link
Member Author

@karenetheridge, I found the bug and I've confirmed that your schema does trip up my validator. Let's definitely add it to the test suite.

@Relequestual
Copy link
Member

So does this mean @karenetheridge is correct and the contents of this PR is wrong in relation to...

The spec describes dynamic scope to only be resolvable when processing an instance. This wasn't true for the recursive keywords, and it isn't true for the dynamic keywords either.

?

@karenetheridge
Copy link
Member

ok, test case added! json-schema-org/JSON-Schema-Test-Suite#450

@jdesrosiers jdesrosiers deleted the dynamic-fixes branch December 3, 2020 18:54
@jdesrosiers
Copy link
Member Author

Yes, @Relequestual, that means I was wrong. I'll submit up a PR reverting the relevant changes.

@Relequestual
Copy link
Member

Thanks! =]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants