chore: Refactors AttributeChanges (#3122)

* AttributeChanges as []string instead of struct

* check if autoscaling

* fix doc

* fix linter

* include related fields

* Revert "include related fields"

This reverts commit 07624e8.

* Revert "fix linter"

This reverts commit 5cddaec.

* Revert "fix doc"

This reverts commit 85db779.

* Revert "check if autoscaling"

This reverts commit 70042c3.

Author: lantoli

internal/common/schemafunc/find_changes.go

@@ -10,21 +10,19 @@ import (
 	"github.com/hashicorp/terraform-plugin-framework/types"
 )
 
-type AttributeChanges struct {
-	Changes []string
-}
+type AttributeChanges []string
 
-func (a *AttributeChanges) LeafChanges() map[string]bool {
+func (a AttributeChanges) LeafChanges() map[string]bool {
 	return a.leafChanges(true)
 }
 
-func (a *AttributeChanges) AttributeChanged(name string) bool {
+func (a AttributeChanges) AttributeChanged(name string) bool {
 	changes := a.LeafChanges()
 	changed := changes[name]
 	return changed
 }
 
-func (a *AttributeChanges) KeepUnknown(attributeEffectedMapping map[string][]string) []string {
+func (a AttributeChanges) KeepUnknown(attributeEffectedMapping map[string][]string) []string {
 	var keepUnknown []string
 	for attrChanged, affectedAttributes := range attributeEffectedMapping {
 		if a.AttributeChanged(attrChanged) {
@@ -36,25 +34,25 @@ func (a *AttributeChanges) KeepUnknown(attributeEffectedMapping map[string][]str
 }
 
 // ListIndexChanged returns true if the list at the given index has changed, false if it was added or removed
-func (a *AttributeChanges) ListIndexChanged(name string, index int) bool {
+func (a AttributeChanges) ListIndexChanged(name string, index int) bool {
 	leafChanges := a.leafChanges(false)
 	indexPath := fmt.Sprintf("%s[%d]", name, index)
 	return leafChanges[indexPath]
 }
 
 // NestedListLenChanges accepts a fullPath, e.g., "replication_specs[0].region_configs" and returns true if the length of the nested list has changed
-func (a *AttributeChanges) NestedListLenChanges(fullPath string) bool {
+func (a AttributeChanges) NestedListLenChanges(fullPath string) bool {
 	addPrefix := fmt.Sprintf("%s[+", fullPath)
 	removePrefix := fmt.Sprintf("%s[-", fullPath)
-	for _, change := range a.Changes {
+	for _, change := range a {
 		if strings.HasPrefix(change, addPrefix) || strings.HasPrefix(change, removePrefix) {
 			return true
 		}
 	}
 	return false
 }
 
-func (a *AttributeChanges) ListLenChanges(name string) bool {
+func (a AttributeChanges) ListLenChanges(name string) bool {
 	leafChanges := a.leafChanges(false)
 	addPrefix := fmt.Sprintf("%s[+", name)
 	removePrefix := fmt.Sprintf("%s[-", name)
@@ -66,9 +64,9 @@ func (a *AttributeChanges) ListLenChanges(name string) bool {
 	return false
 }
 
-func (a *AttributeChanges) leafChanges(removeIndex bool) map[string]bool {
+func (a AttributeChanges) leafChanges(removeIndex bool) map[string]bool {
 	leafChanges := map[string]bool{}
-	for _, change := range a.Changes {
+	for _, change := range a {
 		var leaf string
 		parts := strings.Split(change, ".")
 		leaf = parts[len(parts)-1]
@@ -80,17 +78,12 @@ func (a *AttributeChanges) leafChanges(removeIndex bool) map[string]bool {
 	return leafChanges
 }
 
-// FindAttributeChanges: Iterates through TFModel of state+plan and returns AttributeChanges for querying changed attributes
-// The implementation is similar to KeepUnknown, no support for types.Set or types.Tuple yet
-func FindAttributeChanges(ctx context.Context, src, dest any) AttributeChanges {
-	changes := FindChanges(ctx, src, dest)
-	return AttributeChanges{changes}
-}
-
-func FindChanges(ctx context.Context, src, dest any) []string {
+// NewAttributeChanges iterates through TFModel of state+plan and returns AttributeChanges for querying changed attributes.
+// The implementation is similar to KeepUnknown, it doesn't support types.Set or types.Tuple yet.
+func NewAttributeChanges(ctx context.Context, src, dest any) AttributeChanges {
 	valSrc, valDest := validateStructPointers(src, dest)
 	typeDest := valDest.Type()
-	changes := []string{} // Always return an empty list, as nested attributes might be added and then removed, which make the test cases fail on nil vs []
+	changes := []string{} // Always return an empty list so AttributeChanges is not nil, also nested attributes might be added and then removed, which make the test cases fail on nil vs []
 	for i := range typeDest.NumField() {
 		fieldDest := typeDest.Field(i)
 		name, tfName := fieldNameTFName(&fieldDest)

internal/common/schemafunc/find_changes_test.go

@@ -12,7 +12,7 @@ func TestFindChanges(t *testing.T) {
 	tests := map[string]struct {
 		src      any
 		dest     any
-		expected []string
+		expected schemafunc.AttributeChanges
 	}{
 		"no changes": {
 			src:      &TFSimpleModel{Name: types.StringValue("name")},
@@ -118,15 +118,15 @@ func TestFindChanges(t *testing.T) {
 	}
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			actual := schemafunc.FindChanges(ctx, tc.src, tc.dest)
+			actual := schemafunc.NewAttributeChanges(ctx, tc.src, tc.dest)
 			assert.Equal(t, tc.expected, actual)
 		})
 	}
 }
 func TestAttributeChanges_LeafChanges(t *testing.T) {
 	tests := map[string]struct {
 		expected map[string]bool
-		changes  []string
+		changes  schemafunc.AttributeChanges
 	}{
 		"empty changes": {
 			changes:  []string{},
@@ -173,8 +173,7 @@ func TestAttributeChanges_LeafChanges(t *testing.T) {
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			ac := schemafunc.AttributeChanges{Changes: tc.changes}
-			actual := ac.LeafChanges()
+			actual := tc.changes.LeafChanges()
 			assert.Equal(t, tc.expected, actual)
 		})
 	}
@@ -183,7 +182,7 @@ func TestAttributeChanges_LeafChanges(t *testing.T) {
 func TestAttributeChanges_AttributeChanged(t *testing.T) {
 	tests := map[string]struct {
 		attr     string
-		changes  []string
+		changes  schemafunc.AttributeChanges
 		expected bool
 	}{
 		"match found": {
@@ -210,15 +209,14 @@ func TestAttributeChanges_AttributeChanged(t *testing.T) {
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			ac := schemafunc.AttributeChanges{Changes: tc.changes}
-			actual := ac.AttributeChanged(tc.attr)
+			actual := tc.changes.AttributeChanged(tc.attr)
 			assert.Equal(t, tc.expected, actual)
 		})
 	}
 }
 func TestAttributeChanges_KeepUnknown(t *testing.T) {
 	tests := map[string]struct {
-		changes                  []string
+		changes                  schemafunc.AttributeChanges
 		attributeEffectedMapping map[string][]string
 		expectedKeepUnknownAttrs []string
 	}{
@@ -269,16 +267,15 @@ func TestAttributeChanges_KeepUnknown(t *testing.T) {
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			ac := schemafunc.AttributeChanges{Changes: tc.changes}
-			actual := ac.KeepUnknown(tc.attributeEffectedMapping)
+			actual := tc.changes.KeepUnknown(tc.attributeEffectedMapping)
 			assert.ElementsMatch(t, tc.expectedKeepUnknownAttrs, actual)
 		})
 	}
 }
 func TestAttributeChanges_ListLenChanges(t *testing.T) {
 	tests := map[string]struct {
 		name     string
-		changes  []string
+		changes  schemafunc.AttributeChanges
 		expected bool
 	}{
 		"empty changes": {
@@ -325,16 +322,15 @@ func TestAttributeChanges_ListLenChanges(t *testing.T) {
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			ac := schemafunc.AttributeChanges{Changes: tc.changes}
-			actual := ac.ListLenChanges(tc.name)
+			actual := tc.changes.ListLenChanges(tc.name)
 			assert.Equal(t, tc.expected, actual)
 		})
 	}
 }
 func TestAttributeChanges_ListIndexChanged(t *testing.T) {
 	tests := map[string]struct {
 		name     string
-		changes  []string
+		changes  schemafunc.AttributeChanges
 		index    int
 		expected bool
 	}{
@@ -396,16 +392,15 @@ func TestAttributeChanges_ListIndexChanged(t *testing.T) {
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			ac := schemafunc.AttributeChanges{Changes: tc.changes}
-			actual := ac.ListIndexChanged(tc.name, tc.index)
+			actual := tc.changes.ListIndexChanged(tc.name, tc.index)
 			assert.Equal(t, tc.expected, actual)
 		})
 	}
 }
 func TestAttributeChanges_NestedListLenChanges(t *testing.T) {
 	tests := map[string]struct {
 		fullPath string
-		changes  []string
+		changes  schemafunc.AttributeChanges
 		expected bool
 	}{
 		"empty changes": {
@@ -456,8 +451,7 @@ func TestAttributeChanges_NestedListLenChanges(t *testing.T) {
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			ac := schemafunc.AttributeChanges{Changes: tc.changes}
-			actual := ac.NestedListLenChanges(tc.fullPath)
+			actual := tc.changes.NestedListLenChanges(tc.fullPath)
 			assert.Equal(t, tc.expected, actual)
 		})
 	}

internal/service/advancedclustertpf/plan_modifier.go

@@ -31,13 +31,10 @@ var (
 // useStateForUnknowns should be called only in Update, because of findClusterDiff
 func useStateForUnknowns(ctx context.Context, diags *diag.Diagnostics, state, plan *TFModel) {
 	diff := findClusterDiff(ctx, state, plan, diags)
-	if diags.HasError() {
-		return
-	}
-	if diff.isAnyUpgrade() { // Don't do anything in upgrades
+	if diags.HasError() || diff.isAnyUpgrade() { // Don't do anything in upgrades
 		return
 	}
-	attributeChanges := schemafunc.FindAttributeChanges(ctx, state, plan)
+	attributeChanges := schemafunc.NewAttributeChanges(ctx, state, plan)
 	keepUnknown := []string{"connection_strings", "state_name"} // Volatile attributes, should not be copied from state
 	keepUnknown = append(keepUnknown, attributeChanges.KeepUnknown(attributeRootChangeMapping)...)
 	schemafunc.CopyUnknowns(ctx, state, plan, keepUnknown)