Index mappings while assembling configuration reports
This commit is contained in:
@@ -1,7 +1,7 @@
|
|||||||
# Configuration comparison review remediation
|
# Configuration comparison review remediation
|
||||||
|
|
||||||
Status: four open findings. This document records required follow-up work; it
|
Status: one finding complete; three open. This document tracks required
|
||||||
does not implement the fixes.
|
follow-up work and its completion evidence.
|
||||||
|
|
||||||
## Review baseline and scope
|
## 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
|
## 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),
|
**Evidence:** [diff_report.go](../../integrations/ckconfig/diff_report.go),
|
||||||
`placeYAMLValue` and `mappingKeyIndex` (lines 59 and 98 at the review baseline).
|
`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
|
safe-value behavior remains intact. Any additional measurement should be focused
|
||||||
on this lookup cost; a numerical performance gate is not required.
|
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
|
## R-2: Scope CLI guarantees to the actual provider and delivery boundaries
|
||||||
|
|
||||||
**Status:** Open. **Priority:** Medium. **Category:** Inadvertently strengthened
|
**Status:** Open. **Priority:** Medium. **Category:** Inadvertently strengthened
|
||||||
|
|||||||
@@ -56,7 +56,13 @@ func decodeJSONPointer(pointer string) ([]string, error) {
|
|||||||
return parts, nil
|
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 {
|
if value == nil {
|
||||||
return nil, fmt.Errorf("place YAML value: value node is 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
|
current := category
|
||||||
for index, token := range path {
|
for index, token := range path {
|
||||||
keyIndex := mappingKeyIndex(current, token)
|
keys := placer.mappingKeys(current)
|
||||||
|
keyIndex, exists := keys[token]
|
||||||
if index == len(path)-1 {
|
if index == len(path)-1 {
|
||||||
if keyIndex >= 0 {
|
if exists {
|
||||||
current.Content[keyIndex+1] = value
|
current.Content[keyIndex+1] = value
|
||||||
} else {
|
} else {
|
||||||
|
keys[token] = len(current.Content)
|
||||||
current.Content = append(current.Content, stringKey(token), value)
|
current.Content = append(current.Content, stringKey(token), value)
|
||||||
}
|
}
|
||||||
return category, nil
|
return category, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
if keyIndex < 0 {
|
if !exists {
|
||||||
child := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"}
|
child := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"}
|
||||||
|
keys[token] = len(current.Content)
|
||||||
current.Content = append(current.Content, stringKey(token), child)
|
current.Content = append(current.Content, stringKey(token), child)
|
||||||
current = child
|
current = child
|
||||||
continue
|
continue
|
||||||
@@ -95,14 +104,24 @@ func placeYAMLValue(category *yaml.Node, path []string, value *yaml.Node) (*yaml
|
|||||||
return category, nil
|
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 {
|
for index := 0; index+1 < len(mapping.Content); index += 2 {
|
||||||
candidate := mapping.Content[index]
|
candidate := mapping.Content[index]
|
||||||
if candidate.Kind == yaml.ScalarNode && candidate.Tag == "!!str" && candidate.Value == key {
|
if candidate.Kind == yaml.ScalarNode && candidate.Tag == "!!str" {
|
||||||
return index
|
if _, exists := keys[candidate.Value]; !exists {
|
||||||
|
keys[candidate.Value] = index
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return -1
|
placer.keys[mapping] = keys
|
||||||
|
return keys
|
||||||
}
|
}
|
||||||
|
|
||||||
func stringKey(value string) *yaml.Node {
|
func stringKey(value string) *yaml.Node {
|
||||||
@@ -122,6 +141,7 @@ func pointerFromTokens(tokens []string) string {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func renderDiffReport(differences []configkit.Difference) ([]byte, error) {
|
func renderDiffReport(differences []configkit.Difference) ([]byte, error) {
|
||||||
|
var placer yamlValuePlacer
|
||||||
additions := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"}
|
additions := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"}
|
||||||
changes := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"}
|
changes := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"}
|
||||||
deletions := &yaml.Node{Kind: yaml.SequenceNode, Tag: "!!seq"}
|
deletions := &yaml.Node{Kind: yaml.SequenceNode, Tag: "!!seq"}
|
||||||
@@ -138,7 +158,7 @@ func renderDiffReport(differences []configkit.Difference) ([]byte, error) {
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("render addition at %q: %w", path, err)
|
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 {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("render addition at %q: %w", path, err)
|
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 {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("render change at %q: %w", path, err)
|
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 {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("render change at %q: %w", path, err)
|
return nil, fmt.Errorf("render change at %q: %w", path, err)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -74,6 +74,7 @@ func TestDecodeJSONPointerPreservesSemanticTokens(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func TestPlaceYAMLValueBuildsOrderedSemanticMappings(t *testing.T) {
|
func TestPlaceYAMLValueBuildsOrderedSemanticMappings(t *testing.T) {
|
||||||
|
var placer yamlValuePlacer
|
||||||
category := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"}
|
category := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"}
|
||||||
paths := [][]string{
|
paths := [][]string{
|
||||||
{"parent", "0", "dot.name", "50%"},
|
{"parent", "0", "dot.name", "50%"},
|
||||||
@@ -83,7 +84,7 @@ func TestPlaceYAMLValueBuildsOrderedSemanticMappings(t *testing.T) {
|
|||||||
}
|
}
|
||||||
for index, path := range paths {
|
for index, path := range paths {
|
||||||
value := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: string(rune('a' + index))}
|
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 {
|
if err != nil {
|
||||||
t.Fatalf("place %q: %v", pointerFromTokens(path), err)
|
t.Fatalf("place %q: %v", pointerFromTokens(path), err)
|
||||||
}
|
}
|
||||||
@@ -108,16 +109,17 @@ func TestPlaceYAMLValueBuildsOrderedSemanticMappings(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func TestPlaceYAMLValueHandlesRootAndEmptyKey(t *testing.T) {
|
func TestPlaceYAMLValueHandlesRootAndEmptyKey(t *testing.T) {
|
||||||
|
var placer yamlValuePlacer
|
||||||
category := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"}
|
category := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"}
|
||||||
root := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!null", Value: "null"}
|
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 {
|
if err != nil || updated != root {
|
||||||
t.Fatalf("root placement = %#v, %v", updated, err)
|
t.Fatalf("root placement = %#v, %v", updated, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
category = &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"}
|
category = &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"}
|
||||||
value := &yaml.Node{Kind: yaml.ScalarNode, Tag: "!!str", Value: "empty-key"}
|
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 {
|
if err != nil {
|
||||||
t.Fatal(err)
|
t.Fatal(err)
|
||||||
}
|
}
|
||||||
@@ -127,10 +129,11 @@ func TestPlaceYAMLValueHandlesRootAndEmptyKey(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func TestPlaceYAMLValueRejectsIncompatibleAncestors(t *testing.T) {
|
func TestPlaceYAMLValueRejectsIncompatibleAncestors(t *testing.T) {
|
||||||
|
var placer yamlValuePlacer
|
||||||
category := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map", Content: []*yaml.Node{
|
category := &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map", Content: []*yaml.Node{
|
||||||
stringKey("parent"), {Kind: yaml.ScalarNode, Tag: "!!str", Value: "leaf"},
|
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") {
|
if err == nil || !strings.Contains(err.Error(), "/parent/child") || !strings.Contains(err.Error(), "/parent") {
|
||||||
t.Fatalf("incompatible ancestor error = %v", err)
|
t.Fatalf("incompatible ancestor error = %v", err)
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user