Avoid pollution of map constants via defensive copy (#3599)

elastic · Aug 11, 2020 · 1a0e23b · 1a0e23b
1 parent d34d62f
commit 1a0e23b
Show file tree

Hide file tree

Showing 2 changed files with 109 additions and 3 deletions.
diff --git a/pkg/controller/elasticsearch/user/roles.go b/pkg/controller/elasticsearch/user/roles.go
@@ -192,10 +192,14 @@ func (r RolesFileContent) FileBytes() ([]byte, error) {
 	return yaml.Marshal(&r)
 }
 
-// mergeWith merges multiple rolesFileContent, giving priority to other.
+// mergeWith merges multiple rolesFileContent, giving priority to other returning a new RolesFileContent.
 func (r RolesFileContent) MergeWith(other RolesFileContent) RolesFileContent {
+	result := make(RolesFileContent)
+	for roleName, roleSpec := range r {
+		result[roleName] = roleSpec
+	}
 	for roleName, roleSpec := range other {
-		r[roleName] = roleSpec
+		result[roleName] = roleSpec
 	}
-	return r
+	return result
 }
diff --git a/pkg/controller/elasticsearch/user/roles_test.go b/pkg/controller/elasticsearch/user/roles_test.go
@@ -5,6 +5,7 @@
 package user
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/ghodss/yaml"
@@ -43,3 +44,104 @@ click_admins:
 	// the initial yaml and the serialized/de-serialized rolesFileContent should be the same
 	require.Equal(t, r, rAgain)
 }
+
+func TestRolesFileContent_MergeWith(t *testing.T) {
+	type args struct {
+		other RolesFileContent
+	}
+	tests := []struct {
+		name   string
+		r      RolesFileContent
+		args   args
+		want   RolesFileContent
+		assert func(r, other, result RolesFileContent)
+	}{
+
+		{
+			name: "when r is nil",
+			r:    nil,
+			args: args{
+				other: RolesFileContent(map[string]interface{}{"a": "c"}),
+			},
+			want: RolesFileContent(map[string]interface{}{"a": "c"}),
+		},
+		{
+			name: "when other is nil",
+			r:    RolesFileContent(map[string]interface{}{"a": "c"}),
+			args: args{
+				other: nil,
+			},
+			want: RolesFileContent(map[string]interface{}{"a": "c"}),
+		},
+		{
+			name: "when both are nil",
+			r:    nil,
+			args: args{
+				other: nil,
+			},
+			want: RolesFileContent(map[string]interface{}{}),
+		},
+		{
+			name: "when r is empty",
+			r:    RolesFileContent(map[string]interface{}{}),
+			args: args{
+				other: RolesFileContent(map[string]interface{}{"a": "c"}),
+			},
+			want: RolesFileContent(map[string]interface{}{"a": "c"}),
+		},
+		{
+			name: "when other is empty",
+			r:    RolesFileContent(map[string]interface{}{"a": "c"}),
+			args: args{
+				other: RolesFileContent(map[string]interface{}{}),
+			},
+			want: RolesFileContent(map[string]interface{}{"a": "c"}),
+		},
+		{
+			name: "when r has more items",
+			r:    RolesFileContent(map[string]interface{}{"a": "b", "d": "e"}),
+			args: args{
+				other: RolesFileContent(map[string]interface{}{"a": "c"}),
+			},
+			want: RolesFileContent(map[string]interface{}{"a": "c", "d": "e"}),
+		},
+		{
+			name: "when other has more items",
+			r:    RolesFileContent(map[string]interface{}{"a": "b"}),
+			args: args{
+				other: RolesFileContent(map[string]interface{}{"a": "c", "d": "e"}),
+			},
+			want: RolesFileContent(map[string]interface{}{"a": "c", "d": "e"}),
+		},
+		{
+			name: "does give priority to other",
+			r:    RolesFileContent(map[string]interface{}{"a": "b"}),
+			args: args{
+				other: RolesFileContent(map[string]interface{}{"a": "c"}),
+			},
+			want: RolesFileContent(map[string]interface{}{"a": "c"}),
+		},
+		{
+			name: "does not mutate in place",
+			r:    RolesFileContent(map[string]interface{}{"a": "b"}),
+			args: args{
+				other: RolesFileContent(map[string]interface{}{"a": "c"}),
+			},
+			want: RolesFileContent(map[string]interface{}{"a": "c"}),
+			assert: func(r, other, result RolesFileContent) {
+				require.Equal(t, r, RolesFileContent(map[string]interface{}{"a": "b"}))
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tt.r.MergeWith(tt.args.other)
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("MergeWith() = %v, want %v", got, tt.want)
+			}
+			if tt.assert != nil {
+				tt.assert(tt.r, tt.args.other, got)
+			}
+		})
+	}
+}