From 3a540b8509440f53544877a88fe43686f680f1ff Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Mon, 11 Nov 2024 14:29:47 -0800 Subject: [PATCH 1/3] Add support for diffing field sets --- tools/diff-processor/diff/diff.go | 155 ++++++++++++++++++------- tools/diff-processor/diff/diff_test.go | 45 ++++++- tools/diff-processor/diff/sets.go | 55 +++++++++ 3 files changed, 215 insertions(+), 40 deletions(-) create mode 100644 tools/diff-processor/diff/sets.go diff --git a/tools/diff-processor/diff/diff.go b/tools/diff-processor/diff/diff.go index 1f70760b1a89..ba33cd9f1725 100644 --- a/tools/diff-processor/diff/diff.go +++ b/tools/diff-processor/diff/diff.go @@ -2,11 +2,10 @@ package diff import ( "reflect" + "strings" "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" - "golang.org/x/exp/maps" ) // SchemaDiff is a nested map with resource names as top-level keys. @@ -15,8 +14,23 @@ type SchemaDiff map[string]ResourceDiff type ResourceDiff struct { ResourceConfig ResourceConfigDiff Fields map[string]FieldDiff + FieldSets ResourceFieldSetsDiff } +type ResourceFieldSetsDiff struct { + Old ResourceFieldSets + New ResourceFieldSets +} + +type ResourceFieldSets struct { + ConflictsWith []FieldSet + ExactlyOneOf []FieldSet + AtLeastOneOf []FieldSet + RequiredWith []FieldSet +} + +type FieldSet map[string]struct{} + type ResourceConfigDiff struct { Old *schema.Resource New *schema.Resource @@ -29,7 +43,7 @@ type FieldDiff struct { func ComputeSchemaDiff(oldResourceMap, newResourceMap map[string]*schema.Resource) SchemaDiff { schemaDiff := make(SchemaDiff) - for resource, _ := range union(maps.Keys(oldResourceMap), maps.Keys(newResourceMap)) { + for resource := range union(oldResourceMap, newResourceMap) { // Compute diff between old and new resources and fields. // TODO: add support for computing diff between resource configs, not just whether the // resource was added/removed. b/300114839 @@ -47,14 +61,12 @@ func ComputeSchemaDiff(oldResourceMap, newResourceMap map[string]*schema.Resourc } resourceDiff.Fields = make(map[string]FieldDiff) - for key, _ := range union(maps.Keys(flattenedOldSchema), maps.Keys(flattenedNewSchema)) { + for key := range union(flattenedOldSchema, flattenedNewSchema) { oldField := flattenedOldSchema[key] newField := flattenedNewSchema[key] - if fieldChanged(oldField, newField) { - resourceDiff.Fields[key] = FieldDiff{ - Old: oldField, - New: newField, - } + if fieldDiff, fieldSetsDiff, changed := diffFields(oldField, newField, key); changed { + resourceDiff.Fields[key] = fieldDiff + resourceDiff.FieldSets = mergeFieldSetsDiff(fieldSetsDiff, resourceDiff.FieldSets) } } if len(resourceDiff.Fields) > 0 || !cmp.Equal(resourceDiff.ResourceConfig.Old, resourceDiff.ResourceConfig.New) { @@ -64,17 +76,6 @@ func ComputeSchemaDiff(oldResourceMap, newResourceMap map[string]*schema.Resourc return schemaDiff } -func union(keys1, keys2 []string) map[string]struct{} { - allKeys := make(map[string]struct{}) - for _, key := range keys1 { - allKeys[key] = struct{}{} - } - for _, key := range keys2 { - allKeys[key] = struct{}{} - } - return allKeys -} - func flattenSchema(parentKey string, schemaObj map[string]*schema.Schema) map[string]*schema.Schema { flattened := make(map[string]*schema.Schema) @@ -96,16 +97,48 @@ func flattenSchema(parentKey string, schemaObj map[string]*schema.Schema) map[st return flattened } -func fieldChanged(oldField, newField *schema.Schema) bool { +func diffFields(oldField, newField *schema.Schema, fieldName string) (FieldDiff, ResourceFieldSetsDiff, bool) { // If either field is nil, it is changed; if both are nil (which should never happen) it's not if oldField == nil && newField == nil { - return false + return FieldDiff{}, ResourceFieldSetsDiff{}, false + } + + oldFieldSets := fieldSets(oldField, fieldName) + newFieldSets := fieldSets(newField, fieldName) + + fieldDiff := FieldDiff{ + Old: oldField, + New: newField, + } + fieldSetsDiff := ResourceFieldSetsDiff{ + Old: oldFieldSets, + New: newFieldSets, } if oldField == nil || newField == nil { - return true + return fieldDiff, fieldSetsDiff, true } // Check if any basic Schema struct fields have changed. // https://github.com/hashicorp/terraform-plugin-sdk/blob/v2.24.0/helper/schema/schema.go#L44 + if basicSchemaChanged(oldField, newField) { + return fieldDiff, fieldSetsDiff, true + } + + if !cmp.Equal(oldFieldSets, newFieldSets) { + return fieldDiff, fieldSetsDiff, true + } + + if elemChanged(oldField, newField) { + return fieldDiff, fieldSetsDiff, true + } + + if funcsChanged(oldField, newField) { + return fieldDiff, fieldSetsDiff, true + } + + return FieldDiff{}, ResourceFieldSetsDiff{}, false +} + +func basicSchemaChanged(oldField, newField *schema.Schema) bool { if oldField.Type != newField.Type { return true } @@ -148,26 +181,35 @@ func fieldChanged(oldField, newField *schema.Schema) bool { if oldField.Sensitive != newField.Sensitive { return true } + return false +} - // Compare slices - less := func(a, b string) bool { return a < b } - - if (len(oldField.ConflictsWith) > 0 || len(newField.ConflictsWith) > 0) && !cmp.Equal(oldField.ConflictsWith, newField.ConflictsWith, cmpopts.SortSlices(less)) { - return true +func fieldSets(field *schema.Schema, fieldName string) ResourceFieldSets { + if field == nil { + return ResourceFieldSets{} } - - if (len(oldField.ExactlyOneOf) > 0 || len(newField.ExactlyOneOf) > 0) && !cmp.Equal(oldField.ExactlyOneOf, newField.ExactlyOneOf, cmpopts.SortSlices(less)) { - return true + var conflictsWith, exactlyOneOf, atLeastOneOf, requiredWith []FieldSet + if len(field.ConflictsWith) > 0 { + conflictsWith = []FieldSet{sliceToSetRemoveZeroPadding(append(field.ConflictsWith, fieldName))} } - - if (len(oldField.AtLeastOneOf) > 0 || len(newField.AtLeastOneOf) > 0) && !cmp.Equal(oldField.AtLeastOneOf, newField.AtLeastOneOf, cmpopts.SortSlices(less)) { - return true + if len(field.ExactlyOneOf) > 0 { + exactlyOneOf = []FieldSet{sliceToSetRemoveZeroPadding(append(field.ExactlyOneOf, fieldName))} } - - if (len(oldField.RequiredWith) > 0 || len(newField.RequiredWith) > 0) && !cmp.Equal(oldField.RequiredWith, newField.RequiredWith, cmpopts.SortSlices(less)) { - return true + if len(field.AtLeastOneOf) > 0 { + atLeastOneOf = []FieldSet{sliceToSetRemoveZeroPadding(append(field.AtLeastOneOf, fieldName))} + } + if len(field.RequiredWith) > 0 { + requiredWith = []FieldSet{sliceToSetRemoveZeroPadding(append(field.RequiredWith, fieldName))} + } + return ResourceFieldSets{ + ConflictsWith: conflictsWith, + ExactlyOneOf: exactlyOneOf, + AtLeastOneOf: atLeastOneOf, + RequiredWith: requiredWith, } +} +func elemChanged(oldField, newField *schema.Schema) bool { // Check if Elem changed (unless old and new both represent nested fields) if (oldField.Elem == nil && newField.Elem != nil) || (oldField.Elem != nil && newField.Elem == nil) { return true @@ -183,12 +225,15 @@ func fieldChanged(oldField, newField *schema.Schema) bool { return true } if !oldIsResource && !newIsResource { - if fieldChanged(oldField.Elem.(*schema.Schema), newField.Elem.(*schema.Schema)) { + if _, _, changed := diffFields(oldField.Elem.(*schema.Schema), newField.Elem.(*schema.Schema), ""); changed { return true } } } + return false +} +func funcsChanged(oldField, newField *schema.Schema) bool { // Check if any Schema struct fields that are functions have changed if funcChanged(oldField.DiffSuppressFunc, newField.DiffSuppressFunc) { return true @@ -208,7 +253,6 @@ func fieldChanged(oldField, newField *schema.Schema) bool { if funcChanged(oldField.ValidateDiagFunc, newField.ValidateDiagFunc) { return true } - return false } @@ -225,3 +269,36 @@ func funcChanged(oldFunc, newFunc interface{}) bool { // b/300157205 return false } + +func mergeFieldSetsDiff(a, b ResourceFieldSetsDiff) ResourceFieldSetsDiff { + a.Old = mergeResourceFieldSets(a.Old, b.Old) + a.New = mergeResourceFieldSets(a.New, b.New) + return a +} + +func mergeResourceFieldSets(a, b ResourceFieldSets) ResourceFieldSets { + a.ConflictsWith = mergeFieldSets(a.ConflictsWith, b.ConflictsWith) + a.ExactlyOneOf = mergeFieldSets(a.ExactlyOneOf, b.ExactlyOneOf) + a.AtLeastOneOf = mergeFieldSets(a.AtLeastOneOf, b.AtLeastOneOf) + a.RequiredWith = mergeFieldSets(a.RequiredWith, b.RequiredWith) + return a +} + +func mergeFieldSets(a, b []FieldSet) []FieldSet { + keys := make(map[string]struct{}) + for _, set := range a { + slice := setToSortedSlice(set) + key := strings.Join(slice, ",") + keys[key] = struct{}{} + } + for _, set := range b { + slice := setToSortedSlice(set) + key := strings.Join(slice, ",") + if _, ok := keys[key]; ok { + continue + } + keys[key] = struct{}{} + a = append(a, set) + } + return a +} diff --git a/tools/diff-processor/diff/diff_test.go b/tools/diff-processor/diff/diff_test.go index 5fb6057abd5c..bab738355ea7 100644 --- a/tools/diff-processor/diff/diff_test.go +++ b/tools/diff-processor/diff/diff_test.go @@ -984,7 +984,7 @@ func TestFieldChanged(t *testing.T) { tc := tc t.Run(tn, func(t *testing.T) { t.Parallel() - changed := fieldChanged(tc.oldField, tc.newField) + _, _, changed := diffFields(tc.oldField, tc.newField, "") if changed != tc.expectChanged { if diff := cmp.Diff(tc.oldField, tc.newField); diff != "" { t.Errorf("want %t; got %t.\nField diff (-old, +new):\n%s", @@ -1074,9 +1074,15 @@ func TestComputeSchemaDiff(t *testing.T) { Schema: map[string]*schema.Schema{ "field_one": { Type: schema.TypeString, + ConflictsWith: []string{ + "field_two", + }, }, "field_two": { Type: schema.TypeList, + ConflictsWith: []string{ + "field_one", + }, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "field_three": { @@ -1110,9 +1116,15 @@ func TestComputeSchemaDiff(t *testing.T) { Schema: map[string]*schema.Schema{ "field_one": { Type: schema.TypeString, + ConflictsWith: []string{ + "field_two", + }, }, "field_two": { Type: schema.TypeList, + ConflictsWith: []string{ + "field_one", + }, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "field_three": { @@ -1134,9 +1146,15 @@ func TestComputeSchemaDiff(t *testing.T) { Schema: map[string]*schema.Schema{ "field_three": { Type: schema.TypeString, + ConflictsWith: []string{ + "field_two.0.field_four", + }, }, "field_four": { Type: schema.TypeInt, + ConflictsWith: []string{ + "field_two.0.field_three", + }, }, }, }, @@ -1151,10 +1169,35 @@ func TestComputeSchemaDiff(t *testing.T) { New: &schema.Resource{}, }, Fields: map[string]FieldDiff{ + "field_two.field_three": FieldDiff{ + Old: &schema.Schema{ + Type: schema.TypeString, + }, + New: &schema.Schema{ + Type: schema.TypeString, + ConflictsWith: []string{ + "field_two.0.field_four", + }, + }, + }, "field_two.field_four": FieldDiff{ Old: nil, New: &schema.Schema{ Type: schema.TypeInt, + ConflictsWith: []string{ + "field_two.0.field_three", + }, + }, + }, + }, + FieldSets: ResourceFieldSetsDiff{ + Old: ResourceFieldSets{}, + New: ResourceFieldSets{ + ConflictsWith: []FieldSet{ + { + "field_two.field_three": {}, + "field_two.field_four": {}, + }, }, }, }, diff --git a/tools/diff-processor/diff/sets.go b/tools/diff-processor/diff/sets.go new file mode 100644 index 000000000000..51e1636ff837 --- /dev/null +++ b/tools/diff-processor/diff/sets.go @@ -0,0 +1,55 @@ +package diff + +import ( + "sort" + "strings" +) + +// Return the union of two maps, overwriting any shared keys with the second map's values +func union[K comparable, V any](map1, map2 map[K]V) map[K]V { + if len(map1) == 0 { + return map2 + } + if len(map2) == 0 { + return map1 + } + merged := make(map[K]V, len(map1)+len(map2)) + for k, v := range map1 { + merged[k] = v + } + for k, v := range map2 { + merged[k] = v + } + return merged +} + +func sliceToSetRemoveZeroPadding(slice []string) map[string]struct{} { + set := make(map[string]struct{}) + for _, item := range slice { + set[removeZeroPadding(item)] = struct{}{} + } + return set +} + +// field1.0.field2 -> field1.field2 +func removeZeroPadding(zeroPadded string) string { + var trimmed string + for _, part := range strings.Split(zeroPadded, ".") { + if part != "0" { + trimmed += part + "." + } + } + if trimmed == "" { + return "" + } + return trimmed[:len(trimmed)-1] +} + +func setToSortedSlice(set map[string]struct{}) []string { + slice := make([]string, 0, len(set)) + for item := range set { + slice = append(slice, item) + } + sort.Strings(slice) + return slice +} From 73397e4d11f6926bd6ffa77dfd852408b881a663 Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Thu, 26 Dec 2024 21:42:36 +0000 Subject: [PATCH 2/3] Retain field set keys while merging resource --- tools/diff-processor/diff/diff.go | 83 +++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 27 deletions(-) diff --git a/tools/diff-processor/diff/diff.go b/tools/diff-processor/diff/diff.go index ba33cd9f1725..82816bddd430 100644 --- a/tools/diff-processor/diff/diff.go +++ b/tools/diff-processor/diff/diff.go @@ -1,7 +1,9 @@ package diff import ( + "maps" "reflect" + "slices" "strings" "github.com/google/go-cmp/cmp" @@ -22,6 +24,11 @@ type ResourceFieldSetsDiff struct { New ResourceFieldSets } +type ResourceFieldSetsDiffWithKeys struct { + Old ResourceFieldSetsWithKeys + New ResourceFieldSetsWithKeys +} + type ResourceFieldSets struct { ConflictsWith []FieldSet ExactlyOneOf []FieldSet @@ -29,6 +36,13 @@ type ResourceFieldSets struct { RequiredWith []FieldSet } +type ResourceFieldSetsWithKeys struct { + ConflictsWith map[string]FieldSet + ExactlyOneOf map[string]FieldSet + AtLeastOneOf map[string]FieldSet + RequiredWith map[string]FieldSet +} + type FieldSet map[string]struct{} type ResourceConfigDiff struct { @@ -61,14 +75,16 @@ func ComputeSchemaDiff(oldResourceMap, newResourceMap map[string]*schema.Resourc } resourceDiff.Fields = make(map[string]FieldDiff) + fieldSetsDiffWithKeys := ResourceFieldSetsDiffWithKeys{} for key := range union(flattenedOldSchema, flattenedNewSchema) { oldField := flattenedOldSchema[key] newField := flattenedNewSchema[key] if fieldDiff, fieldSetsDiff, changed := diffFields(oldField, newField, key); changed { resourceDiff.Fields[key] = fieldDiff - resourceDiff.FieldSets = mergeFieldSetsDiff(fieldSetsDiff, resourceDiff.FieldSets) + fieldSetsDiffWithKeys = mergeFieldSetsDiff(fieldSetsDiffWithKeys, fieldSetsDiff) } } + resourceDiff.FieldSets = removeFieldSetsDiffKeys(fieldSetsDiffWithKeys) if len(resourceDiff.Fields) > 0 || !cmp.Equal(resourceDiff.ResourceConfig.Old, resourceDiff.ResourceConfig.New) { schemaDiff[resource] = resourceDiff } @@ -270,35 +286,48 @@ func funcChanged(oldFunc, newFunc interface{}) bool { return false } -func mergeFieldSetsDiff(a, b ResourceFieldSetsDiff) ResourceFieldSetsDiff { - a.Old = mergeResourceFieldSets(a.Old, b.Old) - a.New = mergeResourceFieldSets(a.New, b.New) - return a +func mergeFieldSetsDiff(allFields ResourceFieldSetsDiffWithKeys, currentField ResourceFieldSetsDiff) ResourceFieldSetsDiffWithKeys { + allFields.Old = mergeResourceFieldSets(allFields.Old, currentField.Old) + allFields.New = mergeResourceFieldSets(allFields.New, currentField.New) + return allFields } -func mergeResourceFieldSets(a, b ResourceFieldSets) ResourceFieldSets { - a.ConflictsWith = mergeFieldSets(a.ConflictsWith, b.ConflictsWith) - a.ExactlyOneOf = mergeFieldSets(a.ExactlyOneOf, b.ExactlyOneOf) - a.AtLeastOneOf = mergeFieldSets(a.AtLeastOneOf, b.AtLeastOneOf) - a.RequiredWith = mergeFieldSets(a.RequiredWith, b.RequiredWith) - return a +func mergeResourceFieldSets(allFields ResourceFieldSetsWithKeys, currentField ResourceFieldSets) ResourceFieldSetsWithKeys { + allFields.ConflictsWith = mergeFieldSets(allFields.ConflictsWith, currentField.ConflictsWith) + allFields.ExactlyOneOf = mergeFieldSets(allFields.ExactlyOneOf, currentField.ExactlyOneOf) + allFields.AtLeastOneOf = mergeFieldSets(allFields.AtLeastOneOf, currentField.AtLeastOneOf) + allFields.RequiredWith = mergeFieldSets(allFields.RequiredWith, currentField.RequiredWith) + return allFields } -func mergeFieldSets(a, b []FieldSet) []FieldSet { - keys := make(map[string]struct{}) - for _, set := range a { - slice := setToSortedSlice(set) - key := strings.Join(slice, ",") - keys[key] = struct{}{} - } - for _, set := range b { - slice := setToSortedSlice(set) - key := strings.Join(slice, ",") - if _, ok := keys[key]; ok { - continue - } - keys[key] = struct{}{} - a = append(a, set) +func mergeFieldSets(allFields map[string]FieldSet, currentField []FieldSet) map[string]FieldSet { + for _, fieldSet := range currentField { + allFields[setKey(fieldSet)] = fieldSet + } + return allFields +} + +func setKey(set FieldSet) string { + slice := setToSortedSlice(set) + return strings.Join(slice, ",") +} + +func removeFieldSetsDiffKeys(fieldSets ResourceFieldSetsDiffWithKeys) ResourceFieldSetsDiff { + return ResourceFieldSetsDiff{ + Old: removeFieldSetsKey(fieldSets.Old), + New: removeFieldSetsKey(fieldSets.New), + } +} + +func removeFieldSetsKey(fieldSets ResourceFieldSetsWithKeys) ResourceFieldSets { + return ResourceFieldSets{ + ConflictsWith: removeFieldSetKey(fieldSets.ConflictsWith), + ExactlyOneOf: removeFieldSetKey(fieldSets.ExactlyOneOf), + AtLeastOneOf: removeFieldSetKey(fieldSets.AtLeastOneOf), + RequiredWith: removeFieldSetKey(fieldSets.RequiredWith), } - return a +} + +func removeFieldSetKey(fieldSets map[string]FieldSet) []FieldSet { + return slices.Collect(maps.Values(fieldSets)) } From 9963e73077f5cab05f587a47d3312e57cd70dac7 Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Tue, 28 Jan 2025 21:56:49 +0000 Subject: [PATCH 3/3] Add unit test, fix nil map issue --- tools/diff-processor/diff/diff.go | 3 +++ tools/diff-processor/diff/sets_test.go | 33 ++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 tools/diff-processor/diff/sets_test.go diff --git a/tools/diff-processor/diff/diff.go b/tools/diff-processor/diff/diff.go index 82816bddd430..a12eb8eb76e8 100644 --- a/tools/diff-processor/diff/diff.go +++ b/tools/diff-processor/diff/diff.go @@ -301,6 +301,9 @@ func mergeResourceFieldSets(allFields ResourceFieldSetsWithKeys, currentField Re } func mergeFieldSets(allFields map[string]FieldSet, currentField []FieldSet) map[string]FieldSet { + if allFields == nil { + allFields = make(map[string]FieldSet) + } for _, fieldSet := range currentField { allFields[setKey(fieldSet)] = fieldSet } diff --git a/tools/diff-processor/diff/sets_test.go b/tools/diff-processor/diff/sets_test.go new file mode 100644 index 000000000000..f2eb7a81923b --- /dev/null +++ b/tools/diff-processor/diff/sets_test.go @@ -0,0 +1,33 @@ +package diff + +import ( + "testing" +) + +func TestRemoveZeroPadding(t *testing.T) { + for _, tc := range []struct { + name string + zeroPadded string + expected string + }{ + { + name: "no zeroes", + zeroPadded: "a.b.c", + expected: "a.b.c", + }, + { + name: "one zero", + zeroPadded: "a.0.b.c", + expected: "a.b.c", + }, + { + name: "two zeroes", + zeroPadded: "a.0.b.0.c", + expected: "a.b.c", + }, + } { + if got := removeZeroPadding(tc.zeroPadded); got != tc.expected { + t.Errorf("unexpected result for test case %s: %s (expected %s)", tc.name, got, tc.expected) + } + } +}