Skip to content

fix(compilers/openapi): keep path-item docs and extra operations - #310

Merged
OmarAlJarrah merged 4 commits into
mainfrom
fix/openapi-path-item-losslessness
Aug 9, 2026
Merged

fix(compilers/openapi): keep path-item docs and extra operations#310
OmarAlJarrah merged 4 commits into
mainfrom
fix/openapi-path-item-losslessness

Conversation

@OmarAlJarrah

@OmarAlJarrahOmarAlJarrah commented Aug 9, 2026

Copy link
Copy Markdown
Member

Summary

The Path Item Object loses more of itself than any other object in the OpenAPI compiler.
lowerPathItem reads the fixed method fields, parameters, servers and $ref, and nothing
else, so three of the object's remaining fields reached the IR in no form at all — no field, no
Unmodeled entry, and no diagnostic on either channel:

  • summary / description.fillOperationDocs reads the Operation Object's own pair, so a
    path item's documentation was dropped outright.
  • additionalOperations (3.2). A map from an HTTP method with no fixed field of its own to a
    full Operation Object. Every operation declared there vanished whole — operationId,
    parameters, request body, responses — and with it every type reachable only through it, so the
    type registry came out short as well.
  • query (3.2). The fixed field 3.2 added for the QUERY method, missing from the accessor
    table the walk iterates. It is the same whole-operation loss as additionalOperations at the
    same site, so it is fixed here rather than left to reopen this function for one table entry.

The loss repeated at all three routes that reach a path item — paths, webhooks, and a callback
expression. Neither issue says so, and a fix scoped to lowerPathItem alone would have missed two
of the three.

Where the documentation lands

summary and description are now kept verbatim on each operation the path item holds, under
openapi:pathItemSummary / openapi:pathItemDescription with no_ir_home and one info
diagnostic, rather than merged into Docs.

There is no ir.PathItem — a path item is distributed across the operations it holds — so the two
candidate destinations were a merge into Operation.Docs or preservation beside it. ir.Docs
holds one summary and one description and they are the operation's own, while a path item's pair
documents the path. Merging would need a precedence rule against the operation's own, would attach
to an operation documentation its author never wrote (an inference, which belongs in injectable
policy rather than in a lowering), and would leave an emitter unable to tell the two subjects
apart afterwards. Preserving takes no position on precedence and loses nothing. The cost — the
pair is duplicated onto every operation under the path — is the cost path-item servers already
pays at the same site. The reasoning is recorded on applyPathItemDocs and in docs/ir-design.md,
and no_ir_home rather than a boundary reason because the IR could grow a home for it.

How the operations lower

additionalOperations entries become ordinary ir.Operations mounted at their own pointer
(…/additionalOperations/<method>), which is where the source writes them, so none takes an ID a
fixed method field could also claim. The map key becomes HTTPBinding.Methodverbatim: that
field is documented as the method as sent on the wire and OpenAPI reads a method name
case-sensitively, so the compiler neither upper-cases nor neutralizes it. A fixed field's name is
a field name rather than a method, so that one is still upper-cased into its wire spelling.

The three walks now share one pathOperations(pi) reader, and a path item's servers and
documentation are kept through one applyPathItemResidue call per route, so the next construct in
either class cannot reach two routes and miss the third.

Not in scope

A path item's x-* extensions are still dropped. That is a different mechanism — every object
that admits an extension and has no reader for it — and is tracked in #275, which covers the
dozen-odd objects sharing it rather than this one.

Two cases keep the documentation in no form, both shared with the servers half beside it
rather than introduced here. Each is called out on applyPathItemDocs:

One key that names no method

additionalOperations keys reach HTTPBinding.Method verbatim, which is what an empty key
needs reporting for: it binds a method no request can be sent with, and speakeasy accepts it
(it rejects only a key naming a standard method, which has a fixed field of its own). The
entry still lowers in full — dropping it would trade a reported defect for a silent one — and
openapi/invalid-method-key names the entry that declared it. The check sits in
lowerOperation, where all three routes funnel through.

Test plan

  • New tests in compilers/openapi/internal/operation: the documentation pair and an
    additionalOperations entry are each asserted at all three routes with distinct text and
    distinct operationIds apiece, so a fix that kept the enclosing path item's — or the same one
    three times — fails rather than passing on the shape alone. The query field, verbatim method
    casing, the interning of a type reachable only through a previously dropped operation, and a
    callback expression that declares no fixed method at all are asserted alongside.
  • Two conformance fixtures with goldens: path-item-docs (3.1) and path-item-operations (3.2),
    each covering the path, webhook and callback mounts in one document, so the corpus sweeps —
    irverify, the JSON round-trip, the determinism and two-order oracles — run over both.
  • Verified the corpus bites: deleting the path item's summary line from one fixture and the
    mixed-case additionalOperations entry from the other reddens TestConformance for both.
  • Verified the tests bite: with the change to operations.go reverted and the tests kept, the
    three new behavioural tests go red (postP keeps its path item's summary, operation "queryP" not found, operation "purgeCallback" not found) and both conformance rows fail.
  • Full gate green: gofmt, go vet, golangci-lint, go build, and the coverage gate at
    exactly 100%. No existing golden moved.

Closes#292
Closes#293

Fixes the mounted half of #383, which spans a path item whether or not it mounts an
operation. The unmounted half is untouched — there is no operation to carry the entry —
so #383 stays open for the unmounted-path-item work rather than being closed here.

The key becomes ir.HTTPBinding.Method verbatim, which is right for a method name
OpenAPI reads case-sensitively, and is what lets an empty key through as an empty
method — a binding no request can be sent with. speakeasy rejects a key naming a
standard method, since that method has a fixed field of its own, but accepts this
one, so nothing upstream reports it.
The entry still lowers. Dropping it would trade a reported defect for a silent
one, which is the trade this change exists to undo, so the operation and
everything it declares survive and a warning names the entry that declared it.
The check sits in lowerOperation because all three routes funnel through it, so a
fourth would be covered without being remembered. A fixed field's method is a
field name upper-cased and can never be empty.
Two cases keep nothing and report nothing, and both are shared with the servers
half beside them rather than introduced by the documentation one.
A path item that mounts no operation has no operation to carry the entry, so its
pair is lost whole; #383 records that gap for the entire class and the
unmounted-path-item work is where it lands. And RawChildNode scans a mapping's
pairs directly, so a pair arriving through a YAML merge key or an alias is read
by the model and not by the lookup: GetSummary is non-empty, the raw node is nil,
PreserveNode writes nothing and returns no diagnostic. That is #384, filed
against the lookup rather than this caller because 31 non-test sites share it.
TestApplyPathItemDocs_WithoutRootNode reads as a guard against an input the
parser never produces. It is not — a merge-key document reaches that branch
carrying real text — so it now says which behaviour it pins and why.
The OpenAPI row names a warning beside the lowering that emits it wherever one
exists — a reserved header name is "lowered as declared + reserved-header-name
warning". The additionalOperations clause added with this change described the
lowering and stopped there, so the one key that lowers to an unusable binding
read as though it lowered silently.
@OmarAlJarrah
OmarAlJarrah merged commit dfebbf0 into mainAug 9, 2026
1 check passed
@OmarAlJarrah
OmarAlJarrah deleted the fix/openapi-path-item-losslessness branch August 9, 2026 08:16
OmarAlJarrah added a commit that referenced this pull request Aug 9, 2026
The conflict this branch predicted, resolved as it said: #304's
TestDetectCycles_EmptyPointerSegmentIsRefused calls Cycles(0, []byte(src)), and
Cycles now takes a sourceindex.Index, so the call becomes scanBytes(t, ...).
Two the branch could not predict. cycles_test.go: #304 and #310 added a test
where this branch adds a scanIndex helper, both at the same offset sharing a
closing brace — kept both. And #328 consolidated the openapi test scaffolding
into internal/openapitest after this branch was written, so its new
entry_internal_test.go calls sourceOf by the package-local name that no longer
exists; repointed, with the import added.
Byte-identical output re-proven against the merged tree rather than carried over
from the branch: 115 sources under testdata compiled through binaries built from
main and from this merge, capturing document, stderr and exit code each. diff -r
over the two trees is empty.
OmarAlJarrah added a commit that referenced this pull request Aug 13, 2026
The branch forked before #310, #349 and #356 landed, so three conflicts
needed resolving rather than taking a side:
- census() keeps main's provenance-aware unrecorded() filter, its keep()
helper and its filter-before-bound order, under this branch's
(keys, root) signature. Taking the branch's body would have dropped the
collision and unreachable-key reports and spent budget slots on keys
another reader had already kept.
- applyPathItem() absorbs applyPathItemDocs() rather than replacing
applyPathItemResidue() outright, so a path item's summary and
description are still kept on each operation it mounts (#292/#310).
carrier.keepsDocs marks where that pair has a home: only an operation,
since pathItemDocFields keys by bare keyword and one service holds
every unmounted item. #383 keeps the unmounted half.
- lowerPaths/lowerWebhooks keep main's ctx cancellation and
pathOperations walk beside this branch's svc carrier.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

openapi: 3.2 additionalOperations are dropped entirely and silently openapi: path-item summary and description reach the IR nowhere

1 participant

@OmarAlJarrah