Check output of finalizeDiff before using it.

Previously, we'd assign the result of finalizeDiff to the resource diff
without checking its return. This caused problems because a "finalized"
diff for any given attribute could, in fact, be no diff at all. Which we
represent as `nil`. But some consumers of the resource diff expect every
attribute in the map to be non-`nil`, and so crash on these attributes
that have diff entries but no diffs. See for example
hashicorp/terraform-provider-google#7934, which would crash when a
config had an explicit empty string as the value for a field that a
CustomizeDiff function set to ForceNew. Technically, there was a diff,
but finalizeDiff decided it wasn't a "real" diff, because the SDK still
interprets empty strings as "unset" for computed fields to align with
legacy behavior. But that meant a nil in the resource's map of attribute
diffs, which then was dereferenced when populating the response to
PlanResourceChange. This caused a crash.

This commit fixes that issue by updating all our usages of finalizeDiff
to check for a nil diff _before_ writing it to the resource's map of
attribute diffs. This was easier than tracking down all the usages of a
ResourceAttributeDiff and trying to ensure they were ignoring nil
values.

helper/schema/grpc_provider.go

@@ -796,7 +796,6 @@ func (s *GRPCProviderServer) PlanResourceChange(ctx context.Context, req *tfprot
 	newExtra := map[string]interface{}{}
 
 	for k, v := range diff.Attributes {
-		log.Printf("[TRACE] tpg-7934: copying over attribute %q", k)
 		if v.NewExtra != nil {
 			newExtra[k] = v.NewExtra
 		}

helper/schema/resource_diff.go

@@ -3,7 +3,6 @@ package schema
 import (
 	"errors"
 	"fmt"
-	"log"
 	"reflect"
 	"strings"
 	"sync"
@@ -462,11 +461,9 @@ func (d *ResourceDiff) getChange(key string) (getResult, getResult, bool) {
 	var new getResult
 	for p := range d.updatedKeys {
 		if childAddrOf(key, p) {
-			log.Printf("[TRACE] tpg-7934: key %q is child of parent %q, counts as computed", key, p)
 			new = d.getExact(strings.Split(key, "."), "newDiff")
 			return old, new, true
 		}
-		log.Printf("[TRACE] tpg-7934: key %q is not child of parent %q, does not count as computed", key, p)
 	}
 	new = d.get(strings.Split(key, "."), "newDiff")
 	return old, new, false

helper/schema/schema.go

@@ -360,7 +360,6 @@ func (s *Schema) ZeroValue() interface{} {
 
 func (s *Schema) finalizeDiff(d *terraform.ResourceAttrDiff, customized bool) *terraform.ResourceAttrDiff {
 	if d == nil {
-		log.Println("[TRACE] tpg-7934: returning nil from finalizeDiff because nil was passed in")
 		return d
 	}
 
@@ -409,7 +408,7 @@ func (s *Schema) finalizeDiff(d *terraform.ResourceAttrDiff, customized bool) *t
 			if d.Old != "" && d.New == "" {
 				// This is a computed value with an old value set already,
 				// just let it go.
-				log.Println("[TRACE] tpg-7934: returning nil from finalizeDiff because customized")
+				log.Println("[DEBUG] A computed value with the empty string as the new value and a non-empty old value was found. Interpreting the empty string as \"unset\" to align with legacy behavior.")
 				return nil
 			}
 		}
@@ -1063,13 +1062,18 @@ func (m schemaMap) diffList(
 			oldStr = ""
 		}
 
-		diff.Attributes[k+".#"] = countSchema.finalizeDiff(
+		finalizedAttr := countSchema.finalizeDiff(
 			&terraform.ResourceAttrDiff{
 				Old: oldStr,
 				New: newStr,
 			},
 			customized,
 		)
+		if finalizedAttr != nil {
+			diff.Attributes[k+".#"] = finalizedAttr
+		} else {
+			delete(diff.Attributes, k+".#")
+		}
 	}
 
 	// Figure out the maximum
@@ -1169,13 +1173,18 @@ func (m schemaMap) diffMap(
 			oldStr = ""
 		}
 
-		diff.Attributes[k+".%"] = countSchema.finalizeDiff(
+		finalizedAttr := countSchema.finalizeDiff(
 			&terraform.ResourceAttrDiff{
 				Old: oldStr,
 				New: newStr,
 			},
 			customized,
 		)
+		if finalizedAttr != nil {
+			diff.Attributes[k+".%"] = finalizedAttr
+		} else {
+			delete(diff.Attributes, k+".%")
+		}
 	}
 
 	// If the new map is nil and we're computed, then ignore it.
@@ -1192,22 +1201,28 @@ func (m schemaMap) diffMap(
 			continue
 		}
 
-		diff.Attributes[prefix+k] = schema.finalizeDiff(
+		finalizedAttr := schema.finalizeDiff(
 			&terraform.ResourceAttrDiff{
 				Old: old,
 				New: v,
 			},
 			customized,
 		)
+		if finalizedAttr != nil {
+			diff.Attributes[prefix+k] = finalizedAttr
+		}
 	}
 	for k, v := range stateMap {
-		diff.Attributes[prefix+k] = schema.finalizeDiff(
+		finalizedAttr := schema.finalizeDiff(
 			&terraform.ResourceAttrDiff{
 				Old:        v,
 				NewRemoved: true,
 			},
 			customized,
 		)
+		if finalizedAttr != nil {
+			diff.Attributes[prefix+k] = finalizedAttr
+		}
 	}
 
 	return nil
@@ -1279,26 +1294,32 @@ func (m schemaMap) diffSet(
 			countStr = ""
 		}
 
-		diff.Attributes[k+".#"] = countSchema.finalizeDiff(
+		finalizedAttr := countSchema.finalizeDiff(
 			&terraform.ResourceAttrDiff{
 				Old:         countStr,
 				NewComputed: true,
 			},
 			customized,
 		)
+		if finalizedAttr != nil {
+			diff.Attributes[k+".#"] = finalizedAttr
+		}
 		return nil
 	}
 
 	// If the counts are not the same, then record that diff
 	changed := oldLen != newLen
 	if changed || all {
-		diff.Attributes[k+".#"] = countSchema.finalizeDiff(
+		finalizedAttr := countSchema.finalizeDiff(
 			&terraform.ResourceAttrDiff{
 				Old: oldStr,
 				New: newStr,
 			},
 			customized,
 		)
+		if finalizedAttr != nil {
+			diff.Attributes[k+".#"] = finalizedAttr
+		}
 	}
 
 	// Build the list of codes that will make up our set. This is the
@@ -1385,8 +1406,7 @@ func (m schemaMap) diffString(
 		return nil
 	}
 
-	log.Printf("[TRACE] tpg-7934: setting attribute %q", k)
-	diff.Attributes[k] = schema.finalizeDiff(
+	finalizedAttr := schema.finalizeDiff(
 		&terraform.ResourceAttrDiff{
 			Old:         os,
 			New:         ns,
@@ -1396,6 +1416,9 @@ func (m schemaMap) diffString(
 		},
 		customized,
 	)
+	if finalizedAttr != nil {
+		diff.Attributes[k] = finalizedAttr
+	}
 
 	return nil
 }