From 6f09c49f64f95d66b2f722d494f576a36c51031f Mon Sep 17 00:00:00 2001 From: Ilya Kuznetsov Date: Mon, 3 Aug 2026 17:01:09 +0000 Subject: [PATCH] Simplify rename pairing and index scoping --- bundle/configsync/blockindex.go | 6 ++-- bundle/configsync/rename.go | 57 +++++++++++++++------------------ bundle/configsync/resolve.go | 15 ++++++--- 3 files changed, 38 insertions(+), 40 deletions(-) diff --git a/bundle/configsync/blockindex.go b/bundle/configsync/blockindex.go index 20908379ad..02247bbb3a 100644 --- a/bundle/configsync/blockindex.go +++ b/bundle/configsync/blockindex.go @@ -446,10 +446,8 @@ func (r *blockResolver) blocksDefiningSequence(sequencePath dyn.Path) []sourceBl if err != nil { continue } - for _, location := range sequence.Locations() { - if location.File == block.file && !slices.Contains(blocks, block) { - blocks = append(blocks, block) - } + if slices.ContainsFunc(sequence.Locations(), func(l dyn.Location) bool { return l.File == block.file }) { + blocks = append(blocks, block) } } return blocks diff --git a/bundle/configsync/rename.go b/bundle/configsync/rename.go index bda769b010..3231ed17a4 100644 --- a/bundle/configsync/rename.go +++ b/bundle/configsync/rename.go @@ -54,6 +54,19 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch removes, adds := keyedElementChanges(changes) for _, remove := range removes { + // The remove half carries no value, so the old element is read from the + // merged configuration. Resolving it once also gives the sequence steps that + // multiBlockElement needs below. + resolved, err := resolveSelectors(resourceKey+"."+remove.path, b, OperationRemove) + if err != nil || !resolved.leaf.IsValid() { + continue + } + element, ok := resolved.leaf.AsAny().(map[string]any) + if !ok { + continue + } + oldFields := withoutKey(element, remove.keyField) + // Every add the removed element could equally well have become. Two // identically-bodied elements renamed in one run produce two removes that // each match both adds, and the pairing decides which key goes to which @@ -69,7 +82,7 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch if remove.parent != add.parent || remove.keyField != add.keyField { continue } - if sameElementApartFromKey(b, resourceKey, remove, add) { + if sameElementApartFromKey(oldFields, add) { matches = append(matches, add) } } @@ -112,14 +125,14 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch if _, taken := set.addPaths[add.path]; taken { continue } - if sameFieldsApartFromKey(b, resourceKey, remove, add) { + if sameFieldsApartFromKey(oldFields, add) { candidates = append(candidates, add.path) } } if len(candidates) == 0 { continue } - if !multiBlockElement(b, blocks, resourceKey, remove.path) { + if !multiBlockElement(blocks, resolved) { continue } const reason = "a split element cannot be removed and recreated in one run" @@ -131,14 +144,13 @@ func pairRenames(b *bundle.Bundle, blocks *blockResolver, resourceKey string, ch return set } -// multiBlockElement reports whether the element at path is assembled from more -// than one physical block. -func multiBlockElement(b *bundle.Bundle, blocks *blockResolver, resourceKey, path string) bool { - resolved, err := resolveSelectors(resourceKey+"."+path, b, OperationRemove) - if err != nil || len(resolved.steps) == 0 { +// multiBlockElement reports whether the element the change addresses is assembled +// from more than one physical block. +func multiBlockElement(blocks *blockResolver, change resolvedChange) bool { + if len(change.steps) == 0 { return false } - last := resolved.steps[len(resolved.steps)-1] + last := change.steps[len(change.steps)-1] return len(blocks.blocksOf(last.element)) > 1 } @@ -184,42 +196,25 @@ func keyedElementChanges(changes ResourceChanges) (removes, adds []keyedElement) } // sameElementApartFromKey reports whether the added element is the removed one -// with a different key. The remove half carries no value, so the old element is -// read from the merged configuration. -func sameElementApartFromKey(b *bundle.Bundle, resourceKey string, remove, add keyedElement) bool { - resolved, err := resolveSelectors(resourceKey+"."+remove.path, b, OperationRemove) - if err != nil || !resolved.leaf.IsValid() { - return false - } - oldValue, ok := resolved.leaf.AsAny().(map[string]any) - if !ok { - return false - } +// with a different key. oldFields is the removed element without its key field. +func sameElementApartFromKey(oldFields map[string]any, add keyedElement) bool { newValue, ok := add.value.(map[string]any) if !ok { return false } - return reflect.DeepEqual(withoutKey(oldValue, remove.keyField), withoutKey(newValue, add.keyField)) + return reflect.DeepEqual(oldFields, withoutKey(newValue, add.keyField)) } // sameFieldsApartFromKey reports whether the added element has the same fields as // the removed one, ignoring their values. A rename that also edited a field keeps // the element's shape; an unrelated new element generally does not. -func sameFieldsApartFromKey(b *bundle.Bundle, resourceKey string, remove, add keyedElement) bool { - resolved, err := resolveSelectors(resourceKey+"."+remove.path, b, OperationRemove) - if err != nil || !resolved.leaf.IsValid() { - return false - } - oldValue, ok := resolved.leaf.AsAny().(map[string]any) - if !ok { - return false - } +func sameFieldsApartFromKey(oldFields map[string]any, add keyedElement) bool { newValue, ok := add.value.(map[string]any) if !ok { return false } return slices.Equal( - slices.Sorted(maps.Keys(withoutKey(oldValue, remove.keyField))), + slices.Sorted(maps.Keys(oldFields)), slices.Sorted(maps.Keys(withoutKey(newValue, add.keyField))), ) } diff --git a/bundle/configsync/resolve.go b/bundle/configsync/resolve.go index 3d61b6df2f..73bb3cc5ed 100644 --- a/bundle/configsync/resolve.go +++ b/bundle/configsync/resolve.go @@ -164,6 +164,12 @@ func pathDepth(pathStr string) int { return len(node.AsSlice()) } +// scopedParent keys index bookkeeping by the sequence a path addresses, within the +// scope of one block, so operations on one block cannot shift indices in another. +func scopedParent(scope string, path *structpath.PatternNode) string { + return scope + path.Parent().String() +} + // adjustArrayIndex adjusts the index in a PatternNode based on previous operations. // When operations are applied sequentially, removals and additions shift array indices. // This function adjusts the index to account for those shifts. @@ -178,8 +184,7 @@ func adjustArrayIndex(path *structpath.PatternNode, scope string, operations map } parentPath := path.Parent() - parentPathStr := scope + parentPath.String() - ops := operations[parentPathStr] + ops := operations[scopedParent(scope, path)] adjustment := 0 for _, op := range ops { @@ -348,13 +353,13 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes if destChange.Operation == OperationRemove { freeIndex, ok := resolvedPath.Index() if ok { - parentPath := scope + resolvedPath.Parent().String() + parentPath := scopedParent(scope, resolvedPath) indicesToReplaceMap[parentPath] = append(indicesToReplaceMap[parentPath], freeIndex) } } if destChange.Operation == OperationAdd && resolvedPath.BracketStar() { - parentPath := scope + resolvedPath.Parent().String() + parentPath := scopedParent(scope, resolvedPath) indices, ok := indicesToReplaceMap[parentPath] if ok && len(indices) > 0 { index := indices[0] @@ -367,7 +372,7 @@ func ResolveChanges(ctx context.Context, b *bundle.Bundle, configChanges Changes // Track this operation for future index adjustments (only for array element operations) if originalIndex, ok := resolvedPath.Index(); ok { - parentPath := scope + resolvedPath.Parent().String() + parentPath := scopedParent(scope, resolvedPath) indexOperations[parentPath] = append(indexOperations[parentPath], struct { index int operation OperationType