-
-
Notifications
You must be signed in to change notification settings - Fork 314
Clarify the nature of schema placeholder keywords #783
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
Conversation
jsonschema-core.xml
Outdated
@@ -1789,7 +1789,9 @@ | |||
<section title="References to Possible Non-Schemas"> | |||
<t> | |||
Subschema objects (or booleans) are recognized by their use with known | |||
applicator keywords. These keywords may be the standard applicators | |||
applicator keywords, or with placeholder keywords such as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for my late feedback. To be honest I am not sure if this would clarify things (for me) considerably compared to before. I weren't able to draw the line in the opposite direction (as a reader of section Schema Re-Use With "$defs" right to this section and the essence of its meaning). But that might be caused by my personal situation of "not being in the specs" in such depth, so isn't meant to disqualify your answer. Though, the newly introduced term placeholder keyword would require its own definition.
I would accept this PR as closing my issue but would also accept the issue being closed without any further action if majority doesn't see any necessity for clarification.
Thank you.
Just a little note: I can't comment on things in a timely manner ATM so don't make any progress with this depend on me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @about-code! That's helpful feedback, I'll tweak it a bit more (we don't want people to have to already be experts when reading the standard- it's not a tutorial but it should be accessible). If you have time to look at the update, great, but thanks for letting me know that you might not be available.
Essentially, your question is, should The answer is, maybe, but you shouldn't rely on such, because the behaviour is now undefined. Some implementations may choose to support referencing using arbitrary keys, but it's not required. There are lots of "super edge" cases that we would need to consider to define the behaviour, so rather than forbid it, leaving it undefined leaves implementations to document specific support for users who don't need their schemas to be portable. "behaviour is undefined" essentially translates to, "Probably not a good idea, as it's complex, but we won't stop you doing it if you really want, and understand it will likely mean your implementation isn't portable". I hope that gives some additional clarification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this reads better.
In terms of the change itself. Fine by me!
@Relequestual @about-code OK I actually did some serious-ish reworking. I had come up with the idea of @Relequestual I updated both I think this is a good improvement overall, so thanks for bringing it to our attention, @about-code ! |
For example, mine does this. It allows |
@gregsdennis also, presumably if you have a structure like: {
"$id": "https://this.example.com/schemas/foo",
"$ref": "#/a/b/c",
"a": {
"b": {
"$id": "https://that.example.com/schemas/bar",
"c": {
"$ref": "somewhere#/$defs/whatever"
}
}
}
} where Or if you're implementation does look at that @gregsdennis @Relequestual , the point here isn't about implementations choosing to "support" arbitrary keys. The point is that it is absolutely not possible to guarantee the correct behavior of nested arbitrary unknown keys. There is no way to do it. You have to decide whether you look for This is a strange corner case, but it is critically important not to over-promise spec behavior. You cannot do this correctly, and shouldn't claim to do so. However, you can get away with (for lack of a better term) schema duck typing for a single level of unknown keyword, because it's not possible to insert an unexpected |
Yeah, hopefully people don't do that because it's really weird. |
LOL have you met people? 😆 Are you OK with these changes as I've updated things? |
<t> | ||
A fourth category of keywords simply reserve a location to hold re-usable | ||
components or data of interest to schema authors that is not suitable | ||
for re-use. These keywords do not affect validation or annotation results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I'd suggest to use the term placeholder keyword at least once, maybe even emphasized. For example:
These placeholder keywords do not affect validation[...]
Then when the term placeholder keyword is used further down below such as in section References to Possible Non-Schemas it is terminologically clear that placeholder keywords is the phrase used to refer to keywords falling into the category of reserved locations.
Overall the recent additions make things much more consistent and comprehensible, I think. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I'd actually meant to get rid of "placeholder" and use the "location" terminology. I'll sort that out and fix the out-of-order wording, too- thanks!
jsonschema-core.xml
Outdated
@@ -1948,8 +1969,8 @@ | |||
|
|||
<section title='Comments With "$comment"'> | |||
<t> | |||
This keyword is reserved for comments from schema authors to readers or | |||
maintainers of the schema. | |||
This keyword is reserved a location for comments from schema authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is reserved a location for
I guess it should be
is a reserved location
Acknowledge that "$defs" and placeholders like it also indicate that their values are schemas, which is relevant to whether they can be detected as a proper "$ref" target or place where "$id" may appear. This ensures that similar extension keywords are understood to have the same concerns as extension applicator keywords.
Introduce the category of reserved location keywords, which are basically just $defs and $comment. This should make it more clear that you can create keywords of your own like this, and fits them more nicely into the overall keyword taxonomy. Part of this change refers to the typical core+validation meta-schema as the "default meta-schema", which is a change made in another PR.
OK, @about-code I made both of those places consistent by using "location-reserving keywords" and "reserves a location", respectively. Since those changes are pretty trivial, and @Relequestual 's only specific change request was a comma that I fixed yesterday, I'm going to go ahead and merge. @Relequestual seemed to approve the changes aside from that grammar thing. And Greg approved yesterday. |
I fixed the comma, I think that was the only actual change requested.
Fixes #778 (paging @about-code).
Acknowledge that "$defs" and placeholders like it also indicate
that their values are schemas, which is relevant to whether
they can be detected as a proper "$ref" target or place where
"$id" may appear.
This ensures that similar extension keywords are understood
to have the same concerns as extension applicator keywords.
I did not add anything about
$defs
being recommended, becausenow that it is covered in the same way as the applicator vocab,
it is clear that you can add more extension keywords like it.
And I think it is inherently clear that keywords in Core are recommended.
[See later comments from me about a few further changes]