$id updates

[gregsdennis]

What kind of change does this PR introduce?

Cleans up some language around $id.

Issue & Discussion References

• Related to Clean-up of stable features and removal of unstable features #1444
• Related to URI/IRI "Normalization" and Compatibility #1349

Summary

Just some clean-up and clarification that we've previously identified.

Schema document roots SHOULD have an $id (maybe relax this? ...) - from #1444 (comment)

I decided to leave this in for now. We can discuss and change separately if needed.

Does this PR introduce a breaking change?

No

[jviotti]

Nice! Most of the changes are simple one-character edits, and the rest read very well to me

[notEthan]

Stating here that this behavior is undefined seems to me to contradict the above "an embedded schema resource and its subschemas can be identified by JSON Pointer fragments relative to either its own canonical IRI, or relative to any containing resource's IRI".

This also leaves [^8] with no reference to it.

And, minor, but two spaces after a period isn't consistent with the rest of the document.

[gregsdennis]

This also leaves [^8] with no reference to it.

I'll add that back in.

[gregsdennis]

It was discussed that we need to make pointers that cross boundaries an undefined behavior rather than explicitly allowing implementations to support it. Of course, implementations can still support it if they want, but the spec should push schema authors a bit more forcefully toward using the proper $id instead.

I think the critical point here is "an embedded schema resource and its subschemas". It's arguable that an embedded schema resource isn't strictly a subschema since it can be refactored out without any change in behavior. If we need to clarify that, I'm happy to.

[notEthan]

Right, I get making this undefined is the main point of this PR (I edited my comment a bit to clarify that, had it slightly wrong initially ... I'm not sure this should be undefined but if that is the way it is to be, okay).

To me "an embedded schema resource ... can be identified by JSON Pointer fragments ... relative to any containing resource's IRI" reads as defined behavior and not consistent with making it undefined some paragraphs later, and needs some change. I see your point on ambiguity whether "subschema" includes embedded schemas but I don't see language present that actually calls embedded schemas subschemas.

[gregsdennis]

I read that paragraph as informative, not prescriptive. It's setting up the reasoning behind the requirement.

Embedded schemas can be referenced this way, but because reasons it's a bad idea to actually do it, so we're gonna say probably don't. We're also letting implementations decide whether they want to support it, no pressure either way.

[jdesrosiers]

Thanks for tackling these issues!

[gregsdennis]

@notEthan @jdesrosiers I've almost completely reworked the $id section. There were some redundancies in it that didn't sit right with me.

I moved the syntactic definition of the keyword up to the top sentence next to its purpose, and I reworked how it talks about defining a resource, needing to be resolved against the current base IRI, and then providing a base IRI for child resources.

This relationship is necessarily recursive (the $id value needs a base IRI but then also serves as a base IRI), so it's quite difficult to state simply.

[jdesrosiers]

Sorry, for the delay in reviewing.

[jdesrosiers]

I find this paragraph confusing. The word "intervening" doesn't make sense in this context. I also found it awkward that it starts by saying it will behave correctly, then says it must not be relied upon. I think it would be more clear if it was the other way around.

I think this was originally about unknown keywords. Since unknown keywords aren't allowed anymore, it becomes about custom vocabulary defined keywords. Therefore, I think this is saying that you can't use $id in a schema in a custom keyword. I don't think that's what we want. $id should work in any known schema, even in custom keywords.

[gregsdennis]

This paragraph was just moved up. It was previously at line 1404 (which you can see just below).

I also found it awkward that it starts by saying it will behave correctly, then says it must not be relied upon.

The text is "will behave correctly under implementations that attempt to use any reference target as a schema" which is correct.

I think this was originally about unknown keywords.

It's not just about unknown keywords. It's also about keywords that can define structure but not subschemas, like default or const (or custom keywords with structured data). The point of this paragraph is to say that $id in any place where a schema is not expected is to treated as data, not an identifier.

[jdesrosiers]

Yes, I noticed that it was moved. I'm not arguing about its correctness. I can't even say anything about its correctness at this point because I don't understand what it's trying to say.

It's not just about unknown keywords. It's also about keywords that can define structure but not subschemas, like default or const (or custom keywords with structured data).

I don't see how that paragraph can be understood to be about that. It appears this only applies to custom keywords with identical semantics to $defs.

The point of this paragraph is to say that $id in any place where a schema is not expected is to treated as data, not an identifier.

That's well stated and clear. I suggest replacing the paragraph with some variation of that sentence.

[gregsdennis]

Yeah, reading it again, I don't think it's necessary. It looks like it's just trying to explain that you can't have an $id between the keyword and a potential subschema that isn't a subschema because of the structure of the keyword. I'm not sure that needs explaining, so I'll just remove it.

I'm not sure if we need to keep the "requirement" about "MUST NOT be relied upon". It's definitely a requirement on schema authors, but it's a weird one at that.

The next paragraph actually covers exactly what I was saying:

A reference target under a keyword for which the value is known not to be a schema results in undefined behavior in order to avoid burdening implementations with the need to detect such targets.

I still think this can be cleaned up, though.

[jdesrosiers]

Latest changes look great. I think, https://github.com/json-schema-org/json-schema-spec/pull/1537/files#r1819699325 is the only thing left.

[gregsdennis]

@jdesrosiers this should be done now.

[jdesrosiers]

Thanks for working with me through the details. This is a huge improvement and I'm excited to see this merged. 🎉