Skip to content
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

Test for valid use of empty fragment in "$id" #300

Open
2 tasks
handrews opened this issue Nov 11, 2019 · 4 comments · Fixed by #341
Open
2 tasks

Test for valid use of empty fragment in "$id" #300

handrews opened this issue Nov 11, 2019 · 4 comments · Fixed by #341
Assignees
Labels
missing test A request to add a test to the suite that is currently not covered elsewhere.

Comments

@handrews
Copy link
Contributor

  • $id with trailing empty fragment equivalent to $id without a fragment
  • "$id": "#" is a no-op
@handrews handrews added this to the 2019-09 milestone Nov 11, 2019
@Julian Julian added the missing test A request to add a test to the suite that is currently not covered elsewhere. label Nov 29, 2019
@karenetheridge
Copy link
Member

I don't think either of these test cases ended up being covered by the final version of #341 (but the first one might be covered elsewhere).

@Julian Julian removed this from the 2019-09 milestone Aug 7, 2022
@handrews
Copy link
Contributor Author

Both of these cases are implemented.

However, I don't remember why "$id": "#" is a no-op is a test case. Here is the case as implemented:

            {     
                "description": "Identifier name with base URI change in subschema",
                "data": {
                    "$id": "http://localhost:1234/root",
                    "$ref": "http://localhost:1234/nested.json#/$defs/B",
                    "$defs": {
                        "A": {
                            "$id": "nested.json",
                            "$defs": {
                                "B": {
                                    "$id": "#",
                                    "type": "integer"
                                }         
                            }             
                        }             
                    }             
                },            
                "valid": true
            }         

The problem here is that this identifies both A and B as http://localhost:1234/nested.json, which makes the reference ambiguous. I think what I meant was using "$id": "#" in the document root, in which case the schema URI is the retrieval URI, which is exactly what happens if you don't have $id at all. So I think that's the "no-op" I meant. But I don't think the test suite can set retrieval URIs so this weird case is probably un-testable.

@Julian would you be OK with me making a PR to delete that case across all relevant drafts? If there's a way to set an external base URI I can replace it with the document root one, but otherwise this is more-or-less equivalent to having two identical $anchor values, which the spec notes has undefined behavior (see the last paragraph of the linked section).

@Julian
Copy link
Member

Julian commented Aug 12, 2022

If the test is wrong yes definitely a PR to remove would be appreciated!

We have a way to communicate retrieval URIs, it's basically by putting the remote at a corresponding path inside remotes/ -- so putting foo.json in there is saying the retrieval URI is http://localhost:1234/foo.json even when it doesn't contain a $id (in fact some tests I have for a PR shortly are about noticing we don't test cases where that retrieval URI is different from the one in $id)

But yeah I think it sounds like that should work for this scenario?

@handrews
Copy link
Contributor Author

@Julian sounds good, I'll make a PR for that.

The test as it stands runs afoul of §9.1.2 "Loading a referenced schema" (last paragraph):

A schema MAY (and likely will) have multiple IRIs, but there is no way for an IRI to identify more than one schema. When multiple schemas try to identify as the same IRI, validators SHOULD raise an error condition.

(I've filed json-schema-org/json-schema-spec#1271 to consolidate this and §8.1.2 which I linked in the comments, since one of them has a MAY around raising an error and the other a SHOULD).

So it's not wrong to pass that test as written, but it would be equally correct to fail (resolving http://localhost:1234/nested.json#/$defs/B to be equivalent to http://localhost:1234/root#/$defs/A/$defs/B/$defs/B instead of http://localhost:1234/root#/$defs/A/$defs/B. And it would actually be more correct to error out upon schema load.

Unless the test suite can test for error conditions, in which case we could make that an optional test for raising an error, it should just be removed.

@handrews handrews self-assigned this Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
missing test A request to add a test to the suite that is currently not covered elsewhere.
Projects
None yet
3 participants