Skip to content

internal: Add MarshalJSON to resource types that inherited one by promotion - #6542

Merged
denik merged 2 commits into
mainfrom
denik/config-struct-marshalling
Sep 7, 2026
Merged

internal: Add MarshalJSON to resource types that inherited one by promotion#6542
denik merged 2 commits into
mainfrom
denik/config-struct-marshalling

Conversation

@denik

@denik denik commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

12 of the 33 resource types embed BaseResource plus a member that declares its own
MarshalJSON (an SDK type, or an inner *Config). Go promotes that method to the outer
type, so json.Marshal serializes only that member's fields and silently drops
everything else on the struct: id, url, lifecycle, modified_status and
permissions.

PostgresProject -> {"project_id":"proj"}

bundle/direct/dresources/serialize_test.go was written for exactly this failure mode,
but only pointed at StateType and RemoteType. Pointing it at InputConfigType too
reports all 12. Each gets the same two marshalers the other 21 types already have.

Bundle config is normally read and written through libs/dyn, which walks the struct
itself and never calls these marshalers, so this is a latent trap rather than live
corruption - no golden output moves. No changelog entry for that reason.

cluster_policies is skipped on the InputConfigType surface, with the reason in a
comment: its definition is typed any and deliberately shadows
compute.CreatePolicy.Definition (a string) so the policy document can be authored as
inline YAML, and marshal.Unmarshal hands the whole payload to the embedded member, so
the shadowed string comes back holding the raw JSON text and reads as a lost field. It
loses exactly those two fields today and nothing else.

Side effect worth naming

The new MarshalJSON uses a value receiver, per the convention across
bundle/config/resources/. For the 7 Postgres types that matters beyond style: the inner
*Config declares MarshalJSON on a pointer receiver, so on main only *T satisfied
json.Marshaler and value-marshalling silently fell back to plain encoding/json - a
second code path that ignores ForceSendFields. A value receiver collapses the two.

Counting that asymmetry (*T marshals itself, T does not) across all 34 adapters and 3
surfaces: 14 on main, 7 here. The 7 remaining are pre-existing and untouched - 6
Postgres*Config reachable as StateType via type alias, plus Secret. They are latent
too (the state path marshals pointers throughout), and the round-trip test cannot detect
them, since it builds values with reflect.New and so only ever marshals a pointer.
Left for a follow-up rather than fixed here, to keep this PR to one change.

12 resource types embed BaseResource plus a member that declares its own
MarshalJSON (an SDK type, or an inner *Config). Go promotes that method to the
outer type, so json.Marshal serializes only that member's fields and silently
drops everything else: id, url, lifecycle, modified_status and permissions.

    PostgresProject -> {"project_id":"proj"}

Point the existing serialize_test at InputConfigType, which is the one surface it
did not cover, and give each of the 12 types the marshalers the other 21 already
have.
@eng-dev-ecosystem-bot

eng-dev-ecosystem-bot commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: f87d55a

Run: 34103852138

Env 💚​RECOVERED ✅​pass 🙈​skip Time
💚​ aws linux 1 275 15 6:46
💚​ aws windows 1 277 13 3:55
💚​ azure linux 1 274 15 6:32
💚​ azure windows 1 276 13 4:09
💚​ gcp linux 1 275 15 7:33
💚​ gcp windows 1 277 13 4:10
Test Name aws linux aws windows azure linux azure windows gcp linux gcp windows
💚​ TestAccept 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
Top 6 slowest tests (at least 2 minutes):
duration env testname
3:59 gcp windows TestAccept
3:58 azure linux TestAccept
3:56 aws linux TestAccept
3:53 gcp linux TestAccept
3:44 azure windows TestAccept
3:34 aws windows TestAccept

Filling a free-form `any` with a string rather than a map made the
cluster_policies round-trip pass, but it also dropped map-valued coverage for
every other `any` field on every surface -- including
DashboardState.SerializedDashboard and GenieSpaceState.SerializedSpace, which are
written to the state file and emitted in the plan's remote_state.

Restore the map fill and skip cluster_policies on the InputConfigType surface
instead, with the reason in a comment.
@denik denik changed the title Add MarshalJSON to resource types that inherited one by promotion internal: Add MarshalJSON to resource types that inherited one by promotion Sep 7, 2026
@denik
denik marked this pull request as ready for review September 7, 2026 12:01
@denik
denik added this pull request to the merge queue Sep 7, 2026
Merged via the queue into main with commit 18fe112 Sep 7, 2026
38 checks passed
@denik
denik deleted the denik/config-struct-marshalling branch September 7, 2026 13:16
denik added a commit that referenced this pull request Sep 7, 2026
#6542 added MarshalJSON to every resource type that was inheriting an SDK
marshaler by promotion. That fixed the bug the entries were tracking: id,
url, modified_status, lifecycle.prevent_destroy and the permissions/file_path
extras now survive encoding/json round-trips for all resource types. The ratchet
correctly detected the fix and failed the test; remove the entries.
denik added a commit that referenced this pull request Sep 7, 2026
#6542 added MarshalJSON to every resource type that was inheriting an SDK
marshaler by promotion. That fixed the bug the entries were tracking: id,
url, modified_status, lifecycle.prevent_destroy and the permissions/file_path
extras now survive encoding/json round-trips for all resource types. The ratchet
correctly detected the fix and failed the test; remove the entries.
denik added a commit that referenced this pull request Sep 8, 2026
#6542 added MarshalJSON to every resource type that was inheriting an SDK
marshaler by promotion. That fixed the bug the entries were tracking: id,
url, modified_status, lifecycle.prevent_destroy and the permissions/file_path
extras now survive encoding/json round-trips for all resource types. The ratchet
correctly detected the fix and failed the test; remove the entries.
chenyuem-db pushed a commit to chenyuem-db/cli that referenced this pull request Sep 9, 2026
Follow-up to databricks#6542.

A pointer-receiver `MarshalJSON` is satisfied by `*T` but not `T`, so
`json.Marshal(&x)` uses
it while `json.Marshal(x)` falls back to plain `encoding/json` — which
ignores
`ForceSendFields` (`json:"-"`):

```
PostgresProjectConfig, PurgeOnDelete=false, ForceSendFields=["PurgeOnDelete"]
  VALUE   {"project_id":""}
  POINTER {"project_id":"","purge_on_delete":false}
```

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 outer
type still on a pointer). `UnmarshalJSON` stays on the pointer, as it is
across the package.

`TestMarshalerValueReceiver` walks every type reachable from the 34
adapter surfaces and
asserts none has a pointer-only `MarshalJSON`; reverting the 8 flags
exactly those 8. It keys
on 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:
`PostgresRoleConfig` is only reachable as an embedded member and would
otherwise be missed.
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.

4 participants