Skip to content

Commit a1f4b66

Browse files
Mat001Mat001
committed
fix cyclomatic complexity by refactoring client.go code
1 parent 9d57add commit a1f4b66

File tree

2 files changed

+126
-149
lines changed

2 files changed

+126
-149
lines changed

‎pkg/client/client.go

Lines changed: 92 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -176,71 +176,42 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string
176176
QualifiedSegments: userContext.GetQualifiedSegments(),
177177
}
178178

179-
var variationKey string
180-
var eventSent, flagEnabled bool
181179
allOptions := o.getAllOptions(options)
182180
decisionReasons := decide.NewDecisionReasons(&allOptions)
183181
decisionContext.Variable = entities.Variable{}
184182
var featureDecision decision.FeatureDecision
185-
var reasons decide.DecisionReasons
186-
var experimentID string
187-
var variationID string
188-
var useCMAB bool
189-
190-
// To avoid cyclo-complexity warning
191-
findRegularDecision := func() {
192-
// regular decision
193-
featureDecision, reasons, err = o.DecisionService.GetFeatureDecision(decisionContext, usrContext, &allOptions)
194-
decisionReasons.Append(reasons)
195-
}
196-
197-
if o.cmabService != nil {
198-
for _, experimentID := range feature.ExperimentIDs {
199-
experiment, err := projectConfig.GetExperimentByID(experimentID)
200-
201-
if err == nil && experiment.Cmab != nil {
202-
cmabDecision, cmabErr := o.cmabService.GetDecision(projectConfig, usrContext, experiment.ID, &allOptions)
183+
var decisionReasonsList decide.DecisionReasons // Fix shadowing - renamed from "reasons"
203184

204-
// Handle CMAB error properly - check for errors BEFORE using the decision
205-
if cmabErr != nil {
206-
o.logger.Warning(fmt.Sprintf("CMAB decision failed for experiment %s: %v", experiment.ID, cmabErr))
207-
continue // Skip to next experiment or fall back to regular decision
208-
}
185+
// Try CMAB decision first
186+
useCMAB := o.tryGetCMABDecision(feature, projectConfig, usrContext, &allOptions, decisionReasons, &featureDecision)
209187

210-
if selectedVariation, exists := experiment.Variations[cmabDecision.VariationID]; exists {
211-
featureDecision = decision.FeatureDecision{
212-
Decision: decision.Decision{Reason: "CMAB decision"},
213-
Variation: &selectedVariation,
214-
Experiment: experiment,
215-
Source: decision.FeatureTest,
216-
CmabUUID: &cmabDecision.CmabUUID,
217-
}
218-
useCMAB = true
219-
decisionReasons.AddInfo("Used CMAB service for decision")
220-
break
188+
// Fall back to other decision types if CMAB didn't work
189+
if !useCMAB {
190+
// To avoid cyclo-complexity warning - forced decision logic
191+
findForcedDecision := func() bool {
192+
if userContext.forcedDecisionService != nil {
193+
var variation *entities.Variation
194+
var forcedErr error
195+
variation, decisionReasonsList, forcedErr = userContext.forcedDecisionService.FindValidatedForcedDecision(projectConfig, decision.OptimizelyDecisionContext{FlagKey: key, RuleKey: ""}, &allOptions) // Fix shadowing by using assignment instead of declaration
196+
decisionReasons.Append(decisionReasonsList)
197+
if forcedErr != nil {
198+
return false
221199
} else {
222-
o.logger.Warning(fmt.Sprintf("CMAB returned invalid variation ID %s for experiment %s", cmabDecision.VariationID, experiment.ID))
200+
featureDecision = decision.FeatureDecision{Decision: decision.Decision{Reason: pkgReasons.ForcedDecisionFound}, Variation: variation, Source: decision.FeatureTest}
201+
return true
223202
}
224-
} else {
225-
o.logger.Warning(fmt.Sprintf("CMAB decision failed for experiment %s: %v", experiment.ID, err))
226203
}
204+
return false
227205
}
228-
}
229206

230-
// Only do regular decision logic if CMAB didn't work
231-
if !useCMAB {
232-
// check forced-decisions first
233-
// Passing empty rule-key because checking mapping with flagKey only
234-
if userContext.forcedDecisionService != nil {
235-
var variation *entities.Variation
236-
variation, reasons, err = userContext.forcedDecisionService.FindValidatedForcedDecision(projectConfig, decision.OptimizelyDecisionContext{FlagKey: key, RuleKey: ""}, &allOptions)
237-
decisionReasons.Append(reasons)
238-
if err != nil {
239-
findRegularDecision()
240-
} else {
241-
featureDecision = decision.FeatureDecision{Decision: decision.Decision{Reason: pkgReasons.ForcedDecisionFound}, Variation: variation, Source: decision.FeatureTest}
242-
}
243-
} else {
207+
// To avoid cyclo-complexity warning - regular decision logic
208+
findRegularDecision := func() {
209+
// regular decision
210+
featureDecision, decisionReasonsList, err = o.DecisionService.GetFeatureDecision(decisionContext, usrContext, &allOptions)
211+
decisionReasons.Append(decisionReasonsList)
212+
}
213+
214+
if !findForcedDecision() {
244215
findRegularDecision()
245216
}
246217
}
@@ -249,38 +220,99 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string
249220
return o.handleDecisionServiceError(err, key, *userContext)
250221
}
251222

223+
return o.buildDecisionResponse(featureDecision, feature, key, userContext, &allOptions, decisionReasons, decisionContext)
224+
}
225+
226+
// tryGetCMABDecision attempts to get a CMAB decision for the feature
227+
func (o *OptimizelyClient) tryGetCMABDecision(feature entities.Feature, projectConfig config.ProjectConfig, usrContext entities.UserContext, options *decide.Options, decisionReasons decide.DecisionReasons, featureDecision *decision.FeatureDecision) bool {
228+
if o.cmabService == nil {
229+
return false
230+
}
231+
232+
for _, experimentID := range feature.ExperimentIDs {
233+
experiment, expErr := projectConfig.GetExperimentByID(experimentID) // Fix shadowing
234+
235+
// Handle CMAB error properly - check for errors BEFORE using the experiment
236+
if expErr == nil && experiment.Cmab != nil {
237+
cmabDecision, cmabErr := o.cmabService.GetDecision(projectConfig, usrContext, experiment.ID, options)
238+
239+
// Handle CMAB service errors gracefully - log and continue to next experiment
240+
if cmabErr != nil {
241+
o.logger.Warning(fmt.Sprintf("CMAB decision failed for experiment %s: %v", experiment.ID, cmabErr))
242+
continue
243+
}
244+
245+
// Validate CMAB response - ensure variation exists before using it
246+
if selectedVariation, exists := experiment.Variations[cmabDecision.VariationID]; exists {
247+
*featureDecision = decision.FeatureDecision{
248+
Decision: decision.Decision{Reason: "CMAB decision"},
249+
Variation: &selectedVariation,
250+
Experiment: experiment,
251+
Source: decision.FeatureTest,
252+
CmabUUID: &cmabDecision.CmabUUID, // Include CMAB UUID for tracking
253+
}
254+
decisionReasons.AddInfo("Used CMAB service for decision")
255+
return true
256+
} else {
257+
// Log invalid variation ID returned by CMAB service
258+
o.logger.Warning(fmt.Sprintf("CMAB returned invalid variation ID %s for experiment %s", cmabDecision.VariationID, experiment.ID))
259+
}
260+
}
261+
}
262+
return false
263+
}
264+
265+
// buildDecisionResponse constructs the final OptimizelyDecision response
266+
func (o *OptimizelyClient) buildDecisionResponse(featureDecision decision.FeatureDecision, feature entities.Feature, key string, userContext *OptimizelyUserContext, options *decide.Options, decisionReasons decide.DecisionReasons, decisionContext decision.FeatureDecisionContext) OptimizelyDecision {
267+
var variationKey string
268+
var eventSent, flagEnabled bool
269+
var experimentID, variationID string
270+
252271
if featureDecision.Variation != nil {
253272
variationKey = featureDecision.Variation.Key
254273
flagEnabled = featureDecision.Variation.FeatureEnabled
255274
experimentID = featureDecision.Experiment.ID
256275
variationID = featureDecision.Variation.ID
257276
}
258277

259-
if !allOptions.DisableDecisionEvent {
278+
usrContext := entities.UserContext{
279+
ID: userContext.GetUserID(),
280+
Attributes: userContext.GetUserAttributes(),
281+
QualifiedSegments: userContext.GetQualifiedSegments(),
282+
}
283+
284+
// Send impression event
285+
if !options.DisableDecisionEvent {
260286
if ue, ok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, featureDecision.Experiment,
261287
featureDecision.Variation, usrContext, key, featureDecision.Experiment.Key, featureDecision.Source, flagEnabled, featureDecision.CmabUUID); ok {
262288
o.EventProcessor.ProcessEvent(ue)
263289
eventSent = true
264290
}
265291
}
266292

293+
// Get variable map
267294
variableMap := map[string]interface{}{}
268-
if !allOptions.ExcludeVariables {
295+
if !options.ExcludeVariables {
296+
var reasons decide.DecisionReasons
269297
variableMap, reasons = o.getDecisionVariableMap(feature, featureDecision.Variation, flagEnabled)
270298
decisionReasons.Append(reasons)
271299
}
272-
optimizelyJSON := optimizelyjson.NewOptimizelyJSONfromMap(variableMap)
273-
reasonsToReport := decisionReasons.ToReport()
274-
ruleKey := featureDecision.Experiment.Key
275300

301+
// Send notification
276302
if o.notificationCenter != nil {
303+
reasonsToReport := decisionReasons.ToReport()
304+
ruleKey := featureDecision.Experiment.Key
277305
decisionNotification := decision.FlagNotification(key, variationKey, ruleKey, experimentID, variationID, flagEnabled, eventSent, usrContext, variableMap, reasonsToReport)
278306
o.logger.Debug(fmt.Sprintf(`Feature %q is enabled for user %q? %v`, key, usrContext.ID, flagEnabled))
279307
if e := o.notificationCenter.Send(notification.Decision, *decisionNotification); e != nil {
280308
o.logger.Warning("Problem with sending notification")
281309
}
282310
}
283311

312+
optimizelyJSON := optimizelyjson.NewOptimizelyJSONfromMap(variableMap)
313+
reasonsToReport := decisionReasons.ToReport()
314+
ruleKey := featureDecision.Experiment.Key
315+
284316
return NewOptimizelyDecision(variationKey, ruleKey, key, flagEnabled, optimizelyJSON, *userContext, reasonsToReport)
285317
}
286318

@@ -509,7 +541,7 @@ func (o *OptimizelyClient) Activate(experimentKey string, userContext entities.U
509541
}
510542

511543
// IsFeatureEnabled returns true if the feature is enabled for the given user. If the user is part of a feature test
512-
// then an impression event will be queued up to be sent to the Optimizely log endpoint for results processing.
544+
// then an impression event will be queued up to the Optimizely log endpoint for results processing.
513545
func (o *OptimizelyClient) IsFeatureEnabled(featureKey string, userContext entities.UserContext) (result bool, err error) {
514546

515547
defer func() {

‎pkg/cmab/service_test.go

Lines changed: 34 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -575,61 +575,6 @@ func (s *CmabServiceTestSuite) TestGetDecisionError() {
575575
s.Equal("", decision.VariationID) // Should be empty
576576
}
577577

578-
func (s *CmabServiceTestSuite) TestNilReasonsErrorHandling() {
579-
// This test specifically verifies that appending to a nil Reasons slice
580-
// causes a panic, while the fix avoids the panic
581-
582-
// Create a test decision with nil Reasons
583-
testDecision := Decision{
584-
VariationID: "test-var",
585-
CmabUUID: "test-uuid",
586-
Reasons: nil, // nil Reasons field
587-
}
588-
589-
// A slice of reasons we want to append
590-
reasons := []string{"Test reason 1", "Test reason 2"}
591-
592-
// Test the buggy behavior
593-
var didPanic bool
594-
595-
func() {
596-
defer func() {
597-
if r := recover(); r != nil {
598-
didPanic = true
599-
s.T().Logf("Panic occurred as expected: %v", r)
600-
}
601-
}()
602-
603-
// This simulates the bug:
604-
// decision.Reasons = append(reasons, decision.Reasons...)
605-
testDecision.Reasons = append(reasons, testDecision.Reasons...)
606-
}()
607-
608-
// Verify the panic occurred
609-
s.True(didPanic, "Appending to nil Reasons should cause a panic")
610-
611-
// Now test the fixed behavior
612-
didPanic = false
613-
614-
func() {
615-
defer func() {
616-
if r := recover(); r != nil {
617-
didPanic = true
618-
s.T().Logf("Unexpected panic in fixed version: %v", r)
619-
}
620-
}()
621-
622-
// This simulates the fix:
623-
// return Decision{Reasons: reasons}, err
624-
fixedDecision := Decision{Reasons: reasons}
625-
s.NotNil(fixedDecision.Reasons, "Fixed version should have non-nil Reasons")
626-
s.Equal(reasons, fixedDecision.Reasons, "Reasons should match")
627-
}()
628-
629-
// Verify no panic with the fix
630-
s.False(didPanic, "Fixed version should not panic")
631-
}
632-
633578
func (s *CmabServiceTestSuite) TestFilterAttributes() {
634579
// Setup mock experiment with CMAB configuration
635580
experiment := entities.Experiment{
@@ -853,38 +798,38 @@ func TestCmabServiceTestSuite(t *testing.T) {
853798
}
854799

855800
func (s *CmabServiceTestSuite) TestGetDecisionApiError() {
856-
// Setup experiment with CMAB config
857-
experiment := entities.Experiment{
858-
ID: "rule-123",
859-
Cmab: &entities.Cmab{
860-
AttributeIds: []string{"attr1"},
861-
},
862-
}
863-
s.mockConfig.On("GetExperimentByID", "rule-123").Return(experiment, nil)
864-
s.mockConfig.On("GetAttributeKeyByID", "attr1").Return("category", nil)
865-
866-
// Configure client to return error
867-
s.mockClient.On("FetchDecision", "rule-123", s.testUserID, mock.Anything, mock.Anything).Return("", errors.New("API error"))
868-
869-
// Setup cache miss
870-
cacheKey := s.cmabService.getCacheKey(s.testUserID, "rule-123")
871-
s.mockCache.On("Lookup", cacheKey).Return(nil)
872-
873-
userContext := entities.UserContext{
874-
ID: s.testUserID,
875-
Attributes: map[string]interface{}{
876-
"category": "cmab",
877-
},
878-
}
879-
880-
decision, err := s.cmabService.GetDecision(s.mockConfig, userContext, "rule-123", nil)
881-
882-
// Should return the exact error format expected by FSC tests
883-
s.Error(err)
884-
s.Contains(err.Error(), "Failed to fetch CMAB data for experiment") // Updated expectation
885-
s.Contains(decision.Reasons, "Failed to fetch CMAB data for experiment rule-123.")
886-
887-
s.mockConfig.AssertExpectations(s.T())
888-
s.mockCache.AssertExpectations(s.T())
889-
s.mockClient.AssertExpectations(s.T())
801+
// Setup experiment with CMAB config
802+
experiment := entities.Experiment{
803+
ID: "rule-123",
804+
Cmab: &entities.Cmab{
805+
AttributeIds: []string{"attr1"},
806+
},
807+
}
808+
s.mockConfig.On("GetExperimentByID", "rule-123").Return(experiment, nil)
809+
s.mockConfig.On("GetAttributeKeyByID", "attr1").Return("category", nil)
810+
811+
// Configure client to return error
812+
s.mockClient.On("FetchDecision", "rule-123", s.testUserID, mock.Anything, mock.Anything).Return("", errors.New("API error"))
813+
814+
// Setup cache miss
815+
cacheKey := s.cmabService.getCacheKey(s.testUserID, "rule-123")
816+
s.mockCache.On("Lookup", cacheKey).Return(nil)
817+
818+
userContext := entities.UserContext{
819+
ID: s.testUserID,
820+
Attributes: map[string]interface{}{
821+
"category": "cmab",
822+
},
823+
}
824+
825+
decision, err := s.cmabService.GetDecision(s.mockConfig, userContext, "rule-123", nil)
826+
827+
// Should return the exact error format expected by FSC tests
828+
s.Error(err)
829+
s.Contains(err.Error(), "Failed to fetch CMAB data for experiment") // Updated expectation
830+
s.Contains(decision.Reasons, "Failed to fetch CMAB data for experiment rule-123.")
831+
832+
s.mockConfig.AssertExpectations(s.T())
833+
s.mockCache.AssertExpectations(s.T())
834+
s.mockClient.AssertExpectations(s.T())
890835
}

0 commit comments

Comments
 (0)