Test interaction between unevaluatedProperties and additionalProperties

[V02460]

I found this scenario where the output of check-jsonschema deviates from my reading of the spec.

In my understanding, unevaluatedProperties: false should evaluate successfully if the subschema additionalProperties matches all properties. E.g. {"foo": "foo"} should be a valid instance of the following schema.

{
    "$schema": "https://json-schema.org/draft/2020-12/schema",
    "additionalProperties": { "type": "string" },
    "unevaluatedProperties": false
}

[gregsdennis]

We do have this test already:

https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/82a077498cc761d69e8530c721702be980926c89/tests/draft2020-12/unevaluatedProperties.json#L134-L162

Your case should be covered by the "with additional properties" scenario. Note how bar is accepted by additionalProperties and thereby ignored by unevaluatedProperties.

[V02460]

Thanks for taking a look :) I am aware of this test, I actually started writing this one based off it. The difference between the two is that a real life JSON Schema implementation (check-jsonschema) works correctly for one, but not for the other. That is a strong reason to cover both variants in the test suite.

[gregsdennis]

I'd be interested in what the difference between the tests is that trips up the implementation. To me, your new test is a duplicate. If you can do some experimentation with it to isolate what the trigger is, maybe we can better target the test.

[V02460]

I did experiment a bit and to me it seems the difference is whether additionalProperties is false or is an object. Thus the “non-bool” bit in the test’s description.

If this is about the description of the tests, I’m fine with rewording them to, e.g.

• unevaluatedProperties with adjacent boolean additionalProperties
• unevaluatedProperties with adjacent object additionalProperties

or anything else you see fit.

I’m not so happy about calling this test a duplicate. Tests do only ever confirm that some property holds for a single input. For everything but the most trivial cases, the wast majority of inputs are never tested. So for any one of these inputs an implementation might misbehave while working fine for the rest. Thus adding a test for another input is not adding a duplicate, but sampling more of the input space and gaining more confidence a property holds.

[V02460]

Sry, reopening, closed by accident

[gregsdennis]

I'll let @Julian weigh in here, but I wonder whether the implementation is failing to handle additionalProperties as an object overall. (We do have tests for that also.)

I'm not arguing that a case that an implementation misses shouldn't be added. In fact, our general policy is to include these cases. I'm just trying to dig down to find what the implementation is actually failing on so that we can create the appropriate test.

[karenetheridge]

I think the additional test being proposed in this PR is good and we should keep it, and additionally we should simplify the existing test that Greg linked to above -- the type keyword is redundant and should be removed, and also the properties keyword should be removed (or we split this into two tests -- one with properties and one with additionalProperties -- where the outcome is the same for both, that all string properties are evaluated, and therefore "unevaluatedProperties": false doesn't cause invalidity.

It's possible that this failing implementation is looking directly at "additionalProperties": true and short-circuiting on that, or perhaps it is failing to interpret this the same as "properties": {"type": "string"} evaluating to true for every property in the instance. Splitting into two tests will let us see which of these is true -- or something else entirely.

[Julian]

Thanks for the PR.

We (speaking on behalf of the implementation for a minute) definitely handle additionalProperties fine on its own, so it's something in the interaction between the two we're not doing right, and probably something trivial to fix (I'd bet it's a one line fix without having looked yet) and yes probably has to do with assuming that additionalProperties always is a boolean when it's next to unevaluatedProperties or something trivially wrong like that.

(Back to ignoring the implementation) -- splitting/simplifying the existing test does seem right to me as well, though changing existing tests is careful work, so I'd typically hope if a new contributor is willing to try to figure that out great, if not this new test seems pretty straightforward to me to merge and then file an issue to do that for whoever is willing.

[V02460]

Simplified tests by removing the superfluous type keywords in the whole file and additionally removing the properties keyword from the adjacent additionalProperties test. Splitting it up was not necessary, because the adjacent properties test does exactly that.

Is there an easy way to actually run this test suite with one or more tools btw?