Skip to content
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

Section 9.2.1 (*Of keywords) is non-specific about sequencing #910

Closed
gregsdennis opened this issue May 3, 2020 · 20 comments · Fixed by #1018
Closed

Section 9.2.1 (*Of keywords) is non-specific about sequencing #910

gregsdennis opened this issue May 3, 2020 · 20 comments · Fixed by #1018

Comments

@gregsdennis
Copy link
Member

This may be irrelevant to the test case that highlighted the issue to me, but it seems that the spec should say whether sequence of evaluation is important.

@notEthan
Copy link
Contributor

notEthan commented May 3, 2020

do you mean the sequence of which comes first among oneOf, anyOf, allOf when multiple are present? or the sequence of evaluating the subschemas in the *Of arrays?

if the former, I think that since *Of are not listed in the exceptions to keyword independence in https://json-schema.org/draft/2019-09/json-schema-core.html#rfc.section.9.1 , they can be assumed to be independent and not not care about sequence.

if you mean the latter, I think that the way each of the keywords is defined is clear that order deosn't matter.

@handrews
Copy link
Contributor

handrews commented May 3, 2020

@gregsdennis there is nothing about ordering that would change the outcome of these keywords at all. Ordering is an implementation detail and best we stay silent on it.

@handrews handrews added the core label May 3, 2020
@gregsdennis
Copy link
Member Author

gregsdennis commented May 4, 2020

I don't mean the sequencing of the keywords, but rather the sequence of evaluation of their subschemas.

For instance, given the schema/instance in the test case I linked:

{
  "$schema": "https://json-schema.org/draft/2019-09/schema",
  "allOf":[
    {
      "properties": {
        "foo": true
      }
    },
    {
      "unevaluatedProperties": false
    }
  ]
}

{
  "foo": 1
}

If an implementation were to get things wrong and implement these to run in order rather than independently, the unevaluatedProperties here would recognize that foo has already been eval'd.

But I feel that if the spec is clear that the sequence of subschemas is unimportant, the implementor would get this right more than not.

@handrews
Copy link
Contributor

handrews commented May 4, 2020

@gregsdennis I was talking about the evaluation of their subschemas.

That unevaluatedProperties would forbid all properties anyway, because {"properties": {"foo": true}} is a sibling schema, not a subschema. That's a really important part of unevaluatedProperties specifically because the JSON Schema processing model doesn't really account for sibling schema processing order or coordination.

We process one schema object at a time, and a schema object's subschemas need to be processed before it can be fully processed itself. I decided that putting a restriction on processing order of keywords within a schema object was OK, because additional* effectively did that already. But processing order / communication among schema objects other than from subschema up to parent schema were not feasible.

@handrews
Copy link
Contributor

handrews commented May 4, 2020

So, yes I see the problem you mention, but I specifically made a point to avoid it with unevaluatedProperties. That was a major point of seeing, as I spent waaaaay too much time trying to account for sibling schemas. But then you have to ask whether cousin schemas should factor in, and suddenly everything affects everything and it's a mess.

@gregsdennis
Copy link
Member Author

So it seems the consensus is that sequence of subschema evaluation does not matter. I just think it should be explicitly stated.

@handrews
Copy link
Contributor

handrews commented May 4, 2020

@gregsdennis I'd like to do a holistic wording/flow review after I finish the big things. In which I will answer questions and generally endorse whatever seems to make things the most clear without bloating the spec. But let's put this on hold until then.

@handrews handrews added this to the draft-08-patch1 milestone May 4, 2020
@Relequestual
Copy link
Member

Just to check my understanding here, are we saying that the annotations from applying subschemas from an applicator which has multiple schemas is not complete till it has applied all the schemas? If so, this (issue and the related test that was added) makes sense. Otherwise, can someone explain this to me please?

@karenetheridge
Copy link
Member

I thought of another case where ordering is significant - take my answer to this SO question here -- https://stackoverflow.com/a/62958916/40468

If that $ref keyword were evaluated first, before any properties present at the top level, unevaluatedProperties will have an incomplete set of annotations to work from and will generate a different result than if properties had been evaluated first before the $ref. What should be the expected behaviour here? or is it indeterminate, because of unspecified keyword evaluation order?

(in my implementation, I evaluate all the core ($*) keywords first, then all the validator keywords like type, minimum etc, and then applicators. I believe the only ordering that is mandated in the spec right now is that unevaluatedItems and unevaluatedProperties should come last.)

@handrews
Copy link
Contributor

@karenetheridge I'm not sure that answer is correct. unevaluatedProperties is useful outside of a $ref or other applicator ($ref is essentially an applicator, it's just that the schema it applies lives somewhere else- you probably know this but just for folks reading who might not).

It is not useful to put it inside of a referenced schema, unless that schema has further complex applicator structure.

unevaluatedProperties cannot "see" parent, sibling, or cousin schemas (in terms of the runtime dynamic structure- lexically they may all be under $defs and therefore all cousins, but it's dynamic scope that matters here, not lexical scope). The reason for this is exactly what you observe: it is not possible to establish a clear ordering, or even necessarily find all of the other potentially relevant keywords without deadlocking as you hit multiple occurrences of unevaluatedProperties, when evaluating anything but child schemas of keywords within the same schema object.

@Relequestual
Copy link
Member

@karenetheridge My thoughts on this are, all applicators have to be processed before unevaluatedItems and unevaluatedProperties, so the SO example you linked to (using $ref applicator) isn't the same as using an allOf applicator.

Essentially, this boils down to, there IS NO order in which an applicator with multiple schemas to apply should apply them in. They are "locally" all applied at the same time, and therefore cannot use the annotations from another sibling schema to determin their results.

These two schemas are not equivilent...

{
  "$schema": "https://json-schema.org/draft/2019-09/schema",
  "allOf":[
    {
      "properties": {
        "foo": true
      }
    },
    {
      "unevaluatedProperties": false
    }
  ]
}

{
  "$schema": "https://json-schema.org/draft/2019-09/schema",
  "$defs": {
    "base_object": {
      "unevaluatedProperties": false
    }
  },
  "$ref": "#/$defs/base_object",
  "properties": {
    "foo": true
  }
}

{
  "foo": 1
}

Do you see that @gregsdennis ? Is there anything we could change in the spec to make this clearer?

@handrews do you think it's worth defining that the annotation collection from subschemas being applied through a multi-applicator keyword are not "fully collected" / "useable" / [something else] till all subschemas are applied?

Maybe something like "Annotation collection from the result of applying subschemas from an applicator keyword that may contain multiple subschemas happens simultaneously. As such, keywords which rely on annotation MUST NOT use annotations collected from sibling schema, because sibling schemas are logically applied simultaneously."

@gregsdennis
Copy link
Member Author

That sounds good to me.

@handrews
Copy link
Contributor

@Relequestual @gregsdennis I'd like to understand why this language:

The behavior of this keyword depends on the annotation results of adjacent keywords that apply to the instance location being validated.

was not sufficient to convey this behavior. Because depending on the answer there may be other stuff to update.

@Relequestual
Copy link
Member

@handrews when you quote that, it makes perfect sense.
It's not just "annotation results attached of that instance location", but as it says, ONLY adjacent keywords.

I think the confusion (forgetting that specific clause) is down to how we (I) often phrase it as "see through applicator keywords" without the "adjacent" qualifier.

Maybe some IETF requirements keyword (rfc2119) could be added in the related sections to draw peoples attention to it better?

(Kind of as an aside, I feel like quite a bit of the spec could be rephrased to include requirement keywords.)

@handrews
Copy link
Contributor

Note that the term "adjacent keywords" is more explicitly described in PR #981. Do we need to do more here?

@gregsdennis
Copy link
Member Author

gregsdennis commented Sep 18, 2020

I think some of my premise was wrong. In [this comment] I hypothesize that an implementor could get something wrong, and in that case the evaluation wouldn't work right.

But I don't think that's an issue that the spec needs to handle. It seems that several test cases (in the test suite) could be created to verrify that an implementation has it right.

Furthermore, the example I give doesn't describe an issue of ordering but one of adjacent subschema independence. In the example, /allOf/0 and /allOf/1 should be processed independently and have no interaction. With this independence, the sequence being unimportant is implied.

@handrews
Copy link
Contributor

@gregsdennis thanks, it sounds like we can just drop #992 and the only clarification that is needed (about what "adjacent keywords" means is in #981).

BTW, in terms of subschema independence, if there were really complications there, it's theoretically possible that evaluating a 3-branch allOf as /allOf/0 and /allOf/1 and then /allOf/2 could interact differently from evaluating /allOf/1 and /allOf/2 and then /allOf/0. That's why I mentioned associativity. I mean, I have no idea how that could happen, but was unclear on the problem to begin with so I was covering all the bases I could think of.

@handrews
Copy link
Contributor

In case anyone wants to know what a non-commutative, non-associative system looks like: https://en.wikipedia.org/wiki/Octonion 😆

@helpr helpr bot added pr-rejected and removed pr-available labels Sep 20, 2020
@Relequestual
Copy link
Member

@gregsdennis given #981 is merged now, can this issue be closed?

@gregsdennis
Copy link
Member Author

gregsdennis commented Nov 14, 2020

I'm not seeing how that PR addresses this issue. That PR is all about unevaluated*. This issue is about *Of and how subschemas (don't) interact.

It mentions "adjacent keywords", but this is about "adjacent subschemas".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment