[openapi3] emit additionalProperties for a declared Record indexer in 3.1 - #11954
Open
Zach Bimson (bimsonz) wants to merge 1 commit into
Open
[openapi3] emit additionalProperties for a declared Record indexer in 3.1#11954Zach Bimson (bimsonz) wants to merge 1 commit into
Zach Bimson (bimsonz) wants to merge 1 commit into
Conversation
… 3.1 A declared `Record<T>` indexer describes a dictionary. Sealing, and a model that declares an indexer while also extending another model, both have to account for the properties evaluated by the `allOf` subschema holding the base model, so those keep `unevaluatedProperties`. Everything else now emits `additionalProperties`. Measured with Ajv 2020-12: in the position this changes the two keywords accept and reject the same instances; in the position it excludes they differ, and `additionalProperties` would make the schema unsatisfiable. A dictionary that does not compose now emits the same schema in 3.0 and 3.1, which the new cross-version tests use as an oracle, with the two exclusions pinned in the other direction.
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates OpenAPI 3.1/3.2 emission so non-composing Record<T> schemas use converter-compatible additionalProperties.
Changes:
- Selects
additionalPropertiesorunevaluatedPropertiesbased on sealing and inheritance. - Adds extensive cross-version, inheritance, sealing, nullable, and versioning tests.
- Updates documentation and changelog metadata.
File summaries
| File | Description |
|---|---|
| website/src/content/docs/docs/getting-started/typespec-for-openapi-dev.md | Updated as part of this pull request. |
| packages/openapi3/test/works-for.ts | Updated as part of this pull request. |
| packages/openapi3/test/versioning.test.ts | Updated as part of this pull request. |
| packages/openapi3/test/test-host.ts | Updated as part of this pull request. |
| packages/openapi3/test/return-types.test.ts | Updated as part of this pull request. |
| packages/openapi3/test/record.test.ts | Updated as part of this pull request. |
| packages/openapi3/test/nullable-properties.test.ts | Updated as part of this pull request. |
| packages/openapi3/test/additional-properties.test.ts | Updated as part of this pull request. |
| packages/openapi3/src/schema-emitter-3-1.ts | Updated as part of this pull request. |
| .chronus/changes/openapi3-record-additional-properties-2026-8-15-19-45-0.md | Updated as part of this pull request. |
Review details
Suppressed comments (3)
.chronus/changes/openapi3-record-additional-properties-2026-8-15-19-45-0.md:2
featureis not the appropriate change kind for this patch: it corrects existingRecord<T>emission for a supported input rather than adding a new capability. Please classify it asfix; that also avoids the feature-only requirement for an illustrative code block.
changeKind: feature
website/src/content/docs/docs/getting-started/typespec-for-openapi-dev.md:539
- This note now says that sealed schemas in OpenAPI 3.1/3.2 use
unevaluatedProperties, but theRecord<never>example immediately below still presentsadditionalPropertiesas the result; that example is wrong for the versions described here. Please qualify the examples as OpenAPI 3.0 or show the version-specific output.
**Note:** when emitting Open API 3.1 and 3.2 specs, `unevaluatedProperties` is used instead of `additionalProperties` for schemas that compose with `allOf`: sealed schemas, and models that declare a `Record` indexer while also extending another model. `additionalProperties` cannot see the properties evaluated by the `allOf` subschema, so it would constrain the inherited ones.
website/src/content/docs/docs/getting-started/typespec-for-openapi-dev.md:539
- This wording is inaccurate for a leaf sealed schema:
seal-object-schemas: trueemitsunevaluatedProperties: { not: {} }even when the schema has noallOf(as covered inpackages/openapi3/test/additional-properties.test.ts:271-280). Separate the sealing case from theallOfrationale so the docs do not imply that composition is required.
**Note:** when emitting Open API 3.1 and 3.2 specs, `unevaluatedProperties` is used instead of `additionalProperties` for schemas that compose with `allOf`: sealed schemas, and models that declare a `Record` indexer while also extending another model. `additionalProperties` cannot see the properties evaluated by the `allOf` subschema, so it would constrain the inherited ones.
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+172
to
+173
| const indexerKeyword = | ||
| shouldSeal || model.baseModel ? "unevaluatedProperties" : "additionalProperties"; |
| - "@typespec/openapi3" | ||
| --- | ||
|
|
||
| OpenAPI 3.1 and 3.2 now emit `additionalProperties` for a `Record<T>` indexer, unless the model also extends another model or the schema is sealed, which still use `unevaluatedProperties`. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
For OpenAPI 3.1 and 3.2, a declared
Record<T>indexer is emitted asadditionalPropertiesinsteadof
unevaluatedProperties, unless the model also extends another model, or the schema is sealed.Those two keep
unevaluatedProperties, because both have to account for the properties evaluated bythe sibling
allOfsubschema.model.baseModelis the same conditionmodelDeclarationuses to attach theallOf, so the guardand the thing it guards against are decided by the same fact.
3.0 is untouched:
schema-emitter-3-0.tshas noapplyModelIndexerand uses the baseimplementation, which already writes
additionalProperties. 3.2 is affected, becauseschema-emitter-3-2.tsextendsOpenAPI31SchemaEmitterwithout overriding the method, and thetests run against both.
Note
Record<never>is unaffected:shouldSealSchemareturns true for it, so it takes the sealingbranch and keeps
unevaluatedProperties: { not: {} }.Why: this repo cannot read its own 3.1 output
tsp-openapi3, the converter in this package, reads onlyadditionalProperties. Of the 36.tsfiles under
packages/openapi3/src/cli, zero mentionunevaluatedPropertiesand four handleadditionalProperties(convert/generators/generate-model.ts:359,convert/generators/generate-types.ts:482,convert/transforms/transform-component-schemas.ts:129,convert/interfaces.ts).So compiling
model Labels is Record<string>;at 3.1 and converting the result back givesmodel Labels {}. At 3.0 it givesmodel Labels { ...Record<string>; }. The round trip loses everydictionary, and exits 0 while doing it. I have filed that separately, since it stands on its own
whichever way this PR goes.
Why not just keep
unevaluatedPropertiesI am not claiming the current behaviour is accidental. #5961 chose it on purpose and said why:
That rationale is about composition, and it is correct. The keyword was then applied to every
declared indexer, including models that compose with nothing. This narrows it to the cases the
rationale covers.
Is it validation-equivalent where it changes?
Ajv 8.20.0 on the 2020-12 dialect, the same schema pair under each keyword, in every position this
emitter can put it:
$refd inside a derived model'sallOfanyOfbranchallOf(the case this excludes)The last row is why the guard exists. With
model Ledger extends Base { ...Record<string>; }andBasedeclaringid: int32,additionalPropertiesconstrains the inheritedidand the schemarejects
{"id": 1}outright.The second row is why the guard is
model.baseModeland not also aderivedModelscheck.Annotations flow bottom-up, so a dictionary in branch position inside someone else's
allOfbehavesidentically under both keywords. A
derivedModelsguard would also have been non-local, becauseRecord<T>is a cached template instantiation shared by every use site in a program.Coverage, and how strong it is
Because the two keywords are equivalent for a non-composing, non-
neverdictionary, the 3.0 documentis an oracle for the 3.1 one: the two spec versions must produce identical schemas.
record.test.tsasserts that, and asserts the two exclusions (composing models, and
Record<never>) in the otherdirection, so the oracle's preconditions are pinned rather than assumed.
14 tests added. I checked they are not vacuous by mutating the production code three ways and
recording which arms go red:
unevaluatedProperties(full revert)|| model.baseModelarmadditionalPropertiesEvery one of the 14 is killed by at least one. An earlier draft of this branch had a test asserting
that plain arrays are unaffected; it survived all three mutants, because arrays route through
arrayDeclaration/arrayLiteraland never reachapplyModelIndexerat all. I removed it ratherthan keep a test that cannot fail.
Shapes covered: declared indexer alone, with a base model, and as the base of other models; sealed
leaf and sealed with a base;
Record<never>; a three-level chain where two levels declare anindexer; arrays of dictionaries; nested dictionaries; cross-version parity in both directions; and
versioning, where the emitter mutates and clones the model graph, so a clone that lost its
baseModelwould silently change the keyword between two versions of the same API.openApiForVersionscould not pass emitter options, so it takes them now.Verified and unchanged, so not given new tests: merge-patch, multipart, discriminated unions, XML,
@visibilityvariants, and circular models.Blast radius
packages/openapi3: 2602 tests before, 2626 after.pnpm regen-samplesandpnpm regen-specsboth produce zero diff, but I want to be straight aboutwhat that is worth: not much. All 30 checked-in sample outputs are
openapi: 3.0.0and no samplesets
openapi-versions, so the 3.1 emitter is not exercised by them; andregen-specsrecords theconverter snapshots, which are the opposite direction. There is no checked-in 3.1 golden output in
the repo, so the unit tests above are the real coverage. The upside is that this change causes no
golden-file churn for reviewers to read.
Worth noting that
tsp initdoes offer an "OpenAPI 3.1 document" template, so zero golden-file churnis not the same as zero user impact.
One case where this narrows behaviour
@extensionwrites arbitrary keys straight onto the schema, afterapplyModelIndexerhas run. Thex-prefix convention is documented for@extensionbut not enforced, so a spec can inject anin-place applicator that the keyword decision structurally cannot see:
The injected
allOfnow sits besideadditionalProperties, and an instance{"flag": true}thatvalidated before does not after. This also reproduces via
@extensionon a property.I have pinned the current output in a test rather than guarded it, because the guard would have to
run before the extension is attached and cannot. If you would prefer
applyModelIndexerto consultgetExtensionsfor applicator keys, say so and I will add it, but it would only cover the modelpath and not the property one, which is why I did not do it unilaterally.
Why
packages/json-schemais not in this PR#5961 changed both emitters in one changeset, so leaving one behind is a fair thing to ask about. I
looked, and a mechanical port would make things worse rather than better:
Record<never>throughshouldSealSchema/isNeverType.json-schemahas nosuch check and reaches
{ not: {} }through the indexer path instead. Applying the same guardthere would flip
Record<never>toadditionalPropertiesinjson-schemawhile openapi3 keepsunevaluatedProperties, creating a divergence rather than removing one.json-schemadecides itsallOfwithmodel.baseModel && !shouldInlineBase, where the secondterm covers discriminated-union inlining that openapi3 does not have.
So aligning the two is a decision about both emitters and I would rather you made it than have me
assume it. Happy to extend this PR, or follow up separately, whichever you prefer.
Notes
feature, matching OpenAPI/JsonSchema - use unevaluatedProperties instead of additionalProperties and support setting to false #5961, which shipped the original change that way.openapi-typescriptalso does not implementunevaluatedPropertiesand renders a dictionary asRecord<string, never>, discarding the declared value type. That is tracked on their side asSupport for
unevaluatedPropertiesopenapi-ts/openapi-typescript#2839 and is a real gap there, not only here. I mention it as evidenceabout ecosystem support rather than as a defect report against this repo.
and I will rework it. The diff is one method, so either is cheap.