feat!: made EvaluationContext fields unexported with a constructor an…

…d setters to enforce immutability (#91)

* feat!: made EvaluationContext fields unexported with a constructor and setters to enforce immutability

Signed-off-by: Skye Gill <gill.skye95@gmail.com>

README.md

@@ -31,7 +31,7 @@ func main() {
 	openfeature.SetProvider(openfeature.NoopProvider{})
 	client := openfeature.NewClient("app")
 	value, err := client.BooleanValue(
-		context.Background(), "v2_enabled", false, openfeature.EvaluationContext{}, openfeature.EvaluationOptions{}, 
+		context.Background(), "v2_enabled", false, openfeature.EvaluationContext{},
 	)
 }
 ```

pkg/openfeature/client.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"sync"
 
 	"github.com/go-logr/logr"
 )
@@ -38,6 +39,7 @@ func (cm ClientMetadata) Name() string {
 
 // Client implements the behaviour required of an openfeature client
 type Client struct {
+	mx                *sync.Mutex
 	metadata          ClientMetadata
 	hooks             []Hook
 	evaluationContext EvaluationContext
@@ -47,6 +49,7 @@ type Client struct {
 // NewClient returns a new Client. Name is a unique identifier for this client
 func NewClient(name string) *Client {
 	return &Client{
+		mx:                &sync.Mutex{},
 		metadata:          ClientMetadata{name: name},
 		hooks:             []Hook{},
 		evaluationContext: EvaluationContext{},
@@ -56,6 +59,8 @@ func NewClient(name string) *Client {
 
 // WithLogger sets the logger of the client
 func (c *Client) WithLogger(l logr.Logger) *Client {
+	c.mx.Lock()
+	defer c.mx.Unlock()
 	c.logger = func() logr.Logger { return l }
 	return c
 }
@@ -67,12 +72,16 @@ func (c Client) Metadata() ClientMetadata {
 
 // AddHooks appends to the client's collection of any previously added hooks
 func (c *Client) AddHooks(hooks ...Hook) {
+	c.mx.Lock()
+	defer c.mx.Unlock()
 	c.hooks = append(c.hooks, hooks...)
 	c.logger().V(info).Info("appended hooks to client", "client", c.Metadata().name, "hooks", hooks)
 }
 
 // SetEvaluationContext sets the client's evaluation context
 func (c *Client) SetEvaluationContext(evalCtx EvaluationContext) {
+	c.mx.Lock()
+	defer c.mx.Unlock()
 	c.evaluationContext = evalCtx
 	c.logger().V(info).Info(
 		"set client evaluation context", "client", c.Metadata().name, "evaluationContext", evalCtx,
@@ -610,11 +619,11 @@ func (c Client) evaluate(
 
 func flattenContext(evalCtx EvaluationContext) FlattenedContext {
 	flatCtx := FlattenedContext{}
-	if evalCtx.Attributes != nil {
-		flatCtx = evalCtx.Attributes
+	if evalCtx.attributes != nil {
+		flatCtx = evalCtx.Attributes()
 	}
-	if evalCtx.TargetingKey != "" {
-		flatCtx[TargetingKey] = evalCtx.TargetingKey
+	if evalCtx.targetingKey != "" {
+		flatCtx[TargetingKey] = evalCtx.targetingKey
 	}
 	return flatCtx
 }
@@ -678,17 +687,21 @@ func mergeContexts(evaluationContexts ...EvaluationContext) EvaluationContext {
 		return EvaluationContext{}
 	}
 
-	mergedCtx := evaluationContexts[0]
+	// create copy to prevent mutation of given EvaluationContext
+	mergedCtx := EvaluationContext{
+		attributes:   evaluationContexts[0].Attributes(),
+		targetingKey: evaluationContexts[0].targetingKey,
+	}
 
 	for i := 1; i < len(evaluationContexts); i++ {
-		if mergedCtx.TargetingKey == "" && evaluationContexts[i].TargetingKey != "" {
-			mergedCtx.TargetingKey = evaluationContexts[i].TargetingKey
+		if mergedCtx.targetingKey == "" && evaluationContexts[i].targetingKey != "" {
+			mergedCtx.targetingKey = evaluationContexts[i].targetingKey
 		}
 
-		for k, v := range evaluationContexts[i].Attributes {
-			_, ok := mergedCtx.Attributes[k]
+		for k, v := range evaluationContexts[i].attributes {
+			_, ok := mergedCtx.attributes[k]
 			if !ok {
-				mergedCtx.Attributes[k] = v
+				mergedCtx.attributes[k] = v
 			}
 		}
 	}

pkg/openfeature/client_test.go

@@ -648,12 +648,12 @@ func TestFlattenContext(t *testing.T) {
 	}{
 		"happy path": {
 			inCtx: EvaluationContext{
-				Attributes: map[string]interface{}{
+				attributes: map[string]interface{}{
 					"1": "string",
 					"2": 0.01,
 					"3": false,
 				},
-				TargetingKey: "user",
+				targetingKey: "user",
 			},
 			outCtx: FlattenedContext{
 				TargetingKey: "user",
@@ -664,7 +664,7 @@ func TestFlattenContext(t *testing.T) {
 		},
 		"no targeting key": {
 			inCtx: EvaluationContext{
-				Attributes: map[string]interface{}{
+				attributes: map[string]interface{}{
 					"1": "string",
 					"2": 0.01,
 					"3": false,
@@ -678,8 +678,8 @@ func TestFlattenContext(t *testing.T) {
 		},
 		"duplicated key": {
 			inCtx: EvaluationContext{
-				TargetingKey: "user",
-				Attributes: map[string]interface{}{
+				targetingKey: "user",
+				attributes: map[string]interface{}{
 					TargetingKey: "not user",
 					"1":          "string",
 					"2":          0.01,
@@ -695,7 +695,7 @@ func TestFlattenContext(t *testing.T) {
 		},
 		"no attributes": {
 			inCtx: EvaluationContext{
-				TargetingKey: "user",
+				targetingKey: "user",
 			},
 			outCtx: FlattenedContext{
 				TargetingKey: "user",
@@ -733,7 +733,7 @@ func TestBeforeHookNilContext(t *testing.T) {
 
 	client := NewClient("test")
 	attributes := map[string]interface{}{"should": "persist"}
-	evalCtx := EvaluationContext{Attributes: attributes}
+	evalCtx := EvaluationContext{attributes: attributes}
 	mockProvider.EXPECT().BooleanEvaluation(gomock.Any(), gomock.Any(), gomock.Any(), attributes)
 
 	_, err := client.BooleanValue(

pkg/openfeature/evaluation_context.go

@@ -1,8 +1,48 @@
 package openfeature
 
-// EvaluationContext
+// EvaluationContext provides ambient information for the purposes of flag evaluation
+// The use of the constructor, NewEvaluationContext, is enforced to set EvaluationContext's fields in order
+// to enforce immutability.
 // https://github.com/open-feature/spec/blob/main/specification/evaluation-context/evaluation-context.md
 type EvaluationContext struct {
-	TargetingKey string                 `json:"targetingKey"` // uniquely identifying the subject (end-user, or client service) of a flag evaluation
-	Attributes   map[string]interface{} `json:"attributes"`
+	targetingKey string // uniquely identifying the subject (end-user, or client service) of a flag evaluation
+	attributes   map[string]interface{}
+}
+
+// Attribute retrieves the attribute with the given key
+func (e EvaluationContext) Attribute(key string) interface{} {
+	return e.attributes[key]
+}
+
+// TargetingKey returns the key uniquely identifying the subject (end-user, or client service) of a flag evaluation
+func (e EvaluationContext) TargetingKey() string {
+	return e.targetingKey
+}
+
+// Attributes returns a copy of the EvaluationContext's attributes
+func (e EvaluationContext) Attributes() map[string]interface{} {
+	// copy attributes to new map to prevent mutation (maps are passed by reference)
+	attrs := make(map[string]interface{}, len(e.attributes))
+	for key, value := range e.attributes {
+		attrs[key] = value
+	}
+
+	return attrs
+}
+
+// NewEvaluationContext constructs an EvaluationContext
+//
+// targetingKey - uniquely identifying the subject (end-user, or client service) of a flag evaluation
+// attributes - contextual data used in flag evaluation
+func NewEvaluationContext(targetingKey string, attributes map[string]interface{}) EvaluationContext {
+	// copy attributes to new map to avoid reference being externally available, thereby enforcing immutability
+	attrs := make(map[string]interface{}, len(attributes))
+	for key, value := range attributes {
+		attrs[key] = value
+	}
+
+	return EvaluationContext{
+		targetingKey: targetingKey,
+		attributes:   attrs,
+	}
 }

pkg/openfeature/evaluation_context_test.go

@@ -14,7 +14,7 @@ func TestRequirement_3_1_1(t *testing.T) {
 	evalCtx := &EvaluationContext{}
 
 	metaValue := reflect.ValueOf(evalCtx).Elem()
-	fieldName := "TargetingKey"
+	fieldName := "targetingKey"
 
 	field := metaValue.FieldByName(fieldName)
 	if field == (reflect.Value{}) {
@@ -27,15 +27,15 @@ func TestRequirement_3_1_1(t *testing.T) {
 func TestRequirement_3_1_2(t *testing.T) {
 	evalCtx := EvaluationContext{}
 
-	tpe := reflect.TypeOf(evalCtx.Attributes)
+	tpe := reflect.TypeOf(evalCtx.attributes)
 	if tpe.Kind() != reflect.Map {
-		t.Fatalf("expected EvaluationContext.Attributes kind to be map, got %s", tpe.Kind())
+		t.Fatalf("expected EvaluationContext.attributes kind to be map, got %s", tpe.Kind())
 	}
 	if tpe.Key().Kind() != reflect.String {
-		t.Errorf("expected EvaluationContext.Attributes key to be string, got %s", tpe.Key().Kind())
+		t.Errorf("expected EvaluationContext.attributes key to be string, got %s", tpe.Key().Kind())
 	}
 	if tpe.Elem().Kind() != reflect.Interface {
-		t.Errorf("expected EvaluationContext.Attributes value to be interface{}, got %s", tpe.Elem().Kind())
+		t.Errorf("expected EvaluationContext.attributes value to be interface{}, got %s", tpe.Elem().Kind())
 	}
 }
 
@@ -90,8 +90,8 @@ func TestRequirement_3_2_2(t *testing.T) {
 	ctrl := gomock.NewController(t)
 
 	apiEvalCtx := EvaluationContext{
-		TargetingKey: "API",
-		Attributes: map[string]interface{}{
+		targetingKey: "API",
+		attributes: map[string]interface{}{
 			"invocationEvalCtx": true,
 			"foo":               2,
 			"user":              2,
@@ -105,8 +105,8 @@ func TestRequirement_3_2_2(t *testing.T) {
 
 	client := NewClient("test")
 	clientEvalCtx := EvaluationContext{
-		TargetingKey: "Client",
-		Attributes: map[string]interface{}{
+		targetingKey: "Client",
+		attributes: map[string]interface{}{
 			"clientEvalCtx": true,
 			"foo":           1,
 			"user":          1,
@@ -115,17 +115,17 @@ func TestRequirement_3_2_2(t *testing.T) {
 	client.SetEvaluationContext(clientEvalCtx)
 
 	invocationEvalCtx := EvaluationContext{
-		TargetingKey: "",
-		Attributes: map[string]interface{}{
+		targetingKey: "",
+		attributes: map[string]interface{}{
 			"apiEvalCtx": true,
 			"foo":        "bar",
 		},
 	}
 
 	mockProvider.EXPECT().Hooks().AnyTimes()
 	expectedMergedEvalCtx := EvaluationContext{
-		TargetingKey: "Client",
-		Attributes: map[string]interface{}{
+		targetingKey: "Client",
+		attributes: map[string]interface{}{
 			"apiEvalCtx":        true,
 			"invocationEvalCtx": true,
 			"clientEvalCtx":     true,
@@ -142,3 +142,29 @@ func TestRequirement_3_2_2(t *testing.T) {
 	}
 
 }
+
+func TestEvaluationContext_AttributesNotPassedByReference(t *testing.T) {
+	attributes := map[string]interface{}{
+		"foo": "bar",
+	}
+	evalCtx := NewEvaluationContext("foo", attributes)
+
+	attributes["immutabilityCheck"] = "safe"
+
+	if _, ok := evalCtx.attributes["immutabilityCheck"]; ok {
+		t.Error("mutation of map passed to NewEvaluationContext caused a mutation of its attributes field")
+	}
+}
+
+func TestEvaluationContext_AttributesFuncNotPassedByReference(t *testing.T) {
+	evalCtx := NewEvaluationContext("foo", map[string]interface{}{
+		"foo": "bar",
+	})
+
+	attributes := evalCtx.Attributes()
+	attributes["immutabilityCheck"] = "safe"
+
+	if _, ok := evalCtx.attributes["immutabilityCheck"]; ok {
+		t.Error("mutation of map passed to SetAttributes caused a mutation of its attributes field")
+	}
+}

pkg/openfeature/hooks_test.go

@@ -206,7 +206,11 @@ func TestRequirement_4_3_3(t *testing.T) {
 
 	flagKey := "foo"
 	defaultValue := "bar"
-	evalCtx := EvaluationContext{}
+	evalCtx := EvaluationContext{
+		attributes: map[string]interface{}{
+			"is": "a test",
+		},
+	}
 
 	mockProvider.EXPECT().Hooks().AnyTimes()
 
@@ -218,10 +222,12 @@ func TestRequirement_4_3_3(t *testing.T) {
 		providerMetadata:  mockProvider.Metadata(),
 		evaluationContext: evalCtx,
 	}
-	hook1EvalCtxResult := &EvaluationContext{TargetingKey: "mockHook1"}
-	hook1EvalCtxResultFlat := flattenContext(*hook1EvalCtxResult)
+	hook1EvalCtxResult := &EvaluationContext{targetingKey: "mockHook1"}
 	mockHook1.EXPECT().Before(hook1Ctx, gomock.Any()).Return(hook1EvalCtxResult, nil)
-	mockProvider.EXPECT().StringEvaluation(gomock.Any(), flagKey, defaultValue, hook1EvalCtxResultFlat)
+	mockProvider.EXPECT().StringEvaluation(gomock.Any(), flagKey, defaultValue, map[string]interface{}{
+		"is":         "a test",
+		TargetingKey: "mockHook1",
+	})
 
 	// assert that the evaluation context returned by the first hook is passed into the second hook
 	hook2Ctx := hook1Ctx
@@ -253,15 +259,15 @@ func TestRequirement_4_3_4(t *testing.T) {
 	client := NewClient("test")
 
 	apiEvalCtx := EvaluationContext{
-		Attributes: map[string]interface{}{
+		attributes: map[string]interface{}{
 			"key":            "api context",
 			"lowestPriority": true,
 		},
 	}
 	SetEvaluationContext(apiEvalCtx)
 
 	clientEvalCtx := EvaluationContext{
-		Attributes: map[string]interface{}{
+		attributes: map[string]interface{}{
 			"key":            "client context",
 			"lowestPriority": false,
 			"beatsClient":    false,
@@ -272,7 +278,7 @@ func TestRequirement_4_3_4(t *testing.T) {
 	flagKey := "foo"
 	defaultValue := "bar"
 	invEvalCtx := EvaluationContext{
-		Attributes: map[string]interface{}{
+		attributes: map[string]interface{}{
 			"key":         "invocation context",
 			"on":          true,
 			"beatsClient": true,
@@ -282,7 +288,7 @@ func TestRequirement_4_3_4(t *testing.T) {
 	mockProvider.EXPECT().Hooks().AnyTimes()
 
 	hookEvalCtxResult := &EvaluationContext{
-		Attributes: map[string]interface{}{
+		attributes: map[string]interface{}{
 			"key":        "hook value",
 			"multiplier": 3,
 		},
@@ -291,7 +297,7 @@ func TestRequirement_4_3_4(t *testing.T) {
 
 	// assert that the EvaluationContext returned by Before hooks is merged with the invocation EvaluationContext
 	expectedMergedContext := EvaluationContext{
-		Attributes: map[string]interface{}{
+		attributes: map[string]interface{}{
 			"key":            "hook value", // hook takes precedence
 			"multiplier":     3,
 			"on":             true,