fix: embedded schema's additionalProperties swallows the outer schema's declared fields - #53
Merged
Merged
Conversation
…'s declared fields A struct embedding a type with generated MarshalJSON/UnmarshalJSON inherited those methods by promotion, and the shadow trick did not stop it: a shadow still embeds the parent. Decoding a composed type therefore ran the parent's unmarshaler, which knows only the parent's fields and swept the composed type's declared properties into the parent's catch-all map. With an embedded union the promoted marshaler emitted only the union value, dropping the outer fields or producing invalid JSON for scalar variants, and with two catch-all parents the ambiguous selector promoted nothing, so behavior flipped on how many parents a schema composes. A struct with marshaler-bearing embeds now gets part-wise marshalers, even when it has no catch-all of its own: each such embed is encoded through its own marshaler and the resulting objects are merged with the struct's directly declared fields and extras, so no promoted method ever speaks for the whole object. Unmarshaling decodes each embed and the struct's own fields from the same object, then cleans the embedded catch-alls: cleared when the outer type collects extras itself, pruned of declared names otherwise. Structs without such embeds keep the shadow approach, now assembled through the same object merge helpers, which also retires the byte-splice and its assumption that the shadow marshal produced an object. dropShadowedCatchAlls is removed: it existed to keep a second catch-all from fighting the promoted one, and with promotion out of the picture a composed schema's own additionalProperties works again. Fixes #50
… lost extras Post-review fixes to the part-wise marshalers: - Cyclic allOf schemas made the generated unmarshalers re-enter each other with the same bytes until the stack overflowed. A pointer embed that closes a reference cycle is no longer decoded, matching encoding/json, which also stops its field walk where a type repeats. - An embedded union's variant keys landed in the composed type's catch-all and overrode an updated union value on re-marshal. The catch-all is now pruned with the keys the decoded union value actually marshals. - The embedded catch-all cleanup nilled the maps outright, dropping an extra that only a wider embedded map could hold when the outer catch-all is typed narrower. Cleanup now removes declared names and the keys the outer catch-all collected, leaving the rest with the embed that accepted them. - The catch-all reset ran after the embed decodes, so a failed re-unmarshal into a reused struct kept the previous payload's extras. The reset is back at the top of UnmarshalJSON. Also: the package-wide bearing fixed point is computed once per package behind a mutex instead of per helper call, the declared-names literal and alias walks are shared instead of repeated, and new e2e coverage exercises cyclic schemas, narrowly typed outer catch-alls, and union re-marshal.
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
1 participant
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.
Fixes #50.
What was broken
Go promotes an embedded type's methods, and
type shadow Tdoes not stop that: the shadow still embeds the parent. So for a composed schema whose parent hasadditionalProperties,json.Unmarshalinto the child ran the parent's unmarshaler, which swept the child's declared fields into the parent's catch-all map while the typed fields stayed nil. The round trip looked lossless, which is why it hid. The union variant of the same bug was loud: the promoted union marshaler returned whateverValueheld, so the splice produced invalid JSON for scalar values and dropped the outer fields for objects. And with two catch-all parents the selector is ambiguous, so nothing was promoted and behavior flipped on parent count.The fix
A struct with marshaler-bearing embeds (catch-all structs or unions, found transitively through aliases) now gets part-wise marshalers, even when it has no catch-all of its own:
Structs without bearing embeds keep the shadow approach, now assembled through the same merge helpers, which retires the byte-splice and its assumption that the shadow marshal produced an object.
dropShadowedCatchAllsis removed: it was the #49 stopgap for exactly this promotion, and with promotion out of the picture a composed schema's ownadditionalPropertiesworks again. A struct whose only field is a single bearing embed still relies on promotion, which is correct there because it has nothing of its own to lose.Tests
New generate-compile-run wire tests cover the issue's matrix: the repro (typed field populated, extras on the outer type, embedded catch-all empty), a grandparent chain, two catch-all parents (extras kept on the parents, declared names pruned, each key emitted once), a union embed with object and scalar values, and a union embed on a schema with no catch-all of its own.