Add tests for location-independent identifiers #225
Conversation
|
@handrews can I ask you to review this one? |
|
@Julian I'll take a look at it. Just got back from out of town, may take a bit for me to catch up with things. |
| ] | ||
| }, | ||
| { | ||
| "description": "Canonical location-independent identifier", |
There was a problem hiding this comment.
I'd like to avoid the word "canonical" due to its role in the confusing draft-04 canonical vs inline mess.
I'm not entirely sure it applies to this case even in draft-04 terms, but TBH none of the current spec team really know what the point of that was AFAIK.
Is the point of this test to use a URI (with scheme) rather than a URI reference? If so, that seems like the thing to note in the description.
tests/draft4/ref.json
Outdated
| "description": "Location-independent identifier with base URI change in subschema", | ||
| "schema": { | ||
| "id": "http://localhost:1234/root", | ||
| "$ref": "http://localhost:1234/nested.json#foo", |
There was a problem hiding this comment.
Does having id adjacent to $ref work? I'd really rather wrap the $ref in an allOf. I know that within the spec we allow adjacent definitions, and I suppose that argument could be extended to id. But really it's a gray area/bug in the spec and implementations are not consistent about whether it's supported.
tests/draft6/ref.json
Outdated
| { | ||
| "description": "Location-independent identifiers should take precedent over regular fragments", | ||
| "schema": { | ||
| "$ref": "#foo", |
There was a problem hiding this comment.
I really want the $refs to all be in allOfs, I've decided. It's such a confusing and underspecified area, when you then start throwing in odd corner cases like an unrecognized "foo" keyword alongside a $ref I have absolutely no idea what should happen, and even less clue what the majority of implementations will do.
Let's avoid gray areas except when explicitly testing around them.
tests/draft6/ref.json
Outdated
| ] | ||
| }, | ||
| { | ||
| "description": "Location-independent identifiers should take precedent over regular fragments", |
There was a problem hiding this comment.
I'm trying to figure out the failure mode of this test case. Is there reason to think that an implementation might insert a / after the # and convert it into a pointer?
There was a problem hiding this comment.
At first I didn't notice the difference between JSON pointers which require a / and a regular plain fragment which doesn't. Now on second glance this test makes no sense and I'll remove it as it is only confusing and has no purpose.
|
Hello! We are working in python-jsonschema/jsonschema#371 on adding support for Location-independent identifiers on Python's jsonschema implementation, and it would be great to first have some tests in the suite for them. This PR seems to add exactly those tests. I've been reviewing the comments here, and I'm willing to help fixing the remaining items. I've identified the following issues to fix before merging this:
Please, tell if I'm right and if you would like me to contribute those changes. I can push them to my fork for you to merge them here, or we can create a new PR. |
|
Fantastic! (Thanks again). On the last point, yes, please do add them to the new draft folder. I think if you fork the repo and then put your commits on top of this branch that's perfect. |
|
Done! I have pushed the changes to my alvarogzp:location-independent-identifiers-tests branch. |
|
@alvarogzp Thanks for updating it. I'll update this PR this weekend and poke someone with a stick so this is merged. |
|
I think this looks good to my eyes -- merging! Thanks to you both @johandorland / @alvarogzp. (Well, sticking a final commit on top to rename the new draft to |
The section in the RFC isn't super detailed about this, so I took the liberty to come up with some edge cases to resolve #221.