don't re-add removed list values even when planned

Providers were not strict (and were not forced to be) about customizing
the diff when a computed attribute needed to be updated during apply.
The fix we have in place to prevent loss of information during the
helper/schema apply process would add in single missing value back in.

The first place this was caught was when we attempt to fix up the
flatmapped attributes. The 1->0 count error is now better handled by our
cty.Value normalization step, so we can remove the special apply case
here altogether

The next place is in normalizeNullValues, and since the intent was to
re-insert missing zero-value lists and sets, adding a check for a length
of 0 protects us from adding in extra elements.

The new test fixture emulated common provider behavior of re-computing
values without customizing the diff. Since we can work around it, and
core will provider appropriate warnings, the shims should try to
maintain the legacy behavior.

builtin/providers/test/resource_list.go

@@ -58,21 +58,54 @@ func testResourceList() *schema.Resource {
 					},
 				},
 			},
+			"dependent_list": {
+				Type:     schema.TypeList,
+				Optional: true,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"val": {
+							Type:     schema.TypeString,
+							Required: true,
+						},
+					},
+				},
+			},
+			"computed_list": {
+				Type:     schema.TypeList,
+				Computed: true,
+				Elem:     &schema.Schema{Type: schema.TypeString},
+			},
 		},
 	}
 }
 
 func testResourceListCreate(d *schema.ResourceData, meta interface{}) error {
 	d.SetId("testId")
-	return nil
+	return testResourceListRead(d, meta)
 }
 
 func testResourceListRead(d *schema.ResourceData, meta interface{}) error {
+	fixedIps := d.Get("dependent_list")
+
+	// all_fixed_ips should be set as computed with a CustomizeDiff func, but
+	// we're trying to emulate legacy provider behavior, and updating a
+	// computed field was a common case.
+	ips := []interface{}{}
+	if fixedIps != nil {
+		for _, v := range fixedIps.([]interface{}) {
+			m := v.(map[string]interface{})
+			ips = append(ips, m["val"])
+		}
+	}
+	if err := d.Set("computed_list", ips); err != nil {
+		return err
+	}
+
 	return nil
 }
 
 func testResourceListUpdate(d *schema.ResourceData, meta interface{}) error {
-	return nil
+	return testResourceListRead(d, meta)
 }
 
 func testResourceListDelete(d *schema.ResourceData, meta interface{}) error {

builtin/providers/test/resource_list_test.go

@@ -276,3 +276,45 @@ resource "test_resource_list" "foo" {
 		},
 	})
 }
+
+func TestResourceList_addRemove(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_list" "foo" {
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("test_resource_list.foo", "computed_list.#", "0"),
+					resource.TestCheckResourceAttr("test_resource_list.foo", "dependent_list.#", "0"),
+				),
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_list" "foo" {
+	dependent_list {
+		val = "a"
+	}
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("test_resource_list.foo", "computed_list.#", "1"),
+					resource.TestCheckResourceAttr("test_resource_list.foo", "dependent_list.#", "1"),
+				),
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_list" "foo" {
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("test_resource_list.foo", "computed_list.#", "0"),
+					resource.TestCheckResourceAttr("test_resource_list.foo", "dependent_list.#", "0"),
+				),
+			},
+		},
+	})
+}

helper/plugin/grpc_provider.go

@@ -431,7 +431,7 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso
 		// here we use the prior state to check for unknown/zero containers values
 		// when normalizing the flatmap.
 		stateAttrs := hcl2shim.FlatmapValueFromHCL2(stateVal)
-		newInstanceState.Attributes = normalizeFlatmapContainers(stateAttrs, newInstanceState.Attributes, false)
+		newInstanceState.Attributes = normalizeFlatmapContainers(stateAttrs, newInstanceState.Attributes)
 	}
 
 	if newInstanceState == nil || newInstanceState.ID == "" {
@@ -572,7 +572,7 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl
 	// now we need to apply the diff to the prior state, so get the planned state
 	plannedAttrs, err := diff.Apply(priorState.Attributes, block)
 
-	plannedAttrs = normalizeFlatmapContainers(priorState.Attributes, plannedAttrs, false)
+	plannedAttrs = normalizeFlatmapContainers(priorState.Attributes, plannedAttrs)
 
 	plannedStateVal, err := hcl2shim.HCL2ValueFromFlatmap(plannedAttrs, block.ImpliedType())
 	if err != nil {
@@ -808,7 +808,7 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A
 	// here we use the planned state to check for unknown/zero containers values
 	// when normalizing the flatmap.
 	plannedState := hcl2shim.FlatmapValueFromHCL2(plannedStateVal)
-	newInstanceState.Attributes = normalizeFlatmapContainers(plannedState, newInstanceState.Attributes, true)
+	newInstanceState.Attributes = normalizeFlatmapContainers(plannedState, newInstanceState.Attributes)
 
 	// We keep the null val if we destroyed the resource, otherwise build the
 	// entire object, even if the new state was nil.
@@ -819,6 +819,7 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A
 	}
 
 	newStateVal = normalizeNullValues(newStateVal, plannedStateVal, false)
+
 	newStateVal = copyTimeoutValues(newStateVal, plannedStateVal)
 
 	newStateMP, err := msgpack.Marshal(newStateVal, block.ImpliedType())
@@ -995,7 +996,7 @@ func pathToAttributePath(path cty.Path) *proto.AttributePath {
 // allows a provider to set an empty computed container in the state without
 // creating perpetual diff. This can differ slightly between plan and apply, so
 // the apply flag is passed when called from ApplyResourceChange.
-func normalizeFlatmapContainers(prior map[string]string, attrs map[string]string, apply bool) map[string]string {
+func normalizeFlatmapContainers(prior map[string]string, attrs map[string]string) map[string]string {
 	isCount := regexp.MustCompile(`.\.[%#]$`).MatchString
 
 	// While we can't determine if the value was actually computed here, we will
@@ -1009,11 +1010,6 @@ func normalizeFlatmapContainers(prior map[string]string, attrs map[string]string
 		if isCount(k) && (v == "0" || v == hcl2shim.UnknownVariableValue) {
 			zeros[k] = true
 		}
-
-		// fixup any 1->0 conversions that happened during Apply
-		if apply && isCount(k) && v == "1" && attrs[k] == "0" {
-			attrs[k] = "1"
-		}
 	}
 
 	for k, v := range attrs {
@@ -1291,7 +1287,7 @@ func normalizeNullValues(dst, src cty.Value, plan bool) cty.Value {
 		// If the dst is nil, and the src is known, then we lost an empty value
 		// so take the original.
 		if dst.IsNull() {
-			if src.IsWhollyKnown() && !plan {
+			if src.IsWhollyKnown() && src.LengthInt() == 0 && !plan {
 				return src
 			}
 			return dst

helper/plugin/grpc_provider_test.go

@@ -716,7 +716,7 @@ func TestNormalizeFlatmapContainers(t *testing.T) {
 		},
 	} {
 		t.Run(strconv.Itoa(i), func(t *testing.T) {
-			got := normalizeFlatmapContainers(tc.prior, tc.attrs, false)
+			got := normalizeFlatmapContainers(tc.prior, tc.attrs)
 			if !reflect.DeepEqual(tc.expect, got) {
 				t.Fatalf("expected:\n%#v\ngot:\n%#v\n", tc.expect, got)
 			}