Skip to content

Drop the idea of automatically combining annotations #906

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

Closed
handrews opened this issue May 1, 2020 · 4 comments · Fixed by #986
Closed

Drop the idea of automatically combining annotations #906

handrews opened this issue May 1, 2020 · 4 comments · Fixed by #986

Comments

@handrews
Copy link
Contributor

handrews commented May 1, 2020

We have this idea that some annotations get combined (e.g. if multiple readOnly keywords apply to the same location, the overall annotation is true if any of them are true).

However, our output recommendations don't support that. They just return all of the information needed to implement it (all of the annotation values, where they come from, and to where they are applied).

Since annotations are intended to be processed by the application anyway, we can get the necessary effect for keywords like readOnly by directing applications on how they process the results. For readOnly and writeOnly, our language actually supports that anyway:

The value of these keywords MUST be a boolean. When multiple occurrences of these keywords are applicable to a single sub-instance, the resulting behavior SHOULD be as for a true value if any occurrence specifies a true value, and SHOULD be as for a false value otherwise.

Note that this doesn't say anything about implementations combining things, just about the resulting behavior.


This would involve some changes to the Core spec:

In section 7.7.1 Collecting Annotations, we would change:

If the same keyword attaches values from multiple schema locations to the same instance location, and the annotation defines a process for combining such values, then the combined value MUST also be associated with the instance location. The output formats described in this specification that include annotation information meet this requirement.

to something like:

When the same annotation keyword attaches values from multiple schema locations to the same instance location, the keyword MAY specify how applications SHOULD or MUST interpret the different values.

In section 7.5 Applicators, we would drop:

Annotation results are combined according to the rules specified by each annotation keyword.

There are probably other spots, but this gets the idea across. The whole "combine annotations" thing was never really thought through all the way in the context of output formats and other things that were added after the initial idea.

@ssilverman
Copy link
Member

This seems like a good place to ask this. If we drop "combining annotations", that leaves the rest of the sentence, and I'd like to inquire as to its intent.

The text:

Annotation results for "properties" keywords from multiple schemas applied to the same instance location

Can a short example be provided that shows where you can actually apply multiple schemas to the same instance location? I can think of, for example, "allOf", but since "additionalProperties" (I'm neglecting "unevaluatedProperties" for now) can't peer inside an "allOf", how can multiple schemas even apply?

I should be asking, actually: "What is the intent behind 'multiple schemas'" here?

@ssilverman
Copy link
Member

ssilverman commented May 1, 2020

I'll answer my own question. The phrase, "same schema object" is key in this paragraph from "annotationProperties":

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

@Relequestual
Copy link
Member

@Julian @gregsdennis
Do tests need altering as a result of this change?

@gregsdennis
Copy link
Member

The tests aren't checking output right now because no one had been able to identify a reasonable mechanism for checking it (not sure how much effort has gone into trying, either). As long as the validation results are the same, no change is required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants