tests for $ref using base URI learned from $id

[ryangalamb]

Oddly enough, the "local" version worked fine with my implementation, while the "remote" version broke it.

This is my first time adding a remote case. I tried to amend the right places in the scripts, but I may have missed some spots.

I put the same tests in draft7 and 2019-09.

[ryangalamb]

Oops! I just remembered that "$defs" is only in 2019-09. For the remote file, should I make that test 2019-09 only?

[karenetheridge]

For cross-checking, the errors returned by the new ref.json tests for my implementation are:

{
  "errors" : [
    {
      "absoluteKeywordLocation" : "/referencing.json#/$defs/referenced/enum",
      "error" : "value does not match",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf/0/$ref/allOf/0/$ref/enum"
    },
    {
      "absoluteKeywordLocation" : "/referencing.json#/allOf",
      "error" : "subschema 0 is not valid",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf/0/$ref/allOf"
    },
    {
      "error" : "subschema 0 is not valid",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf"
    }
  ],
  "valid" : false
}

{
  "errors" : [
    {
      "absoluteKeywordLocation" : "/referencing.json#/$defs/referenced/enum",
      "error" : "value does not match",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf/0/$ref/allOf/0/$ref/enum"
    },
    {
      "absoluteKeywordLocation" : "/referencing.json#/allOf",
      "error" : "subschema 0 is not valid",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf/0/$ref/allOf"
    },
    {
      "error" : "subschema 0 is not valid",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf"
    }
  ],
  "valid" : false
}

{
  "valid": true
}

and the new refRemote.json tests:

{
  "errors" : [
    {
      "absoluteKeywordLocation" : "http://localhost:1234/referencing.json#/$defs/referenced/enum",
      "error" : "value does not match",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf/0/$ref/allOf/0/$ref/allOf/0/$ref/enum"
    },
    {
      "absoluteKeywordLocation" : "http://localhost:1234/referencing.json#/allOf",
      "error" : "subschema 0 is not valid",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf/0/$ref/allOf/0/$ref/allOf"
    },
    {
      "absoluteKeywordLocation" : "http://localhost:1234/subSchemas-ids.json#/allOf",
      "error" : "subschema 0 is not valid",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf/0/$ref/allOf"
    },
    {
      "absoluteKeywordLocation" : "http://localhost:1234/#/allOf",
      "error" : "subschema 0 is not valid",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf"
    }
  ],
  "valid" : false
}

{
  "errors" : [
    {
      "absoluteKeywordLocation" : "http://localhost:1234/referencing.json#/$defs/referenced/enum",
      "error" : "value does not match",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf/0/$ref/allOf/0/$ref/allOf/0/$ref/enum"
    },
    {
      "absoluteKeywordLocation" : "http://localhost:1234/referencing.json#/allOf",
      "error" : "subschema 0 is not valid",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf/0/$ref/allOf/0/$ref/allOf"
    },
    {
      "absoluteKeywordLocation" : "http://localhost:1234/subSchemas-ids.json#/allOf",
      "error" : "subschema 0 is not valid",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf/0/$ref/allOf"
    },
    {
      "absoluteKeywordLocation" : "http://localhost:1234/#/allOf",
      "error" : "subschema 0 is not valid",
      "instanceLocation" : "",
      "keywordLocation" : "/allOf"
    }
  ],
  "valid" : false
}

{
  "valid": true
}

[karenetheridge]

The other files in remotes use 'definitions', and the 2019-09 spec says that that location should be kept reserved for use by refs, so I would just rename $defs to definitions there.

[ryangalamb]

I'm happy to move it, but I am curious: Where does the spec require 2019-09 implementations to handle "definitions"?

I see that the keyword is reserved to prevent redefining it with different behavior, but I can't find where its behavior is actually specified in the 2019-09 spec.

[ryangalamb]

That's how I read the section on references to possible non schemas. Seems like a ref to "definitions" (an unknown keyword) would be undefined behavior.

do we want to be that picky?

For the official test suite? Yes :) If the spec classifies it as undefined behavior, I don't think it's right for the acceptance tests to expect specific behavior there. I'm happy to acknowledge that I might be misinterpreting the spec, but with my current understanding, I'm not comfortable using "definitions" in 2019-09 test cases.

At the very least, I can resolve the issue in this PR by creating two files for this case: one with "$defs", and one with "definitions".

[Julian]

@rjmill sorry for the long delay here -- any chance you're willing to resolve the conflicts for this PR so I can have a look for review?

[handrews]

@rjmill

That's how I read the section on references to possible non schemas. Seems like a ref to "definitions" (an unknown keyword) would be undefined behavior.

Yes, that's correct, we should use $defs in the test suite for 2019-09+. While definitions is basically reserved for implementations that want to support it for compatibility reasons, it is not required and I know of at least one 2019-09/2020-12-only implementation that will not find schemas under definitions, but will find them under $defs.

[Julian]

(I believe we already do in all other places -- if that means we need multiple versions of the remote so be it...)

[ryangalamb]

@Julian

sorry for the long delay here -- any chance you're willing to resolve the conflicts for this PR so I can have a look for review?

I'm willing, but free time may be a bit of an issue :sweat_smile: If someone wants to take this PR over, I won't be offended. Otherwise, I'll try to get to it when I have the time.

The merge conflicts don't look too tricky, from what I can see.

Looks like my bin/jsonschema_suite and index.js changes are irrelevant now. The rest seem like normal merge conflicts? I haven't been following changes in this repo, so it's hard to say at a glance.

[gregsdennis]

@rjmill I've run these tests against my implementation and they all look good. If you can resolve the conflicts, I think we're happy to merge.

[ryangalamb]

@gregsdennis I fixed the merge conflicts and pushed again. Let me know if there's anything else I can do.

btw, I am still the same person. I just changed my last name (got married) and changed my gh handle to match.

[Julian]

@Julian do you think it's worth having this test for draft 4?

We write it's "frozen" in the README. I still personally honestly do try to cover it though when I add tests, because it's still reasonably popular to see in the wild from what I see, regardless of what we hope.

So I'd personally do it, yeah, but I understand if/when others are less willing to put in the effort for it.