From 2170b36f3e9d27f90bcc1ea1d0fd3b55b5b308e4 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 9 Sep 2026 17:35:50 +0200 Subject: [PATCH 1/3] Fix whole-block plan misclassification via bidirectional rule matching Keep structdiff emitting whole-block changes: when a nested struct is nil on one side it is one change at the block level, which is the right granularity for a whole add/remove. Fix the two bundle-plan consumers that need to relate a rule to such a change: - findMatchingRule is now bidirectional (matchesFieldRule): a rule on foo.bar matches a change at foo, so adding/removing a whole block that contains an immutable field recreates instead of updating. - prepareChanges keeps one level per subtree: when one diff descended to a leaf while the other stayed at the block, the block-level entry wins and inner-field entries are dropped. update_mask is left alone, so the postgres default_endpoint_settings message-mask case stays a documented bug. structdiff and configsync are unchanged, so there is no empty-parent regression. Co-authored-by: Isaac --- .../apps/config-drift-stopped/output.txt | 6 --- .../job_runs/on_bundle_deploy/output.txt | 6 --- .../whole-block-overlap/out.plan.direct.json | 5 --- .../resources/jobs/whole-block-overlap/script | 7 ++- .../jobs/whole-block-overlap/test.toml | 8 ++-- .../telemetry_config_unmanaged/output.txt | 2 +- .../out.plan.direct.json | 5 ++- .../pipelines/recreate-add-whole-block/script | 6 +-- .../recreate-add-whole-block/test.toml | 8 ++-- .../out.plan.direct.json | 5 ++- .../recreate-remove-whole-block/script | 6 +-- .../recreate-remove-whole-block/test.toml | 8 ++-- bundle/direct/bundle_plan.go | 44 ++++++++++++++++++- bundle/direct/bundle_plan_test.go | 9 ++-- 14 files changed, 75 insertions(+), 50 deletions(-) diff --git a/acceptance/bundle/resources/apps/config-drift-stopped/output.txt b/acceptance/bundle/resources/apps/config-drift-stopped/output.txt index 1a33d51a6f6..ad32982f834 100644 --- a/acceptance/bundle/resources/apps/config-drift-stopped/output.txt +++ b/acceptance/bundle/resources/apps/config-drift-stopped/output.txt @@ -54,12 +54,6 @@ Resources: 1 created, 0 changed, 0 deleted, 0 unchanged } ] } - }, - "config.env[0].value": { - "action": "skip", - "reason": "no active deployment", - "old": "original_value", - "new": "changed_value" } } diff --git a/acceptance/bundle/resources/job_runs/on_bundle_deploy/output.txt b/acceptance/bundle/resources/job_runs/on_bundle_deploy/output.txt index 39123f3370f..58049806a38 100644 --- a/acceptance/bundle/resources/job_runs/on_bundle_deploy/output.txt +++ b/acceptance/bundle/resources/job_runs/on_bundle_deploy/output.txt @@ -44,12 +44,6 @@ Plan: 1 to add, 0 to change, 1 to delete, 1 unchanged } } }, - "lifecycle.triggers.on_bundle_deploy": { - "action": "recreate", - "reason": "immutable", - "old": "[UUID]", - "new": "[UUID]" - }, "result_state": { "action": "skip", "reason": "remote_already_set", diff --git a/acceptance/bundle/resources/jobs/whole-block-overlap/out.plan.direct.json b/acceptance/bundle/resources/jobs/whole-block-overlap/out.plan.direct.json index 03edfb9f1d8..f44e2f49b8a 100644 --- a/acceptance/bundle/resources/jobs/whole-block-overlap/out.plan.direct.json +++ b/acceptance/bundle/resources/jobs/whole-block-overlap/out.plan.direct.json @@ -85,11 +85,6 @@ "timezone_id": "America/New_York" } }, - "schedule.timezone_id": { - "action": "update", - "old": "UTC", - "new": "America/New_York" - }, "tasks[task_key='main'].email_notifications": { "action": "skip", "reason": "empty", diff --git a/acceptance/bundle/resources/jobs/whole-block-overlap/script b/acceptance/bundle/resources/jobs/whole-block-overlap/script index c9f858bba49..f98a33fe69a 100644 --- a/acceptance/bundle/resources/jobs/whole-block-overlap/script +++ b/acceptance/bundle/resources/jobs/whole-block-overlap/script @@ -13,9 +13,8 @@ r["schedule"] = None EOF # The local diff is at the leaf (schedule.timezone_id) while the remote diff is at -# the block (schedule = nil vs the configured block). structdiff records the remote -# side as one block-level change instead of descending, so the plan's "changes" -# carries BOTH the whole "schedule" block and the "schedule.timezone_id" leaf -# instead of merging them into leaves. +# the block (schedule = nil vs the configured block). The block-level change wins: +# the plan's "changes" keeps one level per subtree (the whole "schedule" block), not +# an overlapping block + "schedule.timezone_id" leaf pair. trace $CLI bundle plan $CLI bundle plan -o json | nostamp > out.plan.direct.json diff --git a/acceptance/bundle/resources/jobs/whole-block-overlap/test.toml b/acceptance/bundle/resources/jobs/whole-block-overlap/test.toml index 4606dbf9d33..b54b7eeb5c5 100644 --- a/acceptance/bundle/resources/jobs/whole-block-overlap/test.toml +++ b/acceptance/bundle/resources/jobs/whole-block-overlap/test.toml @@ -1,7 +1,7 @@ -# Direct-only: the overlapping parent+child entries are a direct-engine changes-map -# artifact (terraform emits no per-field changes). +# Direct-only: the per-field "changes" map is a direct-engine artifact (terraform +# emits no per-field changes). A local leaf change plus a whole block dropped on the +# remote merge into a single block-level entry (one level per subtree), not an +# overlapping block + child pair. EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] RecordRequests = false Ignore = [".databricks"] - -Badness = "The plan's 'changes' map carries BOTH the whole 'schedule' block and the 'schedule.timezone_id' leaf. The local diff (saved state vs config) is at the leaf while the remote diff (remote vs config) is at the block level (schedule dropped out-of-band), and structdiff records the remote side as one block-level change instead of descending, so the two never merge into leaves." diff --git a/acceptance/bundle/resources/model_serving_endpoints/drift/telemetry_config_unmanaged/output.txt b/acceptance/bundle/resources/model_serving_endpoints/drift/telemetry_config_unmanaged/output.txt index f4c7bbac418..e6092671249 100644 --- a/acceptance/bundle/resources/model_serving_endpoints/drift/telemetry_config_unmanaged/output.txt +++ b/acceptance/bundle/resources/model_serving_endpoints/drift/telemetry_config_unmanaged/output.txt @@ -12,7 +12,7 @@ Resources: 1 created, 0 changed, 0 deleted, 0 unchanged { "telemetry_config": { "action": "skip", - "reason": "backend_default", + "reason": "input_only", "remote": { "telemetry_profile_id": "[UUID]" } diff --git a/acceptance/bundle/resources/pipelines/recreate-add-whole-block/out.plan.direct.json b/acceptance/bundle/resources/pipelines/recreate-add-whole-block/out.plan.direct.json index 155e2f82c9e..345c3fb80fd 100644 --- a/acceptance/bundle/resources/pipelines/recreate-add-whole-block/out.plan.direct.json +++ b/acceptance/bundle/resources/pipelines/recreate-add-whole-block/out.plan.direct.json @@ -5,7 +5,7 @@ "serial": 1, "plan": { "resources.pipelines.my": { - "action": "update", + "action": "recreate", "new_state": { "value": { "channel": "CURRENT", @@ -57,7 +57,8 @@ }, "changes": { "ingestion_definition": { - "action": "update", + "action": "recreate", + "reason": "immutable", "new": { "connection_name": "my_connection", "objects": [ diff --git a/acceptance/bundle/resources/pipelines/recreate-add-whole-block/script b/acceptance/bundle/resources/pipelines/recreate-add-whole-block/script index 1af640a02f5..d137348fea9 100644 --- a/acceptance/bundle/resources/pipelines/recreate-add-whole-block/script +++ b/acceptance/bundle/resources/pipelines/recreate-add-whole-block/script @@ -9,9 +9,9 @@ trace $CLI bundle deploy # markers. connection_name is immutable, so adding it must recreate the pipeline. trace update_file.py databricks.yml "#TO_ADD: " "" -# The direct engine records the whole ingestion_definition block as one 'update' -# entry (missing the recreate rule on the connection_name leaf); terraform records -# 'recreate'. See the per-engine "changes" / "action" in the plan JSON. +# Both engines recreate. structdiff records the added block as one change at +# "ingestion_definition"; bidirectional rule matching relates that to the recreate +# rule on the connection_name leaf. See the per-engine "changes"/"action" in the JSON. $CLI bundle plan -o json | nostamp > out.plan.$DATABRICKS_BUNDLE_ENGINE.json rm -f out.requests.txt diff --git a/acceptance/bundle/resources/pipelines/recreate-add-whole-block/test.toml b/acceptance/bundle/resources/pipelines/recreate-add-whole-block/test.toml index 6a23e467a6a..86f874c8578 100644 --- a/acceptance/bundle/resources/pipelines/recreate-add-whole-block/test.toml +++ b/acceptance/bundle/resources/pipelines/recreate-add-whole-block/test.toml @@ -1,5 +1,5 @@ -# Runs on both engines to show the difference: the direct engine plans this as -# 'update' (see Badness), while terraform correctly recreates. The plan verdict -# is the only divergent output, so it is routed to per-engine out.plan.*.txt. -Badness = "On the direct engine, adding the whole ingestion_definition block is planned as 'update' instead of 'recreate': structdiff records one change at path 'ingestion_definition' instead of descending to 'ingestion_definition.connection_name', so the immutable recreate rule never matches. Terraform recreates as expected." +# Runs on both engines: both recreate (connection_name is immutable). The direct +# plan's per-engine "changes" map (out.plan.direct.json) shows the recreate keyed on +# the ingestion_definition block via bidirectional rule matching; terraform's plan +# JSON just carries the action. Ignore = ["foo.py", ".databricks"] diff --git a/acceptance/bundle/resources/pipelines/recreate-remove-whole-block/out.plan.direct.json b/acceptance/bundle/resources/pipelines/recreate-remove-whole-block/out.plan.direct.json index 55ae2bb0a30..e9d4cb5b6d8 100644 --- a/acceptance/bundle/resources/pipelines/recreate-remove-whole-block/out.plan.direct.json +++ b/acceptance/bundle/resources/pipelines/recreate-remove-whole-block/out.plan.direct.json @@ -5,7 +5,7 @@ "serial": 1, "plan": { "resources.pipelines.my": { - "action": "update", + "action": "recreate", "new_state": { "value": { "channel": "CURRENT", @@ -57,7 +57,8 @@ }, "changes": { "ingestion_definition": { - "action": "update", + "action": "recreate", + "reason": "immutable", "old": { "connection_name": "my_connection", "objects": [ diff --git a/acceptance/bundle/resources/pipelines/recreate-remove-whole-block/script b/acceptance/bundle/resources/pipelines/recreate-remove-whole-block/script index 08d834a886f..416cd19e2df 100644 --- a/acceptance/bundle/resources/pipelines/recreate-remove-whole-block/script +++ b/acceptance/bundle/resources/pipelines/recreate-remove-whole-block/script @@ -9,9 +9,9 @@ trace $CLI bundle deploy # lines. connection_name is immutable, so removing it must recreate the pipeline. grep -v '# TO_DELETE' databricks.yml > databricks.yml.tmp && mv databricks.yml.tmp databricks.yml -# The direct engine records the whole ingestion_definition block as one 'update' -# entry (missing the recreate rule on the connection_name leaf); terraform records -# 'recreate'. See the per-engine "changes" / "action" in the plan JSON. +# Both engines recreate. structdiff records the removed block as one change at +# "ingestion_definition"; bidirectional rule matching relates that to the recreate +# rule on the connection_name leaf. See the per-engine "changes"/"action" in the JSON. $CLI bundle plan -o json | nostamp > out.plan.$DATABRICKS_BUNDLE_ENGINE.json rm -f out.requests.txt diff --git a/acceptance/bundle/resources/pipelines/recreate-remove-whole-block/test.toml b/acceptance/bundle/resources/pipelines/recreate-remove-whole-block/test.toml index 085b0cff5be..86f874c8578 100644 --- a/acceptance/bundle/resources/pipelines/recreate-remove-whole-block/test.toml +++ b/acceptance/bundle/resources/pipelines/recreate-remove-whole-block/test.toml @@ -1,5 +1,5 @@ -# Runs on both engines to show the difference: the direct engine plans this as -# 'update' (see Badness), while terraform correctly recreates. The plan verdict -# is the only divergent output, so it is routed to per-engine out.plan.*.txt. -Badness = "On the direct engine, removing the whole ingestion_definition block is planned as 'update' instead of 'recreate': structdiff records one change at path 'ingestion_definition' instead of descending to 'ingestion_definition.connection_name', so the immutable recreate rule never matches. Terraform recreates as expected." +# Runs on both engines: both recreate (connection_name is immutable). The direct +# plan's per-engine "changes" map (out.plan.direct.json) shows the recreate keyed on +# the ingestion_definition block via bidirectional rule matching; terraform's plan +# JSON just carries the action. Ignore = ["foo.py", ".databricks"] diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index 48d76abe97f..51f089440d1 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -428,6 +428,30 @@ func prepareChanges(ctx context.Context, adapter *dresources.Adapter, localDiff, } } + // Keep one level per subtree. A whole block added or removed shows up as a single + // block-level change, while the other diff (local vs remote) may have descended to + // inner fields of the same subtree. The block-level change already describes them, + // so drop any entry that has an ancestor entry in the map. + for pathStr := range m { + node, err := structpath.ParsePath(pathStr) + if err != nil { + continue + } + for other := range m { + if other == pathStr { + continue + } + ancestor, err := structpath.ParsePath(other) + if err != nil { + continue + } + if node.HasPrefix(ancestor) { + delete(m, pathStr) + break + } + } + } + return m, nil } @@ -550,13 +574,31 @@ func isFieldMissingInRemote(adapter *dresources.Adapter, path *structpath.PathNo func findMatchingRule(path *structpath.PathNode, rules []dresources.FieldRule) (string, bool) { for _, r := range rules { - if path.HasPatternPrefix(r.Field) { + if matchesFieldRule(path, r.Field) { return r.Reason, true } } return "", false } +// matchesFieldRule reports whether a rule targeting the pattern matches a change at +// path, in either direction: +// - path at or below the rule: the field the rule names changed (the usual case). +// - path above the rule: a whole block was added or removed, recorded as one +// block-level change, so the field the rule names is part of it. +// +// The second direction is what lets a rule on ingestion_definition.connection_name +// match a whole-block change recorded at ingestion_definition. +func matchesFieldRule(path *structpath.PathNode, pattern *structpath.PatternNode) bool { + if path.HasPatternPrefix(pattern) { + return true + } + if path.Len() < pattern.Len() { + return path.HasPatternPrefix(pattern.Prefix(path.Len())) + } + return false +} + func shouldSkip(cfg *dresources.ResourceLifecycleConfig, path *structpath.PathNode, ch *deployplan.ChangeDesc) (string, bool) { if cfg == nil { return "", false diff --git a/bundle/direct/bundle_plan_test.go b/bundle/direct/bundle_plan_test.go index c178522f7d7..7147e6f0f4b 100644 --- a/bundle/direct/bundle_plan_test.go +++ b/bundle/direct/bundle_plan_test.go @@ -451,10 +451,9 @@ func TestPrepareChangesWholeBlockOverlap(t *testing.T) { } slices.Sort(keys) - // Unexpected: a coarse "field.a" entry overlaps the fine "field.a.c" entry. - // Probably should be: []string{"field.a.b", "field.a.c"}. - assert.Equal(t, []string{"field.a", "field.a.c"}, keys) - - // The coarse parent entry carries the whole sub-block rather than a leaf value. + // One level per subtree: the local diff produced the field.a.c leaf and the remote + // diff the whole field.a block; the block-level entry wins and the inner leaf is + // dropped, so the merged change set is just the block. + assert.Equal(t, []string{"field.a"}, keys) assert.Equal(t, threeWayInner{B: "old", C: "newc"}, changes["field.a"].New) } From 78a876d7299da77e66a386f73be9b83ca6c66122 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 10 Sep 2026 13:36:46 +0200 Subject: [PATCH 2/3] Scope bidirectional matching to recreate; drop the shared dedup Applying bidirectional matching through the shared findMatchingRule and deduping in prepareChanges both leaked into config-remote-sync (which consumes the plan's ChangeDesc): ignore_remote rules wrongly skipped whole blocks (dropping a job_clusters rename), and the dedup dropped fine-grained ops the split logic needs (leaving a num_workers remnant). So bidirectional matching is now recreate-only (findRecreateRule); ignore/skip rules stay descendant-only, and prepareChanges no longer dedups. The PR now only fixes the recreate misclassification (pipelines/recreate-{add,remove}-whole-block). The plan "one level" overlap and update_mask remain documented follow-ups. Co-authored-by: Isaac --- .../apps/config-drift-stopped/output.txt | 6 ++ .../job_runs/on_bundle_deploy/output.txt | 6 ++ .../whole-block-overlap/out.plan.direct.json | 5 ++ .../resources/jobs/whole-block-overlap/script | 7 ++- .../jobs/whole-block-overlap/test.toml | 8 +-- .../telemetry_config_unmanaged/output.txt | 2 +- bundle/direct/bundle_plan.go | 56 +++++++------------ bundle/direct/bundle_plan_test.go | 9 +-- 8 files changed, 51 insertions(+), 48 deletions(-) diff --git a/acceptance/bundle/resources/apps/config-drift-stopped/output.txt b/acceptance/bundle/resources/apps/config-drift-stopped/output.txt index ad32982f834..1a33d51a6f6 100644 --- a/acceptance/bundle/resources/apps/config-drift-stopped/output.txt +++ b/acceptance/bundle/resources/apps/config-drift-stopped/output.txt @@ -54,6 +54,12 @@ Resources: 1 created, 0 changed, 0 deleted, 0 unchanged } ] } + }, + "config.env[0].value": { + "action": "skip", + "reason": "no active deployment", + "old": "original_value", + "new": "changed_value" } } diff --git a/acceptance/bundle/resources/job_runs/on_bundle_deploy/output.txt b/acceptance/bundle/resources/job_runs/on_bundle_deploy/output.txt index 58049806a38..39123f3370f 100644 --- a/acceptance/bundle/resources/job_runs/on_bundle_deploy/output.txt +++ b/acceptance/bundle/resources/job_runs/on_bundle_deploy/output.txt @@ -44,6 +44,12 @@ Plan: 1 to add, 0 to change, 1 to delete, 1 unchanged } } }, + "lifecycle.triggers.on_bundle_deploy": { + "action": "recreate", + "reason": "immutable", + "old": "[UUID]", + "new": "[UUID]" + }, "result_state": { "action": "skip", "reason": "remote_already_set", diff --git a/acceptance/bundle/resources/jobs/whole-block-overlap/out.plan.direct.json b/acceptance/bundle/resources/jobs/whole-block-overlap/out.plan.direct.json index f44e2f49b8a..03edfb9f1d8 100644 --- a/acceptance/bundle/resources/jobs/whole-block-overlap/out.plan.direct.json +++ b/acceptance/bundle/resources/jobs/whole-block-overlap/out.plan.direct.json @@ -85,6 +85,11 @@ "timezone_id": "America/New_York" } }, + "schedule.timezone_id": { + "action": "update", + "old": "UTC", + "new": "America/New_York" + }, "tasks[task_key='main'].email_notifications": { "action": "skip", "reason": "empty", diff --git a/acceptance/bundle/resources/jobs/whole-block-overlap/script b/acceptance/bundle/resources/jobs/whole-block-overlap/script index f98a33fe69a..c9f858bba49 100644 --- a/acceptance/bundle/resources/jobs/whole-block-overlap/script +++ b/acceptance/bundle/resources/jobs/whole-block-overlap/script @@ -13,8 +13,9 @@ r["schedule"] = None EOF # The local diff is at the leaf (schedule.timezone_id) while the remote diff is at -# the block (schedule = nil vs the configured block). The block-level change wins: -# the plan's "changes" keeps one level per subtree (the whole "schedule" block), not -# an overlapping block + "schedule.timezone_id" leaf pair. +# the block (schedule = nil vs the configured block). structdiff records the remote +# side as one block-level change instead of descending, so the plan's "changes" +# carries BOTH the whole "schedule" block and the "schedule.timezone_id" leaf +# instead of merging them into leaves. trace $CLI bundle plan $CLI bundle plan -o json | nostamp > out.plan.direct.json diff --git a/acceptance/bundle/resources/jobs/whole-block-overlap/test.toml b/acceptance/bundle/resources/jobs/whole-block-overlap/test.toml index b54b7eeb5c5..4606dbf9d33 100644 --- a/acceptance/bundle/resources/jobs/whole-block-overlap/test.toml +++ b/acceptance/bundle/resources/jobs/whole-block-overlap/test.toml @@ -1,7 +1,7 @@ -# Direct-only: the per-field "changes" map is a direct-engine artifact (terraform -# emits no per-field changes). A local leaf change plus a whole block dropped on the -# remote merge into a single block-level entry (one level per subtree), not an -# overlapping block + child pair. +# Direct-only: the overlapping parent+child entries are a direct-engine changes-map +# artifact (terraform emits no per-field changes). EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] RecordRequests = false Ignore = [".databricks"] + +Badness = "The plan's 'changes' map carries BOTH the whole 'schedule' block and the 'schedule.timezone_id' leaf. The local diff (saved state vs config) is at the leaf while the remote diff (remote vs config) is at the block level (schedule dropped out-of-band), and structdiff records the remote side as one block-level change instead of descending, so the two never merge into leaves." diff --git a/acceptance/bundle/resources/model_serving_endpoints/drift/telemetry_config_unmanaged/output.txt b/acceptance/bundle/resources/model_serving_endpoints/drift/telemetry_config_unmanaged/output.txt index e6092671249..f4c7bbac418 100644 --- a/acceptance/bundle/resources/model_serving_endpoints/drift/telemetry_config_unmanaged/output.txt +++ b/acceptance/bundle/resources/model_serving_endpoints/drift/telemetry_config_unmanaged/output.txt @@ -12,7 +12,7 @@ Resources: 1 created, 0 changed, 0 deleted, 0 unchanged { "telemetry_config": { "action": "skip", - "reason": "input_only", + "reason": "backend_default", "remote": { "telemetry_profile_id": "[UUID]" } diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index 51f089440d1..0ca118c1e28 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -428,30 +428,6 @@ func prepareChanges(ctx context.Context, adapter *dresources.Adapter, localDiff, } } - // Keep one level per subtree. A whole block added or removed shows up as a single - // block-level change, while the other diff (local vs remote) may have descended to - // inner fields of the same subtree. The block-level change already describes them, - // so drop any entry that has an ancestor entry in the map. - for pathStr := range m { - node, err := structpath.ParsePath(pathStr) - if err != nil { - continue - } - for other := range m { - if other == pathStr { - continue - } - ancestor, err := structpath.ParsePath(other) - if err != nil { - continue - } - if node.HasPrefix(ancestor) { - delete(m, pathStr) - break - } - } - } - return m, nil } @@ -506,10 +482,10 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change } else if isFieldMissingInRemote(adapter, path) && structdiff.IsEqual(ch.Old, ch.New) { ch.Action = deployplan.Skip ch.Reason = deployplan.ReasonMissingInRemote - } else if reason, ok := findMatchingRule(path, cfg.RecreateOnChanges); ok { + } else if reason, ok := findRecreateRule(path, cfg.RecreateOnChanges); ok { ch.Action = deployplan.Recreate ch.Reason = reason - } else if reason, ok := findMatchingRule(path, generatedCfg.RecreateOnChanges); ok { + } else if reason, ok := findRecreateRule(path, generatedCfg.RecreateOnChanges); ok { ch.Action = deployplan.Recreate ch.Reason = reason } else { @@ -574,22 +550,30 @@ func isFieldMissingInRemote(adapter *dresources.Adapter, path *structpath.PathNo func findMatchingRule(path *structpath.PathNode, rules []dresources.FieldRule) (string, bool) { for _, r := range rules { - if matchesFieldRule(path, r.Field) { + if path.HasPatternPrefix(r.Field) { return r.Reason, true } } return "", false } -// matchesFieldRule reports whether a rule targeting the pattern matches a change at -// path, in either direction: -// - path at or below the rule: the field the rule names changed (the usual case). -// - path above the rule: a whole block was added or removed, recorded as one -// block-level change, so the field the rule names is part of it. -// -// The second direction is what lets a rule on ingestion_definition.connection_name -// match a whole-block change recorded at ingestion_definition. -func matchesFieldRule(path *structpath.PathNode, pattern *structpath.PatternNode) bool { +// findRecreateRule matches recreate rules bidirectionally: in addition to the usual +// descendant match, a rule on foo.bar matches a change recorded at foo, because a +// whole block added or removed is one block-level change and the field the rule +// names is part of it. This is only sound for an escalating action like recreate. +// Suppressing rules (ignore_remote/ignore_local, backend_default, normalize) stay +// descendant-only via findMatchingRule: a whole block that merely contains an +// ignored/defaulted leaf is still a real change and must not be skipped. +func findRecreateRule(path *structpath.PathNode, rules []dresources.FieldRule) (string, bool) { + for _, r := range rules { + if matchesFieldRuleBidirectional(path, r.Field) { + return r.Reason, true + } + } + return "", false +} + +func matchesFieldRuleBidirectional(path *structpath.PathNode, pattern *structpath.PatternNode) bool { if path.HasPatternPrefix(pattern) { return true } diff --git a/bundle/direct/bundle_plan_test.go b/bundle/direct/bundle_plan_test.go index 7147e6f0f4b..c178522f7d7 100644 --- a/bundle/direct/bundle_plan_test.go +++ b/bundle/direct/bundle_plan_test.go @@ -451,9 +451,10 @@ func TestPrepareChangesWholeBlockOverlap(t *testing.T) { } slices.Sort(keys) - // One level per subtree: the local diff produced the field.a.c leaf and the remote - // diff the whole field.a block; the block-level entry wins and the inner leaf is - // dropped, so the merged change set is just the block. - assert.Equal(t, []string{"field.a"}, keys) + // Unexpected: a coarse "field.a" entry overlaps the fine "field.a.c" entry. + // Probably should be: []string{"field.a.b", "field.a.c"}. + assert.Equal(t, []string{"field.a", "field.a.c"}, keys) + + // The coarse parent entry carries the whole sub-block rather than a leaf value. assert.Equal(t, threeWayInner{B: "old", C: "newc"}, changes["field.a"].New) } From 62f46c9ecf06b08754e84c2ceebe928bdd705a85 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 10 Sep 2026 14:39:34 +0200 Subject: [PATCH 3/3] Rename findRecreateRule to findMatchingRuleBidirectional It is a generic bidirectional matcher, not recreate-specific; recreate is just its only current caller. Co-authored-by: Isaac --- bundle/direct/bundle_plan.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index 0ca118c1e28..26582def481 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -482,10 +482,10 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change } else if isFieldMissingInRemote(adapter, path) && structdiff.IsEqual(ch.Old, ch.New) { ch.Action = deployplan.Skip ch.Reason = deployplan.ReasonMissingInRemote - } else if reason, ok := findRecreateRule(path, cfg.RecreateOnChanges); ok { + } else if reason, ok := findMatchingRuleBidirectional(path, cfg.RecreateOnChanges); ok { ch.Action = deployplan.Recreate ch.Reason = reason - } else if reason, ok := findRecreateRule(path, generatedCfg.RecreateOnChanges); ok { + } else if reason, ok := findMatchingRuleBidirectional(path, generatedCfg.RecreateOnChanges); ok { ch.Action = deployplan.Recreate ch.Reason = reason } else { @@ -557,14 +557,14 @@ func findMatchingRule(path *structpath.PathNode, rules []dresources.FieldRule) ( return "", false } -// findRecreateRule matches recreate rules bidirectionally: in addition to the usual -// descendant match, a rule on foo.bar matches a change recorded at foo, because a -// whole block added or removed is one block-level change and the field the rule -// names is part of it. This is only sound for an escalating action like recreate. -// Suppressing rules (ignore_remote/ignore_local, backend_default, normalize) stay -// descendant-only via findMatchingRule: a whole block that merely contains an -// ignored/defaulted leaf is still a real change and must not be skipped. -func findRecreateRule(path *structpath.PathNode, rules []dresources.FieldRule) (string, bool) { +// findMatchingRuleBidirectional matches rules in both directions: the usual +// descendant match, plus a rule on foo.bar matching a change recorded at foo, +// because a whole block added or removed is one block-level change and the field +// the rule names is part of it. Callers must only use this for escalating actions +// (currently recreate): a whole block that merely contains a leaf named by a +// suppressing rule (ignore_remote/ignore_local, backend_default, normalize) is +// still a real change, so those keep the descendant-only findMatchingRule. +func findMatchingRuleBidirectional(path *structpath.PathNode, rules []dresources.FieldRule) (string, bool) { for _, r := range rules { if matchesFieldRuleBidirectional(path, r.Field) { return r.Reason, true