test $recursiveRef + $recursiveAnchor #391

karenetheridge · 2020-06-04T20:31:07Z

No description provided.

jdesrosiers · 2020-06-05T00:27:58Z

lgtm 👍

ssilverman

These are great tests. May I suggest two more types:

Same but with $recursiveAnchor set to false, and
Same but with an absent $recursiveAnchor, to test that the behaviour of $recursiveRef does indeed follow that of $ref.

ryangalamb · 2020-06-10T21:57:14Z

tests/draft2019-09/recursiveRef.json

+            "$defs": {
+                "myobject": {
+                    "$id": "myobject.json",
+                    "$recursiveAnchor": true,


To be clear, if this "$recursiveAnchor" were removed, would this schema still validate the same way?

In this case yes, because there is an $id at that subschema that sets a new absolute uri, so # would resolve to the same position.

karenetheridge · 2020-06-10T22:02:23Z

"$recursiveAnchor": false would be a schema violation, and there seems to be growing consensus that we should handle cases like that in a different way.

$recursiveRef without $recursiveAnchor is covered in the first test case.

These are pretty basic tests, and definitely could be expanded upon. These also only involve a single document, without bringing in extra documents from the 'remotes' folder, and an implementation could potentially behave differently in that case.

ssilverman · 2020-06-11T00:44:13Z

$recursiveAnchor=false isn't a schema violation. I'd still like to see this case added.

(And for reference, I'm the one who brought up the fact that meta-schema tests should be treated differently in the first place at the end of April, no need to explain that; here's an issue recently created to discuss the topic: #397)

ssilverman · 2020-06-11T00:45:44Z

Here's a spec quote describing $recursiveAnchor (from https://json-schema.org/draft/2019-09/json-schema-core.html#rfc.section.8.2.4.2.2):

If this keyword is set to false, the base URI is unchanged.

karenetheridge · 2020-06-11T14:15:02Z

$recursiveAnchor=false isn't a schema violation. I'd still like to see this case added.

https://github.com/json-schema-org/json-schema-spec/blob/master/meta/core.json#L34-L38

ssilverman · 2020-06-11T14:49:29Z

Interesting. That looks like an error in the schema because it disagrees with the spec. I’ll submit a PR to fix that in the schema.

ChALkeR · 2020-07-04T21:35:23Z

Seems to work for me. Is something stalling this?
The $recursiveAnchor: false case can come in separately after the schema changes, can't it?

Without this PR, there are almost no tests for $recursiveRef + $recursiveAnchor.

karenetheridge · 2020-07-04T22:58:03Z

agreed. I added a test with $recursiveAnchor=false.

…arget schema resource see https://json-schema.org/draft/2019-09/json-schema-core.html#rfc.section.8.2.4.2

karenetheridge · 2020-07-05T00:09:40Z

I also added another test case which tripped up my implementation (!!) -- where there is a $recursiveAnchor in the current dynamic scope, but it is not present at the initial target schema.

ssilverman · 2020-07-05T06:47:48Z

agreed. I added a test with $recursiveAnchor=false.

Note that this will fail schema processing, per your comment (#391 (comment)), until this PR goes through: json-schema-org/json-schema-spec#945 (for future readers of this PR needing context if they see a failure for these tests).

Julian · 2020-08-31T13:46:08Z

This seems approved right? Any reason it hasn't been merged?

ChALkeR · 2020-08-31T14:48:04Z

Took a second look, the updated tests don't pass in my impl, investigating.
At $recursiveRef with no $recursiveAnchor in the initial target schema resource.

ChALkeR · 2020-08-31T16:11:42Z

Seems to be valid, but perhaps a variant of recursiveRef6 should be added when only the inner schema recursiveAnchor is set to true.

i.e.:

"schema": {
    "$id": "http://localhost:4242/recursiveRef7/base.json",
    "anyOf": [
        { "type": "boolean" },
        {
            "type": "object",
            "additionalProperties": {
                "$id": "http://localhost:4242/recursiveRef7/inner.json",
                "$recursiveAnchor": true,
                "anyOf": [
                    { "type": "integer" },
                    { "type": "object", "additionalProperties": { "$recursiveRef": "#" } }
                ]
            }
        }
    ]
},

jdesrosiers · 2020-08-31T17:37:12Z

I also added another test case [...] where there is a $recursiveAnchor in the current dynamic scope, but it is not present at the initial target schema.

I don't think this is correct. My understanding is that $recursiveAnchor is not intended to be a flag to enable or disable $recursiveRef functionality. It marks a schema as being allowed to be the target of a $recursiveRef. A $recursiveRef only behaves like $ref if there is no $recursiveAnchor anywhere in it's dynamic scope. Not every schema in the chain needs to have a $recursiveAnchor.

I re-read the spec and there are parts that do make it sound like $recrusiveAnchor must be present in the same schema for $recrusiveRef to attempt dynamic scope resolution. I think I have the right interpretation based on discussions for and the spec for $dynamicAnchor/$dynamicRef that has been merged for the next draft.

@handrews, would you mind chiming in and letting us know you intended $recursiveAnchor to be interpreted in a case like this.

handrews · 2020-08-31T18:17:41Z

@jdesrosiers is correct.

Is the $recursiveRef target a schema object with "$recursiveAnchor": true?
Is their at least one other schema object higher/further out in the dynamic scope that also sets "$recursiveAnchor": true?
If the answer is "yes" to both, then the value of $recursiveRef is resolved against the base URI at the point of the outermost/highest schema object in the dynamic scope that sets "$recursiveAnchor": true.

The "yes+no" case is actually a degenerate form of the "yes+yes" case, but in practical terms it has the same effect as the "no+(no||yes)" case.

We treat it a mistake to not have a $recursiveAnchor: true in the schema that uses $recursiveRef, so that throws in non-lax modes. Refs: json-schema-org/JSON-Schema-Test-Suite#391

karenetheridge · 2020-11-10T23:53:52Z

Is this good to merge as-is, and work on additional tests in a separate PR?

Julian · 2020-11-11T13:21:05Z

Sounds good to me too!

jdesrosiers · 2020-11-11T18:00:36Z

tests/draft2019-09/recursiveRef.json

+        ]
+    },
+    {
+        "description": "$recursiveRef with no $recursiveAnchor in the initial target schema resource",


This test is wrong as discussed in previous comments. Let's fix or remove this test, then I would love to see this merged!

@jdesrosiers Could you help me out by pointing to which tests are incorrect? I do not see any that are wrong. (revisions for $dynamicRef do not apply to this version of the spec.)

specifically, regarding recursiveRef6, there is no $recursiveAnchor: true at the initial $recursiveRef target, so it behaves like a regular $ref, and we only recurse to the inner subschema, and never the outer one. For the schemas that are used in these tests (and the initial target is '#'), we need to have $recursiveAnchor: true in both the base and inner subschemas in order to recurse to the base -- if either are missing, then we revert to normal $ref behaviour.

we need to have $recursiveAnchor: true in both the base and inner subschemas in order to recurse to the base

That's not correct. That's what I was pointing out in #391 (comment) (and Henry confirmed was the correct interpretation). $recursiveRef should use the first $recursiveAnchor in the heirarchy that was set. $recursiveAnchor is not required to be in every schema $recursiveRef appears in.

So, the second and third tests are wrong as well as the $comment.

(If this needs more discussion, I suggest removing these tests for now and getting the rest merged. We can add these back separately when everyone is in agreement.)

Okay, so we're talking about the test schemas with ids http://localhost:4242/recursiveRef2/schema.json and http://localhost:4242/recursiveRef3/schema.json? I need to be sure if I'm going to merge "the rest".

$recursiveAnchor is not required to be in every schema $recursiveRef appears in.

I don't see any tests that assert that in comments, nor assume that in their schemas. While all the $recursiveRef targets in these tests are '#', that resolves to a different position than the schema $recursiveRef appears in (because of the use of allOf). What is required is that the initial $recursiveRef target have a $recursiveAnchor: true in order to consider earlier/outer $recursiveAnchors, and all the tests are properly relying on that.

Please could you be specific about what schemas and subtests are not behaving properly? This is important not just to get the tests right, but to fix all the implementations that are assuming the tests are right and presently validate against these tests.

No, we're talking about the group of tests that this comment is on. The one with "description": "$recursiveRef with no $recursiveAnchor in the initial target schema resource",. The one with http://localhost:4242/recursiveRef6/inner.json.

I'll comment on the specific lines in question to make sure it can't be missed, but I'd suggest leaving out this whole group of tests for now as the premise of the test is wrong.

…(again reverts to normal $ref behaviour)

jdesrosiers · 2020-11-18T23:30:03Z

tests/draft2019-09/recursiveRef.json

+        ]
+    },
+    {
+        "description": "$recursiveRef with no $recursiveAnchor in the initial target schema resource",


No, we're talking about the group of tests that this comment is on. The one with "description": "$recursiveRef with no $recursiveAnchor in the initial target schema resource",. The one with http://localhost:4242/recursiveRef6/inner.json.

I'll comment on the specific lines in question to make sure it can't be missed, but I'd suggest leaving out this whole group of tests for now as the premise of the test is wrong.

jdesrosiers · 2020-11-18T23:36:10Z

tests/draft2019-09/recursiveRef.json

+                    "type": "object",
+                    "additionalProperties": {
+                        "$id": "http://localhost:4242/recursiveRef6/inner.json",
+                        "$comment": "there is no $recursiveAnchor: true here, so we do NOT recurse to the base",


This $comment is wrong. It doesn't matter that there isn't a $recursiveAnchor in the embedded schema. The $recursiveAnchor that is set on line 239 still applies to this schema.

How is the comment wrong? The initial target of the $recursiveRef is here: '#' resolves to inner.json. The rules for $recursiveRef explicitly says ""$recursiveRef" behaves identically to "$ref", except when its target schema contains "$recursiveAnchor" with a value of true." So the initial target is the final target - we recurse to inner.json.

@handrews, we were hoping you could help us resolve this. I previously commented on this PR the following.

My understanding is that $recursiveAnchor is not intended to be a flag to enable or disable $recursiveRef functionality. It marks a schema as being allowed to be the target of a $recursiveRef. A $recursiveRef only behaves like $ref if there is no $recursiveAnchor anywhere in it's dynamic scope. Not every schema in the chain needs to have a $recursiveAnchor.

You indicated that that was correct.

However, after talking with @karenetheridge and re-reading the spec, it does sound like the spec says that a $recursiveRef can only consider the dynamic scope if there is a $recursiveAnchor in it's lexical scope.

If you could have a look at these tests and tell us how you expect them to validate, I think that should clear up what the intended behavior was.

@jdesrosiers $recursiveRef has three steps IIRC

evaluate as if $ref to find the preliminary target schema

if the preliminary target schema has $recursiveAnchor: true, proceed to the next step, otherwise the preliminary target schema is the final target schema and the reference is considered resolved

unwind the dynamic scope to find the outermost $recursiveAnchor: true, regardless of whether intermediate dynamic scopes also have $recursiveAnchor: true; this is the intermediate target schema

find the base URI of the intermediate target schema, and re-resolve the value of $recursiveRef against that base URI - this produces the final target schema, and the reference is now resolved. (although I think we limited the value of $recursiveAnchor to # or at least said it always had to end in an empty fragment, so in practice the final target schema is always the same as the intermediate target schema)

Keep in mind that $recursive* have cases that don't work, notably as shown in links.json so there's no sensible complete test suite that both strictly conforms to the spec and works with links.json. I forget the details but the keywords are at least a little broken, and I'm really hoping no one actually uses 2019-09 and had advised only providing a test suite for 2020-NN in order to encourage that. But I guess we're supporting 2019-09 so 🤷 just be aware $recursive* doesn't entirely make sense.

great, so given this, it sounds like all the test cases in this PR are correct as written (note I made a few amendments that will be squashed together in the merge commit). @jdesrosiers do you agree?

(Note: as I said on slack#tests, I think it's perfectly acceptable to acknowledge that $recursiveRef isn't quite right in this draft, so one shouldn't try to do anything too complicated with it.)

Thanks, @handrews. That's what I got from re-reading the spec.

unwind the dynamic scope to find the outermost $recursiveAnchor: true, regardless of whether intermediate dynamic scopes also have $recursiveAnchor: true; this is the intermediate target schema

This is where our lines got crossed. We agreed on this part, but I was missing the part where we only do this step if we have $recursiveAnchor in the same schema as the $recursiveRef.

@karenetheridge these tests don't yet cover this case where we have an intermediate dynamic scope that doesn't have a $recursiveAnchor. It can be added later, but I wanted to point that out.

As far as I can tell, the two step resolve and $recursiveAnchor enabling/disabling $recursiveRef seem unnecessary. I can't tell why it doesn't just resolve the $recursiveRef against the current dynamic scope and be done. But, $dynamicAnchor/$dynamicRef handled this way better, so it doesn't matter any more.

but I was missing the part where we only do this step if we have $recursiveAnchor in the same schema as the $recursiveRef.

Correction: in the initial target schema. Not in the schema containing the $recursiveRef (which is the context or source or referring schema I don't remember what I called it)

jdesrosiers · 2020-11-18T23:37:25Z

tests/draft2019-09/recursiveRef.json

+                "description": "leaf node matches: recursion uses the inner schema",
+                "data": { "foo": { "bar": 1 } },
+                "valid": true


Should be valid=false. Recursion uses the outer schema.

I disagree, as per @handrews's latest comment.

jdesrosiers · 2020-11-18T23:37:50Z

tests/draft2019-09/recursiveRef.json

+                "description": "leaf node matches: recursion uses the inner schema",
+                "data": { "foo": { "bar": true } },
+                "valid": false


Should be valid=true. Recursion uses the outer schema.

I disagree, as per @handrews's latest comment.

jdesrosiers · 2020-11-18T23:40:33Z

tests/draft2019-09/recursiveRef.json

+    {
+        "description": "$recursiveRef with no $recursiveAnchor in the initial target schema resource",
+        "schema": {
+            "$id": "http://localhost:4242/recursiveRef6/base.json",
+            "$recursiveAnchor": true,
+            "anyOf": [
+                { "type": "boolean" },
+                {
+                    "type": "object",
+                    "additionalProperties": {
+                        "$id": "http://localhost:4242/recursiveRef6/inner.json",
+                        "$comment": "there is no $recursiveAnchor: true here, so we do NOT recurse to the base",
+                        "anyOf": [
+                            { "type": "integer" },
+                            { "type": "object", "additionalProperties": { "$recursiveRef": "#" } }
+                        ]
+                    }
+                }
+            ]
+        },
+        "tests": [
+            {
+                "description": "leaf node does not match; no recursion",
+                "data": { "foo": true },
+                "valid": false
+            },
+            {
+                "description": "leaf node matches: recursion uses the inner schema",
+                "data": { "foo": { "bar": 1 } },
+                "valid": true
+            },
+            {
+                "description": "leaf node matches: recursion uses the inner schema",
+                "data": { "foo": { "bar": true } },
+                "valid": false
+            }
+        ]
+    },


These are the lines that should be left out for now.

jdesrosiers

@karenetheridge It seems Henry doesn't have anything to say about the issue in question. Therefore, I'm going to assume the most likely scenario, which is that Henry and I misunderstood each other. I withdraw my objections. I'm happy with merging these tests as they are.

handrews · 2020-11-23T22:28:04Z

@jdesrosiers I just missed your earlier comment, let me check...

karenetheridge · 2020-11-25T02:21:19Z

Ok I'm going to rebase, squash and merge this tomorrow if I don't hear howls of anguish.

test $recursiveRef + $recursiveAnchor

karenetheridge · 2020-11-25T22:14:06Z

rebased, squashed and merged.

Julian · 2020-11-29T14:40:19Z

Looks like merging this has started failing the CI build. Not sure if that was as part of the merge or otherwise but if possible can you have a look?

ssilverman · 2020-11-30T01:51:22Z

Looks like a duplicate description.

karenetheridge · 2020-11-30T03:22:12Z

I'll fix.

* ether/unrecognized-keywords: Move the future keywords tests to optional. squash: add $schema keyword to be clear that strict conformance is requested squash: contains does not exist in draft4 squash: fix minContains sq - add $dynamic*, prefixItems to drafts 4,6,7 as well and fix keyword name squash: fix test names squash: $defs - definitions squash: remove tests of $anchor and $ref squash: add prefixItems: another new 2020-0X keyword squash: add $recursiveRef tests from #391 squash - fix 2019-09 future-keywords very basic test of $dynamicRef rename files to future-keywords.json some tests for keywords added in later drafts

test $recursiveRef + $recursiveAnchor

c9a788c

karenetheridge requested review from Julian and a team June 4, 2020 20:31

karenetheridge added a commit to karenetheridge/JSON-Schema-Test-Suite that referenced this pull request Jun 4, 2020

squash: add $recursiveRef tests from json-schema-org#391

3647c03

ssilverman reviewed Jun 10, 2020

View reviewed changes

ryangalamb reviewed Jun 10, 2020

View reviewed changes

karenetheridge added the missing test A request to add a test to the suite that is currently not covered elsewhere. label Jun 10, 2020

squash: use a unique $id to prevent namespace collisions across tests

18bac7a

squash: test $recursiveRef when $recursiveAnchor is false

33dca87

karenetheridge added 3 commits July 4, 2020 16:14

squash: test $recursiveRef when $recursiveAnchor is absent

13e3f91

squash: fix duplicate $id

2b4df7b

squash: test $recursive$ref with no $recursiveAnchor in the initial t…

18a5ffb

…arget schema resource see https://json-schema.org/draft/2019-09/json-schema-core.html#rfc.section.8.2.4.2

karenetheridge added a commit to karenetheridge/JSON-Schema-Modern that referenced this pull request Jul 5, 2020

add test added to json-schema-org/JSON-Schema-Test-Suite#391

a1cb0b7

karenetheridge added a commit to karenetheridge/JSON-Schema-Modern that referenced this pull request Jul 7, 2020

add test added to json-schema-org/JSON-Schema-Test-Suite#391

082b975

jdesrosiers mentioned this pull request Oct 28, 2020

Test dynamic behavior of "$recursiveRef" and "$recursiveAnchor" #298

Closed

2 tasks

jdesrosiers requested changes Nov 11, 2020

View reviewed changes

karenetheridge self-assigned this Nov 12, 2020

squash: one more test, with no recursiveAnchor in the outer resource …

7492ff6

…(again reverts to normal $ref behaviour)

jdesrosiers requested changes Nov 18, 2020

View reviewed changes

jdesrosiers approved these changes Nov 23, 2020

View reviewed changes

karenetheridge added a commit that referenced this pull request Nov 25, 2020

Merge pull request #391 from ether/recursiveRef (rebased, squashed)

bcf1dc8

test $recursiveRef + $recursiveAnchor

karenetheridge closed this Nov 25, 2020

karenetheridge deleted the ether/recursiveRef branch November 25, 2020 22:14

This was referenced Nov 25, 2020

"$recursiveRef" with "$recuriveAnchor": true but no outer "$recursiveAnchor" #297

Closed

Test that "$recursiveRef" without "$recursiveAnchor": true is same as "$ref" #295

Closed

Julian pushed a commit that referenced this pull request Jun 16, 2022

squash: add $recursiveRef tests from #391

ebb359d

test $recursiveRef + $recursiveAnchor #391

test $recursiveRef + $recursiveAnchor #391

Conversation

karenetheridge commented Jun 4, 2020

jdesrosiers commented Jun 5, 2020

ssilverman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenetheridge commented Jun 10, 2020

ssilverman commented Jun 11, 2020 • edited Loading

ssilverman commented Jun 11, 2020 • edited Loading

karenetheridge commented Jun 11, 2020

ssilverman commented Jun 11, 2020 • edited Loading

ChALkeR commented Jul 4, 2020

karenetheridge commented Jul 4, 2020

karenetheridge commented Jul 5, 2020

ssilverman commented Jul 5, 2020

Julian commented Aug 31, 2020

ChALkeR commented Aug 31, 2020 • edited Loading

ChALkeR commented Aug 31, 2020 • edited Loading

jdesrosiers commented Aug 31, 2020

handrews commented Aug 31, 2020

karenetheridge commented Nov 10, 2020 • edited Loading

Julian commented Nov 11, 2020

Choose a reason for hiding this comment

karenetheridge Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karenetheridge Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdesrosiers left a comment

Choose a reason for hiding this comment

handrews commented Nov 23, 2020

karenetheridge commented Nov 25, 2020

karenetheridge commented Nov 25, 2020

Julian commented Nov 29, 2020

ssilverman commented Nov 30, 2020

karenetheridge commented Nov 30, 2020

ssilverman commented Jun 11, 2020 •

edited

Loading

ssilverman commented Jun 11, 2020 •

edited

Loading

ssilverman commented Jun 11, 2020 •

edited

Loading

ChALkeR commented Aug 31, 2020 •

edited

Loading

ChALkeR commented Aug 31, 2020 •

edited

Loading

karenetheridge commented Nov 10, 2020 •

edited

Loading

karenetheridge Nov 18, 2020 •

edited

Loading

karenetheridge Nov 23, 2020 •

edited

Loading