From c223fd3adcffd2bda1cba129e2c0462f5c819638 Mon Sep 17 00:00:00 2001 From: James M Leddy Date: Tue, 23 Sep 2025 14:43:35 -0400 Subject: [PATCH] feat: Allow deeper merge of lists The initial implementation doesn't consider the type of the index. Instead, we should stipulate that the index is a scalar. Specifically a float or a string, since bool doesn't really make sense for this. Signed-off-by: James M Leddy --- pkg/chart/v2/loader/load.go | 48 ++++++--- pkg/chart/v2/loader/load_test.go | 162 ++++++++++++++++++++++++++++++- 2 files changed, 195 insertions(+), 15 deletions(-) diff --git a/pkg/chart/v2/loader/load.go b/pkg/chart/v2/loader/load.go index 7e80c35d9..6450dcecb 100644 --- a/pkg/chart/v2/loader/load.go +++ b/pkg/chart/v2/loader/load.go @@ -270,11 +270,11 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { if sourceList, ok := out[strippedKey].([]map[string]interface{}); ok { if val, ok := v.([]map[string]interface{}); ok { - out[strippedKey] = MergeMapLists(sourceList, val) + out[strippedKey] = mergeMapLists(sourceList, val) continue } - log.Printf("Property \"%s\" mismatch during merge", strippedKey) + log.Printf("Warning: Property \"%s\" mismatch during merge", strippedKey) continue } else if sourceList, ok := out[strippedKey].([]float64); ok { if val, ok := v.([]float64); ok { @@ -282,7 +282,7 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { continue } - log.Printf("Property \"%s\" mismatch during merge", strippedKey) + log.Printf("Warning: Property \"%s\" mismatch during merge", strippedKey) continue } else if sourceList, ok := out[strippedKey].([]bool); ok { if val, ok := v.([]bool); ok { @@ -290,7 +290,7 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { continue } - log.Printf("Property \"%s\" mismatch during merge", strippedKey) + log.Printf("Warning: Property \"%s\" mismatch during merge", strippedKey) continue } else if sourceList, ok := out[strippedKey].([]string); ok { if val, ok := v.([]string); ok { @@ -298,7 +298,7 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { continue } - log.Printf("Property \"%s\" mismatch during merge", strippedKey) + log.Printf("Warning: Property \"%s\" mismatch during merge", strippedKey) continue } else { out[strippedKey] = v @@ -309,27 +309,47 @@ func MergeMaps(a, b map[string]interface{}) map[string]interface{} { return out } -// MergeMapLists merges two lists of maps. If a prefix of * is set on a map key, +// mergeMapLists merges two lists of maps. If a prefix of * is set on a map key, // that key will be used to de-duplicate/merge with the source map -func MergeMapLists(a, b []map[string]interface{}) []map[string]interface{} { - out := a +func mergeMapLists(a, b []map[string]interface{}) []map[string]interface{} { + out := make([]map[string]interface{}, len(a)) + copy(out, a) +OuterList: for j, mapEntry := range b { var uniqueKey string var dedupValue interface{} + alreadyHasPrefix := false for k, v := range mapEntry { + switch v.(type) { + case float64: + break + case string: + break + default: + continue + } if strings.HasPrefix(k, mergePrefix) { - uniqueKey = k + if alreadyHasPrefix { + duplicateKey := "\\*" + uniqueKey + log.Printf("Warning: Can't index on multiple keys \"%s: %v\" and \"%s: %v\" during list merge", duplicateKey, dedupValue, k, v) + // undo prior step + b[j][duplicateKey] = dedupValue + delete(b[j], uniqueKey) + out = append(out, mapEntry) + continue OuterList + } + + alreadyHasPrefix = true + uniqueKey = strings.TrimPrefix(k, mergePrefix) dedupValue = v - b[j][strings.TrimPrefix(uniqueKey, mergePrefix)] = v - delete(b[j], uniqueKey) + b[j][uniqueKey] = v + delete(b[j], k) } } if len(uniqueKey) > 0 { - strippedMergeKey := strings.TrimPrefix(uniqueKey, mergePrefix) - for i, sourceMapEntry := range out { for k, v := range sourceMapEntry { - if (k == strippedMergeKey || k == uniqueKey) && v == dedupValue { + if k == uniqueKey && v == dedupValue { mergedMapEntry := MergeMaps(sourceMapEntry, mapEntry) out[i] = mergedMapEntry break diff --git a/pkg/chart/v2/loader/load_test.go b/pkg/chart/v2/loader/load_test.go index 3ce9347fb..864fadfdc 100644 --- a/pkg/chart/v2/loader/load_test.go +++ b/pkg/chart/v2/loader/load_test.go @@ -871,6 +871,167 @@ func TestMergeLists(t *testing.T) { } +func TestMergeListsDeep(t *testing.T) { + indexedCommonMap := map[string]interface{}{ + "groups": []map[string]interface{}{ + { + "name": "commonRules", + "id": float64(1331), + "rules": []map[string]interface{}{ + { + "name": "firstRule", + "expr": "commonFirstExpr", + "for": "3m", + }, + }, + }, + }, + } + indexedOverrideMap := map[string]interface{}{ + "\\*groups": []map[string]interface{}{ + { + "\\*name": "commonRules", + "\\*rules": []map[string]interface{}{ + { + "\\*name": "firstRule", + "for": "4m", + }, + { + "name": "uncommonRule", + "expr": "secondExpr", + }, + }, + }, + { + "name": "appRules", + "rules": []map[string]interface{}{ + { + "name": "appRule", + "expr": "appExpr", + "for": "4m", + }, + }, + }, + }, + } + indexedMergedMap := map[string]interface{}{ + "groups": []map[string]interface{}{ + { + "name": "commonRules", + "id": float64(1331), + "rules": []map[string]interface{}{ + { + "name": "firstRule", + "expr": "commonFirstExpr", + "for": "4m", + }, + { + "name": "uncommonRule", + "expr": "secondExpr", + }, + }, + }, + { + "name": "appRules", + "rules": []map[string]interface{}{ + { + "name": "appRule", + "expr": "appExpr", + "for": "4m", + }, + }, + }, + }, + } + testMap := MergeMaps(indexedCommonMap, indexedOverrideMap) + equal := reflect.DeepEqual(testMap, indexedMergedMap) + if !equal { + t.Errorf("Expected two level deep merged list. Expected values: %v, got %v", indexedMergedMap, testMap) + } + + indexedOverrideMap = map[string]interface{}{ + "\\*groups": []map[string]interface{}{ + { + "\\*id": float64(1331), + "\\*rules": []map[string]interface{}{ + { + "\\*name": "firstRule", + "for": "4m", + }, + { + "name": "uncommonRule", + "expr": "secondExpr", + }, + }, + }, + }, + } + indexedMergedMap = map[string]interface{}{ + "groups": []map[string]interface{}{ + { + "name": "commonRules", + "id": float64(1331), + "rules": []map[string]interface{}{ + { + "name": "firstRule", + "expr": "commonFirstExpr", + "for": "4m", + }, + { + "name": "uncommonRule", + "expr": "secondExpr", + }, + }, + }, + }, + } + testMap = MergeMaps(indexedCommonMap, indexedOverrideMap) + equal = reflect.DeepEqual(testMap, indexedMergedMap) + if !equal { + t.Errorf("Expected deep merged list on float64 key. Expected values: %v, got %v", indexedMergedMap, testMap) + } + + indexedOverrideMap = map[string]interface{}{ + "\\*groups": []map[string]interface{}{ + { + "\\*name": "commonRules", + "\\*rules": []map[string]interface{}{ + { + "\\*name": "firstRule", + "\\*for": "3m", + "expr": "OverrideFirstExpr", + }, + }, + }, + }, + } + indexedMergedMap = map[string]interface{}{ + "groups": []map[string]interface{}{ + { + "name": "commonRules", + "id": float64(1331), + "rules": []map[string]interface{}{ + { + "name": "firstRule", + "expr": "commonFirstExpr", + "for": "3m", + }, + { + "\\*name": "firstRule", + "\\*for": "3m", + "expr": "OverrideFirstExpr", + }, + }, + }, + }, + } + testMap = MergeMaps(indexedCommonMap, indexedOverrideMap) + equal = reflect.DeepEqual(testMap, indexedMergedMap) + if !equal { + t.Errorf("Expected composed list where we used two erroneously indicies. Expected values: %v, got %v", indexedMergedMap, testMap) + } +} + func verifyChart(t *testing.T, c *chart.Chart) { t.Helper() if c.Name() == "" { @@ -917,7 +1078,6 @@ func verifyChart(t *testing.T, c *chart.Chart) { t.Errorf("Expected %s version %s, got %s", dep.Name(), exp["version"], dep.Metadata.Version) } } - } func verifyDependencies(t *testing.T, c *chart.Chart) {