Skip to content

Commit f4e5070

Browse files
esrakartalOptesrakartalOpt
committed
[AI-FSSDK] [FSSDK-12735] Fix holdout exclusion: local holdouts ignore flag, TD null returns null, always send HO events
1 parent c5d5762 commit f4e5070

4 files changed

Lines changed: 119 additions & 30 deletions

File tree

‎core-api/src/main/java/com/optimizely/ab/Optimizely.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1356,6 +1356,19 @@ private OptimizelyDecision createOptimizelyDecision(
13561356
cmabUuid);
13571357
}
13581358

1359+
if (flagDecision.holdoutDecision != null && !allOptions.contains(OptimizelyDecideOption.DISABLE_DECISION_EVENT)) {
1360+
sendImpression(
1361+
projectConfig,
1362+
flagDecision.holdoutDecision.experiment,
1363+
userId,
1364+
copiedAttributes,
1365+
flagDecision.holdoutDecision.variation,
1366+
flagKey,
1367+
flagDecision.holdoutDecision.decisionSource != null ? flagDecision.holdoutDecision.decisionSource.toString() : FeatureDecision.DecisionSource.HOLDOUT.toString(),
1368+
flagDecision.holdoutDecision.variation != null && flagDecision.holdoutDecision.variation.getFeatureEnabled(),
1369+
null);
1370+
}
1371+
13591372
DecisionNotification decisionNotification = DecisionNotification.newFlagDecisionNotificationBuilder()
13601373
.withUserId(userId)
13611374
.withAttributes(copiedAttributes)

‎core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -365,10 +365,20 @@ public List<DecisionResponse<FeatureDecision>> getVariationsForFeatureList(@Non
365365
FeatureDecision decision = decisionFeatureResponse.getResult();
366366

367367
if (decision != null && decision.variation != null) {
368+
if (globalHoldoutDecision != null) {
369+
decision.setHoldoutDecision(globalHoldoutDecision);
370+
}
371+
String message = reasons.addInfo("The user \"%s\" was bucketed into a rollout for feature flag \"%s\".",
372+
user.getUserId(), featureFlag.getKey());
373+
logger.info(message);
368374
decisions.add(new DecisionResponse(decision, reasons));
369-
} else if (globalHoldoutDecision != null) {
370-
decisions.add(new DecisionResponse<>(globalHoldoutDecision, reasons));
371375
} else {
376+
if (globalHoldoutDecision != null) {
377+
if (decision == null) {
378+
decision = new FeatureDecision(null, null, null);
379+
}
380+
decision.setHoldoutDecision(globalHoldoutDecision);
381+
}
372382
String message = reasons.addInfo("The user \"%s\" was not bucketed into a rollout for feature flag \"%s\".",
373383
user.getUserId(), featureFlag.getKey());
374384
logger.info(message);
@@ -715,14 +725,10 @@ public DecisionResponse<Variation> validatedForcedDecision(@Nonnull OptimizelyDe
715725

716726
DecisionResponse<FeatureDecision> evaluateLocalHoldouts(@Nonnull ExperimentCore rule,
717727
@Nonnull ProjectConfig projectConfig,
718-
@Nonnull OptimizelyUserContext user,
719-
boolean isDeliveryRule) {
728+
@Nonnull OptimizelyUserContext user) {
720729
DecisionReasons reasons = DefaultDecisionReasons.newInstance();
721730
List<Holdout> localHoldouts = projectConfig.getHoldoutsForRule(rule.getId());
722731
for (Holdout holdout : localHoldouts) {
723-
if (isDeliveryRule && holdout.isExcludeTargetedDeliveries()) {
724-
continue;
725-
}
726732
DecisionResponse<Variation> holdoutDecision = getVariationForHoldout(holdout, user, projectConfig);
727733
reasons.merge(holdoutDecision.getReasons());
728734
if (holdoutDecision.getResult() != null) {
@@ -871,7 +877,7 @@ private DecisionResponse<FeatureDecision> getVariationFromExperimentRule(@Nonnul
871877

872878
// Step 2: Check local holdouts
873879
if (rule != null) {
874-
DecisionResponse<FeatureDecision> holdoutResponse = evaluateLocalHoldouts(rule, projectConfig, user, false);
880+
DecisionResponse<FeatureDecision> holdoutResponse = evaluateLocalHoldouts(rule, projectConfig, user);
875881
reasons.merge(holdoutResponse.getReasons());
876882
if (holdoutResponse.getResult() != null) {
877883
return new DecisionResponse<>(holdoutResponse.getResult(), reasons);
@@ -933,7 +939,7 @@ DecisionResponse<AbstractMap.SimpleEntry> getVariationFromDeliveryRule(@Nonnull
933939
}
934940

935941
// Step 2: Check local holdouts
936-
DecisionResponse<FeatureDecision> holdoutResponse = evaluateLocalHoldouts(rule, projectConfig, user, true);
942+
DecisionResponse<FeatureDecision> holdoutResponse = evaluateLocalHoldouts(rule, projectConfig, user);
937943
reasons.merge(holdoutResponse.getReasons());
938944
if (holdoutResponse.getResult() != null) {
939945
resultPair = new AbstractMap.SimpleEntry<>(holdoutResponse.getResult(), false);

‎core-api/src/main/java/com/optimizely/ab/bucketing/FeatureDecision.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,13 @@ public class FeatureDecision {
4545
@Nullable
4646
public String cmabUuid;
4747

48+
@Nullable
49+
public FeatureDecision holdoutDecision;
50+
51+
public void setHoldoutDecision(@Nullable FeatureDecision holdoutDecision) {
52+
this.holdoutDecision = holdoutDecision;
53+
}
54+
4855
public enum DecisionSource {
4956
FEATURE_TEST("feature-test"),
5057
ROLLOUT("rollout"),

‎core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java‎

Lines changed: 84 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1760,8 +1760,7 @@ public void evaluateLocalHoldouts_returnsHoldoutDecisionWhenUserBucketed() {
17601760

17611761
DecisionResponse<FeatureDecision> response = decisionService.evaluateLocalHoldouts(
17621762
targetedRule, localHoldoutConfig,
1763-
optimizely.createUserContext("any_user", Collections.<String, Object>emptyMap()),
1764-
false
1763+
optimizely.createUserContext("any_user", Collections.<String, Object>emptyMap())
17651764
);
17661765

17671766
assertNotNull(response.getResult());
@@ -1781,8 +1780,7 @@ public void evaluateLocalHoldouts_returnsNullWhenNoHoldoutsForRule() {
17811780

17821781
DecisionResponse<FeatureDecision> response = decisionService.evaluateLocalHoldouts(
17831782
untargetedRule, localHoldoutConfig,
1784-
optimizely.createUserContext("any_user", Collections.<String, Object>emptyMap()),
1785-
false
1783+
optimizely.createUserContext("any_user", Collections.<String, Object>emptyMap())
17861784
);
17871785

17881786
assertNull(response.getResult());
@@ -1798,8 +1796,7 @@ public void evaluateLocalHoldouts_returnsNullWhenConfigHasNoHoldouts() {
17981796

17991797
DecisionResponse<FeatureDecision> response = decisionService.evaluateLocalHoldouts(
18001798
rule, noHoldoutConfig,
1801-
optimizely.createUserContext("any_user", Collections.<String, Object>emptyMap()),
1802-
false
1799+
optimizely.createUserContext("any_user", Collections.<String, Object>emptyMap())
18031800
);
18041801

18051802
assertNull(response.getResult());
@@ -1974,7 +1971,7 @@ public void excludeTargetedDeliveries_globalHoldoutFalse_blocksAllRules() {
19741971
}
19751972

19761973
@Test
1977-
public void excludeTargetedDeliveries_globalHoldoutTrue_blocksExperimentRules() {
1974+
public void excludeTargetedDeliveries_globalHoldoutTrue_skipsExperimentRules() {
19781975
ProjectConfig config = ValidProjectConfigV4.generateValidProjectConfigV4_globalHoldoutExcludeTargetedDeliveries();
19791976

19801977
Bucketer bucketer = new Bucketer();
@@ -1986,8 +1983,12 @@ public void excludeTargetedDeliveries_globalHoldoutTrue_blocksExperimentRules()
19861983
config
19871984
).getResult();
19881985

1989-
assertNotNull(decision);
1990-
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource);
1986+
assertTrue("With excludeTargetedDeliveries=true and no rollout, variation should be null",
1987+
decision == null || decision.variation == null);
1988+
if (decision != null) {
1989+
assertNotNull("holdoutDecision should be attached", decision.holdoutDecision);
1990+
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.holdoutDecision.decisionSource);
1991+
}
19911992
}
19921993

19931994
@Test
@@ -2009,7 +2010,7 @@ public void excludeTargetedDeliveries_globalHoldoutTrue_allowsDeliveryRules() {
20092010
}
20102011

20112012
@Test
2012-
public void excludeTargetedDeliveries_globalHoldoutTrue_noDeliveryMatch_returnsHoldout() {
2013+
public void excludeTargetedDeliveries_globalHoldoutTrue_noDeliveryMatch_returnsNullWithHoldoutAttached() {
20132014
ProjectConfig config = ValidProjectConfigV4.generateValidProjectConfigV4_globalHoldoutExcludeTargetedDeliveries();
20142015

20152016
Bucketer bucketer = new Bucketer();
@@ -2021,28 +2022,30 @@ public void excludeTargetedDeliveries_globalHoldoutTrue_noDeliveryMatch_returnsH
20212022
config
20222023
).getResult();
20232024

2024-
assertNotNull(decision);
2025-
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource);
2026-
assertEquals(HOLDOUT_GLOBAL_EXCLUDE_TARGETED_DELIVERIES, decision.experiment);
2025+
assertTrue("When excludeTargetedDeliveries=true and no delivery match, variation should be null",
2026+
decision == null || decision.variation == null);
2027+
if (decision != null) {
2028+
assertNotNull("holdoutDecision should be attached", decision.holdoutDecision);
2029+
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.holdoutDecision.decisionSource);
2030+
}
20272031
}
20282032

20292033
@Test
2030-
public void excludeTargetedDeliveries_localHoldoutTrue_skipsHoldoutForDeliveryRules() {
2034+
public void excludeTargetedDeliveries_localHoldoutTrue_appliesHoldoutForDeliveryRules() {
20312035
ProjectConfig config = ValidProjectConfigV4.generateValidProjectConfigV4_localHoldoutExcludeTargetedDeliveries();
20322036

20332037
Bucketer bucketer = new Bucketer();
20342038
DecisionService ds = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService);
20352039

2036-
Experiment deliveryRule = mock(Experiment.class);
2037-
when(deliveryRule.getId()).thenReturn(ValidProjectConfigV4.EXPERIMENT_BASIC_EXPERIMENT_KEY);
2040+
Experiment deliveryRule = config.getExperimentIdMapping().get("1323241596");
20382041

20392042
DecisionResponse<FeatureDecision> response = ds.evaluateLocalHoldouts(
20402043
deliveryRule, config,
2041-
optimizely.createUserContext("any_user", Collections.<String, Object>emptyMap()),
2042-
true
2044+
optimizely.createUserContext("any_user", Collections.<String, Object>emptyMap())
20432045
);
20442046

2045-
assertNull(response.getResult());
2047+
assertNotNull(response.getResult());
2048+
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, response.getResult().decisionSource);
20462049
}
20472050

20482051
@Test
@@ -2073,8 +2076,7 @@ public void excludeTargetedDeliveries_evaluateLocalHoldouts_falseDoesNotSkipForD
20732076

20742077
DecisionResponse<FeatureDecision> response = ds.evaluateLocalHoldouts(
20752078
targetedRule, config,
2076-
optimizely.createUserContext("any_user", Collections.<String, Object>emptyMap()),
2077-
true
2079+
optimizely.createUserContext("any_user", Collections.<String, Object>emptyMap())
20782080
);
20792081

20802082
assertNotNull(response.getResult());
@@ -2105,6 +2107,67 @@ public void excludeTargetedDeliveries_forcedDecisionBeatsLocalHoldoutWithExclude
21052107
assertNotEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource);
21062108
}
21072109

2110+
//========= local holdout ignores excludeTargetedDeliveries tests =========/
2111+
2112+
@Test
2113+
public void localHoldout_ignoresExcludeTargetedDeliveries() {
2114+
ProjectConfig config = ValidProjectConfigV4.generateValidProjectConfigV4_localHoldoutExcludeTargetedDeliveries();
2115+
2116+
Bucketer bucketer = new Bucketer();
2117+
DecisionService ds = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService);
2118+
2119+
FeatureDecision decision = ds.getVariationForFeature(
2120+
ValidProjectConfigV4.FEATURE_FLAG_BASIC_EXPERIMENT_FEATURE,
2121+
optimizely.createUserContext("any_user", Collections.<String, Object>emptyMap()),
2122+
config
2123+
).getResult();
2124+
2125+
assertNotNull(decision);
2126+
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource);
2127+
assertEquals(HOLDOUT_LOCAL_EXCLUDE_TARGETED_DELIVERIES, decision.experiment);
2128+
}
2129+
2130+
@Test
2131+
public void globalHoldout_excludeTD_rolloutReturnsNull_returnsNull() {
2132+
ProjectConfig config = ValidProjectConfigV4.generateValidProjectConfigV4_globalHoldoutExcludeTargetedDeliveries();
2133+
2134+
Bucketer bucketer = new Bucketer();
2135+
DecisionService ds = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService);
2136+
2137+
FeatureDecision decision = ds.getVariationForFeature(
2138+
FEATURE_FLAG_BOOLEAN_FEATURE,
2139+
optimizely.createUserContext("any_user", Collections.<String, Object>emptyMap()),
2140+
config
2141+
).getResult();
2142+
2143+
assertTrue("When excludeTargetedDeliveries=true and rollout returns null, result should be null or have null variation",
2144+
decision == null || decision.variation == null);
2145+
if (decision != null) {
2146+
assertNotEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource);
2147+
}
2148+
}
2149+
2150+
@Test
2151+
public void globalHoldout_excludeTD_holdoutDecisionAttached() {
2152+
ProjectConfig config = ValidProjectConfigV4.generateValidProjectConfigV4_globalHoldoutExcludeTargetedDeliveries();
2153+
2154+
Bucketer bucketer = new Bucketer();
2155+
DecisionService ds = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService);
2156+
2157+
FeatureDecision decision = ds.getVariationForFeature(
2158+
FEATURE_FLAG_SINGLE_VARIABLE_INTEGER,
2159+
optimizely.createUserContext("any_user", Collections.<String, Object>emptyMap()),
2160+
config
2161+
).getResult();
2162+
2163+
assertNotNull(decision);
2164+
assertEquals(FeatureDecision.DecisionSource.ROLLOUT, decision.decisionSource);
2165+
assertNotNull("holdoutDecision should be attached when user is in holdout with excludeTargetedDeliveries=true",
2166+
decision.holdoutDecision);
2167+
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.holdoutDecision.decisionSource);
2168+
assertEquals(HOLDOUT_GLOBAL_EXCLUDE_TARGETED_DELIVERIES, decision.holdoutDecision.experiment);
2169+
}
2170+
21082171
private Experiment createMockCmabExperiment() {
21092172
List<Variation> variations = Arrays.asList(
21102173
new Variation("111151", "variation_1"),

0 commit comments

Comments
 (0)