No other keywords allowed beside "$ref"

[handrews]

The spec for $ref, whether in its own JSON Reference spec
or as part of the JSON Schema Core spec, states that keywords
adjacent to "$ref" MUST be ignored. Therefore these tests were
invalid, even though the intent was clear to most readers.

The correct way to combine "$ref"s is with "allOf".

This addresses #113.

[handrews]

Oops, one of those was actually testing that the adjacent keyword was organizing! That's what I get for not reading descriptions. Will update momentarily.

[awwright]

Just looking at a glance, the way it is now is so wrong it makes me wonder if it's that way intentionally.

[handrews]

@awwright yeah I was puzzled. I even filed #198 about whether we should be testing to catch this problem (if it is a problem) of definitions being allowed.

Now that we have the assertion vs annotation concept, we could change the $ref description to only forbid adjacent assertion keywords, but allow adjacent annotations? Or... um.. .something. definitions really isn't an annotation either, it's a re-use container. So maybe it's a special case for $ref? I have no idea what the history was here. @Julian do you know?

[inca]

Fwiw we have a use case where we use $id adjacent to $ref as an alias, to dereference one definition from the other.

Example:

{
    "Foo": {
        "$id": "#Foo",
        "$ref": "#GenericObject"
    },
    "GenericObject": {
        "$id": "#GenericObject",
        ...
    }
}

While this works with our validator (ajv) I wonder if this is future-proof — or is it still invalid usage of $ref?

TIA

[epoberezkin]

@handrews Arguably, definitions is not a keyword. Even though it's reserved in meta-schema, it is just "a standardized location" in the JSON instance that exists and can be addressed unrelated to the position of $ref. There is no reason why these schemas should be invalid.

I think we should really fix the spec to allow at least definitions and annotations together with $ref or better make $ref a normal validation keyword, rather than "fix" this test. As we've agreed that $ref is delegation rather than inclusion I see absolutely no reason to require ignoring anything in the same object.

@inca it is future proof. Even if at some point ignoring definitions or $id alongside $ref will become the default behaviour (which I doubt), there will be an option to not ignore them. I also often use annotation keywords with $ref.

[handrews]

As we've agreed that $ref is delegation rather than inclusion

Actually as I've worked out more of how the different keywords need to be treated, I see pros and cons to both. We will discuss it as part of the next draft, not here.

As the spec currently stands, definitions is as much a keyword as anything else, so these tests are incorrect for their versions. If we change that behavior (which I'm open to), then we will change the test cases for that version.

[handrews]

@inca based on the current and prior specifications, that behavior is incorrect. $ref processing always happens first, although that is arguably less clear in recent specifications than it was when JSON Reference was a separate draft and therefore obviously needed to be implemented as a lazily-evaluated preprocessing step.

Again, this is a PR for the tests for the spec as written, and is not the place to debate changes to the spec. This topic should be addressed during draft-08 as it fits with the planned theme of that release (re-usability).

[handrews]

@inca Having looked at this again (I'd forgotten exactly what this PR was doing already), this change will not impact your use of ajv in any way, nor will it cause ajv to start failing the test.

The problem here is that a conforming implementation is allowed by the current spec to ignore the definitions, which means that the test will fail on some conforming implementations. This is not acceptable, and is unfair to people who implemented the spec to the letter, even if we don't like the current spec.

[Julian]

So yeah this one's definitely me, and was definitely "intentional" in that I've had people ask about it in the past and responded that my reading of the spec at the time was that this was fine and allowed :)!

I'm happy to be overruled if others feel that isn't the case.

But essentially my reasoning was that there's a difference between JSON Schema saying that $ref should make implementations ignore $ref for validation and saying that properties that are clearly present there are not addressable -- i.e., if you reference something with a pointer, it's clearly there, and you can index into it, implementations do not have to pop all the items out of a thing rather than simply only applying one validator.

Again though, happy to be convinced (or just defer) that the spec in fact doesn't allow this.

[Julian]

IIRC allOf isn't in draft 3.

[handrews]

... and that's another good reason to close this. I forgot that draft-03 had extends or something.

[Julian]

Here's the last time this came up, with in fact both you @handrews and @awwright participating :)

I'm guessing that there's a slight nuance here in that you're I guess talking specifically about how since definitions is one of the "validators" that that means you need to remove it, but I think my initial intuition is the same -- you remove it for "validation" (which definition in fact does not do), but the thing is there, if you reference it somehow, it's fine, even under the old specs.

But yeah, if there's consensus, happy to defer.

[handrews]

@Julian given the history I think we should actually stick with what's there. Particularly given the work in draft-07 that starts classifying keywords into assertions and/or annotations, I was already intending to pursue that further in draft-08. And definitions is one that specifically does not fit either classification (really, it belongs with $ref as it pretty much exists to be the recommended target of intra-file $ref-ing). So I'm going to close this and we'll follow up with clarifications in draft-08.

[Julian]

Sounds reasonable to me!