Skip to content

Commit b13743c

Browse files
banjohKrzysztofDziankowski
banjoh
authored and
KrzysztofDziankowski
committed
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 <mbuevans@gmail.com>
1 parent b2e4314 commit b13743c

2 files changed

Lines changed: 145 additions & 128 deletions

File tree

‎pkg/chartutil/coalesce.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -283,23 +283,23 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]interface{}, pref
283283
if dst == nil {
284284
return src
285285
}
286-
for key, val := range dst {
287-
if val == nil {
288-
src[key] = nil
289-
}
290-
}
291286
// Because dest has higher precedence than src, dest values override src
292287
// values.
293288
for key, val := range src {
294289
fullkey := concatPrefix(prefix, key)
295-
if dv, ok := dst[key]; ok && !merge && dv == nil {
296-
delete(dst, key)
297-
} else if !ok {
290+
dv, ok := dst[key]
291+
if !ok {
298292
dst[key] = val
293+
} else if dv == nil && !merge && val != nil {
294+
// When coalescing (not merging), if dst has nil and src has a non-nil
295+
// value, the user is nullifying a chart default - remove the key.
296+
// Per Helm docs: setting a key to null deletes it.
297+
// But if src also has nil (or key not in src), preserve the nil (issue #31643).
298+
delete(dst, key)
299299
} else if istable(val) {
300300
if istable(dv) {
301301
coalesceTablesFullKey(printf, dv.(map[string]interface{}), val.(map[string]interface{}), fullkey, merge)
302-
} else {
302+
} else if dv != nil {
303303
printf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val)
304304
}
305305
} else if istable(dv) && val != nil {

‎pkg/chartutil/coalesce_test.go‎

Lines changed: 136 additions & 119 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"testing"
2323

2424
"github.com/stretchr/testify/assert"
25+
req "github.com/stretchr/testify/require"
2526

2627
"helm.sh/helm/v3/pkg/chart"
2728
)
@@ -394,131 +395,113 @@ func TestMergeValues(t *testing.T) {
394395
}
395396

396397
func TestCoalesceTables(t *testing.T) {
397-
dst := map[string]interface{}{
398-
"name": "Ishmael",
399-
"address": map[string]interface{}{
400-
"street": "123 Spouter Inn Ct.",
401-
"city": "Nantucket",
402-
"country": nil,
403-
},
404-
"details": map[string]interface{}{
405-
"friends": []string{"Tashtego"},
406-
},
407-
"boat": "pequod",
408-
"hole": nil,
409-
}
410-
src := map[string]interface{}{
411-
"occupation": "whaler",
412-
"address": map[string]interface{}{
413-
"state": "MA",
414-
"street": "234 Spouter Inn Ct.",
415-
"country": "US",
416-
},
417-
"details": "empty",
418-
"boat": map[string]interface{}{
419-
"mast": true,
420-
},
421-
"hole": "black",
422-
}
423-
424-
// What we expect is that anything in dst overrides anything in src, but that
425-
// otherwise the values are coalesced.
426-
CoalesceTables(dst, src)
427-
428-
if dst["name"] != "Ishmael" {
429-
t.Errorf("Unexpected name: %s", dst["name"])
430-
}
431-
if dst["occupation"] != "whaler" {
432-
t.Errorf("Unexpected occupation: %s", dst["occupation"])
433-
}
434-
435-
addr, ok := dst["address"].(map[string]interface{})
436-
if !ok {
437-
t.Fatal("Address went away.")
438-
}
439-
440-
if addr["street"].(string) != "123 Spouter Inn Ct." {
441-
t.Errorf("Unexpected address: %v", addr["street"])
442-
}
443-
444-
if addr["city"].(string) != "Nantucket" {
445-
t.Errorf("Unexpected city: %v", addr["city"])
446-
}
447-
448-
if addr["state"].(string) != "MA" {
449-
t.Errorf("Unexpected state: %v", addr["state"])
450-
}
451-
452-
if _, ok = addr["country"]; ok {
453-
t.Error("The country is not left out.")
454-
}
455-
456-
if det, ok := dst["details"].(map[string]interface{}); !ok {
457-
t.Fatalf("Details is the wrong type: %v", dst["details"])
458-
} else if _, ok := det["friends"]; !ok {
459-
t.Error("Could not find your friends. Maybe you don't have any. :-(")
460-
}
461-
462-
if dst["boat"].(string) != "pequod" {
463-
t.Errorf("Expected boat string, got %v", dst["boat"])
464-
}
465-
466-
if _, ok = dst["hole"]; ok {
467-
t.Error("The hole still exists.")
468-
}
469-
470-
dst2 := map[string]interface{}{
471-
"name": "Ishmael",
472-
"address": map[string]interface{}{
473-
"street": "123 Spouter Inn Ct.",
474-
"city": "Nantucket",
475-
"country": "US",
476-
},
477-
"details": map[string]interface{}{
478-
"friends": []string{"Tashtego"},
479-
},
480-
"boat": "pequod",
481-
"hole": "black",
482-
}
483-
484-
// What we expect is that anything in dst should have all values set,
485-
// this happens when the --reuse-values flag is set but the chart has no modifications yet
486-
CoalesceTables(dst2, nil)
487-
488-
if dst2["name"] != "Ishmael" {
489-
t.Errorf("Unexpected name: %s", dst2["name"])
490-
}
491-
492-
addr2, ok := dst2["address"].(map[string]interface{})
493-
if !ok {
494-
t.Fatal("Address went away.")
495-
}
398+
is := assert.New(t)
399+
t.Run("case 1", func(t *testing.T) {
400+
dst := map[string]interface{}{
401+
"name": "Ishmael",
402+
"address": map[string]interface{}{
403+
"street": "123 Spouter Inn Ct.",
404+
"city": "Nantucket",
405+
"country": nil,
406+
},
407+
"details": map[string]interface{}{
408+
"friends": []string{"Tashtego"},
409+
},
410+
"boat": "pequod",
411+
"hole": nil,
412+
}
413+
src := map[string]interface{}{
414+
"occupation": "whaler",
415+
"address": map[string]interface{}{
416+
"state": "MA",
417+
"street": "234 Spouter Inn Ct.",
418+
"country": "US",
419+
},
420+
"details": "empty",
421+
"boat": map[string]interface{}{
422+
"mast": true,
423+
},
424+
"hole": "black",
425+
}
496426

497-
if addr2["street"].(string) != "123 Spouter Inn Ct." {
498-
t.Errorf("Unexpected address: %v", addr2["street"])
499-
}
427+
// What we expect is that anything in dst overrides anything in src, but that
428+
// otherwise the values are coalesced.
429+
CoalesceTables(dst, src)
430+
431+
is.Equal("Ishmael", dst["name"], "Unexpected name: %s", dst["name"])
432+
is.Equal("whaler", dst["occupation"], "Unexpected occupation: %s", dst["occupation"])
433+
434+
addr, ok := dst["address"].(map[string]interface{})
435+
req.True(t, ok, "Address went away.")
436+
437+
is.Equal("123 Spouter Inn Ct.", addr["street"], "Unexpected address: %v", addr["street"])
438+
is.Equal("Nantucket", addr["city"], "Unexpected city: %v", addr["city"])
439+
is.Equal("MA", addr["state"], "Unexpected state: %v", addr["state"])
440+
_, ok = addr["country"]
441+
is.False(ok, "The country should be removed")
442+
443+
det, ok := dst["details"].(map[string]interface{})
444+
req.True(t, ok, "Details is the wrong type: %v", dst["details"])
445+
_, ok = det["friends"]
446+
is.True(ok, "Could not find your friends. Maybe you don't have any. :-(")
447+
448+
is.Equal("pequod", dst["boat"], "Expected boat string, got %v", dst["boat"])
449+
_, ok = dst["hole"]
450+
is.False(ok, "The hole should be removed")
451+
})
452+
t.Run("case 2", func(t *testing.T) {
453+
dst2 := map[string]interface{}{
454+
"name": "Ishmael",
455+
"address": map[string]interface{}{
456+
"street": "123 Spouter Inn Ct.",
457+
"city": "Nantucket",
458+
"country": "US",
459+
},
460+
"details": map[string]interface{}{
461+
"friends": []string{"Tashtego"},
462+
},
463+
"boat": "pequod",
464+
"hole": "black",
465+
}
500466

501-
if addr2["city"].(string) != "Nantucket" {
502-
t.Errorf("Unexpected city: %v", addr2["city"])
503-
}
467+
// What we expect is that anything in dst should have all values set,
468+
// this happens when the --reuse-values flag is set but the chart has no modifications yet
469+
CoalesceTables(dst2, nil)
470+
471+
is.Equal("Ishmael", dst2["name"], "Unexpected name: %s", dst2["name"])
472+
addr2, ok := dst2["address"].(map[string]interface{})
473+
req.True(t, ok, "Address went away.")
474+
is.Equal("123 Spouter Inn Ct.", addr2["street"], "Unexpected address: %v", addr2["street"])
475+
is.Equal("Nantucket", addr2["city"], "Unexpected city: %v", addr2["city"])
476+
is.Equal("US", addr2["country"], "Unexpected country: %v", addr2["country"])
477+
is.Equal("US", addr2["country"], "Unexpected country: %v", addr2["country"])
478+
479+
det2, ok := dst2["details"].(map[string]interface{})
480+
req.True(t, ok, "Details is the wrong type: %v", dst2["details"])
481+
_, ok = det2["friends"]
482+
is.True(ok, "Could not find your friends. Maybe you don't have any. :-(")
483+
484+
is.Equal("pequod", dst2["boat"], "Expected boat string, got %v", dst2["boat"])
485+
is.Equal("black", dst2["hole"], "Expected hole string, got %v", dst2["hole"])
486+
})
487+
t.Run("empty chart map with nil user value", func(t *testing.T) {
488+
dst := map[string]any{
489+
"foo": "bar",
490+
"baz": nil, // explicit nil from user
491+
}
504492

505-
if addr2["country"].(string) != "US" {
506-
t.Errorf("Unexpected Country: %v", addr2["country"])
507-
}
493+
// Chart's default values (src - lower priority) - empty map
494+
src := map[string]any{}
508495

509-
if det2, ok := dst2["details"].(map[string]interface{}); !ok {
510-
t.Fatalf("Details is the wrong type: %v", dst2["details"])
511-
} else if _, ok := det2["friends"]; !ok {
512-
t.Error("Could not find your friends. Maybe you don't have any. :-(")
513-
}
496+
CoalesceTables(dst, src)
514497

515-
if dst2["boat"].(string) != "pequod" {
516-
t.Errorf("Expected boat string, got %v", dst2["boat"])
517-
}
498+
// "foo" should be preserved
499+
is.Equal("bar", dst["foo"])
518500

519-
if dst2["hole"].(string) != "black" {
520-
t.Errorf("Expected hole string, got %v", dst2["boat"])
521-
}
501+
_, ok := dst["baz"]
502+
is.True(ok, "Expected baz key to be present but it was removed")
503+
is.True(dst["baz"] == nil, "Expected baz key to be nil but it is not")
504+
})
522505
}
523506

524507
func TestMergeTables(t *testing.T) {
@@ -724,3 +707,37 @@ func TestConcatPrefix(t *testing.T) {
724707
assert.Equal(t, "b", concatPrefix("", "b"))
725708
assert.Equal(t, "a.b", concatPrefix("a", "b"))
726709
}
710+
711+
// TestCoalesceValuesEmptyMapWithNils tests the full CoalesceValues scenario
712+
// from issue #31643 where chart has data: {} and user provides data: {foo: bar, baz: ~}
713+
func TestCoalesceValuesEmptyMapWithNils(t *testing.T) {
714+
is := assert.New(t)
715+
716+
c := &chart.Chart{
717+
Metadata: &chart.Metadata{Name: "test"},
718+
Values: map[string]any{
719+
"data": map[string]any{}, // empty map in chart defaults
720+
},
721+
}
722+
723+
vals := map[string]any{
724+
"data": map[string]any{
725+
"foo": "bar",
726+
"baz": nil, // explicit nil from user
727+
},
728+
}
729+
730+
v, err := CoalesceValues(c, vals)
731+
is.NoError(err)
732+
733+
data, ok := v["data"].(map[string]any)
734+
is.True(ok, "data is not a map")
735+
736+
// "foo" should be preserved
737+
is.Equal("bar", data["foo"])
738+
739+
// "baz" should be preserved with nil value since it wasn't in chart defaults
740+
_, ok = data["baz"]
741+
is.True(ok, "Expected data.baz key to be present but it was removed")
742+
is.Nil(data["baz"], "Expected data.baz key to be nil but it is not")
743+
}

0 commit comments

Comments
 (0)