fix: valid specs lose data, duplicate keys, or fail to compile - #47
Merged
Conversation
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.
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.
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
discriminatorwith no explicitmappingnow silently loses every payload. The analyzer only fillsDiscriminator.Mappingfrom an explicitmappingkey, so an implicit-mapping union generated aswitchwith zero cases and only adefault. Before #41 thatdefaultreturned an error — loud and wrong. After #41 it returns nil withValue == 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: falsewas treated astrue.resolveAdditionalPropertiesTypenever checkedap.B, andconvertObjectonly tested!= nil. A schema that explicitly forbids unknown properties got a catch-all field plus marshalers that collected and re-emitted them. NewallowsAdditionalPropertiesis used at all three sites;type SealedEmpty struct{}still compiles.Declared keys were deleted by exact match, but
encoding/jsonmatches tags case-insensitively. Schema{a: string, additionalProperties: true}receiving{"A":"hello"}: the stdlib fillsAinto 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 generateddeleteDeclaredPropertieshelper usingstrings.EqualFold.*t = T(s)discarded fields the payload omitted.json.Unmarshalinto 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
additionalPropertiesemitted two fields of that name:types.go:100:2: AdditionalProperties redeclared, plus five more errors in the marshalers.catchAllFieldNamenow picksAdditionalProperties2when the name is taken.Correctness and quality
nulland missing-discriminator payloads fell into the unknown-variant branch, so a caller couldn't distinguish "server added a variant" from "malformed body".nullis now a no-op and a missing discriminator is an error (thecase ""arm is suppressed when a mapping legitimately maps"").DoThingResponse2— and inserting a 201 later would rename the 400 type. Non-success bodies now carry their status code;DoThingResponse2becameDoThingResponse400.MarshalJSONmade 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.generateFromSpec/runGeneratedWireTestalready provide; rewritten on top of them (~60 duplicated lines gone). Four WHAT comments on unexported template helpers removed per CLAUDE.md.Deliberately not fixed
additionalPropertieswith a mismatched value fails the whole decode.{owner: string, additionalProperties: {type: string}}receiving{"count":3}errors out and loses the response — the opposite of the resilience fix: an unknown discriminator value no longer fails the whole decode #41 argues for. Whether to error or skip a spec-violating value is a product call; the case-insensitivity fix removes the common false trigger. Say the word and I'll make it skip.allOfstill ignoresadditionalPropertiesentirely.convertAllOfnever emitted a catch-all, so composed schemas drop unknown properties — fix: additionalProperties are dropped on decode and sent as a "-" key #40's bug, unfixed for that shape. The fix isn't just adding the field:type shadow Twould promote an embedded parent's marshalers and break encoding, anddeclaredJSONNamesskips embedded fields so promoted properties would leak into the catch-all. Needs the marshaler generalized to embedded types; larger than this PR.raw json.RawMessagein fix: an unknown discriminator value no longer fails the whole decode #41 meansshape == Shape{}stops compiling downstream. Inherent to preserving the payload — flagging it as a breaking-change note for the release, not a code fix, since fix: an unknown discriminator value no longer fails the whole decode #41 shipped asfix:.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, andgo test ./...pass.Provenance
Produced by
/code-review xhigh --fixover4c237be..ff8808c. The fixes were extracted onto a clean branch offcanarybecause 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.