Skip to content

Commit d7f392e

Browse files
alexjoeyyongalexjoeyyong
committed
[AI-FSSDK] [FSSDK-12958] Keep deprecated holdoutDecision alongside holdoutDecisions
1 parent 912a491 commit d7f392e

3 files changed

Lines changed: 85 additions & 0 deletions

File tree

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,7 @@ public List<DecisionResponse<FeatureDecision>> getVariationsForFeatureList(@Non
362362
boolean error = decisionVariationResponse.isError();
363363

364364
if (decision != null && (decision.variation != null || error)) {
365+
syncDeprecatedHoldoutDecision(decision);
365366
decisions.add(new DecisionResponse(decision, reasons, error, decision.cmabUuid));
366367
continue flagLoop;
367368
}
@@ -380,6 +381,7 @@ public List<DecisionResponse<FeatureDecision>> getVariationsForFeatureList(@Non
380381
}
381382
decision.holdoutDecisions.addAll(0, holdoutDecisions);
382383
}
384+
syncDeprecatedHoldoutDecision(decision);
383385

384386
if (decision != null && decision.variation != null) {
385387
String message = reasons.addInfo("The user \"%s\" was bucketed into a rollout for feature flag \"%s\".",
@@ -401,6 +403,13 @@ public List<DecisionResponse<FeatureDecision>> getVariationsForFeatureList(@Non
401403
return decisions;
402404
}
403405

406+
@SuppressWarnings("deprecation")
407+
private static void syncDeprecatedHoldoutDecision(@Nullable FeatureDecision decision) {
408+
if (decision != null && !decision.holdoutDecisions.isEmpty()) {
409+
decision.setHoldoutDecision(decision.holdoutDecisions.get(0));
410+
}
411+
}
412+
404413
@Nonnull
405414
public DecisionResponse<FeatureDecision> getVariationForFeature(@Nonnull FeatureFlag featureFlag,
406415
@Nonnull OptimizelyUserContext user,

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,24 @@ public class FeatureDecision {
4949
@Nullable
5050
public String cmabUuid;
5151

52+
/**
53+
* @deprecated use {@link #holdoutDecisions}
54+
*/
55+
@Deprecated
56+
@Nullable
57+
public FeatureDecision holdoutDecision;
58+
5259
@Nonnull
5360
public List<FeatureDecision> holdoutDecisions = new ArrayList<>();
5461

62+
/**
63+
* @deprecated use {@link #holdoutDecisions}
64+
*/
65+
@Deprecated
66+
public void setHoldoutDecision(@Nullable FeatureDecision holdoutDecision) {
67+
this.holdoutDecision = holdoutDecision;
68+
}
69+
5570
public enum DecisionSource {
5671
FEATURE_TEST("feature-test"),
5772
ROLLOUT("rollout"),

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

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import static org.junit.Assert.assertNotEquals;
3030
import static org.junit.Assert.assertNotNull;
3131
import static org.junit.Assert.assertNull;
32+
import static org.junit.Assert.assertSame;
3233
import static org.junit.Assert.assertTrue;
3334
import org.junit.Before;
3435
import org.junit.Rule;
@@ -59,6 +60,9 @@
5960
import com.optimizely.ab.cmab.service.CmabService;
6061
import com.optimizely.ab.cmab.service.CmabDecision;
6162
import com.optimizely.ab.config.Cmab;
63+
import com.google.common.base.Charsets;
64+
import com.google.common.io.Resources;
65+
import com.optimizely.ab.config.DatafileProjectConfig;
6266
import com.optimizely.ab.config.DatafileProjectConfigTestUtils;
6367
import static com.optimizely.ab.config.DatafileProjectConfigTestUtils.noAudienceProjectConfigV3;
6468
import static com.optimizely.ab.config.DatafileProjectConfigTestUtils.validProjectConfigV3;
@@ -1864,6 +1868,52 @@ public void localHoldout_hitIsRecordedAndEvaluationContinues() {
18641868
"User (any_user) is in variation (ho_off_key) of holdout (local_holdout_basic_experiment).");
18651869
}
18661870

1871+
private ProjectConfig localHoldoutsProjectConfig() throws Exception {
1872+
String datafile = Resources.toString(Resources.getResource("config/local-holdouts-project-config.json"), Charsets.UTF_8);
1873+
return new DatafileProjectConfig.Builder().withDatafile(datafile).build();
1874+
}
1875+
1876+
@Test
1877+
@SuppressWarnings("deprecation")
1878+
public void localHoldout_hitsOnSeveralRules_deprecatedHoldoutDecisionIsFirstHit() throws Exception {
1879+
ProjectConfig config = localHoldoutsProjectConfig();
1880+
DecisionService decisionService = new DecisionService(new Bucketer(), mockErrorHandler, null, mockCmabService);
1881+
1882+
FeatureDecision decision = decisionService.getVariationForFeature(
1883+
config.getFeatureKeyMapping().get("lh_flag"),
1884+
optimizely.createUserContext("any_user", Collections.<String, Object>singletonMap("lh", "exp1,exp2")),
1885+
config
1886+
).getResult();
1887+
1888+
assertNotNull(decision);
1889+
assertEquals(FeatureDecision.DecisionSource.ROLLOUT, decision.decisionSource);
1890+
assertEquals("delivery_rule", decision.experiment.getKey());
1891+
assertEquals(2, decision.holdoutDecisions.size());
1892+
assertEquals("lh_exp_rule_1", decision.holdoutDecisions.get(0).experiment.getKey());
1893+
assertEquals("lh_exp_rule_2", decision.holdoutDecisions.get(1).experiment.getKey());
1894+
assertSame(decision.holdoutDecisions.get(0), decision.holdoutDecision);
1895+
}
1896+
1897+
@Test
1898+
@SuppressWarnings("deprecation")
1899+
public void localHoldout_hitThenServedByLaterExperimentRule_deprecatedHoldoutDecisionIsFirstHit() throws Exception {
1900+
ProjectConfig config = localHoldoutsProjectConfig();
1901+
DecisionService decisionService = new DecisionService(new Bucketer(), mockErrorHandler, null, mockCmabService);
1902+
1903+
FeatureDecision decision = decisionService.getVariationForFeature(
1904+
config.getFeatureKeyMapping().get("lh_flag"),
1905+
optimizely.createUserContext("any_user", Collections.<String, Object>singletonMap("lh", "exp1")),
1906+
config
1907+
).getResult();
1908+
1909+
assertNotNull(decision);
1910+
assertEquals(FeatureDecision.DecisionSource.FEATURE_TEST, decision.decisionSource);
1911+
assertEquals("exp_rule_2", decision.experiment.getKey());
1912+
assertEquals(1, decision.holdoutDecisions.size());
1913+
assertEquals("lh_exp_rule_1", decision.holdoutDecisions.get(0).experiment.getKey());
1914+
assertSame(decision.holdoutDecisions.get(0), decision.holdoutDecision);
1915+
}
1916+
18671917
/**
18681918
* Local holdout miss: when a user does not hit the local holdout, they fall through
18691919
* to regular rule evaluation.
@@ -1976,6 +2026,7 @@ public void excludeTargetedDeliveries_globalHoldoutFalse_blocksAllRules() {
19762026
}
19772027

19782028
@Test
2029+
@SuppressWarnings("deprecation")
19792030
public void excludeTargetedDeliveries_globalHoldoutTrue_skipsExperimentRules() {
19802031
ProjectConfig config = ValidProjectConfigV4.generateValidProjectConfigV4_globalHoldoutExcludeTargetedDeliveries();
19812032

@@ -1991,6 +2042,8 @@ public void excludeTargetedDeliveries_globalHoldoutTrue_skipsExperimentRules() {
19912042
assertTrue("With excludeTargetedDeliveries=true and no rollout, variation should be null",
19922043
decision == null || decision.variation == null);
19932044
if (decision != null) {
2045+
assertNotNull("holdoutDecision should be attached", decision.holdoutDecision);
2046+
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.holdoutDecision.decisionSource);
19942047
assertEquals("holdoutDecision should be attached", 1, decision.holdoutDecisions.size());
19952048
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.holdoutDecisions.get(0).decisionSource);
19962049
}
@@ -2015,6 +2068,7 @@ public void excludeTargetedDeliveries_globalHoldoutTrue_allowsDeliveryRules() {
20152068
}
20162069

20172070
@Test
2071+
@SuppressWarnings("deprecation")
20182072
public void excludeTargetedDeliveries_globalHoldoutTrue_noDeliveryMatch_returnsNullWithHoldoutAttached() {
20192073
ProjectConfig config = ValidProjectConfigV4.generateValidProjectConfigV4_globalHoldoutExcludeTargetedDeliveries();
20202074

@@ -2030,6 +2084,8 @@ public void excludeTargetedDeliveries_globalHoldoutTrue_noDeliveryMatch_returnsN
20302084
assertTrue("When excludeTargetedDeliveries=true and no delivery match, variation should be null",
20312085
decision == null || decision.variation == null);
20322086
if (decision != null) {
2087+
assertNotNull("holdoutDecision should be attached", decision.holdoutDecision);
2088+
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.holdoutDecision.decisionSource);
20332089
assertEquals("holdoutDecision should be attached", 1, decision.holdoutDecisions.size());
20342090
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.holdoutDecisions.get(0).decisionSource);
20352091
}
@@ -2155,6 +2211,7 @@ public void globalHoldout_excludeTD_rolloutReturnsNull_returnsNull() {
21552211
}
21562212

21572213
@Test
2214+
@SuppressWarnings("deprecation")
21582215
public void globalHoldout_excludeTD_holdoutDecisionAttached() {
21592216
ProjectConfig config = ValidProjectConfigV4.generateValidProjectConfigV4_globalHoldoutExcludeTargetedDeliveries();
21602217

@@ -2169,6 +2226,10 @@ public void globalHoldout_excludeTD_holdoutDecisionAttached() {
21692226

21702227
assertNotNull(decision);
21712228
assertEquals(FeatureDecision.DecisionSource.ROLLOUT, decision.decisionSource);
2229+
assertNotNull("holdoutDecision should be attached when user is in holdout with excludeTargetedDeliveries=true",
2230+
decision.holdoutDecision);
2231+
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.holdoutDecision.decisionSource);
2232+
assertEquals(HOLDOUT_GLOBAL_EXCLUDE_TARGETED_DELIVERIES, decision.holdoutDecision.experiment);
21722233
assertEquals("holdoutDecision should be attached when user is in holdout with excludeTargetedDeliveries=true",
21732234
1, decision.holdoutDecisions.size());
21742235
assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.holdoutDecisions.get(0).decisionSource);

0 commit comments

Comments
 (0)