Skip to content

fix: valid specs lose data, duplicate keys, or fail to compile - #47

Merged
giraffesyo merged 2 commits into
canaryfrom
fix/review-followups
Aug 5, 2026
Merged

fix: valid specs lose data, duplicate keys, or fail to compile#47
giraffesyo merged 2 commits into
canaryfrom
fix/review-followups

Conversation

@giraffesyo

Copy link
Copy Markdown
Member

Follow-up review of #36, #40, and #41. Seven defects in shipped behavior, four of them data-losing or compile-breaking.

Regression introduced by #41

A discriminator with no explicit mapping now silently loses every payload. The analyzer only fills Discriminator.Mapping from an explicit mapping key, so an implicit-mapping union generated a switch with zero cases and only a default. Before #41 that default returned an error — loud and wrong. After #41 it returns nil with Value == nil, so {"kind":"cat","lives":9} decodes "successfully" into nothing.

The mapping is now derived from the variant schema names, which is what OpenAPI says the implicit values are. This is the one finding that is strictly worse than before my change, and it's the reason this PR exists.

Data loss and corruption in the catch-all (from #40)

additionalProperties: false was treated as true. resolveAdditionalPropertiesType never checked ap.B, and convertObject only tested != nil. A schema that explicitly forbids unknown properties got a catch-all field plus marshalers that collected and re-emitted them. New allowsAdditionalProperties is used at all three sites; type SealedEmpty struct{} still compiles.

Declared keys were deleted by exact match, but encoding/json matches tags case-insensitively. Schema {a: string, additionalProperties: true} receiving {"A":"hello"}: the stdlib fills A into the declared field via its case-insensitive fallback, delete(obj, "a") doesn't remove "A", so it also lands in the catch-all — and re-marshals as {"A":"hello","a":"hello"}. One logical property, duplicated on the wire. Both marshalers now share a generated deleteDeclaredProperties helper using strings.EqualFold.

*t = T(s) discarded fields the payload omitted. json.Unmarshal into a non-zero value merges for every generated struct except the ones that got custom marshalers, which silently replaced the whole value and kept stale catch-all entries. Now decodes through (*shadow)(t) after clearing the map — stdlib semantics, and one struct copy fewer.

Compile break (from #40)

A schema with a property named additionalProperties emitted two fields of that name: types.go:100:2: AdditionalProperties redeclared, plus five more errors in the marshalers. catchAllFieldName now picks AdditionalProperties2 when the name is taken.

Correctness and quality

  • null and missing-discriminator payloads fell into the unknown-variant branch, so a caller couldn't distinguish "server added a variant" from "malformed body". null is now a no-op and a missing discriminator is an error (the case "" arm is suppressed when a mapping legitimately maps "").
  • Every response shared one name hint, so two inline union bodies on one operation collided into DoThingResponse2 — and inserting a 201 later would rename the 400 type. Non-success bodies now carry their status code; DoThingResponse2 became DoThingResponse400.
  • MarshalJSON made three JSON passes (marshal → unmarshal to map → marshal) and reordered declared properties alphabetically as a side effect. Now splices the two encoded objects: one pass fewer, struct field order preserved.
  • The new e2e test re-implemented scaffolding that generateFromSpec/runGeneratedWireTest already provide; rewritten on top of them (~60 duplicated lines gone). Four WHAT comments on unexported template helpers removed per CLAUDE.md.

Deliberately not fixed

Tests

Runtime e2e coverage added for the case-insensitive leak, catch-all reset, and additionalProperties: false; the inline-union e2e extended with missing-discriminator and null cases. gofmt, go vet, and go test ./... pass.

Provenance

Produced by /code-review xhigh --fix over 4c237be..ff8808c. The fixes were extracted onto a clean branch off canary because a concurrent session had unrelated in-flight work (non-JSON request bodies, enum naming, nullable-union collapsing) interleaved in the same files; none of it is included here.

Follow-up review of #36, #40, and #41 found seven defects in the shipped
behavior.

A discriminator declared without an explicit mapping generated a switch with no
cases. #41 turned that from a loud error into silent loss: every payload became
an unknown variant with a nil Value. The mapping is now derived from the variant
schema names, which is what the spec says the values are.

`additionalProperties: false` was treated as `true`, so a schema that forbids
unknown properties got a catch-all field and marshalers that collected and
re-emitted them.

A schema with a property actually named `additionalProperties` emitted two
fields of that name and the generated package did not compile.

encoding/json matches tags case-insensitively, but the catch-all marshalers
deleted declared keys by exact match, so `{"A":1}` against a declared `a` landed
in both and re-marshaled as two keys.

UnmarshalJSON replaced the whole struct, discarding fields the payload omitted
and leaving stale catch-all entries; it now decodes through the shadow type
after clearing the map, matching stdlib merge semantics.

MarshalJSON made three JSON passes and reordered declared properties
alphabetically; it now splices the two encoded objects.

A null payload, or one missing the discriminator entirely, fell into the
unknown-variant branch instead of erroring, so callers could not tell a new
server variant from a malformed body.

Responses also shared one name hint per operation, so two inline union bodies
collided into <Op>Response2; non-success bodies now carry their status code.
@giraffesyo
giraffesyo merged commit 99ef133 into canary Aug 5, 2026
7 checks passed
@giraffesyo
giraffesyo deleted the fix/review-followups branch August 5, 2026 00:30
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.

1 participant