The meaning of "additionalProperties" has changed #57
Replies: 5 comments 17 replies
-
I have a feeling we discussed this at the time and landed on the conclusion that "validation failed so it doesn't matter". Obviously it does matter if you are testing specific error output. I'm in favour of leaving "as is", although it is strictly different from draft-07 in terms of validation failure output. I know others have been feeling that our use of annotations to determine how things interact is not always ideal and "we can do better" (@karenetheridge). I don't dissagree, but I'm not immeditatly sure of a solution or direction to resolve those feelings. If we want to address "annotations as an interaction vehicle is too clunky", we have 6 months after we put out the patch release to figure something out. It likely won't be high on my priorty list personally, but I'll make time if others feel strongly enough about it. |
Beta Was this translation helpful? Give feedback.
-
We could change the definition to allow It's true that the true/false validation result comes out the same either way, but the difference in error output is undesirable, and I do believe that producing too many errors can be confusing to the user. Implementations should not be producing different errors depending on whether they choose to optimize the implementation of |
Beta Was this translation helpful? Give feedback.
-
Count me in that group. I think we should be defining the behaviors of keywords. Whether or not it's implemented with annotations or not is an implementation detail.
I disagree. The "different failure output" is not just different, it's harmful. It produces strange and confusing results. But, the bigger problem is not that the failure output is different, it's that the spec defines two valid alternatives and they don't produce the same results. That means there is no right answer for what the validation results for
If validation fails, then there are no annotations. Since Here's an example, {
"type": "object",
"properties": {
"foo": { "type": "string" }
},
"unevaluatedProperties": false
} Given the instance Given the instance What this would mean for {
"type": "object",
"properties": {
"foo": { "type": "string" }
},
"additionalProperties": false
} Given the instance So, if this is a valid interpretation, it fixes the bad error messaging, but it's still a degraded experience from the previous non-annotation-based definition. |
Beta Was this translation helpful? Give feedback.
-
I have two solutions to suggest -- one in the shorter term that we can include as an amendment to the current specification, for publication in our draft2020-12 update, and a longer term solution which would necessitate a new schema revision. I'll address the larger change in a separate discussion post as it involves unevaluated* as well (edit: you can find that here). The short term solution for the immediate problem described by the That is, change this paragraph:
to (bringing back some of the same language as from draft7):
...and omit the final paragraph in this section. PS. the same inconsistency exists in |
Beta Was this translation helpful? Give feedback.
-
Related discussion suggesting an altered behaviour #67 |
Beta Was this translation helpful? Give feedback.
-
This is how "additionalProperties" is defined in draft 7:
It's a great definition. It describes clearly and unambiguously the meaning of the "additionalProperties" keyword and its relationship to adjacent keywords.
In 2019-09 this changed: the behavior of "additionalProperties" was defined in terms of the presence and annotation results of "properties" and "patternProperties". The annotation result of "properties" is described thus:
And this annotation result controls the behavior of "additionalProperties":
Based on the above information, one might expect the "additionalProperties" keyword to behave exactly as it did in draft 7. In fact, the spec itself expects this behavior, as it provides the draft 7 approach as an alternative that "produces the same effect".
But there's a problem. The section on annotations is clear that:
And:
Now, consider using the following schema:
to evaluate this instance:
What errors should we expect to appear in the output? Clearly, there must be an error from
/properties/foo
. But what about/additionalProperties
? Draft 7 is unequivocal: the instance does not contain any 'additional properties' that need to be validated by "additionalProperties".But according to the current specification, there can be no annotation from "properties" because its failure implies the failure of its containing schema, and the schema must therefore not produce any annotations. Consequently, the instance property "foo" must also be validated by "additionalProperties", and we end up with two leaf errors in the output: one from
/properties/foo
and one seemingly superfluous error from/additionalProperties
.Now, one 'solution' to this problem might be to allow "properties" to produce its annotation - which in the above case would be
["foo"]
- within the context of processing the current schema, and simply not propagate that annotation into the schema's result. Sounds reasonable, but there's a catch! Doing so would change the behavior of "unevaluatedProperties". Consider this variation on the above example:The expectation here is that we should see two errors in the output - one from
/properties/foo
and one from/unevaluatedProperties
, because the instance property "foo" was never successfully evaluated by "properties". But if we were to allow the "properties" annotation to linger while processing the current schema, "unevaluatedProperties" would not have to validate "foo" and would not produce any error. Or, to put it another way, if we adjust the rules for annotations to fix "additionalProperties", then we break "unevaluatedProperties".In my view, the current specification of "additionalProperties" is wrong. According to its annotation-based definition, "additionalProperties" behaves like a local "unevaluatedProperties". I believe that the behavior of "additionalProperties" should not be defined in terms of the annotation results of any adjacent keywords, but should rather - as in draft 7 - be defined directly in terms of the set of instance names matched by "properties" and "patternProperties".
Beta Was this translation helpful? Give feedback.
All reactions