From 292fe702193e8ba9ce4c8ffffdd90cdfa761501c Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Fri, 12 Dec 2025 16:32:32 +0000 Subject: [PATCH 1/3] fix(values): preserve nil values when chart default is empty map Only delete nil user values when overriding a non-nil chart default. When chart has empty map or no default for a key, preserve user's nil. | Scenario | Result | |----------|--------| | User sets `baz: ~`, chart has `baz: "value"` | Key deleted | | User sets `baz: ~`, chart has empty map `{}` | Nil preserved | | User sets `baz: ~`, chart has `baz: ~` | Nil preserved | Fixes #31643 Signed-off-by: Evans Mungai --- pkg/chart/common/util/coalesce.go | 18 +- pkg/chart/common/util/coalesce_test.go | 255 +++++++++++++------------ 2 files changed, 145 insertions(+), 128 deletions(-) diff --git a/pkg/chart/common/util/coalesce.go b/pkg/chart/common/util/coalesce.go index 07794a04a..3220a275c 100644 --- a/pkg/chart/common/util/coalesce.go +++ b/pkg/chart/common/util/coalesce.go @@ -302,23 +302,23 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref if dst == nil { return src } - for key, val := range dst { - if val == nil { - src[key] = nil - } - } // Because dest has higher precedence than src, dest values override src // 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 dv == nil && !merge && val != nil { + // When coalescing (not merging), if dst has nil and src has a non-nil + // value, the user is nullifying a chart default - remove the key. + // Per Helm docs: setting a key to null deletes it. + // But if src also has nil (or key not in src), preserve the nil (issue #31643). + delete(dst, key) } else if istable(val) { if istable(dv) { coalesceTablesFullKey(printf, dv.(map[string]interface{}), val.(map[string]interface{}), fullkey, merge) - } else { + } else if dv != nil { printf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val) } } else if istable(dv) && val != nil { diff --git a/pkg/chart/common/util/coalesce_test.go b/pkg/chart/common/util/coalesce_test.go index 871bfa8da..598f1f9ef 100644 --- a/pkg/chart/common/util/coalesce_test.go +++ b/pkg/chart/common/util/coalesce_test.go @@ -25,6 +25,7 @@ import ( "text/template" "github.com/stretchr/testify/assert" + req "github.com/stretchr/testify/require" "helm.sh/helm/v4/pkg/chart/common" chart "helm.sh/helm/v4/pkg/chart/v2" @@ -401,131 +402,113 @@ func TestMergeValues(t *testing.T) { } func TestCoalesceTables(t *testing.T) { - dst := map[string]interface{}{ - "name": "Ishmael", - "address": map[string]interface{}{ - "street": "123 Spouter Inn Ct.", - "city": "Nantucket", - "country": nil, - }, - "details": map[string]interface{}{ - "friends": []string{"Tashtego"}, - }, - "boat": "pequod", - "hole": nil, - } - src := map[string]interface{}{ - "occupation": "whaler", - "address": map[string]interface{}{ - "state": "MA", - "street": "234 Spouter Inn Ct.", - "country": "US", - }, - "details": "empty", - "boat": map[string]interface{}{ - "mast": true, - }, - "hole": "black", - } - - // What we expect is that anything in dst overrides anything in src, but that - // otherwise the values are coalesced. - CoalesceTables(dst, src) - - if dst["name"] != "Ishmael" { - t.Errorf("Unexpected name: %s", dst["name"]) - } - if dst["occupation"] != "whaler" { - t.Errorf("Unexpected occupation: %s", dst["occupation"]) - } - - addr, ok := dst["address"].(map[string]interface{}) - if !ok { - t.Fatal("Address went away.") - } - - if addr["street"].(string) != "123 Spouter Inn Ct." { - t.Errorf("Unexpected address: %v", addr["street"]) - } - - if addr["city"].(string) != "Nantucket" { - t.Errorf("Unexpected city: %v", addr["city"]) - } - - if addr["state"].(string) != "MA" { - t.Errorf("Unexpected state: %v", addr["state"]) - } - - if _, ok = addr["country"]; ok { - t.Error("The country is not left out.") - } - - if det, ok := dst["details"].(map[string]interface{}); !ok { - t.Fatalf("Details is the wrong type: %v", dst["details"]) - } else if _, ok := det["friends"]; !ok { - t.Error("Could not find your friends. Maybe you don't have any. :-(") - } - - if dst["boat"].(string) != "pequod" { - t.Errorf("Expected boat string, got %v", dst["boat"]) - } - - if _, ok = dst["hole"]; ok { - t.Error("The hole still exists.") - } - - dst2 := map[string]interface{}{ - "name": "Ishmael", - "address": map[string]interface{}{ - "street": "123 Spouter Inn Ct.", - "city": "Nantucket", - "country": "US", - }, - "details": map[string]interface{}{ - "friends": []string{"Tashtego"}, - }, - "boat": "pequod", - "hole": "black", - } - - // What we expect is that anything in dst should have all values set, - // this happens when the --reuse-values flag is set but the chart has no modifications yet - CoalesceTables(dst2, nil) - - if dst2["name"] != "Ishmael" { - t.Errorf("Unexpected name: %s", dst2["name"]) - } - - addr2, ok := dst2["address"].(map[string]interface{}) - if !ok { - t.Fatal("Address went away.") - } + is := assert.New(t) + t.Run("case 1", func(t *testing.T) { + dst := map[string]interface{}{ + "name": "Ishmael", + "address": map[string]interface{}{ + "street": "123 Spouter Inn Ct.", + "city": "Nantucket", + "country": nil, + }, + "details": map[string]interface{}{ + "friends": []string{"Tashtego"}, + }, + "boat": "pequod", + "hole": nil, + } + src := map[string]interface{}{ + "occupation": "whaler", + "address": map[string]interface{}{ + "state": "MA", + "street": "234 Spouter Inn Ct.", + "country": "US", + }, + "details": "empty", + "boat": map[string]interface{}{ + "mast": true, + }, + "hole": "black", + } - if addr2["street"].(string) != "123 Spouter Inn Ct." { - t.Errorf("Unexpected address: %v", addr2["street"]) - } + // What we expect is that anything in dst overrides anything in src, but that + // otherwise the values are coalesced. + CoalesceTables(dst, src) + + is.Equal("Ishmael", dst["name"], "Unexpected name: %s", dst["name"]) + is.Equal("whaler", dst["occupation"], "Unexpected occupation: %s", dst["occupation"]) + + addr, ok := dst["address"].(map[string]interface{}) + req.True(t, ok, "Address went away.") + + is.Equal("123 Spouter Inn Ct.", addr["street"], "Unexpected address: %v", addr["street"]) + is.Equal("Nantucket", addr["city"], "Unexpected city: %v", addr["city"]) + is.Equal("MA", addr["state"], "Unexpected state: %v", addr["state"]) + _, ok = addr["country"] + is.False(ok, "The country should be removed") + + det, ok := dst["details"].(map[string]interface{}) + req.True(t, ok, "Details is the wrong type: %v", dst["details"]) + _, ok = det["friends"] + is.True(ok, "Could not find your friends. Maybe you don't have any. :-(") + + is.Equal("pequod", dst["boat"], "Expected boat string, got %v", dst["boat"]) + _, ok = dst["hole"] + is.False(ok, "The hole should be removed") + }) + t.Run("case 2", func(t *testing.T) { + dst2 := map[string]interface{}{ + "name": "Ishmael", + "address": map[string]interface{}{ + "street": "123 Spouter Inn Ct.", + "city": "Nantucket", + "country": "US", + }, + "details": map[string]interface{}{ + "friends": []string{"Tashtego"}, + }, + "boat": "pequod", + "hole": "black", + } - if addr2["city"].(string) != "Nantucket" { - t.Errorf("Unexpected city: %v", addr2["city"]) - } + // What we expect is that anything in dst should have all values set, + // this happens when the --reuse-values flag is set but the chart has no modifications yet + CoalesceTables(dst2, nil) + + is.Equal("Ishmael", dst2["name"], "Unexpected name: %s", dst2["name"]) + addr2, ok := dst2["address"].(map[string]interface{}) + req.True(t, ok, "Address went away.") + is.Equal("123 Spouter Inn Ct.", addr2["street"], "Unexpected address: %v", addr2["street"]) + is.Equal("Nantucket", addr2["city"], "Unexpected city: %v", addr2["city"]) + is.Equal("US", addr2["country"], "Unexpected country: %v", addr2["country"]) + is.Equal("US", addr2["country"], "Unexpected country: %v", addr2["country"]) + + det2, ok := dst2["details"].(map[string]interface{}) + req.True(t, ok, "Details is the wrong type: %v", dst2["details"]) + _, ok = det2["friends"] + is.True(ok, "Could not find your friends. Maybe you don't have any. :-(") + + is.Equal("pequod", dst2["boat"], "Expected boat string, got %v", dst2["boat"]) + is.Equal("black", dst2["hole"], "Expected hole string, got %v", dst2["hole"]) + }) + t.Run("empty chart map with nil user value", func(t *testing.T) { + dst := map[string]any{ + "foo": "bar", + "baz": nil, // explicit nil from user + } - if addr2["country"].(string) != "US" { - t.Errorf("Unexpected Country: %v", addr2["country"]) - } + // Chart's default values (src - lower priority) - empty map + src := map[string]any{} - if det2, ok := dst2["details"].(map[string]interface{}); !ok { - t.Fatalf("Details is the wrong type: %v", dst2["details"]) - } else if _, ok := det2["friends"]; !ok { - t.Error("Could not find your friends. Maybe you don't have any. :-(") - } + CoalesceTables(dst, src) - if dst2["boat"].(string) != "pequod" { - t.Errorf("Expected boat string, got %v", dst2["boat"]) - } + // "foo" should be preserved + is.Equal("bar", dst["foo"]) - if dst2["hole"].(string) != "black" { - t.Errorf("Expected hole string, got %v", dst2["boat"]) - } + _, ok := dst["baz"] + is.True(ok, "Expected baz key to be present but it was removed") + is.True(dst["baz"] == nil, "Expected baz key to be nil but it is not") + }) } func TestMergeTables(t *testing.T) { @@ -731,3 +714,37 @@ func TestConcatPrefix(t *testing.T) { assert.Equal(t, "b", concatPrefix("", "b")) assert.Equal(t, "a.b", concatPrefix("a", "b")) } + +// TestCoalesceValuesEmptyMapWithNils tests the full CoalesceValues scenario +// from issue #31643 where chart has data: {} and user provides data: {foo: bar, baz: ~} +func TestCoalesceValuesEmptyMapWithNils(t *testing.T) { + is := assert.New(t) + + c := &chart.Chart{ + Metadata: &chart.Metadata{Name: "test"}, + Values: map[string]any{ + "data": map[string]any{}, // empty map in chart defaults + }, + } + + vals := map[string]any{ + "data": map[string]any{ + "foo": "bar", + "baz": nil, // explicit nil from user + }, + } + + v, err := CoalesceValues(c, vals) + is.NoError(err) + + data, ok := v["data"].(map[string]any) + is.True(ok, "data is not a map") + + // "foo" should be preserved + is.Equal("bar", data["foo"]) + + // "baz" should be preserved with nil value since it wasn't in chart defaults + _, ok = data["baz"] + is.True(ok, "Expected data.baz key to be present but it was removed") + is.Nil(data["baz"], "Expected data.baz key to be nil but it is not") +} From 679f0519804afeaa5ce8b930a30976ade2860fe0 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Fri, 12 Dec 2025 17:00:18 +0000 Subject: [PATCH 2/3] Preserve nil values in chart already Signed-off-by: Evans Mungai --- pkg/chart/common/util/coalesce.go | 27 +++++++++++++++++++------- pkg/chart/common/util/coalesce_test.go | 13 +++++++++---- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/pkg/chart/common/util/coalesce.go b/pkg/chart/common/util/coalesce.go index 3220a275c..6c72b3d56 100644 --- a/pkg/chart/common/util/coalesce.go +++ b/pkg/chart/common/util/coalesce.go @@ -302,23 +302,36 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref if dst == nil { return src } + // Track original non-nil src keys before modifying src + // This lets us distinguish between user nullifying a chart default vs + // user setting nil for a key not in chart defaults. + srcOriginalNonNil := make(map[string]bool) + for key, val := range src { + if val != nil { + srcOriginalNonNil[key] = true + } + } + for key, val := range dst { + if val == nil { + src[key] = nil + } + } // Because dest has higher precedence than src, dest values override src // values. for key, val := range src { fullkey := concatPrefix(prefix, key) - dv, ok := dst[key] - if !ok { - dst[key] = val - } else if dv == nil && !merge && val != nil { + if dv, ok := dst[key]; ok && !merge && dv == nil && srcOriginalNonNil[key] { // When coalescing (not merging), if dst has nil and src has a non-nil // value, the user is nullifying a chart default - remove the key. - // Per Helm docs: setting a key to null deletes it. - // But if src also has nil (or key not in src), preserve the nil (issue #31643). + // But if src also has nil (or key not in src), preserve the nil delete(dst, key) + } else if !ok { + // key not in user values, preserve src value (including nil) + dst[key] = val } else if istable(val) { if istable(dv) { coalesceTablesFullKey(printf, dv.(map[string]interface{}), val.(map[string]interface{}), fullkey, merge) - } else if dv != nil { + } else { printf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val) } } else if istable(dv) && val != nil { diff --git a/pkg/chart/common/util/coalesce_test.go b/pkg/chart/common/util/coalesce_test.go index 598f1f9ef..84442e6cd 100644 --- a/pkg/chart/common/util/coalesce_test.go +++ b/pkg/chart/common/util/coalesce_test.go @@ -491,23 +491,28 @@ func TestCoalesceTables(t *testing.T) { is.Equal("pequod", dst2["boat"], "Expected boat string, got %v", dst2["boat"]) is.Equal("black", dst2["hole"], "Expected hole string, got %v", dst2["hole"]) }) - t.Run("empty chart map with nil user value", func(t *testing.T) { + t.Run("chart values with nil user value", func(t *testing.T) { dst := map[string]any{ "foo": "bar", "baz": nil, // explicit nil from user } // Chart's default values (src - lower priority) - empty map - src := map[string]any{} + src := map[string]any{ + "ben": nil, + } CoalesceTables(dst, src) // "foo" should be preserved is.Equal("bar", dst["foo"]) + _, ok := dst["ben"] + is.True(ok, "Expected ben key to be present") + is.Nil(dst["ben"], "Expected ben key to be nil but it is not") - _, ok := dst["baz"] + _, ok = dst["baz"] is.True(ok, "Expected baz key to be present but it was removed") - is.True(dst["baz"] == nil, "Expected baz key to be nil but it is not") + is.Nil(dst["baz"], "Expected baz key to be nil but it is not") }) } From 3416dd5f215a6421a70c6ab22340a96312ce8c0b Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Fri, 12 Dec 2025 17:09:18 +0000 Subject: [PATCH 3/3] Fix lint warning Signed-off-by: Evans Mungai --- pkg/chart/common/util/coalesce_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/chart/common/util/coalesce_test.go b/pkg/chart/common/util/coalesce_test.go index 84442e6cd..a7d841bbc 100644 --- a/pkg/chart/common/util/coalesce_test.go +++ b/pkg/chart/common/util/coalesce_test.go @@ -402,8 +402,8 @@ func TestMergeValues(t *testing.T) { } func TestCoalesceTables(t *testing.T) { - is := assert.New(t) t.Run("case 1", func(t *testing.T) { + is := assert.New(t) dst := map[string]interface{}{ "name": "Ishmael", "address": map[string]interface{}{ @@ -456,7 +456,8 @@ func TestCoalesceTables(t *testing.T) { _, ok = dst["hole"] is.False(ok, "The hole should be removed") }) - t.Run("case 2", func(t *testing.T) { + t.Run("case 2", func(_ *testing.T) { + is := assert.New(t) dst2 := map[string]interface{}{ "name": "Ishmael", "address": map[string]interface{}{ @@ -492,6 +493,7 @@ func TestCoalesceTables(t *testing.T) { is.Equal("black", dst2["hole"], "Expected hole string, got %v", dst2["hole"]) }) t.Run("chart values with nil user value", func(t *testing.T) { + is := assert.New(t) dst := map[string]any{ "foo": "bar", "baz": nil, // explicit nil from user