diff --git a/helper/schema/field_writer_map.go b/helper/schema/field_writer_map.go index 689ed8d1c..814c7ba8e 100644 --- a/helper/schema/field_writer_map.go +++ b/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 diff --git a/helper/schema/field_writer_map_test.go b/helper/schema/field_writer_map_test.go index 5fc308aa7..42ed9c208 100644 --- a/helper/schema/field_writer_map_test.go +++ b/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) + } + } +}