From cf61a689eb8b957cbe6e9103c188c6cfffc745a4 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Mon, 13 May 2019 18:39:55 -0400
Subject: [PATCH] only hold back empty container changes in apply

When normalizing the state during read, if the resource was previously
imported, most nil-able values will be nil, and we need to prefer the
values returned by the latest Read operation. This didn't come up
before, because Read is usually working with a state create by plan and
Apply which has already shaped the state with the expected empty values.

Having the src value preferred only during Apply better follows the
intent of this function, which should allow Read to return whatever
values it deems necessary. Since Read and Plan use the same
normalization logic, the implied Read before plan should take care of any
perpetual diffs.
---
 helper/plugin/grpc_provider.go      | 22 +++++++++---------
 helper/plugin/grpc_provider_test.go | 35 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go
index c8065cac3..b9b420995 100644
--- a/helper/plugin/grpc_provider.go
+++ b/helper/plugin/grpc_provider.go
@@ -1163,25 +1163,22 @@ func normalizeNullValues(dst, src cty.Value, apply bool) cty.Value {
 	if !src.IsNull() && !src.IsKnown() {
 		// Return src during plan to retain unknown interpolated placeholders,
 		// which could be lost if we're only updating a resource. If this is a
-		// read scenario, then there shouldn't be any unknowns all.
+		// read scenario, then there shouldn't be any unknowns at all.
 		if dst.IsNull() && !apply {
 			return src
 		}
 		return dst
 	}
 
-	// handle null/empty changes for collections
-	if ty.IsCollectionType() {
-		if src.IsNull() && !dst.IsNull() && dst.IsKnown() {
-			if dst.LengthInt() == 0 {
-				return src
-			}
-		}
+	// Handle null/empty changes for collections during apply.
+	// A change between null and empty values prefers src to make sure the state
+	// is consistent between plan and apply.
+	if ty.IsCollectionType() && apply {
+		dstEmpty := !dst.IsNull() && dst.IsKnown() && dst.LengthInt() == 0
+		srcEmpty := !src.IsNull() && src.IsKnown() && src.LengthInt() == 0
 
-		if dst.IsNull() && !src.IsNull() && src.IsKnown() {
-			if src.LengthInt() == 0 {
-				return src
-			}
+		if (src.IsNull() && dstEmpty) || (srcEmpty && dst.IsNull()) {
+			return src
 		}
 	}
 
@@ -1214,6 +1211,7 @@ func normalizeNullValues(dst, src cty.Value, apply bool) cty.Value {
 				}
 				dstVal = cty.NullVal(v.Type())
 			}
+
 			dstMap[key] = normalizeNullValues(dstVal, v, apply)
 		}
 
diff --git a/helper/plugin/grpc_provider_test.go b/helper/plugin/grpc_provider_test.go
index 4a61f24fb..f58778c47 100644
--- a/helper/plugin/grpc_provider_test.go
+++ b/helper/plugin/grpc_provider_test.go
@@ -1141,6 +1141,41 @@ func TestNormalizeNullValues(t *testing.T) {
 				}),
 			}),
 		},
+		{
+			Src: cty.ObjectVal(map[string]cty.Value{
+				"set": cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{
+					"list": cty.List(cty.String),
+				}))),
+			}),
+			Dst: cty.ObjectVal(map[string]cty.Value{
+				"set": cty.SetValEmpty(cty.Object(map[string]cty.Type{
+					"list": cty.List(cty.String),
+				})),
+			}),
+			Expect: cty.ObjectVal(map[string]cty.Value{
+				"set": cty.SetValEmpty(cty.Object(map[string]cty.Type{
+					"list": cty.List(cty.String),
+				})),
+			}),
+		},
+		{
+			Src: cty.ObjectVal(map[string]cty.Value{
+				"set": cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{
+					"list": cty.List(cty.String),
+				}))),
+			}),
+			Dst: cty.ObjectVal(map[string]cty.Value{
+				"set": cty.SetValEmpty(cty.Object(map[string]cty.Type{
+					"list": cty.List(cty.String),
+				})),
+			}),
+			Expect: cty.ObjectVal(map[string]cty.Value{
+				"set": cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{
+					"list": cty.List(cty.String),
+				}))),
+			}),
+			Apply: true,
+		},
 	} {
 		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
 			got := normalizeNullValues(tc.Dst, tc.Src, tc.Apply)