fix(diffDynamoDbGSI): ignore ordering of non_key_attributes in equali…

…ty check to stop forced reconstruction of GSI [fixes #hashicorp#3828]
shalka · Oct 7, 2019 · bee8f31 · bee8f31
1 parent be01d68
commit bee8f31
Show file tree

Hide file tree

Showing 3 changed files with 87 additions and 3 deletions.
diff --git a/aws/resource_aws_dynamodb_table.go b/aws/resource_aws_dynamodb_table.go
@@ -208,7 +208,7 @@ func resourceAwsDynamoDbTable() *schema.Resource {
 							Required: true,
 						},
 						"non_key_attributes": {
-							Type:     schema.TypeList,
+							Type:     schema.TypeSet,
 							Optional: true,
 							Elem:     &schema.Schema{Type: schema.TypeString},
 						},

diff --git a/aws/resource_aws_dynamodb_table_test.go b/aws/resource_aws_dynamodb_table_test.go
@@ -58,7 +58,7 @@ func TestDiffDynamoDbGSI(t *testing.T) {
 		New             []interface{}
 		ExpectedUpdates []*dynamodb.GlobalSecondaryIndexUpdate
 	}{
-		{ // No-op
+		{ // No-op => no changes
 			Old: []interface{}{
 				map[string]interface{}{
 					"name":            "att1-index",
@@ -79,6 +79,29 @@ func TestDiffDynamoDbGSI(t *testing.T) {
 			},
 			ExpectedUpdates: []*dynamodb.GlobalSecondaryIndexUpdate{},
 		},
+		{ // No-op => ignore ordering of non_key_attributes
+			Old: []interface{}{
+				map[string]interface{}{
+					"name":               "att1-index",
+					"hash_key":           "att1",
+					"write_capacity":     10,
+					"read_capacity":      10,
+					"projection_type":    "INCLUDE",
+					"non_key_attributes": []interface{}{"attr3", "attr1", "attr2"},
+				},
+			},
+			New: []interface{}{
+				map[string]interface{}{
+					"name":               "att1-index",
+					"hash_key":           "att1",
+					"write_capacity":     10,
+					"read_capacity":      10,
+					"projection_type":    "INCLUDE",
+					"non_key_attributes": []interface{}{"attr1", "attr2", "attr3"},
+				},
+			},
+			ExpectedUpdates: []*dynamodb.GlobalSecondaryIndexUpdate{},
+		},
 
 		{ // Creation
 			Old: []interface{}{

diff --git a/aws/structure.go b/aws/structure.go
@@ -4165,17 +4165,31 @@ func diffDynamoDbGSI(oldGsi, newGsi []interface{}, billingMode string) (ops []*d
 			newWriteCapacity, newReadCapacity := newMap["write_capacity"].(int), newMap["read_capacity"].(int)
 			capacityChanged := (oldWriteCapacity != newWriteCapacity || oldReadCapacity != newReadCapacity)
 
+			// pluck non_key_attributes from oldAttributes and newAttributes as reflect.DeepEquals will compare
+			// ordinal of elements in its equality (which we actually don't care about)
+			nonKeyAttributesChanged := checkIfNonKeyAttributesChanged(oldMap, newMap)
+
 			oldAttributes, err := stripCapacityAttributes(oldMap)
 			if err != nil {
 				e = err
 				return
 			}
+			oldAttributes, err = stripNonKeyAttributes(oldAttributes)
+			if err != nil {
+				e = err
+				return
+			}
 			newAttributes, err := stripCapacityAttributes(newMap)
 			if err != nil {
 				e = err
 				return
 			}
-			otherAttributesChanged := !reflect.DeepEqual(oldAttributes, newAttributes)
+			newAttributes, err = stripNonKeyAttributes(newAttributes)
+			if err != nil {
+				e = err
+				return
+			}
+			otherAttributesChanged := nonKeyAttributesChanged || !reflect.DeepEqual(oldAttributes, newAttributes)
 
 			if capacityChanged && !otherAttributesChanged {
 				update := &dynamodb.GlobalSecondaryIndexUpdate{
@@ -4214,6 +4228,49 @@ func diffDynamoDbGSI(oldGsi, newGsi []interface{}, billingMode string) (ops []*d
 	return
 }
 
+func stripNonKeyAttributes(in map[string]interface{}) (map[string]interface{}, error) {
+	mapCopy, err := copystructure.Copy(in)
+	if err != nil {
+		return nil, err
+	}
+
+	m := mapCopy.(map[string]interface{})
+
+	delete(m, "non_key_attributes")
+
+	return m, nil
+}
+
+// checkIfNonKeyAttributesChanged returns true if non_key_attributes between old map and new map are different
+func checkIfNonKeyAttributesChanged(oldMap, newMap map[string]interface{}) bool {
+	oldNonKeyAttributes, oldNkaExists := oldMap["non_key_attributes"].([]string)
+	newNonKeyAttributes, newNkaExists := newMap["non_key_attributes"].([]string)
+	if oldNkaExists != newNkaExists {
+		return true
+	}
+
+	o := map[string]bool{}
+	for _, oldNonKeyAttribute := range oldNonKeyAttributes {
+		o[oldNonKeyAttribute] = true
+	}
+	n := map[string]bool{}
+	for _, newNonKeyAttribute := range newNonKeyAttributes {
+		n[newNonKeyAttribute] = true
+	}
+
+	// perform this check after populating map so we ignore duplicated fields in non_key_attributes
+	if len(o) != len(n) {
+		return true
+	}
+
+	for k, v := range n {
+		if oVal, oExists := o[k]; !oExists || v != oVal {
+			return true
+		}
+	}
+	return false
+}
+
 func stripCapacityAttributes(in map[string]interface{}) (map[string]interface{}, error) {
 	mapCopy, err := copystructure.Copy(in)
 	if err != nil {
@@ -4474,6 +4531,10 @@ func expandDynamoDbProjection(data map[string]interface{}) *dynamodb.Projection
 		projection.NonKeyAttributes = expandStringList(v)
 	}
 
+	if v, ok := data["non_key_attributes"].(*schema.Set); ok && v.Len() > 0 {
+		projection.NonKeyAttributes = expandStringList(v.List())
+	}
+
 	return projection
 }