Match keyed slices by value in structdiff, ignoring the key field

.nextchanges/bundles/6732.md

@@ -0,0 +1 @@
+* direct: Fix a spurious `permissions` update reported on every plan and deploy when a service principal is declared under `user_name` and the Permissions API returns it as `service_principal_name`. ([#6732](https://github.com/databricks/cli/pull/6732))

acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/databricks.yml.tmpl

@@ -0,0 +1,18 @@
+bundle:
+  name: dashboard-sp-perm-$UNIQUE_NAME
+
+resources:
+  dashboards:
+    foo:
+      display_name: test-dashboard-$UNIQUE_NAME
+      warehouse_id: $TEST_DEFAULT_WAREHOUSE_ID
+      serialized_dashboard: '{"pages":[{"name":"page1","displayName":"Page 1"}]}'
+      permissions:
+        # A service principal declared under user_name using its application-ID UUID.
+        # The Permissions API resolves the UUID to a service principal and returns it
+        # as service_principal_name on GET, so desired (user_name) and remote
+        # (service_principal_name) name the same principal under different fields. The
+        # plan matches the entry by value and treats that field difference as no
+        # change, so the deploy converges instead of reporting a perpetual update.
+        - level: CAN_MANAGE
+          user_name: aaaaaaaa-bbbb-4ccc-dddd-eeeeeeeeeeee

acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/output.txt

@@ -0,0 +1,35 @@
+
+=== Deploy the bundle
+>>> [CLI] bundle deploy
+Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/dashboard-sp-perm-[UNIQUE_NAME]/default/files...
+Created dashboards.foo
+Created dashboards.foo.permissions
+Files: 4 uploaded, 0 deleted
+Resources: 2 created, 0 changed, 0 deleted, 0 unchanged
+
+=== Re-plan after deploy (expected: no changes)
+>>> [CLI] bundle plan
+Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged
+
+=== Permissions node of the JSON plan{
+  "action": "skip",
+  "changes": null
+}
+
+=== Lower the permission level: real drift is still detected
+>>> [CLI] bundle plan
+update dashboards.foo.permissions
+
+Plan: 0 to add, 1 to change, 0 to delete, 1 unchanged
+
+=== Permissions node of the JSON plan{
+  "action": "update",
+  "changes": {
+    "[user_name='[UUID]'].level": {
+      "action": "update",
+      "old": "CAN_MANAGE",
+      "new": "CAN_READ",
+      "remote": "CAN_MANAGE"
+    }
+  }
+}

acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/script

@@ -0,0 +1,23 @@
+envsubst < databricks.yml.tmpl > databricks.yml
+
+title "Deploy the bundle"
+trace $CLI bundle deploy
+replace_ids.py
+
+# The service principal is declared under user_name but the API returns it as
+# service_principal_name. The entry is matched by value and the field difference is
+# relaxed, so a re-plan reports no changes rather than a perpetual no-op "update".
+title "Re-plan after deploy (expected: no changes)"
+trace $CLI bundle plan
+
+title "Permissions node of the JSON plan"
+$CLI bundle plan -o json | jq '.plan["resources.dashboards.foo.permissions"] | {action, changes}'
+
+# A genuine change to the same principal must still be detected: lowering the level
+# leaves the principal-field difference relaxed but reports the level update.
+title "Lower the permission level: real drift is still detected"
+update_file.py databricks.yml "CAN_MANAGE" "CAN_READ"
+trace $CLI bundle plan
+
+title "Permissions node of the JSON plan"
+$CLI bundle plan -o json | jq '.plan["resources.dashboards.foo.permissions"] | {action, changes}'

acceptance/bundle/resources/permissions/dashboards/sp_declared_as_user/test.toml

@@ -0,0 +1,6 @@
+# Reproduction of a false-positive permissions "update" after migrating to the
+# direct engine. Direct-only: the customer hit this on the direct engine and the
+# JSON plan format is direct-specific.
+EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"]
+
+Ignore = [".databricks", "databricks.yml"]

bundle/direct/bundle_plan.go

@@ -424,6 +424,14 @@ func getMaxAction(m map[string]*deployplan.ChangeDesc) deployplan.ActionType {
 func prepareChanges(ctx context.Context, adapter *dresources.Adapter, localDiff, remoteDiff []structdiff.Change, oldState, remoteState any) (deployplan.Changes, error) {
 	m := make(deployplan.Changes)
 
+	// A keyed-slice element is matched by value, so the local diff (against saved
+	// state) and the remote diff (against the backend) can label the same element's
+	// key field differently — e.g. a principal saved under user_name but read back as
+	// service_principal_name. Index by the key-field-agnostic path so the two views
+	// merge into one change instead of appearing twice; the display key stays the
+	// first (local) path.
+	displayByKey := make(map[string]string)
+
 	for _, ch := range localDiff {
 		e := deployplan.ChangeDesc{
 			Old: ch.Old,
@@ -433,31 +441,35 @@ func prepareChanges(ctx context.Context, adapter *dresources.Adapter, localDiff,
 			// We cannot assume e.Remote is the same as config: if the whole struct is missing, there might be diff entry for parent
 			e.Remote, _ = structaccess.Get(remoteState, ch.Path)
 		}
-		m[ch.Path.String()] = &e
+		display := ch.Path.String()
+		m[display] = &e
+		displayByKey[ch.Path.KeyValueAgnosticString()] = display
 	}
 
 	for _, ch := range remoteDiff {
-		entry := m[ch.Path.String()]
-		if entry == nil {
-			// we have difference for remoteState but not difference for localState
-			// from remoteDiff we can find out remote value (ch.Old) and new config value (ch.New) but we don't know oldState value
-			oldStateVal, err := structaccess.Get(oldState, ch.Path)
-			_, isNotFound := errors.AsType[*structaccess.NotFoundError](err)
-			if err != nil && !isNotFound {
-				log.Debugf(ctx, "Constructing diff: accessing %q on %T: %s", ch.Path, oldState, err)
-			}
-			m[ch.Path.String()] = &deployplan.ChangeDesc{
-				Old:    oldStateVal,
-				New:    ch.New,
-				Remote: ch.Old,
-			}
-		} else {
+		if display, ok := displayByKey[ch.Path.KeyValueAgnosticString()]; ok {
+			entry := m[display]
 			entry.Remote = ch.Old
 			if !structdiff.IsEqual(entry.New, ch.New) {
 				// this is not fatal (may result in unexpected drift or undetected change but not incorrect deploy), but good to log this
 				log.Warnf(ctx, "unexpected local and remote diffs (%T, %T); entry=%v ch=%v", entry.New, ch.New, entry, ch)
 			}
+			continue
+		}
+		// we have difference for remoteState but not difference for localState
+		// from remoteDiff we can find out remote value (ch.Old) and new config value (ch.New) but we don't know oldState value
+		oldStateVal, err := structaccess.Get(oldState, ch.Path)
+		_, isNotFound := errors.AsType[*structaccess.NotFoundError](err)
+		if err != nil && !isNotFound {
+			log.Debugf(ctx, "Constructing diff: accessing %q on %T: %s", ch.Path, oldState, err)
+		}
+		display := ch.Path.String()
+		m[display] = &deployplan.ChangeDesc{
+			Old:    oldStateVal,
+			New:    ch.New,
+			Remote: ch.Old,
 		}
+		displayByKey[ch.Path.KeyValueAgnosticString()] = display
 	}
 
 	return m, nil

bundle/direct/dresources/adapter.go

@@ -109,6 +109,9 @@ type IResource interface {
 	WaitAfterDelete(ctx context.Context, id string) error
 
 	// [Optional] KeyedSlices returns a map from path patterns to KeyFunc for comparing slices by key instead of by index.
+	// Elements are matched by the KeyFunc's value, not its field, and a matched element's own key field is not
+	// diffed, so the same identity under a different field (e.g. user_name vs service_principal_name) compares
+	// equal. See structdiff.KeyFunc for the full semantics.
 	// Example: func (*ResourcePermissions) KeyedSlices(state *PermissionsState) map[string]any
 	KeyedSlices() map[string]any
 

libs/structs/structdiff/diff.go

@@ -23,8 +23,14 @@ type Change struct {
 //   - func(T) (string, string) - typed function for specific element type T
 //   - func(any) (string, string) - generic function accepting any element
 //
-// The function returns (keyField, keyValue). The keyField is typically a field name
-// like "task_key", and keyValue is the value that uniquely identifies the element.
+// The function returns (keyField, keyValue). keyValue is the identity: elements are
+// matched across the two sides by keyValue alone. keyField is only used to render
+// the path (e.g. "task_key" -> [task_key='...']) and may vary between elements — a
+// permission, for instance, keys on "user_name", "service_principal_name", or
+// "group_name" depending on which is set. Because keyValue is the identity, a matched
+// element's own key field is never diffed: the same value carried under a different
+// key field (e.g. user_name vs service_principal_name for one principal) compares
+// equal rather than reporting a spurious change.
 type KeyFunc = any
 
 // keyFuncCaller wraps a KeyFunc and provides a type-checked Call method.
@@ -70,8 +76,11 @@ type diffContext struct {
 //
 // The sliceKeys parameter maps path patterns to functions that extract
 // key field/value pairs from slice elements. When provided, slices at matching
-// paths are compared as maps keyed by (keyField, keyValue) instead of by index.
-// Path patterns use dot notation (e.g., "tasks" or "job.tasks").
+// paths are matched by key value instead of by index (the key field is used only
+// to render the path). Because the value identifies the element, a diff on the
+// element's own key field is not a real change and is dropped: this lets the same
+// identity carried under different fields (e.g. user_name vs service_principal_name)
+// compare equal. Path patterns use dot notation (e.g., "tasks" or "job.tasks").
 // The [*] wildcard matches any slice index in the path.
 // Note, key wildcard is not supported yet ("a.*.c")
 // Pass nil if no slice key functions are needed.
@@ -366,9 +375,23 @@ func validateKeyFuncElementType(seq reflect.Value, expected reflect.Type) error
 	return nil
 }
 
+// appendSkippingKeyFields appends pairChanges to changes, dropping any change on an
+// element's own key field: a direct child of node whose field name is keyField1 or
+// keyField2. See the call site for why such a diff is not a real change.
+func appendSkippingKeyFields(changes *[]Change, pairChanges []Change, node *structpath.PathNode, keyField1, keyField2 string) {
+	for _, ch := range pairChanges {
+		if ch.Path.Parent() == node {
+			if field, ok := ch.Path.StringKey(); ok && (field == keyField1 || field == keyField2) {
+				continue
+			}
+		}
+		*changes = append(*changes, ch)
+	}
+}
+
 // diffSliceByKey compares two slices using the provided key function.
-// Elements are matched by their (keyField, keyValue) pairs instead of by index.
-// Duplicate keys are allowed and matched in order.
+// Elements are matched by their key value instead of by index (keyField is only
+// used to render the path). Duplicate keys are allowed and matched in order.
 func diffSliceByKey(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect.Value, keyFunc KeyFunc, changes *[]Change) error {
 	caller, err := newKeyFuncCaller(keyFunc)
 	if err != nil {
@@ -427,9 +450,16 @@ func diffSliceByKey(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect.
 		minLen := min(len(list1), len(list2))
 		for i := range minLen {
 			node := structpath.NewKeyValue(path, keyField, keyValue)
-			if err := diffValues(ctx, node, list1[i].value, list2[i].value, changes); err != nil {
+			var pairChanges []Change
+			if err := diffValues(ctx, node, list1[i].value, list2[i].value, &pairChanges); err != nil {
 				return err
 			}
+			// Elements are matched by key value alone (keyField is only for display), so a
+			// diff on an element's own key field means the two sides carry the same identity
+			// under different fields (e.g. a principal returned as service_principal_name but
+			// declared as user_name). The identity is unchanged, so drop that field diff;
+			// non-key fields still diff normally.
+			appendSkippingKeyFields(changes, pairChanges, node, list1[i].keyField, list2[i].keyField)
 		}
 
 		// Handle extra elements in old (deleted)

libs/structs/structdiff/diff_test.go

@@ -602,6 +602,67 @@ func TestGetStructDiffEmbedTagWithKeyFunc(t *testing.T) {
 	}
 }
 
+// principal has two interchangeable identity fields (like a permission's
+// user_name / service_principal_name) plus a non-key field.
+type principal struct {
+	UserName string `json:"user_name,omitempty"`
+	SpName   string `json:"service_principal_name,omitempty"`
+	Level    string `json:"level,omitempty"`
+}
+
+type principalContainer struct {
+	ObjectID      string      `json:"object_id"`
+	EmbeddedSlice []principal `json:"items,omitempty"`
+}
+
+// principalKey returns a varying key field depending on which identity field is set.
+func principalKey(p principal) (string, string) {
+	if p.UserName != "" {
+		return "user_name", p.UserName
+	}
+	return "service_principal_name", p.SpName
+}
+
+// TestGetStructDiffKeyFieldSwap covers a KeyFunc whose key field varies per element:
+// the same identity value carried under a different field is not a change, but a
+// non-key field still diffs.
+func TestGetStructDiffKeyFieldSwap(t *testing.T) {
+	sliceKeys := map[string]KeyFunc{"": principalKey}
+
+	tests := []struct {
+		name string
+		a, b principalContainer
+		want []ResolvedChange
+	}{
+		{
+			name: "field swap only is not a change",
+			a:    principalContainer{EmbeddedSlice: []principal{{SpName: "X", Level: "CAN_MANAGE"}}},
+			b:    principalContainer{EmbeddedSlice: []principal{{UserName: "X", Level: "CAN_MANAGE"}}},
+			want: nil,
+		},
+		{
+			name: "field swap with level change reports only the level",
+			a:    principalContainer{EmbeddedSlice: []principal{{SpName: "X", Level: "CAN_MANAGE"}}},
+			b:    principalContainer{EmbeddedSlice: []principal{{UserName: "X", Level: "CAN_READ"}}},
+			want: []ResolvedChange{{Field: "[service_principal_name='X'].level", Old: "CAN_MANAGE", New: "CAN_READ"}},
+		},
+		{
+			name: "same field, level change",
+			a:    principalContainer{EmbeddedSlice: []principal{{UserName: "X", Level: "CAN_MANAGE"}}},
+			b:    principalContainer{EmbeddedSlice: []principal{{UserName: "X", Level: "CAN_READ"}}},
+			want: []ResolvedChange{{Field: "[user_name='X'].level", Old: "CAN_MANAGE", New: "CAN_READ"}},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := GetStructDiff(tt.a, tt.b, sliceKeys)
+			assert.NoError(t, err)
+			assert.Equal(t, tt.want, resolveChanges(got))
+		})
+	}
+}
+
 type Dep struct {
 	TaskKey string `json:"task_key,omitempty"`
 	Outcome string `json:"outcome,omitempty"`

libs/structs/structpath/path.go

@@ -181,6 +181,18 @@ func NewKeyValue(prev *PathNode, key, value string) *PathNode {
 //		"resources.jobs.foo.tags['cost-center']": {}
 //	}
 func (p *PathNode) String() string {
+	return p.render(false)
+}
+
+// KeyValueAgnosticString renders the path like String, except key-value segments
+// [field='value'] are rendered as [='value'] (the key field is omitted). Two paths
+// that address the same keyed element under different key fields therefore render
+// identically, which lets callers treat them as the same element.
+func (p *PathNode) KeyValueAgnosticString() string {
+	return p.render(true)
+}
+
+func (p *PathNode) render(omitKeyField bool) string {
 	if p == nil {
 		return ""
 	}
@@ -206,7 +218,9 @@ func (p *PathNode) String() string {
 			result.WriteString("[*]")
 		} else if node.index == tagKeyValue {
 			result.WriteString("[")
-			result.WriteString(node.key)
+			if !omitKeyField {
+				result.WriteString(node.key)
+			}
 			result.WriteString("=")
 			result.WriteString(EncodeMapKey(node.value))
 			result.WriteString("]")

libs/testserver/permissions.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 
 	"github.com/databricks/databricks-sdk-go/service/iam"
+	"github.com/google/uuid"
 )
 
 // source: https://github.com/databricks/terraform-provider-databricks/blob/main/permissions/permission_definitions.go
@@ -39,6 +40,13 @@ var requestObjectTypeToObjectType = map[string]string{
 	"alertsv2":                "alertv2",
 }
 
+// isServicePrincipalID reports whether name is a service principal application ID
+// (a UUID). The Permissions API treats a UUID-valued user_name as a service principal.
+func isServicePrincipalID(name string) bool {
+	_, err := uuid.Parse(name)
+	return err == nil
+}
+
 // aclPrincipalKey returns a unique key identifying the principal in an ACL entry.
 func aclPrincipalKey(acl iam.AccessControlResponse) string {
 	switch {
@@ -269,15 +277,27 @@ func (s *FakeWorkspace) SetPermissions(req Request) any {
 	// Convert AccessControlRequest to AccessControlResponse and replace the ACL.
 	existingPermissions.AccessControlList = nil
 	for _, acl := range updateRequest.AccessControlList {
-		display := acl.UserName
+		userName := acl.UserName
+		servicePrincipalName := acl.ServicePrincipalName
+
+		// The real Permissions API resolves a user_name that is actually a service
+		// principal's application ID (a UUID) to a service principal, and returns it
+		// as service_principal_name on GET. Model that here so a bundle that declares
+		// a service principal under user_name converges the same way it does on cloud.
+		if userName != "" && isServicePrincipalID(userName) {
+			servicePrincipalName = userName
+			userName = ""
+		}
+
+		display := userName
 		if display == "" {
-			display = acl.ServicePrincipalName
+			display = servicePrincipalName
 		}
 
 		response := iam.AccessControlResponse{
-			UserName:             acl.UserName,
+			UserName:             userName,
 			GroupName:            acl.GroupName,
-			ServicePrincipalName: acl.ServicePrincipalName,
+			ServicePrincipalName: servicePrincipalName,
 			DisplayName:          display,
 		}