Consolidate and clarify language around duplicate IRIs #1272
Conversation
In a document root schema, "$id": "#" is a no-op, and in an embedded schema resource root, it results in the embeddded resource having the same URI as its context resource. Given that the specificaiton says implementations SHOULD raise an error for duplicate IRIs, there is no reason to allow this form.
jsonschema-core.xml
Outdated
| valid <xref target="RFC3987">IRI-reference</xref> with a non-empty non-fragment | ||
| component. This IRI-reference SHOULD be normalized, and MUST resolve to an |
There was a problem hiding this comment.
I don't think this change is necessary. The two valid options are presented as an absolute-IRI or an IRI with an empty fragment. Both absolute-IRI and an IRI are by definition non-relative, so we don't need to say "non-empty". It also says it "MUST resolve to", which also indicates a non-relative result.
There was a problem hiding this comment.
@jdesrosiers: This doesn't sound right. $id can be a relative IRI. If that relative IRI is empty (apart from fragment), it resolves to the same absolute IRI as the parent schema (or the same as its retrieval IRI if it's at the root, but that part isn't a problem). Then a subschema has the same id as its parent. Forbidding the empty non-fragment portion disallows this id collision.
@handrews: The wording is awkward, "with a non-empty non-fragment component". Maybe a new sentence would be better:
If present, the value for this keyword MUST be a string, and MUST represent a valid IRI-reference. [unchanged]
This IRI-reference MUST NOT be empty or consist only of an empty fragment.
[or]
This IRI-reference MUST NOT have an empty non-fragment component.This IRI-reference SHOULD be normalized ...
There was a problem hiding this comment.
@jdesrosiers I'm not sure I understand. "$id": "foo/bar" is valid, both before and after this change. It is a path-only relative IRI-reference that, resolved against a base IRI (say, https://example.com/schemas/base) produces an absolute-IRI (https://example.com/schemas/foo/bar).
@notEthan Thanks, I think it like "This IRI-reference MUST NOT be empty or consist only of an empty fragment." I checked RFC 3986 and it uses the term "empty" to describe relative references in §4.4 Same-Document Reference.
There was a problem hiding this comment.
I wasn't saying that $id can't be relative. I was talking about the result of resolving the $id against the base IRI. I thought this change was trying to protect against case where you don't have a baseURI and therefore the IRI can't be fully resolved. I was pointing out that the current wording already protects against that case.
If that relative IRI is empty (apart from fragment), it resolves to the same absolute IRI as the parent schema
Oh, ok, now I understand where this change is coming from. But, I still don't think it's necessary because we talk about not allowing duplicate identifiers more generally elsewhere. IMO, it's already covered and doesn't need to be repeated here.
There was a problem hiding this comment.
I thought this change was trying to protect against case where you don't have a baseURI and therefore the IRI can't be fully resolved. I was pointing out that the current wording already protects against that case.
Ah, I see. Yeah, that is already handled and not improved (or made worse) by this change.
we talk about not allowing duplicate identifiers more generally elsewhere. IMO, it's already covered and doesn't need to be repeated here.
Cool- yeah, that's the main question (for one of the commits in this PR- the rest of it remains valid as it clarifies that "already covered" part that, as currently written, kinda contradicts itself in a confusing way).
I'd like to leave this open for probably another week and see if we get any more opinions from other @json-schema-org/spec-team folks (or anyone else, I just can't tag the whole universe). I don't feel strongly on this point, so if no one advocates for the change I'll strip it out and we can go with just the clean-up of the duplicate IRI stuff.
There was a problem hiding this comment.
One option might be to note that, while syntactically allowed, "" and "#" effectively produce duplicate IRIs which are considered errors per §whatever:
Note that while the empty IRI and the empty fragment-only IRI are legal values, they produce duplicate IRIs when used in an embedded resource, which is considered an error per 8.2.3.
That's probably enough to discourage the values without requiring people to detect and block them. I'm currently leaning towards this option.
There was a problem hiding this comment.
it might be a little odd to include a MUST NOT with the intent of heading off a route to duplicate IRIs, when the language on duplicate IRIs itself isn't a MUST NOT ("implementations SHOULD raise an error condition").
But do highlight that they are errors in the section where that is likely to be the most relevant.
handrews
changed the title
|
Based on feedback so far I have removed the forbidding of certain |
gregsdennis
added
clarification
Fixes issue #1271 and addresses one point mentioned in #1059 (comment) (but please do not drag the rest of that issue into this PR, thanks!)
The only actual changed requirement is the forbidding of
"$id": "#", and"$id": ""which are confusing and are either pointless (in a document root where they resolve to the retrieval IRI, exactly as if$idwas not present) or problematic (in an embedded resource root, where they produce duplicate IRIs for the embedded and containing resource).This also consolidates the two different places where duplicate IRIs were addressed. Since the more general of the two paragraphs stated that this SHOULD be an error, I kept the SHOULD rather than the MAY that addressed only a subset of the cases. The SHOULD technically covered the subset as well anyway.
I have emphasized that "undefined" here means that the result, whatever it is, will not be interoperable (most likely, implementations will just return the last-seen schema under that IRI, but who knows).
Note that we could continue to allow
"$id": "#"and"$id": ""and just assume that the SHOULD regarding duplicate IRIs is enough to discourage that. If that is the consensus then I can remove the commit that handles that part of the change.