direct: Ignore changes between nulls and empty slices/maps (#4313)

## Why
Although these changes can appear in configs, they cannot be represented
by SDK's structs, so they cannot be persisted or appear in remote reply,
thus causing a permanent drift.

Fixes #4121

## Tests
Existing and new acceptance tests.

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: denik

acceptance/bundle/resources/jobs/on_failure_empty_slice/out.plan.direct.json

@@ -1,6 +1,7 @@
 {
   "email_notifications.on_failure": {
-    "action": "update",
+    "action": "skip",
+    "reason": "empty_slice",
     "new": []
   },
   "tasks[task_key='usage_logs_grants'].notebook_task.source": {

acceptance/bundle/resources/jobs/tags_empty_map/databricks.yml

@@ -0,0 +1,8 @@
+bundle:
+  name: tags_empty_map
+
+resources:
+  jobs:
+    test_job:
+      name: Test Job with empty tags
+      tags: {}

acceptance/bundle/resources/jobs/tags_empty_map/out.plan.direct.json

@@ -0,0 +1,22 @@
+{
+  "email_notifications": {
+    "action": "skip",
+    "reason": "server_side_default",
+    "remote": {}
+  },
+  "tags": {
+    "action": "skip",
+    "reason": "empty_map",
+    "new": {}
+  },
+  "timeout_seconds": {
+    "action": "skip",
+    "reason": "server_side_default",
+    "remote": 0
+  },
+  "webhook_notifications": {
+    "action": "skip",
+    "reason": "server_side_default",
+    "remote": {}
+  }
+}

acceptance/bundle/resources/jobs/tags_empty_map/out.plan.terraform.json

@@ -0,0 +1,9 @@
+{
+  "plan_version": 2,
+  "cli_version": "[DEV_VERSION]",
+  "plan": {
+    "resources.jobs.test_job": {
+      "action": "skip"
+    }
+  }
+}

acceptance/bundle/resources/jobs/tags_empty_map/out.test.toml

@@ -0,0 +1,13 @@
+
+>>> [CLI] bundle plan
+create jobs.test_job
+
+Plan: 1 to add, 0 to change, 0 to delete, 0 unchanged
+
+>>> [CLI] bundle deploy
+Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/tags_empty_map/default/files...
+Deploying resources...
+Updating deployment state...
+Deployment complete!
+
+>>> [CLI] bundle plan -o json

acceptance/bundle/resources/jobs/tags_empty_map/output.txt

@@ -0,0 +1,3 @@
+trace $CLI bundle plan
+trace $CLI bundle deploy
+trace $CLI bundle plan -o json | jq '.plan."resources.jobs.test_job".changes // .' > out.plan.$DATABRICKS_BUNDLE_ENGINE.json

acceptance/bundle/resources/jobs/tags_empty_map/script

@@ -0,0 +1 @@
+RecordRequests = false

acceptance/bundle/resources/jobs/tags_empty_map/test.toml

@@ -93,6 +93,8 @@ const (
 	ReasonRemoteAlreadySet  = "remote_already_set"
 	ReasonBuiltinRule       = "builtin_rule"
 	ReasonConfigOnly        = "config_only"
+	ReasonEmptySlice        = "empty_slice"
+	ReasonEmptyMap          = "empty_map"
 )
 
 // HasChange checks if there are any changes for fields with the given prefix.

bundle/deployplan/plan.go

@@ -368,17 +368,28 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change
 			return err
 		}
 
-		if ch.New == nil && ch.Old == nil && ch.Remote != nil && path.IsDotString() {
+		if structdiff.IsEqual(ch.Remote, ch.New) {
+			ch.Action = deployplan.Skip
+			ch.Reason = deployplan.ReasonRemoteAlreadySet
+		} else if isEmptySlice(ch.Old, ch.New, ch.Remote) {
+			// Empty slice in config should not cause drift when remote has values
+			ch.Action = deployplan.Skip
+			ch.Reason = deployplan.ReasonEmptySlice
+		} else if isEmptyMap(ch.Old, ch.New, ch.Remote) {
+			// Empty map in config should not cause drift when remote has values
+			ch.Action = deployplan.Skip
+			ch.Reason = deployplan.ReasonEmptyMap
+		} else if action := shouldIgnore(cfg, pathString); action != deployplan.Undefined {
+			ch.Action = action
+			ch.Reason = deployplan.ReasonBuiltinRule
+		} else if ch.New == nil && ch.Old == nil && ch.Remote != nil && path.IsDotString() {
 			// The field was not set by us, but comes from the remote state.
 			// This could either be server-side default or a policy.
 			// In any case, this is not a change we should react to.
 			// Note, we only consider struct fields here. Adding/removing elements to/from maps and slices should trigger updates.
 			ch.Action = deployplan.Skip
 			ch.Reason = deployplan.ReasonServerSideDefault
-		} else if structdiff.IsEqual(ch.Remote, ch.New) {
-			ch.Action = deployplan.Skip
-			ch.Reason = deployplan.ReasonRemoteAlreadySet
-		} else if action := getActionFromConfig(cfg, pathString); action != deployplan.Undefined {
+		} else if action := shouldUpdateOrRecreate(cfg, pathString); action != deployplan.Undefined {
 			ch.Action = action
 			ch.Reason = deployplan.ReasonBuiltinRule
 		} else {
@@ -394,9 +405,7 @@ func addPerFieldActions(ctx context.Context, adapter *dresources.Adapter, change
 	return nil
 }
 
-// getActionFromConfig returns the action for a field path based on resource config.
-// Returns Undefined if no config applies.
-func getActionFromConfig(cfg *dresources.ResourceLifecycleConfig, pathString string) deployplan.ActionType {
+func shouldIgnore(cfg *dresources.ResourceLifecycleConfig, pathString string) deployplan.ActionType {
 	if cfg == nil {
 		return deployplan.Undefined
 	}
@@ -405,6 +414,13 @@ func getActionFromConfig(cfg *dresources.ResourceLifecycleConfig, pathString str
 			return deployplan.Skip
 		}
 	}
+	return deployplan.Undefined
+}
+
+func shouldUpdateOrRecreate(cfg *dresources.ResourceLifecycleConfig, pathString string) deployplan.ActionType {
+	if cfg == nil {
+		return deployplan.Undefined
+	}
 	for _, p := range cfg.RecreateOnChanges {
 		if structpath.HasPrefix(pathString, p.String()) {
 			return deployplan.Recreate
@@ -418,6 +434,36 @@ func getActionFromConfig(cfg *dresources.ResourceLifecycleConfig, pathString str
 	return deployplan.Undefined
 }
 
+// Empty slices and maps cannot be represented in proto and because of that they cannot be represented
+// by SDK's JSON encoder. However, they can be provided by users in the config and can be represented in
+// Bundle struct (currently libs/structs and libs/dyn use ForceSendFields for maps and slices, unlike SDK).
+// Thus we get permanent drift because we see that new config is [] but in the state it is omitted.
+func isEmptySlice(values ...any) bool {
+	for _, v := range values {
+		if v == nil {
+			continue
+		}
+		rv := reflect.ValueOf(v)
+		if rv.Kind() != reflect.Slice || rv.Len() != 0 {
+			return false
+		}
+	}
+	return true
+}
+
+func isEmptyMap(values ...any) bool {
+	for _, v := range values {
+		if v == nil {
+			continue
+		}
+		rv := reflect.ValueOf(v)
+		if rv.Kind() != reflect.Map || rv.Len() != 0 {
+			return false
+		}
+	}
+	return true
+}
+
 // TODO: calling this "Local" is not right, it can resolve "id" and remote refrences for "skip" targets
 func (b *DeploymentBundle) LookupReferenceLocal(ctx context.Context, path *structpath.PathNode) (any, error) {
 	// TODO: Prefix(3) assumes resources.jobs.foo but not resources.jobs.foo.permissions