The meaning of "additionalProperties" has changed

[marksparkza]

This is how "additionalProperties" is defined in draft 7:

Validation with "additionalProperties" applies only to the child
values of instance names that do not match any names in "properties",
and do not match any regular expression in "patternProperties".

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:

The annotation result of this keyword is the set of instance property names matched by this keyword.

And this annotation result controls the behavior of "additionalProperties":

Validation with "additionalProperties" applies only to the child values of instance names that do not appear in the annotation results of either "properties" or "patternProperties".

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:

annotations are not retained for failing schemas.

And:

Schema objects that produce a false assertion result MUST NOT produce any annotation results, whether from their own keywords or from keywords in subschemas.

Now, consider using the following schema:

{
    "properties": {"foo": false},
    "additionalProperties": false
}

to evaluate this instance:

{"foo": "bar"}

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:

{
    "properties": {"foo": false},
    "unevaluatedProperties": false
}

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

[Relequestual]

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.

[gregsdennis]

I have a feeling we discussed this at the time and landed on the conclusion that "validation failed so it doesn't matter".

That's exactly where we landed.

[karenetheridge]

If we want to address "annotations as an interaction vehicle is too clunky"...

I'm really starting to come around to this point of view. We could potentially replace it with a separate, internal-only mechanism for conveying information between subschemas. We have precedence for doing this by way of $dynamicRef (tracking the dynamic scopes necessary for its function requires one to add new information to internal state) and it would also clean up the semantics of unevaluatedProperties/unevaluatedItems.

[gregsdennis]

In regard to using annotations as the driving mechanism, I'm inclined to agree that the spec requiring this is wrong.

The spec should define input and output; how an implementation gets there is up to them. But since the spec is requiring that annotations drive behavior, it also prescribes an implementations architecture.

I could be happy with either:

• removing the language that says annotations drive behavior, or
• updating that language to say that this approach is for the benefit of the spec as a reference and not prescriptive of implementations.

We did something similar with JSON Path.

[karenetheridge]

We could change the definition to allow additionalProperties to consider not just local annotations, but also local errors, from the relevant sibling keywords. This is exactly what I would like to do with unevaluatedProperties (although I haven't written up a discussion for that yet). That would allow the behaviour of additionalProperties to be identical no matter how it is implemented - whether it looks at the presence of those keywords in the schema directly, or uses the results of annotations or errors (because every failing keyword must produce an error, and the *Properties keywords all produce annotations on success).

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 additionalProperties, and the specification should not be suggesting that one way is equivalent to the other when indeed they are not.

[jdesrosiers]

We could change the definition to allow additionalProperties to consider not just local annotations, but also local errors, from the relevant sibling keywords. This is exactly what I would like to do with unevaluatedProperties (although I haven't written up a discussion for that yet).

I'm looking forward to seeing more about that proposal. I'm not quite sure how that would work. (Edit: Nevermind, I do think that would work)

[jdesrosiers]

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"

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'm in favour of leaving "as is", although it is strictly different from draft-07 in terms of validation failure output.

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 addtionalProperties should be. We need to pick one.

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.

unevaluatedProperties has the same problem with bad error messaging. In my implementation, I take an approach that I'm not sure is correct, but it solves the bad messaging problem. If it's a valid interpretation, it could be applied to the annotation-based definition of addtionalProperties as well. Here's what I do ...

If validation fails, then there are no annotations. Since unevaluatedProperties is defined based on annotations, the result is unknowable. Therefore, it passes (or is ignored) just like any other keyword that doesn't apply to the instance (Ex. minimum of an object).

Here's an example,

{
  "type": "object",
  "properties": {
    "foo": { "type": "string" }
  },
  "unevaluatedProperties": false
}

Given the instance { "foo": "bar" }, The annotations passed to unevalutedProperties might look like this: { evaluatedProperties: ["foo"] }.

Given the instance { "foo": 1 }. The annotations passed to unevaluatedProperties would be null because annotations don't make sense in the context of a schema that failed validation. Because there are no annotations, unevaluatedProperties doesn't apply and it effectively passes.

What this would mean for additionalProperties is that error messages would make sense, but validation errors elsewhere in the schema will mask errors that would otherwise be reported by additionalProperties.

{
  "type": "object",
  "properties": {
    "foo": { "type": "string" }
  },
  "additionalProperties": false
}

Given the instance { "foo": 1, "bar": 2 }, the only error we would get is that "foo" needs to be a string. The additional "bar" property will not be reported because the schema evaluation failed so we don't have annotations to know if there are additional properties or not... even tho it is possible to know. The additionalProperties error can only be reported if everything else about the instance passes validation.

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.

[jdesrosiers]

Wow, apparently I completely failed to explain this. I really don't know how else to say it.

this statement doesn't make sense..

If validation fails, then there are no annotations. Since unevaluatedProperties is defined based on annotations, the result is unknowable. Therefore, it passes (or is ignored) just like any other keyword that doesn't apply to the instance (Ex. minimum of an object).

Consider the data instance { "foo": 1 } and the schema { "unevaluatedProperties": false }. There are no annotations here either,

The highlighted part is not correct. This schema has passed so there are annotations, but the annotations are empty. It's the difference between annotations = {} and annotations = null. It's the difference between an empty box and having no box at all. If there's a box, I can make an assertion. If I don't have a box, I can't. If the schema fails, I don't get an empty box of annotations, I don't get a box at all. If the schema passes, I always get a box of annotations, although that box could be empty.

that violates the principle of keyword independence, where each keyword applies its rules without regard to the pass/fail status of other keywords -- which also lets us evaluate keywords in basically any order we wish. We are already bending the principle of independence for the unevaluated keywords though, so perhaps this is not a good argument.

We break keyword independence for many keywords, not just unevaluated. then and else are dependent on the pass/fail status of if. This is absolutely not a new type of behavior.

some errors being generated should not ever be there from the start.

I agree. This isn't a matter of being too verbose.

[karenetheridge]

This schema has passed so there are annotations, but the annotations are empty.

Okay, that's fair.

It's the difference between annotations = {} and annotations = null. It's the difference between an empty box and having no box at all. If there's a box, I can make an assertion. If I don't have a box, I can't.

I can't agree with this. Where are you reading in the spec that there is a difference of behaviour between properties producing an annotation consisting of an empty array, vs no annotation being produced at all? unevaluatedProperties only acts when there is a non-empty annotation from the relevant keywords. There being a an annotation from properties indicating that the keyword exists in the schema, but matched no properties in the data instance and therefore producing an annotation of [], is no different from no properties keyword existing at all. In either case, unevaluatedProperties will act on whatever properties exist in the data that aren't annotated, and that's the same set regardless of whether an annotation of [] is present or not.

This is all in the weeds though and not relevant to discussing how behaviour should be changed in future drafts.

[jdesrosiers]

Where are you reading in the spec that there is a difference of behaviour between properties producing an annotation consisting of an empty array, vs no annotation being produced at all?

I wasn't describing what the spec says, I was describing my approach. Like I said, "I take an approach that I'm not sure is correct, but it solves the bad messaging problem". There's nothing in the spec that says this is the way it should work. My approach is somewhere between a valid implementation choice and a harmless but beneficial deviation from the spec. I'm not exactly sure where it lands, but I don't care because the worst case is harmless and beneficial.

This is all in the weeds though and not relevant to discussing how behaviour should be changed in future drafts.

How exactly is this not relevant? This is a possible solution to the problem that would require only a very small update, wouldn't break any existing validation behavior, and has an existing implementation proving that it works.

[karenetheridge]

Okay, I guess I'm still not understanding then. I'll re-read.

[karenetheridge]

@jdesrosiers I've re-read your approach and I think I understand it now. But isn't this a gap in your reasoning.. wouldn't the instance { "foo": 1 } pass against the schema "unevaluatedProperties": true because nothing created any annotations in this case either? And you say that unevaluatedProperties only considers the instance properties if there is an annotation of properties (even if [], and not if there is none?

[karenetheridge]

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 additionalProperties behaviour being inconsistent depending on whether annotations are used is to simply drop the mention of annotations for determining the result (true/false, and individual errors).

That is, change this paragraph:

The behavior of this keyword depends on the presence and annotation results of "properties" and "patternProperties" within the same schema object. Validation with "additionalProperties" applies only to the child values of instance names that do not appear in the annotation results of either "properties" or "patternProperties".

to (bringing back some of the same language as from draft7):

The behavior of this keyword depends on the presence of "properties" and "patternProperties" within the same schema object. Validation with "additionalProperties" applies only to the child values of instance names that do not match any names in "properties" or match any regular expression in "patternProperties".

...and omit the final paragraph in this section.

PS. the same inconsistency exists in additionalItems, so we should do the same thing there.

[Relequestual]

I'm in agreement here.

[karenetheridge]

I'll try to find the time to put together a PR (or two).

[karenetheridge]

PR created for the interim revision: json-schema-org/json-schema-spec#1154

I'm putting together a test implementation of my ideas for changing unevaluated* and will start a discussion post for that (targeting the next major revision).

[marksparkza]

Thanks for doing this PR @karenetheridge. It looks great to me. And thank you all for the follow-up discussion on this topic. You guys are doing amazing work!

[Relequestual]

Related discussion suggesting an altered behaviour #67

[gregsdennis]

More related discussions from users of my library:

• "additionalProperties" being validated as a property  json-everything/json-everything#39
• JsonSchema errors for additionalProperties not the same as 2019-09 draft specification json-everything/json-everything#89
• All values fail against the false schema issue json-everything/json-everything#216
• All values fail against the false schema when additionalProperties is set to false json-everything/json-everything#221

This is definitely a confusing result of using annotations. From the latest of those:

this is a weird side effect of the annotation behavior, and I'm not sure that we (the JSON Schema team) fully understood all of this when we put this behavior in. It was intended to make things simpler (for implementers) by providing a model to describe how keywords could interact. Instead it made failures harder to debug for schema authors (who should be the real focus, IMO).