Skip to content

fix(compilers/openapi): stop silently dropping operation details - #272

Merged
OmarAlJarrah merged 8 commits into
mainfrom
fix/openapi-lossless-operation-sweep
Aug 6, 2026
Merged

fix(compilers/openapi): stop silently dropping operation details#272
OmarAlJarrah merged 8 commits into
mainfrom
fix/openapi-lossless-operation-sweep

Conversation

@OmarAlJarrah

@OmarAlJarrah OmarAlJarrah commented Aug 6, 2026

Copy link
Copy Markdown
Member

Summary

Six places in the operation, parameter and content lowering were reported as dropping declared
source content with no IR representation and no diagnostic. I reproduced each one against the
current tree before touching anything — compiling a minimal spec and reading both the emitted IR
and the complete diagnostic list — because the report predates the compiler split and several
losslessness fixes have landed since.

# Reported drop State on main before this change
1 Response-header metadata (description, deprecated, style/explode, examples, extensions, content-form headers) Partly handled. Everything except style and explode already reached ir.Property; those two were still dropped in silence.
2 Webhook and callback path-item servers Still dropped. The preserve-plus-diagnostic path was reached only from the paths walk; the other two routes produced neither an Unmodeled entry nor a diagnostic. Probing it also showed an Operation Object's own servers read nowhere at all — the overriding declaration of the same keyword — so both are covered here.
3 Parameter allowEmptyValue Still dropped. Read nowhere; a document declaring it compiled to IR identical to one that did not.
4 Reserved header parameters (Accept, Content-Type, Authorization) Still silent. Lowered as ordinary parameters with no marker of any kind — and the report names only the parameter position, while the specification states the same rule for a Content-Type entry in a response's and an encoding's headers map. All three are covered here.
5 Single-media-type error responses lose the media type Still dropped. With two media types the whole content map was kept; with one, only the schema survived as ErrorCase.Type and the media type went nowhere.
6 Sibling constraints beside a component-level $ref Already fixed, in bddc8da, with a committed regression test (TestComponentConstraints_RefSiblingKeepsThem). A component Bounded: {$ref: …, minimum: 5} interns with Constraints.Min == "5", matching what the property position does. No change here.

The five that still reproduced share one mechanism: declared content that reaches the IR in no form
at all and says nothing about it. Each is now either lowered, kept verbatim under Unmodeled with
ReasonNoIRHome and a degraded-construct diagnostic, or — for the one case where keeping it as
declared is the right answer — reported under a diagnostic code of its own.

1. Header style and explode. explode decides whether a collection-valued header goes on
the wire as one repeated field or one comma-joined value, so losing it left the IR unable to say
how the header serializes. ir.Property has a field for neither (ir.PartEncoding does, but that
is a multipart part's own config), so both are kept verbatim, the same way a parameter schema's
xml hints already are. This fixes both callers of the shared header lowering — response headers
and a multipart part's per-encoding headers — rather than only the response position it was noticed
at.

2. Path-item servers on every route. A path item is one object with three parents. The
existing applyPathServers is now called from the webhook walk and the callback walk as well as
from paths, so a delivery-host override written on a webhook or a callback is kept and reported
exactly as the same declaration under paths already was. No second helper was written.

2b. An operation's own servers, which override the path item's. OpenAPI lets both a Path
Item Object and an Operation Object declare servers and says the operation's override the path
item's. Only the path item's were read, so a document declaring both kept the superseded list
under openapi:servers and dropped the effective one outright: an emitter reading that entry would
route to a host the operation had replaced, and nothing reported it either way. Fixing item 2 alone
would have propagated that to two further routes.

The two are kept under separate keys — the operation's own under openapi:operationServers
because they are two declarations at two pointers, and one map key cannot hold both without the
surviving list depending on which lowering ran last, silently. The path item's keeps the key it
already ships under, so no golden changes meaning. Preservation happens in lowerOperation rather
than at each route, so no route added later can forget it, which is exactly how the path-item half
came to be missing on two of its three.

3. allowEmptyValue. Its neighbours — style, explode, allowReserved, the content-style
media type — all reach ir.HTTPParamBinding; this one has no field there and no reader anywhere.
It is kept on ir.Parameter, the carrier at this position that has an Unmodeled map at all.
Preservation keys off presence rather than truth: allowEmptyValue: false is a declared fact too,
and recording only the true spelling would be the compiler deciding which declarations count.

4. Reserved headers — an explicit, documented policy, at every position that states it.
OpenAPI says a reserved header declaration SHALL be ignored in three places, not one: a parameter
with in: header named Accept, Content-Type or Authorization (§4.8.12); a Content-Type
entry in a response's headers map (§4.8.17), whose media type the response's own content map
already names; and a Content-Type entry in an encoding's headers map (§4.8.15), which the
encoding's own contentType describes separately. The compiler now keeps lowering all three and
reports each under one new stable code, openapi/reserved-header-name, at warning severity.

Keeping the declaration is the deliberate choice: dropping declared content is a loss, and whether
to generate a header that collides with the security scheme, with content negotiation, or with the
media type the position already owns is an emitter's decision, not a compiler's. What was wrong was
making that deviation invisible — an emitter had no way to tell such a declaration from any other.
The comparison is case-insensitive, since HTTP field names are. The rationale is recorded on the
diagnostic code itself, which is where the next reader will reach it.

The two headers-map positions report at the map entry's own pointer rather than the header object's,
because the reserved thing is the key a header is mapped under: one component $ref'd from a
reserved key and from an ordinary one is two declarations, and only the reserved key is reported.
That is the opposite choice from the style/explode preservation above, which keeps keywords the
header object itself writes and so records them at the declaration.

The report is unconditional rather than an Options switch, and that too is recorded on the code.
Invariant 6 sends anything inferred to injectable policy, and nothing here is inferred: the names
are fixed by the specification, the comparison is against a declared name, and the lowered document
is identical whether or not the diagnostic fires — removing the call reddens only the diagnostic
assertions, no golden and no conformance IR. Warning rather than error because the document is
well-formed, and because harness.Check returns at the first error diagnostic, which would hide
every later finding in the same spec.

5. Error-response content, whatever its arity. ir.ErrorCase holds a single TypeRef and no
media type, so an error declared only as application/problem+json reached the IR indistinguishable
from one declared as application/json. One entry losing the key it was written under is the same
loss as several losing all but the first, so the content map is now kept in both cases, with the
diagnostic naming which of the two happened rather than one message covering both.

docs/ir-design.md's OpenAPI mapping row is updated for each of these, since it is normative about
where a construct lands.

Scope

Nothing from the report was left out. Item 6 needed no change and is recorded above with the
evidence rather than quietly omitted. None of the five fixes mints an IR node, so no new ID
namespace is involved; the two-order oracle in internal/harness sweeps the whole conformance
corpus and stays green over the extended fixtures.

Probing turned up four further drops in the same walk that are not part of this change:

Each is a separate rule from anything here, so each gets its own change rather than riding along.

Test plan

Fixtures were extended in the conformance corpus rather than added beside it, since none of these is
a new capability row: header-content-schema gains a collection header declaring style/explode,
webhooks and callbacks each gain a path-item servers override with a distinct URL,
param-styles gains an allowEmptyValue parameter, and per-status-errors already carried a
single-media error response. Unit tests in the operation package pin the pointers and diagnostic
messages the goldens only record. unwitnessed.golden.txt shrinks by one line: ErrorCase.Unmodeled
had no witness in the corpus before this.

webhooks additionally declares an operation-level servers override beside its path-item one, so
the corpus witnesses both keys of that pair and the two-order oracle, irverify and the JSON round
trip all reach the case where they could collide.

The reserved-header rule is unit-tested rather than added to the corpus, since it changes only the
diagnostic stream: the header lowers identically either way, so a fixture would move a golden
without witnessing the report. Both lowerHeaders callers are covered, and the two-keys-one-
component case pins which pointer the report lands on.

Every new behavioural assertion was checked by planting the defect back and watching it go red:

Mutation planted Went red
preserveHeaderSerialization iterates an empty keyword list TestConformance/header-content-schema, TestHeaders_SerializationKeywordsKept (both sub-cases)
…iterates style only, dropping explode same two, so the loop is not passing on style alone
applyPathServers call removed from the webhook walk TestConformance/webhooks, TestOperations_PathItemServersKeptOnEveryRoute
applyPathServers call removed from the callback walk TestConformance/callbacks, TestOperations_PathItemServersKeptOnEveryRoute
applyOperationServers call removed TestOperations_OwnServersKeptBesideThePathItems, TestOperations_OwnServersSurviveBesideExtensions
…writes the operation's list to the path item's openapi:servers key TestOperations_OwnServersKeptBesideThePathItems, TestOperations_ServersKeysAreIndependent, so the key collision is pinned rather than assumed
…preserves at the operation's pointer instead of the servers keyword's TestOperations_OwnServersKeptBesideThePathItems
…called before the extensions assignment that overwrites op.Unmodeled TestOperations_OwnServersSurviveBesideExtensions, which exists because this mutation survived the rest of the suite
…skipped on the webhook and callback routes TestConformance/webhooks, TestOperations_OwnServersKeptBesideThePathItems. Both this and the webhook-only and callback-only variants; the test covered one route until this mutation survived the whole suite
operation servers deleted from webhooks.yaml TestConformance/webhooks, confirming that fixture addition is load-bearing rather than inert
fillErrorType guarded by content.Len() > 1 again TestGolden, TestConformance/per-status-errors, TestConformance/component-reuse, TestConformance_UnwitnessedIRFields, TestErrorCase_SingleMediaTypeKeepsContentMap, TestDiag_SharedDeclarationReportsEachDefectOnce
preserveAllowEmptyValue call removed TestConformance/param-styles, TestParams_AllowEmptyValueKept
reservedHeaderParamDiag call removed all four reported cases of TestParams_ReservedHeaderNamesAreReported
reservedHeaderParamDiag reports unconditionally (no location guard, no name match) the two unreported cases of the same test, so it guards over-reporting as well as under-reporting
reservedHeaderEntryDiag call removed from lowerHeaders TestHeaders_ReservedContentTypeEntryIsReported, TestHeaders_ReservedContentTypeInEncodingIsReported, TestHeaders_ReservedNameIsTheKeyNotTheDeclaration
reservedHeaderEntryDiag reports every entry (no name guard) the unreported cases of the same three, so it guards over-reporting too
…compares with == rather than strings.EqualFold the lowercase-spelling case, so case-insensitivity is asserted rather than assumed
…reports at the header's declaration instead of the map key TestHeaders_ReservedNameIsTheKeyNotTheDeclaration, so the two-keys-one-component case is pinned
servers deleted from webhooks.yaml TestConformance/webhooks, confirming the fixture addition is load-bearing rather than inert

Full gate, in order: gofmt -l clean, go vet ./... clean, golangci-lint run 0 issues,
go build ./... clean, ./scripts/check-coverage.sh passes at 100% of statements.
go run ./cmd/morphic-harness over both testdata/conformance/openapi and testdata/golden/openapi
reports ok for every spec, which is what exercises the two-order diff, irverify and the JSON
round-trip over the extended fixtures.

Closes #39.

…licy

The reserved-header-parameter code records why the compiler keeps such a
parameter and reports it, but not why the report is unconditional. Invariant 6
sends anything inferred to an injectable policy that can be disabled, so the
next reader reaching this code has to work out for themselves whether the
omission is deliberate.

It is: the three names are fixed by the specification rather than inferred from
the document, the comparison is against a declared name, and the lowered
document is identical whether or not the diagnostic fires — removing the call
reddens only the diagnostic assertions, no golden or conformance IR. There is
no inference to mark Inferred and no semantics to switch off. The severity
choice is recorded alongside it, since error would both misstate a well-formed
document and stop harness.Check before any later finding in the same spec.
…operation-sweep

# Conflicts:
#	docs/ir-design.md
The reserved-header report landed at one of the three positions OpenAPI
states the rule. A header parameter named Accept, Content-Type or
Authorization was reported (§4.8.12), but a Content-Type entry in a
response's headers map (§4.8.17) or an encoding's (§4.8.15) still lowered
in silence — the same deviation from the same SHALL, at the two positions
the shared header lowering serves.

Report all three under one code, renamed openapi/reserved-header-name
since it is no longer parameter-specific. The headers-map half reports at
the map entry's own pointer rather than the header object's: the reserved
thing is the key a header is mapped under, so one component referenced
from a reserved key and an ordinary one is two declarations, and only the
reserved key is reported.
OpenAPI lets both a Path Item Object and an Operation Object declare servers,
and says the operation's override the path item's. Only the path item's were
read, so a document declaring both kept the superseded list under
openapi:servers and dropped the effective one outright — an emitter reading
that entry would route to a host the operation had replaced, with nothing
reported either way.

Keep the operation's own beside it under openapi:operationServers. They need
separate keys because they are two declarations at two pointers: one key for
both would make the surviving list depend on which lowering ran last, and do
it silently. The path item's keeps the key it already shipped under.

Preserved from lowerOperation rather than from each route, so no route added
later can forget it — which is how the path-item half came to be missing on
two of its three routes.
The operation-level servers test asserted one of the three routes that lower
an operation, while the code comment claimed lowerOperation makes all three
unmissable. Planting a guard that skipped the webhook and callback routes left
the whole suite green — the same blind spot the path-item half of this pair
was fixed out of.

Assert all three routes with a distinct URL apiece, and witness the pair in
the conformance corpus so the two-order oracle, irverify and the JSON round
trip reach it. Deleting the fixture's override reddens TestConformance.
@OmarAlJarrah
OmarAlJarrah merged commit 0ed7683 into main Aug 6, 2026
1 check passed
@OmarAlJarrah
OmarAlJarrah deleted the fix/openapi-lossless-operation-sweep branch August 6, 2026 12:05
Sign up for free to 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: several source details silently dropped, violating lossless-by-default

1 participant