From d906d6fe3e228d18e93c2d07be53ef07fb787083 Mon Sep 17 00:00:00 2001 From: warber <72415058+warber@users.noreply.github.com> Date: Wed, 4 Dec 2024 17:10:03 +0100 Subject: [PATCH] fix: consider default client when loading states (#309) fix: consider default client when loading states Signed-off-by: warber <72415058+warber@users.noreply.github.com> --- openfeature/client.go | 21 ++++++++------- openfeature/client_test.go | 26 +++++++++++++++--- openfeature/event_executor.go | 23 ++++++++++------ openfeature/event_executor_test.go | 18 ++++++------- openfeature/openfeature_test.go | 43 ++++++++++++++++++++++++++++++ 5 files changed, 101 insertions(+), 30 deletions(-) diff --git a/openfeature/client.go b/openfeature/client.go index ea64f05..095d398 100644 --- a/openfeature/client.go +++ b/openfeature/client.go @@ -713,16 +713,19 @@ func (c *Client) evaluate( c.finallyHooks(ctx, hookCtx, providerInvocationClientApiHooks, options) }() - // short circuit if provider is in NOT READY state - if c.State() == NotReadyState { - c.errorHooks(ctx, hookCtx, providerInvocationClientApiHooks, ProviderNotReadyError, options) - return evalDetails, ProviderNotReadyError - } + // bypass short-circuit logic for the Noop provider; it is essentially stateless and a "special case" + if _, ok := provider.(NoopProvider); !ok { + // short circuit if provider is in NOT READY state + if c.State() == NotReadyState { + c.errorHooks(ctx, hookCtx, providerInvocationClientApiHooks, ProviderNotReadyError, options) + return evalDetails, ProviderNotReadyError + } - // short circuit if provider is in FATAL state - if c.State() == FatalState { - c.errorHooks(ctx, hookCtx, providerInvocationClientApiHooks, ProviderFatalError, options) - return evalDetails, ProviderFatalError + // short circuit if provider is in FATAL state + if c.State() == FatalState { + c.errorHooks(ctx, hookCtx, providerInvocationClientApiHooks, ProviderFatalError, options) + return evalDetails, ProviderFatalError + } } evalCtx, err = c.beforeHooks(ctx, hookCtx, apiClientInvocationProviderHooks, evalCtx, options) diff --git a/openfeature/client_test.go b/openfeature/client_test.go index ac257dd..0975684 100644 --- a/openfeature/client_test.go +++ b/openfeature/client_test.go @@ -3,6 +3,7 @@ package openfeature import ( "context" "errors" + "math" "reflect" "testing" "time" @@ -1385,7 +1386,24 @@ func TestRequirement_1_7_6(t *testing.T) { mockHook.EXPECT().Error(gomock.Any(), gomock.Any(), ProviderNotReadyError, gomock.Any()) mockHook.EXPECT().Finally(gomock.Any(), gomock.Any(), gomock.Any()) - client := GetApiInstance().GetNamedClient(t.Name()) + notReadyEventingProvider := struct { + FeatureProvider + StateHandler + EventHandler + }{ + NoopProvider{}, + &stateHandlerForTests{ + initF: func(e EvaluationContext) error { + <-time.After(math.MaxInt) + return nil + }, + }, + &ProviderEventing{}, + } + + _ = GetApiInstance().SetProvider(notReadyEventingProvider) + + client := GetApiInstance().GetNamedClient("somOtherClient") client.AddHooks(mockHook) if client.State() != NotReadyState { @@ -1401,7 +1419,6 @@ func TestRequirement_1_7_6(t *testing.T) { if res != defaultVal { t.Fatalf("expected resolved boolean value to default to %t, got %t", defaultVal, res) } - } // The client MUST default, run error hooks, and indicate an error if flag resolution is attempted while the provider @@ -1422,7 +1439,10 @@ func TestRequirement_1_7_7(t *testing.T) { &ProviderEventing{}, } - _ = SetNamedProviderAndWait(t.Name(), provider) + err := SetNamedProviderAndWait(t.Name(), provider) + if err == nil { + t.Errorf("provider registration was expected to fail but succeeded unexpectedly") + } ctrl := gomock.NewController(t) mockHook := NewMockHook(ctrl) diff --git a/openfeature/event_executor.go b/openfeature/event_executor.go index f4edccd..a7a19c7 100644 --- a/openfeature/event_executor.go +++ b/openfeature/event_executor.go @@ -27,7 +27,7 @@ type clientEvent interface { State(domain string) State } -const defaultClient = "" +const defaultDomain = "" // event executor is a registry to connect API and Client event handlers to Providers @@ -102,7 +102,7 @@ func (e *eventExecutor) AddHandler(t EventType, c EventCallback) { e.apiRegistry[t] = append(e.apiRegistry[t], c) } - e.emitOnRegistration(defaultClient, e.defaultProviderReference, t, c) + e.emitOnRegistration(defaultDomain, e.defaultProviderReference, t, c) } // RemoveHandler removes an API(global) level handler @@ -189,7 +189,7 @@ func (e *eventExecutor) GetClientRegistry(client string) scopedCallback { // emitOnRegistration fulfils the spec requirement to fire events if the // event type and the state of the associated provider are compatible. func (e *eventExecutor) emitOnRegistration(domain string, providerReference providerReference, eventType EventType, callback EventCallback) { - state, ok := e.states.Load(domain) + state, ok := e.loadState(domain) if !ok { return } @@ -213,12 +213,19 @@ func (e *eventExecutor) emitOnRegistration(domain string, providerReference prov } } -func (e *eventExecutor) State(domain string) State { - s, ok := e.states.Load(domain) +func (e *eventExecutor) loadState(domain string) (State, bool) { + state, ok := e.states.Load(domain) if !ok { - return NotReadyState + if state, ok = e.states.Load(defaultDomain); !ok { + return NotReadyState, false + } } - return s.(State) + return state.(State), true +} + +func (e *eventExecutor) State(domain string) State { + state, _ := e.loadState(domain) + return state } // registerDefaultProvider registers the default FeatureProvider and remove the old default provider if available @@ -357,7 +364,7 @@ func (e *eventExecutor) triggerEvent(event Event, handler FeatureProvider) { } // handling the default provider - e.states.Store(defaultClient, stateFromEvent(event)) + e.states.Store(defaultDomain, stateFromEvent(event)) // invoke default provider bound (no provider associated) handlers by filtering for domain, registry := range e.scopedRegistry { if _, ok := e.namedProviderReference[domain]; ok { diff --git a/openfeature/event_executor_test.go b/openfeature/event_executor_test.go index 1e55074..0315c45 100644 --- a/openfeature/event_executor_test.go +++ b/openfeature/event_executor_test.go @@ -638,7 +638,7 @@ func TestEventHandler_ProviderReadiness(t *testing.T) { defer t.Cleanup(initSingleton) eventingImpl := &ProviderEventing{ - c: make(chan Event, 1), + c: make(chan Event), } readyEventingProvider := struct { @@ -673,7 +673,7 @@ func TestEventHandler_ProviderReadiness(t *testing.T) { defer t.Cleanup(initSingleton) eventingImpl := &ProviderEventing{ - c: make(chan Event, 1), + c: make(chan Event), } readyEventingProvider := struct { @@ -685,7 +685,7 @@ func TestEventHandler_ProviderReadiness(t *testing.T) { } clientAssociation := "clientA" - err := SetNamedProvider(clientAssociation, readyEventingProvider) + err := SetNamedProviderAndWait(clientAssociation, readyEventingProvider) if err != nil { t.Fatal(err) } @@ -695,7 +695,7 @@ func TestEventHandler_ProviderReadiness(t *testing.T) { rsp <- e } - client := NewClient(clientAssociation) + client := api.GetNamedClient(clientAssociation) client.AddHandler(ProviderReady, &callback) select { @@ -710,9 +710,8 @@ func TestEventHandler_ProviderReadiness(t *testing.T) { defer t.Cleanup(initSingleton) eventingImpl := &ProviderEventing{ - c: make(chan Event, 1), + c: make(chan Event), } - eventingImpl.Invoke(Event{EventType: ProviderConfigChange}) readyEventingProvider := struct { FeatureProvider @@ -722,7 +721,7 @@ func TestEventHandler_ProviderReadiness(t *testing.T) { eventingImpl, } - err := SetProvider(readyEventingProvider) + err := SetProviderAndWait(readyEventingProvider) if err != nil { t.Fatal(err) } @@ -732,7 +731,7 @@ func TestEventHandler_ProviderReadiness(t *testing.T) { rsp <- e } - client := NewClient("someClient") + client := api.GetNamedClient("someClient") client.AddHandler(ProviderReady, &callback) select { @@ -770,7 +769,7 @@ func TestEventHandler_ProviderReadiness(t *testing.T) { rsp <- e } - client := NewClient("someClient") + client := api.GetNamedClient("someClient") client.AddHandler(ProviderReady, &callback) select { @@ -1357,7 +1356,6 @@ func TestEventHandler_1ToNMapping(t *testing.T) { } // Contract tests - registration & removal - func TestEventHandler_Registration(t *testing.T) { t.Run("API handlers", func(t *testing.T) { executor := newEventExecutor() diff --git a/openfeature/openfeature_test.go b/openfeature/openfeature_test.go index 922f0cc..d230847 100644 --- a/openfeature/openfeature_test.go +++ b/openfeature/openfeature_test.go @@ -1,7 +1,9 @@ package openfeature import ( + "context" "errors" + "fmt" "reflect" "testing" "time" @@ -718,6 +720,47 @@ func TestDefaultClientUsage(t *testing.T) { } } +func TestLateBindingOfDefaultProvider(t *testing.T) { + // we are expecting + expectedResultUnboundProvider := "default-value-from-unbound-provider" + expectedResultFromLateDefaultProvider := "value-from-late-default-provider" + + ctrl := gomock.NewController(t) + defaultProvider := NewMockFeatureProvider(ctrl) + defaultProvider.EXPECT().Metadata().Return(Metadata{Name: "defaultClientReplacement"}).AnyTimes() + defaultProvider.EXPECT().Hooks().AnyTimes().Return([]Hook{}) + defaultProvider.EXPECT().StringEvaluation(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(StringResolutionDetail{Value: expectedResultFromLateDefaultProvider}) + + client := NewClient("app") + strResult, err := client.StringValue(context.TODO(), "flag", expectedResultUnboundProvider, EvaluationContext{}) + if err != nil { + if err != nil { + t.Errorf("flag evaluation failed %v", err) + } + } + + if strResult != expectedResultUnboundProvider { + t.Errorf("expected %s, but got %s", expectedResultUnboundProvider, strResult) + } + + err = SetProviderAndWait(defaultProvider) + if err != nil { + t.Errorf("provider registration failed %v", err) + } + + strResult, err = client.StringValue(context.TODO(), "flag", "default", EvaluationContext{}) + if err != nil { + if err != nil { + t.Errorf("flag evaluation failed %v", err) + } + } + + if strResult != expectedResultFromLateDefaultProvider { + t.Errorf("expected %s, but got %s", expectedResultFromLateDefaultProvider, strResult) + } + fmt.Println(strResult) +} + // Nil providers are not accepted for default and named providers func TestForNilProviders(t *testing.T) { defer t.Cleanup(initSingleton)