From c0058b53ecb5dce0a47d7325340397bf572e2d93 Mon Sep 17 00:00:00 2001
From: Mingxiang Xue <mingxiangxue@gmail.com>
Date: Fri, 6 Mar 2020 22:13:49 +0800
Subject: [PATCH] Fix nested null value overrides

Signed-off-by: Mingxiang Xue <mingxiangxue@gmail.com>
---
 pkg/chartutil/coalesce.go               | 19 +++++++++---------
 pkg/chartutil/coalesce_test.go          | 26 +++++++++++++++++++++----
 pkg/chartutil/testdata/moby/values.yaml |  2 ++
 3 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/pkg/chartutil/coalesce.go b/pkg/chartutil/coalesce.go
index bbdd5f21c..9da5c1556 100644
--- a/pkg/chartutil/coalesce.go
+++ b/pkg/chartutil/coalesce.go
@@ -186,19 +186,18 @@ func CoalesceTables(dst, src map[string]interface{}) map[string]interface{} {
 	// Because dest has higher precedence than src, dest values override src
 	// values.
 	for key, val := range src {
-		if istable(val) {
-			switch innerdst, ok := dst[key]; {
-			case !ok:
-				dst[key] = val
-			case istable(innerdst):
-				CoalesceTables(innerdst.(map[string]interface{}), val.(map[string]interface{}))
-			default:
+		if dv, ok := dst[key]; ok && dv == nil {
+			delete(dst, key)
+		} else if !ok {
+			dst[key] = val
+		} else if istable(val) {
+			if istable(dv) {
+				CoalesceTables(dv.(map[string]interface{}), val.(map[string]interface{}))
+			} else {
 				log.Printf("warning: cannot overwrite table with non table for %s (%v)", key, val)
 			}
-		} else if dv, ok := dst[key]; ok && istable(dv) {
+		} else if istable(dv) {
 			log.Printf("warning: destination for %s is a table. Ignoring non-table value %v", key, val)
-		} else if !ok { // <- ok is still in scope from preceding conditional.
-			dst[key] = val
 		}
 	}
 	return dst
diff --git a/pkg/chartutil/coalesce_test.go b/pkg/chartutil/coalesce_test.go
index 6e82de590..fc3531a58 100644
--- a/pkg/chartutil/coalesce_test.go
+++ b/pkg/chartutil/coalesce_test.go
@@ -31,6 +31,8 @@ right: Null
 left: NULL
 front: ~
 back: ""
+nested:
+  boat: null
 
 global:
   name: Ishmael
@@ -114,6 +116,10 @@ func TestCoalesceValues(t *testing.T) {
 		}
 	}
 
+	if _, ok := v["nested"].(map[string]interface{})["boat"]; ok {
+		t.Error("Expected nested boat key to be removed, still present")
+	}
+
 	// CoalesceValues should not mutate the passed arguments
 	is.Equal(valsCopy, vals)
 }
@@ -122,24 +128,28 @@ func TestCoalesceTables(t *testing.T) {
 	dst := map[string]interface{}{
 		"name": "Ishmael",
 		"address": map[string]interface{}{
-			"street": "123 Spouter Inn Ct.",
-			"city":   "Nantucket",
+			"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.",
+			"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
@@ -170,6 +180,10 @@ func TestCoalesceTables(t *testing.T) {
 		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 {
@@ -179,4 +193,8 @@ func TestCoalesceTables(t *testing.T) {
 	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.")
+	}
 }
diff --git a/pkg/chartutil/testdata/moby/values.yaml b/pkg/chartutil/testdata/moby/values.yaml
index 54e1ce463..2169d7566 100644
--- a/pkg/chartutil/testdata/moby/values.yaml
+++ b/pkg/chartutil/testdata/moby/values.yaml
@@ -7,3 +7,5 @@ right: exists
 left: exists
 front: exists
 back: exists
+nested:
+  boat: true