Merge pull request #21935 from hashicorp/jbardin/revert-21721

Revert "Remove removed attribute from applied state"

builtin/providers/test/resource.go

@@ -202,13 +202,16 @@ 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 mimics many providers always setting a *string value.
+	// The existing behavior is that this will appear in the state as an empty
+	// string, which we have to maintain.
+	o := d.Get("optional")
+	if o == "" {
+		d.Set("optional", nil)
 	}
 
 	// 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")
 	}

builtin/providers/test/resource_test.go

@@ -36,55 +36,6 @@ 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,
@@ -1099,3 +1050,39 @@ resource "test_resource" "foo" {
 		},
 	})
 }
+
+func TestResource_unsetNil(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"
+	}
+	optional = "a"
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("test_resource.foo", "optional", "a"),
+				),
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+	required = "yep"
+	required_map = {
+	    key = "value"
+	}
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("test_resource.foo", "optional", ""),
+				),
+			},
+		},
+	})
+}

helper/plugin/grpc_provider.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"log"
 	"strconv"
-	"strings"
 
 	"github.com/zclconf/go-cty/cty"
 	ctyconvert "github.com/zclconf/go-cty/cty/convert"
@@ -868,7 +867,6 @@ 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
@@ -876,10 +874,8 @@ 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, and record them
-		// to make sure they are actually removed from the state.
+		// prior state, or helper/schema will confuse itself
 		if d.NewRemoved {
-			newRemoved = append(newRemoved, k)
 			if _, ok := priorState.Attributes[k]; !ok {
 				delete(diff.Attributes, k)
 			}
@@ -908,19 +904,6 @@ 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())