docs(adr): add ADR-0001 separating additionalProperties from x-gts-sealed#74
docs(adr): add ADR-0001 separating additionalProperties from x-gts-sealed#74aviator5 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ADR-0001 introducing Changesx-gts-sealed Modifier ADR
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@Artifizer please take a look |
aviator5
force-pushed
the
additional-properties-adr
branch
from
e63821b to
33631a0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adr/0001-additional-properties-and-sealed.md`:
- Around line 281-282: The sentence claiming full backward compatibility
contradicts the new §5 MUST-NOT rule; update the paragraph to narrow the claim
(e.g., replace the current sentence with a line such as “No runtime
instance-validation behavior changes unless an implementation adopts the
x-gts-sealed extension” or equivalent) so it accurately reflects that
registration-time conformance may reject previously-accepted schemas while
runtime validation remains unchanged absent x-gts-sealed; reference §5 MUST-NOT
and the term x-gts-sealed when editing the sentence.
- Around line 232-233: The OP#12 enforcement text for "x-gts-sealed" currently
only references the "properties" key and leaves "patternProperties" ambiguous;
update the ADR text around OP#12 (the paragraphs discussing "OP#12 enforcement"
and the later lines about patternProperties) to either (a) explicitly include
patternProperties in the sealed check (e.g., state that keys matched by
patternProperties are treated as sealed and derived schemas may not introduce
new pattern-matching keys), or (b) explicitly exclude patternProperties from
OP#12 and note that pattern-based key semantics are deferred to a follow-up ADR;
reference the "x-gts-sealed" / "OP#12 enforcement" paragraphs and the later
"patternProperties" mention when making this clarification so implementations
have an unambiguous normative decision.
- Around line 220-223: Update the rule for `additionalProperties: false` in
adr/0001-additional-properties-and-sealed.md to narrow the disallowed cases:
forbid only when the set of allowed property names is introduced or becomes
known inside composition branches or `$ref` (i.e., when the object with
`additionalProperties: false` does not itself declare the complete set of
`properties`/`patternProperties`), allow `additionalProperties: false` when all
property names are declared at that same object level even if
`oneOf`/`anyOf`/`allOf` are used to constrain values, and add an explicit
recommendation to prefer `unevaluatedProperties: false` for modular/composed
schemas.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03d841d3-30ed-47d1-b640-ad0d66dbbec7
📒 Files selected for processing (1)
adr/0001-additional-properties-and-sealed.md
| > `additionalProperties: false` MUST NOT appear in a schema object that also contains `allOf`, `$ref`, `oneOf`, or `anyOf` referencing another schema (whether at the top level of the schema, or inside an `allOf` branch with sibling property declarations). Such combinations are rejected by JSON Schema 2020-12 in-place-applicator semantics and would reject the very inherited properties the author intended to keep. `additionalProperties: false` IS allowed in: | ||
| > - schemas that do not compose via `allOf`/`$ref` (standalone or root base schemas with no GTS parent), and | ||
| > - inline-defined nested object schemas inside a derived schema (e.g., the `payload` sub-schema of an event derivation). | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JSON Schema draft 2020-12, does using additionalProperties: falsetogether withoneOforanyOfalways produce the same in-place-applicator inheritance issue asallOf+$ref, or are there valid patterns where it is safe?
💡 Result:
No—additionalProperties: false combined with oneOf/anyOf does not always force the same “in-place-applicator inheritance” problem you may have seen with allOf + $ref. In JSON Schema 2020-12, the root issue is that additionalProperties only accounts for property evaluations “from its immediate sibling keywords”, and it cannot “see through” applicators/composition keywords like allOf/anyOf/oneOf the way 2020-12’s unevaluatedProperties can. This is why patterns that work with unevaluatedProperties can fail with additionalProperties in composition/ref “inheritance” situations [1][2][3][4]. So there are safe patterns where additionalProperties: false behaves correctly with oneOf/anyOf, but they are constrained: 1) Safe pattern: every possible property name is declared at the same object level as additionalProperties If the object schema that contains additionalProperties: false also has a complete properties/patternProperties coverage for all allowed keys (without relying on additional keys being introduced/validated only inside oneOf/anyOf branches), then additionalProperties: false is safe because there are no “unknown” keys from the viewpoint of that immediate object schema [2][5]. 2) Safe pattern: avoid “branch-specific allowed keys” at that object level If each oneOf/anyOf branch constrains only the values of already-declared keys (i.e., keys are present in the root properties/patternProperties set), then additionalProperties: false will not suffer from missing coverage due to composition boundaries [2][5]. 3) Unsafe pattern (common cause): “inherit” or “introduce” allowed properties only inside oneOf/anyOf/allOf branches If you use composition to modularize object shapes (e.g., $ref + allOf or oneOf branches that each add their own properties) while keeping additionalProperties: false in a surrounding schema, you can get failures because additionalProperties does not see that those property names were validated inside the composition branches; this “inheritance”/modularity problem is a known behavior and unevaluatedProperties is proposed as the fix specifically because it can see through composition/applicators and $ref [3][4]. In short: - additionalProperties: false is not intrinsically tied to allOf vs oneOf/anyOf; the failure mode occurs when allowed property names are only “known” within composition branches but not by the immediate object-level properties/patternProperties that additionalProperties evaluates against [2][3][4]. - unevaluatedProperties: false is the 2020-12 construct designed to be safe with “inheritance”-style modular schemas using allOf/anyOf/oneOf and $ref, because it can see through those applicators [3][1]. If you want “strict object keys” across oneOf/anyOf/allOf branches with refs, prefer unevaluatedProperties: false (possibly combined with keeping properties/patternProperties for truly unconditional constraints), rather than relying on additionalProperties: false in a parent schema that expects branch-introduced properties to count as evaluated [1][3].
Citations:
- 1: https://stackoverflow.com/questions/79214721/json-schema-anyof-and-additionalproperties-false-failing-validation
- 2: https://www.learnjsonschema.com/2020-12/applicator/additionalproperties/
- 3: "unevaluatedProperties" to facilitate re-use and better schema organization json-schema-org/json-schema-spec#556
- 4: Example of problems with "inheritance" and additionalProperties: false json-schema-org/json-schema-spec#661
- 5: https://github.com/sourcemeta/learnjsonschema.com/blob/main/content/2020-12/applicator/properties.markdown
Narrow the additionalProperties: false MUST-NOT scope for oneOf/anyOf
In JSON Schema 2020-12, the problematic behavior is that additionalProperties: false is evaluated only against the immediate object schema’s declared properties/patternProperties, and it does not “see through” composition/applicators and $ref the way unevaluatedProperties can. As a result, additionalProperties: false with oneOf/anyOf is not inherently unsafe; it’s safe when the set of allowed property names is fully declared at that same object level (and composition branches only constrain values of those already-declared keys).
Update the rule at adr/0001-additional-properties-and-sealed.md lines 220-223 to prohibit only the unsafe pattern where the allowed/validated property names are introduced or become “known” only inside oneOf/anyOf/allOf branches (or via $ref) rather than being declared in the object that contains additionalProperties: false—or explicitly recommend using unevaluatedProperties: false for modular/composed schemas.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adr/0001-additional-properties-and-sealed.md` around lines 220 - 223, Update
the rule for `additionalProperties: false` in
adr/0001-additional-properties-and-sealed.md to narrow the disallowed cases:
forbid only when the set of allowed property names is introduced or becomes
known inside composition branches or `$ref` (i.e., when the object with
`additionalProperties: false` does not itself declare the complete set of
`properties`/`patternProperties`), allow `additionalProperties: false` when all
property names are declared at that same object level even if
`oneOf`/`anyOf`/`allOf` are used to constrain values, and add an explicit
recommendation to prefer `unevaluatedProperties: false` for modular/composed
schemas.
| - **OP#12 enforcement:** When registering a derived schema, for every object position where any schema in the inheritance chain declares `"x-gts-sealed": true`, the merged set of property keys contributed by the derived schema at that position MUST be a subset of the property keys present in the chain up to that point. New keys cause registration to fail. | ||
| - **Refinement still allowed:** Derived schemas MAY narrow constraints on existing properties (e.g., reduce `maxLength`, narrow `enum`, increase `minimum`), MAY add `required`, MAY refine previously-open nested objects (recursively, subject to their own `x-gts-sealed` annotations). |
There was a problem hiding this comment.
Resolve patternProperties semantics before declaring OP#12 enforcement normative.
Line 232 defines sealed checks only as properties key-subset, while lines 445–446 leave patternProperties as open. That leaves conformance behavior under-specified for sealed objects using pattern-based keys.
Suggested clarification
- the merged set of property keys ... MUST be a subset ...
+ the merged set of declared keys (`properties`, and `patternProperties` if present) ... MUST be a subset ...Or explicitly state in this ADR that patternProperties is excluded for now and deferred to a follow-up ADR, to avoid diverging implementations.
Also applies to: 445-446
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adr/0001-additional-properties-and-sealed.md` around lines 232 - 233, The
OP#12 enforcement text for "x-gts-sealed" currently only references the
"properties" key and leaves "patternProperties" ambiguous; update the ADR text
around OP#12 (the paragraphs discussing "OP#12 enforcement" and the later lines
about patternProperties) to either (a) explicitly include patternProperties in
the sealed check (e.g., state that keys matched by patternProperties are treated
as sealed and derived schemas may not introduce new pattern-matching keys), or
(b) explicitly exclude patternProperties from OP#12 and note that pattern-based
key semantics are deferred to a follow-up ADR; reference the "x-gts-sealed" /
"OP#12 enforcement" paragraphs and the later "patternProperties" mention when
making this clarification so implementations have an unambiguous normative
decision.
…aled - Document three concrete problems with the current spec's overloaded use of additionalProperties: false — allOf-applicator footguns, forward-compatibility break, and missing middle ground between fully-open derivation and x-gts-final - Connect the closed content model to the tolerant reader principle (Postel's law), with pipeline-enrichment, vendor-extension, and producer-side-optionality scenarios beyond pure version evolution - Propose x-gts-sealed as a per-schema-object derivation-control modifier enforced by OP#12; demote additionalProperties: false to its native JSON Schema instance-validation role - Capture considered options with explicit per-option mapping of which problems they address and worked code demonstrations, plus impact on OP#6/OP#12 and reference implementations - Place the ADR at repo-root adr/ for visibility
aviator5
force-pushed
the
additional-properties-adr
branch
from
33631a0 to
7137c8d
Compare
1 participant
Summary by CodeRabbit