Skip to content

[FSSDK-11589] Add go-sdk logic to support agent for cmab #412

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
65 changes: 54 additions & 11 deletions pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -173,6 +175,7 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string
Attributes: userContext.GetUserAttributes(),
QualifiedSegments: userContext.GetQualifiedSegments(),
}

var variationKey string
var eventSent, flagEnabled bool
allOptions := o.getAllOptions(options)
Expand All @@ -182,6 +185,7 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string
var reasons decide.DecisionReasons
var experimentID string
var variationID string
var useCMAB bool

// To avoid cyclo-complexity warning
findRegularDecision := func() {
Expand All @@ -190,19 +194,55 @@ 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()
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)

// 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
}

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
} 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 {
// 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 {
featureDecision = decision.FeatureDecision{Decision: decision.Decision{Reason: pkgReasons.ForcedDecisionFound}, Variation: variation, Source: decision.FeatureTest}
findRegularDecision()
}
} else {
findRegularDecision()
}

if err != nil {
Expand Down Expand Up @@ -1199,6 +1239,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,
}
}

Expand Down
150 changes: 150 additions & 0 deletions pkg/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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))
}
Expand Down
13 changes: 13 additions & 0 deletions pkg/client/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -53,6 +54,7 @@ type OptimizelyFactory struct {
overrideStore decision.ExperimentOverrideStore
userProfileService decision.UserProfileService
notificationCenter notification.Center
cmabService cmab.Service

// ODP
segmentsCacheSize int
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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) {

Expand Down
4 changes: 2 additions & 2 deletions pkg/cmab/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
55 changes: 55 additions & 0 deletions pkg/cmab/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
Loading