Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for diffing field sets #12330

Merged
merged 3 commits into from
Jan 29, 2025
Merged

Add support for diffing field sets #12330

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a540b8
Add support for diffing field sets
trodge Nov 11, 2024
73397e4
Retain field set keys while merging resource
trodge Dec 26, 2024
9963e73
Add unit test, fix nil map issue
trodge Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 116 additions & 39 deletions tools/diff-processor/diff/diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could make this implementation a bit more efficient (and clearer) by using an intermediate structure here in ComputeSchemaDiff. For example, something like (ignoring whitespace issues):

for resource := range union(oldResourceMap, newResourceMap) {
  conflictsWithSets := map[string][]string
  // etc...
  	if fieldDiff, fieldSetsDiff, changed := diffFields(oldField, newField, key); changed {
		conflictsWithSets[setKey(fieldSetsDiff.Old.ConflictsWith)] = fieldSetsDiff.Old.ConflictsWith
  // then at the end of the loop
  resourceDiff.FieldSets = ResourceFieldSets{
    ConflictsWith: maps.Values(conflictsWithSets)
    // etc...
  }

Right now you're keeping fieldSets in the fieldSet structure from beginning to end, even though that means you need to convert them to a map of key: fieldSet every time a new fieldset is changed, to do the comparison. Instead, you could track them directly in a key: value map in the ComputeSchemaDiff function so that the key only has to be computed once per field, then convert those mappings to the correct output structure once per resource.

There's also a question of what the output structure should be. My instinct would be to have each type of fieldset be [][]string of the field names (minus zero padding?)... But I don't care strongly about this since (assuming it's built once per resource) it would be relatively easy to change later if we want to (and I don't have a strong idea in my head of what will be most useful / easiest to understand in the breaking change detector.)

}
}
if len(resourceDiff.Fields) > 0 || !cmp.Equal(resourceDiff.ResourceConfig.Old, resourceDiff.ResourceConfig.New) {
Expand All @@ -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)

Expand All @@ -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
}
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -208,7 +253,6 @@ func fieldChanged(oldField, newField *schema.Schema) bool {
if funcChanged(oldField.ValidateDiagFunc, newField.ValidateDiagFunc) {
return true
}

return false
}

Expand All @@ -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
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just as a style note, these functions end up being hard to read because they are too generic and because (relatedly) the variable names are just a and b. Trying to read through them quickly, it's easy to lose track of what a and b represent. I'm not sure these functions will still be around in the next version of this PR but - just something to keep in mind.

45 changes: 44 additions & 1 deletion tools/diff-processor/diff/diff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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": {
Expand Down Expand Up @@ -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": {
Expand All @@ -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",
},
},
},
},
Expand All @@ -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": {},
},
},
},
},
Expand Down
55 changes: 55 additions & 0 deletions tools/diff-processor/diff/sets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unit tests for this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'm not sure it makes sense to split this out into a separate function since it's only used once. Not a blocker though.

slice := make([]string, 0, len(set))
for item := range set {
slice = append(slice, item)
}
sort.Strings(slice)
return slice
}
Loading