-
-
Notifications
You must be signed in to change notification settings - Fork 277
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
Add requireAllExcept keyword #1144
Conversation
Fair enough! |
I find this keyword somewhat difficult to comprehend. It's behavior is defined in terms of another keyword, which I think we should avoid. It becomes non-intuitive when mixing "allOf", because this keyword does not "see into" sub-schemas. What would be some cases where this keyword is superior to "requiredProperties"? |
I don't feel this is a problem. Many keywords have dependencies. That's one of the reason for the annotations system.
By my understanding of your proposal #846, schema authors could use both Generally we've moved away from keywords doing multiple things, splitting them up, mainly because it was confusing. |
@Relequestual By "... and there's no way around it", I mean: It's fine if a keyword performs multiple things as an author convenience. But we have to be able to break it apart. e.g. Likewise, This keyword, by contrast, has interactions with other keywords. What happens if I use: {
"requireAllExcept": ["foo"],
"allOf": [ { "foo": false } ],
"patternProperties": { "f": true },
"additionalProperties": false
} What's the behavior here? There probably is one—but I have to think about it. And I can't really take an educated guess. I can't even come up with a good example, maybe if I replaced It's true that we have other keywords like this (additionalProperties), but they tend to consume a disproportionate amount of descriptive effort we have to do.
This feels like exactly the kind of keyword that does multiple things you're talking about. |
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.
In reviewing this further, I think some clarification is required.
I've left a suggested change.
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 overall object to this because it "reads" the value of other keywords, which is something we try to avoid, see "Keyword independence".
However if this keyword absolutely must be added, we would need to list this in said "Keyword independence" section. (Or rather, since this keyword does nothing if in a schema all by itself, it may be more consistent to say the "properties" keyword reads this one.)
@awwright what about |
@gregsdennis Those keywords are specifically listed as exceptions... I mean, yes, we never really went into detail as to why they're exceptions... but as an exercise, write an alternative where the only information available to the keyword is its own value. The best I can do is something like {
"contains": {
"needle": "foo",
"min": 1,
"max": 4
}
} While this nicely emphasizes how "minContains" doesn't do anything by itself—that it's technically an argument to "contains"—this is obviously clunky to write, which is why I think an "argument keyword" is acceptable as an authoring convenience. In contrast, the single-keyword alternative to "requireAllExcept" combines the functionality of "required" and "properties" into a single keyword, entirely replacing the need for "required"; all the while "properties" can maintain its usual function for listing optional properties that "requireAllExcept" keyword is doing. This idea that some keywords can be decomposed into more complicated but equal schemas should be familiar, e.g. ※ A keyword "doesn't anything by itself" if the single-keyword schema |
I don't follow this. The new keyword is an array of property names that are optional. The presence of the keyword implies that all properties in It's not combining |
I'm not sure what you don't follow, so let me elaborate a little bit. "requireAllExcept" is a "keyword" but it's not a validation keyword per se, it's an argument keyword: It's modifying the behavior of another (validation) keyword. We try to avoid argument keywords unless there's no better way to do it. So, is there a better way to accomplish the goal here? Yes. We can accomplish the same behavior by reworking the keyword that's doing the heavy lifting. Take for example: {
properties: {
"name": {type: "string"},
"comment": {type: "string"}
},
requireAllExcept: [
"comment"
]
} How do we adapt "requireAllExcept" so that it operates by itself? It can pull in the schema for each of the properties: {
properties: {
"name": {type: "string"},
},
requireAllExcept: {
"comment": {type: "string"}
}
} There, now "requireAllExcept" can be used by itself. And we're not redundantly listing the "comment" key name! But wait, isn't "requireAllExcept" just listing optional properties here—identical to how "properties" works right now? Let's swap the names around. {
requireAll: {
"name": {type: "string"},
},
properties: {
"comment": {type: "string"}
}
} Is there possibly a better name for "requireAll" that indicates it accepts a key=>schema map? {
requiredProperties: {
"name": {type: "string"},
},
properties: {
"comment": {type: "string"}
}
} ...so "requiredProperties" is the same thing as "requireAllExcept", except not broken. |
With this approach you also need to update |
@gregsdennis Yes— this is sort of implied when I say "requiredProperties" would 'behave the same way as using "required" and "properties"'—but for consistency you are correct, it would be a good idea to update the definitions of those keywords. |
Also, to bring this back to my original point, "requireAllExcept" also needs to update some other paragraphs, specifically the section(s) that discuss keyword independence, so it can be added as one of the exceptions. |
Reading the more recent comments, I think we need to re-evaluate our approach here. I know we're not all happy with the annotations system as defined, however I don't think that means we intend to throw it out. I feel that we can still use it, better defined, and any "keyword interactions" should rely on the premise annotation collection. I'm VERY strongly opposed to adding another way to list properties which effects Adding new constraint base keywords without effecting existing keywords feels preferable to me. |
Can you detail what this would look like?
We're dealing with an authoring convenience, i.e. how to combine common patterns of keywords into a single keyword. There's going to be inherent complexity in that, regardless of the solution. There's also some amount of subjectivity. We're going to have to balance what new people would expect, with what keeps the cognitive requirements low on very large schemas. (Also, these things might be the same.) I think all authoring conveniences will introduce some "surprise", but we can minimize that; the real purpose of an authoring convenience is it lets you build more complicated schemas on the same amount of brain power (it is probably more difficult to work with an if/then statement than to think "x property means y becomes required" — and so we have "dependentRequired"). We may want to put out a survey that tries to measure these two properties (expected/surprising behavior, and scalable/non-scalable for authors).
I don't think there's a way to do this. You can think of "requireAllExcept" as a keyword that first reads the value of "properties", then returns a validation result; or you can think of it as an argument to "properties" (which currently reads no arguments); these two interpretations are logically indistinguishable. However I'm inclined to say we should think of it only an argument to "properties", because the single-keyword schema (An alternative name for these "argument keywords" could be "interacts with"... because even though "minContains" is an argument keyword to "contains", it still makes sense to talk about "minContains" as the source of validation errors. It just won't do so when it's the only keyword in the schema.) |
Yes, but I'll have to get back to you. I'm overflowing with half done work right now 😭 |
6e4ab89
to
5d35b16
Compare
Agreed. This was an oversight. I updated the PR. |
This is not the way |
jsonschema-validation.xml
Outdated
<t> | ||
An object instance is valid against this keyword if every property name | ||
declared in "properties" is the name of a property in the instance, with | ||
the exception of the property names that appear in this keyword's array. |
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.
We also need clarification on what the exception means. This could be read as "the properties in the array must not be present."
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 see your point. I'll try to reword.
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 pushed some alternate wording.
This isn't correct. |
@jdesrosiers See my full explanation at #1144 (comment); by "per se," I mean "by itself." Since JSON Schema is declarative, there's multiple equivalent ways validation could be performed in actuality, (1) the way I'm thinking about it where you perform the validation at the moment you're iterating through the keywords in "properties" and making sure that each property is either in the instance, or listed in "requireAllExcept"; or (2) your implementation where you validate it at the time the "requireAllExcept" keyword is encountered. ((3) You can also write a validator that performs all the validations at the same time, and you can do so deterministically, proving that "where" the validation occurs truly doesn't matter.) What makes a keyword an "argument keyword" is that a schema consisting only of e.g. |
Aside: By "at the same time", I mean you can compile a validator for a good number of schemas down to a finite state machine; since common states are factored out, it doesn't make sense to consider a failure to be "from" one keyword or another (the best you can do is say this failure occurs because of the existence of a keyword in the schema, and would have been valid if not for its existence). |
I've seen implementations that evaluate all object-based keywords together, all array- together, etc. I'm not a fan of this approach, and I chose to implement each keyword separately in its own function, because it made it easier to selectively enable/disable individual keywords depending on which version of the specification was active -- but what matters is the outcome. As long as the validation result and emitted errors/annotations are correct, do whatever makes sense for your mental model and your choice of language/architecture. This is, FWIW, why I think we have such divisive arguments about evaluation behaviour sometimes -- the implementation choices and mental models vary quite widely, and this informs our beliefs about how new features ought to work. |
jsonschema-validation.xml
Outdated
An object instance is valid against this keyword if every property name | ||
declared in "properties" within the same schema object is the name of |
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.
We should consider the advantages/disadvantages of having this keyword use annotations from properties
, which would allow it to use property definitions from other schemas via composition.
I could see this pattern being quite useful:
$defs:
model.user:
type: object
properties:
id:
type: string
name:
type: string
type:
enum: [foo, bar]
comment:
type: [string, 'null']
create_operation:
$ref: '#/$defs/model.user'
requireAllExcept: [id, comment]
get_operation:
$ref: '#/$defs/model.user'
requireAllExcept: []
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.
[briefly de-lurks]
@karenetheridge it's worth considering that these might be two separate use cases. "require all of these properties right here" is a straightforward thing that would become hard if this always used annotations from applicators in subschemas. And trying to make it optional within the keyword would introduce more complex syntax.
[re-lurks]
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.
All good points. It's worth discussing.
b862023
to
b9c2e9f
Compare
I see what you're trying to say, but I don't think I think it would be fair to call something an argument keyword if it has no meaning if not in the presence of another keyword (Examples: Let me take a step back and acknowledge that I share your concerns about adding a new keyword that breaks keyword independence. In fact, I hate it. I'd rather be finding ways to remove or redefine all such keywords, but weighing the pros and cons, I think
|
The
|
66d4638
to
e8c3583
Compare
Co-authored-by: Karen Etheridge <[email protected]>
Co-authored-by: Ben Hutton <[email protected]>
It was pointed out that the previous wording could be mistaken to mean that properties were forbidden rather than optional.
e8c3583
to
37eacba
Compare
The problem I have with how (from @awwright's examples above) {
"properties": {
"name": {"type": "string"},
"comment": {"type": "string"}
},
"requireAllExcept": [
"comment"
]
} That means that for A possible solution to this would be to have Also with this approach |
@gregsdennis You're right, this should probably be defined based on annotations (although I still think that approach needs to be revisited) and the current annotation behavior of From a classification point of view, this keyword should work exactly like |
This PR needs to be rewritten as a proposal document. See #1450 for an example. Closing this. |
Resolves #1112
Adds the
requireAllExcept
keyword.