Merge pull request #7393 from TimeIncOSS/b-schema-set-in-list

helper/schema: Make nested Set(s) in List(s) work

helper/schema/field_reader.go

@@ -130,60 +130,6 @@ func addrToSchema(addr []string, schemaMap map[string]*Schema) []*Schema {
 	return result
 }
 
-// readListField is a generic method for reading a list field out of a
-// a FieldReader. It does this based on the assumption that there is a key
-// "foo.#" for a list "foo" and that the indexes are "foo.0", "foo.1", etc.
-// after that point.
-func readListField(
-	r FieldReader, addr []string, schema *Schema) (FieldReadResult, error) {
-	addrPadded := make([]string, len(addr)+1)
-	copy(addrPadded, addr)
-	addrPadded[len(addrPadded)-1] = "#"
-
-	// Get the number of elements in the list
-	countResult, err := r.ReadField(addrPadded)
-	if err != nil {
-		return FieldReadResult{}, err
-	}
-	if !countResult.Exists {
-		// No count, means we have no list
-		countResult.Value = 0
-	}
-
-	// If we have an empty list, then return an empty list
-	if countResult.Computed || countResult.Value.(int) == 0 {
-		return FieldReadResult{
-			Value:    []interface{}{},
-			Exists:   countResult.Exists,
-			Computed: countResult.Computed,
-		}, nil
-	}
-
-	// Go through each count, and get the item value out of it
-	result := make([]interface{}, countResult.Value.(int))
-	for i, _ := range result {
-		is := strconv.FormatInt(int64(i), 10)
-		addrPadded[len(addrPadded)-1] = is
-		rawResult, err := r.ReadField(addrPadded)
-		if err != nil {
-			return FieldReadResult{}, err
-		}
-		if !rawResult.Exists {
-			// This should never happen, because by the time the data
-			// gets to the FieldReaders, all the defaults should be set by
-			// Schema.
-			rawResult.Value = nil
-		}
-
-		result[i] = rawResult.Value
-	}
-
-	return FieldReadResult{
-		Value:  result,
-		Exists: true,
-	}, nil
-}
-
 // readObjectField is a generic method for reading objects out of FieldReaders
 // based on the assumption that building an address of []string{k, FIELD}
 // will result in the proper field data.

helper/schema/field_reader_config.go

@@ -83,7 +83,7 @@ func (r *ConfigFieldReader) readField(
 	case TypeBool, TypeFloat, TypeInt, TypeString:
 		return r.readPrimitive(k, schema)
 	case TypeList:
-		return readListField(&nestedConfigFieldReader{r}, address, schema)
+		return r.readList(address, schema)
 	case TypeMap:
 		return r.readMap(k)
 	case TypeSet:
@@ -216,13 +216,65 @@ func (r *ConfigFieldReader) readPrimitive(
 	}, nil
 }
 
+func (r *ConfigFieldReader) readList(
+	address []string, schema *Schema) (FieldReadResult, error) {
+
+	addrPadded := make([]string, len(address)+1)
+	copy(addrPadded, address)
+
+	// Get the number of elements in the list
+	addrPadded[len(addrPadded)-1] = "#"
+	countResult, err := r.readPrimitive(
+		strings.Join(addrPadded, "."), &Schema{Type: TypeInt})
+	if err != nil {
+		return FieldReadResult{}, err
+	}
+	if !countResult.Exists {
+		// No count, means we have no list
+		countResult.Value = 0
+	}
+
+	// If we have an empty list, then return an empty list
+	if countResult.Computed || countResult.Value.(int) == 0 {
+		return FieldReadResult{
+			Value:    []interface{}{},
+			Exists:   countResult.Exists,
+			Computed: countResult.Computed,
+		}, nil
+	}
+
+	// Go through each count, and get the item value out of it
+	result := make([]interface{}, countResult.Value.(int))
+	for i, _ := range result {
+		idx := strconv.Itoa(i)
+		addrPadded[len(addrPadded)-1] = idx
+		rawResult, err := r.readField(addrPadded, true)
+		if err != nil {
+			return FieldReadResult{}, err
+		}
+		if !rawResult.Exists {
+			// This should never happen, because by the time the data
+			// gets to the FieldReaders, all the defaults should be set by
+			// Schema.
+			panic("missing field in list: " + strings.Join(addrPadded, "."))
+		}
+
+		result[i] = rawResult.Value
+	}
+
+	return FieldReadResult{
+		Value:  result,
+		Exists: true,
+	}, nil
+}
+
 func (r *ConfigFieldReader) readSet(
 	address []string, schema *Schema) (FieldReadResult, error) {
 	indexMap := make(map[string]int)
 	// Create the set that will be our result
 	set := schema.ZeroValue().(*Set)
 
-	raw, err := readListField(&nestedConfigFieldReader{r}, address, schema)
+	raw, err := r.readList(address, schema)
 	if err != nil {
 		return FieldReadResult{}, err
 	}

helper/schema/field_reader_diff.go

@@ -2,6 +2,8 @@ package schema
 
 import (
 	"fmt"
+	"sort"
+	"strconv"
 	"strings"
 
 	"github.com/hashicorp/terraform/terraform"
@@ -12,8 +14,8 @@ import (
 //
 // It also requires access to a Reader that reads fields from the structure
 // that the diff was derived from. This is usually the state. This is required
-// because a diff on its own doesn't have complete data about full objects
-// such as maps.
+// because a diff on its own doesn't have complete data about non-primitive
+// objects such as maps, lists and sets.
 //
 // The Source MUST be the data that the diff was derived from. If it isn't,
 // the behavior of this struct is undefined.
@@ -42,7 +44,7 @@ func (r *DiffFieldReader) ReadField(address []string) (FieldReadResult, error) {
 	case TypeBool, TypeInt, TypeFloat, TypeString:
 		return r.readPrimitive(address, schema)
 	case TypeList:
-		return readListField(r, address, schema)
+		return r.readList(address, schema)
 	case TypeMap:
 		return r.readMap(address, schema)
 	case TypeSet:
@@ -57,7 +59,7 @@ func (r *DiffFieldReader) ReadField(address []string) (FieldReadResult, error) {
 func (r *DiffFieldReader) readMap(
 	address []string, schema *Schema) (FieldReadResult, error) {
 	result := make(map[string]interface{})
-	resultSet := false
+	exists := false
 
 	// First read the map from the underlying source
 	source, err := r.Source.ReadField(address)
@@ -66,7 +68,7 @@ func (r *DiffFieldReader) readMap(
 	}
 	if source.Exists {
 		result = source.Value.(map[string]interface{})
-		resultSet = true
+		exists = true
 	}
 
 	// Next, read all the elements we have in our diff, and apply
@@ -81,7 +83,7 @@ func (r *DiffFieldReader) readMap(
 			continue
 		}
 
-		resultSet = true
+		exists = true
 
 		k = k[len(prefix):]
 		if v.NewRemoved {
@@ -93,13 +95,13 @@ func (r *DiffFieldReader) readMap(
 	}
 
 	var resultVal interface{}
-	if resultSet {
+	if exists {
 		resultVal = result
 	}
 
 	return FieldReadResult{
 		Value:  resultVal,
-		Exists: resultSet,
+		Exists: exists,
 	}, nil
 }
 
@@ -136,17 +138,160 @@ func (r *DiffFieldReader) readPrimitive(
 	return result, nil
 }
 
+func (r *DiffFieldReader) readList(
+	address []string, schema *Schema) (FieldReadResult, error) {
+	prefix := strings.Join(address, ".") + "."
+
+	addrPadded := make([]string, len(address)+1)
+	copy(addrPadded, address)
+
+	// Get the number of elements in the list
+	addrPadded[len(addrPadded)-1] = "#"
+	countResult, err := r.readPrimitive(addrPadded, &Schema{Type: TypeInt})
+	if err != nil {
+		return FieldReadResult{}, err
+	}
+	if !countResult.Exists {
+		// No count, means we have no list
+		countResult.Value = 0
+	}
+	// If we have an empty list, then return an empty list
+	if countResult.Computed || countResult.Value.(int) == 0 {
+		return FieldReadResult{
+			Value:    []interface{}{},
+			Exists:   countResult.Exists,
+			Computed: countResult.Computed,
+		}, nil
+	}
+
+	// Bail out if diff doesn't contain the given field at all
+	// This has to be a separate loop because we're only
+	// iterating over raw list items (list.idx).
+	// Other fields (list.idx.*) are left for other read* methods
+	// which can deal with these fields appropriately.
+	diffContainsField := false
+	for k, _ := range r.Diff.Attributes {
+		if strings.HasPrefix(k, address[0]+".") {
+			diffContainsField = true
+		}
+	}
+	if !diffContainsField {
+		return FieldReadResult{
+			Value:  []interface{}{},
+			Exists: false,
+		}, nil
+	}
+
+	// Create the list that will be our result
+	list := []interface{}{}
+
+	// Go through the diff and find all the list items
+	// We are not iterating over the diff directly as some indexes
+	// may be missing and we expect the whole list to be returned.
+	for i := 0; i < countResult.Value.(int); i++ {
+		idx := strconv.Itoa(i)
+		addrString := prefix + idx
+
+		d, ok := r.Diff.Attributes[addrString]
+		if ok && d.NewRemoved {
+			// If the field is being removed, we ignore it
+			continue
+		}
+
+		addrPadded[len(addrPadded)-1] = idx
+		raw, err := r.ReadField(addrPadded)
+		if err != nil {
+			return FieldReadResult{}, err
+		}
+		if !raw.Exists {
+			// This should never happen, because by the time the data
+			// gets to the FieldReaders, all the defaults should be set by
+			// Schema.
+			panic("missing field in set: " + addrString + "." + idx)
+		}
+		list = append(list, raw.Value)
+	}
+
+	// Determine if the list "exists". It exists if there are items or if
+	// the diff explicitly wanted it empty.
+	exists := len(list) > 0
+	if !exists {
+		// We could check if the diff value is "0" here but I think the
+		// existence of "#" on its own is enough to show it existed. This
+		// protects us in the future from the zero value changing from
+		// "0" to "" breaking us (if that were to happen).
+		if _, ok := r.Diff.Attributes[prefix+"#"]; ok {
+			exists = true
+		}
+	}
+
+	return FieldReadResult{
+		Value:  list,
+		Exists: exists,
+	}, nil
+}
+
 func (r *DiffFieldReader) readSet(
 	address []string, schema *Schema) (FieldReadResult, error) {
 	prefix := strings.Join(address, ".") + "."
 
 	// Create the set that will be our result
 	set := schema.ZeroValue().(*Set)
 
+	// Check if we're supposed to remove it
+	v, ok := r.Diff.Attributes[prefix+"#"]
+	if ok && v.New == "0" {
+		// I'm not entirely sure what's the point of
+		// returning empty set w/ Exists: true
+		return FieldReadResult{
+			Value:  set,
+			Exists: true,
+		}, nil
+	}
+
+	// Compose list of all keys (diff + source)
+	var keys []string
+
+	// Add keys from diff
+	diffContainsField := false
+	for k, _ := range r.Diff.Attributes {
+		if strings.HasPrefix(k, address[0]+".") {
+			diffContainsField = true
+		}
+		keys = append(keys, k)
+	}
+	// Bail out if diff doesn't contain the given field at all
+	if !diffContainsField {
+		return FieldReadResult{
+			Value:  set,
+			Exists: false,
+		}, nil
+	}
+	// Add keys from source
+	sourceResult, err := r.Source.ReadField(address)
+	if err == nil && sourceResult.Exists {
+		sourceSet := sourceResult.Value.(*Set)
+		sourceMap := sourceSet.Map()
+
+		for k, _ := range sourceMap {
+			key := prefix + k
+			_, ok := r.Diff.Attributes[key]
+			if !ok {
+				keys = append(keys, key)
+			}
+		}
+	}
+
+	// Keep the order consistent for hashing functions
+	sort.Strings(keys)
+
 	// Go through the map and find all the set items
-	for k, d := range r.Diff.Attributes {
-		if d.NewRemoved {
-			// If the field is removed, we always ignore it
+	// We are not iterating over the diff directly as some indexes
+	// may be missing and we expect the whole set to be returned.
+	for _, k := range keys {
+		d, ok := r.Diff.Attributes[k]
+		if ok && d.NewRemoved {
+			// If the field is being removed, we ignore it
 			continue
 		}
 		if !strings.HasPrefix(k, prefix) {