feat: isolate interfaces from SDK to improve testability

README.md

@@ -364,6 +364,33 @@ func (h MyHook) Error(context context.Context, hookContext openfeature.HookConte
 
 > Built a new hook? [Let us know](https://github.com/open-feature/openfeature.dev/issues/new?assignees=&labels=hook&projects=&template=document-hook.yaml&title=%5BHook%5D%3A+) so we can add it to the docs!
 
+## Testing
+
+To test interactions with OpenFeature API and Client, you can rely on `openfeature.IEvaluation` & `openfeature.IClient` interfaces.
+
+While you may use global methods to interact with the API, it is recommended to obtain the singleton API instance so that you can use appropriate mocks for your testing needs,
+
+```go
+// global helper
+openfeature.SetProvider(myProvider)
+
+// singleton instance - preferred
+apiInstance := openfeature.GetApiInstance()
+apiInstance.SetProvider(myProvider)
+```
+
+Similarly, while you have option (due to historical reasons) to create a client with `openfeature.NewClient()` helper, it is recommended to use API to generate the client which returns an `IClient` instance.
+
+```go
+// global helper
+openfeature.NewClient("myClient")
+
+// using API instance - preferred
+apiInstance := openfeature.GetApiInstance()
+apiInstance.GetClient()
+apiInstance.GetNamedClient("myClient")
+```
+
 <!-- x-hide-in-docs-start -->
 ## ⭐️ Support the project
 

openfeature/client.go

@@ -10,32 +10,6 @@
 	"github.com/go-logr/logr"
 )
 
-// IClient defines the behaviour required of an openfeature client
-type IClient interface {
-	Metadata() ClientMetadata
-	AddHooks(hooks ...Hook)
-	AddHandler(eventType EventType, callback EventCallback)
-	RemoveHandler(eventType EventType, callback EventCallback)
-	SetEvaluationContext(evalCtx EvaluationContext)
-	EvaluationContext() EvaluationContext
-	BooleanValue(ctx context.Context, flag string, defaultValue bool, evalCtx EvaluationContext, options ...Option) (bool, error)
-	StringValue(ctx context.Context, flag string, defaultValue string, evalCtx EvaluationContext, options ...Option) (string, error)
-	FloatValue(ctx context.Context, flag string, defaultValue float64, evalCtx EvaluationContext, options ...Option) (float64, error)
-	IntValue(ctx context.Context, flag string, defaultValue int64, evalCtx EvaluationContext, options ...Option) (int64, error)
-	ObjectValue(ctx context.Context, flag string, defaultValue interface{}, evalCtx EvaluationContext, options ...Option) (interface{}, error)
-	BooleanValueDetails(ctx context.Context, flag string, defaultValue bool, evalCtx EvaluationContext, options ...Option) (BooleanEvaluationDetails, error)
-	StringValueDetails(ctx context.Context, flag string, defaultValue string, evalCtx EvaluationContext, options ...Option) (StringEvaluationDetails, error)
-	FloatValueDetails(ctx context.Context, flag string, defaultValue float64, evalCtx EvaluationContext, options ...Option) (FloatEvaluationDetails, error)
-	IntValueDetails(ctx context.Context, flag string, defaultValue int64, evalCtx EvaluationContext, options ...Option) (IntEvaluationDetails, error)
-	ObjectValueDetails(ctx context.Context, flag string, defaultValue interface{}, evalCtx EvaluationContext, options ...Option) (InterfaceEvaluationDetails, error)
-
-	Boolean(ctx context.Context, flag string, defaultValue bool, evalCtx EvaluationContext, options ...Option) bool
-	String(ctx context.Context, flag string, defaultValue string, evalCtx EvaluationContext, options ...Option) string
-	Float(ctx context.Context, flag string, defaultValue float64, evalCtx EvaluationContext, options ...Option) float64
-	Int(ctx context.Context, flag string, defaultValue int64, evalCtx EvaluationContext, options ...Option) int64
-	Object(ctx context.Context, flag string, defaultValue interface{}, evalCtx EvaluationContext, options ...Option) interface{}
-}
-
 // ClientMetadata provides a client's metadata
 type ClientMetadata struct {
 	name string
@@ -62,31 +36,41 @@
 
 // Client implements the behaviour required of an openfeature client
 type Client struct {
-	mx                sync.RWMutex
+	api               evaluationImpl
+	clientEventing    clientEvent
 	metadata          ClientMetadata
 	hooks             []Hook
 	evaluationContext EvaluationContext
-	logger            func() logr.Logger
+	logger            logr.Logger
+
+	mx sync.RWMutex
 }
 
 // interface guard to ensure that Client implements IClient
 var _ IClient = (*Client)(nil)
 
 // NewClient returns a new Client. Name is a unique identifier for this client
-func NewClient(domain string) *Client {
+// This helper exists for historical reasons. It is recommended to interact with IEvaluation to derive IClient instances.
+func NewClient(name string) *Client {
+	return newClient(name, api, eventing, logger)
+}
+
+func newClient(name string, apiRef evaluationImpl, eventRef clientEvent, log logr.Logger) *Client {
 	return &Client{
-		metadata:          ClientMetadata{name: domain},
+		api:               apiRef,
+		clientEventing:    eventRef,
+		logger:            log,
+		metadata:          ClientMetadata{name: name},
 		hooks:             []Hook{},
 		evaluationContext: EvaluationContext{},
-		logger:            globalLogger,
 	}
 }
 
 // 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 }
+	c.logger = l
 	return c
 }
 
@@ -106,12 +90,12 @@
 
 // AddHandler allows to add Client level event handler
 func (c *Client) AddHandler(eventType EventType, callback EventCallback) {
-	addClientHandler(c.metadata.Domain(), eventType, callback)
+	c.clientEventing.AddClientHandler(c.metadata.Domain(), eventType, callback)
 }
 
 // RemoveHandler allows to remove Client level event handler
 func (c *Client) RemoveHandler(eventType EventType, callback EventCallback) {
-	removeClientHandler(c.metadata.Domain(), eventType, callback)
+	c.clientEventing.RemoveClientHandler(c.metadata.Domain(), eventType, callback)
 }
 
 // SetEvaluationContext sets the client's evaluation context
@@ -411,7 +395,7 @@
 	value, ok := evalDetails.Value.(bool)
 	if !ok {
 		err := errors.New("evaluated value is not a boolean")
-		c.logger().Error(
+		c.logger.Error(
 			err, "invalid flag resolution type", "expectedType", "boolean",
 			"gotType", fmt.Sprintf("%T", evalDetails.Value),
 		)
@@ -459,7 +443,7 @@
 	value, ok := evalDetails.Value.(string)
 	if !ok {
 		err := errors.New("evaluated value is not a string")
-		c.logger().Error(
+		c.logger.Error(
 			err, "invalid flag resolution type", "expectedType", "string",
 			"gotType", fmt.Sprintf("%T", evalDetails.Value),
 		)
@@ -507,7 +491,7 @@
 	value, ok := evalDetails.Value.(float64)
 	if !ok {
 		err := errors.New("evaluated value is not a float64")
-		c.logger().Error(
+		c.logger.Error(
 			err, "invalid flag resolution type", "expectedType", "float64",
 			"gotType", fmt.Sprintf("%T", evalDetails.Value),
 		)
@@ -555,7 +539,7 @@
 	value, ok := evalDetails.Value.(int64)
 	if !ok {
 		err := errors.New("evaluated value is not an int64")
-		c.logger().Error(
+		c.logger.Error(
 			err, "invalid flag resolution type", "expectedType", "int64",
 			"gotType", fmt.Sprintf("%T", evalDetails.Value),
 		)
@@ -691,7 +675,7 @@
 	}
 
 	// ensure that the same provider & hooks are used across this transaction to avoid unexpected behaviour
-	provider, globalHooks, globalCtx := forTransaction(c.metadata.name)
+	provider, globalHooks, globalCtx := c.api.ForEvaluation(c.metadata.name)
 
 	evalCtx = mergeContexts(evalCtx, c.evaluationContext, globalCtx)                                                           // API (global) -> client -> invocation
 	apiClientInvocationProviderHooks := append(append(append(globalHooks, c.hooks...), options.hooks...), provider.Hooks()...) // API, Client, Invocation, Provider
@@ -714,7 +698,7 @@
 	evalCtx, err = c.beforeHooks(ctx, hookCtx, apiClientInvocationProviderHooks, evalCtx, options)
 	hookCtx.evaluationContext = evalCtx
 	if err != nil {
-		c.logger().Error(
+		c.logger.Error(
 			err, "before hook", "flag", flag, "defaultValue", defaultValue,
 			"evaluationContext", evalCtx, "evaluationOptions", options, "type", flagType.String(),
 		)
@@ -752,7 +736,7 @@
 
 	err = resolution.Error()
 	if err != nil {
-		c.logger().Error(
+		c.logger.Error(
 			err, "flag resolution", "flag", flag, "defaultValue", defaultValue,
 			"evaluationContext", evalCtx, "evaluationOptions", options, "type", flagType.String(), "errorCode", err,
 			"errMessage", resolution.ResolutionError.message,
@@ -767,7 +751,7 @@
 	evalDetails.ResolutionDetail = resolution.ResolutionDetail()
 
 	if err := c.afterHooks(ctx, hookCtx, providerInvocationClientApiHooks, evalDetails, options); err != nil {
-		c.logger().Error(
+		c.logger.Error(
 			err, "after hook", "flag", flag, "defaultValue", defaultValue,
 			"evaluationContext", evalCtx, "evaluationOptions", options, "type", flagType.String(),
 		)

openfeature/client_test.go

@@ -416,7 +416,6 @@ func TestRequirement_1_4_8(t *testing.T) {
 // The MUST NOT abnormally terminate clause of this requirement is satisfied by the error included in the return
 // signatures, as is idiomatic in Go. Errors aren't fatal, the operations won't terminate as a result of an error.
 func TestRequirement_1_4_9(t *testing.T) {
-	client := NewClient("test-client")
 	flagKey := "flag-key"
 	evalCtx := EvaluationContext{}
 	flatCtx := flattenContext(evalCtx)
@@ -425,6 +424,8 @@ func TestRequirement_1_4_9(t *testing.T) {
 
 	t.Run("Boolean", func(t *testing.T) {
 		defer t.Cleanup(initSingleton)
+
+		client := NewClient("test-client")
 		mockProvider := NewMockFeatureProvider(ctrl)
 		defaultValue := true
 		mockProvider.EXPECT().Metadata().AnyTimes()
@@ -463,6 +464,8 @@ func TestRequirement_1_4_9(t *testing.T) {
 
 	t.Run("String", func(t *testing.T) {
 		defer t.Cleanup(initSingleton)
+
+		client := NewClient("test-client")
 		mockProvider := NewMockFeatureProvider(ctrl)
 		defaultValue := "default"
 		mockProvider.EXPECT().Metadata().AnyTimes()
@@ -501,6 +504,8 @@ func TestRequirement_1_4_9(t *testing.T) {
 
 	t.Run("Float", func(t *testing.T) {
 		defer t.Cleanup(initSingleton)
+
+		client := NewClient("test-client")
 		mockProvider := NewMockFeatureProvider(ctrl)
 		defaultValue := 3.14159
 		mockProvider.EXPECT().Metadata().AnyTimes()
@@ -539,6 +544,8 @@ func TestRequirement_1_4_9(t *testing.T) {
 
 	t.Run("Int", func(t *testing.T) {
 		defer t.Cleanup(initSingleton)
+
+		client := NewClient("test-client")
 		mockProvider := NewMockFeatureProvider(ctrl)
 		var defaultValue int64 = 3
 		mockProvider.EXPECT().Metadata().AnyTimes()
@@ -577,6 +584,8 @@ func TestRequirement_1_4_9(t *testing.T) {
 
 	t.Run("Object", func(t *testing.T) {
 		defer t.Cleanup(initSingleton)
+
+		client := NewClient("test-client")
 		mockProvider := NewMockFeatureProvider(ctrl)
 		type obj struct {
 			foo string
@@ -670,14 +679,15 @@ func TestRequirement_1_4_12(t *testing.T) {
 // the `evaluation details` structure's `flag metadata` field MUST contain that value. Otherwise,
 // it MUST contain an empty record.
 func TestRequirement_1_4_13(t *testing.T) {
-	client := NewClient("test-client")
 	flagKey := "flag-key"
 	evalCtx := EvaluationContext{}
 	flatCtx := flattenContext(evalCtx)
 
 	ctrl := gomock.NewController(t)
 	t.Run("No Metadata", func(t *testing.T) {
 		defer t.Cleanup(initSingleton)
+
+		client := NewClient("test-client")
 		mockProvider := NewMockFeatureProvider(ctrl)
 		defaultValue := true
 		mockProvider.EXPECT().Metadata().AnyTimes()
@@ -709,6 +719,8 @@ func TestRequirement_1_4_13(t *testing.T) {
 
 	t.Run("Metadata present", func(t *testing.T) {
 		defer t.Cleanup(initSingleton)
+
+		client := NewClient("test-client")
 		mockProvider := NewMockFeatureProvider(ctrl)
 		defaultValue := true
 		metadata := FlagMetadata{
@@ -818,7 +830,7 @@ func TestFlattenContext(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			out := flattenContext(test.inCtx)
 			if !reflect.DeepEqual(test.outCtx, out) {
-				t.Fatalf(
+				t.Errorf(
 					"%s, unexpected value received from flatten context, expected %v got %v",
 					name,
 					test.outCtx,
@@ -855,7 +867,7 @@ func TestBeforeHookNilContext(t *testing.T) {
 		context.Background(), "foo", false, evalCtx, WithHooks(hookNilContext),
 	)
 	if err != nil {
-		t.Fatal(err)
+		t.Error(err)
 	}
 }