Skip to content

test $recursiveRef + $recursiveAnchor #391

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

test $recursiveRef + $recursiveAnchor #391

Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c9a788c
test $recursiveRef + $recursiveAnchor
karenetheridge Jun 4, 2020
18bac7a
squash: use a unique $id to prevent namespace collisions across tests
karenetheridge Jun 23, 2020
33dca87
squash: test $recursiveRef when $recursiveAnchor is false
karenetheridge Jul 4, 2020
13e3f91
squash: test $recursiveRef when $recursiveAnchor is absent
karenetheridge Jul 4, 2020
2b4df7b
squash: fix duplicate $id
karenetheridge Jul 5, 2020
18a5ffb
squash: test $recursive$ref with no $recursiveAnchor in the initial t…
karenetheridge Jul 5, 2020
7492ff6
squash: one more test, with no recursiveAnchor in the outer resource …
karenetheridge Nov 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
273 changes: 273 additions & 0 deletions tests/draft2019-09/recursiveRef.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,273 @@
[
{
"description": "$recursiveRef without $recursiveAnchor works like $ref",
"schema": {
"properties": {
"foo": { "$recursiveRef": "#" }
},
"additionalProperties": false
},
"tests": [
{
"description": "match",
"data": {"foo": false},
"valid": true
},
{
"description": "recursive match",
"data": { "foo": { "foo": false } },
"valid": true
},
{
"description": "mismatch",
"data": { "bar": false },
"valid": false
},
{
"description": "recursive mismatch",
"data": { "foo": { "bar": false } },
"valid": false
}
]
},
{
"description": "$recursiveRef without using nesting",
"schema": {
"$id": "http://localhost:4242/recursiveRef2/schema.json",
"$defs": {
"myobject": {
"$id": "myobject.json",
"$recursiveAnchor": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

"anyOf": [
{ "type": "string" },
{
"type": "object",
"additionalProperties": { "$recursiveRef": "#" }
}
]
}
},
"anyOf": [
{ "type": "integer" },
{ "$ref": "#/$defs/myobject" }
]
},
"tests": [
{
"description": "integer matches at the outer level",
"data": 1,
"valid": true
},
{
"description": "single level match",
"data": { "foo": "hi" },
"valid": true
},
{
"description": "integer does not match as a property value",
"data": { "foo": 1 },
"valid": false
},
{
"description": "two levels, properties match with inner definition",
"data": { "foo": { "bar": "hi" } },
"valid": true
},
{
"description": "two levels, no match",
"data": { "foo": { "bar": 1 } },
"valid": false
}
]
},
{
"description": "$recursiveRef with nesting",
"schema": {
"$id": "http://localhost:4242/recursiveRef3/schema.json",
"$recursiveAnchor": true,
"$defs": {
"myobject": {
"$id": "myobject.json",
"$recursiveAnchor": true,
"anyOf": [
{ "type": "string" },
{
"type": "object",
"additionalProperties": { "$recursiveRef": "#" }
}
]
}
},
"anyOf": [
{ "type": "integer" },
{ "$ref": "#/$defs/myobject" }
]
},
"tests": [
{
"description": "integer matches at the outer level",
"data": 1,
"valid": true
},
{
"description": "single level match",
"data": { "foo": "hi" },
"valid": true
},
{
"description": "integer now matches as a property value",
"data": { "foo": 1 },
"valid": true
},
{
"description": "two levels, properties match with inner definition",
"data": { "foo": { "bar": "hi" } },
"valid": true
},
{
"description": "two levels, properties match with $recursiveRef",
"data": { "foo": { "bar": 1 } },
"valid": true
}
]
},
{
"description": "$recursiveRef with $recursiveAnchor: false works like $ref",
"schema": {
"$id": "http://localhost:4242/recursiveRef4/schema.json",
"$recursiveAnchor": false,
"$defs": {
"myobject": {
"$id": "myobject.json",
"$recursiveAnchor": false,
"anyOf": [
{ "type": "string" },
{
"type": "object",
"additionalProperties": { "$recursiveRef": "#" }
}
]
}
},
"anyOf": [
{ "type": "integer" },
{ "$ref": "#/$defs/myobject" }
]
},
"tests": [
{
"description": "integer matches at the outer level",
"data": 1,
"valid": true
},
{
"description": "single level match",
"data": { "foo": "hi" },
"valid": true
},
{
"description": "integer does not match as a property value",
"data": { "foo": 1 },
"valid": false
},
{
"description": "two levels, properties match with inner definition",
"data": { "foo": { "bar": "hi" } },
"valid": true
},
{
"description": "two levels, integer does not match as a property value",
"data": { "foo": { "bar": 1 } },
"valid": false
}
]
},
{
"description": "$recursiveRef with no $recursiveAnchor works like $ref",
"schema": {
"$id": "http://localhost:4242/recursiveRef5/schema.json",
"$defs": {
"myobject": {
"$id": "myobject.json",
"$recursiveAnchor": false,
"anyOf": [
{ "type": "string" },
{
"type": "object",
"additionalProperties": { "$recursiveRef": "#" }
}
]
}
},
"anyOf": [
{ "type": "integer" },
{ "$ref": "#/$defs/myobject" }
]
},
"tests": [
{
"description": "integer matches at the outer level",
"data": 1,
"valid": true
},
{
"description": "single level match",
"data": { "foo": "hi" },
"valid": true
},
{
"description": "integer does not match as a property value",
"data": { "foo": 1 },
"valid": false
},
{
"description": "two levels, properties match with inner definition",
"data": { "foo": { "bar": "hi" } },
"valid": true
},
{
"description": "two levels, integer does not match as a property value",
"data": { "foo": { "bar": 1 } },
"valid": false
}
]
},
{
"description": "$recursiveRef with no $recursiveAnchor in the initial target schema resource",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@karenetheridge karenetheridge Nov 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

"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",
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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)

"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 only uses inner schema",
"data": { "foo": { "bar": 1 } },
"valid": true
},
{
"description": "leaf node does not match: recursion only uses inner schema",
"data": { "foo": { "bar": true } },
"valid": false
}
]
}
]