From 98c9c3f00d6e3112a40efea814cd30fafe431422 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 12 Oct 2026 00:14:20 +0000 Subject: [PATCH] Index mappings while assembling configuration reports --- docs/roadmap/remediation.md | 15 +++++++-- integrations/ckconfig/diff_report.go | 40 +++++++++++++++++------ integrations/ckconfig/diff_report_test.go | 11 ++++--- 3 files changed, 49 insertions(+), 17 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 81b67ea..00238fd 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,7 +1,7 @@ # Configuration comparison review remediation -Status: four open findings. This document records required follow-up work; it -does not implement the fixes. +Status: one finding complete; three open. This document tracks required +follow-up work and its completion evidence. ## Review baseline and scope @@ -39,7 +39,7 @@ caller-owned aliases/caches are deliberate choices and require no remediation. ## R-1: Avoid repeated full scans when assembling wide report mappings -**Status:** Open. **Priority:** Medium. **Category:** Code quality / efficiency. +**Status:** Complete. **Priority:** Medium. **Category:** Code quality / efficiency. **Evidence:** [diff_report.go](../../integrations/ckconfig/diff_report.go), `placeYAMLValue` and `mappingKeyIndex` (lines 59 and 98 at the review baseline). @@ -62,6 +62,15 @@ the accumulated mapping. Existing semantic-path, YAML-type, ordering, and native safe-value behavior remains intact. Any additional measurement should be focused on this lookup cost; a numerical performance gate is not required. +**Completed 2026-10-12:** A private, report-local `yamlValuePlacer` now indexes +each encountered mapping once and updates that index as keys are appended. +Subsequent placements reuse key positions while YAML content keeps insertion +order. Root values, native regions, and incompatible-ancestor failures retain +their handling. Existing helper tests use the same placer across placements. +`go test ./...` passed in `integrations/ckconfig`, including the simple consumer; +the diff and whitespace review passed. No public API, limits, global cache, +benchmark, or performance gate was added. + ## R-2: Scope CLI guarantees to the actual provider and delivery boundaries **Status:** Open. **Priority:** Medium. **Category:** Inadvertently strengthened diff --git a/integrations/ckconfig/diff_report.go b/integrations/ckconfig/diff_report.go index 506546b..48190e0 100644 --- a/integrations/ckconfig/diff_report.go +++ b/integrations/ckconfig/diff_report.go @@ -56,7 +56,13 @@ func decodeJSONPointer(pointer string) ([]string, error) { return parts, nil } -func placeYAMLValue(category *yaml.Node, path []string, value *yaml.Node) (*yaml.Node, error) { +// yamlValuePlacer indexes each mapping once while assembling one report. +// Content remains the ordered representation used by the YAML encoder. +type yamlValuePlacer struct { + keys map[*yaml.Node]map[string]int +} + +func (placer *yamlValuePlacer) placeValue(category *yaml.Node, path []string, value *yaml.Node) (*yaml.Node, error) { if value == nil { return nil, fmt.Errorf("place YAML value: value node is nil") } @@ -69,18 +75,21 @@ func placeYAMLValue(category *yaml.Node, path []string, value *yaml.Node) (*yaml current := category for index, token := range path { - keyIndex := mappingKeyIndex(current, token) + keys := placer.mappingKeys(current) + keyIndex, exists := keys[token] if index == len(path)-1 { - if keyIndex >= 0 { + if exists { current.Content[keyIndex+1] = value } else { + keys[token] = len(current.Content) current.Content = append(current.Content, stringKey(token), value) } return category, nil } - if keyIndex < 0 { + if !exists { child := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} + keys[token] = len(current.Content) current.Content = append(current.Content, stringKey(token), child) current = child continue @@ -95,14 +104,24 @@ func placeYAMLValue(category *yaml.Node, path []string, value *yaml.Node) (*yaml return category, nil } -func mappingKeyIndex(mapping *yaml.Node, key string) int { +func (placer *yamlValuePlacer) mappingKeys(mapping *yaml.Node) map[string]int { + if keys, exists := placer.keys[mapping]; exists { + return keys + } + if placer.keys == nil { + placer.keys = make(map[*yaml.Node]map[string]int) + } + keys := make(map[string]int, len(mapping.Content)/2) for index := 0; index+1 < len(mapping.Content); index += 2 { candidate := mapping.Content[index] - if candidate.Kind == yaml.ScalarNode && candidate.Tag == "!!str" && candidate.Value == key { - return index + if candidate.Kind == yaml.ScalarNode && candidate.Tag == "!!str" { + if _, exists := keys[candidate.Value]; !exists { + keys[candidate.Value] = index + } } } - return -1 + placer.keys[mapping] = keys + return keys } func stringKey(value string) *yaml.Node { @@ -122,6 +141,7 @@ func pointerFromTokens(tokens []string) string { } func renderDiffReport(differences []configkit.Difference) ([]byte, error) { + var placer yamlValuePlacer additions := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} changes := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} deletions := &yaml.Node{Kind: yaml.SequenceNode, Tag: "!!seq"} @@ -138,7 +158,7 @@ func renderDiffReport(differences []configkit.Difference) ([]byte, error) { if err != nil { return nil, fmt.Errorf("render addition at %q: %w", path, err) } - additions, err = placeYAMLValue(additions, tokens, value) + additions, err = placer.placeValue(additions, tokens, value) if err != nil { return nil, fmt.Errorf("render addition at %q: %w", path, err) } @@ -151,7 +171,7 @@ func renderDiffReport(differences []configkit.Difference) ([]byte, error) { if err != nil { return nil, fmt.Errorf("render change at %q: %w", path, err) } - changes, err = placeYAMLValue(changes, tokens, value) + changes, err = placer.placeValue(changes, tokens, value) if err != nil { return nil, fmt.Errorf("render change at %q: %w", path, err) } diff --git a/integrations/ckconfig/diff_report_test.go b/integrations/ckconfig/diff_report_test.go index cf7f1bd..605c889 100644 --- a/integrations/ckconfig/diff_report_test.go +++ b/integrations/ckconfig/diff_report_test.go @@ -74,6 +74,7 @@ func TestDecodeJSONPointerPreservesSemanticTokens(t *testing.T) { } func TestPlaceYAMLValueBuildsOrderedSemanticMappings(t *testing.T) { + var placer yamlValuePlacer category := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} paths := [][]string{ {"parent", "0", "dot.name", "50%"}, @@ -83,7 +84,7 @@ func TestPlaceYAMLValueBuildsOrderedSemanticMappings(t *testing.T) { } for index, path := range paths { value := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: string(rune('a' + index))} - updated, err := placeYAMLValue(category, path, value) + updated, err := placer.placeValue(category, path, value) if err != nil { t.Fatalf("place %q: %v", pointerFromTokens(path), err) } @@ -108,16 +109,17 @@ func TestPlaceYAMLValueBuildsOrderedSemanticMappings(t *testing.T) { } func TestPlaceYAMLValueHandlesRootAndEmptyKey(t *testing.T) { + var placer yamlValuePlacer category := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} root := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!null", Value: "null"} - updated, err := placeYAMLValue(category, nil, root) + updated, err := placer.placeValue(category, nil, root) if err != nil || updated != root { t.Fatalf("root placement = %#v, %v", updated, err) } category = &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} value := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: "empty-key"} - updated, err = placeYAMLValue(category, []string{""}, value) + updated, err = placer.placeValue(category, []string{""}, value) if err != nil { t.Fatal(err) } @@ -127,10 +129,11 @@ func TestPlaceYAMLValueHandlesRootAndEmptyKey(t *testing.T) { } func TestPlaceYAMLValueRejectsIncompatibleAncestors(t *testing.T) { + var placer yamlValuePlacer category := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map", Content: []*yaml.Node{ stringKey("parent"), {Kind: yaml.ScalarNode, Tag: "!!str", Value: "leaf"}, }} - _, err := placeYAMLValue(category, []string{"parent", "child"}, &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!null", Value: "null"}) + _, err := placer.placeValue(category, []string{"parent", "child"}, &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!null", Value: "null"}) if err == nil || !strings.Contains(err.Error(), "/parent/child") || !strings.Contains(err.Error(), "/parent") { t.Fatalf("incompatible ancestor error = %v", err) }