From 2e2a3630528d00c9b6899145f34c9ed04a390300 Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Thu, 13 Jun 2019 16:53:36 -0400
Subject: [PATCH] Remove removed attribute from applied state

When a Diff contains a NewRemoved attribute (which would have been null
in the planned state), the final value is often the "zero" value string
for the type, which the provider itself still applies to the state.
Rather than risking a change of behavior in helper/schema by fixing the
inconsistency, we'll remove the NewRemoved attributes after apply to
prevent further issues resulting from the change in planned value.
---
 builtin/providers/test/resource.go      |  7 +++-
 builtin/providers/test/resource_test.go | 49 +++++++++++++++++++++++++
 helper/plugin/grpc_provider.go          | 19 +++++++++-
 3 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/builtin/providers/test/resource.go b/builtin/providers/test/resource.go
index 09732d421..28c5c79a1 100644
--- a/builtin/providers/test/resource.go
+++ b/builtin/providers/test/resource.go
@@ -202,8 +202,13 @@ func testResourceRead(d *schema.ResourceData, meta interface{}) error {
 		d.Set("set", []interface{}{})
 	}
 
+	_, ok := d.GetOk("optional_computed")
+	if !ok {
+		d.Set("optional_computed", "computed")
+	}
+
 	// This should not show as set unless it's set in the config
-	_, ok := d.GetOkExists("get_ok_exists_false")
+	_, ok = d.GetOkExists("get_ok_exists_false")
 	if ok {
 		return errors.New("get_ok_exists_false should not be set")
 	}
diff --git a/builtin/providers/test/resource_test.go b/builtin/providers/test/resource_test.go
index 8b78bb415..afcbf7967 100644
--- a/builtin/providers/test/resource_test.go
+++ b/builtin/providers/test/resource_test.go
@@ -36,6 +36,55 @@ resource "test_resource" "foo" {
 	})
 }
 
+func TestResource_removedOptional(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+	required = "yep"
+	required_map = {
+	    key = "value"
+	}
+	int = 1
+	optional = "string"
+	optional_computed = "not computed"
+	optional_bool = true
+}
+				`),
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+	required = "yep"
+	required_map = {
+	    key = "value"
+	}
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckNoResourceAttr(
+						"test_resource.foo", "int",
+					),
+					resource.TestCheckNoResourceAttr(
+						"test_resource.foo", "string",
+					),
+					resource.TestCheckNoResourceAttr(
+						"test_resource.foo", "optional_bool",
+					),
+					// while this isn't optimal, the prior config value being
+					// left for optional+computed is expected
+					resource.TestCheckResourceAttr(
+						"test_resource.foo", "optional_computed", "not computed",
+					),
+				),
+			},
+		},
+	})
+}
+
 func TestResource_changedList(t *testing.T) {
 	resource.UnitTest(t, resource.TestCase{
 		Providers:    testAccProviders,
diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go
index 161af4d46..09982f96b 100644
--- a/helper/plugin/grpc_provider.go
+++ b/helper/plugin/grpc_provider.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"log"
 	"strconv"
+	"strings"
 
 	"github.com/zclconf/go-cty/cty"
 	ctyconvert "github.com/zclconf/go-cty/cty/convert"
@@ -833,6 +834,7 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A
 		diff.Meta = private
 	}
 
+	var newRemoved []string
 	for k, d := range diff.Attributes {
 		// We need to turn off any RequiresNew. There could be attributes
 		// without changes in here inserted by helper/schema, but if they have
@@ -840,8 +842,10 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A
 		d.RequiresNew = false
 
 		// Check that any "removed" attributes that don't actually exist in the
-		// prior state, or helper/schema will confuse itself
+		// prior state, or helper/schema will confuse itself, and record them
+		// to make sure they are actually removed from the state.
 		if d.NewRemoved {
+			newRemoved = append(newRemoved, k)
 			if _, ok := priorState.Attributes[k]; !ok {
 				delete(diff.Attributes, k)
 			}
@@ -870,6 +874,19 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A
 		return resp, nil
 	}
 
+	// Now remove any primitive zero values that were left from NewRemoved
+	// attributes. Any attempt to reconcile more complex structures to the best
+	// of our abilities happens in normalizeNullValues.
+	for _, r := range newRemoved {
+		if strings.HasSuffix(r, ".#") || strings.HasSuffix(r, ".%") {
+			continue
+		}
+		switch newInstanceState.Attributes[r] {
+		case "", "0", "false":
+			delete(newInstanceState.Attributes, r)
+		}
+	}
+
 	// We keep the null val if we destroyed the resource, otherwise build the
 	// entire object, even if the new state was nil.
 	newStateVal, err = schema.StateValueFromInstanceState(newInstanceState, schemaBlock.ImpliedType())