Merge pull request #20158 from hashicorp/jbardin/list-block

Improve list block handling in diff application

builtin/providers/test/resource_list_test.go

@@ -15,6 +15,27 @@ func TestResourceList_changed(t *testing.T) {
 		Steps: []resource.TestStep{
 			resource.TestStep{
 				Config: strings.TrimSpace(`
+resource "test_resource_list" "foo" {
+	list_block {
+		string = "a"
+		int = 1
+	}
+}
+				`),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(
+						"test_resource_list.foo", "list_block.#", "1",
+					),
+					resource.TestCheckResourceAttr(
+						"test_resource_list.foo", "list_block.0.string", "a",
+					),
+					resource.TestCheckResourceAttr(
+						"test_resource_list.foo", "list_block.0.int", "1",
+					),
+				),
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
 resource "test_resource_list" "foo" {
 	list_block {
 		string = "a"

builtin/providers/test/resource_nested_set_test.go

@@ -59,16 +59,6 @@ resource "test_resource_nested_set" "foo" {
 
 // the empty type_list must be passed to the provider with 1 nil element
 func TestResourceNestedSet_emptyBlock(t *testing.T) {
-	checkFunc := func(s *terraform.State) error {
-		root := s.ModuleByPath(addrs.RootModuleInstance)
-		res := root.Resources["test_resource_nested_set.foo"]
-		for k, v := range res.Primary.Attributes {
-			if strings.HasPrefix(k, "type_list") && v != "1" {
-				return fmt.Errorf("unexpected set value: %s:%s", k, v)
-			}
-		}
-		return nil
-	}
 	resource.UnitTest(t, resource.TestCase{
 		Providers:    testAccProviders,
 		CheckDestroy: testAccCheckResourceDestroy,
@@ -80,7 +70,9 @@ resource "test_resource_nested_set" "foo" {
 	}
 }
 				`),
-				Check: checkFunc,
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr("test_resource_nested_set.foo", "type_list.#", "1"),
+				),
 			},
 		},
 	})

helper/plugin/grpc_provider.go

@@ -713,15 +713,25 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A
 		}
 	}
 
-	// strip out non-diffs
-	for k, v := range diff.Attributes {
-		if v.New == v.Old && !v.NewComputed && v.NewExtra == "" {
+	if private != nil {
+		diff.Meta = private
+	}
+
+	// We need to turn off any RequiresNew. There could be attributes
+	// without changes in here inserted by helper/schema, but if they have
+	// RequiresNew then the state will will be dropped from the ResourceData.
+	for k := range diff.Attributes {
+		diff.Attributes[k].RequiresNew = false
+	}
+
+	// check that any "removed" attributes actually exist in the prior state, or
+	// helper/schema will confuse itself
+	for k, d := range diff.Attributes {
+		if d.NewRemoved {
+			if _, ok := priorState.Attributes[k]; !ok {
 				delete(diff.Attributes, k)
 			}
 		}
-
-	if private != nil {
-		diff.Meta = private
 	}
 
 	newInstanceState, err := s.provider.Apply(info, priorState, diff)

terraform/diff.go

@@ -4,6 +4,7 @@ import (
 	"bufio"
 	"bytes"
 	"fmt"
+	"log"
 	"reflect"
 	"regexp"
 	"sort"
@@ -494,11 +495,12 @@ func (d *InstanceDiff) applyBlockDiff(path []string, attrs map[string]string, sc
 		// we need to find the set of all keys that traverse this block
 		candidateKeys := map[string]bool{}
 		blockKey := blockPrefix + n + "."
+		localBlockPrefix := localPrefix + n + "."
 
 		// we can only trust the diff for sets, since the path changes, so don't
 		// count existing values as candidate keys. If it turns out we're
-		// keeping the attributes, we will check catch it down below with
-		// "keepBlock" after we check the set count.
+		// keeping the attributes, we will catch it down below with "keepBlock"
+		// after we check the set count.
 		if block.Nesting != configschema.NestingSet {
 			for k := range attrs {
 				if strings.HasPrefix(k, blockKey) {
@@ -570,7 +572,7 @@ func (d *InstanceDiff) applyBlockDiff(path []string, attrs map[string]string, sc
 			}
 
 			for attr, v := range newAttrs {
-				result[localPrefix+n+"."+attr] = v
+				result[localBlockPrefix+attr] = v
 			}
 		}
 
@@ -589,24 +591,51 @@ func (d *InstanceDiff) applyBlockDiff(path []string, attrs map[string]string, sc
 				if strings.HasPrefix(k, blockKey) {
 					// we need the key relative to this block, so remove the
 					// entire prefix, then re-insert the block name.
-					localKey := n + "." + k[len(blockKey):]
-
+					localKey := localBlockPrefix + k[len(blockKey):]
 					result[localKey] = v
 				}
 			}
 		}
 
-		if countDiff, ok := d.Attributes[strings.Join(append(path, n, ".#"), ".")]; ok {
-
+		if countDiff, ok := d.Attributes[strings.Join(append(path, n, "#"), ".")]; ok {
 			if countDiff.NewComputed {
-				result[localPrefix+n+".#"] = hcl2shim.UnknownVariableValue
+				result[localBlockPrefix+"#"] = hcl2shim.UnknownVariableValue
 			} else {
-				result[localPrefix+n+".#"] = countDiff.New
-			}
-		} else {
-			result[localPrefix+n+".#"] = countFlatmapContainerValues(localPrefix+n+".#", result)
+				result[localBlockPrefix+"#"] = countDiff.New
+
+				// While sets are complete, list are not, and we may not have all the
+				// information to track removals. If the list was truncated, we need to
+				// remove the extra items from the result.
+				if block.Nesting == configschema.NestingList &&
+					countDiff.New != "" && countDiff.New != hcl2shim.UnknownVariableValue {
+					length, _ := strconv.Atoi(countDiff.New)
+					for k := range result {
+						if !strings.HasPrefix(k, localBlockPrefix) {
+							continue
 						}
 
+						index := k[len(localBlockPrefix):]
+						nextDot := strings.Index(index, ".")
+						if nextDot < 1 {
+							continue
+						}
+						index = index[:nextDot]
+						i, err := strconv.Atoi(index)
+						if err != nil {
+							// this shouldn't happen since we added these
+							// ourself, but make note of it just in case.
+							log.Printf("[ERROR] bas list index in %q: %s", k, err)
+							continue
+						}
+						if i >= length {
+							delete(result, k)
+						}
+					}
+				}
+			}
+		} else {
+			result[localBlockPrefix+"#"] = countFlatmapContainerValues(localBlockPrefix+"#", result)
+		}
 	}
 
 	return result, nil