diff --git a/README.md b/README.md index 83286596..a9870963 100644 --- a/README.md +++ b/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{}, ) } ``` diff --git a/pkg/openfeature/client.go b/pkg/openfeature/client.go index 999c7e9d..a22e086c 100644 --- a/pkg/openfeature/client.go +++ b/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 } } } diff --git a/pkg/openfeature/client_test.go b/pkg/openfeature/client_test.go index cfcced08..cabc3294 100644 --- a/pkg/openfeature/client_test.go +++ b/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( diff --git a/pkg/openfeature/evaluation_context.go b/pkg/openfeature/evaluation_context.go index 074758f6..ad3ee89f 100644 --- a/pkg/openfeature/evaluation_context.go +++ b/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, + } } diff --git a/pkg/openfeature/evaluation_context_test.go b/pkg/openfeature/evaluation_context_test.go index 46173144..03bbcbdb 100644 --- a/pkg/openfeature/evaluation_context_test.go +++ b/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,8 +115,8 @@ 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", }, @@ -124,8 +124,8 @@ func TestRequirement_3_2_2(t *testing.T) { 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") + } +} diff --git a/pkg/openfeature/hooks_test.go b/pkg/openfeature/hooks_test.go index 866fda69..3d292f2e 100644 --- a/pkg/openfeature/hooks_test.go +++ b/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,7 +259,7 @@ 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, }, @@ -261,7 +267,7 @@ func TestRequirement_4_3_4(t *testing.T) { 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,