From 4dcda04d966c1e87b006df5390e81bb82b8454d3 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Wed, 19 Jun 2019 17:14:14 -0400
Subject: [PATCH] prevent an empty string from being lost

The helper/schema diff process loses empty strings, causing them to show
up as unset (null) during apply. Besides failing to show as set by
GetOk, the absence of the value also triggers the schema to insert a
default value again during apply.

It would also be be preferable if the defaults weren't re-evaluated
again during ApplyResourceChange, but that would require a more invasive
patch to the field readers, and ensuring the empty string is stored in
the plan should block the default.
---
 .../providers/test/resource_defaults_test.go  | 19 ++++++++++++++
 helper/plugin/grpc_provider.go                | 10 ++++++--
 helper/plugin/grpc_provider_test.go           | 25 +++++++++++++++++++
 3 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/builtin/providers/test/resource_defaults_test.go b/builtin/providers/test/resource_defaults_test.go
index b2659966d..eecb00638 100644
--- a/builtin/providers/test/resource_defaults_test.go
+++ b/builtin/providers/test/resource_defaults_test.go
@@ -174,3 +174,22 @@ resource "test_resource_defaults" "foo" {
 		},
 	})
 }
+
+func TestDefaults_emptyString(t *testing.T) {
+	config := `
+resource "test_resource_defaults" "test" {
+  default_string = ""
+}
+`
+	resource.UnitTest(t, resource.TestCase{
+		Providers: testAccProviders,
+		Steps: []resource.TestStep{
+			{
+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("test_resource_defaults.test", "default_string", ""),
+				),
+			},
+		},
+	})
+}
diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go
index db9d12cfb..7f670eb82 100644
--- a/helper/plugin/grpc_provider.go
+++ b/helper/plugin/grpc_provider.go
@@ -1201,6 +1201,8 @@ func normalizeNullValues(dst, src cty.Value, apply bool) cty.Value {
 		}
 	}
 
+	// check the invariants that we need below, to ensure we are working with
+	// non-null and known values.
 	if src.IsNull() || !src.IsKnown() || !dst.IsKnown() {
 		return dst
 	}
@@ -1319,8 +1321,12 @@ func normalizeNullValues(dst, src cty.Value, apply bool) cty.Value {
 			return cty.ListVal(dsts)
 		}
 
-	case ty.IsPrimitiveType():
-		if dst.IsNull() && src.IsWhollyKnown() && apply {
+	case ty == cty.String:
+		// The legacy SDK should not be able to remove a value during plan or
+		// apply, however we are only going to overwrite this if the source was
+		// an empty string, since that is what is often equated with unset and
+		// lost in the diff process.
+		if dst.IsNull() && src.AsString() == "" {
 			return src
 		}
 	}
diff --git a/helper/plugin/grpc_provider_test.go b/helper/plugin/grpc_provider_test.go
index 91b205a59..3f6af35b0 100644
--- a/helper/plugin/grpc_provider_test.go
+++ b/helper/plugin/grpc_provider_test.go
@@ -984,6 +984,18 @@ func TestNormalizeNullValues(t *testing.T) {
 		},
 		{
 			// Plan primitives are kept
+			Src: cty.ObjectVal(map[string]cty.Value{
+				"string": cty.NumberIntVal(0),
+			}),
+			Dst: cty.ObjectVal(map[string]cty.Value{
+				"string": cty.NullVal(cty.Number),
+			}),
+			Expect: cty.ObjectVal(map[string]cty.Value{
+				"string": cty.NullVal(cty.Number),
+			}),
+		},
+		{
+			// Neither plan nor apply should remove empty strings
 			Src: cty.ObjectVal(map[string]cty.Value{
 				"string": cty.StringVal(""),
 			}),
@@ -991,8 +1003,21 @@ func TestNormalizeNullValues(t *testing.T) {
 				"string": cty.NullVal(cty.String),
 			}),
 			Expect: cty.ObjectVal(map[string]cty.Value{
+				"string": cty.StringVal(""),
+			}),
+		},
+		{
+			// Neither plan nor apply should remove empty strings
+			Src: cty.ObjectVal(map[string]cty.Value{
+				"string": cty.StringVal(""),
+			}),
+			Dst: cty.ObjectVal(map[string]cty.Value{
 				"string": cty.NullVal(cty.String),
 			}),
+			Expect: cty.ObjectVal(map[string]cty.Value{
+				"string": cty.StringVal(""),
+			}),
+			Apply: true,
 		},
 		{
 			// The null map is retained, because the src was unknown