Merge pull request #105 from k-yomo/fix-modify-replica-setting

Fix modifying replica setting
k-yomo · May 23, 2022 · b3fcdc7 · b3fcdc7
2 parents 9f1cb6e + 0d3be76
commit b3fcdc7
Show file tree

Hide file tree

Showing 4 changed files with 182 additions and 46 deletions.
diff --git a/internal/algoliautil/replica.go b/internal/algoliautil/replica.go
@@ -0,0 +1,35 @@
+package algoliautil
+
+import (
+	"fmt"
+)
+
+func IndexExistsInReplicas(replicas []string, indexName string, isVirtual bool) bool {
+	replicaIndexName := getReplicaIndexName(indexName, isVirtual)
+	for _, replica := range replicas {
+		if replica == replicaIndexName {
+			return true
+		}
+	}
+	return false
+}
+
+func RemoveIndexFromReplicas(replicas []string, indexName string, isVirtual bool) []string {
+	replicaIndexName := getReplicaIndexName(indexName, isVirtual)
+
+	var newReplicas []string
+	for _, replica := range replicas {
+		if replica == replicaIndexName {
+			continue
+		}
+		newReplicas = append(newReplicas, replica)
+	}
+	return newReplicas
+}
+
+func getReplicaIndexName(indexName string, isVirtual bool) string {
+	if isVirtual {
+		return fmt.Sprintf("virtual(%s)", indexName)
+	}
+	return indexName
+}
diff --git a/internal/algoliautil/replica_test.go b/internal/algoliautil/replica_test.go
@@ -0,0 +1,124 @@
+package algoliautil
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestIndexExistsInReplicas(t *testing.T) {
+	t.Parallel()
+
+	type args struct {
+		replicas  []string
+		indexName string
+		isVirtual bool
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{
+			name: "returns true if target replica exists",
+			args: args{
+				replicas:  []string{"abc", "def", "virtual(ghi)"},
+				indexName: "abc",
+				isVirtual: false,
+			},
+			want: true,
+		},
+		{
+			name: "returns true if target virtual replica exists",
+			args: args{
+				replicas:  []string{"abc", "def", "virtual(ghi)"},
+				indexName: "ghi",
+				isVirtual: true,
+			},
+			want: true,
+		},
+		{
+			name: "returns false if target replica doesn't exist",
+			args: args{
+				replicas:  []string{"abc", "def", "virtual(ghi)"},
+				indexName: "ghi",
+				isVirtual: false,
+			},
+			want: false,
+		},
+		{
+			name: "returns false if target virtual replica doesn't exist",
+			args: args{
+				replicas:  []string{"abc", "def", "virtual(ghi)"},
+				indexName: "abc",
+				isVirtual: true,
+			},
+			want: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := IndexExistsInReplicas(tt.args.replicas, tt.args.indexName, tt.args.isVirtual); got != tt.want {
+				t.Errorf("IndexExistsInReplicas() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func Test_RemoveIndexFromReplicas(t *testing.T) {
+	t.Parallel()
+
+	type args struct {
+		replicas  []string
+		indexName string
+		isVirtual bool
+	}
+	tests := []struct {
+		name string
+		args args
+		want []string
+	}{
+		{
+			name: "returns replica list excluding the target replica",
+			args: args{
+				replicas:  []string{"target", "virtual(abc)", "virtual(target)"},
+				indexName: "target",
+				isVirtual: false,
+			},
+			want: []string{"virtual(abc)", "virtual(target)"},
+		},
+		{
+			name: "returns replica list excluding the target virtual replica",
+			args: args{
+				replicas:  []string{"target", "virtual(abc)", "virtual(target)"},
+				indexName: "target",
+				isVirtual: true,
+			},
+			want: []string{"target", "virtual(abc)"},
+		},
+		{
+			name: "returns original replica list if there is no target replica",
+			args: args{
+				replicas:  []string{"abc", "def", "virtual(target)"},
+				indexName: "target",
+				isVirtual: false,
+			},
+			want: []string{"abc", "def", "virtual(target)"},
+		},
+		{
+			name: "returns original replica list if there is no target virtual replica",
+			args: args{
+				replicas:  []string{"virtual(abc)", "virtual(def)", "target", "virtual(target"},
+				indexName: "target",
+				isVirtual: true,
+			},
+			want: []string{"virtual(abc)", "virtual(def)", "target", "virtual(target"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := RemoveIndexFromReplicas(tt.args.replicas, tt.args.indexName, tt.args.isVirtual); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("removeVirtualIndexFromReplicaList() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
diff --git a/internal/provider/resource_index.go b/internal/provider/resource_index.go
@@ -596,13 +596,7 @@ func resourceIndexCreate(ctx context.Context, d *schema.ResourceData, m interfac
 		if err != nil {
 			return diag.FromErr(err)
 		}
-		replicaExist := false
-		for _, replica := range primaryIndexSettings.Replicas.Get() {
-			if replica == indexName {
-				replicaExist = true
-			}
-		}
-		if !replicaExist {
+		if !algoliautil.IndexExistsInReplicas(primaryIndexSettings.Replicas.Get(), indexName, false) {
 			newReplicas := append(primaryIndexSettings.Replicas.Get(), indexName)
 			res, err := primaryIndex.SetSettings(search.Settings{
 				Replicas: opt.Replicas(newReplicas...),
@@ -672,21 +666,17 @@ func resourceIndexDelete(ctx context.Context, d *schema.ResourceData, m interfac
 		if err != nil {
 			return diag.FromErr(err)
 		}
-		var newReplicas []string
-		for _, replica := range primaryIndexSettings.Replicas.Get() {
-			if replica == indexName {
-				continue
+		if algoliautil.IndexExistsInReplicas(primaryIndexSettings.Replicas.Get(), indexName, false) {
+			newReplicas := algoliautil.RemoveIndexFromReplicas(primaryIndexSettings.Replicas.Get(), indexName, false)
+			updateReplicasRes, err := primaryIndex.SetSettings(search.Settings{
+				Replicas: opt.Replicas(newReplicas...),
+			})
+			if err != nil {
+				return diag.FromErr(err)
+			}
+			if err := updateReplicasRes.Wait(); err != nil {
+				return diag.FromErr(err)
 			}
-			newReplicas = append(newReplicas, replica)
-		}
-		updateReplicasRes, err := primaryIndex.SetSettings(search.Settings{
-			Replicas: opt.Replicas(newReplicas...),
-		})
-		if err != nil {
-			return diag.FromErr(err)
-		}
-		if err := updateReplicasRes.Wait(); err != nil {
-			return diag.FromErr(err)
 		}
 	}
 

diff --git a/internal/provider/resource_virtual_index.go b/internal/provider/resource_virtual_index.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"log"
-	"regexp"
 	"strconv"
 	"time"
 
@@ -562,8 +561,6 @@ This parameter is mainly intended to **limit the response size.** For example, i
 	}
 }
 
-var virtualReplicaNameRegex = regexp.MustCompile(`^virtual\(\S+\)$`)
-
 func resourceVirtualIndexCreate(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics {
 	apiClient := m.(*apiClient)
 
@@ -576,13 +573,8 @@ func resourceVirtualIndexCreate(ctx context.Context, d *schema.ResourceData, m i
 		return diag.FromErr(err)
 	}
 
-	virtualReplicaExist := false
-	for _, replica := range primaryIndexSettings.Replicas.Get() {
-		if virtualReplicaNameRegex.MatchString(replica) {
-			virtualReplicaExist = true
-		}
-	}
-	if !virtualReplicaExist {
+	replicas := primaryIndexSettings.Replicas.Get()
+	if !algoliautil.IndexExistsInReplicas(replicas, indexName, true) {
 		// Modifying the primary's replica setting on primary can cause problems if other replicas
 		// are modifying it at the same time. Lock the primary until we're done in order to prevent that.
 		mutexKV.Lock(algoliaIndexMutexKey(apiClient.appID, primaryIndexName))
@@ -655,23 +647,18 @@ func resourceVirtualIndexDelete(ctx context.Context, d *schema.ResourceData, m i
 	if err != nil {
 		return diag.FromErr(err)
 	}
-	var newReplicas []string
-	for _, replica := range primaryIndexSettings.Replicas.Get() {
-		if virtualReplicaNameRegex.MatchString(replica) {
-			continue
+	if algoliautil.IndexExistsInReplicas(primaryIndexSettings.Replicas.Get(), indexName, true) {
+		newReplicas := algoliautil.RemoveIndexFromReplicas(primaryIndexSettings.Replicas.Get(), indexName, true)
+		updateReplicasRes, err := primaryIndex.SetSettings(search.Settings{
+			Replicas: opt.Replicas(newReplicas...),
+		})
+		if err != nil {
+			return diag.FromErr(err)
+		}
+		if err := updateReplicasRes.Wait(); err != nil {
+			return diag.FromErr(err)
 		}
-		newReplicas = append(newReplicas, replica)
-	}
-	updateReplicasRes, err := primaryIndex.SetSettings(search.Settings{
-		Replicas: opt.Replicas(newReplicas...),
-	})
-	if err != nil {
-		return diag.FromErr(err)
-	}
-	if err := updateReplicasRes.Wait(); err != nil {
-		return diag.FromErr(err)
 	}
-
 	index := apiClient.searchClient.InitIndex(indexName)
 	deleteIndexRes, err := index.Delete(ctx)
 	if err != nil {