From b09950663210ddd62af54b6e92cf85b1d039c2a9 Mon Sep 17 00:00:00 2001 From: Siew Kam Onn Date: Fri, 10 Oct 2025 18:41:52 +0800 Subject: [PATCH] Refactor coalesce functions to handle empty maps Reordered coalesceValues and coalesceTablesFullKey to convert explicit empty overrides into empty maps before merging. This prevents default map entries from resurfacing, even for nested tables. Additionally, the regression suite is expanded to cover nested empty overrides, ensuring empty maps are preserved across different override shapes. Signed-off-by: Siew Kam Onn --- pkg/chart/common/util/coalesce.go | 74 +++++++++++++++++++++----- pkg/chart/common/util/coalesce_test.go | 58 ++++++++++++++++++++ pkg/strvals/parser.go | 13 ++++- pkg/strvals/parser_test.go | 5 ++ 4 files changed, 135 insertions(+), 15 deletions(-) diff --git a/pkg/chart/common/util/coalesce.go b/pkg/chart/common/util/coalesce.go index 7326ee685..e5f7664c6 100644 --- a/pkg/chart/common/util/coalesce.go +++ b/pkg/chart/common/util/coalesce.go @@ -239,7 +239,23 @@ func coalesceValues(printf printFn, c chart.Charter, v map[string]interface{}, p // This allows Helm's various sources of values (value files or --set) to // remove incompatible keys from any previous chart, file, or set values. delete(v, key) - } else if dest, ok := value.(map[string]interface{}); ok { + continue + } + + if isExplicitEmptyOverride(value) { + // Interpret empty slices from values sources (for example, --set key={}) + // as an explicit request for an empty map so chart defaults are removed. + dest := map[string]interface{}{} + v[key] = dest + + if src, ok := val.(map[string]interface{}); ok { + merge := childChartMergeTrue(c, key, merge) + coalesceTablesFullKey(printf, dest, src, concatPrefix(subPrefix, key), merge) + } + continue + } + + if dest, ok := value.(map[string]interface{}); ok { // if v[key] is a table, merge nv's val table into v[key]. src, ok := val.(map[string]interface{}) if !ok { @@ -248,14 +264,16 @@ func coalesceValues(printf printFn, c chart.Charter, v map[string]interface{}, p if val != nil { printf("warning: skipped value for %s.%s: Not a table.", subPrefix, key) } - } else { - // If the key is a child chart, coalesce tables with Merge set to true - merge := childChartMergeTrue(c, key, merge) - - // Because v has higher precedence than nv, dest values override src - // values. - coalesceTablesFullKey(printf, dest, src, concatPrefix(subPrefix, key), merge) + continue } + + // If the key is a child chart, coalesce tables with Merge set to true + merge := childChartMergeTrue(c, key, merge) + + // Because v has higher precedence than nv, dest values override src + // values. + coalesceTablesFullKey(printf, dest, src, concatPrefix(subPrefix, key), merge) + continue } } else { // If the key is not in v, copy it from nv. @@ -320,17 +338,36 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref // values. for key, val := range src { fullkey := concatPrefix(prefix, key) - if dv, ok := dst[key]; ok && !merge && dv == nil { - delete(dst, key) - } else if !ok { + dv, ok := dst[key] + if !ok { dst[key] = val - } else if istable(val) { + continue + } + + if !merge && dv == nil { + delete(dst, key) + continue + } + + if isExplicitEmptyOverride(dv) { + empty := map[string]interface{}{} + dst[key] = empty + if srcMap, ok := val.(map[string]interface{}); ok { + coalesceTablesFullKey(printf, empty, srcMap, fullkey, merge) + } + continue + } + + if istable(val) { if istable(dv) { coalesceTablesFullKey(printf, dv.(map[string]interface{}), val.(map[string]interface{}), fullkey, merge) } else { printf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val) } - } else if istable(dv) && val != nil { + continue + } + + if istable(dv) && val != nil { printf("warning: destination for %s is a table. Ignoring non-table value (%v)", fullkey, val) } } @@ -342,3 +379,14 @@ func istable(v interface{}) bool { _, ok := v.(map[string]interface{}) return ok } + +func isExplicitEmptyOverride(v interface{}) bool { + switch vv := v.(type) { + case []interface{}: + return len(vv) == 0 || (len(vv) == 1 && vv[0] == "") + case []string: + return len(vv) == 0 || (len(vv) == 1 && vv[0] == "") + default: + return false + } +} diff --git a/pkg/chart/common/util/coalesce_test.go b/pkg/chart/common/util/coalesce_test.go index f127b5f0f..3d1b110bc 100644 --- a/pkg/chart/common/util/coalesce_test.go +++ b/pkg/chart/common/util/coalesce_test.go @@ -267,6 +267,64 @@ func TestCoalesceValuesEmptyMapOverride(t *testing.T) { assert.NotContains(t, result, "toDelete", "expected toDelete key to be removed when set to nil override") } +func TestCoalesceValuesEmptySliceOverrideForMap(t *testing.T) { + newChart := func() *chart.Chart { + return &chart.Chart{ + Metadata: &chart.Metadata{Name: "emptymap"}, + Values: map[string]interface{}{ + "config": map[string]interface{}{ + "foo": "bar", + "nested": map[string]interface{}{"baz": "qux"}, + }, + }, + } + } + + t.Run("empty interface slice", func(t *testing.T) { + overrides := map[string]interface{}{ + "config": []interface{}{}, + } + + result, err := CoalesceValues(newChart(), overrides) + require.NoError(t, err) + + config, ok := result["config"].(map[string]interface{}) + require.Truef(t, ok, "expected config to remain a map, got %T", result["config"]) + assert.Empty(t, config, "expected config map to be empty") + }) + + t.Run("legacy single empty element slice", func(t *testing.T) { + overrides := map[string]interface{}{ + "config": []interface{}{""}, + } + + result, err := CoalesceValues(newChart(), overrides) + require.NoError(t, err) + + config, ok := result["config"].(map[string]interface{}) + require.Truef(t, ok, "expected config to remain a map, got %T", result["config"]) + assert.Empty(t, config, "expected config map to be empty") + }) + + t.Run("nested empty slice", func(t *testing.T) { + overrides := map[string]interface{}{ + "config": map[string]interface{}{ + "nested": []interface{}{}, + }, + } + + result, err := CoalesceValues(newChart(), overrides) + require.NoError(t, err) + + config, ok := result["config"].(map[string]interface{}) + require.Truef(t, ok, "expected config to remain a map, got %T", result["config"]) + + nested, ok := config["nested"].(map[string]interface{}) + require.Truef(t, ok, "expected nested override to remain a map, got %T", config["nested"]) + assert.Empty(t, nested, "expected nested map to be empty") + }) +} + func ttpl(tpl string, v map[string]interface{}) (string, error) { var b bytes.Buffer tt := template.Must(template.New("t").Parse(tpl)) diff --git a/pkg/strvals/parser.go b/pkg/strvals/parser.go index c65e98c84..a889ab9b4 100644 --- a/pkg/strvals/parser.go +++ b/pkg/strvals/parser.go @@ -485,8 +485,17 @@ func (t *parser) valList() ([]interface{}, error) { return list, err case last == '}': // If this is followed by ',', consume it. - if r, _, e := t.sc.ReadRune(); e == nil && r != ',' { - t.sc.UnreadRune() + r, _, e := t.sc.ReadRune() + if e == nil { + if r != ',' { + t.sc.UnreadRune() + } + } else if e != io.EOF { + return list, e + } + if len(rs) == 0 && len(list) == 0 { + // Interpret "{}" as an explicit empty map, not an empty list. + return []interface{}{map[string]interface{}{}}, nil } v, e := t.reader(rs) list = append(list, v) diff --git a/pkg/strvals/parser_test.go b/pkg/strvals/parser_test.go index 73403fc52..9e83bb4ac 100644 --- a/pkg/strvals/parser_test.go +++ b/pkg/strvals/parser_test.go @@ -265,6 +265,11 @@ func TestParseSet(t *testing.T) { map[string]interface{}{"name1": []string{"value1", "value2"}}, false, }, + { + "emptylist={}", + map[string]interface{}{"emptylist": []interface{}{}}, + false, + }, { "name1={value1,value2},name2={value1,value2}", map[string]interface{}{