Merge pull request #1615 from hashicorp/b-computed-map-diff-mitchellh

helper/schema: empty map in state should not compute map

helper/schema/field_reader_config.go

@@ -105,34 +105,67 @@ func (r *ConfigFieldReader) readField(
 }
 
 func (r *ConfigFieldReader) readMap(k string) (FieldReadResult, error) {
-	mraw, ok := r.Config.Get(k)
+	// We want both the raw value and the interpolated. We use the interpolated
+	// to store actual values and we use the raw one to check for
+	// computed keys.
+	mraw, ok := r.Config.GetRaw(k)
 	if !ok {
 		return FieldReadResult{}, nil
 	}
 
 	result := make(map[string]interface{})
+	computed := false
 	switch m := mraw.(type) {
 	case []interface{}:
-		for _, innerRaw := range m {
-			for k, v := range innerRaw.(map[string]interface{}) {
-				result[k] = v
+		for i, innerRaw := range m {
+			for ik, _ := range innerRaw.(map[string]interface{}) {
+				key := fmt.Sprintf("%s.%d.%s", k, i, ik)
+				if r.Config.IsComputed(key) {
+					computed = true
+					break
+				}
+
+				v, _ := r.Config.Get(key)
+				result[ik] = v
 			}
 		}
 	case []map[string]interface{}:
-		for _, innerRaw := range m {
-			for k, v := range innerRaw {
-				result[k] = v
+		for i, innerRaw := range m {
+			for ik, _ := range innerRaw {
+				key := fmt.Sprintf("%s.%d.%s", k, i, ik)
+				if r.Config.IsComputed(key) {
+					computed = true
+					break
+				}
+
+				v, _ := r.Config.Get(key)
+				result[ik] = v
 			}
 		}
 	case map[string]interface{}:
-		result = m
+		for ik, _ := range m {
+			key := fmt.Sprintf("%s.%s", k, ik)
+			if r.Config.IsComputed(key) {
+				computed = true
+				break
+			}
+
+			v, _ := r.Config.Get(key)
+			result[ik] = v
+		}
 	default:
 		panic(fmt.Sprintf("unknown type: %#v", mraw))
 	}
 
+	var value interface{}
+	if !computed {
+		value = result
+	}
+
 	return FieldReadResult{
-		Value:  result,
-		Exists: true,
+		Value:    value,
+		Exists:   true,
+		Computed: computed,
 	}, nil
 }
 

helper/schema/field_reader_config_test.go

@@ -135,6 +135,79 @@ func TestConfigFieldReader_DefaultHandling(t *testing.T) {
 	}
 }
 
+func TestConfigFieldReader_ComputedMap(t *testing.T) {
+	schema := map[string]*Schema{
+		"map": &Schema{
+			Type:     TypeMap,
+			Computed: true,
+		},
+	}
+
+	cases := map[string]struct {
+		Addr   []string
+		Result FieldReadResult
+		Config *terraform.ResourceConfig
+		Err    bool
+	}{
+		"set, normal": {
+			[]string{"map"},
+			FieldReadResult{
+				Value: map[string]interface{}{
+					"foo": "bar",
+				},
+				Exists:   true,
+				Computed: false,
+			},
+			testConfig(t, map[string]interface{}{
+				"map": map[string]interface{}{
+					"foo": "bar",
+				},
+			}),
+			false,
+		},
+
+		"computed element": {
+			[]string{"map"},
+			FieldReadResult{
+				Exists:   true,
+				Computed: true,
+			},
+			testConfigInterpolate(t, map[string]interface{}{
+				"map": map[string]interface{}{
+					"foo": "${var.foo}",
+				},
+			}, map[string]ast.Variable{
+				"var.foo": ast.Variable{
+					Value: config.UnknownVariableValue,
+					Type:  ast.TypeString,
+				},
+			}),
+			false,
+		},
+	}
+
+	for name, tc := range cases {
+		r := &ConfigFieldReader{
+			Schema: schema,
+			Config: tc.Config,
+		}
+		out, err := r.ReadField(tc.Addr)
+		if (err != nil) != tc.Err {
+			t.Fatalf("%s: err: %s", name, err)
+		}
+		if s, ok := out.Value.(*Set); ok {
+			// If it is a set, convert to the raw map
+			out.Value = s.m
+			if len(s.m) == 0 {
+				out.Value = nil
+			}
+		}
+		if !reflect.DeepEqual(tc.Result, out) {
+			t.Fatalf("%s: bad: %#v", name, out)
+		}
+	}
+}
+
 func TestConfigFieldReader_ComputedSet(t *testing.T) {
 	schema := map[string]*Schema{
 		"strSet": &Schema{

helper/schema/field_reader_map.go

@@ -56,10 +56,11 @@ func (r *MapFieldReader) readMap(k string) (FieldReadResult, error) {
 	prefix := k + "."
 	r.Map.Range(func(k, v string) bool {
 		if strings.HasPrefix(k, prefix) {
+			resultSet = true
+
 			key := k[len(prefix):]
 			if key != "#" {
 				result[key] = v
-				resultSet = true
 			}
 		}
 

helper/schema/field_reader_map_test.go

@@ -49,11 +49,14 @@ func TestMapFieldReader(t *testing.T) {
 func TestMapFieldReader_extra(t *testing.T) {
 	r := &MapFieldReader{
 		Schema: map[string]*Schema{
-			"mapDel": &Schema{Type: TypeMap},
+			"mapDel":   &Schema{Type: TypeMap},
+			"mapEmpty": &Schema{Type: TypeMap},
 		},
 
 		Map: BasicMapReader(map[string]string{
 			"mapDel": "",
+
+			"mapEmpty.#": "0",
 		}),
 	}
 
@@ -71,6 +74,14 @@ func TestMapFieldReader_extra(t *testing.T) {
 			false,
 			false,
 		},
+
+		"mapEmpty": {
+			[]string{"mapEmpty"},
+			map[string]interface{}{},
+			true,
+			false,
+			false,
+		},
 	}
 
 	for name, tc := range cases {

helper/schema/schema.go

@@ -626,7 +626,7 @@ func (m schemaMap) diffMap(
 
 	// First get all the values from the state
 	var stateMap, configMap map[string]string
-	o, n, _, _ := d.diffChange(k)
+	o, n, _, nComputed := d.diffChange(k)
 	if err := mapstructure.WeakDecode(o, &stateMap); err != nil {
 		return fmt.Errorf("%s: %s", k, err)
 	}
@@ -634,28 +634,40 @@ func (m schemaMap) diffMap(
 		return fmt.Errorf("%s: %s", k, err)
 	}
 
+	// Keep track of whether the state _exists_ at all prior to clearing it
+	stateExists := o != nil
+
 	// Delete any count values, since we don't use those
 	delete(configMap, "#")
 	delete(stateMap, "#")
 
-	// Check if the number of elements has changed. If we're computing
-	// a list and there isn't a config, then it hasn't changed.
+	// Check if the number of elements has changed.
 	oldLen, newLen := len(stateMap), len(configMap)
 	changed := oldLen != newLen
 	if oldLen != 0 && newLen == 0 && schema.Computed {
 		changed = false
 	}
-	computed := oldLen == 0 && newLen == 0 && schema.Computed
-	if changed || computed {
+
+	// It is computed if we have no old value, no new value, the schema
+	// says it is computed, and it didn't exist in the state before. The
+	// last point means: if it existed in the state, even empty, then it
+	// has already been computed.
+	computed := oldLen == 0 && newLen == 0 && schema.Computed && !stateExists
+
+	// If the count has changed or we're computed, then add a diff for the
+	// count. "nComputed" means that the new value _contains_ a value that
+	// is computed. We don't do granular diffs for this yet, so we mark the
+	// whole map as computed.
+	if changed || computed || nComputed {
 		countSchema := &Schema{
 			Type:     TypeInt,
-			Computed: schema.Computed,
+			Computed: schema.Computed || nComputed,
 			ForceNew: schema.ForceNew,
 		}
 
 		oldStr := strconv.FormatInt(int64(oldLen), 10)
 		newStr := ""
-		if !computed {
+		if !computed && !nComputed {
 			newStr = strconv.FormatInt(int64(newLen), 10)
 		} else {
 			oldStr = ""

helper/schema/schema_test.go

@@ -2201,6 +2201,29 @@ func TestSchemaMap_Diff(t *testing.T) {
 
 			Err: false,
 		},
+
+		// #57 - Computed map without config that's known to be empty does not
+		//       generate diff
+		{
+			Schema: map[string]*Schema{
+				"tags": &Schema{
+					Type:     TypeMap,
+					Computed: true,
+				},
+			},
+
+			Config: nil,
+
+			State: &terraform.InstanceState{
+				Attributes: map[string]string{
+					"tags.#": "0",
+				},
+			},
+
+			Diff: nil,
+
+			Err: false,
+		},
 	}
 
 	for i, tc := range cases {

terraform/resource.go

@@ -134,6 +134,15 @@ func (c *ResourceConfig) Get(k string) (interface{}, bool) {
 	return c.get(k, c.Raw)
 }
 
+// GetRaw looks up a configuration value by key and returns the value,
+// from the raw, uninterpolated config.
+//
+// The second return value is true if the get was successful. Get will
+// not succeed if the value is being computed.
+func (c *ResourceConfig) GetRaw(k string) (interface{}, bool) {
+	return c.get(k, c.Raw)
+}
+
 // IsComputed returns whether the given key is computed or not.
 func (c *ResourceConfig) IsComputed(k string) bool {
 	_, ok := c.get(k, c.Config)