Skip to content

[openapi3] emit additionalProperties for a declared Record indexer in 3.1 - #11954

Open
Zach Bimson (bimsonz) wants to merge 1 commit into
microsoft:mainfrom
bimsonz:openapi3-record-additional-properties
Open

[openapi3] emit additionalProperties for a declared Record indexer in 3.1#11954
Zach Bimson (bimsonz) wants to merge 1 commit into
microsoft:mainfrom
bimsonz:openapi3-record-additional-properties

Conversation

@bimsonz

Copy link
Copy Markdown

Follow-up to #11953, which I closed as prematurely filed and wrongly framed as a bug. This is the
properly scoped version: it concedes #5961's design, narrows the claim to the case where the two
keywords are provably equivalent, and leaves sealing and composition alone.

What this changes

For OpenAPI 3.1 and 3.2, a declared Record<T> indexer is emitted as additionalProperties instead
of 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 by
the sibling allOf subschema.

- const unevaluatedPropertiesSchema = shouldSeal
+ const indexerSchema = shouldSeal
    ? { not: {} }
    : this.emitter.emitTypeReference(model.indexer!.value);
- setProperty(schema, "unevaluatedProperties", unevaluatedPropertiesSchema);
+ const indexerKeyword =
+   shouldSeal || model.baseModel ? "unevaluatedProperties" : "additionalProperties";
+ setProperty(schema, indexerKeyword, indexerSchema);

model.baseModel is the same condition modelDeclaration uses to attach the allOf, so the guard
and the thing it guards against are decided by the same fact.

3.0 is untouched: schema-emitter-3-0.ts has no applyModelIndexer and uses the base
implementation, which already writes additionalProperties. 3.2 is affected, because
schema-emitter-3-2.ts extends OpenAPI31SchemaEmitter without overriding the method, and the
tests run against both.

Note Record<never> is unaffected: shouldSealSchema returns true for it, so it takes the sealing
branch and keeps unevaluatedProperties: { not: {} }.

Why: this repo cannot read its own 3.1 output

tsp-openapi3, the converter in this package, reads only additionalProperties. Of the 36 .ts
files under packages/openapi3/src/cli, zero mention unevaluatedProperties and four handle
additionalProperties (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 gives
model Labels {}. At 3.0 it gives model Labels { ...Record<string>; }. The round trip loses every
dictionary, 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 unevaluatedProperties

I am not claiming the current behaviour is accidental. #5961 chose it on purpose and said why:

One key difference from additionalProperties though is that it evaluates properties after any
in-place applicators. [...] This is particularly useful when trying to set additionalProperties
to false on a schema that has sub-schemas.

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:

position result
leaf dictionary, standalone (the case this changes) identical
dictionary $refd inside a derived model's allOf identical
dictionary inside a nullable anyOf branch identical
keyword as a sibling of an allOf (the case this excludes) different

The last row is why the guard exists. With model Ledger extends Base { ...Record<string>; } and
Base declaring id: int32, additionalProperties constrains the inherited id and the schema
rejects {"id": 1} outright.

The second row is why the guard is model.baseModel and not also a derivedModels check.
Annotations flow bottom-up, so a dictionary in branch position inside someone else's allOf behaves
identically under both keywords. A derivedModels guard would also have been non-local, because
Record<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-never dictionary, the 3.0 document
is an oracle for the 3.1 one: the two spec versions must produce identical schemas. record.test.ts
asserts that, and asserts the two exclusions (composing models, and Record<never>) in the other
direction, 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:

mutant arms killed
always unevaluatedProperties (full revert) 8 of 14
drop the || model.baseModel arm 3 of 14
always additionalProperties 7 of 14

Every 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/arrayLiteral and never reach applyModelIndexer at all. I removed it rather
than 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 an
indexer; 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
baseModel would silently change the keyword between two versions of the same API.
openApiForVersions could not pass emitter options, so it takes them now.

Verified and unchanged, so not given new tests: merge-patch, multipart, discriminated unions, XML,
@visibility variants, and circular models.

Blast radius

packages/openapi3: 2602 tests before, 2626 after.

pnpm regen-samples and pnpm regen-specs both produce zero diff, but I want to be straight about
what that is worth: not much. All 30 checked-in sample outputs are openapi: 3.0.0 and no sample
sets openapi-versions, so the 3.1 emitter is not exercised by them; and regen-specs records the
converter 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 init does offer an "OpenAPI 3.1 document" template, so zero golden-file churn
is not the same as zero user impact.

One case where this narrows behaviour

@extension writes arbitrary keys straight onto the schema, after applyModelIndexer has run. The
x- prefix convention is documented for @extension but not enforced, so a spec can inject an
in-place applicator that the keyword decision structurally cannot see:

@extension("allOf", #[#{ type: "object", properties: #{ flag: #{ type: "boolean" } } }])
model Dict { ...Record<string>; }

The injected allOf now sits beside additionalProperties, and an instance {"flag": true} that
validated before does not after. This also reproduces via @extension on 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 applyModelIndexer to consult
getExtensions for applicator keys, say so and I will add it, but it would only cover the model
path and not the property one, which is why I did not do it unilaterally.

Why packages/json-schema is 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:

  • openapi3 classifies Record<never> through shouldSealSchema/isNeverType. json-schema has no
    such check and reaches { not: {} } through the indexer path instead. Applying the same guard
    there would flip Record<never> to additionalProperties in json-schema while openapi3 keeps
    unevaluatedProperties, creating a divergence rather than removing one.
  • json-schema decides its allOf with model.baseModel && !shouldInlineBase, where the second
    term 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

… 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

Copy link
Copy Markdown
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 additionalProperties or unevaluatedProperties based 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

  • feature is not the appropriate change kind for this patch: it corrects existing Record<T> emission for a supported input rather than adding a new capability. Please classify it as fix; 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 the Record<never> example immediately below still presents additionalProperties as 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: true emits unevaluatedProperties: { not: {} } even when the schema has no allOf (as covered in packages/openapi3/test/additional-properties.test.ts:271-280). Separate the sealing case from the allOf rationale 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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

emitter:openapi3 Issues for @typespec/openapi3 emitter meta:website TypeSpec.io updates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants