diff --git a/pkg/client/factory_test.go b/pkg/client/factory_test.go index 46909efe2..b23cc0dcc 100644 --- a/pkg/client/factory_test.go +++ b/pkg/client/factory_test.go @@ -151,7 +151,7 @@ func TestClientWithCustomDecisionServiceOptions(t *testing.T) { factory := OptimizelyFactory{SDKKey: "1212"} mockUserProfileService := new(MockUserProfileService) - mockOverrideStore := new(decision.MapOverridesStore) + mockOverrideStore := new(decision.MapExperimentOverridesStore) optimizelyClient, err := factory.Client( WithUserProfileService(mockUserProfileService), WithExperimentOverrides(mockOverrideStore), diff --git a/pkg/decision/composite_experiment_service_test.go b/pkg/decision/composite_experiment_service_test.go index b934da16f..fad1ce0d0 100644 --- a/pkg/decision/composite_experiment_service_test.go +++ b/pkg/decision/composite_experiment_service_test.go @@ -160,7 +160,7 @@ func (s *CompositeExperimentTestSuite) TestNewCompositeExperimentService() { func (s *CompositeExperimentTestSuite) TestNewCompositeExperimentServiceWithCustomOptions() { mockUserProfileService := new(MockUserProfileService) - mockExperimentOverrideStore := new(MapOverridesStore) + mockExperimentOverrideStore := new(MapExperimentOverridesStore) compositeExperimentService := NewCompositeExperimentService( WithUserProfileService(mockUserProfileService), WithOverrideStore(mockExperimentOverrideStore), diff --git a/pkg/decision/experiment_override_service.go b/pkg/decision/experiment_override_service.go index 9386d1fc8..194a4c76c 100644 --- a/pkg/decision/experiment_override_service.go +++ b/pkg/decision/experiment_override_service.go @@ -20,6 +20,7 @@ package decision import ( "errors" "fmt" + "sync" "github.com/optimizely/go-sdk/pkg/decision/reasons" "github.com/optimizely/go-sdk/pkg/entities" @@ -39,17 +40,27 @@ type ExperimentOverrideStore interface { GetVariation(overrideKey ExperimentOverrideKey) (string, bool) } -// MapOverridesStore is a map-based implementation of OverrideStore -type MapOverridesStore struct { +// MapExperimentOverridesStore is a map-based implementation of ExperimentOverridesStore that is safe to use concurrently +type MapExperimentOverridesStore struct { overridesMap map[ExperimentOverrideKey]string + mutex sync.RWMutex } -// GetVariation returns the override associated with the given key in the map -func (m *MapOverridesStore) GetVariation(overrideKey ExperimentOverrideKey) (string, bool) { +// GetVariation returns the override variation key associated with the given user+experiment key +func (m *MapExperimentOverridesStore) GetVariation(overrideKey ExperimentOverrideKey) (string, bool) { + m.mutex.RLock() variationKey, ok := m.overridesMap[overrideKey] + m.mutex.RUnlock() return variationKey, ok } +// SetVariation sets the given variation key as an override for the given user+experiment key +func (m *MapExperimentOverridesStore) SetVariation(overrideKey ExperimentOverrideKey, variationKey string) { + m.mutex.Lock() + m.overridesMap[overrideKey] = variationKey + m.mutex.Unlock() +} + // ExperimentOverrideService makes a decision using an ExperimentOverridesStore // Implements the ExperimentService interface type ExperimentOverrideService struct { diff --git a/pkg/decision/experiment_override_service_test.go b/pkg/decision/experiment_override_service_test.go index ca50b84e1..26825db74 100644 --- a/pkg/decision/experiment_override_service_test.go +++ b/pkg/decision/experiment_override_service_test.go @@ -18,6 +18,7 @@ package decision import ( + "sync" "testing" "github.com/optimizely/go-sdk/pkg/decision/reasons" @@ -28,16 +29,16 @@ import ( type ExperimentOverrideServiceTestSuite struct { suite.Suite mockConfig *mockProjectConfig - overrides map[ExperimentOverrideKey]string + overrides *MapExperimentOverridesStore overrideService *ExperimentOverrideService } func (s *ExperimentOverrideServiceTestSuite) SetupTest() { s.mockConfig = new(mockProjectConfig) - s.overrides = make(map[ExperimentOverrideKey]string) - s.overrideService = NewExperimentOverrideService(&MapOverridesStore{ - overridesMap: s.overrides, - }) + s.overrides = &MapExperimentOverridesStore{ + overridesMap: make(map[ExperimentOverrideKey]string), + } + s.overrideService = NewExperimentOverrideService(s.overrides) } func (s *ExperimentOverrideServiceTestSuite) TestOverridesIncludeVariation() { @@ -48,7 +49,7 @@ func (s *ExperimentOverrideServiceTestSuite) TestOverridesIncludeVariation() { testUserContext := entities.UserContext{ ID: "test_user_1", } - s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}] = testExp1111Var2222.Key + s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}, testExp1111Var2222.Key) decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) s.NoError(err) s.NotNil(decision.Variation) @@ -77,7 +78,7 @@ func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForExperiment() { ID: "test_user_1", } // The decision context refers to testExp1111, but this override is for another experiment - s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1113.Key, UserID: "test_user_1"}] = testExp1113Var2224.Key + s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1113.Key, UserID: "test_user_1"}, testExp1113Var2224.Key) decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) s.NoError(err) s.Nil(decision.Variation) @@ -93,7 +94,7 @@ func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForUser() { ID: "test_user_1", } // The user context refers to "test_user_1", but this override is for another user - s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_2"}] = testExp1111Var2222.Key + s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_2"}, testExp1111Var2222.Key) decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) s.NoError(err) s.Nil(decision.Variation) @@ -109,7 +110,7 @@ func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForUserOrExperiment() ID: "test_user_1", } // This override is for both a different user and a different experiment than the ones in the contexts above - s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1113.Key, UserID: "test_user_3"}] = testExp1111Var2222.Key + s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1113.Key, UserID: "test_user_3"}, testExp1111Var2222.Key) decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) s.NoError(err) s.Nil(decision.Variation) @@ -125,13 +126,69 @@ func (s *ExperimentOverrideServiceTestSuite) TestInvalidVariationInOverride() { ID: "test_user_1", } // This override variation key does not exist in the experiment - s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}] = "invalid_variation_key" + s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}, "invalid_variation_key") decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) s.NoError(err) s.Nil(decision.Variation) s.Exactly(reasons.InvalidOverrideVariationAssignment, decision.Reason) } +// Test concurrent use of the MapExperimentOverrideStore +// Create 3 goroutines that set and get variations, and assert that all their sets take effect at the end +func (s *ExperimentOverrideServiceTestSuite) TestMapExperimentOverridesStoreConcurrent() { + testDecisionContext := ExperimentDecisionContext{ + Experiment: &testExp1111, + ProjectConfig: s.mockConfig, + } + var wg sync.WaitGroup + wg.Add(1) + go func() { + s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}, testExp1111Var2222.Key) + user1Decision, _ := s.overrideService.GetDecision(testDecisionContext, entities.UserContext{ + ID: "test_user_1", + }) + s.NotNil(user1Decision.Variation) + s.Exactly(testExp1111Var2222.Key, user1Decision.Variation.Key) + wg.Done() + }() + wg.Add(1) + go func() { + s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_2"}, testExp1111Var2222.Key) + user2Decision, _ := s.overrideService.GetDecision(testDecisionContext, entities.UserContext{ + ID: "test_user_2", + }) + s.NotNil(user2Decision.Variation) + s.Exactly(testExp1111Var2222.Key, user2Decision.Variation.Key) + wg.Done() + }() + wg.Add(1) + go func() { + s.overrides.SetVariation(ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_3"}, testExp1111Var2222.Key) + user3Decision, _ := s.overrideService.GetDecision(testDecisionContext, entities.UserContext{ + ID: "test_user_3", + }) + s.NotNil(user3Decision.Variation) + s.Exactly(testExp1111Var2222.Key, user3Decision.Variation.Key) + wg.Done() + }() + wg.Wait() + user1Decision, _ := s.overrideService.GetDecision(testDecisionContext, entities.UserContext{ + ID: "test_user_1", + }) + user2Decision, _ := s.overrideService.GetDecision(testDecisionContext, entities.UserContext{ + ID: "test_user_2", + }) + user3Decision, _ := s.overrideService.GetDecision(testDecisionContext, entities.UserContext{ + ID: "test_user_3", + }) + s.NotNil(user1Decision.Variation) + s.Exactly(testExp1111Var2222.Key, user1Decision.Variation.Key) + s.NotNil(user2Decision.Variation) + s.Exactly(testExp1111Var2222.Key, user2Decision.Variation.Key) + s.NotNil(user3Decision.Variation) + s.Exactly(testExp1111Var2222.Key, user3Decision.Variation.Key) +} + func TestExperimentOverridesTestSuite(t *testing.T) { suite.Run(t, new(ExperimentOverrideServiceTestSuite)) }