Merge pull request #16957 from hashicorp/jbardin/computed-diff

Restore ability for an empty string to be considered unset in a computed value

helper/schema/resource_data.go

@@ -445,7 +445,7 @@ func (d *ResourceData) init() {
 }
 
 func (d *ResourceData) diffChange(
-	k string) (interface{}, interface{}, bool, bool) {
+	k string) (interface{}, interface{}, bool, bool, bool) {
 	// Get the change between the state and the config.
 	o, n := d.getChange(k, getSourceState, getSourceConfig|getSourceExact)
 	if !o.Exists {
@@ -456,7 +456,7 @@ func (d *ResourceData) diffChange(
 	}
 
 	// Return the old, new, and whether there is a change
-	return o.Value, n.Value, !reflect.DeepEqual(o.Value, n.Value), n.Computed
+	return o.Value, n.Value, !reflect.DeepEqual(o.Value, n.Value), n.Computed, false
 }
 
 func (d *ResourceData) getChange(

helper/schema/resource_diff.go

@@ -236,8 +236,8 @@ func (d *ResourceDiff) clear(key string) error {
 
 // diffChange helps to implement resourceDiffer and derives its change values
 // from ResourceDiff's own change data, in addition to existing diff, config, and state.
-func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, bool) {
+func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, bool, bool) {
-	old, new := d.getChange(key)
+	old, new, customized := d.getChange(key)
 
 	if !old.Exists {
 		old.Value = nil
@@ -246,7 +246,7 @@ func (d *ResourceDiff) diffChange(key string) (interface{}, interface{}, bool, b
 		new.Value = nil
 	}
 
-	return old.Value, new.Value, !reflect.DeepEqual(old.Value, new.Value), new.Computed
+	return old.Value, new.Value, !reflect.DeepEqual(old.Value, new.Value), new.Computed, customized
 }
 
 // SetNew is used to set a new diff value for the mentioned key. The value must
@@ -327,7 +327,7 @@ func (d *ResourceDiff) Get(key string) interface{} {
 // results from the exact levels for the new diff, then from state and diff as
 // per normal.
 func (d *ResourceDiff) GetChange(key string) (interface{}, interface{}) {
-	old, new := d.getChange(key)
+	old, new, _ := d.getChange(key)
 	return old.Value, new.Value
 }
 
@@ -387,18 +387,17 @@ func (d *ResourceDiff) Id() string {
 // This implementation differs from ResourceData's in the way that we first get
 // results from the exact levels for the new diff, then from state and diff as
 // per normal.
-func (d *ResourceDiff) getChange(key string) (getResult, getResult) {
+func (d *ResourceDiff) getChange(key string) (getResult, getResult, bool) {
 	old := d.get(strings.Split(key, "."), "state")
 	var new getResult
 	for p := range d.updatedKeys {
 		if childAddrOf(key, p) {
 			new = d.getExact(strings.Split(key, "."), "newDiff")
-			goto done
+			return old, new, true
 		}
 	}
 	new = d.get(strings.Split(key, "."), "newDiff")
-done:
+	return old, new, false
-	return old, new
 }
 
 // get performs the appropriate multi-level reader logic for ResourceDiff,

helper/schema/schema.go

@@ -296,8 +296,7 @@ func (s *Schema) ZeroValue() interface{} {
 	}
 }
 
-func (s *Schema) finalizeDiff(
+func (s *Schema) finalizeDiff(d *terraform.ResourceAttrDiff, customized bool) *terraform.ResourceAttrDiff {
-	d *terraform.ResourceAttrDiff) *terraform.ResourceAttrDiff {
 	if d == nil {
 		return d
 	}
@@ -337,14 +336,21 @@ func (s *Schema) finalizeDiff(
 		return d
 	}
 
-	if s.Computed && !d.NewComputed {
+	if s.Computed {
+		// FIXME: This is where the customized bool from getChange finally
+		//        comes into play.  It allows the previously incorrect behavior
+		//        of an empty string being used as "unset" when the value is
+		//        computed. This should be removed once we can properly
+		//        represent an unset/nil value from the configuration.
+		if !customized {
 			if d.Old != "" && d.New == "" {
 				// This is a computed value with an old value set already,
 				// just let it go.
 				return nil
 			}
+		}
 
-		if d.New == "" {
+		if d.New == "" && !d.NewComputed {
 			// Computed attribute without a new value set
 			d.NewComputed = true
 		}
@@ -744,7 +750,7 @@ func isValidFieldName(name string) bool {
 // This helps facilitate diff logic for both ResourceData and ResoureDiff with
 // minimal divergence in code.
 type resourceDiffer interface {
-	diffChange(string) (interface{}, interface{}, bool, bool)
+	diffChange(string) (interface{}, interface{}, bool, bool, bool)
 	Get(string) interface{}
 	GetChange(string) (interface{}, interface{})
 	GetOk(string) (interface{}, bool)
@@ -797,7 +803,7 @@ func (m schemaMap) diffList(
 	diff *terraform.InstanceDiff,
 	d resourceDiffer,
 	all bool) error {
-	o, n, _, computedList := d.diffChange(k)
+	o, n, _, computedList, customized := d.diffChange(k)
 	if computedList {
 		n = nil
 	}
@@ -864,10 +870,13 @@ func (m schemaMap) diffList(
 			oldStr = ""
 		}
 
-		diff.Attributes[k+".#"] = countSchema.finalizeDiff(&terraform.ResourceAttrDiff{
+		diff.Attributes[k+".#"] = countSchema.finalizeDiff(
+			&terraform.ResourceAttrDiff{
 				Old: oldStr,
 				New: newStr,
-		})
+			},
+			customized,
+		)
 	}
 
 	// Figure out the maximum
@@ -920,7 +929,7 @@ func (m schemaMap) diffMap(
 
 	// First get all the values from the state
 	var stateMap, configMap map[string]string
-	o, n, _, nComputed := d.diffChange(k)
+	o, n, _, nComputed, customized := d.diffChange(k)
 	if err := mapstructure.WeakDecode(o, &stateMap); err != nil {
 		return fmt.Errorf("%s: %s", k, err)
 	}
@@ -972,6 +981,7 @@ func (m schemaMap) diffMap(
 				Old: oldStr,
 				New: newStr,
 			},
+			customized,
 		)
 	}
 
@@ -989,16 +999,22 @@ func (m schemaMap) diffMap(
 			continue
 		}
 
-		diff.Attributes[prefix+k] = schema.finalizeDiff(&terraform.ResourceAttrDiff{
+		diff.Attributes[prefix+k] = schema.finalizeDiff(
+			&terraform.ResourceAttrDiff{
 				Old: old,
 				New: v,
-		})
+			},
+			customized,
+		)
 	}
 	for k, v := range stateMap {
-		diff.Attributes[prefix+k] = schema.finalizeDiff(&terraform.ResourceAttrDiff{
+		diff.Attributes[prefix+k] = schema.finalizeDiff(
+			&terraform.ResourceAttrDiff{
 				Old:        v,
 				NewRemoved: true,
-		})
+			},
+			customized,
+		)
 	}
 
 	return nil
@@ -1011,7 +1027,7 @@ func (m schemaMap) diffSet(
 	d resourceDiffer,
 	all bool) error {
 
-	o, n, _, computedSet := d.diffChange(k)
+	o, n, _, computedSet, customized := d.diffChange(k)
 	if computedSet {
 		n = nil
 	}
@@ -1070,20 +1086,26 @@ func (m schemaMap) diffSet(
 			countStr = ""
 		}
 
-		diff.Attributes[k+".#"] = countSchema.finalizeDiff(&terraform.ResourceAttrDiff{
+		diff.Attributes[k+".#"] = countSchema.finalizeDiff(
+			&terraform.ResourceAttrDiff{
 				Old:         countStr,
 				NewComputed: true,
-		})
+			},
+			customized,
+		)
 		return nil
 	}
 
 	// If the counts are not the same, then record that diff
 	changed := oldLen != newLen
 	if changed || all {
-		diff.Attributes[k+".#"] = countSchema.finalizeDiff(&terraform.ResourceAttrDiff{
+		diff.Attributes[k+".#"] = countSchema.finalizeDiff(
+			&terraform.ResourceAttrDiff{
 				Old: oldStr,
 				New: newStr,
-		})
+			},
+			customized,
+		)
 	}
 
 	// Build the list of codes that will make up our set. This is the
@@ -1133,7 +1155,7 @@ func (m schemaMap) diffString(
 	all bool) error {
 	var originalN interface{}
 	var os, ns string
-	o, n, _, computed := d.diffChange(k)
+	o, n, _, computed, customized := d.diffChange(k)
 	if schema.StateFunc != nil && n != nil {
 		originalN = n
 		n = schema.StateFunc(n)
@@ -1170,13 +1192,16 @@ func (m schemaMap) diffString(
 		return nil
 	}
 
-	diff.Attributes[k] = schema.finalizeDiff(&terraform.ResourceAttrDiff{
+	diff.Attributes[k] = schema.finalizeDiff(
+		&terraform.ResourceAttrDiff{
 			Old:         os,
 			New:         ns,
 			NewExtra:    originalN,
 			NewRemoved:  removed,
 			NewComputed: computed,
-	})
+		},
+		customized,
+	)
 
 	return nil
 }

helper/schema/schema_test.go

@@ -3110,6 +3110,38 @@ func TestSchemaMap_Diff(t *testing.T) {
 
 			Err: true,
 		},
+
+		// A lot of resources currently depended on using the empty string as a
+		// nil/unset value.
+		// FIXME: We want this to eventually produce a diff, since there
+		// technically is a new value in the config.
+		{
+			Name: "optional, computed, empty string",
+			Schema: map[string]*Schema{
+				"attr": &Schema{
+					Type:     TypeString,
+					Optional: true,
+					Computed: true,
+				},
+			},
+
+			State: &terraform.InstanceState{
+				Attributes: map[string]string{
+					"attr": "bar",
+				},
+			},
+
+			// this does necessarily depend on an interpolated value, but this
+			// is often how it comes about in a configuration, otherwise the
+			// value would be unset.
+			Config: map[string]interface{}{
+				"attr": "${var.foo}",
+			},
+
+			ConfigVariables: map[string]ast.Variable{
+				"var.foo": interfaceToVariableSwallowError(""),
+			},
+		},
 	}
 
 	for i, tc := range cases {