Jlohmer/coalesce nil

.gitignore

@@ -3,6 +3,7 @@
 .DS_Store
 .coverage/
 .idea
+.claude
 .vimrc
 .vscode/
 .devcontainer/

pkg/chart/common/util/coalesce.go

@@ -251,13 +251,29 @@ func coalesceValues(printf printFn, c chart.Charter, v map[string]any, prefix st
 					// If the key is a child chart, coalesce tables with Merge set to true
 					merge := childChartMergeTrue(c, key, merge)
 
+					// When coalescing, clean nils from chart defaults before merging
+					// so they don't leak into the result.
+					if !merge {
+						cleanNilValues(src)
+					}
+
 					// Because v has higher precedence than nv, dest values override src
 					// values.
 					coalesceTablesFullKey(printf, dest, src, concatPrefix(subPrefix, key), merge)
 				}
 			}
 		} else {
 			// If the key is not in v, copy it from nv.
+			// When coalescing, skip chart default nils and clean nils from
+			// nested maps so they don't shadow globals or produce %!s(<nil>).
+			if !merge {
+				if val == nil {
+					continue
+				}
+				if sub, ok := val.(map[string]any); ok {
+					cleanNilValues(sub)
+				}
+			}
 			v[key] = val
 		}
 	}
@@ -326,7 +342,6 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]any, prefix strin
 			// 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) {
@@ -341,6 +356,18 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]any, prefix strin
 	return dst
 }
 
+// cleanNilValues recursively removes nil entries in-place from a map so that chart
+// default nils don't leak into the coalesced result.
+func cleanNilValues(m map[string]any) {
+	for key, val := range m {
+		if val == nil {
+			delete(m, key)
+		} else if sub, ok := val.(map[string]any); ok {
+			cleanNilValues(sub)
+		}
+	}
+}
+
 // istable is a special-purpose function to see if the present thing matches the definition of a YAML table.
 func istable(v any) bool {
 	_, ok := v.(map[string]any)

pkg/chart/common/util/coalesce_test.go

@@ -765,3 +765,166 @@ func TestCoalesceValuesEmptyMapWithNils(t *testing.T) {
 	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")
 }
+
+// TestCoalesceValuesSubchartDefaultNilsCleaned tests that nil values in subchart defaults
+// are cleaned up during coalescing when the parent doesn't set those keys.
+// Regression test for issue #31919.
+func TestCoalesceValuesSubchartDefaultNilsCleaned(t *testing.T) {
+	is := assert.New(t)
+
+	// Subchart has a default with nil values (e.g. keyMapping: {password: null})
+	subchart := &chart.Chart{
+		Metadata: &chart.Metadata{Name: "child"},
+		Values: map[string]any{
+			"keyMapping": map[string]any{
+				"password": nil,
+			},
+		},
+	}
+
+	parent := withDeps(&chart.Chart{
+		Metadata: &chart.Metadata{Name: "parent"},
+		Values:   map[string]any{},
+	}, subchart)
+
+	// Parent user values don't mention keyMapping at all
+	vals := map[string]any{}
+
+	v, err := CoalesceValues(parent, vals)
+	is.NoError(err)
+
+	childVals, ok := v["child"].(map[string]any)
+	is.True(ok, "child values should be a map")
+
+	keyMapping, ok := childVals["keyMapping"].(map[string]any)
+	is.True(ok, "keyMapping should be a map")
+
+	// The nil "password" key from chart defaults should be cleaned up
+	_, ok = keyMapping["password"]
+	is.False(ok, "Expected keyMapping.password (nil from chart defaults) to be removed, but it is still present")
+}
+
+// TestCoalesceValuesUserNullErasesSubchartDefault tests that a user-supplied null
+// value erases a subchart's default value during coalescing.
+// Regression test for issue #31919.
+func TestCoalesceValuesUserNullErasesSubchartDefault(t *testing.T) {
+	is := assert.New(t)
+
+	subchart := &chart.Chart{
+		Metadata: &chart.Metadata{Name: "child"},
+		Values: map[string]any{
+			"someKey": "default",
+		},
+	}
+
+	parent := withDeps(&chart.Chart{
+		Metadata: &chart.Metadata{Name: "parent"},
+		Values:   map[string]any{},
+	}, subchart)
+
+	// User explicitly nullifies the subchart key via parent values
+	vals := map[string]any{
+		"child": map[string]any{
+			"someKey": nil,
+		},
+	}
+
+	v, err := CoalesceValues(parent, vals)
+	is.NoError(err)
+
+	childVals, ok := v["child"].(map[string]any)
+	is.True(ok, "child values should be a map")
+
+	// someKey should be erased — user null overrides subchart default
+	_, ok = childVals["someKey"]
+	is.False(ok, "Expected someKey to be removed by user null override, but it is still present")
+}
+
+// TestCoalesceValuesSubchartNilDoesNotShadowGlobal tests that a nil value in
+// subchart defaults doesn't shadow a global value accessible via pluck-like access.
+// Regression test for issue #31971.
+func TestCoalesceValuesSubchartNilDoesNotShadowGlobal(t *testing.T) {
+	is := assert.New(t)
+
+	subchart := &chart.Chart{
+		Metadata: &chart.Metadata{Name: "child"},
+		Values: map[string]any{
+			"ingress": map[string]any{
+				"feature": nil, // nil in subchart defaults
+			},
+		},
+	}
+
+	parent := withDeps(&chart.Chart{
+		Metadata: &chart.Metadata{Name: "parent"},
+		Values:   map[string]any{},
+	}, subchart)
+
+	// Parent sets the global value
+	vals := map[string]any{
+		"global": map[string]any{
+			"ingress": map[string]any{
+				"feature": true,
+			},
+		},
+	}
+
+	v, err := CoalesceValues(parent, vals)
+	is.NoError(err)
+
+	childVals, ok := v["child"].(map[string]any)
+	is.True(ok, "child values should be a map")
+
+	ingress, ok := childVals["ingress"].(map[string]any)
+	is.True(ok, "ingress should be a map")
+
+	// The nil "feature" from subchart defaults should be cleaned up,
+	// so that pluck can fall through to the global value
+	_, ok = ingress["feature"]
+	is.False(ok, "Expected ingress.feature (nil from chart defaults) to be removed so global can be used via pluck, but it is still present")
+}
+
+// TestCoalesceValuesSubchartNilCleanedWhenUserPartiallyOverrides tests that nil
+// values in subchart defaults are cleaned even when the user partially overrides
+// the same map. Regression test for the coalesceTablesFullKey merge path.
+func TestCoalesceValuesSubchartNilCleanedWhenUserPartiallyOverrides(t *testing.T) {
+	is := assert.New(t)
+
+	subchart := &chart.Chart{
+		Metadata: &chart.Metadata{Name: "child"},
+		Values: map[string]any{
+			"keyMapping": map[string]any{
+				"password": nil,
+				"format":   "bcrypt",
+			},
+		},
+	}
+
+	parent := withDeps(&chart.Chart{
+		Metadata: &chart.Metadata{Name: "parent"},
+		Values:   map[string]any{},
+	}, subchart)
+
+	// User overrides format but doesn't mention password
+	vals := map[string]any{
+		"child": map[string]any{
+			"keyMapping": map[string]any{
+				"format": "sha256",
+			},
+		},
+	}
+
+	v, err := CoalesceValues(parent, vals)
+	is.NoError(err)
+
+	childVals, ok := v["child"].(map[string]any)
+	is.True(ok, "child values should be a map")
+
+	keyMapping, ok := childVals["keyMapping"].(map[string]any)
+	is.True(ok, "keyMapping should be a map")
+
+	is.Equal("sha256", keyMapping["format"], "User override should be preserved")
+
+	_, ok = keyMapping["password"]
+	is.False(ok, "Expected keyMapping.password (nil from chart defaults) to be removed even when user partially overrides the map")
+}

pkg/cmd/testdata/output/issue-9027.txt

@@ -2,11 +2,15 @@
 # Source: issue-9027/charts/subchart/templates/values.yaml
 global:
   hash:
+    key1: 1
+    key2: 2
     key3: 13
     key4: 4
     key5: 5
     key6: 6
 hash:
+  key1: 1
+  key2: 2
   key3: 13
   key4: 4
   key5: 5
@@ -15,17 +19,19 @@ hash:
 # Source: issue-9027/templates/values.yaml
 global:
   hash:
-    key1: null
-    key2: null
     key3: 13
 subchart:
   global:
     hash:
+      key1: 1
+      key2: 2
       key3: 13
       key4: 4
       key5: 5
       key6: 6
   hash:
+    key1: 1
+    key2: 2
     key3: 13
     key4: 4
     key5: 5

pkg/engine/engine_test.go

@@ -1505,3 +1505,65 @@ func TestTraceableError_NoTemplateForm(t *testing.T) {
 		}
 	}
 }
+
+// TestRenderSubchartDefaultNilNoStringify tests the full pipeline: subchart default
+// nil values should not produce "%!s(<nil>)" in rendered template output.
+// Regression test for the Bitnami common.secrets.key issue.
+func TestRenderSubchartDefaultNilNoStringify(t *testing.T) {
+	modTime := time.Now()
+
+	// Subchart has a default with nil values
+	subchart := &chart.Chart{
+		Metadata: &chart.Metadata{Name: "child"},
+		Templates: []*common.File{
+			{
+				Name:    "templates/test.yaml",
+				ModTime: modTime,
+				Data:    []byte(`{{- if hasKey .Values.keyMapping "password" -}}{{- printf "subPath: %s" (index .Values.keyMapping "password") -}}{{- else -}}subPath: fallback{{- end -}}`),
+			},
+		},
+		Values: map[string]any{
+			"keyMapping": map[string]any{
+				"password": nil, // nil in chart defaults
+			},
+		},
+	}
+
+	parent := &chart.Chart{
+		Metadata: &chart.Metadata{Name: "parent"},
+		Values:   map[string]any{},
+	}
+	parent.AddDependency(subchart)
+
+	// Parent user values don't set keyMapping
+	injValues := map[string]any{}
+
+	tmp, err := util.CoalesceValues(parent, injValues)
+	if err != nil {
+		t.Fatalf("Failed to coalesce values: %s", err)
+	}
+
+	inject := common.Values{
+		"Values": tmp,
+		"Chart":  parent.Metadata,
+		"Release": common.Values{
+			"Name": "test-release",
+		},
+	}
+
+	out, err := Render(parent, inject)
+	if err != nil {
+		t.Fatalf("Failed to render templates: %s", err)
+	}
+
+	rendered := out["parent/charts/child/templates/test.yaml"]
+
+	if strings.Contains(rendered, "%!s(<nil>)") {
+		t.Errorf("Rendered output contains %%!s(<nil>), got: %q", rendered)
+	}
+
+	expected := "subPath: fallback"
+	if rendered != expected {
+		t.Errorf("Expected %q, got %q", expected, rendered)
+	}
+}