direct: Support configuration for unordered slices; use in permissions to ignore order (#4014)

denik · web-flow · commit 0bc7fe3b8470 · 2025-11-27T15:52:15.000Z
## Changes - Extend structpath to support [key=‘value’] syntax. - Extend structdiff with extra parameter that maps path to keyFunc which is a function that returns key/value pair for a given slice element. They pair is used to identify the slice element. - Use this functionality on permissions resource in direct to solve drift due to backend reordering permissions. ## Why - Prevent remote drift due to backend reordering slice elements. - Make diffs more readable / precise. This is an alternative to always sorting the input, which destroys the references ($resources.jobs.foo.tasks[1] is not valid today because of sorting). In a follow up PR I’ll revert job tasks sorting #3212 and implement KeyedSlices instead.
diff --git a/acceptance/bundle/resources/permissions/jobs/delete_one/cloud/out.plan.direct.txt b/acceptance/bundle/resources/permissions/jobs/delete_one/cloud/out.plan.direct.txt
diff --git a/acceptance/bundle/resources/permissions/jobs/delete_one/cloud/out.plan.terraform.txt b/acceptance/bundle/resources/permissions/jobs/delete_one/cloud/out.plan.terraform.txt
diff --git a/acceptance/bundle/resources/permissions/jobs/delete_one/cloud/output.txt b/acceptance/bundle/resources/permissions/jobs/delete_one/cloud/output.txt
@@ -8,6 +8,7 @@ Updating deployment state...
 Deployment complete!
 
 >>> [CLI] bundle plan
+Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged
 
 === Delete one permission and deploy again
 
diff --git a/acceptance/bundle/resources/permissions/jobs/delete_one/cloud/script b/acceptance/bundle/resources/permissions/jobs/delete_one/cloud/script
@@ -33,8 +33,7 @@ trace $CLI bundle deploy
 # although they are semantically the same. We're not doing the same transformation in direct, because permissions get endpoint uses a different order.
 print_requests | sort_acl_requests > out.requests_create.json
 
-# Badness: because of remote reordering we have drift in direct there
-trace $CLI bundle plan > out.plan.$DATABRICKS_BUNDLE_ENGINE.txt
+trace $CLI bundle plan
 
 job_id=$($CLI bundle summary --output json | jq -r '.resources.jobs.job_with_permissions.id')
 
diff --git a/acceptance/bundle/resources/permissions/jobs/delete_one/local/out.plan_update.direct.json b/acceptance/bundle/resources/permissions/jobs/delete_one/local/out.plan_update.direct.json
@@ -100,7 +100,7 @@
       },
       "changes": {
         "local": {
-          "permissions": {
+          "permissions[user_name='test-dabs-1@databricks.com']": {
             "action": "update"
           }
         }
diff --git a/acceptance/bundle/resources/permissions/jobs/deleted_remotely/out.plan_restore.direct.json b/acceptance/bundle/resources/permissions/jobs/deleted_remotely/out.plan_restore.direct.json
@@ -88,7 +88,10 @@
       },
       "changes": {
         "remote": {
-          "permissions": {
+          "permissions[group_name='data-team']": {
+            "action": "update"
+          },
+          "permissions[user_name='viewer@example.com']": {
             "action": "update"
           }
         }
diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_locally/databricks.yml b/acceptance/bundle/resources/permissions/jobs/reorder_locally/databricks.yml
@@ -0,0 +1,19 @@
+bundle:
+  name: test-bundle
+
+resources:
+  jobs:
+    foo:
+      name: job1
+      tasks:
+        - task_key: main
+          notebook_task:
+            notebook_path: /Workspace/Users/user@example.com/notebook
+            source: WORKSPACE
+      permissions:
+        - {"level": "CAN_VIEW", "user_name": "viewer@example.com"}
+        - level: CAN_MANAGE
+          group_name: data-team
+        - level: CAN_MANAGE
+          service_principal_name: f37d18cd-98a8-4db5-8112-12dd0a6bfe38
+          # PLACEHOLDER
diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_locally/out.test.toml b/acceptance/bundle/resources/permissions/jobs/reorder_locally/out.test.toml
diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_locally/output.txt b/acceptance/bundle/resources/permissions/jobs/reorder_locally/output.txt
@@ -0,0 +1,18 @@
+
+>>> [CLI] bundle deploy
+Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files...
+Deploying resources...
+Updating deployment state...
+Deployment complete!
+
+>>> [CLI] bundle plan
+Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged
+
+>>> [CLI] bundle deploy
+Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files...
+Deploying resources...
+Updating deployment state...
+Deployment complete!
+
+>>> [CLI] bundle plan
+Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged
diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_locally/script b/acceptance/bundle/resources/permissions/jobs/reorder_locally/script
@@ -0,0 +1,7 @@
+trace $CLI bundle deploy
+update_file.py databricks.yml '- {"level": "CAN_VIEW", "user_name": "viewer@example.com"}' ''
+update_file.py databricks.yml '  # PLACEHOLDER' '- {"level": "CAN_VIEW", "user_name": "viewer@example.com"}'
+
+trace $CLI bundle plan
+trace $CLI bundle deploy
+trace $CLI bundle plan
diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_locally/test.toml b/acceptance/bundle/resources/permissions/jobs/reorder_locally/test.toml
@@ -0,0 +1 @@
+RecordRequests = false
diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_remotely/databricks.yml b/acceptance/bundle/resources/permissions/jobs/reorder_remotely/databricks.yml
@@ -0,0 +1,18 @@
+bundle:
+  name: test-bundle
+
+resources:
+  jobs:
+    foo:
+      name: job1
+      tasks:
+        - task_key: main
+          notebook_task:
+            notebook_path: /Workspace/Users/user@example.com/notebook
+            source: WORKSPACE
+      permissions:
+        - {"level": "CAN_VIEW", "user_name": "viewer@example.com"}
+        - level: CAN_MANAGE
+          group_name: data-team
+        - level: CAN_MANAGE
+          service_principal_name: f37d18cd-98a8-4db5-8112-12dd0a6bfe38
diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_remotely/out.test.toml b/acceptance/bundle/resources/permissions/jobs/reorder_remotely/out.test.toml
diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_remotely/output.txt b/acceptance/bundle/resources/permissions/jobs/reorder_remotely/output.txt
@@ -0,0 +1,14 @@
+
+>>> [CLI] bundle deploy
+Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files...
+Deploying resources...
+Updating deployment state...
+Deployment complete!
+
+>>> [CLI] bundle plan
+Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged
+
+>>> [CLI] permissions set jobs [NUMID] --json @reorder.json
+
+>>> [CLI] bundle plan
+Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged
diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_remotely/reorder.json b/acceptance/bundle/resources/permissions/jobs/reorder_remotely/reorder.json
@@ -0,0 +1,8 @@
+{
+  "access_control_list": [
+      {"permission_level": "CAN_MANAGE", "group_name": "data-team"},
+      {"permission_level": "CAN_VIEW", "user_name": "viewer@example.com"},
+      {"permission_level": "IS_OWNER", "user_name": "tester@databricks.com"},
+      {"permission_level": "CAN_MANAGE", "service_principal_name": "f37d18cd-98a8-4db5-8112-12dd0a6bfe38"}
+  ]
+}
diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_remotely/script b/acceptance/bundle/resources/permissions/jobs/reorder_remotely/script
@@ -0,0 +1,8 @@
+trace $CLI bundle deploy
+trace $CLI bundle plan | contains.py "2 unchanged"
+
+job_id=$($CLI bundle summary --output json | jq -r '.resources.jobs.foo.id')
+
+trace $CLI permissions set jobs $job_id --json @reorder.json > /dev/null
+
+trace $CLI bundle plan | contains.py "2 unchanged"
diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_remotely/test.toml b/acceptance/bundle/resources/permissions/jobs/reorder_remotely/test.toml
@@ -0,0 +1 @@
+RecordRequests = false
diff --git a/acceptance/bundle/resources/permissions/jobs/update/out.plan_delete_one.direct.json b/acceptance/bundle/resources/permissions/jobs/update/out.plan_delete_one.direct.json
@@ -92,7 +92,7 @@
       },
       "changes": {
         "local": {
-          "permissions": {
+          "permissions[group_name='data-team']": {
             "action": "update"
           }
         }
diff --git a/acceptance/bundle/resources/permissions/jobs/update/out.plan_update.direct.json b/acceptance/bundle/resources/permissions/jobs/update/out.plan_update.direct.json
@@ -96,7 +96,7 @@
       },
       "changes": {
         "local": {
-          "permissions[0].permission_level": {
+          "permissions[user_name='viewer@example.com'].permission_level": {
             "action": "update"
           }
         }
diff --git a/acceptance/bundle/resources/permissions/pipelines/update/out.plan_delete_one.direct.json b/acceptance/bundle/resources/permissions/pipelines/update/out.plan_delete_one.direct.json
@@ -72,7 +72,7 @@
       },
       "changes": {
         "local": {
-          "permissions": {
+          "permissions[group_name='data-team']": {
             "action": "update"
           }
         }
diff --git a/acceptance/bundle/resources/permissions/pipelines/update/out.plan_update.direct.json b/acceptance/bundle/resources/permissions/pipelines/update/out.plan_update.direct.json
@@ -76,7 +76,7 @@
       },
       "changes": {
         "local": {
-          "permissions[0].permission_level": {
+          "permissions[user_name='viewer@example.com'].permission_level": {
             "action": "update"
           }
         }
diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go
@@ -144,7 +144,7 @@ func (b *DeploymentBundle) CalculatePlan(ctx context.Context, client *databricks
 		// for integers: compare 0 with actual object ID. As long as real object IDs are never 0 we're good.
 		// Once we add non-id fields or add per-field details to "bundle plan", we must read dynamic data and deal with references as first class citizen.
 		// This means distinguishing between 0 that are actually object ids and 0 that are there because typed struct integer cannot contain ${...} string.
-		localDiff, err := structdiff.GetStructDiff(savedState, entry.NewState.Value)
+		localDiff, err := structdiff.GetStructDiff(savedState, entry.NewState.Value, adapter.KeyedSlices())
 		if err != nil {
 			logdiag.LogError(ctx, fmt.Errorf("%s: diffing local state: %w", errorPrefix, err))
 			return false
@@ -187,7 +187,7 @@ func (b *DeploymentBundle) CalculatePlan(ctx context.Context, client *databricks
 				return false
 			}
 
-			remoteDiff, err := structdiff.GetStructDiff(savedState, remoteStateComparable)
+			remoteDiff, err := structdiff.GetStructDiff(savedState, remoteStateComparable, adapter.KeyedSlices())
 			if err != nil {
 				logdiag.LogError(ctx, fmt.Errorf("%s: diffing remote state: %w", errorPrefix, err))
 				return false
diff --git a/bundle/direct/dresources/adapter.go b/bundle/direct/dresources/adapter.go
@@ -80,6 +80,10 @@ type IResource interface {
 
 	// [Optional] WaitAfterUpdate waits for the resource to become ready after update. Returns optionally updated remote state.
 	WaitAfterUpdate(ctx context.Context, newState any) (remoteState any, e error)
+
+	// [Optional] KeyedSlices returns a map from path patterns to KeyFunc for comparing slices by key instead of by index.
+	// Example: func (*ResourcePermissions) KeyedSlices(state *PermissionsState) map[string]any
+	KeyedSlices() map[string]any
 }
 
 // Adapter wraps resource implementation, validates signatures and type consistency across methods
@@ -102,6 +106,7 @@ type Adapter struct {
 
 	fieldTriggersLocal  map[string]deployplan.ActionType
 	fieldTriggersRemote map[string]deployplan.ActionType
+	keyedSlices         map[string]any
 }
 
 func NewAdapter(typedNil any, client *databricks.WorkspaceClient) (*Adapter, error) {
@@ -131,6 +136,7 @@ func NewAdapter(typedNil any, client *databricks.WorkspaceClient) (*Adapter, err
 		classifyChange:      nil,
 		fieldTriggersLocal:  map[string]deployplan.ActionType{},
 		fieldTriggersRemote: map[string]deployplan.ActionType{},
+		keyedSlices:         nil,
 	}
 
 	err = adapter.initMethods(impl)
@@ -189,6 +195,16 @@ func loadFieldTriggers(triggerCall *calladapt.BoundCaller, isLocal bool) (map[st
 	return result, nil
 }
 
+// loadKeyedSlices validates and calls KeyedSlices method, returning the resulting map.
+func loadKeyedSlices(call *calladapt.BoundCaller) (map[string]any, error) {
+	outs, err := call.Call()
+	if err != nil {
+		return nil, fmt.Errorf("failed to call KeyedSlices: %w", err)
+	}
+	result := outs[0].(map[string]any)
+	return result, nil
+}
+
 func (a *Adapter) initMethods(resource any) error {
 	err := calladapt.EnsureNoExtraMethods(resource, calladapt.TypeOf[IResource]())
 	if err != nil {
@@ -252,6 +268,17 @@ func (a *Adapter) initMethods(resource any) error {
 		return err
 	}
 
+	keyedSlicesCall, err := calladapt.PrepareCall(resource, calladapt.TypeOf[IResource](), "KeyedSlices")
+	if err != nil {
+		return err
+	}
+	if keyedSlicesCall != nil {
+		a.keyedSlices, err = loadKeyedSlices(keyedSlicesCall)
+		if err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 
@@ -582,6 +609,12 @@ func (a *Adapter) ClassifyChange(change structdiff.Change, remoteState any, isLo
 	return actionType, nil
 }
 
+// KeyedSlices returns a map from path patterns to KeyFunc for comparing slices by key.
+// If the resource doesn't implement KeyedSlices, returns nil.
+func (a *Adapter) KeyedSlices() map[string]any {
+	return a.keyedSlices
+}
+
 // prepareCallRequired prepares a call and ensures the method is found.
 func prepareCallRequired(resource any, methodName string) (*calladapt.BoundCaller, error) {
 	caller, err := calladapt.PrepareCall(resource, calladapt.TypeOf[IResource](), methodName)
diff --git a/bundle/direct/dresources/all_test.go b/bundle/direct/dresources/all_test.go
@@ -470,7 +470,7 @@ func testCRUD(t *testing.T, group string, adapter *Adapter, client *databricks.W
 	if remoteStateFromUpdate != nil {
 		remappedStateFromUpdate, err := adapter.RemapState(remoteStateFromUpdate)
 		require.NoError(t, err)
-		changes, err := structdiff.GetStructDiff(remappedState, remappedStateFromUpdate)
+		changes, err := structdiff.GetStructDiff(remappedState, remappedStateFromUpdate, nil)
 		require.NoError(t, err)
 		// Filter out timestamp fields that are expected to differ in value
 		var relevantChanges []structdiff.Change
diff --git a/bundle/direct/dresources/permissions.go b/bundle/direct/dresources/permissions.go
@@ -85,6 +85,25 @@ func (*ResourcePermissions) PrepareState(s *PermissionsState) *PermissionsState
 	return s
 }
 
+func accessControlRequestKey(x iam.AccessControlRequest) (string, string) {
+	if x.UserName != "" {
+		return "user_name", x.UserName
+	}
+	if x.ServicePrincipalName != "" {
+		return "service_principal_name", x.ServicePrincipalName
+	}
+	if x.GroupName != "" {
+		return "group_name", x.GroupName
+	}
+	return "", ""
+}
+
+func (*ResourcePermissions) KeyedSlices() map[string]any {
+	return map[string]any{
+		"permissions": accessControlRequestKey,
+	}
+}
+
 // parsePermissionsID extracts the object type and ID from a permissions ID string.
 // Handles both 3-part IDs ("/jobs/123") and 4-part IDs ("/sql/warehouses/uuid").
 func parsePermissionsID(id string) (extractedType, extractedID string, err error) {
diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go
@@ -460,7 +460,7 @@ func TestSet(t *testing.T) {
 			require.NoError(t, err)
 
 			// Compare the actual changes using structdiff
-			changes, err := structdiff.GetStructDiff(original, target)
+			changes, err := structdiff.GetStructDiff(original, target, nil)
 			require.NoError(t, err)
 			assert.Equal(t, tt.expectedChanges, changes)
 		})
diff --git a/libs/structs/structdiff/bench_test.go b/libs/structs/structdiff/bench_test.go
@@ -19,7 +19,7 @@ func bench(b *testing.B, job1, job2 string) {
 
 	b.ResetTimer()
 	for range b.N {
-		changes, err := GetStructDiff(&x, &y)
+		changes, err := GetStructDiff(&x, &y, nil)
 		if err != nil {
 			b.Fatalf("error: %s", err)
 		}
diff --git a/libs/structs/structdiff/diff.go b/libs/structs/structdiff/diff.go
diff --git a/libs/structs/structdiff/diff_test.go b/libs/structs/structdiff/diff_test.go
diff --git a/libs/structs/structdiff/jobsettings_test.go b/libs/structs/structdiff/jobsettings_test.go
diff --git a/libs/structs/structpath/path.go b/libs/structs/structpath/path.go
diff --git a/libs/structs/structpath/path_test.go b/libs/structs/structpath/path_test.go
diff --git a/libs/structs/structvar/structvar_test.go b/libs/structs/structvar/structvar_test.go

Original file line number	Diff line number	Diff line change
`@@ -100,7 +100,7 @@`
`100`	`100`	`},`
`101`	`101`	`"changes": {`
`102`	`102`	`"local": {`
`103`		`- "permissions": {`
	`103`	`+ "permissions[user_name='[email protected]']": {`
`104`	`104`	`"action": "update"`
`105`	`105`	`}`
`106`	`106`	`}`
Original file line number	Diff line number	Diff line change
`@@ -88,7 +88,10 @@`
`88`	`88`	`},`
`89`	`89`	`"changes": {`
`90`	`90`	`"remote": {`
`91`		`- "permissions": {`
	`91`	`+ "permissions[group_name='data-team']": {`
	`92`	`+ "action": "update"`
	`93`	`+ },`
	`94`	`+ "permissions[user_name='[email protected]']": {`
`92`	`95`	`"action": "update"`
`93`	`96`	`}`
`94`	`97`	`}`
-Original file line number
+Diff line change
@@ @@ -0,0 +1,7 @@ @@
 +trace $CLI bundle deploy
 +update_file.py databricks.yml '- {"level": "CAN_VIEW", "user_name": "[email protected]"}' ''
 +update_file.py databricks.yml '  # PLACEHOLDER' '- {"level": "CAN_VIEW", "user_name": "[email protected]"}'
++
 +trace $CLI bundle plan
 +trace $CLI bundle deploy
 +trace $CLI bundle plan