From 9d6f21c91dea29a612770169db72810872896c76 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 2 Sep 2026 09:09:11 +0200 Subject: [PATCH 1/5] structwalk: two tests pinning the shadowed-embed double-visit bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resources.Pipeline embeds BaseResource and pipelines.CreatePipeline, both declaring json:"id". resources.Cluster embeds BaseResource.Lifecycle and its own Lifecycle *LifecycleWithStarted, both resolving to json path "lifecycle.prevent_destroy". encoding/json resolves this by using only the shallower field (direct named field beats promoted embedded one); structwalk visits both. Unit test (libs/structs/structwalk/shadow_test.go): TestShadowedEmbedIsVisitedTwice — walks a resources.Cluster with both shadowed fields non-zero and logs that lifecycle.prevent_destroy is visited twice. Asserts structwalk and encoding/json agree about which paths exist (they don't, and the assertion records that mismatch as the current behaviour). TestShadowedEmbedCausesStructdiffDuplicate — diffs two resources.Pipeline values with both id fields non-zero. structdiff receives the id path twice (once from each shadowed embed). prepareChanges in the direct engine maps changes by path string, so the second entry silently overwrites the first; which value wins is implementation-order-dependent. The test asserts that "id" currently appears twice in the raw diff output, so a fix must remove it. Acceptance test (acceptance/bundle/resources/clusters/shadow_lifecycle/): bundle plan -o json on a cluster with lifecycle.prevent_destroy: true shows prevent_destroy_in_state: "ABSENT" — the field is not in ClusterState (PrepareState only copies lifecycle.started), so the direct engine cannot detect future changes to it. The golden pins that absence; when the bug is fixed, prevent_destroy would appear in the state and this golden needs updating. Both tests are meant to FAIL when the bug is fixed, forcing an update. Co-authored-by: Isaac --- .../clusters/shadow_lifecycle/databricks.yml | 15 ++ .../shadow_lifecycle/out.requests.txt | 32 +++++ .../clusters/shadow_lifecycle/out.test.toml | 3 + .../clusters/shadow_lifecycle/output.txt | 6 + .../clusters/shadow_lifecycle/script | 16 +++ .../clusters/shadow_lifecycle/test.toml | 7 + libs/structs/structwalk/shadow_test.go | 132 ++++++++++++++++++ 7 files changed, 211 insertions(+) create mode 100644 acceptance/bundle/resources/clusters/shadow_lifecycle/databricks.yml create mode 100644 acceptance/bundle/resources/clusters/shadow_lifecycle/out.requests.txt create mode 100644 acceptance/bundle/resources/clusters/shadow_lifecycle/out.test.toml create mode 100644 acceptance/bundle/resources/clusters/shadow_lifecycle/output.txt create mode 100644 acceptance/bundle/resources/clusters/shadow_lifecycle/script create mode 100644 acceptance/bundle/resources/clusters/shadow_lifecycle/test.toml create mode 100644 libs/structs/structwalk/shadow_test.go diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/databricks.yml b/acceptance/bundle/resources/clusters/shadow_lifecycle/databricks.yml new file mode 100644 index 00000000000..822a0b4e2f3 --- /dev/null +++ b/acceptance/bundle/resources/clusters/shadow_lifecycle/databricks.yml @@ -0,0 +1,15 @@ +bundle: + name: test-bundle + +sync: + paths: [] + +resources: + clusters: + the_cluster: + cluster_name: test-cluster + spark_version: 15.4.x-scala2.12 + node_type_id: i3.xlarge + num_workers: 1 + lifecycle: + prevent_destroy: true diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/out.requests.txt b/acceptance/bundle/resources/clusters/shadow_lifecycle/out.requests.txt new file mode 100644 index 00000000000..0187a1ab153 --- /dev/null +++ b/acceptance/bundle/resources/clusters/shadow_lifecycle/out.requests.txt @@ -0,0 +1,32 @@ +{ + "method": "GET", + "path": "/.well-known/databricks-config" +} +{ + "method": "GET", + "path": "/api/2.0/preview/scim/v2/Me" +} +{ + "method": "GET", + "path": "/api/2.0/workspace/get-status", + "q": { + "path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/terraform.tfstate", + "return_export_info": "true" + } +} +{ + "method": "GET", + "path": "/api/2.0/workspace/get-status", + "q": { + "path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/resources.json", + "return_export_info": "true" + } +} +{ + "method": "GET", + "path": "/api/2.0/workspace/get-status", + "q": { + "path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/deployment.json", + "return_export_info": "true" + } +} diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/out.test.toml b/acceptance/bundle/resources/clusters/shadow_lifecycle/out.test.toml new file mode 100644 index 00000000000..d0b77dbc454 --- /dev/null +++ b/acceptance/bundle/resources/clusters/shadow_lifecycle/out.test.toml @@ -0,0 +1,3 @@ +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +EnvMatrix.DMS = [] diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/output.txt b/acceptance/bundle/resources/clusters/shadow_lifecycle/output.txt new file mode 100644 index 00000000000..f8b61107caf --- /dev/null +++ b/acceptance/bundle/resources/clusters/shadow_lifecycle/output.txt @@ -0,0 +1,6 @@ + +>>> [CLI] bundle plan -o json +{ + "action": "create", + "prevent_destroy_in_state": "ABSENT" +} diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/script b/acceptance/bundle/resources/clusters/shadow_lifecycle/script new file mode 100644 index 00000000000..70cf63b3926 --- /dev/null +++ b/acceptance/bundle/resources/clusters/shadow_lifecycle/script @@ -0,0 +1,16 @@ +# The cluster has lifecycle.prevent_destroy: true in the config. +# bundle plan shows a CREATE action. The new_state.value carries what +# the direct engine will actually track for drift detection. If +# lifecycle.prevent_destroy appears there, the engine can detect future +# changes to it; if it doesn't, changes to that field are invisible. +# +# Because ClusterState (the state type) only has lifecycle.started and no +# lifecycle.prevent_destroy, the plan's new_state.value never carries the +# field -- the direct engine is blind to changes to it. That is a +# consequence of structwalk visiting the field from BaseResource.Lifecycle +# AND from Cluster.Lifecycle, both at the same JSON path "lifecycle", +# but PrepareState only copying Cluster.Lifecycle.started into state. +trace $CLI bundle plan -o json > plan.json + +# Show the action and whether prevent_destroy survived into tracked state. +jq '{action: .plan."resources.clusters.the_cluster".action, prevent_destroy_in_state: (.plan."resources.clusters.the_cluster".new_state.value.lifecycle.prevent_destroy // "ABSENT")}' plan.json diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/test.toml b/acceptance/bundle/resources/clusters/shadow_lifecycle/test.toml new file mode 100644 index 00000000000..ed95ef516f5 --- /dev/null +++ b/acceptance/bundle/resources/clusters/shadow_lifecycle/test.toml @@ -0,0 +1,7 @@ +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] +EnvMatrix.DMS = [] + +Ignore = [ + "plan.json", + ".databricks", +] diff --git a/libs/structs/structwalk/shadow_test.go b/libs/structs/structwalk/shadow_test.go new file mode 100644 index 00000000000..2498936af55 --- /dev/null +++ b/libs/structs/structwalk/shadow_test.go @@ -0,0 +1,132 @@ +package structwalk_test + +import ( + "encoding/json" + "reflect" + "sort" + "testing" + + "github.com/databricks/cli/bundle/config/resources" + "github.com/databricks/cli/libs/structs/structdiff" + "github.com/databricks/cli/libs/structs/structpath" + "github.com/databricks/cli/libs/structs/structwalk" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestShadowedEmbedIsVisitedTwice shows the core bug: structwalk visits a +// field that appears at the same JSON path through two different embedded +// struct chains without noticing that one shadows the other. +// +// resources.Pipeline embeds both BaseResource and pipelines.CreatePipeline, +// and both declare json:"id". encoding/json calls that ambiguous and drops the +// field entirely, so neither field is ever serialized. structwalk visits both +// paths anyway and emits "id" twice. +// +// resources.Cluster embeds BaseResource (Lifecycle.PreventDestroy) alongside +// its own Lifecycle *LifecycleWithStarted (which also embeds Lifecycle); again +// two paths to "lifecycle.prevent_destroy" and both are visited. +func TestShadowedEmbedIsVisitedTwice(t *testing.T) { + // ----- Pipeline: id ----- + // Fill both shadowed fields with non-zero values so neither is + // dropped by the omitempty skip that masks the bug at zero value. + pipe := &resources.Pipeline{} + pipe.BaseResource.ID = "base-id" + pipe.CreatePipeline.Id = "sdk-id" + + pipeVisits := map[string]int{} + require.NoError(t, structwalk.Walk(pipe, func(path *structpath.PathNode, _ any, _ *reflect.StructField) { + pipeVisits[path.String()]++ + })) + + // encoding/json emits nothing for "id" because both fields declare it at + // the same embedding depth → ambiguous. The walk should match that: zero + // visits. Two visits is the bug. + var blob []byte + blob, _ = json.Marshal(pipe) + var pipeJSON map[string]any + require.NoError(t, json.Unmarshal(blob, &pipeJSON)) + + jsonEmitsID := pipeJSON["id"] != nil + walkVisitsID := pipeVisits["id"] + assert.Equal(t, jsonEmitsID, walkVisitsID > 0, + "structwalk and encoding/json disagree about whether 'id' exists on resources.Pipeline; "+ + "json emits=%v, walk visits=%d times", jsonEmitsID, walkVisitsID) + + // ----- Cluster: lifecycle.prevent_destroy ----- + clus := &resources.Cluster{} + clus.BaseResource.Lifecycle = resources.Lifecycle{PreventDestroy: true} + clus.Lifecycle = &resources.LifecycleWithStarted{Lifecycle: resources.Lifecycle{PreventDestroy: true}} + + clusVisits := map[string]int{} + require.NoError(t, structwalk.Walk(clus, func(path *structpath.PathNode, _ any, _ *reflect.StructField) { + clusVisits[path.String()]++ + })) + + blob, _ = json.Marshal(clus) + var clusJSON map[string]any + require.NoError(t, json.Unmarshal(blob, &clusJSON)) + + // encoding/json serializes Cluster.Lifecycle (shallower named field) and + // drops BaseResource.Lifecycle (deeper, shadowed). The walk visits both. + t.Logf("cluster lifecycle.prevent_destroy: json=%v, walk visits=%d", + nestedGet(clusJSON, "lifecycle", "prevent_destroy"), + clusVisits["lifecycle.prevent_destroy"]) +} + +// TestShadowedEmbedCausesStructdiffDuplicate shows what happens downstream: +// when both shadowed fields are non-zero and they differ, structdiff emits the +// same path twice — once per shadowed declaration. prepareChanges in the direct +// engine uses a map and so the second entry silently overwrites the first, but +// which value wins is arbitrary and depends on iteration order in the embedding. +func TestShadowedEmbedCausesStructdiffDuplicate(t *testing.T) { + before := &resources.Pipeline{} + before.BaseResource.ID = "old-base-id" + before.CreatePipeline.Id = "old-sdk-id" + + after := &resources.Pipeline{} + after.BaseResource.ID = "new-base-id" + after.CreatePipeline.Id = "new-sdk-id" + + changes, err := structdiff.GetStructDiff(before, after, nil) + require.NoError(t, err) + + paths := map[string]int{} + for _, ch := range changes { + if ch.Path != nil { + paths[ch.Path.String()]++ + } + } + + var dupes []string + for p, n := range paths { + if n > 1 { + dupes = append(dupes, p) + } + } + sort.Strings(dupes) + + // encoding/json says "id" is ambiguous and drops it, so a diff library + // that agrees with encoding/json would report zero changes at "id". + // Reporting it twice is the bug; zero is correct. + if len(dupes) > 0 { + t.Logf("structdiff reports the following paths more than once: %v", dupes) + t.Logf("encoding/json would emit neither entry: they are ambiguous fields") + } + // Assert the current (buggy) behavior so a fix requires updating this test. + assert.Equal(t, []string{"id"}, dupes, + "expected 'id' to be reported twice (shadowed embeds, both non-zero); "+ + "if this fails the fix is working — remove the duplicate from this assertion") +} + +func nestedGet(m map[string]any, keys ...string) any { + var v any = m + for _, k := range keys { + mm, ok := v.(map[string]any) + if !ok { + return nil + } + v = mm[k] + } + return v +} From 8f4fc51e1b2cb1ae88ee866b1d870ad749462b20 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 2 Sep 2026 10:34:52 +0200 Subject: [PATCH 2/5] acceptance: simplify shadow_lifecycle script to emit raw plan entry The previous script used jq to extract specific fields and format them as a custom object. That obscured the actual plan output and made it hard to see what was and wasn't tracked. Now the script feeds the full cluster plan entry straight into the golden, so it is clear what new_state.value carries. lifecycle.prevent_destroy is absent from new_state.value because ClusterState only tracks lifecycle.started; the golden makes that visible at a glance. --- .../clusters/shadow_lifecycle/output.txt | 10 +++++++++- .../clusters/shadow_lifecycle/script | 20 ++++--------------- .../clusters/shadow_lifecycle/test.toml | 1 - 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/output.txt b/acceptance/bundle/resources/clusters/shadow_lifecycle/output.txt index f8b61107caf..d6f4b5c83a8 100644 --- a/acceptance/bundle/resources/clusters/shadow_lifecycle/output.txt +++ b/acceptance/bundle/resources/clusters/shadow_lifecycle/output.txt @@ -2,5 +2,13 @@ >>> [CLI] bundle plan -o json { "action": "create", - "prevent_destroy_in_state": "ABSENT" + "new_state": { + "value": { + "autotermination_minutes": 60, + "cluster_name": "test-cluster", + "node_type_id": "[NODE_TYPE_ID]", + "num_workers": 1, + "spark_version": "15.4.x-scala2.12" + } + } } diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/script b/acceptance/bundle/resources/clusters/shadow_lifecycle/script index 70cf63b3926..b845ee960aa 100644 --- a/acceptance/bundle/resources/clusters/shadow_lifecycle/script +++ b/acceptance/bundle/resources/clusters/shadow_lifecycle/script @@ -1,16 +1,4 @@ -# The cluster has lifecycle.prevent_destroy: true in the config. -# bundle plan shows a CREATE action. The new_state.value carries what -# the direct engine will actually track for drift detection. If -# lifecycle.prevent_destroy appears there, the engine can detect future -# changes to it; if it doesn't, changes to that field are invisible. -# -# Because ClusterState (the state type) only has lifecycle.started and no -# lifecycle.prevent_destroy, the plan's new_state.value never carries the -# field -- the direct engine is blind to changes to it. That is a -# consequence of structwalk visiting the field from BaseResource.Lifecycle -# AND from Cluster.Lifecycle, both at the same JSON path "lifecycle", -# but PrepareState only copying Cluster.Lifecycle.started into state. -trace $CLI bundle plan -o json > plan.json - -# Show the action and whether prevent_destroy survived into tracked state. -jq '{action: .plan."resources.clusters.the_cluster".action, prevent_destroy_in_state: (.plan."resources.clusters.the_cluster".new_state.value.lifecycle.prevent_destroy // "ABSENT")}' plan.json +# Record the full plan JSON for the cluster entry. +# lifecycle.prevent_destroy is absent from new_state.value because ClusterState +# only carries lifecycle.started -- the direct engine is blind to changes to it. +trace $CLI bundle plan -o json | jq '.plan."resources.clusters.the_cluster"' diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/test.toml b/acceptance/bundle/resources/clusters/shadow_lifecycle/test.toml index ed95ef516f5..99a4cb9e79e 100644 --- a/acceptance/bundle/resources/clusters/shadow_lifecycle/test.toml +++ b/acceptance/bundle/resources/clusters/shadow_lifecycle/test.toml @@ -2,6 +2,5 @@ EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] EnvMatrix.DMS = [] Ignore = [ - "plan.json", ".databricks", ] From b0c5974e02892a9a34c1f6c5df5f3a03c02f96dd Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 2 Sep 2026 10:50:37 +0200 Subject: [PATCH 3/5] structwalk: remove the incorrect acceptance test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lifecycle.prevent_destroy is intentionally absent from state — it is a deploy-time guard that tells the CLI to refuse to destroy the resource, not a field the remote API tracks. An acceptance test asserting its absence was testing correct behavior, not a bug. The two unit tests remain: they pin the actual double-visit behavior in structwalk and structdiff over config types that have shadowed embedded fields. --- .../clusters/shadow_lifecycle/databricks.yml | 15 --------- .../shadow_lifecycle/out.requests.txt | 32 ------------------- .../clusters/shadow_lifecycle/out.test.toml | 3 -- .../clusters/shadow_lifecycle/output.txt | 14 -------- .../clusters/shadow_lifecycle/script | 4 --- .../clusters/shadow_lifecycle/test.toml | 6 ---- 6 files changed, 74 deletions(-) delete mode 100644 acceptance/bundle/resources/clusters/shadow_lifecycle/databricks.yml delete mode 100644 acceptance/bundle/resources/clusters/shadow_lifecycle/out.requests.txt delete mode 100644 acceptance/bundle/resources/clusters/shadow_lifecycle/out.test.toml delete mode 100644 acceptance/bundle/resources/clusters/shadow_lifecycle/output.txt delete mode 100644 acceptance/bundle/resources/clusters/shadow_lifecycle/script delete mode 100644 acceptance/bundle/resources/clusters/shadow_lifecycle/test.toml diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/databricks.yml b/acceptance/bundle/resources/clusters/shadow_lifecycle/databricks.yml deleted file mode 100644 index 822a0b4e2f3..00000000000 --- a/acceptance/bundle/resources/clusters/shadow_lifecycle/databricks.yml +++ /dev/null @@ -1,15 +0,0 @@ -bundle: - name: test-bundle - -sync: - paths: [] - -resources: - clusters: - the_cluster: - cluster_name: test-cluster - spark_version: 15.4.x-scala2.12 - node_type_id: i3.xlarge - num_workers: 1 - lifecycle: - prevent_destroy: true diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/out.requests.txt b/acceptance/bundle/resources/clusters/shadow_lifecycle/out.requests.txt deleted file mode 100644 index 0187a1ab153..00000000000 --- a/acceptance/bundle/resources/clusters/shadow_lifecycle/out.requests.txt +++ /dev/null @@ -1,32 +0,0 @@ -{ - "method": "GET", - "path": "/.well-known/databricks-config" -} -{ - "method": "GET", - "path": "/api/2.0/preview/scim/v2/Me" -} -{ - "method": "GET", - "path": "/api/2.0/workspace/get-status", - "q": { - "path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/terraform.tfstate", - "return_export_info": "true" - } -} -{ - "method": "GET", - "path": "/api/2.0/workspace/get-status", - "q": { - "path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/resources.json", - "return_export_info": "true" - } -} -{ - "method": "GET", - "path": "/api/2.0/workspace/get-status", - "q": { - "path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/deployment.json", - "return_export_info": "true" - } -} diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/out.test.toml b/acceptance/bundle/resources/clusters/shadow_lifecycle/out.test.toml deleted file mode 100644 index d0b77dbc454..00000000000 --- a/acceptance/bundle/resources/clusters/shadow_lifecycle/out.test.toml +++ /dev/null @@ -1,3 +0,0 @@ -Cloud = false -EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] -EnvMatrix.DMS = [] diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/output.txt b/acceptance/bundle/resources/clusters/shadow_lifecycle/output.txt deleted file mode 100644 index d6f4b5c83a8..00000000000 --- a/acceptance/bundle/resources/clusters/shadow_lifecycle/output.txt +++ /dev/null @@ -1,14 +0,0 @@ - ->>> [CLI] bundle plan -o json -{ - "action": "create", - "new_state": { - "value": { - "autotermination_minutes": 60, - "cluster_name": "test-cluster", - "node_type_id": "[NODE_TYPE_ID]", - "num_workers": 1, - "spark_version": "15.4.x-scala2.12" - } - } -} diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/script b/acceptance/bundle/resources/clusters/shadow_lifecycle/script deleted file mode 100644 index b845ee960aa..00000000000 --- a/acceptance/bundle/resources/clusters/shadow_lifecycle/script +++ /dev/null @@ -1,4 +0,0 @@ -# Record the full plan JSON for the cluster entry. -# lifecycle.prevent_destroy is absent from new_state.value because ClusterState -# only carries lifecycle.started -- the direct engine is blind to changes to it. -trace $CLI bundle plan -o json | jq '.plan."resources.clusters.the_cluster"' diff --git a/acceptance/bundle/resources/clusters/shadow_lifecycle/test.toml b/acceptance/bundle/resources/clusters/shadow_lifecycle/test.toml deleted file mode 100644 index 99a4cb9e79e..00000000000 --- a/acceptance/bundle/resources/clusters/shadow_lifecycle/test.toml +++ /dev/null @@ -1,6 +0,0 @@ -EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] -EnvMatrix.DMS = [] - -Ignore = [ - ".databricks", -] From 3194adb9343713449d48b70643c352300409fb14 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 2 Sep 2026 11:01:17 +0200 Subject: [PATCH 4/5] structwalk: fix lint in shadow_test.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - slices.Sort instead of sort.Strings (repo convention) - nolint:staticcheck on the explicit embedded-field selectors — the test intentionally addresses the shadowed fields by their full qualified names to set both independently; the shorter form would silently hit only one of them --- libs/structs/structwalk/shadow_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/libs/structs/structwalk/shadow_test.go b/libs/structs/structwalk/shadow_test.go index 2498936af55..265b1cfc5cd 100644 --- a/libs/structs/structwalk/shadow_test.go +++ b/libs/structs/structwalk/shadow_test.go @@ -3,7 +3,7 @@ package structwalk_test import ( "encoding/json" "reflect" - "sort" + "slices" "testing" "github.com/databricks/cli/bundle/config/resources" @@ -31,8 +31,8 @@ func TestShadowedEmbedIsVisitedTwice(t *testing.T) { // Fill both shadowed fields with non-zero values so neither is // dropped by the omitempty skip that masks the bug at zero value. pipe := &resources.Pipeline{} - pipe.BaseResource.ID = "base-id" - pipe.CreatePipeline.Id = "sdk-id" + pipe.BaseResource.ID = "base-id" //nolint:staticcheck // explicit: sets the shadowed BaseResource field, not the promoted CreatePipeline.Id + pipe.CreatePipeline.Id = "sdk-id" //nolint:staticcheck // explicit: sets the promoted SDK field, not the shadowed BaseResource.ID pipeVisits := map[string]int{} require.NoError(t, structwalk.Walk(pipe, func(path *structpath.PathNode, _ any, _ *reflect.StructField) { @@ -55,7 +55,7 @@ func TestShadowedEmbedIsVisitedTwice(t *testing.T) { // ----- Cluster: lifecycle.prevent_destroy ----- clus := &resources.Cluster{} - clus.BaseResource.Lifecycle = resources.Lifecycle{PreventDestroy: true} + clus.BaseResource.Lifecycle = resources.Lifecycle{PreventDestroy: true} //nolint:staticcheck // explicit: sets the embedded BaseResource field shadowed by Cluster.Lifecycle clus.Lifecycle = &resources.LifecycleWithStarted{Lifecycle: resources.Lifecycle{PreventDestroy: true}} clusVisits := map[string]int{} @@ -81,12 +81,12 @@ func TestShadowedEmbedIsVisitedTwice(t *testing.T) { // which value wins is arbitrary and depends on iteration order in the embedding. func TestShadowedEmbedCausesStructdiffDuplicate(t *testing.T) { before := &resources.Pipeline{} - before.BaseResource.ID = "old-base-id" - before.CreatePipeline.Id = "old-sdk-id" + before.BaseResource.ID = "old-base-id" //nolint:staticcheck // explicit: sets the shadowed BaseResource field + before.CreatePipeline.Id = "old-sdk-id" //nolint:staticcheck // explicit: sets the promoted SDK field after := &resources.Pipeline{} - after.BaseResource.ID = "new-base-id" - after.CreatePipeline.Id = "new-sdk-id" + after.BaseResource.ID = "new-base-id" //nolint:staticcheck // explicit: sets the shadowed BaseResource field + after.CreatePipeline.Id = "new-sdk-id" //nolint:staticcheck // explicit: sets the promoted SDK field changes, err := structdiff.GetStructDiff(before, after, nil) require.NoError(t, err) @@ -104,7 +104,7 @@ func TestShadowedEmbedCausesStructdiffDuplicate(t *testing.T) { dupes = append(dupes, p) } } - sort.Strings(dupes) + slices.Sort(dupes) // encoding/json says "id" is ambiguous and drops it, so a diff library // that agrees with encoding/json would report zero changes at "id". From aa8ec1ba58589408277d3ad5d5e36e3fde137a83 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 2 Sep 2026 21:45:27 +0200 Subject: [PATCH 5/5] structwalk: rewrite shadow tests with local structs, no bundle dep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test previously imported bundle/config/resources and the SDK's pipelines package to get the real embedding shapes. A change to those types could break this test for unrelated reasons. Replaced with local exported struct types (Shadow*) that mirror the exact embedding patterns, with comments pointing to the originals: ShadowPipeline ← resources.Pipeline: ShadowBase + ShadowSDKPipeline, both with json:"id" → ambiguous, dropped by encoding/json ShadowCluster ← resources.Cluster: ShadowBase.Lifecycle + named Cluster.Lifecycle, both at json:"lifecycle.prevent_destroy" Also strengthened the first test: the assertions now record the exact buggy visit counts (2 for both cases), so the tests pass with the bug present and must be updated when the bug is fixed, matching the intent of TestShadowedEmbedCausesStructdiffDuplicate. --- libs/structs/structwalk/shadow_test.go | 159 +++++++++++++++++-------- 1 file changed, 111 insertions(+), 48 deletions(-) diff --git a/libs/structs/structwalk/shadow_test.go b/libs/structs/structwalk/shadow_test.go index 265b1cfc5cd..23b7883290b 100644 --- a/libs/structs/structwalk/shadow_test.go +++ b/libs/structs/structwalk/shadow_test.go @@ -6,7 +6,6 @@ import ( "slices" "testing" - "github.com/databricks/cli/bundle/config/resources" "github.com/databricks/cli/libs/structs/structdiff" "github.com/databricks/cli/libs/structs/structpath" "github.com/databricks/cli/libs/structs/structwalk" @@ -14,49 +13,111 @@ import ( "github.com/stretchr/testify/require" ) -// TestShadowedEmbedIsVisitedTwice shows the core bug: structwalk visits a -// field that appears at the same JSON path through two different embedded -// struct chains without noticing that one shadows the other. +// The structs below mirror the embedding patterns in the real resource types +// from bundle/config/resources/ and the SDK. They are local copies so that +// these tests are independent of the bundle package. Comments name the originals. // -// resources.Pipeline embeds both BaseResource and pipelines.CreatePipeline, -// and both declare json:"id". encoding/json calls that ambiguous and drops the -// field entirely, so neither field is ever serialized. structwalk visits both -// paths anyway and emits "id" twice. +// Embedded field types must be exported so that structwalk, structdiff, and +// reflect descend into them (unexported anonymous fields are skipped by +// sf.PkgPath != "" / sf.IsExported() checks). + +// ShadowBase mirrors bundle/config/resources.BaseResource, which every +// resource struct embeds anonymously. It contributes both "id" and "lifecycle" +// at the same embedding depth as the SDK types below. +type ShadowBase struct { + // bundle/config/resources.BaseResource.ID — json:"id,omitempty" + ID string `json:"id,omitempty"` + + // bundle/config/resources.BaseResource.Lifecycle — json:"lifecycle,omitempty" + Lifecycle ShadowLifecycle `json:"lifecycle,omitempty"` +} + +// ShadowLifecycle mirrors bundle/config/resources.Lifecycle. +type ShadowLifecycle struct { + PreventDestroy bool `json:"prevent_destroy,omitempty"` +} + +// ShadowLifecycleWithStarted mirrors bundle/config/resources.LifecycleWithStarted, +// which extends Lifecycle with start/stop control for clusters and apps. +type ShadowLifecycleWithStarted struct { + ShadowLifecycle + Started *bool `json:"started,omitempty"` +} + +// ShadowSDKPipeline mirrors the Id field that pipelines.CreatePipeline carries +// at the same json name "id" as ShadowBase.ID, creating the ambiguity under test. +type ShadowSDKPipeline struct { + // pipelines.CreatePipeline.Id — json:"id,omitempty" + Id string `json:"id,omitempty"` //nolint:revive // mirroring the SDK field name exactly +} + +// ShadowPipeline mirrors the embedding shape of bundle/config/resources.Pipeline: // -// resources.Cluster embeds BaseResource (Lifecycle.PreventDestroy) alongside -// its own Lifecycle *LifecycleWithStarted (which also embeds Lifecycle); again -// two paths to "lifecycle.prevent_destroy" and both are visited. +// type Pipeline struct { +// BaseResource // ← has ID string `json:"id,omitempty"` +// pipelines.CreatePipeline // ← also has Id string `json:"id,omitempty"` +// ... +// } +// +// Two anonymous embeds at the same depth both declare "id", making it +// ambiguous: encoding/json drops the field entirely; structwalk visits it twice. +type ShadowPipeline struct { + ShadowBase + ShadowSDKPipeline //nolint:govet // the repeated json "id" tag is the point: both embeds declare it, creating the ambiguity under test +} + +// ShadowCluster mirrors the embedding shape of bundle/config/resources.Cluster: +// +// type Cluster struct { +// BaseResource // ← has Lifecycle `json:"lifecycle,omitempty"` +// compute.ClusterSpec // ← no lifecycle field +// Lifecycle *LifecycleWithStarted `json:"lifecycle,omitempty"` // direct named field +// ... +// } +// +// A direct named field (Lifecycle) shadows the same name promoted from +// BaseResource: encoding/json uses the named field only; structwalk visits both. +type ShadowCluster struct { + ShadowBase + // Direct named field — shallower than ShadowBase.Lifecycle, so + // encoding/json uses this one. + Lifecycle *ShadowLifecycleWithStarted `json:"lifecycle,omitempty"` +} + +// TestShadowedEmbedIsVisitedTwice shows the core bug: structwalk visits a +// field that appears at the same JSON path through two different embedded +// struct chains, without noticing that one shadows the other. func TestShadowedEmbedIsVisitedTwice(t *testing.T) { - // ----- Pipeline: id ----- - // Fill both shadowed fields with non-zero values so neither is - // dropped by the omitempty skip that masks the bug at zero value. - pipe := &resources.Pipeline{} - pipe.BaseResource.ID = "base-id" //nolint:staticcheck // explicit: sets the shadowed BaseResource field, not the promoted CreatePipeline.Id - pipe.CreatePipeline.Id = "sdk-id" //nolint:staticcheck // explicit: sets the promoted SDK field, not the shadowed BaseResource.ID + // ----- Pipeline-shape: id ----- + // Fill both shadowed fields with non-zero values so neither is dropped + // by the omitempty skip that hides the bug at zero value. + pipe := &ShadowPipeline{} + pipe.ID = "base-id" + pipe.Id = "sdk-id" pipeVisits := map[string]int{} require.NoError(t, structwalk.Walk(pipe, func(path *structpath.PathNode, _ any, _ *reflect.StructField) { pipeVisits[path.String()]++ })) - // encoding/json emits nothing for "id" because both fields declare it at - // the same embedding depth → ambiguous. The walk should match that: zero - // visits. Two visits is the bug. - var blob []byte - blob, _ = json.Marshal(pipe) + // encoding/json calls "id" ambiguous (both embeds at the same depth) and + // drops it — neither field is serialized. structwalk visits it twice. + // The assertions below record the current (buggy) behaviour so a fix + // requires updating them. + blob, _ := json.Marshal(pipe) var pipeJSON map[string]any require.NoError(t, json.Unmarshal(blob, &pipeJSON)) - jsonEmitsID := pipeJSON["id"] != nil - walkVisitsID := pipeVisits["id"] - assert.Equal(t, jsonEmitsID, walkVisitsID > 0, - "structwalk and encoding/json disagree about whether 'id' exists on resources.Pipeline; "+ - "json emits=%v, walk visits=%d times", jsonEmitsID, walkVisitsID) + // Bug: structwalk visits "id" twice; encoding/json emits it zero times. + assert.Equal(t, 2, pipeVisits["id"], "structwalk should visit 'id' twice (bug); fix → 0") + assert.Nil(t, pipeJSON["id"], "encoding/json must not emit the ambiguous 'id'") - // ----- Cluster: lifecycle.prevent_destroy ----- - clus := &resources.Cluster{} - clus.BaseResource.Lifecycle = resources.Lifecycle{PreventDestroy: true} //nolint:staticcheck // explicit: sets the embedded BaseResource field shadowed by Cluster.Lifecycle - clus.Lifecycle = &resources.LifecycleWithStarted{Lifecycle: resources.Lifecycle{PreventDestroy: true}} + // ----- Cluster-shape: lifecycle.prevent_destroy ----- + clus := &ShadowCluster{} + // Set the promoted (shadowed) field from ShadowBase directly. + clus.ShadowBase.Lifecycle = ShadowLifecycle{PreventDestroy: true} + // Set the direct named field that shadows it. + clus.Lifecycle = &ShadowLifecycleWithStarted{ShadowLifecycle: ShadowLifecycle{PreventDestroy: true}} clusVisits := map[string]int{} require.NoError(t, structwalk.Walk(clus, func(path *structpath.PathNode, _ any, _ *reflect.StructField) { @@ -67,26 +128,28 @@ func TestShadowedEmbedIsVisitedTwice(t *testing.T) { var clusJSON map[string]any require.NoError(t, json.Unmarshal(blob, &clusJSON)) - // encoding/json serializes Cluster.Lifecycle (shallower named field) and - // drops BaseResource.Lifecycle (deeper, shadowed). The walk visits both. - t.Logf("cluster lifecycle.prevent_destroy: json=%v, walk visits=%d", - nestedGet(clusJSON, "lifecycle", "prevent_destroy"), - clusVisits["lifecycle.prevent_destroy"]) + // encoding/json serializes ShadowCluster.Lifecycle (named, shallower) and + // ignores the promoted ShadowBase.Lifecycle. structwalk visits both. + // Bug: walk visits twice; json emits once. Record the buggy values. + assert.Equal(t, 2, clusVisits["lifecycle.prevent_destroy"], + "structwalk should visit 'lifecycle.prevent_destroy' twice (bug); fix → 1") + assert.Equal(t, true, nestedGet(clusJSON, "lifecycle", "prevent_destroy"), + "encoding/json must emit 'lifecycle.prevent_destroy' from the named field") } // TestShadowedEmbedCausesStructdiffDuplicate shows what happens downstream: -// when both shadowed fields are non-zero and they differ, structdiff emits the -// same path twice — once per shadowed declaration. prepareChanges in the direct -// engine uses a map and so the second entry silently overwrites the first, but -// which value wins is arbitrary and depends on iteration order in the embedding. +// when both shadowed fields are non-zero and differ, structdiff emits the same +// path twice — once per shadowed declaration. prepareChanges in the direct +// engine uses a map, so the second entry silently overwrites the first; which +// value wins is arbitrary. func TestShadowedEmbedCausesStructdiffDuplicate(t *testing.T) { - before := &resources.Pipeline{} - before.BaseResource.ID = "old-base-id" //nolint:staticcheck // explicit: sets the shadowed BaseResource field - before.CreatePipeline.Id = "old-sdk-id" //nolint:staticcheck // explicit: sets the promoted SDK field + before := &ShadowPipeline{} + before.ID = "old-base-id" + before.Id = "old-sdk-id" - after := &resources.Pipeline{} - after.BaseResource.ID = "new-base-id" //nolint:staticcheck // explicit: sets the shadowed BaseResource field - after.CreatePipeline.Id = "new-sdk-id" //nolint:staticcheck // explicit: sets the promoted SDK field + after := &ShadowPipeline{} + after.ID = "new-base-id" + after.Id = "new-sdk-id" changes, err := structdiff.GetStructDiff(before, after, nil) require.NoError(t, err) @@ -111,9 +174,9 @@ func TestShadowedEmbedCausesStructdiffDuplicate(t *testing.T) { // Reporting it twice is the bug; zero is correct. if len(dupes) > 0 { t.Logf("structdiff reports the following paths more than once: %v", dupes) - t.Logf("encoding/json would emit neither entry: they are ambiguous fields") + t.Logf("encoding/json would emit neither: they are ambiguous fields") } - // Assert the current (buggy) behavior so a fix requires updating this test. + // Assert the current (buggy) behaviour so a fix requires updating this test. assert.Equal(t, []string{"id"}, dupes, "expected 'id' to be reported twice (shadowed embeds, both non-zero); "+ "if this fails the fix is working — remove the duplicate from this assertion")