From e1686d71e4667724d28c8c77a067bae86e69bb9c Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 26 Nov 2025 16:26:45 +0100 Subject: [PATCH 01/16] syntax for [task_key='mytask'] --- libs/structs/structpath/path.go | 110 ++++++++++++++++++++++++-- libs/structs/structpath/path_test.go | 113 ++++++++++++++++++++++++++- 2 files changed, 216 insertions(+), 7 deletions(-) diff --git a/libs/structs/structpath/path.go b/libs/structs/structpath/path.go index 55ab1e8d15..423c54fca1 100644 --- a/libs/structs/structpath/path.go +++ b/libs/structs/structpath/path.go @@ -11,14 +11,17 @@ import ( ) const ( - // Encodes string key, which is encoded as .field or as ['spark.conf']x + // Encodes string key, which is encoded as .field or as ['spark.conf'] tagStringKey = -1 + // Encodes key/value index, which is encoded as [key='value'] or [key="value"] + tagKeyValue = -2 + // Encodes wildcard after a dot: foo.* - tagDotStar = -2 + tagDotStar = -3 // Encodes wildcard in brackets: foo[*] - tagBracketStar = -3 + tagBracketStar = -4 ) // PathNode represents a node in a path for struct diffing. @@ -29,6 +32,7 @@ type PathNode struct { // If index >= 0, the node specifies a slice/array index in index. // If index < 0, this describes the type of node index int + value string // Used for tagKeyValue: stores the value part of [key='value'] } func (p *PathNode) IsRoot() bool { @@ -59,6 +63,16 @@ func (p *PathNode) BracketStar() bool { return p.index == tagBracketStar } +func (p *PathNode) KeyValue() (key, value string, ok bool) { + if p == nil { + return "", "", false + } + if p.index == tagKeyValue { + return p.key, p.value, true + } + return "", "", false +} + // StringKey returns either Field() or MapKey() if either is available func (p *PathNode) StringKey() (string, bool) { if p == nil { @@ -128,6 +142,15 @@ func NewBracketStar(prev *PathNode) *PathNode { } } +func NewKeyValue(prev *PathNode, key, value string) *PathNode { + return &PathNode{ + prev: prev, + key: key, + index: tagKeyValue, + value: value, + } +} + // String returns the string representation of the path. // The string keys are encoded in dot syntax (foo.bar) if they don't have any reserved characters (so can be parsed as fields). // Otherwise they are encoded in brackets + single quotes: tags['name']. Single quote can escaped by placing two single quotes. @@ -160,6 +183,12 @@ func (p *PathNode) String() string { } } else if node.index == tagBracketStar { result.WriteString("[*]") + } else if node.index == tagKeyValue { + result.WriteString("[") + result.WriteString(node.key) + result.WriteString("=") + result.WriteString(EncodeMapKey(node.value)) + result.WriteString("]") } else if isValidField(node.key) { // Valid field name if i != 0 { @@ -191,11 +220,15 @@ func EncodeMapKey(s string) string { // - FIELD_START: After a dot, expects field name or "*" // - FIELD: Reading field name characters // - DOT_STAR: Encountered "*" (at start or after dot), expects ".", "[", or EOF -// - BRACKET_OPEN: Just encountered "[", expects digit, "'" or "*" +// - BRACKET_OPEN: Just encountered "[", expects digit, "'", "*", or identifier for key-value // - INDEX: Reading array index digits, expects more digits or "]" // - MAP_KEY: Reading map key content, expects any char or "'" // - MAP_KEY_QUOTE: Encountered "'" in map key, expects "'" (escape) or "]" (end) // - WILDCARD: Reading "*" in brackets, expects "]" +// - KEYVALUE_KEY: Reading key part of [key='value'], expects identifier chars or "=" +// - KEYVALUE_EQUALS: After key, expecting "'" or '"' to start value +// - KEYVALUE_VALUE: Reading value content, expects any char or quote +// - KEYVALUE_VALUE_QUOTE: Encountered quote in value, expects same quote (escape) or "]" (end) // - EXPECT_DOT_OR_END: After bracket close, expects ".", "[" or end of string // - END: Successfully completed parsing // @@ -204,11 +237,15 @@ func EncodeMapKey(s string) string { // - FIELD_START: [a-zA-Z_-] -> FIELD, "*" -> DOT_STAR, other -> ERROR // - FIELD: [a-zA-Z0-9_-] -> FIELD, "." -> FIELD_START, "[" -> BRACKET_OPEN, EOF -> END // - DOT_STAR: "." -> FIELD_START, "[" -> BRACKET_OPEN, EOF -> END, other -> ERROR -// - BRACKET_OPEN: [0-9] -> INDEX, "'" -> MAP_KEY, "*" -> WILDCARD +// - BRACKET_OPEN: [0-9] -> INDEX, "'" -> MAP_KEY, "*" -> WILDCARD, identifier -> KEYVALUE_KEY // - INDEX: [0-9] -> INDEX, "]" -> EXPECT_DOT_OR_END // - MAP_KEY: (any except "'") -> MAP_KEY, "'" -> MAP_KEY_QUOTE // - MAP_KEY_QUOTE: "'" -> MAP_KEY (escape), "]" -> EXPECT_DOT_OR_END (end key) // - WILDCARD: "]" -> EXPECT_DOT_OR_END +// - KEYVALUE_KEY: identifier -> KEYVALUE_KEY, "=" -> KEYVALUE_EQUALS +// - KEYVALUE_EQUALS: "'" or '"' -> KEYVALUE_VALUE +// - KEYVALUE_VALUE: (any except quote) -> KEYVALUE_VALUE, quote -> KEYVALUE_VALUE_QUOTE +// - KEYVALUE_VALUE_QUOTE: quote -> KEYVALUE_VALUE (escape), "]" -> EXPECT_DOT_OR_END // - EXPECT_DOT_OR_END: "." -> FIELD_START, "[" -> BRACKET_OPEN, EOF -> END func Parse(s string) (*PathNode, error) { if s == "" { @@ -226,6 +263,10 @@ func Parse(s string) (*PathNode, error) { stateMapKey stateMapKeyQuote stateWildcard + stateKeyValueKey + stateKeyValueEquals + stateKeyValueValue + stateKeyValueValueQuote stateExpectDotOrEnd stateEnd ) @@ -233,6 +274,8 @@ func Parse(s string) (*PathNode, error) { state := stateStart var result *PathNode var currentToken strings.Builder + var keyValueKey string // Stores the key part of [key='value'] + var keyValueQuote byte // Stores the quote character used (' or ") pos := 0 for pos < len(s) { @@ -296,6 +339,9 @@ func Parse(s string) (*PathNode, error) { state = stateMapKey } else if ch == '*' { state = stateWildcard + } else if isKeyValueKeyChar(ch) { + currentToken.WriteByte(ch) + state = stateKeyValueKey } else { return nil, fmt.Errorf("unexpected character '%c' after '[' at position %d", ch, pos) } @@ -346,6 +392,47 @@ func Parse(s string) (*PathNode, error) { return nil, fmt.Errorf("unexpected character '%c' after '*' at position %d", ch, pos) } + case stateKeyValueKey: + if ch == '=' { + keyValueKey = currentToken.String() + currentToken.Reset() + state = stateKeyValueEquals + } else if isKeyValueKeyChar(ch) { + currentToken.WriteByte(ch) + } else { + return nil, fmt.Errorf("unexpected character '%c' in key-value key at position %d", ch, pos) + } + + case stateKeyValueEquals: + if ch == '\'' || ch == '"' { + keyValueQuote = ch + state = stateKeyValueValue + } else { + return nil, fmt.Errorf("expected quote after '=' but got '%c' at position %d", ch, pos) + } + + case stateKeyValueValue: + if ch == keyValueQuote { + state = stateKeyValueValueQuote + } else { + currentToken.WriteByte(ch) + } + + case stateKeyValueValueQuote: + if ch == keyValueQuote { + // Escaped quote - add single quote to value and continue + currentToken.WriteByte(ch) + state = stateKeyValueValue + } else if ch == ']' { + // End of key-value + result = NewKeyValue(result, keyValueKey, currentToken.String()) + currentToken.Reset() + keyValueKey = "" + state = stateExpectDotOrEnd + } else { + return nil, fmt.Errorf("unexpected character '%c' after quote in key-value at position %d", ch, pos) + } + case stateExpectDotOrEnd: switch ch { case '.': @@ -390,6 +477,14 @@ func Parse(s string) (*PathNode, error) { return nil, errors.New("unexpected end of input after quote in map key") case stateWildcard: return nil, errors.New("unexpected end of input after wildcard '*'") + case stateKeyValueKey: + return nil, errors.New("unexpected end of input while parsing key-value key") + case stateKeyValueEquals: + return nil, errors.New("unexpected end of input after '=' in key-value") + case stateKeyValueValue: + return nil, errors.New("unexpected end of input while parsing key-value value") + case stateKeyValueValueQuote: + return nil, errors.New("unexpected end of input after quote in key-value value") case stateEnd: return result, nil default: @@ -397,6 +492,11 @@ func Parse(s string) (*PathNode, error) { } } +// isKeyValueKeyChar checks if character is valid for key-value key (identifier-like) +func isKeyValueKeyChar(ch byte) bool { + return (ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || (ch >= '0' && ch <= '9') || ch == '_' +} + // isReservedFieldChar checks if character is reserved and cannot be used in field names func isReservedFieldChar(ch byte) bool { switch ch { diff --git a/libs/structs/structpath/path_test.go b/libs/structs/structpath/path_test.go index a9e19501dc..da92c8a85e 100644 --- a/libs/structs/structpath/path_test.go +++ b/libs/structs/structpath/path_test.go @@ -13,6 +13,7 @@ func TestPathNode(t *testing.T) { String string Index any StringKey any + KeyValue []string // [key, value] or nil Root any DotStar bool BracketStar bool @@ -48,6 +49,12 @@ func TestPathNode(t *testing.T) { String: "[*]", BracketStar: true, }, + { + name: "key value", + node: NewKeyValue(nil, "name", "foo"), + String: "[name='foo']", + KeyValue: []string{"name", "foo"}, + }, // Two node tests { @@ -175,8 +182,61 @@ func TestPathNode(t *testing.T) { String: "bla.*[*]", BracketStar: true, }, + + // Key-value tests + { + name: "key value with parent", + node: NewKeyValue(NewStringKey(nil, "tasks"), "task_key", "my_task"), + String: "tasks[task_key='my_task']", + KeyValue: []string{"task_key", "my_task"}, + }, + { + name: "key value then field", + node: NewStringKey(NewKeyValue(nil, "name", "foo"), "id"), + String: "[name='foo'].id", + StringKey: "id", + }, + { + name: "key value with quote in value", + node: NewKeyValue(nil, "name", "it's"), + String: "[name='it''s']", + KeyValue: []string{"name", "it's"}, + }, + { + name: "key value with empty value", + node: NewKeyValue(nil, "key", ""), + String: "[key='']", + KeyValue: []string{"key", ""}, + }, + { + name: "complex path with key value", + node: NewStringKey(NewKeyValue(NewStringKey(NewStringKey(nil, "resources"), "jobs"), "task_key", "my_task"), "notebook_task"), + String: "resources.jobs[task_key='my_task'].notebook_task", + StringKey: "notebook_task", + }, } + // Additional test for parsing double-quoted key-value + t.Run("key value with double quotes parses correctly", func(t *testing.T) { + parsed, err := Parse(`[name="foo"]`) + assert.NoError(t, err) + key, value, ok := parsed.KeyValue() + assert.True(t, ok) + assert.Equal(t, "name", key) + assert.Equal(t, "foo", value) + // Serializes with single quotes + assert.Equal(t, "[name='foo']", parsed.String()) + }) + + t.Run("key value with double quotes and escaped double quote", func(t *testing.T) { + parsed, err := Parse(`[name="it""s"]`) + assert.NoError(t, err) + key, value, ok := parsed.KeyValue() + assert.True(t, ok) + assert.Equal(t, "name", key) + assert.Equal(t, `it"s`, value) + }) + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Test String() method @@ -212,6 +272,18 @@ func TestPathNode(t *testing.T) { assert.True(t, isStringKey) } + // KeyValue + gotKey, gotValue, isKeyValue := tt.node.KeyValue() + if tt.KeyValue == nil { + assert.Equal(t, "", gotKey) + assert.Equal(t, "", gotValue) + assert.False(t, isKeyValue) + } else { + assert.Equal(t, tt.KeyValue[0], gotKey) + assert.Equal(t, tt.KeyValue[1], gotValue) + assert.True(t, isKeyValue) + } + // IsRoot isRoot := tt.node.IsRoot() if tt.Root == nil { @@ -266,7 +338,7 @@ func TestParseErrors(t *testing.T) { { name: "invalid array index", input: "[abc]", - error: "unexpected character 'a' after '[' at position 1", + error: "unexpected character ']' in key-value key at position 4", }, { name: "negative array index", @@ -331,7 +403,7 @@ func TestParseErrors(t *testing.T) { { name: "invalid character in bracket", input: "field[name", - error: "unexpected character 'n' after '[' at position 6", + error: "unexpected end of input while parsing key-value key", }, { name: "unexpected character after valid path", @@ -380,6 +452,43 @@ func TestParseErrors(t *testing.T) { input: "bla.*foo", error: "unexpected character 'f' after '.*' at position 5", }, + + // Invalid key-value patterns + { + name: "key-value missing equals", + input: "[name'value']", + error: "unexpected character ''' in key-value key at position 5", + }, + { + name: "key-value missing value quote", + input: "[name=value]", + error: "expected quote after '=' but got 'v' at position 6", + }, + { + name: "key-value incomplete key", + input: "[name", + error: "unexpected end of input while parsing key-value key", + }, + { + name: "key-value incomplete after equals", + input: "[name=", + error: "unexpected end of input after '=' in key-value", + }, + { + name: "key-value incomplete value", + input: "[name='value", + error: "unexpected end of input while parsing key-value value", + }, + { + name: "key-value incomplete after value quote", + input: "[name='value'", + error: "unexpected end of input after quote in key-value value", + }, + { + name: "key-value invalid char after value quote", + input: "[name='value'x]", + error: "unexpected character 'x' after quote in key-value at position 13", + }, } for _, tt := range tests { From 8d66740fa6053d13cabbd14d5c671802145442c2 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 26 Nov 2025 17:21:19 +0100 Subject: [PATCH 02/16] GetStructDiff new parameter --- bundle/direct/bundle_plan.go | 4 +- bundle/direct/dresources/all_test.go | 2 +- libs/structs/structaccess/set_test.go | 2 +- libs/structs/structdiff/bench_test.go | 2 +- libs/structs/structdiff/diff.go | 233 ++++++++++++++++++-- libs/structs/structdiff/diff_test.go | 148 ++++++++++++- libs/structs/structdiff/jobsettings_test.go | 12 +- libs/structs/structpath/path.go | 7 +- 8 files changed, 373 insertions(+), 37 deletions(-) diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index 8103fde30f..2f6d7bb643 100644 --- 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, nil) 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, nil) if err != nil { logdiag.LogError(ctx, fmt.Errorf("%s: diffing remote state: %w", errorPrefix, err)) return false diff --git a/bundle/direct/dresources/all_test.go b/bundle/direct/dresources/all_test.go index b9c115768d..9ed83ab68f 100644 --- 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/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index f2348618c6..0d29bea822 100644 --- 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 index 30988d00fc..f040604783 100644 --- 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 index 03e6423f04..10e85e9613 100644 --- a/libs/structs/structdiff/diff.go +++ b/libs/structs/structdiff/diff.go @@ -5,6 +5,7 @@ import ( "reflect" "slices" "sort" + "strings" "github.com/databricks/cli/libs/structs/structpath" "github.com/databricks/cli/libs/structs/structtag" @@ -16,10 +17,70 @@ type Change struct { New any } +// SliceKeyFunc extracts a key field name and value from a slice element. +// It can be either: +// - func(T) (string, string, error) - typed function for specific element type T +// - func(any) (string, string, error) - generic function accepting any element +// +// The function returns (keyField, keyValue, error). The keyField is typically a field name +// like "task_key", and keyValue is the value that uniquely identifies the element. +type SliceKeyFunc = any + +// sliceKeyFuncCaller wraps a SliceKeyFunc and provides a type-checked Call method. +type sliceKeyFuncCaller struct { + fn reflect.Value +} + +func newSliceKeyFuncCaller(fn any) (*sliceKeyFuncCaller, error) { + v := reflect.ValueOf(fn) + if v.Kind() != reflect.Func { + return nil, fmt.Errorf("SliceKeyFunc must be a function, got %T", fn) + } + t := v.Type() + if t.NumIn() != 1 { + return nil, fmt.Errorf("SliceKeyFunc must have exactly 1 parameter, got %d", t.NumIn()) + } + if t.NumOut() != 3 { + return nil, fmt.Errorf("SliceKeyFunc must return exactly 3 values, got %d", t.NumOut()) + } + if t.Out(0).Kind() != reflect.String || t.Out(1).Kind() != reflect.String { + return nil, fmt.Errorf("SliceKeyFunc must return (string, string, error), got (%v, %v, %v)", t.Out(0), t.Out(1), t.Out(2)) + } + errType := reflect.TypeOf((*error)(nil)).Elem() + if !t.Out(2).Implements(errType) && t.Out(2) != errType { + return nil, fmt.Errorf("SliceKeyFunc third return must be error, got %v", t.Out(2)) + } + return &sliceKeyFuncCaller{fn: v}, nil +} + +func (c *sliceKeyFuncCaller) call(elem any) (string, string, error) { + in := []reflect.Value{reflect.ValueOf(elem)} + out := c.fn.Call(in) + keyField := out[0].String() + keyValue := out[1].String() + var err error + if !out[2].IsNil() { + err = out[2].Interface().(error) + } + return keyField, keyValue, err +} + +// diffContext holds configuration for the diff operation. +type diffContext struct { + sliceKeys map[string]SliceKeyFunc +} + // GetStructDiff compares two Go structs and returns a list of Changes or an error. // Respects ForceSendFields if present. // Types of a and b must match exactly, otherwise returns an error. -func GetStructDiff(a, b any) ([]Change, error) { +// +// 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"). +// The [*] wildcard matches any slice index in the path. +// Pass nil if no slice key functions are needed. +func GetStructDiff(a, b any, sliceKeys map[string]SliceKeyFunc) ([]Change, error) { v1 := reflect.ValueOf(a) v2 := reflect.ValueOf(b) @@ -38,24 +99,27 @@ func GetStructDiff(a, b any) ([]Change, error) { return nil, fmt.Errorf("type mismatch: %v vs %v", v1.Type(), v2.Type()) } - diffValues(nil, v1, v2, &changes) + ctx := &diffContext{sliceKeys: sliceKeys} + if err := diffValues(ctx, nil, v1, v2, &changes); err != nil { + return nil, err + } return changes, nil } // diffValues appends changes between v1 and v2 to the slice. path is the current // JSON-style path (dot + brackets). At the root path is "". -func diffValues(path *structpath.PathNode, v1, v2 reflect.Value, changes *[]Change) { +func diffValues(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect.Value, changes *[]Change) error { if !v1.IsValid() { if !v2.IsValid() { - return + return nil } *changes = append(*changes, Change{Path: path, Old: nil, New: v2.Interface()}) - return + return nil } else if !v2.IsValid() { // v1 is valid *changes = append(*changes, Change{Path: path, Old: v1.Interface(), New: nil}) - return + return nil } v1Type := v1.Type() @@ -63,7 +127,7 @@ func diffValues(path *structpath.PathNode, v1, v2 reflect.Value, changes *[]Chan // This should not happen; if it does, record this a full change if v1Type != v2.Type() { *changes = append(*changes, Change{Path: path, Old: v1.Interface(), New: v2.Interface()}) - return + return nil } kind := v1.Kind() @@ -74,11 +138,11 @@ func diffValues(path *structpath.PathNode, v1, v2 reflect.Value, changes *[]Chan v1Nil := v1.IsNil() v2Nil := v2.IsNil() if v1Nil && v2Nil { - return + return nil } if v1Nil || v2Nil { *changes = append(*changes, Change{Path: path, Old: v1.Interface(), New: v2.Interface()}) - return + return nil } default: // Not a nilable type. @@ -87,27 +151,32 @@ func diffValues(path *structpath.PathNode, v1, v2 reflect.Value, changes *[]Chan switch kind { case reflect.Pointer: - diffValues(path, v1.Elem(), v2.Elem(), changes) + return diffValues(ctx, path, v1.Elem(), v2.Elem(), changes) case reflect.Struct: - diffStruct(path, v1, v2, changes) + return diffStruct(ctx, path, v1, v2, changes) case reflect.Slice, reflect.Array: - if v1.Len() != v2.Len() { + if keyFunc := ctx.findSliceKeyFunc(path); keyFunc != nil { + return diffSliceByKey(ctx, path, v1, v2, keyFunc, changes) + } else if v1.Len() != v2.Len() { *changes = append(*changes, Change{Path: path, Old: v1.Interface(), New: v2.Interface()}) } else { for i := range v1.Len() { node := structpath.NewIndex(path, i) - diffValues(node, v1.Index(i), v2.Index(i), changes) + if err := diffValues(ctx, node, v1.Index(i), v2.Index(i), changes); err != nil { + return err + } } } case reflect.Map: if v1Type.Key().Kind() == reflect.String { - diffMapStringKey(path, v1, v2, changes) + return diffMapStringKey(ctx, path, v1, v2, changes) } else { deepEqualValues(path, v1, v2, changes) } default: deepEqualValues(path, v1, v2, changes) } + return nil } func deepEqualValues(path *structpath.PathNode, v1, v2 reflect.Value, changes *[]Change) { @@ -116,7 +185,7 @@ func deepEqualValues(path *structpath.PathNode, v1, v2 reflect.Value, changes *[ } } -func diffStruct(path *structpath.PathNode, s1, s2 reflect.Value, changes *[]Change) { +func diffStruct(ctx *diffContext, path *structpath.PathNode, s1, s2 reflect.Value, changes *[]Change) error { t := s1.Type() forced1 := getForceSendFields(s1) forced2 := getForceSendFields(s2) @@ -129,7 +198,9 @@ func diffStruct(path *structpath.PathNode, s1, s2 reflect.Value, changes *[]Chan // Continue traversing embedded structs. Do not add the key to the path though. if sf.Anonymous { - diffValues(path, s1.Field(i), s2.Field(i), changes) + if err := diffValues(ctx, path, s1.Field(i), s2.Field(i), changes); err != nil { + return err + } continue } @@ -163,11 +234,14 @@ func diffStruct(path *structpath.PathNode, s1, s2 reflect.Value, changes *[]Chan } } - diffValues(node, v1Field, v2Field, changes) + if err := diffValues(ctx, node, v1Field, v2Field, changes); err != nil { + return err + } } + return nil } -func diffMapStringKey(path *structpath.PathNode, m1, m2 reflect.Value, changes *[]Change) { +func diffMapStringKey(ctx *diffContext, path *structpath.PathNode, m1, m2 reflect.Value, changes *[]Change) error { keySet := map[string]reflect.Value{} for _, k := range m1.MapKeys() { // Key is always string at this point @@ -190,8 +264,11 @@ func diffMapStringKey(path *structpath.PathNode, m1, m2 reflect.Value, changes * v1 := m1.MapIndex(k) v2 := m2.MapIndex(k) node := structpath.NewStringKey(path, ks) - diffValues(node, v1, v2, changes) + if err := diffValues(ctx, node, v1, v2, changes); err != nil { + return err + } } + return nil } func getForceSendFields(v reflect.Value) []string { @@ -208,3 +285,121 @@ func getForceSendFields(v reflect.Value) []string { } return nil } + +// findSliceKeyFunc returns the SliceKeyFunc for the given path, or nil if none matches. +// Path patterns support [*] to match any slice index. +func (ctx *diffContext) findSliceKeyFunc(path *structpath.PathNode) SliceKeyFunc { + if ctx.sliceKeys == nil { + return nil + } + pathStr := pathToPattern(path) + return ctx.sliceKeys[pathStr] +} + +// pathToPattern converts a PathNode to a pattern string for matching. +// Slice indices are converted to [*] wildcard. +func pathToPattern(path *structpath.PathNode) string { + if path == nil { + return "" + } + + components := path.AsSlice() + var result strings.Builder + + for i, node := range components { + if idx, ok := node.Index(); ok { + // Convert numeric index to wildcard + _ = idx + result.WriteString("[*]") + } else if key, value, ok := node.KeyValue(); ok { + // Key-value syntax + result.WriteString("[") + result.WriteString(key) + result.WriteString("=") + result.WriteString(structpath.EncodeMapKey(value)) + result.WriteString("]") + } else if key, ok := node.StringKey(); ok { + if i != 0 { + result.WriteString(".") + } + result.WriteString(key) + } + } + + return result.String() +} + +// sliceElement holds a slice element with its key information. +type sliceElement struct { + keyField string + keyValue string + index int + value reflect.Value +} + +// diffSliceByKey compares two slices using the provided key function. +// Elements are matched by their (keyField, keyValue) pairs instead of by index. +func diffSliceByKey(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect.Value, keyFunc SliceKeyFunc, changes *[]Change) error { + caller, err := newSliceKeyFuncCaller(keyFunc) + if err != nil { + return err + } + + elements1 := make(map[string]sliceElement) + elements2 := make(map[string]sliceElement) + seen := make(map[string]bool) + var orderedKeys []string + + // Build map from first slice, collecting keys in order + for i := range v1.Len() { + elem := v1.Index(i) + keyField, keyValue, err := caller.call(elem.Interface()) + if err != nil { + return err + } + elements1[keyValue] = sliceElement{keyField: keyField, keyValue: keyValue, index: i, value: elem} + if !seen[keyValue] { + seen[keyValue] = true + orderedKeys = append(orderedKeys, keyValue) + } + } + + // Build map from second slice, adding new keys in order + for i := range v2.Len() { + elem := v2.Index(i) + keyField, keyValue, err := caller.call(elem.Interface()) + if err != nil { + return err + } + elements2[keyValue] = sliceElement{keyField: keyField, keyValue: keyValue, index: i, value: elem} + if !seen[keyValue] { + seen[keyValue] = true + orderedKeys = append(orderedKeys, keyValue) + } + } + + // Compare elements by key in original order + for _, keyValue := range orderedKeys { + elem1, has1 := elements1[keyValue] + elem2, has2 := elements2[keyValue] + + var keyField string + if has1 { + keyField = elem1.keyField + } else { + keyField = elem2.keyField + } + node := structpath.NewKeyValue(path, keyField, keyValue) + + if has1 && has2 { + if err := diffValues(ctx, node, elem1.value, elem2.value, changes); err != nil { + return err + } + } else if has1 { + *changes = append(*changes, Change{Path: node, Old: elem1.value.Interface(), New: nil}) + } else { + *changes = append(*changes, Change{Path: node, Old: nil, New: elem2.value.Interface()}) + } + } + return nil +} diff --git a/libs/structs/structdiff/diff_test.go b/libs/structs/structdiff/diff_test.go index fb15ecb03c..b6b33f64f2 100644 --- a/libs/structs/structdiff/diff_test.go +++ b/libs/structs/structdiff/diff_test.go @@ -391,7 +391,7 @@ func TestGetStructDiff(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := GetStructDiff(tt.a, tt.b) + got, err := GetStructDiff(tt.a, tt.b, nil) assert.Equal(t, tt.want, resolveChanges(got)) @@ -403,7 +403,7 @@ func TestGetStructDiff(t *testing.T) { }) t.Run(tt.name+" mirror", func(t *testing.T) { - got, err := GetStructDiff(tt.b, tt.a) + got, err := GetStructDiff(tt.b, tt.a, nil) var mirrorWant []ResolvedChange for _, ch := range tt.want { @@ -424,15 +424,155 @@ func TestGetStructDiff(t *testing.T) { }) t.Run(tt.name+" equal A", func(t *testing.T) { - got, err := GetStructDiff(tt.a, tt.a) + got, err := GetStructDiff(tt.a, tt.a, nil) assert.NoError(t, err) assert.Nil(t, got) }) t.Run(tt.name+" equal B", func(t *testing.T) { - got, err := GetStructDiff(tt.b, tt.b) + got, err := GetStructDiff(tt.b, tt.b, nil) assert.NoError(t, err) assert.Nil(t, got) }) } } + +type Task struct { + TaskKey string `json:"task_key,omitempty"` + Description string `json:"description,omitempty"` + Timeout int `json:"timeout,omitempty"` +} + +type Job struct { + Name string `json:"name,omitempty"` + Tasks []Task `json:"tasks,omitempty"` +} + +func taskKeyFunc(task Task) (string, string, error) { + return "task_key", task.TaskKey, nil +} + +func TestGetStructDiffSliceKeys(t *testing.T) { + sliceKeys := map[string]SliceKeyFunc{ + "tasks": taskKeyFunc, + } + + tests := []struct { + name string + a, b any + want []ResolvedChange + }{ + { + name: "slice with same keys same order", + a: Job{Tasks: []Task{{TaskKey: "a", Description: "one"}, {TaskKey: "b", Description: "two"}}}, + b: Job{Tasks: []Task{{TaskKey: "a", Description: "one"}, {TaskKey: "b", Description: "two"}}}, + want: nil, + }, + { + name: "slice with same keys different order", + a: Job{Tasks: []Task{{TaskKey: "a", Description: "one"}, {TaskKey: "b", Description: "two"}}}, + b: Job{Tasks: []Task{{TaskKey: "b", Description: "two"}, {TaskKey: "a", Description: "one"}}}, + want: nil, + }, + { + name: "slice with same keys field change", + a: Job{Tasks: []Task{{TaskKey: "a", Description: "one"}}}, + b: Job{Tasks: []Task{{TaskKey: "a", Description: "changed"}}}, + want: []ResolvedChange{{Field: "tasks[task_key='a'].description", Old: "one", New: "changed"}}, + }, + { + name: "slice element added", + a: Job{Tasks: []Task{{TaskKey: "a", Description: "one"}}}, + b: Job{Tasks: []Task{{TaskKey: "a", Description: "one"}, {TaskKey: "b", Description: "two"}}}, + want: []ResolvedChange{{Field: "tasks[task_key='b']", Old: nil, New: Task{TaskKey: "b", Description: "two"}}}, + }, + { + name: "slice element removed", + a: Job{Tasks: []Task{{TaskKey: "a", Description: "one"}, {TaskKey: "b", Description: "two"}}}, + b: Job{Tasks: []Task{{TaskKey: "a", Description: "one"}}}, + want: []ResolvedChange{{Field: "tasks[task_key='b']", Old: Task{TaskKey: "b", Description: "two"}, New: nil}}, + }, + { + name: "slice element replaced", + a: Job{Tasks: []Task{{TaskKey: "a", Description: "one"}}}, + b: Job{Tasks: []Task{{TaskKey: "b", Description: "two"}}}, + want: []ResolvedChange{ + {Field: "tasks[task_key='a']", Old: Task{TaskKey: "a", Description: "one"}, New: nil}, + {Field: "tasks[task_key='b']", Old: nil, New: Task{TaskKey: "b", Description: "two"}}, + }, + }, + { + name: "multiple changes with reorder", + a: Job{Tasks: []Task{{TaskKey: "a", Description: "one"}, {TaskKey: "b", Description: "two"}, {TaskKey: "c", Description: "three"}}}, + b: Job{Tasks: []Task{{TaskKey: "c", Description: "changed"}, {TaskKey: "a", Description: "one"}}}, + want: []ResolvedChange{ + {Field: "tasks[task_key='b']", Old: Task{TaskKey: "b", Description: "two"}, New: nil}, + {Field: "tasks[task_key='c'].description", Old: "three", New: "changed"}, + }, + }, + } + + 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 Nested struct { + Items []Item `json:"items,omitempty"` +} + +type Item struct { + ID string `json:"id,omitempty"` + Value int `json:"value,omitempty"` +} + +type Root struct { + Nested []Nested `json:"nested,omitempty"` +} + +func itemKeyFunc(item Item) (string, string, error) { + return "id", item.ID, nil +} + +func TestGetStructDiffNestedSliceKeys(t *testing.T) { + sliceKeys := map[string]SliceKeyFunc{ + "nested[*].items": itemKeyFunc, + } + + tests := []struct { + name string + a, b any + want []ResolvedChange + }{ + { + name: "nested slice with same keys different order", + a: Root{Nested: []Nested{{Items: []Item{{ID: "x", Value: 1}, {ID: "y", Value: 2}}}}}, + b: Root{Nested: []Nested{{Items: []Item{{ID: "y", Value: 2}, {ID: "x", Value: 1}}}}}, + want: nil, + }, + { + name: "nested slice field change", + a: Root{Nested: []Nested{{Items: []Item{{ID: "x", Value: 1}}}}}, + b: Root{Nested: []Nested{{Items: []Item{{ID: "x", Value: 99}}}}}, + want: []ResolvedChange{{Field: "nested[0].items[id='x'].value", Old: 1, New: 99}}, + }, + { + name: "nested slice element added", + a: Root{Nested: []Nested{{Items: []Item{{ID: "x", Value: 1}}}}}, + b: Root{Nested: []Nested{{Items: []Item{{ID: "x", Value: 1}, {ID: "y", Value: 2}}}}}, + want: []ResolvedChange{{Field: "nested[0].items[id='y']", Old: nil, New: Item{ID: "y", Value: 2}}}, + }, + } + + 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)) + }) + } +} diff --git a/libs/structs/structdiff/jobsettings_test.go b/libs/structs/structdiff/jobsettings_test.go index beeb1c5878..198cd34510 100644 --- a/libs/structs/structdiff/jobsettings_test.go +++ b/libs/structs/structdiff/jobsettings_test.go @@ -444,16 +444,16 @@ const jobExampleResponseNils = ` func testEqual(t *testing.T, input string) { var x, y jobs.JobSettings require.NoError(t, json.Unmarshal([]byte(input), &x)) - changes, err := GetStructDiff(x, x) + changes, err := GetStructDiff(x, x, nil) require.NoError(t, err) require.Nil(t, changes) require.NoError(t, json.Unmarshal([]byte(input), &y)) - changes, err = GetStructDiff(x, y) + changes, err = GetStructDiff(x, y, nil) require.NoError(t, err) require.Nil(t, changes) - changes, err = GetStructDiff(&x, &y) + changes, err = GetStructDiff(&x, &y, nil) require.NoError(t, err) require.Nil(t, changes) } @@ -471,7 +471,7 @@ func TestJobDiff(t *testing.T) { require.NoError(t, json.Unmarshal([]byte(jobExampleResponseZeroes), &zero)) require.NoError(t, json.Unmarshal([]byte(jobExampleResponseNils), &nils)) - changes, err := GetStructDiff(job, zero) + changes, err := GetStructDiff(job, zero, nil) require.NoError(t, err) require.GreaterOrEqual(t, len(changes), 75) assert.Equal(t, "budget_policy_id", changes[0].Path.String()) @@ -488,7 +488,7 @@ func TestJobDiff(t *testing.T) { assert.Equal(t, "string", changes[3].Old) assert.Equal(t, "", changes[3].New) - changes, err = GetStructDiff(job, nils) + changes, err = GetStructDiff(job, nils, nil) require.NoError(t, err) require.GreaterOrEqual(t, len(changes), 77) assert.Equal(t, "budget_policy_id", changes[0].Path.String()) @@ -509,7 +509,7 @@ func TestJobDiff(t *testing.T) { assert.Equal(t, "string", changes[3].Old) assert.Nil(t, changes[3].New) - changes, err = GetStructDiff(zero, nils) + changes, err = GetStructDiff(zero, nils, nil) require.NoError(t, err) assert.GreaterOrEqual(t, len(changes), 58) assert.Equal(t, "budget_policy_id", changes[0].Path.String()) diff --git a/libs/structs/structpath/path.go b/libs/structs/structpath/path.go index 423c54fca1..f6327c4f78 100644 --- a/libs/structs/structpath/path.go +++ b/libs/structs/structpath/path.go @@ -419,17 +419,18 @@ func Parse(s string) (*PathNode, error) { } case stateKeyValueValueQuote: - if ch == keyValueQuote { + switch ch { + case keyValueQuote: // Escaped quote - add single quote to value and continue currentToken.WriteByte(ch) state = stateKeyValueValue - } else if ch == ']' { + case ']': // End of key-value result = NewKeyValue(result, keyValueKey, currentToken.String()) currentToken.Reset() keyValueKey = "" state = stateExpectDotOrEnd - } else { + default: return nil, fmt.Errorf("unexpected character '%c' after quote in key-value at position %d", ch, pos) } From e44abff8f34d5e85a9b6685982028e5599ecccfe Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 26 Nov 2025 17:22:59 +0100 Subject: [PATCH 03/16] fix test --- libs/structs/structvar/structvar_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/structs/structvar/structvar_test.go b/libs/structs/structvar/structvar_test.go index 60705a4fb6..9d8c1d2d8d 100644 --- a/libs/structs/structvar/structvar_test.go +++ b/libs/structs/structvar/structvar_test.go @@ -110,7 +110,7 @@ func TestResolveRef(t *testing.T) { }, reference: "${var.test}", value: "value", - errorMsg: "unexpected character", + errorMsg: "invalid path", }, } From d2ad9348b6612bc4bd5ef68055a572da9b676526 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 26 Nov 2025 17:31:54 +0100 Subject: [PATCH 04/16] wip --- libs/structs/structdiff/diff.go | 12 +++-- libs/structs/structdiff/diff_test.go | 67 ++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 4 deletions(-) diff --git a/libs/structs/structdiff/diff.go b/libs/structs/structdiff/diff.go index 10e85e9613..d1501846d3 100644 --- a/libs/structs/structdiff/diff.go +++ b/libs/structs/structdiff/diff.go @@ -28,7 +28,8 @@ type SliceKeyFunc = any // sliceKeyFuncCaller wraps a SliceKeyFunc and provides a type-checked Call method. type sliceKeyFuncCaller struct { - fn reflect.Value + fn reflect.Value + argType reflect.Type } func newSliceKeyFuncCaller(fn any) (*sliceKeyFuncCaller, error) { @@ -50,12 +51,15 @@ func newSliceKeyFuncCaller(fn any) (*sliceKeyFuncCaller, error) { if !t.Out(2).Implements(errType) && t.Out(2) != errType { return nil, fmt.Errorf("SliceKeyFunc third return must be error, got %v", t.Out(2)) } - return &sliceKeyFuncCaller{fn: v}, nil + return &sliceKeyFuncCaller{fn: v, argType: t.In(0)}, nil } func (c *sliceKeyFuncCaller) call(elem any) (string, string, error) { - in := []reflect.Value{reflect.ValueOf(elem)} - out := c.fn.Call(in) + elemValue := reflect.ValueOf(elem) + if !elemValue.Type().AssignableTo(c.argType) { + return "", "", fmt.Errorf("SliceKeyFunc expects %v, got %T", c.argType, elem) + } + out := c.fn.Call([]reflect.Value{elemValue}) keyField := out[0].String() keyValue := out[1].String() var err error diff --git a/libs/structs/structdiff/diff_test.go b/libs/structs/structdiff/diff_test.go index b6b33f64f2..9c7c270a11 100644 --- a/libs/structs/structdiff/diff_test.go +++ b/libs/structs/structdiff/diff_test.go @@ -576,3 +576,70 @@ func TestGetStructDiffNestedSliceKeys(t *testing.T) { }) } } + +func TestGetStructDiffSliceKeysInvalidFunc(t *testing.T) { + tests := []struct { + name string + keyFunc any + errMsg string + }{ + { + name: "not a function", + keyFunc: "not a function", + errMsg: "SliceKeyFunc must be a function, got string", + }, + { + name: "wrong number of parameters", + keyFunc: func() (string, string, error) { return "", "", nil }, + errMsg: "SliceKeyFunc must have exactly 1 parameter, got 0", + }, + { + name: "too many parameters", + keyFunc: func(a, b Task) (string, string, error) { return "", "", nil }, + errMsg: "SliceKeyFunc must have exactly 1 parameter, got 2", + }, + { + name: "wrong number of returns", + keyFunc: func(t Task) string { return "" }, + errMsg: "SliceKeyFunc must return exactly 3 values, got 1", + }, + { + name: "wrong first return type", + keyFunc: func(t Task) (int, string, error) { return 0, "", nil }, + errMsg: "SliceKeyFunc must return (string, string, error), got (int, string, error)", + }, + { + name: "wrong second return type", + keyFunc: func(t Task) (string, int, error) { return "", 0, nil }, + errMsg: "SliceKeyFunc must return (string, string, error), got (string, int, error)", + }, + { + name: "wrong third return type", + keyFunc: func(t Task) (string, string, string) { return "", "", "" }, + errMsg: "SliceKeyFunc third return must be error, got string", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sliceKeys := map[string]SliceKeyFunc{"tasks": tt.keyFunc} + a := Job{Tasks: []Task{{TaskKey: "a"}}} + b := Job{Tasks: []Task{{TaskKey: "a"}}} + _, err := GetStructDiff(a, b, sliceKeys) + assert.EqualError(t, err, tt.errMsg) + }) + } +} + +func TestGetStructDiffSliceKeysWrongArgType(t *testing.T) { + // Function expects Item but slice contains Task + sliceKeys := map[string]SliceKeyFunc{ + "tasks": func(item Item) (string, string, error) { + return "id", item.ID, nil + }, + } + a := Job{Tasks: []Task{{TaskKey: "a"}}} + b := Job{Tasks: []Task{{TaskKey: "b"}}} + _, err := GetStructDiff(a, b, sliceKeys) + assert.EqualError(t, err, "SliceKeyFunc expects structdiff.Item, got structdiff.Task") +} From 5c278abf6599ff7e39e1fa75c5621e2f1ddda47d Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 27 Nov 2025 10:20:51 +0100 Subject: [PATCH 05/16] wip --- bundle/direct/bundle_plan.go | 4 +- bundle/direct/dresources/adapter.go | 44 ++++++++++++++++++++ bundle/direct/dresources/permissions.go | 19 +++++++++ libs/structs/structdiff/diff.go | 50 ++++++++++++++--------- libs/structs/structdiff/diff_test.go | 54 +++++++++++++++++++++++++ 5 files changed, 150 insertions(+), 21 deletions(-) diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index 2f6d7bb643..5cd389417b 100644 --- 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, nil) + 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, nil) + 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 index 3e57bb80d2..36a03e8a53 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -83,6 +83,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 SliceKeyFunc for comparing slices by key instead of by index. + // Example: func (*ResourcePermissions) KeyedSlices(state *PermissionsState) map[string]any + KeyedSlices(state any) map[string]any } // Adapter wraps resource implementation, validates signatures and type consistency across methods @@ -106,6 +110,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) { @@ -136,6 +141,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) @@ -194,6 +200,27 @@ 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) { + // Validate signature: func(stateType) map[string]any + if len(call.InTypes) != 1 { + return nil, fmt.Errorf("KeyedSlices must take exactly 1 parameter, got %d", len(call.InTypes)) + } + expectedReturnType := reflect.TypeOf(map[string]any{}) + if len(call.OutTypes) != 1 || call.OutTypes[0] != expectedReturnType { + return nil, fmt.Errorf("KeyedSlices must return map[string]any, got %v", call.OutTypes) + } + + // Create a nil pointer of the expected type (KeyedSlices implementations should not depend on the state value) + nilArg := reflect.Zero(call.InTypes[0]).Interface() + outs, err := call.Call(nilArg) + 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 { @@ -262,6 +289,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 } @@ -610,6 +648,12 @@ func (a *Adapter) ClassifyChange(change structdiff.Change, remoteState any, isLo return actionType, nil } +// KeyedSlices returns a map from path patterns to SliceKeyFunc 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/permissions.go b/bundle/direct/dresources/permissions.go index c6da6851d8..90eca157c2 100644 --- 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, error) { + if x.UserName != "" { + return "user_name", x.UserName, nil + } + if x.ServicePrincipalName != "" { + return "service_principal_name", x.ServicePrincipalName, nil + } + if x.GroupName != "" { + return "group_name", x.GroupName, nil + } + return "", "", fmt.Errorf("no key found in AccessControlRequest: %+v", x) +} + +func (*ResourcePermissions) KeyedSlices(s *PermissionsState) 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/structdiff/diff.go b/libs/structs/structdiff/diff.go index d1501846d3..6ad763b31e 100644 --- a/libs/structs/structdiff/diff.go +++ b/libs/structs/structdiff/diff.go @@ -302,6 +302,7 @@ func (ctx *diffContext) findSliceKeyFunc(path *structpath.PathNode) SliceKeyFunc // pathToPattern converts a PathNode to a pattern string for matching. // Slice indices are converted to [*] wildcard. +// XXX why indices are special; why do we need this; if we need this, should it be in structpath instead? func pathToPattern(path *structpath.PathNode) string { if path == nil { return "" @@ -337,45 +338,46 @@ func pathToPattern(path *structpath.PathNode) string { type sliceElement struct { keyField string keyValue string - index int value reflect.Value } // 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. func diffSliceByKey(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect.Value, keyFunc SliceKeyFunc, changes *[]Change) error { caller, err := newSliceKeyFuncCaller(keyFunc) if err != nil { return err } - elements1 := make(map[string]sliceElement) - elements2 := make(map[string]sliceElement) + // Build lists of elements grouped by key, preserving order within each key + elements1 := make(map[string][]sliceElement) + elements2 := make(map[string][]sliceElement) seen := make(map[string]bool) var orderedKeys []string - // Build map from first slice, collecting keys in order + // Build from first slice for i := range v1.Len() { elem := v1.Index(i) keyField, keyValue, err := caller.call(elem.Interface()) if err != nil { return err } - elements1[keyValue] = sliceElement{keyField: keyField, keyValue: keyValue, index: i, value: elem} + elements1[keyValue] = append(elements1[keyValue], sliceElement{keyField: keyField, keyValue: keyValue, value: elem}) if !seen[keyValue] { seen[keyValue] = true orderedKeys = append(orderedKeys, keyValue) } } - // Build map from second slice, adding new keys in order + // Build from second slice for i := range v2.Len() { elem := v2.Index(i) keyField, keyValue, err := caller.call(elem.Interface()) if err != nil { return err } - elements2[keyValue] = sliceElement{keyField: keyField, keyValue: keyValue, index: i, value: elem} + elements2[keyValue] = append(elements2[keyValue], sliceElement{keyField: keyField, keyValue: keyValue, value: elem}) if !seen[keyValue] { seen[keyValue] = true orderedKeys = append(orderedKeys, keyValue) @@ -384,25 +386,35 @@ func diffSliceByKey(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect. // Compare elements by key in original order for _, keyValue := range orderedKeys { - elem1, has1 := elements1[keyValue] - elem2, has2 := elements2[keyValue] + list1 := elements1[keyValue] + list2 := elements2[keyValue] var keyField string - if has1 { - keyField = elem1.keyField + if len(list1) > 0 { + keyField = list1[0].keyField } else { - keyField = elem2.keyField + keyField = list2[0].keyField } - node := structpath.NewKeyValue(path, keyField, keyValue) - if has1 && has2 { - if err := diffValues(ctx, node, elem1.value, elem2.value, changes); err != nil { + // Match elements in order + 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 { return err } - } else if has1 { - *changes = append(*changes, Change{Path: node, Old: elem1.value.Interface(), New: nil}) - } else { - *changes = append(*changes, Change{Path: node, Old: nil, New: elem2.value.Interface()}) + } + + // Handle extra elements in old (deleted) + for i := minLen; i < len(list1); i++ { + node := structpath.NewKeyValue(path, keyField, keyValue) + *changes = append(*changes, Change{Path: node, Old: list1[i].value.Interface(), New: nil}) + } + + // Handle extra elements in new (added) + for i := minLen; i < len(list2); i++ { + node := structpath.NewKeyValue(path, keyField, keyValue) + *changes = append(*changes, Change{Path: node, Old: nil, New: list2[i].value.Interface()}) } } return nil diff --git a/libs/structs/structdiff/diff_test.go b/libs/structs/structdiff/diff_test.go index 9c7c270a11..4fa16fe550 100644 --- a/libs/structs/structdiff/diff_test.go +++ b/libs/structs/structdiff/diff_test.go @@ -643,3 +643,57 @@ func TestGetStructDiffSliceKeysWrongArgType(t *testing.T) { _, err := GetStructDiff(a, b, sliceKeys) assert.EqualError(t, err, "SliceKeyFunc expects structdiff.Item, got structdiff.Task") } + +func TestGetStructDiffSliceKeysDuplicates(t *testing.T) { + sliceKeys := map[string]SliceKeyFunc{ + "tasks": taskKeyFunc, + } + + tests := []struct { + name string + a, b Job + want []ResolvedChange + }{ + { + name: "same duplicates no change", + a: Job{Tasks: []Task{{TaskKey: "a", Description: "1"}, {TaskKey: "a", Description: "2"}}}, + b: Job{Tasks: []Task{{TaskKey: "a", Description: "1"}, {TaskKey: "a", Description: "2"}}}, + want: nil, + }, + { + name: "duplicates with field change", + a: Job{Tasks: []Task{{TaskKey: "a", Description: "1"}, {TaskKey: "a", Description: "2"}}}, + b: Job{Tasks: []Task{{TaskKey: "a", Description: "1"}, {TaskKey: "a", Description: "changed"}}}, + want: []ResolvedChange{{Field: "tasks[task_key='a'].description", Old: "2", New: "changed"}}, + }, + { + name: "extra in old is deleted", + a: Job{Tasks: []Task{{TaskKey: "a", Description: "1"}, {TaskKey: "a", Description: "2"}}}, + b: Job{Tasks: []Task{{TaskKey: "a", Description: "1"}}}, + want: []ResolvedChange{{Field: "tasks[task_key='a']", Old: Task{TaskKey: "a", Description: "2"}, New: nil}}, + }, + { + name: "extra in new is added", + a: Job{Tasks: []Task{{TaskKey: "a", Description: "1"}}}, + b: Job{Tasks: []Task{{TaskKey: "a", Description: "1"}, {TaskKey: "a", Description: "2"}}}, + want: []ResolvedChange{{Field: "tasks[task_key='a']", Old: nil, New: Task{TaskKey: "a", Description: "2"}}}, + }, + { + name: "two in old one in new with change", + a: Job{Tasks: []Task{{TaskKey: "a", Description: "1"}, {TaskKey: "a", Description: "2"}}}, + b: Job{Tasks: []Task{{TaskKey: "a", Description: "changed"}}}, + want: []ResolvedChange{ + {Field: "tasks[task_key='a'].description", Old: "1", New: "changed"}, + {Field: "tasks[task_key='a']", Old: Task{TaskKey: "a", Description: "2"}, New: nil}, + }, + }, + } + + 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)) + }) + } +} From 0fb5e5b7705b1fa1ef8a5590945a306935db04ba Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 27 Nov 2025 10:25:10 +0100 Subject: [PATCH 06/16] add reorder_locally reorder_remotely --- .../jobs/reorder_locally/databricks.yml | 19 +++++++++++++++++++ .../jobs/reorder_locally/out.test.toml | 5 +++++ .../jobs/reorder_locally/output.txt | 18 ++++++++++++++++++ .../permissions/jobs/reorder_locally/script | 7 +++++++ .../jobs/reorder_locally/test.toml | 1 + .../jobs/reorder_remotely/databricks.yml | 18 ++++++++++++++++++ .../jobs/reorder_remotely/out.test.toml | 5 +++++ .../jobs/reorder_remotely/output.txt | 14 ++++++++++++++ .../jobs/reorder_remotely/reorder.json | 8 ++++++++ .../permissions/jobs/reorder_remotely/script | 8 ++++++++ .../jobs/reorder_remotely/test.toml | 1 + 11 files changed, 104 insertions(+) create mode 100644 acceptance/bundle/resources/permissions/jobs/reorder_locally/databricks.yml create mode 100644 acceptance/bundle/resources/permissions/jobs/reorder_locally/out.test.toml create mode 100644 acceptance/bundle/resources/permissions/jobs/reorder_locally/output.txt create mode 100644 acceptance/bundle/resources/permissions/jobs/reorder_locally/script create mode 100644 acceptance/bundle/resources/permissions/jobs/reorder_locally/test.toml create mode 100644 acceptance/bundle/resources/permissions/jobs/reorder_remotely/databricks.yml create mode 100644 acceptance/bundle/resources/permissions/jobs/reorder_remotely/out.test.toml create mode 100644 acceptance/bundle/resources/permissions/jobs/reorder_remotely/output.txt create mode 100644 acceptance/bundle/resources/permissions/jobs/reorder_remotely/reorder.json create mode 100644 acceptance/bundle/resources/permissions/jobs/reorder_remotely/script create mode 100644 acceptance/bundle/resources/permissions/jobs/reorder_remotely/test.toml diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_locally/databricks.yml b/acceptance/bundle/resources/permissions/jobs/reorder_locally/databricks.yml new file mode 100644 index 0000000000..e1543b2478 --- /dev/null +++ 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 new file mode 100644 index 0000000000..d560f1de04 --- /dev/null +++ b/acceptance/bundle/resources/permissions/jobs/reorder_locally/out.test.toml @@ -0,0 +1,5 @@ +Local = true +Cloud = false + +[EnvMatrix] + DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_locally/output.txt b/acceptance/bundle/resources/permissions/jobs/reorder_locally/output.txt new file mode 100644 index 0000000000..73bedc7d33 --- /dev/null +++ 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 new file mode 100644 index 0000000000..4cd0a20ce7 --- /dev/null +++ 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 new file mode 100644 index 0000000000..a030353d57 --- /dev/null +++ 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 new file mode 100644 index 0000000000..1ec2f87780 --- /dev/null +++ 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 new file mode 100644 index 0000000000..d560f1de04 --- /dev/null +++ b/acceptance/bundle/resources/permissions/jobs/reorder_remotely/out.test.toml @@ -0,0 +1,5 @@ +Local = true +Cloud = false + +[EnvMatrix] + DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_remotely/output.txt b/acceptance/bundle/resources/permissions/jobs/reorder_remotely/output.txt new file mode 100644 index 0000000000..b6052360bf --- /dev/null +++ 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 new file mode 100644 index 0000000000..f826a97da0 --- /dev/null +++ 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 new file mode 100644 index 0000000000..71389de5c9 --- /dev/null +++ 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 new file mode 100644 index 0000000000..a030353d57 --- /dev/null +++ b/acceptance/bundle/resources/permissions/jobs/reorder_remotely/test.toml @@ -0,0 +1 @@ +RecordRequests = false From 5b22be2ec747198d52a44aca2568a62bcaaa6030 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 27 Nov 2025 10:28:20 +0100 Subject: [PATCH 07/16] update tests --- .../jobs/delete_one/local/out.plan_update.direct.json | 2 +- .../jobs/deleted_remotely/out.plan_restore.direct.json | 5 ++++- .../permissions/jobs/update/out.plan_delete_one.direct.json | 2 +- .../permissions/jobs/update/out.plan_update.direct.json | 2 +- .../pipelines/update/out.plan_delete_one.direct.json | 2 +- .../permissions/pipelines/update/out.plan_update.direct.json | 2 +- 6 files changed, 9 insertions(+), 6 deletions(-) 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 index 6109bdb49a..1366b05c71 100644 --- 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 index 0c6b2fcac3..f79a3b3e73 100644 --- 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/update/out.plan_delete_one.direct.json b/acceptance/bundle/resources/permissions/jobs/update/out.plan_delete_one.direct.json index b26a8e0320..4cd774c170 100644 --- 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 index 25f5ffc8c3..b407988457 100644 --- 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 index 4cc6f95d91..76c46e7a58 100644 --- 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 index 66f025be6d..cb547063ee 100644 --- 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" } } From a16bc2d91ff77f2af52a209172c6fd2b83c9b78b Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 27 Nov 2025 10:41:10 +0100 Subject: [PATCH 08/16] work around yaml formatter --- .../resources/permissions/jobs/reorder_locally/databricks.yml | 2 +- .../bundle/resources/permissions/jobs/reorder_locally/script | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_locally/databricks.yml b/acceptance/bundle/resources/permissions/jobs/reorder_locally/databricks.yml index e1543b2478..4a5270fe68 100644 --- a/acceptance/bundle/resources/permissions/jobs/reorder_locally/databricks.yml +++ b/acceptance/bundle/resources/permissions/jobs/reorder_locally/databricks.yml @@ -16,4 +16,4 @@ resources: group_name: data-team - level: CAN_MANAGE service_principal_name: f37d18cd-98a8-4db5-8112-12dd0a6bfe38 -# PLACEHOLDER + # PLACEHOLDER diff --git a/acceptance/bundle/resources/permissions/jobs/reorder_locally/script b/acceptance/bundle/resources/permissions/jobs/reorder_locally/script index 4cd0a20ce7..be6a8540ec 100644 --- a/acceptance/bundle/resources/permissions/jobs/reorder_locally/script +++ b/acceptance/bundle/resources/permissions/jobs/reorder_locally/script @@ -1,6 +1,6 @@ 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"}' +update_file.py databricks.yml ' # PLACEHOLDER' '- {"level": "CAN_VIEW", "user_name": "viewer@example.com"}' trace $CLI bundle plan trace $CLI bundle deploy From 9ee67c5a008d565671365e37fed3cc3d6832ea66 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 27 Nov 2025 10:54:13 +0100 Subject: [PATCH 09/16] update test --- .../permissions/jobs/delete_one/cloud/out.plan.direct.txt | 3 --- .../permissions/jobs/delete_one/cloud/out.plan.terraform.txt | 1 - .../resources/permissions/jobs/delete_one/cloud/output.txt | 1 + .../bundle/resources/permissions/jobs/delete_one/cloud/script | 3 +-- 4 files changed, 2 insertions(+), 6 deletions(-) delete mode 100644 acceptance/bundle/resources/permissions/jobs/delete_one/cloud/out.plan.direct.txt delete mode 100644 acceptance/bundle/resources/permissions/jobs/delete_one/cloud/out.plan.terraform.txt 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 deleted file mode 100644 index 03adb9ae87..0000000000 --- a/acceptance/bundle/resources/permissions/jobs/delete_one/cloud/out.plan.direct.txt +++ /dev/null @@ -1,3 +0,0 @@ -update jobs.job_with_permissions.permissions - -Plan: 0 to add, 1 to change, 0 to delete, 1 unchanged 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 deleted file mode 100644 index 045b1c2486..0000000000 --- a/acceptance/bundle/resources/permissions/jobs/delete_one/cloud/out.plan.terraform.txt +++ /dev/null @@ -1 +0,0 @@ -Plan: 0 to add, 0 to change, 0 to delete, 2 unchanged diff --git a/acceptance/bundle/resources/permissions/jobs/delete_one/cloud/output.txt b/acceptance/bundle/resources/permissions/jobs/delete_one/cloud/output.txt index e229cda82a..b0cd23c833 100644 --- 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 index d9e45e18bd..cc1d9464e1 100644 --- 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') From a6819ca3094ce26af1a69892f118ba3332892743 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 27 Nov 2025 11:10:18 +0100 Subject: [PATCH 10/16] clean up; remove dq support --- libs/structs/structpath/path.go | 20 ++++++++---------- libs/structs/structpath/path_test.go | 31 +++++++++------------------- 2 files changed, 19 insertions(+), 32 deletions(-) diff --git a/libs/structs/structpath/path.go b/libs/structs/structpath/path.go index f6327c4f78..9de58dbbea 100644 --- a/libs/structs/structpath/path.go +++ b/libs/structs/structpath/path.go @@ -14,14 +14,14 @@ const ( // Encodes string key, which is encoded as .field or as ['spark.conf'] tagStringKey = -1 - // Encodes key/value index, which is encoded as [key='value'] or [key="value"] - tagKeyValue = -2 - // Encodes wildcard after a dot: foo.* - tagDotStar = -3 + tagDotStar = -2 // Encodes wildcard in brackets: foo[*] - tagBracketStar = -4 + tagBracketStar = -3 + + // Encodes key/value index, which is encoded as [key='value'] or [key="value"] + tagKeyValue = -4 ) // PathNode represents a node in a path for struct diffing. @@ -226,7 +226,7 @@ func EncodeMapKey(s string) string { // - MAP_KEY_QUOTE: Encountered "'" in map key, expects "'" (escape) or "]" (end) // - WILDCARD: Reading "*" in brackets, expects "]" // - KEYVALUE_KEY: Reading key part of [key='value'], expects identifier chars or "=" -// - KEYVALUE_EQUALS: After key, expecting "'" or '"' to start value +// - KEYVALUE_EQUALS: After key, expecting "'" to start value // - KEYVALUE_VALUE: Reading value content, expects any char or quote // - KEYVALUE_VALUE_QUOTE: Encountered quote in value, expects same quote (escape) or "]" (end) // - EXPECT_DOT_OR_END: After bracket close, expects ".", "[" or end of string @@ -275,7 +275,6 @@ func Parse(s string) (*PathNode, error) { var result *PathNode var currentToken strings.Builder var keyValueKey string // Stores the key part of [key='value'] - var keyValueQuote byte // Stores the quote character used (' or ") pos := 0 for pos < len(s) { @@ -404,15 +403,14 @@ func Parse(s string) (*PathNode, error) { } case stateKeyValueEquals: - if ch == '\'' || ch == '"' { - keyValueQuote = ch + if ch == '\'' { state = stateKeyValueValue } else { return nil, fmt.Errorf("expected quote after '=' but got '%c' at position %d", ch, pos) } case stateKeyValueValue: - if ch == keyValueQuote { + if ch == '\'' { state = stateKeyValueValueQuote } else { currentToken.WriteByte(ch) @@ -420,7 +418,7 @@ func Parse(s string) (*PathNode, error) { case stateKeyValueValueQuote: switch ch { - case keyValueQuote: + case '\'': // Escaped quote - add single quote to value and continue currentToken.WriteByte(ch) state = stateKeyValueValue diff --git a/libs/structs/structpath/path_test.go b/libs/structs/structpath/path_test.go index da92c8a85e..42fcd34cc7 100644 --- a/libs/structs/structpath/path_test.go +++ b/libs/structs/structpath/path_test.go @@ -216,27 +216,6 @@ func TestPathNode(t *testing.T) { }, } - // Additional test for parsing double-quoted key-value - t.Run("key value with double quotes parses correctly", func(t *testing.T) { - parsed, err := Parse(`[name="foo"]`) - assert.NoError(t, err) - key, value, ok := parsed.KeyValue() - assert.True(t, ok) - assert.Equal(t, "name", key) - assert.Equal(t, "foo", value) - // Serializes with single quotes - assert.Equal(t, "[name='foo']", parsed.String()) - }) - - t.Run("key value with double quotes and escaped double quote", func(t *testing.T) { - parsed, err := Parse(`[name="it""s"]`) - assert.NoError(t, err) - key, value, ok := parsed.KeyValue() - assert.True(t, ok) - assert.Equal(t, "name", key) - assert.Equal(t, `it"s`, value) - }) - for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Test String() method @@ -489,6 +468,16 @@ func TestParseErrors(t *testing.T) { input: "[name='value'x]", error: "unexpected character 'x' after quote in key-value at position 13", }, + { + name: "double quotes are not supported a.t.m", + input: "[name=\"value\"]", + error: "expected quote after '=' but got '\"' at position 6", + }, + { + name: "mixed quotes never going to be supported", + input: "[name='value\"]", + error: "unexpected end of input while parsing key-value value", + }, } for _, tt := range tests { From 07721eb1ad3043f324afd47914761ee69581525a Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 27 Nov 2025 11:17:32 +0100 Subject: [PATCH 11/16] rename --- bundle/direct/dresources/adapter.go | 4 +-- libs/structs/structdiff/diff.go | 40 ++++++++++++++-------------- libs/structs/structdiff/diff_test.go | 26 +++++++++--------- 3 files changed, 35 insertions(+), 35 deletions(-) diff --git a/bundle/direct/dresources/adapter.go b/bundle/direct/dresources/adapter.go index 36a03e8a53..100b7561ef 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -84,7 +84,7 @@ 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 SliceKeyFunc for comparing slices by key instead of by index. + // [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(state any) map[string]any } @@ -648,7 +648,7 @@ func (a *Adapter) ClassifyChange(change structdiff.Change, remoteState any, isLo return actionType, nil } -// KeyedSlices returns a map from path patterns to SliceKeyFunc for comparing slices by key. +// 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 diff --git a/libs/structs/structdiff/diff.go b/libs/structs/structdiff/diff.go index 6ad763b31e..b67c40fc9b 100644 --- a/libs/structs/structdiff/diff.go +++ b/libs/structs/structdiff/diff.go @@ -17,47 +17,47 @@ type Change struct { New any } -// SliceKeyFunc extracts a key field name and value from a slice element. +// KeyFunc extracts a key field name and value from a slice element. // It can be either: // - func(T) (string, string, error) - typed function for specific element type T // - func(any) (string, string, error) - generic function accepting any element // // The function returns (keyField, keyValue, error). The keyField is typically a field name // like "task_key", and keyValue is the value that uniquely identifies the element. -type SliceKeyFunc = any +type KeyFunc = any -// sliceKeyFuncCaller wraps a SliceKeyFunc and provides a type-checked Call method. -type sliceKeyFuncCaller struct { +// keyFuncCaller wraps a KeyFunc and provides a type-checked Call method. +type keyFuncCaller struct { fn reflect.Value argType reflect.Type } -func newSliceKeyFuncCaller(fn any) (*sliceKeyFuncCaller, error) { +func newKeyFuncCaller(fn any) (*keyFuncCaller, error) { v := reflect.ValueOf(fn) if v.Kind() != reflect.Func { - return nil, fmt.Errorf("SliceKeyFunc must be a function, got %T", fn) + return nil, fmt.Errorf("KeyFunc must be a function, got %T", fn) } t := v.Type() if t.NumIn() != 1 { - return nil, fmt.Errorf("SliceKeyFunc must have exactly 1 parameter, got %d", t.NumIn()) + return nil, fmt.Errorf("KeyFunc must have exactly 1 parameter, got %d", t.NumIn()) } if t.NumOut() != 3 { - return nil, fmt.Errorf("SliceKeyFunc must return exactly 3 values, got %d", t.NumOut()) + return nil, fmt.Errorf("KeyFunc must return exactly 3 values, got %d", t.NumOut()) } if t.Out(0).Kind() != reflect.String || t.Out(1).Kind() != reflect.String { - return nil, fmt.Errorf("SliceKeyFunc must return (string, string, error), got (%v, %v, %v)", t.Out(0), t.Out(1), t.Out(2)) + return nil, fmt.Errorf("KeyFunc must return (string, string, error), got (%v, %v, %v)", t.Out(0), t.Out(1), t.Out(2)) } errType := reflect.TypeOf((*error)(nil)).Elem() if !t.Out(2).Implements(errType) && t.Out(2) != errType { - return nil, fmt.Errorf("SliceKeyFunc third return must be error, got %v", t.Out(2)) + return nil, fmt.Errorf("KeyFunc third return must be error, got %v", t.Out(2)) } - return &sliceKeyFuncCaller{fn: v, argType: t.In(0)}, nil + return &keyFuncCaller{fn: v, argType: t.In(0)}, nil } -func (c *sliceKeyFuncCaller) call(elem any) (string, string, error) { +func (c *keyFuncCaller) call(elem any) (string, string, error) { elemValue := reflect.ValueOf(elem) if !elemValue.Type().AssignableTo(c.argType) { - return "", "", fmt.Errorf("SliceKeyFunc expects %v, got %T", c.argType, elem) + return "", "", fmt.Errorf("KeyFunc expects %v, got %T", c.argType, elem) } out := c.fn.Call([]reflect.Value{elemValue}) keyField := out[0].String() @@ -71,7 +71,7 @@ func (c *sliceKeyFuncCaller) call(elem any) (string, string, error) { // diffContext holds configuration for the diff operation. type diffContext struct { - sliceKeys map[string]SliceKeyFunc + sliceKeys map[string]KeyFunc } // GetStructDiff compares two Go structs and returns a list of Changes or an error. @@ -84,7 +84,7 @@ type diffContext struct { // Path patterns use dot notation (e.g., "tasks" or "job.tasks"). // The [*] wildcard matches any slice index in the path. // Pass nil if no slice key functions are needed. -func GetStructDiff(a, b any, sliceKeys map[string]SliceKeyFunc) ([]Change, error) { +func GetStructDiff(a, b any, sliceKeys map[string]KeyFunc) ([]Change, error) { v1 := reflect.ValueOf(a) v2 := reflect.ValueOf(b) @@ -159,7 +159,7 @@ func diffValues(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect.Valu case reflect.Struct: return diffStruct(ctx, path, v1, v2, changes) case reflect.Slice, reflect.Array: - if keyFunc := ctx.findSliceKeyFunc(path); keyFunc != nil { + if keyFunc := ctx.findKeyFunc(path); keyFunc != nil { return diffSliceByKey(ctx, path, v1, v2, keyFunc, changes) } else if v1.Len() != v2.Len() { *changes = append(*changes, Change{Path: path, Old: v1.Interface(), New: v2.Interface()}) @@ -290,9 +290,9 @@ func getForceSendFields(v reflect.Value) []string { return nil } -// findSliceKeyFunc returns the SliceKeyFunc for the given path, or nil if none matches. +// findKeyFunc returns the KeyFunc for the given path, or nil if none matches. // Path patterns support [*] to match any slice index. -func (ctx *diffContext) findSliceKeyFunc(path *structpath.PathNode) SliceKeyFunc { +func (ctx *diffContext) findKeyFunc(path *structpath.PathNode) KeyFunc { if ctx.sliceKeys == nil { return nil } @@ -344,8 +344,8 @@ type sliceElement struct { // 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. -func diffSliceByKey(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect.Value, keyFunc SliceKeyFunc, changes *[]Change) error { - caller, err := newSliceKeyFuncCaller(keyFunc) +func diffSliceByKey(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect.Value, keyFunc KeyFunc, changes *[]Change) error { + caller, err := newKeyFuncCaller(keyFunc) if err != nil { return err } diff --git a/libs/structs/structdiff/diff_test.go b/libs/structs/structdiff/diff_test.go index 4fa16fe550..ea0a15d95b 100644 --- a/libs/structs/structdiff/diff_test.go +++ b/libs/structs/structdiff/diff_test.go @@ -453,7 +453,7 @@ func taskKeyFunc(task Task) (string, string, error) { } func TestGetStructDiffSliceKeys(t *testing.T) { - sliceKeys := map[string]SliceKeyFunc{ + sliceKeys := map[string]KeyFunc{ "tasks": taskKeyFunc, } @@ -539,7 +539,7 @@ func itemKeyFunc(item Item) (string, string, error) { } func TestGetStructDiffNestedSliceKeys(t *testing.T) { - sliceKeys := map[string]SliceKeyFunc{ + sliceKeys := map[string]KeyFunc{ "nested[*].items": itemKeyFunc, } @@ -586,43 +586,43 @@ func TestGetStructDiffSliceKeysInvalidFunc(t *testing.T) { { name: "not a function", keyFunc: "not a function", - errMsg: "SliceKeyFunc must be a function, got string", + errMsg: "KeyFunc must be a function, got string", }, { name: "wrong number of parameters", keyFunc: func() (string, string, error) { return "", "", nil }, - errMsg: "SliceKeyFunc must have exactly 1 parameter, got 0", + errMsg: "KeyFunc must have exactly 1 parameter, got 0", }, { name: "too many parameters", keyFunc: func(a, b Task) (string, string, error) { return "", "", nil }, - errMsg: "SliceKeyFunc must have exactly 1 parameter, got 2", + errMsg: "KeyFunc must have exactly 1 parameter, got 2", }, { name: "wrong number of returns", keyFunc: func(t Task) string { return "" }, - errMsg: "SliceKeyFunc must return exactly 3 values, got 1", + errMsg: "KeyFunc must return exactly 3 values, got 1", }, { name: "wrong first return type", keyFunc: func(t Task) (int, string, error) { return 0, "", nil }, - errMsg: "SliceKeyFunc must return (string, string, error), got (int, string, error)", + errMsg: "KeyFunc must return (string, string, error), got (int, string, error)", }, { name: "wrong second return type", keyFunc: func(t Task) (string, int, error) { return "", 0, nil }, - errMsg: "SliceKeyFunc must return (string, string, error), got (string, int, error)", + errMsg: "KeyFunc must return (string, string, error), got (string, int, error)", }, { name: "wrong third return type", keyFunc: func(t Task) (string, string, string) { return "", "", "" }, - errMsg: "SliceKeyFunc third return must be error, got string", + errMsg: "KeyFunc third return must be error, got string", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - sliceKeys := map[string]SliceKeyFunc{"tasks": tt.keyFunc} + sliceKeys := map[string]KeyFunc{"tasks": tt.keyFunc} a := Job{Tasks: []Task{{TaskKey: "a"}}} b := Job{Tasks: []Task{{TaskKey: "a"}}} _, err := GetStructDiff(a, b, sliceKeys) @@ -633,7 +633,7 @@ func TestGetStructDiffSliceKeysInvalidFunc(t *testing.T) { func TestGetStructDiffSliceKeysWrongArgType(t *testing.T) { // Function expects Item but slice contains Task - sliceKeys := map[string]SliceKeyFunc{ + sliceKeys := map[string]KeyFunc{ "tasks": func(item Item) (string, string, error) { return "id", item.ID, nil }, @@ -641,11 +641,11 @@ func TestGetStructDiffSliceKeysWrongArgType(t *testing.T) { a := Job{Tasks: []Task{{TaskKey: "a"}}} b := Job{Tasks: []Task{{TaskKey: "b"}}} _, err := GetStructDiff(a, b, sliceKeys) - assert.EqualError(t, err, "SliceKeyFunc expects structdiff.Item, got structdiff.Task") + assert.EqualError(t, err, "KeyFunc expects structdiff.Item, got structdiff.Task") } func TestGetStructDiffSliceKeysDuplicates(t *testing.T) { - sliceKeys := map[string]SliceKeyFunc{ + sliceKeys := map[string]KeyFunc{ "tasks": taskKeyFunc, } From f4f5496d53571f4ca985c93eb17cb2d257ce94b9 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 27 Nov 2025 11:32:02 +0100 Subject: [PATCH 12/16] clean up --- libs/structs/structdiff/diff.go | 1 - 1 file changed, 1 deletion(-) diff --git a/libs/structs/structdiff/diff.go b/libs/structs/structdiff/diff.go index b67c40fc9b..14e548f5f5 100644 --- a/libs/structs/structdiff/diff.go +++ b/libs/structs/structdiff/diff.go @@ -302,7 +302,6 @@ func (ctx *diffContext) findKeyFunc(path *structpath.PathNode) KeyFunc { // pathToPattern converts a PathNode to a pattern string for matching. // Slice indices are converted to [*] wildcard. -// XXX why indices are special; why do we need this; if we need this, should it be in structpath instead? func pathToPattern(path *structpath.PathNode) string { if path == nil { return "" From 94ca23b386047e585dda3f63482617fdf00dfac7 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 27 Nov 2025 11:33:20 +0100 Subject: [PATCH 13/16] add a comment about key wildcard --- libs/structs/structdiff/diff.go | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/structs/structdiff/diff.go b/libs/structs/structdiff/diff.go index 14e548f5f5..516a445fe1 100644 --- a/libs/structs/structdiff/diff.go +++ b/libs/structs/structdiff/diff.go @@ -83,6 +83,7 @@ type diffContext struct { // paths are compared as maps keyed by (keyField, keyValue) instead of by index. // 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. func GetStructDiff(a, b any, sliceKeys map[string]KeyFunc) ([]Change, error) { v1 := reflect.ValueOf(a) From 0987eab9fdec4ae48fb6ff9e947281f03ff46471 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 27 Nov 2025 14:50:27 +0100 Subject: [PATCH 14/16] simplify KeyFunc to drop error return; validate arguments in advance --- bundle/direct/dresources/permissions.go | 10 ++--- libs/structs/structdiff/diff.go | 58 +++++++++++++------------ libs/structs/structdiff/diff_test.go | 31 ++++++------- 3 files changed, 49 insertions(+), 50 deletions(-) diff --git a/bundle/direct/dresources/permissions.go b/bundle/direct/dresources/permissions.go index 90eca157c2..df4ddc765f 100644 --- a/bundle/direct/dresources/permissions.go +++ b/bundle/direct/dresources/permissions.go @@ -85,17 +85,17 @@ func (*ResourcePermissions) PrepareState(s *PermissionsState) *PermissionsState return s } -func accessControlRequestKey(x iam.AccessControlRequest) (string, string, error) { +func accessControlRequestKey(x iam.AccessControlRequest) (string, string) { if x.UserName != "" { - return "user_name", x.UserName, nil + return "user_name", x.UserName } if x.ServicePrincipalName != "" { - return "service_principal_name", x.ServicePrincipalName, nil + return "service_principal_name", x.ServicePrincipalName } if x.GroupName != "" { - return "group_name", x.GroupName, nil + return "group_name", x.GroupName } - return "", "", fmt.Errorf("no key found in AccessControlRequest: %+v", x) + return "", "" } func (*ResourcePermissions) KeyedSlices(s *PermissionsState) map[string]any { diff --git a/libs/structs/structdiff/diff.go b/libs/structs/structdiff/diff.go index 516a445fe1..160f14b649 100644 --- a/libs/structs/structdiff/diff.go +++ b/libs/structs/structdiff/diff.go @@ -19,10 +19,10 @@ type Change struct { // KeyFunc extracts a key field name and value from a slice element. // It can be either: -// - func(T) (string, string, error) - typed function for specific element type T -// - func(any) (string, string, error) - generic function accepting any element +// - 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, error). The keyField is typically a field name +// 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. type KeyFunc = any @@ -41,32 +41,21 @@ func newKeyFuncCaller(fn any) (*keyFuncCaller, error) { if t.NumIn() != 1 { return nil, fmt.Errorf("KeyFunc must have exactly 1 parameter, got %d", t.NumIn()) } - if t.NumOut() != 3 { - return nil, fmt.Errorf("KeyFunc must return exactly 3 values, got %d", t.NumOut()) + if t.NumOut() != 2 { + return nil, fmt.Errorf("KeyFunc must return exactly 2 values, got %d", t.NumOut()) } if t.Out(0).Kind() != reflect.String || t.Out(1).Kind() != reflect.String { - return nil, fmt.Errorf("KeyFunc must return (string, string, error), got (%v, %v, %v)", t.Out(0), t.Out(1), t.Out(2)) - } - errType := reflect.TypeOf((*error)(nil)).Elem() - if !t.Out(2).Implements(errType) && t.Out(2) != errType { - return nil, fmt.Errorf("KeyFunc third return must be error, got %v", t.Out(2)) + return nil, fmt.Errorf("KeyFunc must return (string, string), got (%v, %v)", t.Out(0), t.Out(1)) } return &keyFuncCaller{fn: v, argType: t.In(0)}, nil } -func (c *keyFuncCaller) call(elem any) (string, string, error) { +func (c *keyFuncCaller) call(elem any) (string, string) { elemValue := reflect.ValueOf(elem) - if !elemValue.Type().AssignableTo(c.argType) { - return "", "", fmt.Errorf("KeyFunc expects %v, got %T", c.argType, elem) - } out := c.fn.Call([]reflect.Value{elemValue}) keyField := out[0].String() keyValue := out[1].String() - var err error - if !out[2].IsNil() { - err = out[2].Interface().(error) - } - return keyField, keyValue, err + return keyField, keyValue } // diffContext holds configuration for the diff operation. @@ -341,6 +330,19 @@ type sliceElement struct { value reflect.Value } +// validateKeyFuncElementType verifies that the first element type in the sequence +// is assignable to the expected type. If the sequence is empty, it succeeds. +func validateKeyFuncElementType(seq reflect.Value, expected reflect.Type) error { + if seq.Len() == 0 { + return nil + } + elem := seq.Index(0) + if !elem.Type().AssignableTo(expected) { + return fmt.Errorf("KeyFunc expects %v, got %v", expected, elem.Type()) + } + return nil +} + // 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. @@ -350,6 +352,14 @@ func diffSliceByKey(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect. return err } + // Validate element types up-front to avoid runtime panics and to return a clear error. + if err := validateKeyFuncElementType(v1, caller.argType); err != nil { + return err + } + if err := validateKeyFuncElementType(v2, caller.argType); err != nil { + return err + } + // Build lists of elements grouped by key, preserving order within each key elements1 := make(map[string][]sliceElement) elements2 := make(map[string][]sliceElement) @@ -359,10 +369,7 @@ func diffSliceByKey(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect. // Build from first slice for i := range v1.Len() { elem := v1.Index(i) - keyField, keyValue, err := caller.call(elem.Interface()) - if err != nil { - return err - } + keyField, keyValue := caller.call(elem.Interface()) elements1[keyValue] = append(elements1[keyValue], sliceElement{keyField: keyField, keyValue: keyValue, value: elem}) if !seen[keyValue] { seen[keyValue] = true @@ -373,10 +380,7 @@ func diffSliceByKey(ctx *diffContext, path *structpath.PathNode, v1, v2 reflect. // Build from second slice for i := range v2.Len() { elem := v2.Index(i) - keyField, keyValue, err := caller.call(elem.Interface()) - if err != nil { - return err - } + keyField, keyValue := caller.call(elem.Interface()) elements2[keyValue] = append(elements2[keyValue], sliceElement{keyField: keyField, keyValue: keyValue, value: elem}) if !seen[keyValue] { seen[keyValue] = true diff --git a/libs/structs/structdiff/diff_test.go b/libs/structs/structdiff/diff_test.go index ea0a15d95b..9a216e0c4f 100644 --- a/libs/structs/structdiff/diff_test.go +++ b/libs/structs/structdiff/diff_test.go @@ -448,8 +448,8 @@ type Job struct { Tasks []Task `json:"tasks,omitempty"` } -func taskKeyFunc(task Task) (string, string, error) { - return "task_key", task.TaskKey, nil +func taskKeyFunc(task Task) (string, string) { + return "task_key", task.TaskKey } func TestGetStructDiffSliceKeys(t *testing.T) { @@ -534,8 +534,8 @@ type Root struct { Nested []Nested `json:"nested,omitempty"` } -func itemKeyFunc(item Item) (string, string, error) { - return "id", item.ID, nil +func itemKeyFunc(item Item) (string, string) { + return "id", item.ID } func TestGetStructDiffNestedSliceKeys(t *testing.T) { @@ -590,33 +590,28 @@ func TestGetStructDiffSliceKeysInvalidFunc(t *testing.T) { }, { name: "wrong number of parameters", - keyFunc: func() (string, string, error) { return "", "", nil }, + keyFunc: func() (string, string) { return "", "" }, errMsg: "KeyFunc must have exactly 1 parameter, got 0", }, { name: "too many parameters", - keyFunc: func(a, b Task) (string, string, error) { return "", "", nil }, + keyFunc: func(a, b Task) (string, string) { return "", "" }, errMsg: "KeyFunc must have exactly 1 parameter, got 2", }, { name: "wrong number of returns", keyFunc: func(t Task) string { return "" }, - errMsg: "KeyFunc must return exactly 3 values, got 1", + errMsg: "KeyFunc must return exactly 2 values, got 1", }, { name: "wrong first return type", - keyFunc: func(t Task) (int, string, error) { return 0, "", nil }, - errMsg: "KeyFunc must return (string, string, error), got (int, string, error)", + keyFunc: func(t Task) (int, string) { return 0, "" }, + errMsg: "KeyFunc must return (string, string), got (int, string)", }, { name: "wrong second return type", - keyFunc: func(t Task) (string, int, error) { return "", 0, nil }, - errMsg: "KeyFunc must return (string, string, error), got (string, int, error)", - }, - { - name: "wrong third return type", - keyFunc: func(t Task) (string, string, string) { return "", "", "" }, - errMsg: "KeyFunc third return must be error, got string", + keyFunc: func(t Task) (string, int) { return "", 0 }, + errMsg: "KeyFunc must return (string, string), got (string, int)", }, } @@ -634,8 +629,8 @@ func TestGetStructDiffSliceKeysInvalidFunc(t *testing.T) { func TestGetStructDiffSliceKeysWrongArgType(t *testing.T) { // Function expects Item but slice contains Task sliceKeys := map[string]KeyFunc{ - "tasks": func(item Item) (string, string, error) { - return "id", item.ID, nil + "tasks": func(item Item) (string, string) { + return "id", item.ID }, } a := Job{Tasks: []Task{{TaskKey: "a"}}} From 9cb62616f757c39fded9e843e0bf797e7345401e Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 27 Nov 2025 16:10:10 +0100 Subject: [PATCH 15/16] clean up --- bundle/direct/dresources/adapter.go | 15 ++------------- bundle/direct/dresources/permissions.go | 2 +- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/bundle/direct/dresources/adapter.go b/bundle/direct/dresources/adapter.go index 100b7561ef..e747cb6fca 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -86,7 +86,7 @@ type IResource interface { // [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(state any) map[string]any + KeyedSlices() map[string]any } // Adapter wraps resource implementation, validates signatures and type consistency across methods @@ -202,18 +202,7 @@ func loadFieldTriggers(triggerCall *calladapt.BoundCaller, isLocal bool) (map[st // loadKeyedSlices validates and calls KeyedSlices method, returning the resulting map. func loadKeyedSlices(call *calladapt.BoundCaller) (map[string]any, error) { - // Validate signature: func(stateType) map[string]any - if len(call.InTypes) != 1 { - return nil, fmt.Errorf("KeyedSlices must take exactly 1 parameter, got %d", len(call.InTypes)) - } - expectedReturnType := reflect.TypeOf(map[string]any{}) - if len(call.OutTypes) != 1 || call.OutTypes[0] != expectedReturnType { - return nil, fmt.Errorf("KeyedSlices must return map[string]any, got %v", call.OutTypes) - } - - // Create a nil pointer of the expected type (KeyedSlices implementations should not depend on the state value) - nilArg := reflect.Zero(call.InTypes[0]).Interface() - outs, err := call.Call(nilArg) + outs, err := call.Call() if err != nil { return nil, fmt.Errorf("failed to call KeyedSlices: %w", err) } diff --git a/bundle/direct/dresources/permissions.go b/bundle/direct/dresources/permissions.go index df4ddc765f..9fb407749e 100644 --- a/bundle/direct/dresources/permissions.go +++ b/bundle/direct/dresources/permissions.go @@ -98,7 +98,7 @@ func accessControlRequestKey(x iam.AccessControlRequest) (string, string) { return "", "" } -func (*ResourcePermissions) KeyedSlices(s *PermissionsState) map[string]any { +func (*ResourcePermissions) KeyedSlices() map[string]any { return map[string]any{ "permissions": accessControlRequestKey, } From 575f2ba730d2c9ddbbb184b2662d9dd0bb1becc8 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 27 Nov 2025 16:23:18 +0100 Subject: [PATCH 16/16] remove isKeyValueKeyChar; use !isReserved --- libs/structs/structpath/path.go | 9 ++------- libs/structs/structpath/path_test.go | 2 +- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/libs/structs/structpath/path.go b/libs/structs/structpath/path.go index 9de58dbbea..1627ae6f7d 100644 --- a/libs/structs/structpath/path.go +++ b/libs/structs/structpath/path.go @@ -338,7 +338,7 @@ func Parse(s string) (*PathNode, error) { state = stateMapKey } else if ch == '*' { state = stateWildcard - } else if isKeyValueKeyChar(ch) { + } else if !isReservedFieldChar(ch) { currentToken.WriteByte(ch) state = stateKeyValueKey } else { @@ -396,7 +396,7 @@ func Parse(s string) (*PathNode, error) { keyValueKey = currentToken.String() currentToken.Reset() state = stateKeyValueEquals - } else if isKeyValueKeyChar(ch) { + } else if !isReservedFieldChar(ch) { currentToken.WriteByte(ch) } else { return nil, fmt.Errorf("unexpected character '%c' in key-value key at position %d", ch, pos) @@ -491,11 +491,6 @@ func Parse(s string) (*PathNode, error) { } } -// isKeyValueKeyChar checks if character is valid for key-value key (identifier-like) -func isKeyValueKeyChar(ch byte) bool { - return (ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || (ch >= '0' && ch <= '9') || ch == '_' -} - // isReservedFieldChar checks if character is reserved and cannot be used in field names func isReservedFieldChar(ch byte) bool { switch ch { diff --git a/libs/structs/structpath/path_test.go b/libs/structs/structpath/path_test.go index 42fcd34cc7..c4e427666d 100644 --- a/libs/structs/structpath/path_test.go +++ b/libs/structs/structpath/path_test.go @@ -322,7 +322,7 @@ func TestParseErrors(t *testing.T) { { name: "negative array index", input: "[-1]", - error: "unexpected character '-' after '[' at position 1", + error: "unexpected character ']' in key-value key at position 3", }, { name: "unclosed map key quote",