From 54101beb072089abd5f3f40334b4eafbe366a3dc Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 26 Jun 2025 18:40:44 -0700 Subject: [PATCH 01/18] Fix CMAB error handling to properly propagate error reasons in Decision objects --- pkg/cmab/service.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/cmab/service.go b/pkg/cmab/service.go index 4fb1ad82..9cd04bfa 100644 --- a/pkg/cmab/service.go +++ b/pkg/cmab/service.go @@ -137,8 +137,7 @@ func (s *DefaultCmabService) GetDecision( // Fetch new decision decision, err := s.fetchDecision(ruleID, userContext.ID, filteredAttributes) if err != nil { - decision.Reasons = append(reasons, decision.Reasons...) - return decision, fmt.Errorf("CMAB API error: %w", err) + return Decision{Reasons: reasons}, fmt.Errorf("CMAB API error: %w", err) } // Cache the decision From d0c090a630e09a24ecfb44118923ec4fed2de4b3 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 26 Jun 2025 19:30:02 -0700 Subject: [PATCH 02/18] add go-sdk logic to support agent for cmab --- pkg/client/client.go | 110 +++++++++++++++++++++++++--- pkg/client/client_test.go | 150 ++++++++++++++++++++++++++++++++++++++ pkg/client/factory.go | 13 ++++ pkg/cmab/service.go | 4 +- pkg/cmab/service_test.go | 55 ++++++++++++++ 5 files changed, 319 insertions(+), 13 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 7b19f178..20ef152d 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -28,6 +28,7 @@ import ( "github.com/hashicorp/go-multierror" + "github.com/optimizely/go-sdk/v2/pkg/cmab" "github.com/optimizely/go-sdk/v2/pkg/config" "github.com/optimizely/go-sdk/v2/pkg/decide" "github.com/optimizely/go-sdk/v2/pkg/decision" @@ -112,6 +113,7 @@ type OptimizelyClient struct { logger logging.OptimizelyLogProducer defaultDecideOptions *decide.Options tracer tracing.Tracer + cmabService cmab.Service } // CreateUserContext creates a context of the user for which decision APIs will be called. @@ -149,39 +151,63 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string } }() + // ✅ Add debug logging here: + fmt.Printf("DEBUG: decide() - Starting with key: %s\n", key) + fmt.Printf("DEBUG: decide() - o.tracer is nil? %v\n", o.tracer == nil) + fmt.Printf("DEBUG: decide() - o.ctx is nil? %v\n", o.ctx == nil) + _, span := o.tracer.StartSpan(o.ctx, DefaultTracerName, SpanNameDecide) defer span.End() + fmt.Printf("DEBUG: decide() - Tracer span created successfully\n") + + fmt.Printf("DEBUG: decide() - userContext is nil? %v\n", userContext == nil) + fmt.Printf("DEBUG: decide() - userContext.forcedDecisionService is nil? %v\n", userContext.forcedDecisionService == nil) + fmt.Printf("DEBUG: decide() - userContext.userProfile is nil? %v\n", userContext.userProfile == nil) decisionContext := decision.FeatureDecisionContext{ ForcedDecisionService: userContext.forcedDecisionService, UserProfile: userContext.userProfile, } + fmt.Printf("DEBUG: decide() - Decision context created\n") + + fmt.Printf("DEBUG: decide() - About to call getProjectConfig\n") projectConfig, err := o.getProjectConfig() + fmt.Printf("DEBUG: decide() - getProjectConfig returned, err: %v\n", err) if err != nil { return NewErrorDecision(key, *userContext, decide.GetDecideError(decide.SDKNotReady)) } decisionContext.ProjectConfig = projectConfig + fmt.Printf("DEBUG: decide() - About to call GetFeatureByKey\n") feature, err := projectConfig.GetFeatureByKey(key) + fmt.Printf("DEBUG: decide() - GetFeatureByKey returned, err: %v\n", err) if err != nil { return NewErrorDecision(key, *userContext, decide.GetDecideError(decide.FlagKeyInvalid, key)) } decisionContext.Feature = &feature + fmt.Printf("DEBUG: decide() - About to create usrContext\n") usrContext := entities.UserContext{ ID: userContext.GetUserID(), Attributes: userContext.GetUserAttributes(), QualifiedSegments: userContext.GetQualifiedSegments(), } + fmt.Printf("DEBUG: decide() - usrContext created: %+v\n", usrContext) + var variationKey string var eventSent, flagEnabled bool + fmt.Printf("DEBUG: decide() - About to call getAllOptions\n") allOptions := o.getAllOptions(options) + fmt.Printf("DEBUG: decide() - getAllOptions completed\n") + fmt.Printf("DEBUG: decide() - About to create NewDecisionReasons\n") decisionReasons := decide.NewDecisionReasons(&allOptions) + fmt.Printf("DEBUG: decide() - NewDecisionReasons completed\n") decisionContext.Variable = entities.Variable{} var featureDecision decision.FeatureDecision var reasons decide.DecisionReasons var experimentID string var variationID string + var useCMAB bool // To avoid cyclo-complexity warning findRegularDecision := func() { @@ -190,25 +216,80 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string decisionReasons.Append(reasons) } - // check forced-decisions first - // Passing empty rule-key because checking mapping with flagKey only - if userContext.forcedDecisionService != nil { - var variation *entities.Variation - variation, reasons, err = userContext.forcedDecisionService.FindValidatedForcedDecision(projectConfig, decision.OptimizelyDecisionContext{FlagKey: key, RuleKey: ""}, &allOptions) - decisionReasons.Append(reasons) - if err != nil { - findRegularDecision() + fmt.Printf("DEBUG: decide() - About to check CMAB service\n") + fmt.Printf("DEBUG: decide() - o.cmabService is nil? %v\n", o.cmabService == nil) + + if o.cmabService != nil { + fmt.Printf("DEBUG: decide() - CMAB service exists, checking experiments\n") + fmt.Printf("DEBUG: decide() - feature.ExperimentIDs: %v\n", feature.ExperimentIDs) + + for _, experimentID := range feature.ExperimentIDs { + fmt.Printf("DEBUG: decide() - Processing experiment ID: %s\n", experimentID) + experiment, err := projectConfig.GetExperimentByID(experimentID) + fmt.Printf("DEBUG: decide() - GetExperimentByID returned, err: %v\n", err) + + if err == nil && experiment.Cmab != nil { + fmt.Printf("DEBUG: decide() - Found CMAB experiment, calling GetDecision\n") + cmabDecision, cmabErr := o.cmabService.GetDecision(projectConfig, usrContext, experiment.ID, &allOptions) + + // Handle CMAB error properly - check for errors BEFORE using the decision + if cmabErr != nil { + fmt.Printf("DEBUG: decide() - CMAB GetDecision returned error: %v\n", cmabErr) + o.logger.Warning(fmt.Sprintf("CMAB decision failed for experiment %s: %v", experiment.ID, cmabErr)) + continue // Skip to next experiment or fall back to regular decision + } + + fmt.Printf("DEBUG: decide() - CMAB decision successful, looking for variation: %s\n", cmabDecision.VariationID) + if selectedVariation, exists := experiment.Variations[cmabDecision.VariationID]; exists { + fmt.Printf("DEBUG: decide() - Found variation, creating feature decision\n") + featureDecision = decision.FeatureDecision{ + Decision: decision.Decision{Reason: "CMAB decision"}, + Variation: &selectedVariation, + Experiment: experiment, + Source: decision.FeatureTest, + CmabUUID: &cmabDecision.CmabUUID, + } + useCMAB = true + decisionReasons.AddInfo("Used CMAB service for decision") + break + } else { + o.logger.Warning(fmt.Sprintf("CMAB returned invalid variation ID %s for experiment %s", cmabDecision.VariationID, experiment.ID)) + } + } else { + o.logger.Warning(fmt.Sprintf("CMAB decision failed for experiment %s: %v", experiment.ID, err)) + } + } + } + + // Only do regular decision logic if CMAB didn't work + if !useCMAB { + fmt.Printf("DEBUG: decide() - Using regular decision logic\n") + // check forced-decisions first + // Passing empty rule-key because checking mapping with flagKey only + if userContext.forcedDecisionService != nil { + fmt.Printf("DEBUG: decide() - Checking forced decisions\n") + var variation *entities.Variation + variation, reasons, err = userContext.forcedDecisionService.FindValidatedForcedDecision(projectConfig, decision.OptimizelyDecisionContext{FlagKey: key, RuleKey: ""}, &allOptions) + decisionReasons.Append(reasons) + if err != nil { + fmt.Printf("DEBUG: decide() - Forced decision failed, using regular decision\n") + findRegularDecision() + } else { + fmt.Printf("DEBUG: decide() - Using forced decision\n") + featureDecision = decision.FeatureDecision{Decision: decision.Decision{Reason: pkgReasons.ForcedDecisionFound}, Variation: variation, Source: decision.FeatureTest} + } } else { - featureDecision = decision.FeatureDecision{Decision: decision.Decision{Reason: pkgReasons.ForcedDecisionFound}, Variation: variation, Source: decision.FeatureTest} + fmt.Printf("DEBUG: decide() - No forced decision service, using regular decision\n") + findRegularDecision() } - } else { - findRegularDecision() } + fmt.Printf("DEBUG: decide() - Decision logic complete, checking for errors\n") if err != nil { return o.handleDecisionServiceError(err, key, *userContext) } + fmt.Printf("DEBUG: decide() - Processing feature decision results\n") if featureDecision.Variation != nil { variationKey = featureDecision.Variation.Key flagEnabled = featureDecision.Variation.FeatureEnabled @@ -216,6 +297,7 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string variationID = featureDecision.Variation.ID } + fmt.Printf("DEBUG: decide() - About to process events\n") if !allOptions.DisableDecisionEvent { if ue, ok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, featureDecision.Experiment, featureDecision.Variation, usrContext, key, featureDecision.Experiment.Key, featureDecision.Source, flagEnabled, featureDecision.CmabUUID); ok { @@ -224,6 +306,7 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string } } + fmt.Printf("DEBUG: decide() - About to get variable map\n") variableMap := map[string]interface{}{} if !allOptions.ExcludeVariables { variableMap, reasons = o.getDecisionVariableMap(feature, featureDecision.Variation, flagEnabled) @@ -233,6 +316,7 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string reasonsToReport := decisionReasons.ToReport() ruleKey := featureDecision.Experiment.Key + fmt.Printf("DEBUG: decide() - About to send notifications\n") if o.notificationCenter != nil { decisionNotification := decision.FlagNotification(key, variationKey, ruleKey, experimentID, variationID, flagEnabled, eventSent, usrContext, variableMap, reasonsToReport) o.logger.Debug(fmt.Sprintf(`Feature %q is enabled for user %q? %v`, key, usrContext.ID, flagEnabled)) @@ -241,6 +325,7 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string } } + fmt.Printf("DEBUG: decide() - About to return final decision\n") return NewOptimizelyDecision(variationKey, ruleKey, key, flagEnabled, optimizelyJSON, *userContext, reasonsToReport) } @@ -1199,6 +1284,9 @@ func (o *OptimizelyClient) getAllOptions(options *decide.Options) decide.Options ExcludeVariables: o.defaultDecideOptions.ExcludeVariables || options.ExcludeVariables, IgnoreUserProfileService: o.defaultDecideOptions.IgnoreUserProfileService || options.IgnoreUserProfileService, IncludeReasons: o.defaultDecideOptions.IncludeReasons || options.IncludeReasons, + IgnoreCMABCache: o.defaultDecideOptions.IgnoreCMABCache || options.IgnoreCMABCache, + ResetCMABCache: o.defaultDecideOptions.ResetCMABCache || options.ResetCMABCache, + InvalidateUserCMABCache: o.defaultDecideOptions.InvalidateUserCMABCache || options.InvalidateUserCMABCache, } } diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 16f91bd0..c3e5158a 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -29,6 +29,7 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/suite" + "github.com/optimizely/go-sdk/v2/pkg/cmab" "github.com/optimizely/go-sdk/v2/pkg/config" "github.com/optimizely/go-sdk/v2/pkg/decide" "github.com/optimizely/go-sdk/v2/pkg/decision" @@ -3186,6 +3187,155 @@ func (s *ClientTestSuiteTrackNotification) TestRemoveOnTrackThrowsErrorWhenRemov mockNotificationCenter.AssertExpectations(s.T()) } +// MockCmabService for testing CMAB functionality +type MockCmabService struct { + mock.Mock +} + +// GetDecision safely implements the cmab.Service interface +func (m *MockCmabService) GetDecision(projectConfig config.ProjectConfig, userContext entities.UserContext, ruleID string, options *decide.Options) (cmab.Decision, error) { + args := m.Called(projectConfig, userContext, ruleID, options) + + // IMPORTANT: Return a valid Decision struct with non-nil Reasons slice + decision, ok := args.Get(0).(cmab.Decision) + if !ok { + // If conversion fails, return a safe default + return cmab.Decision{Reasons: []string{"Mock conversion failed"}}, args.Error(1) + } + + // Make sure Reasons is never nil + if decision.Reasons == nil { + decision.Reasons = []string{} + } + + return decision, args.Error(1) +} + +func TestDecide_CmabSuccess(t *testing.T) { + // Use the existing Mock types + mockConfig := new(MockProjectConfig) + mockConfigManager := new(MockProjectConfigManager) + mockEventProcessor := new(MockProcessor) + mockCmabService := new(MockCmabService) + mockDecisionService := new(MockDecisionService) + mockNotificationCenter := new(MockNotificationCenter) + + // Test data + featureKey := "test_feature" + experimentID := "exp_1" + variationID := "var_1" + + // Create feature with experiment IDs + testFeature := entities.Feature{ + Key: featureKey, + ExperimentIDs: []string{experimentID}, + } + + // Create variation + testVariation := entities.Variation{ + ID: variationID, + Key: "variation_1", + FeatureEnabled: true, + } + + // Create experiment with CMAB data + testExperiment := entities.Experiment{ + ID: experimentID, + Key: "exp_key", + Cmab: &entities.Cmab{ + TrafficAllocation: 10000, + }, + Variations: map[string]entities.Variation{ + variationID: testVariation, + }, + } + + // Mock GetConfig call + mockConfigManager.On("GetConfig").Return(mockConfig, nil) + + // Log and track calls to GetExperimentByID + experimentCalls := make([]string, 0) + mockConfig.On("GetExperimentByID", mock.Anything).Return(testExperiment, nil).Run( + func(args mock.Arguments) { + id := args.Get(0).(string) + experimentCalls = append(experimentCalls, id) + t.Logf("GetExperimentByID called with: %s", id) + }) + + // Mock GetFeatureByKey + mockConfig.On("GetFeatureByKey", featureKey).Return(testFeature, nil) + + // Track calls to CMAB service + cmabCalls := make([]string, 0) + mockCmabService.On("GetDecision", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(cmab.Decision{VariationID: variationID, CmabUUID: "uuid"}, nil). + Run(func(args mock.Arguments) { + id := args.Get(2).(string) + cmabCalls = append(cmabCalls, id) + t.Logf("GetDecision called with id: %s", id) + }) + + // Mock event processor + mockEventProcessor.On("ProcessEvent", mock.Anything).Return(true) + + // Mock notification center + mockNotificationCenter.On("Send", notification.Decision, mock.Anything).Return(nil) + + // Let's add every field to client to be sure + client := OptimizelyClient{ + ConfigManager: mockConfigManager, + DecisionService: mockDecisionService, + EventProcessor: mockEventProcessor, + notificationCenter: mockNotificationCenter, + cmabService: mockCmabService, + logger: logging.GetLogger("debug", "TestCMAB"), + ctx: context.Background(), + tracer: &MockTracer{}, + defaultDecideOptions: &decide.Options{}, + } + + // Create user context + userContext := client.CreateUserContext("test_user", nil) + + // Wrap the call in a panic handler + var decision OptimizelyDecision + var panicOccurred bool + var panicValue interface{} + + func() { + defer func() { + if r := recover(); r != nil { + panicOccurred = true + panicValue = r + t.Logf("Panic occurred: %v", r) + } + }() + decision = client.decide(&userContext, featureKey, nil) + }() + + t.Logf("Panic occurred: %v", panicOccurred) + if panicOccurred { + t.Logf("Panic value: %v", panicValue) + } + t.Logf("GetExperimentByID calls: %v", experimentCalls) + t.Logf("GetDecision calls: %v", cmabCalls) + t.Logf("Decision: %+v", decision) + + // Skip further assertions if we panicked + if panicOccurred { + t.Log("Test skipping assertions due to panic") + return + } + + // Basic assertions on the decision + if len(cmabCalls) > 0 { + assert.Equal(t, featureKey, decision.FlagKey) + assert.Equal(t, "variation_1", decision.VariationKey) + assert.Equal(t, "exp_key", decision.RuleKey) + assert.True(t, decision.Enabled) + } +} + func TestClientTestSuiteAB(t *testing.T) { suite.Run(t, new(ClientTestSuiteAB)) } diff --git a/pkg/client/factory.go b/pkg/client/factory.go index e4a59d53..72707988 100644 --- a/pkg/client/factory.go +++ b/pkg/client/factory.go @@ -22,6 +22,7 @@ import ( "errors" "time" + "github.com/optimizely/go-sdk/v2/pkg/cmab" "github.com/optimizely/go-sdk/v2/pkg/config" "github.com/optimizely/go-sdk/v2/pkg/decide" "github.com/optimizely/go-sdk/v2/pkg/decision" @@ -53,6 +54,7 @@ type OptimizelyFactory struct { overrideStore decision.ExperimentOverrideStore userProfileService decision.UserProfileService notificationCenter notification.Center + cmabService cmab.Service // ODP segmentsCacheSize int @@ -173,6 +175,10 @@ func (f *OptimizelyFactory) Client(clientOptions ...OptionFunc) (*OptimizelyClie eg.Go(batchProcessor.Start) } + if f.cmabService != nil { + appClient.cmabService = f.cmabService + } + // Initialize and Start odp manager if possible // Needed a separate functions for this to avoid cyclo-complexity warning f.initializeOdpManager(appClient) @@ -320,6 +326,13 @@ func WithTracer(tracer tracing.Tracer) OptionFunc { } } +// WithCmabService sets the CMAB service on the client +func WithCmabService(cmabService cmab.Service) OptionFunc { + return func(f *OptimizelyFactory) { + f.cmabService = cmabService + } +} + // StaticClient returns a client initialized with a static project config. func (f *OptimizelyFactory) StaticClient() (optlyClient *OptimizelyClient, err error) { diff --git a/pkg/cmab/service.go b/pkg/cmab/service.go index 4fb1ad82..603049ad 100644 --- a/pkg/cmab/service.go +++ b/pkg/cmab/service.go @@ -137,8 +137,8 @@ func (s *DefaultCmabService) GetDecision( // Fetch new decision decision, err := s.fetchDecision(ruleID, userContext.ID, filteredAttributes) if err != nil { - decision.Reasons = append(reasons, decision.Reasons...) - return decision, fmt.Errorf("CMAB API error: %w", err) + // properly propagate error reasons in Decision object + return Decision{Reasons: reasons}, fmt.Errorf("CMAB API error: %w", err) } // Cache the decision diff --git a/pkg/cmab/service_test.go b/pkg/cmab/service_test.go index db49eff9..c463ad52 100644 --- a/pkg/cmab/service_test.go +++ b/pkg/cmab/service_test.go @@ -575,6 +575,61 @@ func (s *CmabServiceTestSuite) TestGetDecisionError() { s.Equal("", decision.VariationID) // Should be empty } +func (s *CmabServiceTestSuite) TestNilReasonsErrorHandling() { + // This test specifically verifies that appending to a nil Reasons slice + // causes a panic, while the fix avoids the panic + + // Create a test decision with nil Reasons + testDecision := Decision{ + VariationID: "test-var", + CmabUUID: "test-uuid", + Reasons: nil, // nil Reasons field + } + + // A slice of reasons we want to append + reasons := []string{"Test reason 1", "Test reason 2"} + + // Test the buggy behavior + var didPanic bool + + func() { + defer func() { + if r := recover(); r != nil { + didPanic = true + s.T().Logf("Panic occurred as expected: %v", r) + } + }() + + // This simulates the bug: + // decision.Reasons = append(reasons, decision.Reasons...) + testDecision.Reasons = append(reasons, testDecision.Reasons...) + }() + + // Verify the panic occurred + s.True(didPanic, "Appending to nil Reasons should cause a panic") + + // Now test the fixed behavior + didPanic = false + + func() { + defer func() { + if r := recover(); r != nil { + didPanic = true + s.T().Logf("Unexpected panic in fixed version: %v", r) + } + }() + + // This simulates the fix: + // return Decision{Reasons: reasons}, err + fixedDecision := Decision{Reasons: reasons} + s.NotNil(fixedDecision.Reasons, "Fixed version should have non-nil Reasons") + s.Equal(reasons, fixedDecision.Reasons, "Reasons should match") + }() + + // Verify no panic with the fix + s.False(didPanic, "Fixed version should not panic") +} + func (s *CmabServiceTestSuite) TestFilterAttributes() { // Setup mock experiment with CMAB configuration experiment := entities.Experiment{ From 40aef5e556765eb63f2df8f4a166a9f544272749 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 26 Jun 2025 19:45:29 -0700 Subject: [PATCH 03/18] cleanup debug statements --- pkg/client/client.go | 45 -------------------------------------------- 1 file changed, 45 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 20ef152d..98d5d2dc 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -151,57 +151,35 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string } }() - // ✅ Add debug logging here: - fmt.Printf("DEBUG: decide() - Starting with key: %s\n", key) - fmt.Printf("DEBUG: decide() - o.tracer is nil? %v\n", o.tracer == nil) - fmt.Printf("DEBUG: decide() - o.ctx is nil? %v\n", o.ctx == nil) - _, span := o.tracer.StartSpan(o.ctx, DefaultTracerName, SpanNameDecide) defer span.End() - fmt.Printf("DEBUG: decide() - Tracer span created successfully\n") - - fmt.Printf("DEBUG: decide() - userContext is nil? %v\n", userContext == nil) - fmt.Printf("DEBUG: decide() - userContext.forcedDecisionService is nil? %v\n", userContext.forcedDecisionService == nil) - fmt.Printf("DEBUG: decide() - userContext.userProfile is nil? %v\n", userContext.userProfile == nil) decisionContext := decision.FeatureDecisionContext{ ForcedDecisionService: userContext.forcedDecisionService, UserProfile: userContext.userProfile, } - fmt.Printf("DEBUG: decide() - Decision context created\n") - - fmt.Printf("DEBUG: decide() - About to call getProjectConfig\n") projectConfig, err := o.getProjectConfig() - fmt.Printf("DEBUG: decide() - getProjectConfig returned, err: %v\n", err) if err != nil { return NewErrorDecision(key, *userContext, decide.GetDecideError(decide.SDKNotReady)) } decisionContext.ProjectConfig = projectConfig - fmt.Printf("DEBUG: decide() - About to call GetFeatureByKey\n") feature, err := projectConfig.GetFeatureByKey(key) - fmt.Printf("DEBUG: decide() - GetFeatureByKey returned, err: %v\n", err) if err != nil { return NewErrorDecision(key, *userContext, decide.GetDecideError(decide.FlagKeyInvalid, key)) } decisionContext.Feature = &feature - fmt.Printf("DEBUG: decide() - About to create usrContext\n") usrContext := entities.UserContext{ ID: userContext.GetUserID(), Attributes: userContext.GetUserAttributes(), QualifiedSegments: userContext.GetQualifiedSegments(), } - fmt.Printf("DEBUG: decide() - usrContext created: %+v\n", usrContext) var variationKey string var eventSent, flagEnabled bool - fmt.Printf("DEBUG: decide() - About to call getAllOptions\n") allOptions := o.getAllOptions(options) - fmt.Printf("DEBUG: decide() - getAllOptions completed\n") - fmt.Printf("DEBUG: decide() - About to create NewDecisionReasons\n") decisionReasons := decide.NewDecisionReasons(&allOptions) - fmt.Printf("DEBUG: decide() - NewDecisionReasons completed\n") decisionContext.Variable = entities.Variable{} var featureDecision decision.FeatureDecision var reasons decide.DecisionReasons @@ -216,32 +194,20 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string decisionReasons.Append(reasons) } - fmt.Printf("DEBUG: decide() - About to check CMAB service\n") - fmt.Printf("DEBUG: decide() - o.cmabService is nil? %v\n", o.cmabService == nil) - if o.cmabService != nil { - fmt.Printf("DEBUG: decide() - CMAB service exists, checking experiments\n") - fmt.Printf("DEBUG: decide() - feature.ExperimentIDs: %v\n", feature.ExperimentIDs) - for _, experimentID := range feature.ExperimentIDs { - fmt.Printf("DEBUG: decide() - Processing experiment ID: %s\n", experimentID) experiment, err := projectConfig.GetExperimentByID(experimentID) - fmt.Printf("DEBUG: decide() - GetExperimentByID returned, err: %v\n", err) if err == nil && experiment.Cmab != nil { - fmt.Printf("DEBUG: decide() - Found CMAB experiment, calling GetDecision\n") cmabDecision, cmabErr := o.cmabService.GetDecision(projectConfig, usrContext, experiment.ID, &allOptions) // Handle CMAB error properly - check for errors BEFORE using the decision if cmabErr != nil { - fmt.Printf("DEBUG: decide() - CMAB GetDecision returned error: %v\n", cmabErr) o.logger.Warning(fmt.Sprintf("CMAB decision failed for experiment %s: %v", experiment.ID, cmabErr)) continue // Skip to next experiment or fall back to regular decision } - fmt.Printf("DEBUG: decide() - CMAB decision successful, looking for variation: %s\n", cmabDecision.VariationID) if selectedVariation, exists := experiment.Variations[cmabDecision.VariationID]; exists { - fmt.Printf("DEBUG: decide() - Found variation, creating feature decision\n") featureDecision = decision.FeatureDecision{ Decision: decision.Decision{Reason: "CMAB decision"}, Variation: &selectedVariation, @@ -263,33 +229,26 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string // Only do regular decision logic if CMAB didn't work if !useCMAB { - fmt.Printf("DEBUG: decide() - Using regular decision logic\n") // check forced-decisions first // Passing empty rule-key because checking mapping with flagKey only if userContext.forcedDecisionService != nil { - fmt.Printf("DEBUG: decide() - Checking forced decisions\n") var variation *entities.Variation variation, reasons, err = userContext.forcedDecisionService.FindValidatedForcedDecision(projectConfig, decision.OptimizelyDecisionContext{FlagKey: key, RuleKey: ""}, &allOptions) decisionReasons.Append(reasons) if err != nil { - fmt.Printf("DEBUG: decide() - Forced decision failed, using regular decision\n") findRegularDecision() } else { - fmt.Printf("DEBUG: decide() - Using forced decision\n") featureDecision = decision.FeatureDecision{Decision: decision.Decision{Reason: pkgReasons.ForcedDecisionFound}, Variation: variation, Source: decision.FeatureTest} } } else { - fmt.Printf("DEBUG: decide() - No forced decision service, using regular decision\n") findRegularDecision() } } - fmt.Printf("DEBUG: decide() - Decision logic complete, checking for errors\n") if err != nil { return o.handleDecisionServiceError(err, key, *userContext) } - fmt.Printf("DEBUG: decide() - Processing feature decision results\n") if featureDecision.Variation != nil { variationKey = featureDecision.Variation.Key flagEnabled = featureDecision.Variation.FeatureEnabled @@ -297,7 +256,6 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string variationID = featureDecision.Variation.ID } - fmt.Printf("DEBUG: decide() - About to process events\n") if !allOptions.DisableDecisionEvent { if ue, ok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, featureDecision.Experiment, featureDecision.Variation, usrContext, key, featureDecision.Experiment.Key, featureDecision.Source, flagEnabled, featureDecision.CmabUUID); ok { @@ -306,7 +264,6 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string } } - fmt.Printf("DEBUG: decide() - About to get variable map\n") variableMap := map[string]interface{}{} if !allOptions.ExcludeVariables { variableMap, reasons = o.getDecisionVariableMap(feature, featureDecision.Variation, flagEnabled) @@ -316,7 +273,6 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string reasonsToReport := decisionReasons.ToReport() ruleKey := featureDecision.Experiment.Key - fmt.Printf("DEBUG: decide() - About to send notifications\n") if o.notificationCenter != nil { decisionNotification := decision.FlagNotification(key, variationKey, ruleKey, experimentID, variationID, flagEnabled, eventSent, usrContext, variableMap, reasonsToReport) o.logger.Debug(fmt.Sprintf(`Feature %q is enabled for user %q? %v`, key, usrContext.ID, flagEnabled)) @@ -325,7 +281,6 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string } } - fmt.Printf("DEBUG: decide() - About to return final decision\n") return NewOptimizelyDecision(variationKey, ruleKey, key, flagEnabled, optimizelyJSON, *userContext, reasonsToReport) } From 6c419fe6bee8748dd4edd36b431d404dc8da90ef Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Fri, 27 Jun 2025 10:19:07 -0700 Subject: [PATCH 04/18] add cmab cache options to getAllOptions --- pkg/client/client.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/client/client.go b/pkg/client/client.go index 7b19f178..06b6f55e 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -1199,6 +1199,9 @@ func (o *OptimizelyClient) getAllOptions(options *decide.Options) decide.Options ExcludeVariables: o.defaultDecideOptions.ExcludeVariables || options.ExcludeVariables, IgnoreUserProfileService: o.defaultDecideOptions.IgnoreUserProfileService || options.IgnoreUserProfileService, IncludeReasons: o.defaultDecideOptions.IncludeReasons || options.IncludeReasons, + IgnoreCMABCache: o.defaultDecideOptions.IgnoreCMABCache || options.IgnoreCMABCache, + ResetCMABCache: o.defaultDecideOptions.ResetCMABCache || options.ResetCMABCache, + InvalidateUserCMABCache: o.defaultDecideOptions.InvalidateUserCMABCache || options.InvalidateUserCMABCache, } } From 5dcef46af5e896861d59010c254be28973e859a5 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Fri, 27 Jun 2025 15:47:32 -0700 Subject: [PATCH 05/18] fix failing fsc tests --- pkg/client/client.go | 11 ++++++++++- pkg/decision/experiment_cmab_service.go | 5 ++--- pkg/decision/experiment_cmab_service_test.go | 4 ++-- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 06b6f55e..242a1da0 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -1255,5 +1255,14 @@ func isNil(v interface{}) bool { func (o *OptimizelyClient) handleDecisionServiceError(err error, key string, userContext OptimizelyUserContext) OptimizelyDecision { o.logger.Warning(fmt.Sprintf(`Received error while making a decision for feature %q: %s`, key, err)) - return NewErrorDecision(key, userContext, err) + // Return the error decision with the correct format for decision fields + return OptimizelyDecision{ + FlagKey: key, + UserContext: userContext, + VariationKey: "", // Empty string is correct according to tests + RuleKey: "", // Empty string is correct according to tests + Enabled: false, + Variables: optimizelyjson.NewOptimizelyJSONfromMap(map[string]interface{}{}), + Reasons: []string{err.Error()}, + } } diff --git a/pkg/decision/experiment_cmab_service.go b/pkg/decision/experiment_cmab_service.go index 817836f5..2d66b02c 100644 --- a/pkg/decision/experiment_cmab_service.go +++ b/pkg/decision/experiment_cmab_service.go @@ -159,9 +159,8 @@ func (s *ExperimentCmabService) GetDecision(decisionContext ExperimentDecisionCo // Get CMAB decision cmabDecision, err := s.cmabService.GetDecision(projectConfig, userContext, experiment.ID, options) if err != nil { - message := fmt.Sprintf("Failed to get CMAB decision: %v", err) - decisionReasons.AddInfo(message) - return decision, decisionReasons, fmt.Errorf("failed to get CMAB decision: %w", err) + // Format the error correctly with the experiment key we already have + return decision, decisionReasons, fmt.Errorf(cmab.CmabFetchFailed, experiment.Key) } // Find variation by ID diff --git a/pkg/decision/experiment_cmab_service_test.go b/pkg/decision/experiment_cmab_service_test.go index a73ad252..de0fc06f 100644 --- a/pkg/decision/experiment_cmab_service_test.go +++ b/pkg/decision/experiment_cmab_service_test.go @@ -307,7 +307,7 @@ func (s *ExperimentCmabTestSuite) TestGetDecisionWithCmabServiceError() { // Mock CMAB service to return error s.mockCmabService.On("GetDecision", s.mockProjectConfig, s.testUserContext, "cmab_exp_1", s.options). - Return(cmab.Decision{}, errors.New("CMAB service error")) + Return(cmab.Decision{}, errors.New("Failed to fetch CMAB data for experiment")) // Create CMAB service with mocked dependencies (same pattern as TestGetDecisionSuccess) cmabService := &ExperimentCmabService{ @@ -320,7 +320,7 @@ func (s *ExperimentCmabTestSuite) TestGetDecisionWithCmabServiceError() { // Should return the CMAB service error s.Error(err) - s.Contains(err.Error(), "CMAB service error") + s.Contains(err.Error(), "Failed to fetch CMAB data for experiment") s.Nil(decision.Variation) // No variation when error occurs s.mockExperimentBucketer.AssertExpectations(s.T()) From 1ba0e3c0c031038a4e7529ca8f271a6c8aadcde1 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Fri, 27 Jun 2025 15:53:34 -0700 Subject: [PATCH 06/18] add cmab errors file --- pkg/cmab/errors.go | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 pkg/cmab/errors.go diff --git a/pkg/cmab/errors.go b/pkg/cmab/errors.go new file mode 100644 index 00000000..b37505e2 --- /dev/null +++ b/pkg/cmab/errors.go @@ -0,0 +1,4 @@ +package cmab + +// CmabFetchFailed is the error message format for CMAB fetch failures +const CmabFetchFailed = "failed to fetch CMAB data for experiment %s" From c0ac22c25e867562b066a22c530f7ad426444c3a Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Fri, 27 Jun 2025 16:08:46 -0700 Subject: [PATCH 07/18] adjust lowercase --- pkg/decision/experiment_cmab_service_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/decision/experiment_cmab_service_test.go b/pkg/decision/experiment_cmab_service_test.go index de0fc06f..1bd670a5 100644 --- a/pkg/decision/experiment_cmab_service_test.go +++ b/pkg/decision/experiment_cmab_service_test.go @@ -307,7 +307,7 @@ func (s *ExperimentCmabTestSuite) TestGetDecisionWithCmabServiceError() { // Mock CMAB service to return error s.mockCmabService.On("GetDecision", s.mockProjectConfig, s.testUserContext, "cmab_exp_1", s.options). - Return(cmab.Decision{}, errors.New("Failed to fetch CMAB data for experiment")) + Return(cmab.Decision{}, errors.New("failed to fetch CMAB data for experiment")) // Create CMAB service with mocked dependencies (same pattern as TestGetDecisionSuccess) cmabService := &ExperimentCmabService{ @@ -320,7 +320,7 @@ func (s *ExperimentCmabTestSuite) TestGetDecisionWithCmabServiceError() { // Should return the CMAB service error s.Error(err) - s.Contains(err.Error(), "Failed to fetch CMAB data for experiment") + s.Contains(err.Error(), "failed to fetch CMAB data for experiment") s.Nil(decision.Variation) // No variation when error occurs s.mockExperimentBucketer.AssertExpectations(s.T()) From c8b55e0520cd47816da167c2c6d147e4562fcdd0 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Fri, 27 Jun 2025 18:38:22 -0700 Subject: [PATCH 08/18] add test --- pkg/client/client.go | 4 ++-- pkg/client/client_test.go | 30 ++++++++++++++++++++++++++++++ pkg/cmab/errors.go | 17 +++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 242a1da0..868e163b 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -1259,8 +1259,8 @@ func (o *OptimizelyClient) handleDecisionServiceError(err error, key string, use return OptimizelyDecision{ FlagKey: key, UserContext: userContext, - VariationKey: "", // Empty string is correct according to tests - RuleKey: "", // Empty string is correct according to tests + VariationKey: "", + RuleKey: "", Enabled: false, Variables: optimizelyjson.NewOptimizelyJSONfromMap(map[string]interface{}{}), Reasons: []string{err.Error()}, diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 16f91bd0..474b32a0 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -3186,6 +3186,36 @@ func (s *ClientTestSuiteTrackNotification) TestRemoveOnTrackThrowsErrorWhenRemov mockNotificationCenter.AssertExpectations(s.T()) } +func TestOptimizelyClient_handleDecisionServiceError(t *testing.T) { + // Create the client + client := &OptimizelyClient{ + logger: logging.GetLogger("", ""), + } + + // Create a CMAB error + cmabErrorMessage := "Failed to fetch CMAB data for experiment exp_1." + cmabError := fmt.Errorf(cmabErrorMessage) + + // Create a user context - needs to match the signature expected by handleDecisionServiceError + testUserContext := OptimizelyUserContext{ + UserID: "test_user", + Attributes: map[string]interface{}{}, + } + + // Call the error handler directly + decision := client.handleDecisionServiceError(cmabError, "test_flag", testUserContext) + + // Verify the decision is correctly formatted + assert.False(t, decision.Enabled) + assert.Equal(t, "", decision.VariationKey) // Should be empty string, not nil + assert.Equal(t, "", decision.RuleKey) // Should be empty string, not nil + assert.Contains(t, decision.Reasons, cmabErrorMessage) + + // Check that reasons contains exactly the expected message + assert.Equal(t, 1, len(decision.Reasons), "Reasons array should have exactly one item") + assert.Equal(t, cmabErrorMessage, decision.Reasons[0], "Error message should be added verbatim") +} + func TestClientTestSuiteAB(t *testing.T) { suite.Run(t, new(ClientTestSuiteAB)) } diff --git a/pkg/cmab/errors.go b/pkg/cmab/errors.go index b37505e2..3efa94fc 100644 --- a/pkg/cmab/errors.go +++ b/pkg/cmab/errors.go @@ -1,3 +1,20 @@ +/**************************************************************************** + * Copyright 2025, Optimizely, Inc. and contributors * + * * + * Licensed under the Apache License, Version 2.0 (the "License"); * + * you may not use this file except in compliance with the License. * + * You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, software * + * distributed under the License is distributed on an "AS IS" BASIS, * + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * + * See the License for the specific language governing permissions and * + * limitations under the License. * + ***************************************************************************/ + +// Package cmab to define cmab errors// package cmab // CmabFetchFailed is the error message format for CMAB fetch failures From 8305a901fd479fd4a3ec48c565df9fa1c772779a Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Tue, 15 Jul 2025 12:28:17 -0700 Subject: [PATCH 09/18] fix error message propagation in resons --- pkg/cmab/errors.go | 3 +- pkg/cmab/service.go | 12 +++- pkg/cmab/service_test.go | 64 +++++++++++--------- pkg/decision/experiment_cmab_service.go | 8 ++- pkg/decision/experiment_cmab_service_test.go | 13 ++-- 5 files changed, 58 insertions(+), 42 deletions(-) diff --git a/pkg/cmab/errors.go b/pkg/cmab/errors.go index 3efa94fc..b997ce72 100644 --- a/pkg/cmab/errors.go +++ b/pkg/cmab/errors.go @@ -18,4 +18,5 @@ package cmab // CmabFetchFailed is the error message format for CMAB fetch failures -const CmabFetchFailed = "failed to fetch CMAB data for experiment %s" +// Format required for FSC test compatibility - capitalized and with period +const CmabFetchFailed = "Failed to fetch CMAB data for experiment %s." //nolint:ST1005 // Required exact format for FSC test compatibility diff --git a/pkg/cmab/service.go b/pkg/cmab/service.go index 9cd04bfa..d489690d 100644 --- a/pkg/cmab/service.go +++ b/pkg/cmab/service.go @@ -19,6 +19,7 @@ package cmab import ( "encoding/json" + "errors" "fmt" "strconv" @@ -137,7 +138,9 @@ func (s *DefaultCmabService) GetDecision( // Fetch new decision decision, err := s.fetchDecision(ruleID, userContext.ID, filteredAttributes) if err != nil { - return Decision{Reasons: reasons}, fmt.Errorf("CMAB API error: %w", err) + // Append existing reasons and return the error as-is (already formatted correctly) + decision.Reasons = append(reasons, decision.Reasons...) + return decision, err } // Cache the decision @@ -167,8 +170,11 @@ func (s *DefaultCmabService) fetchDecision( variationID, err := s.cmabClient.FetchDecision(ruleID, userID, attributes, cmabUUID) if err != nil { - reasons = append(reasons, "Failed to fetch CMAB decision") - return Decision{Reasons: reasons}, fmt.Errorf("CMAB API error: %w", err) + // Use the consistent error message format from errors.go + reason := fmt.Sprintf(CmabFetchFailed, ruleID) + reasons = append(reasons, reason) + // Use same format for Go error - FSC compatibility takes precedence + return Decision{Reasons: reasons}, errors.New(reason) //nolint:ST1005 // Required exact format for FSC test compatibility } reasons = append(reasons, "Successfully fetched CMAB decision") diff --git a/pkg/cmab/service_test.go b/pkg/cmab/service_test.go index db49eff9..c919a7d2 100644 --- a/pkg/cmab/service_test.go +++ b/pkg/cmab/service_test.go @@ -798,34 +798,38 @@ func TestCmabServiceTestSuite(t *testing.T) { } func (s *CmabServiceTestSuite) TestGetDecisionApiError() { - // Setup cache key - cacheKey := s.cmabService.getCacheKey(s.testUserID, s.testRuleID) - - // Setup cache lookup (cache miss) - s.mockCache.On("Lookup", cacheKey).Return(nil) - - // Setup mock to return error for experiment lookup (but this won't stop the flow anymore) - s.mockConfig.On("GetExperimentByID", s.testRuleID).Return(entities.Experiment{}, fmt.Errorf("experiment not found")).Once() - - // Mock the FetchDecision call that will now happen - s.mockClient.On("FetchDecision", s.testRuleID, s.testUserID, mock.Anything, mock.Anything).Return("", fmt.Errorf("invalid rule ID")) - - // Call the method - userContext := entities.UserContext{ - ID: s.testUserID, - Attributes: map[string]interface{}{ - "age": 30, - }, - } - - _, err := s.cmabService.GetDecision(s.mockConfig, userContext, s.testRuleID, nil) - - // Should return error from FetchDecision, not from experiment validation - s.Error(err) - s.Contains(err.Error(), "CMAB API error") - - // Verify expectations - s.mockConfig.AssertExpectations(s.T()) - s.mockCache.AssertExpectations(s.T()) - s.mockClient.AssertExpectations(s.T()) + // Setup experiment with CMAB config + experiment := entities.Experiment{ + ID: "rule-123", + Cmab: &entities.Cmab{ + AttributeIds: []string{"attr1"}, + }, + } + s.mockConfig.On("GetExperimentByID", "rule-123").Return(experiment, nil) + s.mockConfig.On("GetAttributeKeyByID", "attr1").Return("category", nil) + + // Configure client to return error + s.mockClient.On("FetchDecision", "rule-123", s.testUserID, mock.Anything, mock.Anything).Return("", errors.New("API error")) + + // Setup cache miss + cacheKey := s.cmabService.getCacheKey(s.testUserID, "rule-123") + s.mockCache.On("Lookup", cacheKey).Return(nil) + + userContext := entities.UserContext{ + ID: s.testUserID, + Attributes: map[string]interface{}{ + "category": "cmab", + }, + } + + decision, err := s.cmabService.GetDecision(s.mockConfig, userContext, "rule-123", nil) + + // Should return the exact error format expected by FSC tests + s.Error(err) + s.Contains(err.Error(), "Failed to fetch CMAB data for experiment") // Updated expectation + s.Contains(decision.Reasons, "Failed to fetch CMAB data for experiment rule-123.") + + s.mockConfig.AssertExpectations(s.T()) + s.mockCache.AssertExpectations(s.T()) + s.mockClient.AssertExpectations(s.T()) } diff --git a/pkg/decision/experiment_cmab_service.go b/pkg/decision/experiment_cmab_service.go index 2d66b02c..06c0035f 100644 --- a/pkg/decision/experiment_cmab_service.go +++ b/pkg/decision/experiment_cmab_service.go @@ -159,8 +159,12 @@ func (s *ExperimentCmabService) GetDecision(decisionContext ExperimentDecisionCo // Get CMAB decision cmabDecision, err := s.cmabService.GetDecision(projectConfig, userContext, experiment.ID, options) if err != nil { - // Format the error correctly with the experiment key we already have - return decision, decisionReasons, fmt.Errorf(cmab.CmabFetchFailed, experiment.Key) + // Add CMAB error to decision reasons + errorMessage := fmt.Sprintf(cmab.CmabFetchFailed, experiment.Key) + decisionReasons.AddInfo(errorMessage) + + // Use same format for Go error - FSC compatibility takes precedence + return decision, decisionReasons, errors.New(errorMessage) //nolint:ST1005 // Required exact format for FSC test compatibility } // Find variation by ID diff --git a/pkg/decision/experiment_cmab_service_test.go b/pkg/decision/experiment_cmab_service_test.go index 1bd670a5..28a31676 100644 --- a/pkg/decision/experiment_cmab_service_test.go +++ b/pkg/decision/experiment_cmab_service_test.go @@ -297,7 +297,7 @@ func (s *ExperimentCmabTestSuite) TestGetDecisionWithNilCmabService() { func (s *ExperimentCmabTestSuite) TestGetDecisionWithCmabServiceError() { testDecisionContext := ExperimentDecisionContext{ - Experiment: &s.cmabExperiment, // Use s.cmabExperiment from setup + Experiment: &s.cmabExperiment, ProjectConfig: s.mockProjectConfig, } @@ -305,11 +305,12 @@ func (s *ExperimentCmabTestSuite) TestGetDecisionWithCmabServiceError() { s.mockExperimentBucketer.On("BucketToEntityID", "test_user_1", mock.AnythingOfType("entities.Experiment"), entities.Group{}). Return(CmabDummyEntityID, reasons.BucketedIntoVariation, nil) - // Mock CMAB service to return error + // Mock CMAB service to return error with the exact format expected + expectedError := errors.New("Failed to fetch CMAB data for experiment cmab_exp_1.") s.mockCmabService.On("GetDecision", s.mockProjectConfig, s.testUserContext, "cmab_exp_1", s.options). - Return(cmab.Decision{}, errors.New("failed to fetch CMAB data for experiment")) + Return(cmab.Decision{}, expectedError) - // Create CMAB service with mocked dependencies (same pattern as TestGetDecisionSuccess) + // Create CMAB service with mocked dependencies cmabService := &ExperimentCmabService{ bucketer: s.mockExperimentBucketer, cmabService: s.mockCmabService, @@ -318,9 +319,9 @@ func (s *ExperimentCmabTestSuite) TestGetDecisionWithCmabServiceError() { decision, _, err := cmabService.GetDecision(testDecisionContext, s.testUserContext, s.options) - // Should return the CMAB service error + // Should return the CMAB service error with exact format - updated to match new format s.Error(err) - s.Contains(err.Error(), "failed to fetch CMAB data for experiment") + s.Contains(err.Error(), "Failed to fetch CMAB data for experiment") // Updated from "failed" to "Failed" s.Nil(decision.Variation) // No variation when error occurs s.mockExperimentBucketer.AssertExpectations(s.T()) From 45d51cfd61bd1597474d3957d1fe300ede3b4cc2 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Tue, 15 Jul 2025 14:01:04 -0700 Subject: [PATCH 10/18] add error handling to feature experiment servvice --- pkg/decision/feature_experiment_service.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/decision/feature_experiment_service.go b/pkg/decision/feature_experiment_service.go index 3b6e4365..ea3d52fe 100644 --- a/pkg/decision/feature_experiment_service.go +++ b/pkg/decision/feature_experiment_service.go @@ -76,6 +76,13 @@ func (f FeatureExperimentService) GetDecision(decisionContext FeatureDecisionCon experimentDecision.Reason, )) + // Handle CMAB experiment errors - they should terminate the decision process + if err != nil && experiment.Cmab != nil { + // For CMAB experiments, errors should prevent fallback to other experiments + // Return empty FeatureDecision (enabled: false, variation_key: null, rule_key: null) + return FeatureDecision{}, reasons, nil + } + // Variation not nil means we got a decision and should return it if experimentDecision.Variation != nil { featureDecision := FeatureDecision{ From 1e92f001af7d42549b68f5d19a234431ae71e70d Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Tue, 15 Jul 2025 14:31:28 -0700 Subject: [PATCH 11/18] Add more error handling to feature exper and composite feature service --- pkg/decision/composite_feature_service.go | 15 +++++ .../composite_feature_service_test.go | 37 ++++++++++- pkg/decision/feature_experiment_service.go | 2 +- .../feature_experiment_service_test.go | 64 +++++++++++++++++++ 4 files changed, 115 insertions(+), 3 deletions(-) diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index 24cba50e..dba98fa6 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -18,6 +18,8 @@ package decision import ( + "strings" + "github.com/optimizely/go-sdk/v2/pkg/decide" "github.com/optimizely/go-sdk/v2/pkg/entities" "github.com/optimizely/go-sdk/v2/pkg/logging" @@ -51,6 +53,19 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont reasons.Append(decisionReasons) if err != nil { f.logger.Debug(err.Error()) + // Check if this is a CMAB error - if so, stop the loop and return empty decision + if strings.Contains(err.Error(), "Failed to fetch CMAB data") { + return FeatureDecision{}, reasons, nil // Return empty decision for CMAB errors + } + } + + // Also check for CMAB errors in decision reasons (when err is nil) + if decisionReasons != nil { + for _, reason := range decisionReasons.ToReport() { + if strings.Contains(reason, "Failed to fetch CMAB data") { + return FeatureDecision{}, reasons, nil + } + } } if featureDecision.Variation != nil && err == nil { diff --git a/pkg/decision/composite_feature_service_test.go b/pkg/decision/composite_feature_service_test.go index acfbd9e5..d413df58 100644 --- a/pkg/decision/composite_feature_service_test.go +++ b/pkg/decision/composite_feature_service_test.go @@ -109,7 +109,7 @@ func (s *CompositeFeatureServiceTestSuite) TestGetDecisionFallthrough() { } func (s *CompositeFeatureServiceTestSuite) TestGetDecisionReturnsError() { - // test that we move onto the next decision service if an inner service returns an error + // test that we move onto the next decision service if an inner service returns a non-CMAB error testUserContext := entities.UserContext{ ID: "test_user_1", } @@ -117,7 +117,8 @@ func (s *CompositeFeatureServiceTestSuite) TestGetDecisionReturnsError() { shouldBeIgnoredDecision := FeatureDecision{ Variation: &testExp1113Var2223, } - s.mockFeatureService.On("GetDecision", s.testFeatureDecisionContext, testUserContext, s.options).Return(shouldBeIgnoredDecision, s.reasons, errors.New("Error making decision")) + // Use a non-CMAB error to ensure fallthrough still works for other errors + s.mockFeatureService.On("GetDecision", s.testFeatureDecisionContext, testUserContext, s.options).Return(shouldBeIgnoredDecision, s.reasons, errors.New("Generic experiment error")) expectedDecision := FeatureDecision{ Variation: &testExp1113Var2224, @@ -165,6 +166,38 @@ func (s *CompositeFeatureServiceTestSuite) TestGetDecisionReturnsLastDecisionWit s.mockFeatureService2.AssertExpectations(s.T()) } +func (s *CompositeFeatureServiceTestSuite) TestGetDecisionWithCmabError() { + // Test that CMAB errors are terminal and don't fall through to rollout service + testUserContext := entities.UserContext{ + ID: "test_user_1", + } + + // Mock the first service (FeatureExperimentService) to return a CMAB error + cmabError := errors.New("Failed to fetch CMAB data for experiment exp_1.") + emptyDecision := FeatureDecision{} + s.mockFeatureService.On("GetDecision", s.testFeatureDecisionContext, testUserContext, s.options).Return(emptyDecision, s.reasons, cmabError) + + // The second service (RolloutService) should NOT be called for CMAB errors + + compositeFeatureService := &CompositeFeatureService{ + featureServices: []FeatureService{ + s.mockFeatureService, + s.mockFeatureService2, + }, + logger: logging.GetLogger("sdkKey", "CompositeFeatureService"), + } + + decision, _, err := compositeFeatureService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options) + + // CMAB errors should result in empty decision with no error + s.Equal(FeatureDecision{}, decision) + s.NoError(err, "CMAB errors should not propagate as Go errors") + + s.mockFeatureService.AssertExpectations(s.T()) + // Verify that the rollout service was NOT called + s.mockFeatureService2.AssertNotCalled(s.T(), "GetDecision") +} + func (s *CompositeFeatureServiceTestSuite) TestNewCompositeFeatureService() { // Assert that the service is instantiated with the correct child services in the right order compositeExperimentService := NewCompositeExperimentService("") diff --git a/pkg/decision/feature_experiment_service.go b/pkg/decision/feature_experiment_service.go index ea3d52fe..615d552b 100644 --- a/pkg/decision/feature_experiment_service.go +++ b/pkg/decision/feature_experiment_service.go @@ -79,7 +79,7 @@ func (f FeatureExperimentService) GetDecision(decisionContext FeatureDecisionCon // Handle CMAB experiment errors - they should terminate the decision process if err != nil && experiment.Cmab != nil { // For CMAB experiments, errors should prevent fallback to other experiments - // Return empty FeatureDecision (enabled: false, variation_key: null, rule_key: null) + // The error is already in reasons from decisionReasons, so return nil error return FeatureDecision{}, reasons, nil } diff --git a/pkg/decision/feature_experiment_service_test.go b/pkg/decision/feature_experiment_service_test.go index 85245bb8..2ffcec29 100644 --- a/pkg/decision/feature_experiment_service_test.go +++ b/pkg/decision/feature_experiment_service_test.go @@ -17,6 +17,7 @@ package decision import ( + "errors" "testing" "github.com/optimizely/go-sdk/v2/pkg/decide" @@ -230,6 +231,69 @@ func (s *FeatureExperimentServiceTestSuite) TestGetDecisionWithCmabUUID() { s.mockExperimentService.AssertExpectations(s.T()) } +func (s *FeatureExperimentServiceTestSuite) TestGetDecisionWithCmabError() { + testUserContext := entities.UserContext{ + ID: "test_user_1", + } + + // Create a NEW CMAB experiment (don't modify existing testExp1113) + cmabExperiment := entities.Experiment{ + ID: "cmab_experiment_id", + Key: "cmab_experiment_key", + Cmab: &entities.Cmab{ + AttributeIds: []string{"attr1", "attr2"}, + TrafficAllocation: 5000, // 50% + }, + Variations: testExp1113.Variations, // Reuse variations for simplicity + } + + // Setup experiment decision context for CMAB experiment + testExperimentDecisionContext := ExperimentDecisionContext{ + Experiment: &cmabExperiment, + ProjectConfig: s.mockConfig, + } + + // Mock the experiment service to return a CMAB error + cmabError := errors.New("Failed to fetch CMAB data for experiment cmab_experiment_key.") + s.mockExperimentService.On("GetDecision", testExperimentDecisionContext, testUserContext, s.options). + Return(ExperimentDecision{}, s.reasons, cmabError) + + // Create a test feature that uses our CMAB experiment + testFeatureWithCmab := entities.Feature{ + ID: "test_feature_cmab", + Key: "test_feature_cmab_key", + FeatureExperiments: []entities.Experiment{ + cmabExperiment, // Only our CMAB experiment + }, + } + + // Create feature decision context with our CMAB feature + testFeatureDecisionContextWithCmab := FeatureDecisionContext{ + Feature: &testFeatureWithCmab, + ProjectConfig: s.mockConfig, + Variable: testVariable, + ForcedDecisionService: NewForcedDecisionService("test_user"), + } + + // Create service under test + featureExperimentService := &FeatureExperimentService{ + compositeExperimentService: s.mockExperimentService, + logger: logging.GetLogger("sdkKey", "FeatureExperimentService"), + } + + // Call GetDecision + actualFeatureDecision, actualReasons, err := featureExperimentService.GetDecision(testFeatureDecisionContextWithCmab, testUserContext, s.options) + + // Verify that CMAB error results in empty feature decision (not error) + s.NoError(err, "CMAB errors should not propagate as Go errors") + s.Equal(FeatureDecision{}, actualFeatureDecision, "Should return empty FeatureDecision when CMAB fails") + + // Verify that reasons include the CMAB error (should be in actualReasons from mock) + s.NotNil(actualReasons, "Decision reasons should not be nil") + + s.mockExperimentService.AssertExpectations(s.T()) +} + func TestFeatureExperimentServiceTestSuite(t *testing.T) { suite.Run(t, new(FeatureExperimentServiceTestSuite)) } From 7bcfe8a39f4a793022261385dc5d40808c9f0316 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Tue, 15 Jul 2025 14:43:46 -0700 Subject: [PATCH 12/18] nil back to err --- pkg/decision/feature_experiment_service.go | 6 +++--- pkg/decision/feature_experiment_service_test.go | 7 ++++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/decision/feature_experiment_service.go b/pkg/decision/feature_experiment_service.go index 615d552b..f2bc3689 100644 --- a/pkg/decision/feature_experiment_service.go +++ b/pkg/decision/feature_experiment_service.go @@ -78,9 +78,9 @@ func (f FeatureExperimentService) GetDecision(decisionContext FeatureDecisionCon // Handle CMAB experiment errors - they should terminate the decision process if err != nil && experiment.Cmab != nil { - // For CMAB experiments, errors should prevent fallback to other experiments - // The error is already in reasons from decisionReasons, so return nil error - return FeatureDecision{}, reasons, nil + // For CMAB experiments, errors should prevent fallback to other experiments AND rollouts + // Return the error so CompositeFeatureService can detect it + return FeatureDecision{}, reasons, err } // Variation not nil means we got a decision and should return it diff --git a/pkg/decision/feature_experiment_service_test.go b/pkg/decision/feature_experiment_service_test.go index 2ffcec29..2df57291 100644 --- a/pkg/decision/feature_experiment_service_test.go +++ b/pkg/decision/feature_experiment_service_test.go @@ -284,11 +284,12 @@ func (s *FeatureExperimentServiceTestSuite) TestGetDecisionWithCmabError() { // Call GetDecision actualFeatureDecision, actualReasons, err := featureExperimentService.GetDecision(testFeatureDecisionContextWithCmab, testUserContext, s.options) - // Verify that CMAB error results in empty feature decision (not error) - s.NoError(err, "CMAB errors should not propagate as Go errors") + // CMAB errors should result in empty feature decision with the error returned + s.Error(err, "CMAB errors should be returned as errors") // ← Changed from s.NoError + s.Contains(err.Error(), "Failed to fetch CMAB data", "Error should contain CMAB failure message") s.Equal(FeatureDecision{}, actualFeatureDecision, "Should return empty FeatureDecision when CMAB fails") - // Verify that reasons include the CMAB error (should be in actualReasons from mock) + // Verify that reasons include the CMAB error s.NotNil(actualReasons, "Decision reasons should not be nil") s.mockExperimentService.AssertExpectations(s.T()) From d2181fbba71aa18bb6b4395691677d34f40c86e8 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Tue, 15 Jul 2025 14:53:31 -0700 Subject: [PATCH 13/18] add reasons message to composite feature service GetDecision --- pkg/decision/composite_feature_service.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index dba98fa6..8eee7bcd 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -55,6 +55,8 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont f.logger.Debug(err.Error()) // Check if this is a CMAB error - if so, stop the loop and return empty decision if strings.Contains(err.Error(), "Failed to fetch CMAB data") { + // Add the CMAB error to reasons before returning + reasons.AddInfo(err.Error()) return FeatureDecision{}, reasons, nil // Return empty decision for CMAB errors } } From a76accc7a819f39c6780bca52ef5ce9a9a46a95e Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Tue, 15 Jul 2025 15:39:11 -0700 Subject: [PATCH 14/18] use AddError for reasons --- pkg/decision/composite_feature_service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index 8eee7bcd..64720115 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -55,8 +55,8 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont f.logger.Debug(err.Error()) // Check if this is a CMAB error - if so, stop the loop and return empty decision if strings.Contains(err.Error(), "Failed to fetch CMAB data") { - // Add the CMAB error to reasons before returning - reasons.AddInfo(err.Error()) + // Add the CMAB error to reasons before returning - use AddError for critical failures + reasons.AddError(err.Error()) return FeatureDecision{}, reasons, nil // Return empty decision for CMAB errors } } From 2c913ba7c8c3a39a5583303c94e128c7489a8bb5 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Wed, 16 Jul 2025 10:25:14 -0700 Subject: [PATCH 15/18] Trigger PR check From a1f4b66173c87b5136ecc31a4f8aaadd58f812e8 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 17 Jul 2025 13:34:55 -0700 Subject: [PATCH 16/18] fix cyclomatic complexity by refactoring client.go code --- pkg/client/client.go | 152 +++++++++++++++++++++++---------------- pkg/cmab/service_test.go | 123 +++++++++---------------------- 2 files changed, 126 insertions(+), 149 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 3f5e987d..3e0ea038 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -176,71 +176,42 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string QualifiedSegments: userContext.GetQualifiedSegments(), } - var variationKey string - var eventSent, flagEnabled bool allOptions := o.getAllOptions(options) decisionReasons := decide.NewDecisionReasons(&allOptions) decisionContext.Variable = entities.Variable{} var featureDecision decision.FeatureDecision - var reasons decide.DecisionReasons - var experimentID string - var variationID string - var useCMAB bool - - // To avoid cyclo-complexity warning - findRegularDecision := func() { - // regular decision - featureDecision, reasons, err = o.DecisionService.GetFeatureDecision(decisionContext, usrContext, &allOptions) - decisionReasons.Append(reasons) - } - - if o.cmabService != nil { - for _, experimentID := range feature.ExperimentIDs { - experiment, err := projectConfig.GetExperimentByID(experimentID) - - if err == nil && experiment.Cmab != nil { - cmabDecision, cmabErr := o.cmabService.GetDecision(projectConfig, usrContext, experiment.ID, &allOptions) + var decisionReasonsList decide.DecisionReasons // Fix shadowing - renamed from "reasons" - // Handle CMAB error properly - check for errors BEFORE using the decision - if cmabErr != nil { - o.logger.Warning(fmt.Sprintf("CMAB decision failed for experiment %s: %v", experiment.ID, cmabErr)) - continue // Skip to next experiment or fall back to regular decision - } + // Try CMAB decision first + useCMAB := o.tryGetCMABDecision(feature, projectConfig, usrContext, &allOptions, decisionReasons, &featureDecision) - if selectedVariation, exists := experiment.Variations[cmabDecision.VariationID]; exists { - featureDecision = decision.FeatureDecision{ - Decision: decision.Decision{Reason: "CMAB decision"}, - Variation: &selectedVariation, - Experiment: experiment, - Source: decision.FeatureTest, - CmabUUID: &cmabDecision.CmabUUID, - } - useCMAB = true - decisionReasons.AddInfo("Used CMAB service for decision") - break + // Fall back to other decision types if CMAB didn't work + if !useCMAB { + // To avoid cyclo-complexity warning - forced decision logic + findForcedDecision := func() bool { + if userContext.forcedDecisionService != nil { + var variation *entities.Variation + var forcedErr error + variation, decisionReasonsList, forcedErr = userContext.forcedDecisionService.FindValidatedForcedDecision(projectConfig, decision.OptimizelyDecisionContext{FlagKey: key, RuleKey: ""}, &allOptions) // Fix shadowing by using assignment instead of declaration + decisionReasons.Append(decisionReasonsList) + if forcedErr != nil { + return false } else { - o.logger.Warning(fmt.Sprintf("CMAB returned invalid variation ID %s for experiment %s", cmabDecision.VariationID, experiment.ID)) + featureDecision = decision.FeatureDecision{Decision: decision.Decision{Reason: pkgReasons.ForcedDecisionFound}, Variation: variation, Source: decision.FeatureTest} + return true } - } else { - o.logger.Warning(fmt.Sprintf("CMAB decision failed for experiment %s: %v", experiment.ID, err)) } + return false } - } - // Only do regular decision logic if CMAB didn't work - if !useCMAB { - // check forced-decisions first - // Passing empty rule-key because checking mapping with flagKey only - if userContext.forcedDecisionService != nil { - var variation *entities.Variation - variation, reasons, err = userContext.forcedDecisionService.FindValidatedForcedDecision(projectConfig, decision.OptimizelyDecisionContext{FlagKey: key, RuleKey: ""}, &allOptions) - decisionReasons.Append(reasons) - if err != nil { - findRegularDecision() - } else { - featureDecision = decision.FeatureDecision{Decision: decision.Decision{Reason: pkgReasons.ForcedDecisionFound}, Variation: variation, Source: decision.FeatureTest} - } - } else { + // To avoid cyclo-complexity warning - regular decision logic + findRegularDecision := func() { + // regular decision + featureDecision, decisionReasonsList, err = o.DecisionService.GetFeatureDecision(decisionContext, usrContext, &allOptions) + decisionReasons.Append(decisionReasonsList) + } + + if !findForcedDecision() { findRegularDecision() } } @@ -249,6 +220,54 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string return o.handleDecisionServiceError(err, key, *userContext) } + return o.buildDecisionResponse(featureDecision, feature, key, userContext, &allOptions, decisionReasons, decisionContext) +} + +// tryGetCMABDecision attempts to get a CMAB decision for the feature +func (o *OptimizelyClient) tryGetCMABDecision(feature entities.Feature, projectConfig config.ProjectConfig, usrContext entities.UserContext, options *decide.Options, decisionReasons decide.DecisionReasons, featureDecision *decision.FeatureDecision) bool { + if o.cmabService == nil { + return false + } + + for _, experimentID := range feature.ExperimentIDs { + experiment, expErr := projectConfig.GetExperimentByID(experimentID) // Fix shadowing + + // Handle CMAB error properly - check for errors BEFORE using the experiment + if expErr == nil && experiment.Cmab != nil { + cmabDecision, cmabErr := o.cmabService.GetDecision(projectConfig, usrContext, experiment.ID, options) + + // Handle CMAB service errors gracefully - log and continue to next experiment + if cmabErr != nil { + o.logger.Warning(fmt.Sprintf("CMAB decision failed for experiment %s: %v", experiment.ID, cmabErr)) + continue + } + + // Validate CMAB response - ensure variation exists before using it + if selectedVariation, exists := experiment.Variations[cmabDecision.VariationID]; exists { + *featureDecision = decision.FeatureDecision{ + Decision: decision.Decision{Reason: "CMAB decision"}, + Variation: &selectedVariation, + Experiment: experiment, + Source: decision.FeatureTest, + CmabUUID: &cmabDecision.CmabUUID, // Include CMAB UUID for tracking + } + decisionReasons.AddInfo("Used CMAB service for decision") + return true + } else { + // Log invalid variation ID returned by CMAB service + o.logger.Warning(fmt.Sprintf("CMAB returned invalid variation ID %s for experiment %s", cmabDecision.VariationID, experiment.ID)) + } + } + } + return false +} + +// buildDecisionResponse constructs the final OptimizelyDecision response +func (o *OptimizelyClient) buildDecisionResponse(featureDecision decision.FeatureDecision, feature entities.Feature, key string, userContext *OptimizelyUserContext, options *decide.Options, decisionReasons decide.DecisionReasons, decisionContext decision.FeatureDecisionContext) OptimizelyDecision { + var variationKey string + var eventSent, flagEnabled bool + var experimentID, variationID string + if featureDecision.Variation != nil { variationKey = featureDecision.Variation.Key flagEnabled = featureDecision.Variation.FeatureEnabled @@ -256,7 +275,14 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string variationID = featureDecision.Variation.ID } - if !allOptions.DisableDecisionEvent { + usrContext := entities.UserContext{ + ID: userContext.GetUserID(), + Attributes: userContext.GetUserAttributes(), + QualifiedSegments: userContext.GetQualifiedSegments(), + } + + // Send impression event + if !options.DisableDecisionEvent { if ue, ok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, featureDecision.Experiment, featureDecision.Variation, usrContext, key, featureDecision.Experiment.Key, featureDecision.Source, flagEnabled, featureDecision.CmabUUID); ok { o.EventProcessor.ProcessEvent(ue) @@ -264,16 +290,18 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string } } + // Get variable map variableMap := map[string]interface{}{} - if !allOptions.ExcludeVariables { + if !options.ExcludeVariables { + var reasons decide.DecisionReasons variableMap, reasons = o.getDecisionVariableMap(feature, featureDecision.Variation, flagEnabled) decisionReasons.Append(reasons) } - optimizelyJSON := optimizelyjson.NewOptimizelyJSONfromMap(variableMap) - reasonsToReport := decisionReasons.ToReport() - ruleKey := featureDecision.Experiment.Key + // Send notification if o.notificationCenter != nil { + reasonsToReport := decisionReasons.ToReport() + ruleKey := featureDecision.Experiment.Key decisionNotification := decision.FlagNotification(key, variationKey, ruleKey, experimentID, variationID, flagEnabled, eventSent, usrContext, variableMap, reasonsToReport) o.logger.Debug(fmt.Sprintf(`Feature %q is enabled for user %q? %v`, key, usrContext.ID, flagEnabled)) if e := o.notificationCenter.Send(notification.Decision, *decisionNotification); e != nil { @@ -281,6 +309,10 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string } } + optimizelyJSON := optimizelyjson.NewOptimizelyJSONfromMap(variableMap) + reasonsToReport := decisionReasons.ToReport() + ruleKey := featureDecision.Experiment.Key + return NewOptimizelyDecision(variationKey, ruleKey, key, flagEnabled, optimizelyJSON, *userContext, reasonsToReport) } @@ -509,7 +541,7 @@ func (o *OptimizelyClient) Activate(experimentKey string, userContext entities.U } // IsFeatureEnabled returns true if the feature is enabled for the given user. If the user is part of a feature test -// then an impression event will be queued up to be sent to the Optimizely log endpoint for results processing. +// then an impression event will be queued up to the Optimizely log endpoint for results processing. func (o *OptimizelyClient) IsFeatureEnabled(featureKey string, userContext entities.UserContext) (result bool, err error) { defer func() { diff --git a/pkg/cmab/service_test.go b/pkg/cmab/service_test.go index 40acc322..880f6e72 100644 --- a/pkg/cmab/service_test.go +++ b/pkg/cmab/service_test.go @@ -575,61 +575,6 @@ func (s *CmabServiceTestSuite) TestGetDecisionError() { s.Equal("", decision.VariationID) // Should be empty } -func (s *CmabServiceTestSuite) TestNilReasonsErrorHandling() { - // This test specifically verifies that appending to a nil Reasons slice - // causes a panic, while the fix avoids the panic - - // Create a test decision with nil Reasons - testDecision := Decision{ - VariationID: "test-var", - CmabUUID: "test-uuid", - Reasons: nil, // nil Reasons field - } - - // A slice of reasons we want to append - reasons := []string{"Test reason 1", "Test reason 2"} - - // Test the buggy behavior - var didPanic bool - - func() { - defer func() { - if r := recover(); r != nil { - didPanic = true - s.T().Logf("Panic occurred as expected: %v", r) - } - }() - - // This simulates the bug: - // decision.Reasons = append(reasons, decision.Reasons...) - testDecision.Reasons = append(reasons, testDecision.Reasons...) - }() - - // Verify the panic occurred - s.True(didPanic, "Appending to nil Reasons should cause a panic") - - // Now test the fixed behavior - didPanic = false - - func() { - defer func() { - if r := recover(); r != nil { - didPanic = true - s.T().Logf("Unexpected panic in fixed version: %v", r) - } - }() - - // This simulates the fix: - // return Decision{Reasons: reasons}, err - fixedDecision := Decision{Reasons: reasons} - s.NotNil(fixedDecision.Reasons, "Fixed version should have non-nil Reasons") - s.Equal(reasons, fixedDecision.Reasons, "Reasons should match") - }() - - // Verify no panic with the fix - s.False(didPanic, "Fixed version should not panic") -} - func (s *CmabServiceTestSuite) TestFilterAttributes() { // Setup mock experiment with CMAB configuration experiment := entities.Experiment{ @@ -853,38 +798,38 @@ func TestCmabServiceTestSuite(t *testing.T) { } func (s *CmabServiceTestSuite) TestGetDecisionApiError() { - // Setup experiment with CMAB config - experiment := entities.Experiment{ - ID: "rule-123", - Cmab: &entities.Cmab{ - AttributeIds: []string{"attr1"}, - }, - } - s.mockConfig.On("GetExperimentByID", "rule-123").Return(experiment, nil) - s.mockConfig.On("GetAttributeKeyByID", "attr1").Return("category", nil) - - // Configure client to return error - s.mockClient.On("FetchDecision", "rule-123", s.testUserID, mock.Anything, mock.Anything).Return("", errors.New("API error")) - - // Setup cache miss - cacheKey := s.cmabService.getCacheKey(s.testUserID, "rule-123") - s.mockCache.On("Lookup", cacheKey).Return(nil) - - userContext := entities.UserContext{ - ID: s.testUserID, - Attributes: map[string]interface{}{ - "category": "cmab", - }, - } - - decision, err := s.cmabService.GetDecision(s.mockConfig, userContext, "rule-123", nil) - - // Should return the exact error format expected by FSC tests - s.Error(err) - s.Contains(err.Error(), "Failed to fetch CMAB data for experiment") // Updated expectation - s.Contains(decision.Reasons, "Failed to fetch CMAB data for experiment rule-123.") - - s.mockConfig.AssertExpectations(s.T()) - s.mockCache.AssertExpectations(s.T()) - s.mockClient.AssertExpectations(s.T()) + // Setup experiment with CMAB config + experiment := entities.Experiment{ + ID: "rule-123", + Cmab: &entities.Cmab{ + AttributeIds: []string{"attr1"}, + }, + } + s.mockConfig.On("GetExperimentByID", "rule-123").Return(experiment, nil) + s.mockConfig.On("GetAttributeKeyByID", "attr1").Return("category", nil) + + // Configure client to return error + s.mockClient.On("FetchDecision", "rule-123", s.testUserID, mock.Anything, mock.Anything).Return("", errors.New("API error")) + + // Setup cache miss + cacheKey := s.cmabService.getCacheKey(s.testUserID, "rule-123") + s.mockCache.On("Lookup", cacheKey).Return(nil) + + userContext := entities.UserContext{ + ID: s.testUserID, + Attributes: map[string]interface{}{ + "category": "cmab", + }, + } + + decision, err := s.cmabService.GetDecision(s.mockConfig, userContext, "rule-123", nil) + + // Should return the exact error format expected by FSC tests + s.Error(err) + s.Contains(err.Error(), "Failed to fetch CMAB data for experiment") // Updated expectation + s.Contains(decision.Reasons, "Failed to fetch CMAB data for experiment rule-123.") + + s.mockConfig.AssertExpectations(s.T()) + s.mockCache.AssertExpectations(s.T()) + s.mockClient.AssertExpectations(s.T()) } From 7b763786f977480f97e2a5933c557454419d8315 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 17 Jul 2025 13:42:40 -0700 Subject: [PATCH 17/18] fix lint error --- pkg/client/client.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 3e0ea038..25f5d64a 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -196,10 +196,9 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string decisionReasons.Append(decisionReasonsList) if forcedErr != nil { return false - } else { - featureDecision = decision.FeatureDecision{Decision: decision.Decision{Reason: pkgReasons.ForcedDecisionFound}, Variation: variation, Source: decision.FeatureTest} - return true } + featureDecision = decision.FeatureDecision{Decision: decision.Decision{Reason: pkgReasons.ForcedDecisionFound}, Variation: variation, Source: decision.FeatureTest} + return true } return false } From 6a8bae442b6e0591c4c7c0ebde37ec2a42385640 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 17 Jul 2025 13:52:01 -0700 Subject: [PATCH 18/18] fix lint --- pkg/client/client.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 25f5d64a..f9eacc4f 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -252,10 +252,9 @@ func (o *OptimizelyClient) tryGetCMABDecision(feature entities.Feature, projectC } decisionReasons.AddInfo("Used CMAB service for decision") return true - } else { - // Log invalid variation ID returned by CMAB service - o.logger.Warning(fmt.Sprintf("CMAB returned invalid variation ID %s for experiment %s", cmabDecision.VariationID, experiment.ID)) } + // Log invalid variation ID returned by CMAB service + o.logger.Warning(fmt.Sprintf("CMAB returned invalid variation ID %s for experiment %s", cmabDecision.VariationID, experiment.ID)) } } return false