Skip to content

Wrong "nested refs" test, points to ignored member #113

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.

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
TerjeBr opened this issue Apr 2, 2016 · 17 comments
Closed

Wrong "nested refs" test, points to ignored member #113

TerjeBr opened this issue Apr 2, 2016 · 17 comments
Assignees
Labels
bug A test is wrong, or tooling is broken or buggy.

Comments

@TerjeBr
Copy link

TerjeBr commented Apr 2, 2016

The file tests/draft4/ref.json contains this schema at line 122:

        "schema": {
            "definitions": {
                "a": {"type": "integer"},
                "b": {"$ref": "#/definitions/a"},
                "c": {"$ref": "#/definitions/b"}
            },
            "$ref": "#/definitions/c"
        },

This example is wrong, because according to the JSON Reference RFC

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

This means that the member "definitions" SHALL be ignored (since it is a member beside the $ref), and so the $ref points to somewhere that should be ignored.

@seagreen
Copy link

seagreen commented Jul 6, 2016

Could that be talking about ignoring members as validators, but not as referenceable data?

@TerjeBr
Copy link
Author

TerjeBr commented Jul 6, 2016

It could, but that would make things complicated, instead of easy to understand. I think that clause was put there in the first place to make the standard more simple and easy.

@seagreen
Copy link

One situation this does make tricky is one like: https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/develop/remotes/subSchemas.json

An example use of it as a reference is "http://localhost:1234/subSchemas.json#/integer". If members other than "$ref" can be referenced that link should stop working if a key named "$ref" is also inserted into that object alongside "integer".

I basically agree with you though, the above is just thinking out loud:)

@Julian what should the next step be here? Is it ready for a PR or should we open an issue in the JSON Schema spec repo and get a final answer? One advantage of the latter approach would be it would let them know that perhaps even more explicit language in the next spec would help.

@Julian
Copy link
Member

Julian commented Jul 15, 2016

That spec is for JSON refs FWIW, not JSON Schema, the latter references and uses the former, but my interpretation of that line is certainly "shall ignore for the purposes of this specification and the resolution process it describes". It's similar to the JSON Schema spec saying that other validators not defined by the spec are ignored. It doesn't say to me "other members are not allowed in there".

@TerjeBr how did you encounter this issue if I may? Do you have an implementation of JSON Ref resolution that special-cases resolving to the same object, and then disallows accessing any members inside it other than $ref?

@TerjeBr
Copy link
Author

TerjeBr commented Jul 15, 2016

I encountered this issue when trying to contribute to https://github.com/justinrainbow/json-schema

I thought a valid way of resolving all the refs in a schema was to just replace the whole object that contained the $ref with the object it references.

@Julian
Copy link
Member

Julian commented Jul 15, 2016

That does in fact work if I'm understanding, at least to resolve it at one specific moment in time, but if you replaced this object with the object it references, it'd have those members in there -- to implement the behavior under your interpretation you'd have to specifically then notice you resolved to your same JSON ref object, and then specifically remove properties from it.

@TerjeBr
Copy link
Author

TerjeBr commented Jul 15, 2016

No, I just have to have an in memory tree structure with all the objects, it does not matter which json schema each object it from, and then just go through the tree and recursively replace any objects with a $ref in them. I might end up with a circular graph instead of a tree, but at least it resolves to something.

@d-frey
Copy link

d-frey commented Oct 3, 2016

While implementing JSON Schema for my library, I ran into the exact same problem as TerjeBr. The strategy was to replace all occurrences of a JSON Reference by a raw pointer to some other node (which is generally useful in my library for other purposes). The problem is that the whole sub-object that represents the JSON Reference is replaced, so anything inside of that object and next to $ref is than gone.

If the JSON Reference can not (as a whole object) be replaced by the referenced node, you end up with additional problems: What if the referenced node and the JSON Reference node both contain the same key? Or if the referenced node is not an object where you can merge keys? It also leads to inefficient lookups as you can not merge the additional keys to the referenced node - they should only be visible if accessed through the JSON Reference, not when accessed directly.

To sum it up: I think that tests/draft4/ref.json:122 is invalid.

@handrews
Copy link
Contributor

I'm pretty sure that schema should be

        "schema": {
            "definitions": {
                "a": {"type": "integer"},
                "b": {"$ref": "#/definitions/a"},
                "c": {"$ref": "#/definitions/b"}
            },
            "allOf": [{"$ref": "#/definitions/c"}]
        },

We had to fix the hyper-schema meta-schema for that recently (see json-schema-org/json-schema-spec@464abb8 )

@epoberezkin
Copy link
Member

And none of these problems would exist if $ref was not considered as replacement of anything by anything and instead considered as a normal validation keyword...

This test shows that that was the intention, by the way.

I am not sure I even understand why the requirement to have $ref as the only keyword is needed. Seems not more sensible than to have not as the only keyword, for example. /rant

Here I rest my case.

@epoberezkin
Copy link
Member

@TerjeBr

I thought a valid way of resolving all the refs in a schema was to just replace the whole object that contained the $ref with the object it references.

It is not for other two reasons as well: recursion and base URI determined by the source schema, not by the target.

@handrews
Copy link
Contributor

@epoberezkin I've read through the issue where we discussed it extensively multiple times and I still really don't understand your "normal validation keyword" terminology.

Allowing other keywords beside "$ref" requires coming up with merge rules, which either means that we're back to $merge (with all the associated problems) or we are coming up with our own merge rules (possibly as simple as "completely overwrite instead of merging" but that's still a rule that we have to define).

How do you think that having additional keywords should work? All of the options just go back into the same swamp as the "re-use with additionalProperties" mess.

@handrews
Copy link
Contributor

base URI determined by the source schema, not by the target.

If you copied it in (which you can't always due to recursion as @epoberezkin noted) then you need to figure out the base URI of the referred schema and make it explicit when you copy by setting "id" in the copied result.

If you want to see that problem addressed in detail, check out XML's XInclude. Not because we should be like XML, but because they go into a lot of detail about the problem.

@epoberezkin
Copy link
Member

@handrews You don't have to merge anything. You simply define the validation result of $ref keyword as the result of validation of the current data instance by the schema referred to by the uri in $ref keyword value. It's delegation, not inclusion.

@epoberezkin
Copy link
Member

@handrews we've also discussed that recursion can be handled if you do inclusion every time you do validation, lasyly sort of. This plus adding ID to handle base URI make inclusion a more complex model to define ref than delegation.

@handrews
Copy link
Contributor

@epoberezkin we're hijacking @TerjeBr's issue :-) I'll follow up with you on email, I think I managed to follow your distinction finally, need to consider it more.

@handrews
Copy link
Contributor

Closing this in favor of json-schema-org/json-schema-spec#505 to resolve in the spec. Will revisit the test cases as needed.

Given the established history of definitions and $ref in the test suite for many years now, we are not going to change them right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A test is wrong, or tooling is broken or buggy.
Projects
None yet
Development

No branches or pull requests

6 participants