Made sure all $ref objects contain only a $ref property

[iainbeeston]

The IETF JSON Reference spec, section 3 (syntax) says the following:

Any members other than "$ref" in a JSON Reference object SHALL be
ignored.

We have two groups of tests that have a $ref at the root level of the
schema, referring to a definition that is also at the root
level. According to the spec, a $ref object should not have any sibling
properties.

To make those JSON references valid, I've refactored those test cases to
use a $ref object as a json schema property definition instead, which
is valid.

[Julian]

I believe this has come up before -- I'm not sure I agree with that interpretation of the spec. I might have to read it again more carefully though. But my initial reaction back then was "the spec is referring to interpretation of properties in the context of itself -- i.e. 'don't interpret any of the other properties'. Just referencing them with a JSON ref is fine, they're not in fact not there, so you should be able to reference them."

[iainbeeston]

Here's the specification:

https://tools.ietf.org/html/draft-pbryan-zyp-json-ref-03

I'm basing my interpretation partly on that, and partly on something written in the json-schema google group way back in 2013 which also suggests other properties should be ignored:

https://groups.google.com/forum/#!topic/json-schema/86GSgHSQ0VI

[iainbeeston]

Also, every example in the test suite (except for those two groups) use $ref objects containing only a $ref property.

That's the basis of my opinion anyway. Whichever is correct, it would be good to define tests to ensure it. I've actually added tests in #128 which test to make sure that properties other than $ref in a $ref object are ignored (that was one of the test cases in the research paper mentioned)

[epoberezkin]

That's not exactly refactoring. Refactoring would be to put $ref inside allOf. In any case, why not simply add clarification to the standard such as for example "validation keywords are ignored, but the properties can be used in $refs"? All current validators should support these tests.

[iainbeeston]

@epoberezkin Yes, technically you're right, I changed the meaning of those schemas so it's not refactoring. My first instinct was to use allOf as well, but I believe that using properties makes a better test, as it is less likely to fail for an unrelated reason (ie. properties is easier to implement correctly than allOf).

The spec in question here is the JSON Reference spec, not the JSON Schema Validation spec. I believe JSON Schema itself has no concept of JSON References? Adding that clarification mean that JSON Schema enforces a non-standard implementation of the JSON Reference spec.

[iainbeeston]

@Julian Can I confirm whether extra properties on reference objects are ignored or not?

As I understand it, the spec says that the schema referenced should completely replace the reference object (overwriting any extra properties). Alternatively, we could merge the properties from the referenced schema into the reference object (in which case, what happens if there are "collisions" where the same property is defined on both - which should take precedence?)

If you can tell me which way it should be I'll update this pull request to specify that behaviour.

[Julian]

@iainbeeston I still haven't had a chance to review the spec here, apologies for the delay.

[awwright]

I would venture to guess the current behavior is correct. I don't see any reason you can't reference a value just because it's inside an object with a "$ref" key.

But if we decided it wasn't, I think it would make sense to remove the tests in question, or flip the "valid" value instead.

[Julian]

@iainbeeston the google groups link you posted is of someone expecting validation to combine -- that's exactly the case the specification says doesn't work, but it's not just referencing another property via a JSON Reference.

Basically this case only comes up when a $ref refers to a subproperty of its own object, and yeah, I still think that my interpretation there is the one that is intended -- that you can do that, just don't expect those other properties to affect validation.

(So I'd close this and leave as-is personally)

[pipobscure]

So does this mean that in the case of

{
  "$ref":"somewhere-else",
  "type": "integer"
}

the type keyword should be ignored?

[Julian]

@pipobscure correct

[handrews]

@pipobscure there has been some discussion recently (scattered in various places including private email/IRC) about whether the way we handle $ref still makes sense given that it is no longer a separate specification, but for now that is correct. Might result in a spec issue soon, one email thread is on hold due to people who weirdly don't want to talk about this over the holidays ;-)

[handrews]

@Julian any reason this was re-opened? It looks done to me with PR #142

[Julian]

It was only accidentally closed while I removed the develop branch, but yeah I agree that it probably should be intentionally closed :), doing that now, thanks!

[handrews]

Thanks, @Julian . It looks like a few others were similarly re-opened, btw.

[Julian]

I think all those still need review, but lemme know if you see one that should be closed

…

On Jan 5, 2017 15:59, "Henry Andrews" ***@***.***> wrote: Thanks, @Julian <https://github.com/Julian> . It looks like a few others were similarly re-opened, btw. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#129 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUIXvga4L_S7VSH1LwVlieZHg-NANR_ks5rPVmYgaJpZM4KLG0B> .