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

keywords only exhibit the behaviors they're defined with #1577

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gregsdennis
Copy link
Member

What kind of change does this PR introduce?

clarification

Issue & Discussion References

Summary

The previous language could imply that all keywords needed to have an assertion result, e.g. annotations would always produce a "true" assertion result.

For min/maxContains, I added explicit text that they do not produce an assertion result, emphasizing that the assertion comes from contains and that these keywords are informative.

This change clarifies that keywords only exhibit the behaviors they're defined with.

Does this PR introduce a breaking change?

No.

@gregsdennis gregsdennis requested a review from a team January 19, 2025 07:24
@gregsdennis gregsdennis added this to the stable-release milestone Jan 19, 2025
@gregsdennis gregsdennis self-assigned this Jan 19, 2025
@karenetheridge
Copy link
Member

karenetheridge commented Jan 19, 2025

So, what is the result of:

allOf: [
  { maxContains: 1 },
  { maxContains: 1 },
]

and

anyOf: [
  { maxContains: 1 },
  { maxContains: 1 },
]

and

oneOf: [
  { maxContains: 1 },
  { maxContains: 1 },
]

?

@gregsdennis
Copy link
Member Author

Good point. The keywords produce no assertions, but the subschemas still need to.

Granted, this is true with 2020-12, too. The validation spec doesn't actually define assertion results for any of the annotations, yet it's still considered a pass because there are no constraints.

I think this could be stated explicitly.

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've always thought of all keywords having an assertion. Annotation-only keywords just always assert true. $defs is another example of a keyword always returns true although it's not an annotation.

I think the way this is worded is great because it allows for implementations to ignore non-assertions or just make them true. They can implement it however makes most sense for their implementation.

Comment on lines 587 to 588
any boolean logic operation to the assertion results of subschemas, but MUST NOT
introduce new assertion conditions of their own.
Copy link
Member

Choose a reason for hiding this comment

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

but MUST NOT introduce new assertion conditions of their own.

I'm not following this. What is this trying to say? What are we protecting against be forbidding it?

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 text was already there.

It's just saying that applicator assertions can only be combinatorial on their subschemas. For instance anyOf only ORs the results of its subschemas without asserting anything else.

I think this is a simplicity guard, kind of a JSON Schema version of SRP. I suppose this means something like requiredProperties would be in violation of the spec since it's combining subschema results and asserting that all of the properties are there.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jdesrosiers what do you think of relaxing this to a SHOULD, or maybe even an informative note suggesting simplicity over complexity?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's unnecessary for that to be a constraint. Your requiredProperties example is great. Although I'm not a fan of that keyword, I don't think there should be any spec reason why it shouldn't be allowed.

or maybe even an informative note suggesting simplicity over complexity?

Yes. I think that's a great idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a footnote and changes this to SHOULD NOT.

fit. Applicators apply subschemas to parts of the instance and combine their
results.
JSON Schema keywords may exhibit one or more behaviors. This specification
defines three such behaviors:
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to say that all keyword behaviors in the specification are in these three categories, but the spec defines other behaviors for e.g. $id and $anchor/$dynamicAnchor. And $schema ... exists.

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 is an interesting point. I was thinking that these keywords don't exhibit a behavior so much as they are merely informative, like maxContains would be to contains.

Copy link
Member

Choose a reason for hiding this comment

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

This is a great point! My implementation doesn't even treat those as keywords. They're meta-data used to process the schema, but they're not directly part of the validation/annotation process. So, I wouldn't consider them similar to maxContains that is involved in validation.

I'm not sure what I'd suggest, but I think something needs to be called that these aren't normal 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.

I've added a footnote. Let me know.

I tend to agree with @jdesrosiers on this. The $* keywords are more of directives than anything else, and I don't think we should be listing "directive" as a behavior. It would encourage people to try to make more.

@gregsdennis gregsdennis force-pushed the gregsdennis/validation-not-required branch from 285a554 to 1a7a34a Compare April 12, 2025 01:19
Comment on lines +425 to +426
- Annotations attach information that applications may use in any way they see
fit.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Annotations attach information that applications may use in any way they see
fit.
- Annotations attach information to instance locations that applications may use in any way they see
fit.

Comment on lines +432 to +434
behaviors. However, it is recommended that extensions avoid defining additional
directive keywords as they could interfere with schema processing and produce
unexpected or undesirable results.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what you mean by "interfering with schema processing".

The way I see it, people should avoid creating these kinds of keywords because it's unlikely that an implementation will be able to support them using typical extension mechanisms. At least my implementation wouldn't.

Comment on lines +442 to +444
Keywords which are not defined to exhibit a particular behavior MUST NOT affect
that aspect of evaluation. For example, a keyword which does not act as an
assertion MUST NOT affect the validation result.
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 sure what this means. It sounds like it's saying that if you say it doesn't have an assertion behavior, then it shouldn't have an assertion behavior. Is there some nuance that I'm missing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
4 participants