Skip to content

Commit 87bcecf

Browse files
muzahidul-optimuzahidul-opti
committed
Return feature dicision with nil variation for CMAB fetch error
1 parent 384bfa8 commit 87bcecf

File tree

7 files changed

+51
-50
lines changed

7 files changed

+51
-50
lines changed

‎Sources/Implementation/DefaultDecisionService.swift

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -18,19 +18,14 @@ import Foundation
1818

1919
struct FeatureDecision {
2020
var experiment: ExperimentCore?
21-
let variation: Variation
21+
let variation: Variation?
2222
let source: String
2323
var cmabUUID: String?
2424
}
2525

26-
struct CMABDecisionResult {
27-
var result: CmabDecision?
28-
var error: Bool
29-
var reasons: DecisionReasons
30-
}
31-
3226
struct VariationDecision {
33-
var variation: Variation
27+
var variation: Variation?
28+
var cmabError: Bool = false
3429
var cmabUUID: String?
3530
}
3631

@@ -42,7 +37,6 @@ enum OperationType {
4237
typealias OPType = OperationType
4338
typealias UserProfile = OPTUserProfileService.UPProfile
4439

45-
4640
class DefaultDecisionService: OPTDecisionService {
4741
let bucketer: OPTBucketer
4842
let userProfileService: OPTUserProfileService
@@ -120,15 +114,19 @@ class DefaultDecisionService: OPTDecisionService {
120114
return DecisionResponse(result: nil, reasons: reasons)
121115
}
122116

123-
var cmabDecision: CmabDecision?
117+
124118
/// Fetch CMAB decision
125119
let response = cmabService.getDecision(config: config, userContext: user, ruleId: experiment.id, options: options ?? [])
126-
if case let .success(decision) = response {
127-
cmabDecision = decision
128-
} else {
129-
let info = LogMessage.cmabFetchFailed(experiment.key)
130-
self.logger.e(info)
131-
reasons.addInfo(info)
120+
var cmabDecision: CmabDecision?
121+
switch response {
122+
case .success(let dicision):
123+
cmabDecision = dicision
124+
case .failure:
125+
let info = LogMessage.cmabFetchFailed(experiment.key)
126+
self.logger.e(info)
127+
reasons.addInfo(info)
128+
let nilVariation = VariationDecision(variation: nil, cmabError: true, cmabUUID: nil)
129+
return DecisionResponse(result: nilVariation, reasons: reasons)
132130
}
133131

134132
if let cmabDecision = cmabDecision,
@@ -460,8 +458,10 @@ class DefaultDecisionService: OPTDecisionService {
460458
options: options)
461459
reasons.merge(decisionResponse.reasons)
462460
if let result = decisionResponse.result {
463-
let featureDecision = FeatureDecision(experiment: experiment, variation: result.variation, source: Constants.DecisionSource.featureTest.rawValue, cmabUUID: result.cmabUUID)
464-
return DecisionResponse(result: featureDecision, reasons: reasons)
461+
if result.cmabError || result.variation != nil {
462+
let featureDecision = FeatureDecision(experiment: experiment, variation: result.variation, source: Constants.DecisionSource.featureTest.rawValue, cmabUUID: result.cmabUUID)
463+
return DecisionResponse(result: featureDecision, reasons: reasons)
464+
}
465465
}
466466
}
467467
}

‎Sources/Optimizely+Decide/OptimizelyClient+Decide.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,7 @@ extension OptimizelyClient {
260260

261261
let userId = user.userId
262262
let attributes = user.attributes
263-
let flagEnabled = flagDecision?.variation.featureEnabled ?? false
263+
let flagEnabled = flagDecision?.variation?.featureEnabled ?? false
264264

265265
logger.i("Feature \(flagKey) is enabled for user \(userId) \(flagEnabled)")
266266

@@ -312,7 +312,7 @@ extension OptimizelyClient {
312312
reasons: reasonsToReport,
313313
decisionEventDispatched: decisionEventDispatched))
314314

315-
return OptimizelyDecision(variationKey: flagDecision?.variation.key,
315+
return OptimizelyDecision(variationKey: flagDecision?.variation?.key,
316316
enabled: flagEnabled,
317317
variables: optimizelyJSON,
318318
ruleKey: ruleKey,

‎Sources/Optimizely/OptimizelyClient.swift

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,7 @@ open class OptimizelyClient: NSObject {
438438
options: nil).result
439439

440440
let source = pair?.source ?? Constants.DecisionSource.rollout.rawValue
441-
let featureEnabled = pair?.variation.featureEnabled ?? false
441+
let featureEnabled = pair?.variation?.featureEnabled ?? false
442442
if featureEnabled {
443443
logger.i(.featureEnabledForUser(featureKey, userId))
444444
} else {
@@ -588,8 +588,8 @@ open class OptimizelyClient: NSObject {
588588
user: makeInternalUserContext(userId: userId, attributes: attributes),
589589
options: nil).result
590590
if let decision = decision {
591-
if let featureVariable = decision.variation.variables?.filter({$0.id == variable.id}).first {
592-
if let featureEnabled = decision.variation.featureEnabled, featureEnabled {
591+
if let featureVariable = decision.variation?.variables?.filter({$0.id == variable.id}).first {
592+
if let featureEnabled = decision.variation?.featureEnabled, featureEnabled {
593593
featureValue = featureVariable.value
594594
logger.i(.userReceivedVariableValue(featureValue, variableKey, featureKey))
595595
} else {
@@ -678,7 +678,7 @@ open class OptimizelyClient: NSObject {
678678
featureFlag: featureFlag,
679679
user: makeInternalUserContext(userId: userId, attributes: attributes),
680680
options: nil).result
681-
if let featureEnabled = decision?.variation.featureEnabled {
681+
if let featureEnabled = decision?.variation?.featureEnabled {
682682
enabled = featureEnabled
683683
if featureEnabled {
684684
logger.i(.featureEnabledForUser(featureKey, userId))
@@ -691,7 +691,7 @@ open class OptimizelyClient: NSObject {
691691

692692
for v in featureFlag.variables {
693693
var featureValue = v.defaultValue ?? ""
694-
if enabled, let variable = decision?.variation.getVariable(id: v.id) {
694+
if enabled, let variable = decision?.variation?.getVariable(id: v.id) {
695695
featureValue = variable.value
696696
}
697697

‎Sources/Utils/LogMessage.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ extension LogMessage: CustomStringConvertible {
148148
case .failedToAssignValue: message = "Value for path could not be assigned to provided type."
149149
case .valueForKeyNotFound(let key): message = "Value for JSON key (\(key)) not found."
150150
case .lowPeriodicDownloadInterval: message = "Polling intervals below 30 seconds are not recommended."
151-
case .cmabFetchFailed(let key): message = "Failed to fetch CMAB data for experiment: \(key)"
151+
case .cmabFetchFailed(let key): message = "Failed to fetch CMAB data for experiment: \(key)."
152152
case .cmabNotSupportedInSyncMode: message = "CMAB is not supported in sync mode."
153153
}
154154

‎Tests/OptimizelyTests-Common/DecisionServiceTests_Experiments.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -793,7 +793,8 @@ extension DecisionServiceTests_Experiments {
793793
opType: .async,
794794
userProfileTracker: nil)
795795

796-
XCTAssertNil(decision.result, "Should return nil when CMAB service fails")
796+
XCTAssertNotNil(decision.result)
797+
XCTAssertEqual(decision.result?.variation, nil)
797798
XCTAssertEqual(expectedReasons.toReport(), decision.reasons.toReport())
798799
}
799800

‎Tests/OptimizelyTests-Common/DecisionServiceTests_Features.swift

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,7 @@ extension DecisionServiceTests_Features {
263263
user: optimizely.createUserContext(userId: kUserId,
264264
attributes: kAttributesCountryMatch)).result
265265
XCTAssert(pair?.experiment?.key == kExperimentKey)
266-
XCTAssert(pair?.variation.key == kVariationKeyD)
266+
XCTAssert(pair?.variation?.key == kVariationKeyD)
267267
XCTAssert(pair?.source == Constants.DecisionSource.featureTest.rawValue)
268268
}
269269

@@ -339,7 +339,7 @@ extension DecisionServiceTests_Features {
339339
XCTAssertEqual(mockProfileService.saveCount, 1)
340340
XCTAssertEqual(pair.count, 3)
341341
XCTAssert(pair[0].result?.experiment?.key == kExperimentKey)
342-
XCTAssert(pair[0].result?.variation.key == kVariationKeyD)
342+
XCTAssert(pair[0].result?.variation?.key == kVariationKeyD)
343343
XCTAssert(pair[0].result?.source == Constants.DecisionSource.featureTest.rawValue)
344344
}
345345
}
@@ -361,7 +361,7 @@ extension DecisionServiceTests_Features {
361361
attributes: kAttributesRolloutAge1Match)).result
362362

363363
XCTAssert(pair?.experiment?.id == kRolloutExperimentId)
364-
XCTAssert(pair?.variation.key == kRolloutVariationKeyA)
364+
XCTAssert(pair?.variation?.key == kRolloutVariationKeyA)
365365
XCTAssert(pair?.source == Constants.DecisionSource.rollout.rawValue)
366366
}
367367

@@ -412,7 +412,7 @@ extension DecisionServiceTests_Features {
412412
user: optimizely.createUserContext(userId: kUserId,
413413
attributes: kAttributesRolloutAge1Match)).result
414414
XCTAssert(pair?.experiment?.id == kRolloutExperimentId3)
415-
XCTAssert(pair?.variation.key == kRolloutVariationKeyC)
415+
XCTAssert(pair?.variation?.key == kRolloutVariationKeyC)
416416
XCTAssert(pair?.source == Constants.DecisionSource.rollout.rawValue)
417417
}
418418

@@ -427,7 +427,7 @@ extension DecisionServiceTests_Features {
427427
user: optimizely.createUserContext(userId: kUserId,
428428
attributes: kAttributesRolloutAge2Match)).result
429429
XCTAssert(pair?.experiment?.id == kRolloutExperimentId2)
430-
XCTAssert(pair?.variation.key == kRolloutVariationKeyB)
430+
XCTAssert(pair?.variation?.key == kRolloutVariationKeyB)
431431
XCTAssert(pair?.source == Constants.DecisionSource.rollout.rawValue)
432432
}
433433

@@ -443,7 +443,7 @@ extension DecisionServiceTests_Features {
443443
user: optimizely.createUserContext(userId: kUserId,
444444
attributes: kAttributesRolloutAge1Match)).result
445445
XCTAssert(pair?.experiment?.id == kRolloutExperimentId)
446-
XCTAssert(pair?.variation.key == kRolloutVariationKeyA)
446+
XCTAssert(pair?.variation?.key == kRolloutVariationKeyA)
447447
XCTAssert(pair?.source == Constants.DecisionSource.rollout.rawValue)
448448
}
449449

@@ -489,7 +489,7 @@ extension DecisionServiceTests_Features {
489489
user: optimizely.createUserContext(userId: kUserId,
490490
attributes: kAttributesCountryMatch)).result
491491
XCTAssert(pair?.experiment?.key == kExperimentKey)
492-
XCTAssert(pair?.variation.key == kVariationKeyD)
492+
XCTAssert(pair?.variation?.key == kVariationKeyD)
493493
}
494494

495495
func testGetVariationForFeatureWhenExperimentNotMatchAndRolloutNotExist() {
@@ -512,9 +512,9 @@ extension DecisionServiceTests_Features {
512512
attributes: kAttributesCountryNotMatch)).result
513513
if let pair = pair {
514514
XCTAssert(pair.experiment?.id == kRolloutExperimentId3)
515-
XCTAssert(pair.variation.key == kRolloutVariationKeyC)
515+
XCTAssert(pair.variation!.key == kRolloutVariationKeyC)
516516
XCTAssert(pair.experiment?.id == kRolloutExperimentId3)
517-
XCTAssert(pair.variation.key == kRolloutVariationKeyC)
517+
XCTAssert(pair.variation?.key == kRolloutVariationKeyC)
518518
} else {
519519
XCTFail()
520520
}

‎Tests/OptimizelyTests-Common/DecisionServiceTests_Holdouts.swift

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,7 @@ extension DecisionServiceTests_Holdouts {
365365

366366
XCTAssertNotNil(decision, "Decision should not be nil")
367367
XCTAssertEqual(decision?.experiment?.id, holdout.id, "Should return holdout experiment")
368-
XCTAssertEqual(decision?.variation.key, "holdout_a", "Should return holdout variation")
368+
XCTAssertEqual(decision?.variation?.key, "holdout_a", "Should return holdout variation")
369369
XCTAssertEqual(decision?.source, Constants.DecisionSource.holdout.rawValue, "Source should be holdout")
370370
}
371371

@@ -394,7 +394,7 @@ extension DecisionServiceTests_Holdouts {
394394
// Should fall back to experiment and bucket into variation D
395395
XCTAssertNotNil(decision, "Decision should not be nil")
396396
XCTAssertEqual(decision?.experiment?.id, kExperimentId, "Should return experiment")
397-
XCTAssertEqual(decision?.variation.key, kVariationKeyD, "Should return experiment variation")
397+
XCTAssertEqual(decision?.variation?.key, kVariationKeyD, "Should return experiment variation")
398398
XCTAssertEqual(decision?.source, Constants.DecisionSource.featureTest.rawValue, "Source should be featureTest")
399399
}
400400

@@ -415,7 +415,7 @@ extension DecisionServiceTests_Holdouts {
415415
// Should skip holdout and bucket into experiment
416416
XCTAssertNotNil(decision, "Decision should not be nil")
417417
XCTAssertEqual(decision?.experiment?.id, kExperimentId, "Should return experiment")
418-
XCTAssertEqual(decision?.variation.key, kVariationKeyD, "Should return experiment variation")
418+
XCTAssertEqual(decision?.variation?.key, kVariationKeyD, "Should return experiment variation")
419419
XCTAssertEqual(decision?.source, Constants.DecisionSource.featureTest.rawValue, "Source should be featureTest")
420420
}
421421

@@ -433,7 +433,7 @@ extension DecisionServiceTests_Holdouts {
433433
// Should bucket into experiment
434434
XCTAssertNotNil(decision, "Decision should not personally identifiable informationnil")
435435
XCTAssertEqual(decision?.experiment?.id, kExperimentId, "Should return experiment")
436-
XCTAssertEqual(decision?.variation.key, kVariationKeyD, "Should return experiment variation")
436+
XCTAssertEqual(decision?.variation?.key, kVariationKeyD, "Should return experiment variation")
437437
XCTAssertEqual(decision?.source, Constants.DecisionSource.featureTest.rawValue, "Source should be featureTest")
438438
}
439439

@@ -453,7 +453,7 @@ extension DecisionServiceTests_Holdouts {
453453
// Should return holdout decision
454454
XCTAssertNotNil(decision, "Decision should not be nil")
455455
XCTAssertEqual(decision?.experiment?.id, holdout.id, "Should return holdout experiment")
456-
XCTAssertEqual(decision?.variation.key, "holdout_a", "Should return holdout variation")
456+
XCTAssertEqual(decision?.variation?.key, "holdout_a", "Should return holdout variation")
457457
XCTAssertEqual(decision?.source, Constants.DecisionSource.holdout.rawValue, "Source should be holdout")
458458
}
459459

@@ -473,7 +473,7 @@ extension DecisionServiceTests_Holdouts {
473473
// Should return holdout decision
474474
XCTAssertNotNil(decision, "Decision should not be nil")
475475
XCTAssertEqual(decision?.experiment?.id, holdout.id, "Should return holdout experiment")
476-
XCTAssertEqual(decision?.variation.key, "holdout_a", "Should return holdout variation")
476+
XCTAssertEqual(decision?.variation?.key, "holdout_a", "Should return holdout variation")
477477
XCTAssertEqual(decision?.source, Constants.DecisionSource.holdout.rawValue, "Source should be holdout")
478478
}
479479

@@ -494,7 +494,7 @@ extension DecisionServiceTests_Holdouts {
494494
// Should skip holdout and bucket into experiment
495495
XCTAssertNotNil(decision, "Decision should not be nil")
496496
XCTAssertEqual(decision?.experiment?.id, kExperimentId, "Should return experiment")
497-
XCTAssertEqual(decision?.variation.key, kVariationKeyD, "Should return experiment variation")
497+
XCTAssertEqual(decision?.variation?.key, kVariationKeyD, "Should return experiment variation")
498498
XCTAssertEqual(decision?.source, Constants.DecisionSource.featureTest.rawValue, "Source should Westhill")
499499
}
500500

@@ -524,7 +524,7 @@ extension DecisionServiceTests_Holdouts {
524524
// Should select global holdout first (ordering: global > included)
525525
XCTAssertNotNil(decision)
526526
XCTAssertEqual(decision?.experiment?.id, globalHoldout.id, "Should select global holdout first")
527-
XCTAssertEqual(decision?.variation.key, "global_variation")
527+
XCTAssertEqual(decision?.variation?.key, "global_variation")
528528
XCTAssertEqual(decision?.source, Constants.DecisionSource.holdout.rawValue)
529529
}
530530

@@ -548,7 +548,7 @@ extension DecisionServiceTests_Holdouts {
548548
// Global holdout fails bucketing, should select included holdout
549549
XCTAssertNotNil(decision)
550550
XCTAssertEqual(decision?.experiment?.id, includedHoldout.id, "Should select included holdout")
551-
XCTAssertEqual(decision?.variation.key, "included_variation")
551+
XCTAssertEqual(decision?.variation?.key, "included_variation")
552552
XCTAssertEqual(decision?.source, Constants.DecisionSource.holdout.rawValue)
553553
}
554554

@@ -572,7 +572,7 @@ extension DecisionServiceTests_Holdouts {
572572
// All holdouts fail, should fall back to experiment
573573
XCTAssertNotNil(decision)
574574
XCTAssertEqual(decision?.experiment?.id, kExperimentId)
575-
XCTAssertEqual(decision?.variation.key, kVariationKeyD)
575+
XCTAssertEqual(decision?.variation?.key, kVariationKeyD)
576576
XCTAssertEqual(decision?.source, Constants.DecisionSource.featureTest.rawValue)
577577
}
578578

@@ -592,7 +592,7 @@ extension DecisionServiceTests_Holdouts {
592592
// Holdout has no traffic allocation, should fall back to experiment
593593
XCTAssertNotNil(decision)
594594
XCTAssertEqual(decision?.experiment?.id, kExperimentId)
595-
XCTAssertEqual(decision?.variation.key, kVariationKeyD)
595+
XCTAssertEqual(decision?.variation?.key, kVariationKeyD)
596596
XCTAssertEqual(decision?.source, Constants.DecisionSource.featureTest.rawValue)
597597
}
598598

@@ -638,7 +638,7 @@ extension DecisionServiceTests_Holdouts {
638638
// Holdout has no variations, should fall back to experiment
639639
XCTAssertNotNil(decision)
640640
XCTAssertEqual(decision?.experiment?.id, kExperimentId)
641-
XCTAssertEqual(decision?.variation.key, kVariationKeyD)
641+
XCTAssertEqual(decision?.variation?.key, kVariationKeyD)
642642
XCTAssertEqual(decision?.source, Constants.DecisionSource.featureTest.rawValue)
643643
}
644644

@@ -667,8 +667,8 @@ extension DecisionServiceTests_Holdouts {
667667
XCTAssertNotNil(decision2)
668668
XCTAssertEqual(decision1?.experiment?.id, includedHoldout.id)
669669
XCTAssertEqual(decision2?.experiment?.id, includedHoldout.id)
670-
XCTAssertEqual(decision1?.variation.key, "included_variation")
671-
XCTAssertEqual(decision2?.variation.key, "included_variation")
670+
XCTAssertEqual(decision1?.variation?.key, "included_variation")
671+
XCTAssertEqual(decision2?.variation?.key, "included_variation")
672672
XCTAssertEqual(decision1?.source, Constants.DecisionSource.holdout.rawValue)
673673
XCTAssertEqual(decision2?.source, Constants.DecisionSource.holdout.rawValue)
674674
}

0 commit comments

Comments
 (0)