Skip to content

Commit

Permalink
Add folder for architectural decision records (ADRs)
Browse files Browse the repository at this point in the history
Add first spec related ADR about the handling of additionalProperties ambiguity in 2019-09 and 2020-12 for the patch release by adding a CREF
  • Loading branch information
Relequestual committed Apr 14, 2022
1 parent f13350a commit e8a6028
Show file tree
Hide file tree
Showing 3 changed files with 174 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
# Acknowledge ambiguity in additionalProperties behaviour and fix after patch release

* Status: proposed
* Deciders: @relequestual @gregsdennis, @jdesrosiers, @karenetheridge
* Date: 2022-04-08

https://github.com/json-schema-org/json-schema-spec/issues/1172
https://github.com/json-schema-org/community/discussions/57
https://github.com/json-schema-org/json-schema-spec/pull/1203

## Context and Problem Statement

When we changed the specification to use annotations as the context in which some keywords behave, we included a clause that implementations which didn't use annotations may implement or optimize the processing of `additionalProperties` in another way which produces the same effect as the prior behaviour.
This section created an ambiguity in terms of the resulting output format, but not validation.

We needed to decide on how to proceed for the patch release of the 2020-12 version of the specification.

The two above links are to a GitHub Discussion and a GitHub Issue detailing the problems.
Details with an example of the problem can be seen in the Discussion's opening post specifically.

## Decision Drivers <!-- optional -->

* The "patch release" should not change anything functionally
* Annotations as they are, are confusing to users, implementers, and specification editors alike
* Patch release is behind schedule
* There are currently no tests for the output format
* It's hard to see any immediate consensus on changing the annotation based behaviour

## Considered Options

* [Leaving it "as is" and do nothing](https://github.com/json-schema-org/community/discussions/57#discussioncomment-1413777)
* [Pick one](https://github.com/json-schema-org/community/discussions/57#discussioncomment-1416683) of the behaviours
* [Revert back to draft-07 behaviour](https://github.com/json-schema-org/community/discussions/57#discussioncomment-1453723)
* [Reinterpret how we understand annotation collection](https://github.com/json-schema-org/json-schema-spec/issues/1172#issuecomment-1049686478) to allow reading annotations within the same schema object regardless of assertion results
* [Acknowledge and accept that two approaches and results are allowable](https://github.com/json-schema-org/community/issues/161#issue-1173742930)
* Redefine annotation collection behaviour and/or how `additionalProperties` works

## Decision Outcome

Chosen option: "Acknowledge and accept that two approaches and results are allowable", because

* Leaving it "as is" will continue to cause confusion
* The change is non-functional which is required for the patch release
* The patch release is behind schedule
* Finding consensus of other solutions proved to be difficult
* There's no test suite for the output format, so it's not easy to see unintended consequences of a functional change
* We need to properly re-evaluate annotation collection and how annotations are used by other keywords

### Positive Consequences

* Patch release can move forward
* Validation result is not impacted
* Confusion is at least seen and acknowledged
* Implementations which pick either approach are seen to be compliant

### Negative Consequences

* May have an impact for downstream tools which process full output data
* A test suite (not yet developed) which covers this situation needs to allow for multiple valid answers

## Pros and Cons of the Options

### Leaving it "as is" and do nothing

Agree to do nothing and hope for the best. There isn't any downstream tooling yet anyway.

* Good, because no functional change
* Good, because no impact on downstream tooling
* Bad, because leaves a known ambiguity in the specification

### Pick one / Revert to draft-07 behaviour / Reinterpret annotation collection

* Good, because ambiguity is removed
* Good, because not many tools will be effected
* Bad, because it can be seen as a functional change (not really allowed for the patch release)
* Bad, because it can break existing implementations and downstream tools
* Bad, because without a test suite it's hard to see unexpected consequences

## Links

* Issue: [Ambiguous behaviour of additionalProperties when invalid](https://github.com/json-schema-org/json-schema-spec/issues/1172)
* Discussion: [The meaning of "additionalProperties" has changed](https://github.com/json-schema-org/community/discussions/57)
* Resolving Pull Request: [Add CREF about ambiguous behaviour of additionalProperties](https://github.com/json-schema-org/json-schema-spec/pull/1203)
* Alternative solution proposal: [Resolve contradictions in the described behaviour of "additionalProperties" and "items"](https://github.com/json-schema-org/json-schema-spec/pull/1154)

* [Result of discussing](https://github.com/json-schema-org/json-schema-spec/issues/1172#issuecomment-1063962900) on an Open Community Working Meeting call - @jdesrosiers proposed a less controversial and more agreeable solution to add a comment that both are allowable
* [Related GitHub Discussion](https://github.com/json-schema-org/community/discussions/67) on alternative behaviour for `unevaluated*` keywords
15 changes: 15 additions & 0 deletions adr/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Architectural Decision Log

This log lists the architectural decisions for the JSON Schema specification.

<!-- adrlog -- Regenerate the content by using "adr-log -i". You can install it via "npm install -g adr-log" -->

* [ADR-2022-04-08](2022-04-08-cref-for-ambiguity-and-fix-later-gh-spec-issue-1172.md) - Acknowledge ambiguity in additionalProperties behaviour and fix after patch release

<!-- adrlogstop -->

You can find the ADR for using ADRs in our [community repo ADR log](https://github.com/json-schema-org/community/tree/HEAD/docs/adr).

For new ADRs, please use [template.md](template.md) as basis.
More information on MADR is available at <https://adr.github.io/madr/>.
General information about architectural decision records is available at <https://adr.github.io/>.
72 changes: 72 additions & 0 deletions adr/template.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# [short title of solved problem and solution]

* Status: [proposed | rejected | accepted | deprecated | … | superseded by [ADR-0005](0005-example.md)] <!-- optional -->
* Deciders: [list everyone involved in the decision] <!-- optional -->
* Date: [YYYY-MM-DD when the decision was last updated] <!-- optional -->

Technical Story: [description | ticket/issue URL] <!-- optional -->

## Context and Problem Statement

[Describe the context and problem statement, e.g., in free form using two to three sentences. You may want to articulate the problem in form of a question.]

## Decision Drivers <!-- optional -->

* [driver 1, e.g., a force, facing concern, …]
* [driver 2, e.g., a force, facing concern, …]
*<!-- numbers of drivers can vary -->

## Considered Options

* [option 1]
* [option 2]
* [option 3]
*<!-- numbers of options can vary -->

## Decision Outcome

Chosen option: "[option 1]", because [justification. e.g., only option, which meets k.o. criterion decision driver | which resolves force force | … | comes out best (see below)].

### Positive Consequences <!-- optional -->

* [e.g., improvement of quality attribute satisfaction, follow-up decisions required, …]
*

### Negative Consequences <!-- optional -->

* [e.g., compromising quality attribute, follow-up decisions required, …]
*

## Pros and Cons of the Options <!-- optional -->

### [option 1]

[example | description | pointer to more information | …] <!-- optional -->

* Good, because [argument a]
* Good, because [argument b]
* Bad, because [argument c]
*<!-- numbers of pros and cons can vary -->

### [option 2]

[example | description | pointer to more information | …] <!-- optional -->

* Good, because [argument a]
* Good, because [argument b]
* Bad, because [argument c]
*<!-- numbers of pros and cons can vary -->

### [option 3]

[example | description | pointer to more information | …] <!-- optional -->

* Good, because [argument a]
* Good, because [argument b]
* Bad, because [argument c]
*<!-- numbers of pros and cons can vary -->

## Links <!-- optional -->

* [Link type] [Link to ADR] <!-- example: Refined by [ADR-0005](0005-example.md) -->
*<!-- numbers of links can vary -->

0 comments on commit e8a6028

Please sign in to comment.