diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java index c68ea4575..086aabc7f 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java @@ -325,15 +325,14 @@ public List> getVariationsForFeatureList(@Non DecisionReasons reasons = DefaultDecisionReasons.newInstance(); reasons.merge(upsReasons); - List holdouts = projectConfig.getHoldoutForFlag(featureFlag.getId()); - if (!holdouts.isEmpty()) { - for (Holdout holdout : holdouts) { - DecisionResponse holdoutDecision = getVariationForHoldout(holdout, user, projectConfig); - reasons.merge(holdoutDecision.getReasons()); - if (holdoutDecision.getResult() != null) { - decisions.add(new DecisionResponse<>(new FeatureDecision(holdout, holdoutDecision.getResult(), FeatureDecision.DecisionSource.HOLDOUT), reasons)); - continue flagLoop; - } + // Evaluate global holdouts at the flag level (before any per-rule logic) + List globalHoldouts = projectConfig.getHoldoutConfig().getGlobalHoldouts(); + for (Holdout holdout : globalHoldouts) { + DecisionResponse holdoutDecision = getVariationForHoldout(holdout, user, projectConfig); + reasons.merge(holdoutDecision.getReasons()); + if (holdoutDecision.getResult() != null) { + decisions.add(new DecisionResponse<>(new FeatureDecision(holdout, holdoutDecision.getResult(), FeatureDecision.DecisionSource.HOLDOUT), reasons)); + continue flagLoop; } } @@ -399,6 +398,20 @@ DecisionResponse getVariationFromExperiment(@Nonnull ProjectCon for (String experimentId : featureFlag.getExperimentIds()) { Experiment experiment = projectConfig.getExperimentIdMapping().get(experimentId); + // Check local holdouts targeting this experiment rule before regular evaluation (FSSDK-12369) + if (experiment != null) { + List localHoldouts = projectConfig.getHoldoutConfig().getHoldoutsForRule(experiment.getId()); + for (Holdout holdout : localHoldouts) { + DecisionResponse holdoutDecision = getVariationForHoldout(holdout, user, projectConfig); + reasons.merge(holdoutDecision.getReasons()); + if (holdoutDecision.getResult() != null) { + return new DecisionResponse<>( + new FeatureDecision(holdout, holdoutDecision.getResult(), FeatureDecision.DecisionSource.HOLDOUT), + reasons); + } + } + } + DecisionResponse decisionVariation = getVariationFromExperimentRule(projectConfig, featureFlag.getKey(), experiment, user, options, userProfileTracker, decisionPath); reasons.merge(decisionVariation.getReasons()); @@ -468,6 +481,19 @@ DecisionResponse getVariationForFeatureInRollout(@Nonnull Featu int index = 0; while (index < rolloutRulesLength) { + Experiment rule = rollout.getExperiments().get(index); + + // Check local holdouts targeting this delivery rule before regular evaluation (FSSDK-12369) + List localHoldoutsForRule = projectConfig.getHoldoutConfig().getHoldoutsForRule(rule.getId()); + boolean localHoldoutHit = false; + for (Holdout holdout : localHoldoutsForRule) { + DecisionResponse holdoutDecision = getVariationForHoldout(holdout, user, projectConfig); + reasons.merge(holdoutDecision.getReasons()); + if (holdoutDecision.getResult() != null) { + FeatureDecision holdoutFeatureDecision = new FeatureDecision(holdout, holdoutDecision.getResult(), FeatureDecision.DecisionSource.HOLDOUT); + return new DecisionResponse(holdoutFeatureDecision, reasons); + } + } DecisionResponse decisionVariationResponse = getVariationFromDeliveryRule( projectConfig, @@ -482,7 +508,6 @@ DecisionResponse getVariationForFeatureInRollout(@Nonnull Featu Variation variation = response.getKey(); Boolean skipToEveryoneElse = response.getValue(); if (variation != null) { - Experiment rule = rollout.getExperiments().get(index); FeatureDecision featureDecision = new FeatureDecision(rule, variation, FeatureDecision.DecisionSource.ROLLOUT); return new DecisionResponse(featureDecision, reasons); } @@ -846,6 +871,7 @@ private DecisionResponse getVariationFromExperimentRule(@Nonnull Proj if (variation != null) { return new DecisionResponse(variation, reasons); } + //regular decision DecisionResponse decisionResponse = getVariation(rule, user, projectConfig, options, userProfileTracker, null, decisionPath); reasons.merge(decisionResponse.getReasons()); diff --git a/core-api/src/main/java/com/optimizely/ab/config/DatafileProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/DatafileProjectConfig.java index 0a892b286..c81db0486 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/DatafileProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/DatafileProjectConfig.java @@ -575,11 +575,16 @@ public List getHoldoutForFlag(@Nonnull String id) { return holdoutConfig.getHoldoutForFlag(id); } - @Override + @Override public Holdout getHoldout(@Nonnull String id) { return holdoutConfig.getHoldout(id); } + @Override + public HoldoutConfig getHoldoutConfig() { + return holdoutConfig; + } + @Override public Set getAllSegments() { return this.allSegments; diff --git a/core-api/src/main/java/com/optimizely/ab/config/Holdout.java b/core-api/src/main/java/com/optimizely/ab/config/Holdout.java index 8144b0d7d..2366f9c1b 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/Holdout.java +++ b/core-api/src/main/java/com/optimizely/ab/config/Holdout.java @@ -38,12 +38,20 @@ public class Holdout implements ExperimentCore { private final String id; private final String key; private final String status; - + private final List audienceIds; private final Condition audienceConditions; private final List variations; private final List trafficAllocation; + /** + * Optional list of rule IDs this holdout targets. + * null = global holdout (applies to all rules across all flags) + * non-null = local holdout (applies only to the specified rule IDs) + */ + @Nullable + private final List includedRules; + private final Map variationKeyToVariationMap; private final Map variationIdToVariationMap; // Not necessary for HO @@ -68,10 +76,22 @@ public String toString() { @VisibleForTesting public Holdout(String id, String key) { - this(id, key, "Running", Collections.emptyList(), null, Collections.emptyList(), Collections.emptyList()); + this(id, key, "Running", Collections.emptyList(), null, Collections.emptyList(), Collections.emptyList(), null); + } + + /** + * Convenience constructor for tests that do not need includedRules (maintains backward compatibility). + */ + public Holdout(@Nonnull String id, + @Nonnull String key, + @Nonnull String status, + @Nonnull List audienceIds, + @Nullable Condition audienceConditions, + @Nonnull List variations, + @Nonnull List trafficAllocation) { + this(id, key, status, audienceIds, audienceConditions, variations, trafficAllocation, null); } - // Keep only this constructor and add @JsonCreator to it @JsonCreator public Holdout(@JsonProperty("id") @Nonnull String id, @JsonProperty("key") @Nonnull String key, @@ -79,7 +99,8 @@ public Holdout(@JsonProperty("id") @Nonnull String id, @JsonProperty("audienceIds") @Nonnull List audienceIds, @JsonProperty("audienceConditions") @Nullable Condition audienceConditions, @JsonProperty("variations") @Nonnull List variations, - @JsonProperty("trafficAllocation") @Nonnull List trafficAllocation) { + @JsonProperty("trafficAllocation") @Nonnull List trafficAllocation, + @JsonProperty("includedRules") @Nullable List includedRules) { this.id = id; this.key = key; this.status = status; @@ -87,6 +108,7 @@ public Holdout(@JsonProperty("id") @Nonnull String id, this.audienceConditions = audienceConditions; this.variations = variations; this.trafficAllocation = trafficAllocation; + this.includedRules = includedRules; this.variationKeyToVariationMap = ProjectConfigUtils.generateNameMapping(this.variations); this.variationIdToVariationMap = ProjectConfigUtils.generateIdMapping(this.variations); } @@ -135,6 +157,27 @@ public String getGroupId() { return ""; } + /** + * Returns the list of rule IDs this holdout is scoped to, or null if this is a global holdout. + * + * @return null for a global holdout; a (possibly empty) list of rule IDs for a local holdout + */ + @Nullable + public List getIncludedRules() { + return includedRules; + } + + /** + * Returns {@code true} if this is a global holdout (applies to all rules across all flags). + * A holdout is global when {@code includedRules} is {@code null}. + * An empty list is NOT the same as global — it is a local holdout that targets no rules. + * + * @return true if global, false if local + */ + public boolean isGlobal() { + return includedRules == null; + } + public boolean isActive() { return status.equals(Holdout.HoldoutStatus.RUNNING.toString()); } @@ -154,6 +197,7 @@ public String toString() { + ", variations=" + variations + ", variationKeyToVariationMap=" + variationKeyToVariationMap + ", trafficAllocation=" + trafficAllocation + + ", includedRules=" + includedRules + '}'; } } diff --git a/core-api/src/main/java/com/optimizely/ab/config/HoldoutConfig.java b/core-api/src/main/java/com/optimizely/ab/config/HoldoutConfig.java index 77b9ba30f..e710dac26 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/HoldoutConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/HoldoutConfig.java @@ -28,13 +28,22 @@ import javax.annotation.Nullable; /** - * HoldoutConfig manages collections of Holdout objects. - * All holdouts are global and apply to all flags. + * HoldoutConfig manages collections of Holdout objects and provides access + * by global classification and by rule ID for local holdouts. */ public class HoldoutConfig { private List allHoldouts; private Map holdoutIdMap; + /** All holdouts whose {@code includedRules} is null (global holdouts). */ + private List globalHoldouts; + + /** + * Map from rule ID to the list of local holdouts targeting that rule. + * A holdout is local when its {@code includedRules} is non-null. + */ + private Map> ruleHoldoutsMap; + /** * Initializes a new HoldoutConfig with an empty list of holdouts. */ @@ -50,28 +59,73 @@ public HoldoutConfig() { public HoldoutConfig(@Nonnull List allHoldouts) { this.allHoldouts = new ArrayList<>(allHoldouts); this.holdoutIdMap = new HashMap<>(); + this.globalHoldouts = new ArrayList<>(); + this.ruleHoldoutsMap = new HashMap<>(); updateHoldoutMapping(); } /** - * Updates internal mapping of holdout IDs to holdout objects. + * Rebuilds all internal mappings from the current {@code allHoldouts} list. + * + *

Global holdouts (includedRules == null) are collected into {@code globalHoldouts}. + * Local holdouts (includedRules != null) are indexed by each rule ID they target + * into {@code ruleHoldoutsMap}. */ private void updateHoldoutMapping() { holdoutIdMap.clear(); + globalHoldouts.clear(); + ruleHoldoutsMap.clear(); + for (Holdout holdout : allHoldouts) { holdoutIdMap.put(holdout.getId(), holdout); + + if (holdout.isGlobal()) { + globalHoldouts.add(holdout); + } else { + List rules = holdout.getIncludedRules(); + // rules is non-null here because isGlobal() returned false + for (String ruleId : rules) { + ruleHoldoutsMap.computeIfAbsent(ruleId, k -> new ArrayList<>()).add(holdout); + } + } } } + /** + * Returns all global holdouts (those with {@code includedRules == null}). + * Global holdouts apply to every rule in every flag. + * + * @return An unmodifiable list of global holdouts + */ + @Nonnull + public List getGlobalHoldouts() { + return Collections.unmodifiableList(globalHoldouts); + } + + /** + * Returns all local holdouts that target the given rule ID. + * If no local holdouts are registered for this rule, an empty list is returned. + * + * @param ruleId The experiment or delivery rule ID to look up + * @return An unmodifiable list of holdouts targeting the specified rule + */ + @Nonnull + public List getHoldoutsForRule(@Nonnull String ruleId) { + List result = ruleHoldoutsMap.get(ruleId); + return result != null ? Collections.unmodifiableList(result) : Collections.emptyList(); + } + /** * Returns all holdouts for the given flag ID. - * Since all holdouts are now global, this returns all holdouts. + * For backward compatibility this returns all global holdouts. * - * @param id The flag identifier - * @return A list of all Holdout objects + * @param id The flag identifier (unused — retained for API compatibility) + * @return An unmodifiable list of global Holdout objects + * @deprecated Prefer {@link #getGlobalHoldouts()} for new code. */ + @Deprecated public List getHoldoutForFlag(@Nonnull String id) { - return Collections.unmodifiableList(allHoldouts); + return getGlobalHoldouts(); } /** @@ -90,6 +144,7 @@ public Holdout getHoldout(@Nonnull String id) { * * @return An unmodifiable list of all holdouts */ + @Nonnull public List getAllHoldouts() { return Collections.unmodifiableList(allHoldouts); } diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index 1872061dd..e9d306e1b 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -77,6 +77,8 @@ Experiment getExperimentForKey(@Nonnull String experimentKey, Holdout getHoldout(@Nonnull String id); + HoldoutConfig getHoldoutConfig(); + Set getAllSegments(); List getExperimentsForEventKey(String eventKey); diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/GsonHelpers.java b/core-api/src/main/java/com/optimizely/ab/config/parser/GsonHelpers.java index 8bd82dc0f..ff1413b08 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/GsonHelpers.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/GsonHelpers.java @@ -202,7 +202,17 @@ static Holdout parseHoldout(JsonObject holdoutJson, JsonDeserializationContext c List trafficAllocations = parseTrafficAllocation(holdoutJson.getAsJsonArray("trafficAllocation")); - return new Holdout(id, key, status, audienceIds, conditions, variations, trafficAllocations); + // Parse optional includedRules field (null = global holdout, non-null = local holdout) + List includedRules = null; + if (holdoutJson.has("includedRules") && !holdoutJson.get("includedRules").isJsonNull()) { + JsonArray includedRulesJson = holdoutJson.getAsJsonArray("includedRules"); + includedRules = new ArrayList<>(includedRulesJson.size()); + for (JsonElement ruleIdElement : includedRulesJson) { + includedRules.add(ruleIdElement.getAsString()); + } + } + + return new Holdout(id, key, status, audienceIds, conditions, variations, trafficAllocations, includedRules); } static FeatureFlag parseFeatureFlag(JsonObject featureFlagJson, JsonDeserializationContext context) { diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/JsonConfigParser.java b/core-api/src/main/java/com/optimizely/ab/config/parser/JsonConfigParser.java index b361031e2..ff582127a 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/JsonConfigParser.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/JsonConfigParser.java @@ -218,8 +218,18 @@ private List parseHoldouts(JSONArray holdoutJson) { List trafficAllocations = parseTrafficAllocation(holdoutObject.getJSONArray("trafficAllocation")); + // Parse optional includedRules field (null = global holdout, non-null = local holdout) + List includedRules = null; + if (holdoutObject.has("includedRules") && !holdoutObject.isNull("includedRules")) { + JSONArray includedRulesJson = holdoutObject.getJSONArray("includedRules"); + includedRules = new ArrayList<>(includedRulesJson.length()); + for (int j = 0; j < includedRulesJson.length(); j++) { + includedRules.add(includedRulesJson.getString(j)); + } + } + holdouts.add(new Holdout(id, key, status, audienceIds, conditions, variations, - trafficAllocations)); + trafficAllocations, includedRules)); } return holdouts; diff --git a/core-api/src/main/java/com/optimizely/ab/config/parser/JsonSimpleConfigParser.java b/core-api/src/main/java/com/optimizely/ab/config/parser/JsonSimpleConfigParser.java index 8491f1e3e..cadbfc2a3 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/parser/JsonSimpleConfigParser.java +++ b/core-api/src/main/java/com/optimizely/ab/config/parser/JsonSimpleConfigParser.java @@ -237,8 +237,18 @@ private List parseHoldouts(JSONArray holdoutJson) { List trafficAllocations = parseTrafficAllocation((JSONArray) hoObject.get("trafficAllocation")); + // Parse optional includedRules field (null = global holdout, non-null = local holdout) + List includedRules = null; + if (hoObject.containsKey("includedRules") && hoObject.get("includedRules") != null) { + JSONArray includedRulesJson = (JSONArray) hoObject.get("includedRules"); + includedRules = new ArrayList(includedRulesJson.size()); + for (Object ruleIdObj : includedRulesJson) { + includedRules.add((String) ruleIdObj); + } + } + holdouts.add(new Holdout(id, key, status, audienceIds, conditions, variations, - trafficAllocations)); + trafficAllocations, includedRules)); } return holdouts; diff --git a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java index c33bf95d7..ca164768e 100644 --- a/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java +++ b/core-api/src/test/java/com/optimizely/ab/bucketing/DecisionServiceTest.java @@ -65,6 +65,7 @@ import static com.optimizely.ab.config.DatafileProjectConfigTestUtils.validProjectConfigV4; import com.optimizely.ab.config.Experiment; import com.optimizely.ab.config.FeatureFlag; +import com.optimizely.ab.config.Holdout; import com.optimizely.ab.config.ProjectConfig; import com.optimizely.ab.config.Rollout; import com.optimizely.ab.config.TrafficAllocation; @@ -87,6 +88,7 @@ import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_3_EVERYONE_ELSE_RULE_ENABLED_VARIATION; import static com.optimizely.ab.config.ValidProjectConfigV4.VARIATION_HOLDOUT_VARIATION_OFF; import static com.optimizely.ab.config.ValidProjectConfigV4.generateValidProjectConfigV4_holdout; +import static com.optimizely.ab.config.ValidProjectConfigV4.generateValidProjectConfigV4WithHoldouts; import com.optimizely.ab.config.Variation; import com.optimizely.ab.error.ErrorHandler; import com.optimizely.ab.internal.ControlAttribute; @@ -1746,6 +1748,233 @@ public void getVariationStandardExperimentSavesUserProfile() throws Exception { String.format("Saved user profile of user \"%s\".", genericUserId)); } + // ========= Local Holdouts — FSSDK-12369 ========= + + /** + * Global holdout is evaluated at the flag level before any per-rule logic. + * A user bucketed into a global holdout should receive the holdout variation + * even when they would otherwise be bucketed into an experiment rule. + */ + @Test + public void localHoldouts_globalHoldoutEvaluatedBeforePerRuleLogic() { + ProjectConfig holdoutProjectConfig = generateValidProjectConfigV4_holdout(); + Bucketer mockBucketer = new Bucketer(); + DecisionService svc = new DecisionService(mockBucketer, mockErrorHandler, null, mockCmabService); + + // Bucketing ID "ppid160000" was confirmed to land in HOLDOUT_BASIC_HOLDOUT (global) + Map attributes = new HashMap<>(); + attributes.put("$opt_bucketing_id", "ppid160000"); + + FeatureDecision decision = svc.getVariationForFeature( + FEATURE_FLAG_BOOLEAN_FEATURE, + optimizely.createUserContext("user123", attributes), + holdoutProjectConfig + ).getResult(); + + assertEquals(HOLDOUT_BASIC_HOLDOUT, decision.experiment); + assertEquals(VARIATION_HOLDOUT_VARIATION_OFF, decision.variation); + assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource); + } + + /** + * A user bucketed into a local holdout targeting rule X receives the holdout variation + * for rule X; the rule's audience and traffic allocation are not evaluated. + * Uses FEATURE_FLAG_MULTI_VARIATE_FEATURE which links to EXPERIMENT_MULTIVARIATE_EXPERIMENT_ID. + */ + @Test + public void localHoldouts_userBucketedIntoLocalHoldoutReturnsHoldoutVariation() { + // EXPERIMENT_MULTIVARIATE_EXPERIMENT_ID = "3262035800" + // FEATURE_FLAG_MULTI_VARIATE_FEATURE links to this experiment + final String experimentRuleId = "3262035800"; + Holdout localHoldout = new Holdout( + "local_ho_1", + "local_holdout_rule_multivariate", + Holdout.HoldoutStatus.RUNNING.toString(), + Collections.emptyList(), + null, + DatafileProjectConfigTestUtils.createListOfObjects(VARIATION_HOLDOUT_VARIATION_OFF), + DatafileProjectConfigTestUtils.createListOfObjects( + new TrafficAllocation("$opt_dummy_variation_id", 10000) // 100% traffic + ), + Collections.singletonList(experimentRuleId) + ); + + ProjectConfig config = buildHoldoutProjectConfig(Collections.singletonList(localHoldout)); + Bucketer bucketer = new Bucketer(); + DecisionService svc = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService); + + FeatureDecision decision = svc.getVariationForFeature( + FEATURE_FLAG_MULTI_VARIATE_FEATURE, + optimizely.createUserContext("anyUser", Collections.emptyMap()), + config + ).getResult(); + + assertEquals(localHoldout, decision.experiment); + assertEquals(VARIATION_HOLDOUT_VARIATION_OFF, decision.variation); + assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource); + } + + /** + * A user NOT bucketed into a local holdout (0% traffic) falls through to regular rule evaluation. + */ + @Test + public void localHoldouts_userMissesLocalHoldoutFallsThroughToRuleEvaluation() { + final String experimentRuleId = "3262035800"; // EXPERIMENT_MULTIVARIATE_EXPERIMENT_ID + Holdout localHoldout = new Holdout( + "local_ho_miss", + "local_holdout_zero_traffic", + Holdout.HoldoutStatus.RUNNING.toString(), + Collections.emptyList(), + null, + DatafileProjectConfigTestUtils.createListOfObjects(VARIATION_HOLDOUT_VARIATION_OFF), + DatafileProjectConfigTestUtils.createListOfObjects( + new TrafficAllocation("$opt_dummy_variation_id", 0) // 0% — no one is bucketed + ), + Collections.singletonList(experimentRuleId) + ); + + ProjectConfig config = buildHoldoutProjectConfig(Collections.singletonList(localHoldout)); + Bucketer bucketer = new Bucketer(); + DecisionService svc = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService); + + // User should miss the local holdout and proceed to regular experiment rule evaluation + FeatureDecision decision = svc.getVariationForFeature( + FEATURE_FLAG_MULTI_VARIATE_FEATURE, + optimizely.createUserContext("anyUser", Collections.emptyMap()), + config + ).getResult(); + + // Decision source must NOT be HOLDOUT since the local holdout was missed + assertNotEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource); + } + + /** + * A local holdout targeting rule X does not affect rule Y. + * Specifically: a local holdout that targets experiment rule "3262035800" (multivariate experiment) + * should NOT affect rule Y — here we verify by using FEATURE_FLAG_BOOLEAN_FEATURE which has + * no experiment IDs and no rollout, so local holdouts targeting other rule IDs don't fire. + * Instead, we test specificity: the holdout for multivariate experiment must NOT appear when + * FEATURE_FLAG_BOOLEAN_FEATURE is evaluated (it has a different, empty experiment list). + * + * We use a purely made-up rule ID that matches neither experiment rules nor rollout rules + * of the tested feature flag, ensuring the local holdout is never triggered. + */ + @Test + public void localHoldouts_holdoutTargetingRuleXDoesNotAffectRuleY() { + // A made-up rule ID that is not present in ANY experiment or rollout rule in the config. + // This ensures the local holdout cannot intercept any rule evaluated for the feature flag. + final String nonExistentRuleId = "nonexistent_rule_999"; + Holdout localHoldoutForNonExistentRule = new Holdout( + "local_ho_ruleX", + "local_holdout_targeting_nonexistent_rule", + Holdout.HoldoutStatus.RUNNING.toString(), + Collections.emptyList(), + null, + DatafileProjectConfigTestUtils.createListOfObjects(VARIATION_HOLDOUT_VARIATION_OFF), + DatafileProjectConfigTestUtils.createListOfObjects( + new TrafficAllocation("$opt_dummy_variation_id", 10000) // 100% traffic for the nonexistent rule + ), + Collections.singletonList(nonExistentRuleId) + ); + + ProjectConfig config = buildHoldoutProjectConfig(Collections.singletonList(localHoldoutForNonExistentRule)); + Bucketer bucketer = new Bucketer(); + DecisionService svc = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService); + + // FEATURE_FLAG_MULTI_VARIATE_FEATURE uses experiment "3262035800" and ROLLOUT_2. + // The local holdout targets a rule ID that is in neither — it must not intercept. + FeatureDecision decision = svc.getVariationForFeature( + FEATURE_FLAG_MULTI_VARIATE_FEATURE, + optimizely.createUserContext("anyUser", Collections.emptyMap()), + config + ).getResult(); + + assertNotEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource); + } + + /** + * Local holdout check applies to experiment rules (getVariationFromExperimentRule path). + * Asserts that the holdout source is returned when user is bucketed into the local holdout + * that specifically targets the feature flag's linked experiment rule. + */ + @Test + public void localHoldouts_appliesToExperimentRules() { + final String experimentRuleId = "3262035800"; // EXPERIMENT_MULTIVARIATE_EXPERIMENT_ID + Holdout localHoldout = new Holdout( + "local_exp_ho", + "local_holdout_experiment_rule", + Holdout.HoldoutStatus.RUNNING.toString(), + Collections.emptyList(), + null, + DatafileProjectConfigTestUtils.createListOfObjects(VARIATION_HOLDOUT_VARIATION_OFF), + DatafileProjectConfigTestUtils.createListOfObjects( + new TrafficAllocation("$opt_dummy_variation_id", 10000) + ), + Collections.singletonList(experimentRuleId) + ); + + ProjectConfig config = buildHoldoutProjectConfig(Collections.singletonList(localHoldout)); + Bucketer bucketer = new Bucketer(); + DecisionService svc = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService); + + FeatureDecision decision = svc.getVariationForFeature( + FEATURE_FLAG_MULTI_VARIATE_FEATURE, + optimizely.createUserContext("user1", Collections.emptyMap()), + config + ).getResult(); + + assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource); + } + + /** + * Local holdout check applies to delivery (rollout) rules (getVariationFromDeliveryRule path). + * FEATURE_FLAG_MULTI_VARIATE_FEATURE uses ROLLOUT_2 which has ROLLOUT_2_RULE_1 (ID "3421010877"). + * We target that delivery rule with a local holdout to verify the delivery rule path is covered. + * The user has no audience match for ROLLOUT_2_RULE_1 (requires Gryffindor), but the local + * holdout check happens before audience evaluation, so using "everyone else" rule ID instead. + */ + @Test + public void localHoldouts_appliesToDeliveryRules() { + // ROLLOUT_2_EVERYONE_ELSE_RULE ID — the "everyone else" delivery rule in ROLLOUT_2 + // This rule has no audience requirement, so any user reaches it + final String deliveryRuleId = "828245624"; // ROLLOUT_2_EVERYONE_ELSE_RULE + Holdout localHoldout = new Holdout( + "local_delivery_ho", + "local_holdout_delivery_rule", + Holdout.HoldoutStatus.RUNNING.toString(), + Collections.emptyList(), + null, + DatafileProjectConfigTestUtils.createListOfObjects(VARIATION_HOLDOUT_VARIATION_OFF), + DatafileProjectConfigTestUtils.createListOfObjects( + new TrafficAllocation("$opt_dummy_variation_id", 10000) + ), + Collections.singletonList(deliveryRuleId) + ); + + // FEATURE_FLAG_MULTI_VARIATE_FEATURE uses ROLLOUT_2 for rollout fallback + ProjectConfig config = buildHoldoutProjectConfig(Collections.singletonList(localHoldout)); + Bucketer bucketer = new Bucketer(); + DecisionService svc = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService); + + // Use a user with no attributes — will miss experiment (no audience), then evaluate rollout + FeatureDecision decision = svc.getVariationForFeature( + FEATURE_FLAG_MULTI_VARIATE_FEATURE, + optimizely.createUserContext("user_delivery", Collections.emptyMap()), + config + ).getResult(); + + assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource); + } + + /** + * Builds a full DatafileProjectConfig using the same setup as generateValidProjectConfigV4_holdout() + * but with the given holdout list substituted. This avoids spy-based mocking and ensures that + * HoldoutConfig is built from the provided holdouts so that local holdout lookups work correctly. + */ + private ProjectConfig buildHoldoutProjectConfig(List holdouts) { + return ValidProjectConfigV4.generateValidProjectConfigV4WithHoldouts(holdouts); + } + private Experiment createMockCmabExperiment() { List variations = Arrays.asList( new Variation("111151", "variation_1"), diff --git a/core-api/src/test/java/com/optimizely/ab/config/HoldoutConfigTest.java b/core-api/src/test/java/com/optimizely/ab/config/HoldoutConfigTest.java index c0ddf7c71..183e6829e 100644 --- a/core-api/src/test/java/com/optimizely/ab/config/HoldoutConfigTest.java +++ b/core-api/src/test/java/com/optimizely/ab/config/HoldoutConfigTest.java @@ -35,12 +35,50 @@ public class HoldoutConfigTest { private Holdout holdout2; private Holdout holdout3; + /** Local holdout that targets rule "ruleA" only. */ + private Holdout localHoldoutRuleA; + /** Local holdout that targets both "ruleA" and "ruleB". */ + private Holdout localHoldoutRuleAAndB; + /** Local holdout with an empty includedRules list — targets no rule. */ + private Holdout localHoldoutEmptyRules; + @Before public void setUp() { - // All holdouts are now global (apply to all flags) + // Global holdouts — includedRules is null holdout1 = new Holdout("holdout1", "first_holdout"); holdout2 = new Holdout("holdout2", "second_holdout"); holdout3 = new Holdout("holdout3", "third_holdout"); + + // Local holdouts — includedRules is non-null + localHoldoutRuleA = new Holdout( + "local1", "local_ruleA", + "Running", + Collections.emptyList(), + null, + Collections.emptyList(), + Collections.emptyList(), + Collections.singletonList("ruleA") + ); + + localHoldoutRuleAAndB = new Holdout( + "local2", "local_ruleAB", + "Running", + Collections.emptyList(), + null, + Collections.emptyList(), + Collections.emptyList(), + Arrays.asList("ruleA", "ruleB") + ); + + localHoldoutEmptyRules = new Holdout( + "local3", "local_empty", + "Running", + Collections.emptyList(), + null, + Collections.emptyList(), + Collections.emptyList(), + Collections.emptyList() // empty list — not global, but targets no rule + ); } @Test @@ -126,4 +164,123 @@ public void testEmptyFlagHoldouts() { assertTrue(flagHoldouts.isEmpty()); } + // ========= Local Holdouts — FSSDK-12369 ========= + + /** + * isGlobal() returns true when includedRules is null (field absent in datafile). + */ + @Test + public void testIsGlobalReturnsTrueWhenIncludedRulesIsNull() { + // holdout1 was created with the 2-arg convenience constructor → includedRules == null + assertTrue(holdout1.isGlobal()); + assertNull(holdout1.getIncludedRules()); + } + + /** + * isGlobal() returns false when includedRules is a non-null list (even if empty). + */ + @Test + public void testIsGlobalReturnsFalseWhenIncludedRulesIsNonNull() { + assertFalse(localHoldoutRuleA.isGlobal()); + assertFalse(localHoldoutEmptyRules.isGlobal()); + } + + /** + * Empty includedRules list is NOT treated as global. + */ + @Test + public void testEmptyIncludedRulesIsNotGlobal() { + assertFalse(localHoldoutEmptyRules.isGlobal()); + assertNotNull(localHoldoutEmptyRules.getIncludedRules()); + assertTrue(localHoldoutEmptyRules.getIncludedRules().isEmpty()); + } + + /** + * getGlobalHoldouts() returns only holdouts with includedRules == null. + */ + @Test + public void testGetGlobalHoldoutsReturnsOnlyGlobalHoldouts() { + List holdouts = Arrays.asList(holdout1, localHoldoutRuleA, holdout2, localHoldoutEmptyRules); + HoldoutConfig config = new HoldoutConfig(holdouts); + + List globals = config.getGlobalHoldouts(); + assertEquals(2, globals.size()); + assertTrue(globals.contains(holdout1)); + assertTrue(globals.contains(holdout2)); + assertFalse(globals.contains(localHoldoutRuleA)); + assertFalse(globals.contains(localHoldoutEmptyRules)); + } + + /** + * getHoldoutsForRule() returns local holdouts that target a given rule ID. + */ + @Test + public void testGetHoldoutsForRuleReturnMatchingLocalHoldouts() { + List holdouts = Arrays.asList(holdout1, localHoldoutRuleA, localHoldoutRuleAAndB); + HoldoutConfig config = new HoldoutConfig(holdouts); + + List ruleAHoldouts = config.getHoldoutsForRule("ruleA"); + assertEquals(2, ruleAHoldouts.size()); + assertTrue(ruleAHoldouts.contains(localHoldoutRuleA)); + assertTrue(ruleAHoldouts.contains(localHoldoutRuleAAndB)); + + List ruleBHoldouts = config.getHoldoutsForRule("ruleB"); + assertEquals(1, ruleBHoldouts.size()); + assertTrue(ruleBHoldouts.contains(localHoldoutRuleAAndB)); + } + + /** + * getHoldoutsForRule() returns an empty list for an unknown rule ID. + */ + @Test + public void testGetHoldoutsForRuleUnknownRuleReturnsEmpty() { + HoldoutConfig config = new HoldoutConfig(Arrays.asList(localHoldoutRuleA)); + assertTrue(config.getHoldoutsForRule("unknownRule").isEmpty()); + } + + /** + * A holdout with an empty includedRules list does not appear in any rule's local holdout list. + */ + @Test + public void testEmptyIncludedRulesHoldoutDoesNotMatchAnyRule() { + HoldoutConfig config = new HoldoutConfig(Arrays.asList(localHoldoutEmptyRules)); + assertTrue(config.getGlobalHoldouts().isEmpty()); + assertTrue(config.getHoldoutsForRule("anyRule").isEmpty()); + } + + /** + * Backward compatibility: old datafiles without includedRules field → holdout treated as global. + */ + @Test + public void testBackwardCompatibilityNoIncludedRulesFieldTreatedAsGlobal() { + // The 7-arg constructor sets includedRules to null → global + Holdout legacyHoldout = new Holdout( + "legacy1", "legacy_holdout", + "Running", + Collections.emptyList(), + null, + Collections.emptyList(), + Collections.emptyList() + ); + assertTrue(legacyHoldout.isGlobal()); + + HoldoutConfig config = new HoldoutConfig(Collections.singletonList(legacyHoldout)); + assertEquals(1, config.getGlobalHoldouts().size()); + assertTrue(config.getGlobalHoldouts().contains(legacyHoldout)); + } + + /** + * getAllHoldouts() still returns both global and local holdouts. + */ + @Test + public void testGetAllHoldoutsContainsBothGlobalAndLocal() { + List holdouts = Arrays.asList(holdout1, localHoldoutRuleA, localHoldoutEmptyRules); + HoldoutConfig config = new HoldoutConfig(holdouts); + + assertEquals(3, config.getAllHoldouts().size()); + } + + private static void assertNotNull(Object obj) { + assertTrue("Expected non-null value", obj != null); + } } \ No newline at end of file diff --git a/core-api/src/test/java/com/optimizely/ab/config/ValidProjectConfigV4.java b/core-api/src/test/java/com/optimizely/ab/config/ValidProjectConfigV4.java index df62f048c..02dba3bb7 100644 --- a/core-api/src/test/java/com/optimizely/ab/config/ValidProjectConfigV4.java +++ b/core-api/src/test/java/com/optimizely/ab/config/ValidProjectConfigV4.java @@ -1534,6 +1534,19 @@ public static ProjectConfig generateValidProjectConfigV4() { } public static ProjectConfig generateValidProjectConfigV4_holdout() { + List holdouts = new ArrayList(); + holdouts.add(HOLDOUT_ZERO_TRAFFIC_HOLDOUT); + holdouts.add(HOLDOUT_BASIC_HOLDOUT); + holdouts.add(HOLDOUT_TYPEDAUDIENCE_HOLDOUT); + return generateValidProjectConfigV4WithHoldouts(holdouts); + } + + /** + * Builds the same holdout project config as {@link #generateValidProjectConfigV4_holdout()} + * but substitutes the given holdout list. Useful for testing local holdout scenarios + * without spy-based mocking. + */ + public static ProjectConfig generateValidProjectConfigV4WithHoldouts(List holdouts) { // list attributes List attributes = new ArrayList(); @@ -1580,12 +1593,6 @@ public static ProjectConfig generateValidProjectConfigV4_holdout() { experiments.add(EXPERIMENT_LAUNCHED_EXPERIMENT); experiments.add(EXPERIMENT_WITH_MALFORMED_AUDIENCE); - // list holdouts - List holdouts = new ArrayList(); - holdouts.add(HOLDOUT_ZERO_TRAFFIC_HOLDOUT); - holdouts.add(HOLDOUT_BASIC_HOLDOUT); - holdouts.add(HOLDOUT_TYPEDAUDIENCE_HOLDOUT); - // list featureFlags List featureFlags = new ArrayList(); featureFlags.add(FEATURE_FLAG_BOOLEAN_FEATURE);