Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Blocking provider mutator #244

Closed
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added openfeature/__debug_bin1610825419
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this file

Binary file not shown.
100 changes: 75 additions & 25 deletions openfeature/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,31 @@ func (api *evaluationAPI) setProvider(provider FeatureProvider) error {
oldProvider := api.defaultProvider
api.defaultProvider = provider

api.initNewAndShutdownOld(provider, oldProvider)
api.initNewAndShutdownOldAsync(provider, oldProvider)
err := api.eventExecutor.registerDefaultProvider(provider)
if err != nil {
return err
}

return nil
}

// setProviderAndWait sets the default FeatureProvider of the evaluationAPI. Returns an error if FeatureProvider is nil
// This is a blocking call and will wait for the provider to be ready
func (api *evaluationAPI) setProviderAndWait(provider FeatureProvider) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: to stay consistent with the naming i would suggest to call this function setProviderSync

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. The spec doesn't dictate function names. I think your suggestion makes sense here.

api.mu.Lock()
defer api.mu.Unlock()

if provider == nil {
return errors.New("default provider cannot be set to nil")
}

// Initialize new default provider and shutdown the old one
// Provider update must be non-blocking, hence initialization & shutdown happens concurrently
oldProvider := api.defaultProvider
api.defaultProvider = provider

api.initNewAndShutdownOldSync(provider, oldProvider)
err := api.eventExecutor.registerDefaultProvider(provider)
if err != nil {
return err
Expand Down Expand Up @@ -80,7 +104,7 @@ func (api *evaluationAPI) setNamedProvider(clientName string, provider FeaturePr
oldProvider := api.namedProviders[clientName]
api.namedProviders[clientName] = provider

api.initNewAndShutdownOld(provider, oldProvider)
api.initNewAndShutdownOldAsync(provider, oldProvider)
err := api.eventExecutor.registerNamedEventingProvider(clientName, provider)
if err != nil {
return err
Expand Down Expand Up @@ -166,32 +190,35 @@ func (api *evaluationAPI) forTransaction(clientName string) (FeatureProvider, []
return provider, api.hks, api.evalCtx
}

// initNewAndShutdownOld is a helper to initialise new FeatureProvider and shutdown the old FeatureProvider.
// initAndNotify is a helper to initialise FeatureProvider and notify the event executor.
func initAndNotify(provider FeatureProvider, stateHandler StateHandler, evalCtx EvaluationContext, eventChan chan eventPayload) {
err := stateHandler.Init(evalCtx)
// emit ready/error event once initialization is complete
if err != nil {
eventChan <- eventPayload{
Event{
ProviderName: provider.Metadata().Name,
EventType: ProviderError,
ProviderEventDetails: ProviderEventDetails{},
}, provider,
}
} else {
eventChan <- eventPayload{
Event{
ProviderName: provider.Metadata().Name,
EventType: ProviderReady,
ProviderEventDetails: ProviderEventDetails{},
}, provider,
}
}
}

// initNewAndShutdownOldAsync is a helper to initialise new FeatureProvider and shutdown the old FeatureProvider.
// Operations happen concurrently.
func (api *evaluationAPI) initNewAndShutdownOld(newProvider FeatureProvider, oldProvider FeatureProvider) {
func (api *evaluationAPI) initNewAndShutdownOldAsync(newProvider FeatureProvider, oldProvider FeatureProvider) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may refactor to combine initNewAndShutdownOldAsync & initNewAndShutdownOldSync as they perform similar set of operations with only difference being sync vs async.

v, ok := newProvider.(StateHandler)
if ok && v.Status() == NotReadyState {
go func(provider FeatureProvider, stateHandler StateHandler, evalCtx EvaluationContext, eventChan chan eventPayload) {
err := stateHandler.Init(evalCtx)
// emit ready/error event once initialization is complete
if err != nil {
eventChan <- eventPayload{
Event{
ProviderName: provider.Metadata().Name,
EventType: ProviderError,
ProviderEventDetails: ProviderEventDetails{},
}, provider,
}
} else {
eventChan <- eventPayload{
Event{
ProviderName: provider.Metadata().Name,
EventType: ProviderReady,
ProviderEventDetails: ProviderEventDetails{},
}, provider,
}
}
}(newProvider, v, api.evalCtx, api.eventExecutor.eventChan)
go initAndNotify(newProvider, v, api.evalCtx, api.eventExecutor.eventChan)
}

v, ok = oldProvider.(StateHandler)
Expand All @@ -211,6 +238,29 @@ func (api *evaluationAPI) initNewAndShutdownOld(newProvider FeatureProvider, old
}(v)
}

// initNewAndShutdownOldSync is a helper to initialise new FeatureProvider and shutdown the old FeatureProvider.
// Operations happen synchronously.
func (api *evaluationAPI) initNewAndShutdownOldSync(newProvider FeatureProvider, oldProvider FeatureProvider) {
v, ok := newProvider.(StateHandler)
if ok && v.Status() == NotReadyState {
initAndNotify(newProvider, v, api.evalCtx, api.eventExecutor.eventChan)
}

v, ok = oldProvider.(StateHandler)

// oldProvider can be nil or without state handling capability
if oldProvider == nil || !ok {
return
}

// check for multiple bindings
if oldProvider == api.defaultProvider || contains(oldProvider, maps.Values(api.namedProviders)) {
return
}

v.Shutdown()
}

func contains(provider FeatureProvider, in []FeatureProvider) bool {
for _, p := range in {
if provider == p {
Expand Down
81 changes: 81 additions & 0 deletions openfeature/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,87 @@ func TestRequirement_1_4_13(t *testing.T) {
// Is tested by TestRequirement_4_4_2.

// TODO Requirement_1_6_1

func TestWaitForProvider(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to add a test that works like this:

  • set a provider which is ready after 1s (has an init function that returns after 1s)
  • returns a valid flag value after that time only
  • returns an error before 1s

This is consistent with how we'd expect many providers to work. Maybe you could add some options to NewMockFeatureProvider that would support this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, I got the idea. I'll give it a try, but since I'm new to Go, it might take me some time 😇 .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had a look at the test, and to make sure the Init function actually gets called, you will need to have a type that consolidates a MockFeatureProvider and MockStateHandler - otherwise the check here will fail and the function will proceed without calling the Init function of the provider.

Since gomock, which we use for creating the mocks seems to not support the generation of a type that implements multiple interfaces, i believe the simplest way to get a struct that implements both the FeatureProvider and the StateHandler interfaces is to consolidate them into a single struct, such as follows:

type mockProviderAndStateHandler struct {
	*MockFeatureProvider
	*MockStateHandler
}

This type should be defined in the client_test.go file, and can then be used to pass it to the SetProviderAndWait function, such as in the following example:

...
		mockProvider.EXPECT().Metadata().AnyTimes()
		mockProvider.EXPECT().Hooks().AnyTimes()
		mockProvider.EXPECT().IntEvaluation(context.Background(), flagKey, defaultValue, flatCtx).
			Return(IntResolutionDetail{
				Value: 0,
				ProviderResolutionDetail: ProviderResolutionDetail{
					ResolutionError: NewGeneralResolutionError("test"),
				},
			}).Times(2)

		mockStateHandler := NewMockStateHandler(ctrl)

                // TODO add the desired behavior for the Init function of the MockStateHandler

		o := & mockProviderAndStateHandler{
			MockFeatureProvider: mockProvider,
			MockStateHandler:    mockStateHandler,
		}

		err := SetProviderAndWait(o)
...

client := NewClient("test-client")
flagKey := "flag-key"
evalCtx := EvaluationContext{}
flatCtx := flattenContext(evalCtx)

ctrl := gomock.NewController(t)
t.Run("Int Wait Provider", func(t *testing.T) {
defer t.Cleanup(initSingleton)
mockProvider := NewMockFeatureProvider(ctrl)
var defaultValue int64 = 3
mockProvider.EXPECT().Metadata().AnyTimes()
mockProvider.EXPECT().Hooks().AnyTimes()
mockProvider.EXPECT().IntEvaluation(context.Background(), flagKey, defaultValue, flatCtx).
Return(IntResolutionDetail{
Value: 0,
ProviderResolutionDetail: ProviderResolutionDetail{
ResolutionError: NewGeneralResolutionError("test"),
},
}).Times(2)

err := SetProviderAndWait(mockProvider)
if err != nil {
t.Errorf("error setting up provider %v", err)
}

value, err := client.IntValue(context.Background(), flagKey, defaultValue, evalCtx)
if err == nil {
t.Error("expected IntValue to return an error, got nil")
}

if value != defaultValue {
t.Errorf("expected default value from IntValue, got %v", value)
}

valueDetails, err := client.IntValueDetails(context.Background(), flagKey, defaultValue, evalCtx)
if err == nil {
t.Error("expected FloatValueDetails to return an error, got nil")
}

if valueDetails.Value != defaultValue {
t.Errorf("expected default value from IntValueDetails, got %v", value)
}
})
}

func TestWaitForProviderTimeout(t *testing.T) {
defer t.Cleanup(initSingleton)
client := NewClient("test-client")

ctrl := gomock.NewController(t)
mockProvider := NewMockFeatureProvider(ctrl)
mockProvider.EXPECT().Metadata().AnyTimes()
mockProvider.EXPECT().Hooks().AnyTimes()
mockProvider.EXPECT().BooleanEvaluation(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
Return(BoolResolutionDetail{
Value: false,
ProviderResolutionDetail: ProviderResolutionDetail{
ResolutionError: NewGeneralResolutionError("test"),
},
})

err := SetProviderAndWait(mockProvider)
if err != nil {
t.Errorf("error setting up provider %v", err)
}

res, err := client.evaluate(
context.Background(), "foo", Boolean, true, EvaluationContext{}, EvaluationOptions{},
)
if err == nil {
t.Error("expected err, got nil")
}

expectedErrorCode := GeneralCode
if res.ErrorCode != expectedErrorCode {
t.Errorf("expected error code to be '%s', got '%s'", expectedErrorCode, res.ErrorCode)
}
}

// The `client` SHOULD transform the `evaluation context` using the `provider's` `context transformer` function
// if one is defined, before passing the result of the transformation to the provider's flag resolution functions.

Expand Down
8 changes: 7 additions & 1 deletion openfeature/openfeature.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,18 @@ func initSingleton() {
api = newEvaluationAPI()
}

// SetProvider sets the default provider. Provider initialization is asynchronous and status can be checked from
// SetProvider sets the default provider. Provider initialization is asynchronous (non-blocking) and status can be checked from
// provider status
func SetProvider(provider FeatureProvider) error {
return api.setProvider(provider)
}

// SetProviderAndWait sets the default provider. Provider initialization is synchronous (blocking) and status can be checked from
// provider status
func SetProviderAndWait(provider FeatureProvider) error {
znonthedev marked this conversation as resolved.
Show resolved Hide resolved
return api.setProviderAndWait(provider)
}

// SetNamedProvider sets a provider mapped to the given Client name. Provider initialization is asynchronous and
// status can be checked from provider status
func SetNamedProvider(clientName string, provider FeatureProvider) error {
Expand Down
Loading