-
-
Notifications
You must be signed in to change notification settings - Fork 308
Allow contains to apply to objects as well as arrays #1092
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
Allow contains to apply to objects as well as arrays #1092
Conversation
jsonschema-validation.xml
Outdated
<xref target="json-schema">"contains"</xref> keyword. The first way is if | ||
the annotation result is an array and the length of that array is less than | ||
or equal to the "maxContains" value. The second way is if the annotation | ||
result is a boolean "true" and the instance array length is less than or | ||
equal to the "maxContains" value. | ||
result is a boolean "true" and the instance length is less than or equal to |
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.
Can "length" be applied to objects? Maybe we keep "instance array length" and add "or property count"?
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 think "length" makes sense. It may be an object, but in this context, it's being treated like a map, which does have a concept of length. But I you think it's questionable, others will be confused, so we should be more explicit.
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.
"number of properties" is explicit and doesn't depend on a language/framework's particular data model.
jsonschema-validation.xml
Outdated
<xref target="json-schema">"contains"</xref> keyword. The first way is if | ||
the annotation result is an array and the length of that array is greater | ||
than or equal to the "minContains" value. The second way is if the | ||
annotation result is a boolean "true" and the instance array length is | ||
greater than or equal to the "minContains" value. | ||
annotation result is a boolean "true" and the instance length is greater |
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.
Same here.
I think this keyword should be moved out of this section entirely -- as it is presently located in "Keywords for Applying Subschemas to Arrays". It can be moved to a third section after "Keywords for Applying Subschemas to Objects", called "Keywords for Applying Subschemas to Arrays or Objects", or even just "Other Keywords for Applying Subschemas". |
What happens if someone wants to specify that an array to contain an item, OR that an object to contain a property value? I would like to suggest these should be different keywords, e.g. |
If they only wanted one or the other, they would need to also specify |
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.
Seems reasonable to me.
Want to see all PRs adding to the changelog to avoid the mess of making a changelog at the end 😅
- Added to changelog
Good thinking about adding to the change log! It sounds like this PR might get closed as preference seems to be shifting toward using new keywords. When it's decided, I will make sure whichever one we go with is included in the changelog. |
While it sounds like this PR is a bit in-limbo, if that changes and it's re-approved, it would be good to wait to merge until we decide whether to branch for the next major draft. |
It appears we have general consensus on this direction, so I'm removing "draft" status to open this up for review. However, @handrews, I will not merge until we decide what's happening with the bug-fix draft. This should not be included in the bug-fix draft. |
I had the create a new section for the change log because it doesn't exist yet. I used |
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.
Excellent work on reaching this solution!
I just want to include a quick summary of the discussion around adding this keyword. There was no opposition to adding {
"not": {
"patternProperties": {
"": { "not": { ... schema ... } }
}
}
} Note: I used The only point of contention was whether In the end, the only people expressing strong preferences were for overloading |
I noticed there were no changes to meta/applicator.json. Shall we do those in a subsequent PR? |
I don't think there are any meta-schema changes needed for this. Am I missing something? |
Resolves #1077
The two ways to go with this were to have
contains
/minContains
/maxContains
apply to objects, or to introduce three new keywords that apply to objects. I went with the first option because that was the only one anyone expressed a preference for and no one objected.