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

Remove support for unrecognized keywords #1512

Merged
merged 2 commits into from
Jun 21, 2024

Conversation

gregsdennis
Copy link
Member

@gregsdennis gregsdennis commented May 23, 2024

We discussed this a lot, but never actually made the changes.

Resolves #1340
Resolves #1365

@gregsdennis gregsdennis added this to the stable-release milestone May 23, 2024
@gregsdennis gregsdennis self-assigned this May 24, 2024
@gregsdennis gregsdennis requested a review from a team May 24, 2024 01:08
Copy link
Member

@Relequestual Relequestual left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment. Currently contradictory as far as I can tell.

Otherwise, looks good.

Comment on lines 68 to +70
JSON Schema can be extended either by defining additional vocabularies, or less
formally by defining additional keywords outside of any vocabulary. Unrecognized
individual keywords simply have their values collected as annotations, while the
behavior with respect to an unrecognized vocabulary can be controlled when
declaring which vocabularies are in use.
individual keywords are not supported.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the purpose of this paragraph is to describe extension mechanisms. The x- convention is an extension mechanism, so I think it should be mentioned. I don't think it's necessary to say here that unrecognized keywords aren't allowed. This section is just an introduction. It doesn't have to be exhaustive. I think it's enough to briefly mention the extension mechanisms without needing to say what isn't an extension mechanism.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The x- convention is an extension mechanism...

I'm curious of your definition of an extension mechanism. The x- behavior is defined by the spec, so in my eyes, these keywords are not extensions. This is clarified by #1518.

(This paragraph is going to change in #1510 anyway, so I'm not too concerned about getting it 100% right here.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we agree that the vocabulary system is an extension mechanism. It allows you to use custom keywords in your schemas. But, we decided that the vocabulary system was too cumbersome for users who just wanted to use simple annotation custom keywords, so we introduced x- as an alternative for users to define simple custom keywords without the overhead of the vocabulary system. Both are extension mechanisms for using custom keywords. One is complicated and powerful and the other is simple and highly constrained, but they're serving the same purpose.

x- is so constrained that I can see how you can view it as one thing defined by the spec, but I think there's more to it. x-foo and x-bar have the same annotation collection and validation behavior, but they aren't the same keyword. They have different semantics given to them by the schema author, not by the spec. I see the spec defined behavior of x- as constraints that x- extension keywords must adhere to, but it's not a complete definition. It take the same author to define the semantics for it to be a complete keyword.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Are you happy, then, to have x- keywords clarified as being "recognized" (using that word) in #1518?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would work, but I just posted in the other tread an alternative that I think would be better. It avoids the problem altogether.

Comment on lines 437 to 438
Implementations MUST refuse to process schemas which contain keywords they do
not recognize, or that they recognize but do not support.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording here feels contradictory although I get that it isn't and why it isn't. I get that x- keywords are considered "recognized", but I'm not sure we're making that clear. Maybe we need to be more clear what is considered "recognized" or maybe we need to say it different way that's harder to misunderstand.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Relequestual had this same concern in Slack. It's addressed in #1518.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced the rewording in #1518 really addresses this concern. Not enough anyway. I think that rewording here is probably a better approach. Something like,

Implementations MUST refuse to evaluate schemas which contain keywords they do not know how to process.

I think that's clear, covers all the bases, and avoids confusion about whether x- counts as a recognized keyword. Implementations know how to process x- keywords, so it's clear this statement isn't referring to those keywords.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That also addresses implementations which choose to support custom keywords outside of vocabs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with one tweak.

Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think that paragraph in the overview section should reference x- as an extension mechanism, but I'll let you decide if you want to make that change. Otherwise, this looks good to me.

@gregsdennis
Copy link
Member Author

gregsdennis commented Jun 19, 2024

I'm going to leave it out of the extensions section.

x-foo and x-bar have the same annotation collection and validation behavior, but they aren't the same keyword. They have different semantics given to them by the schema author, not by the spec.

You're not wrong about this. However, those extra semantics only apply outside of the context of evaluating the schema, regardless of how or for what purpose the evaluation occurs.

The specification is saying that x- keywords are annotations only, which means the only semantics they carry or behavior they exhibit during evaluation of the schema is collection of their values as annotations. Any other semantics that a schema author or application infers must be carried out outside of the context of evaluation, where the spec cannot define requirements.

Copy link
Member

@jviotti jviotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my first time being involved in specs, so take my approval with a grain of salt, but at least all the proposed changes make perfect sense to me and it reads well. I think an implementer (or at least I) would definitely get it.

@gregsdennis gregsdennis merged commit 25d0109 into main Jun 21, 2024
3 checks passed
@gregsdennis gregsdennis deleted the gregsdennis/unknown-keywords branch June 21, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Improved handling of unknown properties Update spec to disallow unknown keywords
4 participants