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 3 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.

gregsdennis and others added 2 commits February 2, 2025 13:29
Co-authored-by: Jason Desrosiers <[email protected]>
Co-authored-by: Jason Desrosiers <[email protected]>
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.

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