internal: Add MarshalJSON to resource types that inherited one by promotion - #6542
Merged
Conversation
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.
Collaborator
Integration test reportCommit: f87d55a
Top 6 slowest tests (at least 2 minutes):
|
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
marked this pull request as ready for review
September 7, 2026 12:01
shreyas-goenka
approved these changes
Sep 7, 2026
janniklasrose
approved these changes
Sep 7, 2026
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.
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.
12 of the 33 resource types embed
BaseResourceplus a member that declares its ownMarshalJSON(an SDK type, or an inner*Config). Go promotes that method to the outertype, so
json.Marshalserializes only that member's fields and silently dropseverything else on the struct:
id,url,lifecycle,modified_statusandpermissions.bundle/direct/dresources/serialize_test.gowas written for exactly this failure mode,but only pointed at
StateTypeandRemoteType. Pointing it atInputConfigTypetooreports 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 structitself 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_policiesis skipped on theInputConfigTypesurface, with the reason in acomment: its
definitionis typedanyand deliberately shadowscompute.CreatePolicy.Definition(a string) so the policy document can be authored asinline YAML, and
marshal.Unmarshalhands the whole payload to the embedded member, sothe 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
MarshalJSONuses a value receiver, per the convention acrossbundle/config/resources/. For the 7 Postgres types that matters beyond style: the inner*ConfigdeclaresMarshalJSONon a pointer receiver, so onmainonly*Tsatisfiedjson.Marshalerand value-marshalling silently fell back to plainencoding/json- asecond code path that ignores
ForceSendFields. A value receiver collapses the two.Counting that asymmetry (
*Tmarshals itself,Tdoes not) across all 34 adapters and 3surfaces: 14 on
main, 7 here. The 7 remaining are pre-existing and untouched - 6Postgres*Configreachable asStateTypevia type alias, plusSecret. They are latenttoo (the state path marshals pointers throughout), and the round-trip test cannot detect
them, since it builds values with
reflect.Newand so only ever marshals a pointer.Left for a follow-up rather than fixed here, to keep this PR to one change.