From dcf0379e0274ab7b6fb9bec5f26501b6c0222ce5 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 24 Oct 2026 00:11:21 +0000 Subject: [PATCH] Apply nested collection operations when copying mappings --- docs/roadmap/remediation.md | 11 +- internal/engine/collection_ancestor_test.go | 106 ++++++++++++++++++++ internal/engine/inheritance_schedule.go | 6 +- internal/engine/item_policy_test.go | 16 +-- internal/engine/recursive_merge.go | 46 ++++++--- 5 files changed, 156 insertions(+), 29 deletions(-) create mode 100644 internal/engine/collection_ancestor_test.go diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index a21a907..c3a0340 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -219,7 +219,16 @@ without a replacement value and during append/upsert. ## 6. Enforce Collection Operations Beneath Newly Added Ancestors — P2 -Status: [ ] Open. +Status: [x] Complete. + +Completion: copying a mapping now visits nested collection operations with absent +lower values, preserving ordinary ancestor removal and whole-record semantics. +Incoming record-owned policy transfers through the same operation. Regressions +cover absent/empty/null ancestors, identity and kind failures, empty and valid +initialization, and later inheritance using an imported item's nested identity. +Corrected an older test that deferred the required error until a later tier. +Verified focused merge/collection/inheritance tests, focused ownership race +coverage, and `./scripts/check.sh`. Owners: [`mergeRecursiveMappings`](../../internal/engine/recursive_merge.go) and [`mergeOverlayMappingsWithCollectionPolicy`](../../internal/engine/overlay.go). diff --git a/internal/engine/collection_ancestor_test.go b/internal/engine/collection_ancestor_test.go new file mode 100644 index 0000000..e059fc3 --- /dev/null +++ b/internal/engine/collection_ancestor_test.go @@ -0,0 +1,106 @@ +package engine + +import ( + "errors" + "fmt" + "testing" + "testing/fstest" +) + +func TestNestedCollectionInitializationIsIndependentOfAncestorPresence(t *testing.T) { + t.Parallel() + for _, ancestor := range []string{"{}", "group: {}", "group: null"} { + for _, test := range []struct { + mode, key, value string + valid bool + }{ + {mode: "append", value: "[{id: a}]"}, + {mode: "upsert", value: "[{id: a}]"}, + {mode: "append", value: "[]"}, + {mode: "upsert", value: "[]"}, + {mode: "append", key: ", key: id", value: "{}"}, + {mode: "upsert", key: ", key: id", value: "null"}, + {mode: "append", key: ", key: id", value: "[{id: a}]", valid: true}, + {mode: "upsert", key: ", key: id", value: "[]", valid: true}, + } { + t.Run(fmt.Sprintf("%s/%s/%s/%s", ancestor, test.mode, test.key, test.value), func(t *testing.T) { + main := "configkit:\n sequences: [{destination: [group, items], mode: " + test.mode + test.key + "}]\ngroup: {items: " + test.value + "}\n" + loaded, err := testGroupedLoad(loadRequest[map[string]any]{ + main: loadInput{source: Bytes("main", []byte(main))}, + underlays: []loadGroup{{inputs: []loadInput{{source: Bytes("lower", []byte(ancestor))}}}}, + }) + if test.valid { + if err != nil || loaded == nil { + t.Fatalf("valid initialization failed: %v", err) + } + return + } + var loadErr *LoadError + if !errors.As(err, &loadErr) || loaded != nil { + t.Fatalf("invalid collection operation succeeded: %#v, %v", loaded, err) + } + diagnostics := loadErr.Diagnostics() + if len(diagnostics) != 1 || diagnostics[0].Code != CodeSequenceInvalid || diagnostics[0].Path.String() != "/group/items" { + t.Fatalf("operation diagnostics = %#v", diagnostics) + } + }) + } + } +} + +func TestAddedAncestorTransfersNestedRecordPolicies(t *testing.T) { + t.Parallel() + type record struct { + ID string `yaml:"id"` + Steps []map[string]any `yaml:"steps"` + } + type config struct { + Group struct { + Items []record `yaml:"items"` + } `yaml:"group"` + } + files := fstest.MapFS{ + "main.yml": {Data: []byte(`configkit: + sequences: [{destination: [group, items], key: id}] + imports: [{path: base.yml, destination: [group, items], placement: sequence-item}] +group: + items: [{id: child, extends: {id: base}, steps: [{name: child}]}] +`)}, + "base.yml": {Data: []byte("configkit:\n sequences: [{destination: [steps], key: name}]\nid: base\nsteps: [{name: base}]\n")}, + } + loaded, err := testGroupedLoad(loadRequest[config]{ + main: loadInput{source: NewFilesystem("catalog", files).File("main.yml"), imports: true}, + underlays: []loadGroup{{inputs: []loadInput{{source: Bytes("lower", []byte("{}"))}}}}, + }) + if err != nil { + t.Fatal(err) + } + for _, item := range loaded.Config.Group.Items { + if item.ID == "child" { + if len(item.Steps) != 2 || item.Steps[0]["name"] != "base" || item.Steps[1]["name"] != "child" { + t.Fatalf("record-local identity was lost when adding its ancestor: %#v", item.Steps) + } + return + } + } + t.Fatal("missing child record") +} + +func TestInheritanceChecksCollectionOperationUnderAddedAncestor(t *testing.T) { + t.Parallel() + _, err := testLoad[map[string]any](Bytes("inheritance", []byte(`configkit: + sequences: [{destination: [child, group, items], mode: append}] +base: {} +child: + extends: base + group: {items: [{id: a}]} +`))) + var loadErr *LoadError + if !errors.As(err, &loadErr) { + t.Fatalf("missing nested operation error: %v", err) + } + diagnostics := loadErr.Diagnostics() + if len(diagnostics) != 1 || diagnostics[0].Code != CodeSequenceInvalid || diagnostics[0].Path.String() != "/child/group/items" { + t.Fatalf("nested inheritance operation diagnostics = %#v", diagnostics) + } +} diff --git a/internal/engine/inheritance_schedule.go b/internal/engine/inheritance_schedule.go index 997e1a5..1f5d8c0 100644 --- a/internal/engine/inheritance_schedule.go +++ b/internal/engine/inheritance_schedule.go @@ -267,11 +267,7 @@ func applyInheritanceScopeTemplates(adapter inheritanceScopeAdapter, order []int if !active { return nil, false } - lowerPath, within := relativeHistoryPath(node.path, childPath) - if !within { - return nil, false - } - lowerValue, _ := documentAtPath(parent, lowerPath) + lowerValue := existing combined, problems := combineSequence(lowerValue, incoming, resolution, sequenceCombinationContext{destination: childPath}) if len(problems) != 0 { combinationDiagnostics = append(combinationDiagnostics, problems...) diff --git a/internal/engine/item_policy_test.go b/internal/engine/item_policy_test.go index bb9abb6..14b5302 100644 --- a/internal/engine/item_policy_test.go +++ b/internal/engine/item_policy_test.go @@ -298,7 +298,7 @@ func TestAncestorReplacementRequiresIdentityWhenRetainedModeReturns(t *testing.T collectionPolicies: scope, origins: make(map[string]Origin), }) } - reintroduceAncestor := func(t *testing.T, assembly *assemblyAccumulator) { + removeAncestor := func(t *testing.T, assembly *assemblyAccumulator) { t.Helper() if diagnostics := assembly.applyOverlayWithCollectionPolicy( parseItemPolicyDocument(t, "remove", "catalog: null\n"), nil, collectionPolicyScope{}, nil, @@ -306,23 +306,17 @@ func TestAncestorReplacementRequiresIdentityWhenRetainedModeReturns(t *testing.T ); len(diagnostics) != 0 { t.Fatalf("ancestor removal diagnostics = %#v", diagnostics) } - if diagnostics := assembly.applyOverlayWithCollectionPolicy( - parseItemPolicyDocument(t, "reintroduction", "catalog:\n prompts: [{id: restored, name: restored}]\n"), nil, - collectionPolicyScope{}, nil, LayerBase, sequencePolicy{}, nil, - ); len(diagnostics) != 0 { - t.Fatalf("ancestor reintroduction diagnostics = %#v", diagnostics) - } } t.Run("retained append mode requires a renewed identity", func(t *testing.T) { assembly := newAssembly(t) - reintroduceAncestor(t, assembly) + removeAncestor(t, assembly) policy, ok := assembly.current.collectionPolicies.at(path) if !ok || policy.identityPresent || !policy.explicitModePresent || policy.explicitMode != sequenceModeAppend { - t.Fatalf("reintroduced policy = %#v, %v", policy, ok) + t.Fatalf("retired collection policy = %#v, %v", policy, ok) } diagnostics := assembly.applyOverlayWithCollectionPolicy( - parseItemPolicyDocument(t, "next", "catalog:\n prompts: [{value: next}]\n"), nil, + parseItemPolicyDocument(t, "reintroduction", "catalog:\n prompts: [{id: restored, name: restored}]\n"), nil, collectionPolicyScope{}, nil, LayerBase, sequencePolicy{}, nil, ) if len(diagnostics) != 1 || diagnostics[0].Code != CodeSequenceInvalid { @@ -332,7 +326,7 @@ func TestAncestorReplacementRequiresIdentityWhenRetainedModeReturns(t *testing.T t.Run("a renewed key restores the retained append mode", func(t *testing.T) { assembly := newAssembly(t) - reintroduceAncestor(t, assembly) + removeAncestor(t, assembly) declaration := sequenceDeclarationSpec{ destination: path, identityFields: []string{"name"}, identityPresent: true, } diff --git a/internal/engine/recursive_merge.go b/internal/engine/recursive_merge.go index a6176b3..76e1460 100644 --- a/internal/engine/recursive_merge.go +++ b/internal/engine/recursive_merge.go @@ -22,21 +22,28 @@ func mergeRecursiveMappings(target, incoming *document.Node, current Path, index if hooks.merge != nil { if merged, handled := hooks.merge(path, nil, entry.Value); handled { cloned := entry - cloned.Value = document.Clone(merged) + cloned.Value = cloneMergeSubtree(merged, path, hooks.merge) index.append(target, cloned) continue } } - cloned := entry - cloned.Value = document.Clone(entry.Value) - index.append(target, cloned) if hooks.added != nil { hooks.added(path, entry.Value) } + cloned := entry + cloned.Value = cloneMergeSubtree(entry.Value, path, hooks.merge) + index.append(target, cloned) continue } existing := target.Mapping[position].Value + if hooks.merge != nil { + if merged, handled := hooks.merge(path, existing, entry.Value); handled { + target.Mapping[position] = entry + target.Mapping[position].Value = cloneMergeSubtree(merged, path, hooks.merge) + continue + } + } if existing.Kind == document.Mapping && entry.Value.Kind == document.Mapping { if hooks.combined != nil { hooks.combined(path, entry.Value) @@ -44,18 +51,33 @@ func mergeRecursiveMappings(target, incoming *document.Node, current Path, index mergeRecursiveMappings(existing, entry.Value, path, index, hooks) continue } - if hooks.merge != nil { - if merged, handled := hooks.merge(path, existing, entry.Value); handled { - target.Mapping[position] = entry - target.Mapping[position].Value = document.Clone(merged) - continue - } - } if hooks.replaced != nil { hooks.replaced(path, existing, entry.Value) } target.Mapping[position] = entry - target.Mapping[position].Value = document.Clone(entry.Value) + target.Mapping[position].Value = cloneMergeSubtree(entry.Value, path, hooks.merge) } } + +// A copied mapping can contain collection operations even though its ancestor +// has no retained mapping. Visit those operations with absent lower values. +// Ordinary origin/replacement hooks already accounted for the whole subtree; +// do not replay them for every descendant. Records inside sequences remain whole. +func cloneMergeSubtree(incoming *document.Node, path Path, merge func(Path, *document.Node, *document.Node) (*document.Node, bool)) *document.Node { + if incoming == nil || incoming.Kind != document.Mapping || merge == nil { + return document.Clone(incoming) + } + cloned := *incoming + cloned.Mapping = make([]document.Entry, len(incoming.Mapping)) + for index, entry := range incoming.Mapping { + childPath := path.Key(entry.Key) + value := entry.Value + if merged, handled := merge(childPath, nil, value); handled { + value = merged + } + entry.Value = cloneMergeSubtree(value, childPath, merge) + cloned.Mapping[index] = entry + } + return &cloned +}