Merge pull request #16565 from hashicorp/b-mapwriter-overwrites

helper/schema: Clear existing map/set/list contents before overwriting

helper/schema/field_writer_map.go

@@ -39,6 +39,19 @@ func (w *MapFieldWriter) unsafeWriteField(addr string, value string) {
 	w.result[addr] = value
 }
 
+// clearTree clears a field and any sub-fields of the given address out of the
+// map. This should be used to reset some kind of complex structures (namely
+// sets) before writing to make sure that any conflicting data is removed (for
+// example, if the set was previously written to the writer's layer).
+func (w *MapFieldWriter) clearTree(addr []string) {
+	prefix := strings.Join(addr, ".") + "."
+	for k := range w.result {
+		if strings.HasPrefix(k, prefix) {
+			delete(w.result, k)
+		}
+	}
+}
+
 func (w *MapFieldWriter) WriteField(addr []string, value interface{}) error {
 	w.lock.Lock()
 	defer w.lock.Unlock()
@@ -115,6 +128,14 @@ func (w *MapFieldWriter) setList(
 		return fmt.Errorf("%s: %s", k, err)
 	}
 
+	// Wipe the set from the current writer prior to writing if it exists.
+	// Multiple writes to the same layer is a lot safer for lists than sets due
+	// to the fact that indexes are always deterministic and the length will
+	// always be updated with the current length on the last write, but making
+	// sure we have a clean namespace removes any chance for edge cases to pop up
+	// and ensures that the last write to the set is the correct value.
+	w.clearTree(addr)
+
 	// Set the entire list.
 	var err error
 	for i, elem := range vs {
@@ -162,6 +183,10 @@ func (w *MapFieldWriter) setMap(
 		vs[mk.String()] = mv.Interface()
 	}
 
+	// Wipe this address tree. The contents of the map should always reflect the
+	// last write made to it.
+	w.clearTree(addr)
+
 	// Remove the pure key since we're setting the full map value
 	delete(w.result, k)
 
@@ -308,6 +333,13 @@ func (w *MapFieldWriter) setSet(
 		value = s
 	}
 
+	// Clear any keys that match the set address first. This is necessary because
+	// it's always possible and sometimes may be necessary to write to a certain
+	// writer layer more than once with different set data each time, which will
+	// lead to different keys being inserted, which can lead to determinism
+	// problems when the old data isn't wiped first.
+	w.clearTree(addr)
+
 	for code, elem := range value.(*Set).m {
 		if err := w.set(append(addrCopy, code), elem); err != nil {
 			return err

helper/schema/field_writer_map_test.go

@@ -261,3 +261,278 @@ func TestMapFieldWriter(t *testing.T) {
 		}
 	}
 }
+
+func TestMapFieldWriterCleanSet(t *testing.T) {
+	schema := map[string]*Schema{
+		"setDeep": &Schema{
+			Type: TypeSet,
+			Elem: &Resource{
+				Schema: map[string]*Schema{
+					"index": &Schema{Type: TypeInt},
+					"value": &Schema{Type: TypeString},
+				},
+			},
+			Set: func(a interface{}) int {
+				return a.(map[string]interface{})["index"].(int)
+			},
+		},
+	}
+
+	values := []struct {
+		Addr  []string
+		Value interface{}
+		Out   map[string]string
+	}{
+		{
+			[]string{"setDeep"},
+			[]interface{}{
+				map[string]interface{}{
+					"index": 10,
+					"value": "foo",
+				},
+				map[string]interface{}{
+					"index": 50,
+					"value": "bar",
+				},
+			},
+			map[string]string{
+				"setDeep.#":        "2",
+				"setDeep.10.index": "10",
+				"setDeep.10.value": "foo",
+				"setDeep.50.index": "50",
+				"setDeep.50.value": "bar",
+			},
+		},
+		{
+			[]string{"setDeep"},
+			[]interface{}{
+				map[string]interface{}{
+					"index": 20,
+					"value": "baz",
+				},
+				map[string]interface{}{
+					"index": 60,
+					"value": "qux",
+				},
+			},
+			map[string]string{
+				"setDeep.#":        "2",
+				"setDeep.20.index": "20",
+				"setDeep.20.value": "baz",
+				"setDeep.60.index": "60",
+				"setDeep.60.value": "qux",
+			},
+		},
+		{
+			[]string{"setDeep"},
+			[]interface{}{
+				map[string]interface{}{
+					"index": 30,
+					"value": "one",
+				},
+				map[string]interface{}{
+					"index": 70,
+					"value": "two",
+				},
+			},
+			map[string]string{
+				"setDeep.#":        "2",
+				"setDeep.30.index": "30",
+				"setDeep.30.value": "one",
+				"setDeep.70.index": "70",
+				"setDeep.70.value": "two",
+			},
+		},
+	}
+
+	w := &MapFieldWriter{Schema: schema}
+
+	for n, tc := range values {
+		err := w.WriteField(tc.Addr, tc.Value)
+		if err != nil {
+			t.Fatalf("%d: err: %s", n, err)
+		}
+
+		actual := w.Map()
+		if !reflect.DeepEqual(actual, tc.Out) {
+			t.Fatalf("%d: bad: %#v", n, actual)
+		}
+	}
+}
+
+func TestMapFieldWriterCleanList(t *testing.T) {
+	schema := map[string]*Schema{
+		"listDeep": &Schema{
+			Type: TypeList,
+			Elem: &Resource{
+				Schema: map[string]*Schema{
+					"thing1": &Schema{Type: TypeString},
+					"thing2": &Schema{Type: TypeString},
+				},
+			},
+		},
+	}
+
+	values := []struct {
+		Addr  []string
+		Value interface{}
+		Out   map[string]string
+	}{
+		{
+			// Base list
+			[]string{"listDeep"},
+			[]interface{}{
+				map[string]interface{}{
+					"thing1": "a",
+					"thing2": "b",
+				},
+				map[string]interface{}{
+					"thing1": "c",
+					"thing2": "d",
+				},
+				map[string]interface{}{
+					"thing1": "e",
+					"thing2": "f",
+				},
+				map[string]interface{}{
+					"thing1": "g",
+					"thing2": "h",
+				},
+			},
+			map[string]string{
+				"listDeep.#":        "4",
+				"listDeep.0.thing1": "a",
+				"listDeep.0.thing2": "b",
+				"listDeep.1.thing1": "c",
+				"listDeep.1.thing2": "d",
+				"listDeep.2.thing1": "e",
+				"listDeep.2.thing2": "f",
+				"listDeep.3.thing1": "g",
+				"listDeep.3.thing2": "h",
+			},
+		},
+		{
+			// Remove an element
+			[]string{"listDeep"},
+			[]interface{}{
+				map[string]interface{}{
+					"thing1": "a",
+					"thing2": "b",
+				},
+				map[string]interface{}{
+					"thing1": "c",
+					"thing2": "d",
+				},
+				map[string]interface{}{
+					"thing1": "e",
+					"thing2": "f",
+				},
+			},
+			map[string]string{
+				"listDeep.#":        "3",
+				"listDeep.0.thing1": "a",
+				"listDeep.0.thing2": "b",
+				"listDeep.1.thing1": "c",
+				"listDeep.1.thing2": "d",
+				"listDeep.2.thing1": "e",
+				"listDeep.2.thing2": "f",
+			},
+		},
+		{
+			// Rewrite with missing keys. This should normally not be necessary, as
+			// hopefully the writers are writing zero values as necessary, but for
+			// brevity we want to make sure that what exists in the writer is exactly
+			// what the last write looked like coming from the provider.
+			[]string{"listDeep"},
+			[]interface{}{
+				map[string]interface{}{
+					"thing1": "a",
+				},
+				map[string]interface{}{
+					"thing1": "c",
+				},
+				map[string]interface{}{
+					"thing1": "e",
+				},
+			},
+			map[string]string{
+				"listDeep.#":        "3",
+				"listDeep.0.thing1": "a",
+				"listDeep.1.thing1": "c",
+				"listDeep.2.thing1": "e",
+			},
+		},
+	}
+
+	w := &MapFieldWriter{Schema: schema}
+
+	for n, tc := range values {
+		err := w.WriteField(tc.Addr, tc.Value)
+		if err != nil {
+			t.Fatalf("%d: err: %s", n, err)
+		}
+
+		actual := w.Map()
+		if !reflect.DeepEqual(actual, tc.Out) {
+			t.Fatalf("%d: bad: %#v", n, actual)
+		}
+	}
+}
+
+func TestMapFieldWriterCleanMap(t *testing.T) {
+	schema := map[string]*Schema{
+		"map": &Schema{
+			Type: TypeMap,
+		},
+	}
+
+	values := []struct {
+		Value interface{}
+		Out   map[string]string
+	}{
+		{
+			// Base map
+			map[string]interface{}{
+				"thing1": "a",
+				"thing2": "b",
+				"thing3": "c",
+				"thing4": "d",
+			},
+			map[string]string{
+				"map.%":      "4",
+				"map.thing1": "a",
+				"map.thing2": "b",
+				"map.thing3": "c",
+				"map.thing4": "d",
+			},
+		},
+		{
+			// Base map
+			map[string]interface{}{
+				"thing1": "a",
+				"thing2": "b",
+				"thing4": "d",
+			},
+			map[string]string{
+				"map.%":      "3",
+				"map.thing1": "a",
+				"map.thing2": "b",
+				"map.thing4": "d",
+			},
+		},
+	}
+
+	w := &MapFieldWriter{Schema: schema}
+
+	for n, tc := range values {
+		err := w.WriteField([]string{"map"}, tc.Value)
+		if err != nil {
+			t.Fatalf("%d: err: %s", n, err)
+		}
+
+		actual := w.Map()
+		if !reflect.DeepEqual(actual, tc.Out) {
+			t.Fatalf("%d: bad: %#v", n, actual)
+		}
+	}
+}