Marshal resource types by value, not only by pointer - #6551
Merged
Conversation
A pointer-receiver MarshalJSON is satisfied by *T but not by T, and encoding/json
reaches it only for an addressable value. json.Marshal(&x) uses the marshaler
while json.Marshal(x) silently falls back to plain struct-field encoding -- two
code paths for one type. They disagree on more than key order, because
encoding/json knows nothing about ForceSendFields (tagged json:"-"):
PurgeOnDelete=false, ForceSendFields=["PurgeOnDelete"]
VALUE {"project_id":""}
POINTER {"project_id":"","purge_on_delete":false}
Give the 8 affected types a value receiver, and assert the invariant over every
adapter surface. The existing round-trip tests cannot catch it: they build values
with reflect.New, so they only ever marshal a pointer.
Nothing marshals these by value today (the state path passes pointers throughout,
and bundle config travels through libs/dyn), so this is a latent trap rather than
live corruption -- no golden output moves.
Collaborator
Integration test reportCommit: a16e960
Top 3 slowest tests (at least 2 minutes):
|
The first version of the test only looked at each adapter surface type, so it covered 7 of the 8 receivers this branch flips. PostgresRoleConfig is reachable only as an embedded member of PostgresRole -- PostgresRoleState is its own struct rather than an alias to the Config -- and PostgresRole declares its own value receiver, which hides the member's asymmetry from a top-level check. Reverting that one receiver left the test passing. Walk the reachable type graph instead. Reverting all 8 now flags exactly those 8. The walk also covers named fields and collection elements, which is where the same defect would be live rather than latent: marshal's structAsMap stores those into a map via .Interface(), and a map value is not addressable, so a pointer receiver is unreachable there. None exist today.
Four defects in the detector, from an adversarial review pass:
- Restricting to Kind()==Struct skipped named non-struct types, which diverge
identically. `type ID string` with a pointer-receiver MarshalJSON as a struct
field marshals as {"n":"real"} by value and {"n":"MARSHALER"} by pointer.
- derefType stripped pointers in a loop, so `type L *L` -- legal Go, where Elem()
returns the type itself -- spun forever. Pointers are now followed as graph
edges, terminating on seen.
- A type reachable only through a json:"-" field was reported even though it is
never serialized. Skipped now, for the same reason unexported fields are.
- The doc comment claimed collection elements are non-addressable. Slice elements
are addressable and do reach a pointer receiver; only map values (and array
elements in a non-addressable slot) do not.
Widening reports no new types in the tree, and reverting the 8 receivers still
flags exactly those 8.
TestMarshalerValueReceiver accumulated every surface into one shared walk and asserted once at the end, unlike its neighbours which subtest per resource type. The reason given was deduplication, and it does not hold: measured at the pre-fix baseline, each violating type is reachable from exactly one resource, so there is nothing to deduplicate. Subtest per resource type instead. A failure now names the resource, so `-run TestMarshalerValueReceiver/postgres_roles` selects it. Each subtest owns its maps rather than sharing them, since a shared seen map would attribute a type to whichever subtest reached it first and iteration order over SupportedResources is random. That re-walks shared SDK types: 1628 type visits instead of 1111, 3.8ms instead of 0.8ms, against a test that already takes ~0.6s.
TestMarshalerValueReceiver asserts the invariant reflectively; these pin the concrete behaviour it protects, as a plain marshal-and-compare that reads at a glance. Both fail with a pointer receiver and pass with a value one: - ForceSendFields on an omitempty zero value (purge_on_delete) is emitted only by the SDK marshaler; the plain encoding/json fallback a value takes under a pointer receiver would drop it. - json.Marshal(x) and json.Marshal(&x) produce identical bytes.
denik
marked this pull request as ready for review
September 8, 2026 10:39
| // TestPostgresBranchConfigMarshalHonorsForceSendFields pins the reason | ||
| // PostgresBranchConfig.MarshalJSON is declared on a value receiver. | ||
| // | ||
| // PurgeOnDelete is omitempty, so its zero value is emitted only because |
Contributor
There was a problem hiding this comment.
I assume a marshaller on the struct itself is also used for the pointer type then?
Contributor
There was a problem hiding this comment.
Ok, I see you have a test right below.
shreyas-goenka
approved these changes
Sep 8, 2026
shreyas-goenka
left a comment
shreyas-goenka
left a comment
Contributor
There was a problem hiding this comment.
The PR description is very hard to parse. Could you make it easier to understand?
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
3 participants
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 to #6542.
A pointer-receiver
MarshalJSONis satisfied by*Tbut notT, sojson.Marshal(&x)usesit while
json.Marshal(x)falls back to plainencoding/json— which ignoresForceSendFields(json:"-"):Latent today, not a live bug: the state path marshals pointers throughout and bundle config
travels through
libs/dyn. So no golden output moves and no changelog entry.Give the 8 affected types a value receiver (7
Postgres*Config+Secret, the last outertype still on a pointer).
UnmarshalJSONstays on the pointer, as it is across the package.TestMarshalerValueReceiverwalks every type reachable from the 34 adapter surfaces andasserts none has a pointer-only
MarshalJSON; reverting the 8 flags exactly those 8. It keyson the asymmetry, not the mere absence of a marshaler — many types legitimately have none. The
walk descends into embedded members and named fields, not just the surface type:
PostgresRoleConfigis only reachable as an embedded member and would otherwise be missed.