-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
"unevaluatedItems" should consider "contains" #810
Comments
This was me being unable to read that unevaluatedItems only applies when items is defined as an array. With that light, there’s no valid argument that I can think of that would warrant interaction between these keywords. -- Edit: The intent of |
Actually, I'm pretty sure I left out something important. Consider the following schema: {
"type": "array",
"oneOf": [
{
"contains": {"type": "string"},
},
{
"items": {"type": "integer"}
}
],
"unevaluatedItems": {"type": "boolean"}
} The following instances should all be valid:
I had intended for all array child applicators to affect I seem to have left out the necessary specification of a Not sure if there's a use case, but logical consistency was the goal. |
Logical. Sorry, on mobile. |
Yes, there is language in the existing description that requires
That is correct. |
Hm... lots of test cases there. |
Well, my understanding / expectation is correct then. |
@Relequestual if you find anywhere that makes that behavior explicit, let me know. I don't think there's anything there that contradicts it, and it obviously won't require a meta-schema update. I made that "draft-08-patch1" milestone to collect compatible fixes based on early feedback. Or if we end up thinking that this is a kind of glaring error we can put this and whatever else we've noticed in and re-publish the core spec sooner rather than later. |
I think the phrase "annotations are dropped from failed evaluations" is key here. Does It only really matters what format the annotations take for the output format data. I guess The prose / explanation about how annotation results are used will need to be updated here: json-schema-spec/jsonschema-core.xml Lines 2209 to 2217 in 38e1606
|
Given the proceeding paragraphs, making this fix would legalistically be a non backwards compatible change, but I think it's clearly a mistake. json-schema-spec/jsonschema-core.xml Lines 2219 to 2232 in 38e1606
|
@Relequestual yeah, this falls under "bugfix" |
@handrews I don't think that |
Also, I don't think I'm doing this right. (Edit: Fixed with v12.1.0) |
Yes, that's correct, good catch. |
I read that and made a wrong assumption. Yikes. |
This issue has changed from it's original, but that's not a problem, as a bug is still highlighted. |
Wait, no. We are forgetting something here.
It's here in draft 2019-09: json-schema-spec/jsonschema-core.xml Lines 2219 to 2225 in d561ae6
From draft-7... json-schema-spec/jsonschema-validation.xml Lines 527 to 536 in 1afc34b
If we want to change this, so you COULD do Thoughts? Here's my example of |
I'm opening a channel for this on slack as I think we will have some discussion. We should make sure we are on the same page and understand correctly before determining what to do here. |
|
@handrews @gregsdennis I think I follow now. {
"type": "array",
"oneOf": [
{
"contains": {"type": "string"}
},
{
"items": {"type": "integer"}
}
],
"unevaluatedItems": {"type": "boolean"}
}
// Valid Instances
[1, 3, 4 ],
// each item is valid according to the `items` schema value which results in a `true` annotation,
// so `unevaluatedItems` is not applied
["a", "b", "c"],
// each item is valid when the `contains` schema value is applied, resulting in an annotation of [1, 2, 3],
// there are no additional items in the array, so `unevaluatedItems` is not applied
[true, "hello", false],
// `items` fails validation, so produces no annotation
// `contains` is applied, and results in an annotation of [1] for the index of which item is valid
// `unevaluatedItems` is applied to the other indexs of the array
// Invalid instances
["a", false, 2, "b", 8 ]
// `items` is not met, so produces no annotation
// `contains` is applied and results in an annotation of [0, 3]
// `unevaluatedItems` is applied to the other items in the array, and fails because numbers are not booleans
// Another example
{
"allOf": [
{
"items": [
{ "type": "integer" },
{ "type": "string" }
true,
]
},
{
"contains": { "type": "object", "required": ["error"] }
}
],
"unevaluatedItems": { "type": "object", "required": ["debug"] }
}
// Valid
[1, "errorABC", { "error": 123 }]
// Int, string, anything is fine
// Must contain an object with an error
// `items` results in annotation of [0,1,2]
// `contains` results in annotation of [2]
// `unevaluatedItems` is not applied to the array as there are not additional items beyond item at index 2.
[2, "errorABC", "fail reason as string", { "error": 123 }]
// `items` results in annotation of [0,1,2]
// `contains` results in annotation of [3]
// `unevaluatedItems` is not applied to the array as above
[3, "errorABC", "fail reason as string", { "error": 123 }, { "error": 456 }]
// `items` results in annotation of [0,1,2]
// `contains` results in annotation of [3, 4]
// `unevaluatedItems` is not applied to the array as above
[4, "errorABC", "fail reason as string", { "error": 123 }, { "debug": "some debug info" }]
// Same annotations as previous
// item at index 4 has `unevaluatedItems` schema value applied
[5, "errorABC", "fail reason as string", { "debug": "some debug info" }, { "error": 123 }]
// `items` results in annotation of [0,1,2]
// `contains` results in annotation of [4]
// `unevaluatedItems` is applied to item at index 3
// Invalid
[6, "errorABC", "fail reason as string", { "info": "some other info" }, { "error": 123 }]
// `items` results in annotation of [0,1,2]
// `contains` results in annotation of [4]
// `unevaluatedItems` is applied to item at index 3
// Validation fails for item at index 3 when `unevaluatedItems` schema value is applied.
[7, "errorABC", "fail reason as string", false, { "error": 123 }]
// Same as above
[8, "errorABC", "fail reason as string", { "error": 123 }, { "info": "some other info" }, { "error": 456 }]
// `items` results in annotation of [0,1,2]
// `contains` results in annotation of [3, 5]
// `unevaluatedItems` is applied to item at index 4
// Validation fails for item at index 4 when `unevaluatedItems` schema value is applied. |
I think that lines up with what @handrews is saying, but I don't agree with In my interpretation, That means that your third case changes:
Writing that out, it feels wrong. Maybe I'm getting hung up on the word "contains" meaning that its target is the array and is validating whether it contains something. Instead it's working as sort of a "some of the items should be this, and I want to know which items they are, then don't re-evaluate them, like ever." But I don't know of a single word that does that, so maybe "contains" works, but it still feels weird. In short, @handrews wrote the |
@handrews For me, this is enough justification for That being said, it may still be useful to collect the annotations which indicate WHICH items in the array pass the Assuming we resolve #864 and end up with Thoughts? |
@Relequestual I disagree with @gregsdennis's interpretation. Let's pretend we had a keyword If we could write out an infinite sequence of these, so that they could cover an indefinitely long array, we could write
"allOf": [
{
"singleItem": [0, {...}]
},
{
"singleItem": [1, {...}]
},
{
"singleItem": [2, {...}]
},
...
]
"anyOf": [
{
"singleItem": [0, {...}]
},
{
"singleItem": [1, {...}]
},
{
"singleItem": [2, {...}]
},
...
] So, if we said that Is this more clear? |
Conceptually, {
"type": "array",
"items": [{ "type": "string" }, { "type": "number" }],
"contains": { "const": 42 },
"additionalItems": { "type": "number", "maximum": 10 }
} Example 1: If we replace {
"type": "array",
"items": [{ "type": "string" }, { "type": "number" }],
"contains": { "const": 42 },
"unevaluatedItems": { "type": "number", "maximum": 10 }
} Assuming Notice that the validation result for the second example has changed. I strongly believe that nothing should change, but if there's consensus that |
Core Specification Section 9.1 (keyword independence):
This makes no mention of Section 9.3.1.4 (
The array is valid, not the element. The subschema must be applied to all elements when collecting annotations, but it does not say that the annotation applies to the element. These two combined implies that the annotations apply to the array. |
@gregsdennis I'm not quite sure why you're quoting the spec here. We're trying to work out if the spec IS right or needs changes. @jdesrosiers I can see your point, and conceptually I agree, but in reality I think that IS OK. After all, the keyword @handrews I think I agree with you here. The example @jdesrosiers provided showing how the |
I probably agree with @jdesrosiers on
at least in theory. Whether that means I don't think anyone has ever asked how or if So, on the plus side, defining consistent mechanisms is helping us figure out how keywords should behave. On the minus side, that's kind of irritating 😛 |
@Relequestual I'm quoting the spec because my understanding is derived from reading it. (It provides evidence for my arguments.) Ergo, if my understanding is wrong, then the spec needs to change to be more explicit about the intended meaning. It's always helpful to know what the current state is when deciding whether it should change. |
you are right @gregsdennis in your reading and understading of the spec 2019-09, but it also says the subschema is applied to each item in the array... but I get your point. |
I'm marking this as blocked till #864 is resolved. |
Good use case from @rjmill
|
We decided to go with the two-keyword This further validates breaking the array/object symmetry by dumping This means that I'm going to write a PR for both this and #864 as they need to rework basically the same areas of the spec. |
@gregsdennis ok I lied I have one thing to say since I decided to reread and figure out why I thought this was done.
The annotations from the subschema apply to the element(s) because the subschema applies to the element(s). That's how annotations work. The annotation from This is why it never occurred to me that this was unresolved. There's nothing strange about contains except that it ORs instead of ANDs. But that's also true of |
@Relequestual this needs a final call ASAP. |
I have discussed this thoroughly in Slack in a PM and have come to the conclusion that
NOTE Italic text is unchanged since the keyword was introduced in Draft 6. Similarly The note that was added about annotations states that the
Annotations should be applied as specified by the keyword that produces them.
If Not only does this interpretation yield symmetry between objects and arrays, it matches more closely to the definition of the word "contains" and how the schema authors (not spec authors; we're a strange lot) would interpret that keyword and the definition that the spec currently gives for it. As a consequence of the realized parallelism with |
I don't think I don't think there is a parallel keyword to |
Had another discussion, and the point came up that single-form- Building on that symmetry, I'd be okay with applying annotations at both the array and item level, given this. |
@gregsdennis yes, |
I would like to clarify the instance location of the annotations of a child-schema applicator keyword - be it @handrews says (#810 (comment)):
and @gregsdennis says (#810 (comment))
(bolding added) but the spec seems to me to indicate that the annotation of
it doesn't quite explicitly say this is attached to the array, but the section on collecting annotations indicates the instance location of an annotation is the one to which the keyword applies. for items that's the array. moreover, it doesn't make sense to apply the annotations from same with array subschema keywords producing annotations indicating applied indices, whether it's using true or max index or an array of indices. this is long-winded for a bit of a sidetrack, but I think it's worth getting everybody on the same page, given how much of the above discussion is hung up on where these annotations apply. |
The interaction of these seems underspecified. @Relequestual what was the problem you ran into here exactly? I've already forgotten 😬
The text was updated successfully, but these errors were encountered: