Add tests for unevaluated with dynamic reference #696
Conversation
| "$recursiveAnchor": true, | ||
|
|
||
| "type": "array", | ||
| "prefixItems": [ |
There was a problem hiding this comment.
Good catch. I seems I accidentally have prefixItems registered as a 2019-09 keyword, so the test passed when I tested on my implementation.
jdesrosiers
force-pushed
the
unevaluated-dynamicref
branch
from
f13fb46 to
2834c63
Compare
| "foo": { "type": "string" } | ||
| }, | ||
| "$dynamicRef": "#addons", | ||
| "unevaluatedProperties": false, |
There was a problem hiding this comment.
The order of keywords might affect order of execution in implementations, so should we put unevaluatedProperties first, to cover that as well? (that might be a separate issue, since there are a lot of test cases here that have unevaluatedProperties last in the list)
There was a problem hiding this comment.
We have other tests that ensure key order is not a factor in evaluation.
There was a problem hiding this comment.
Not for $dynamicRef and $ref though. My implementation passed this test case but just by luck 😅
Moving "$ref": "#/$defs/bar" down in "unevaluatedItems with $ref", for example, fails.
It's also really important to test references bc they are not explicitly listed in the spec section: https://json-schema.org/draft/2020-12/json-schema-core#name-unevaluatedproperties
There was a problem hiding this comment.
Not for $dynamicRef and $ref though.
I don't think it's practical to check order of keywords for every combination of keywords. We have tests that ensure order of keywords is irrelevant. It's expected that this is applied across an implementation, not just for those tests.
There was a problem hiding this comment.
It's also really important to test references bc they are not explicitly listed in the spec section: https://json-schema.org/draft/2020-12/json-schema-core#name-unevaluatedproperties
The spec does address references:
"This includes but is not limited to the in-place applicators defined in this document."
References are in-place applicators because they evaluate the instance against the schema they're referencing.
There was a problem hiding this comment.
While I agree that it's impractical to test ordering for every combination of keywords, I'm of the opinion that anything that exposed a real bug in an implementation belongs in the test suite. I'll move unevaluated* to be the first keyword and include a note explaining why.
|
I applied the suggested changes. I also realized that I forgot to add the tests to draft-next, so I added that as well. |
|
CI is failing here (for correct reasons, it's saying you have invalid JSON), AFAICT that's the only thing preventing merging this, it's got an approval, yes? |
Julian
added
the
waiting for author
|
I didn't notice the build failure. Thanks for pointing that out. I was waiting for review on the requested changes and then completely forgot about this when no review happened. |
|
Cool, thanks! Merging. |
This was previously correct for $refs, but not $dynamicRefs, which had no test in the JSON Schema test suite. This behavior is now properly compliant with the 2020 spec (as well as 2019, for $recursiveRef). Refs: json-schema-org/JSON-Schema-Test-Suite#696
|
This introduced duplicate identifiers: "https://example.com/extended-tree" is used as a URI across multiple tests in the same specification version. There are also duplicate identifiers produced with "baseSchema" and "tree" -- "https://example.com/baseSchema" and "https://example.com/tree" end up referring to multiple locations. |
Fixes #691