From c3fffde0dcfeaf4b32773ee461cdd9097a82c4db Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 14 May 2026 15:34:46 -0700 Subject: [PATCH 1/3] [AI-FSSDK] [FSSDK-12369] Add local holdouts support to Java SDK --- .../ab/bucketing/DecisionService.java | 46 +++- .../ab/config/DatafileProjectConfig.java | 7 +- .../com/optimizely/ab/config/Holdout.java | 52 +++- .../optimizely/ab/config/HoldoutConfig.java | 69 +++++- .../optimizely/ab/config/ProjectConfig.java | 2 + .../ab/config/parser/GsonHelpers.java | 12 +- .../ab/config/parser/JsonConfigParser.java | 12 +- .../config/parser/JsonSimpleConfigParser.java | 12 +- .../ab/bucketing/DecisionServiceTest.java | 229 ++++++++++++++++++ .../ab/config/HoldoutConfigTest.java | 159 +++++++++++- .../ab/config/ValidProjectConfigV4.java | 19 +- 11 files changed, 587 insertions(+), 32 deletions(-) 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); From 59256c178aa7204e161808c5b48d8d1520376e74 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Fri, 15 May 2026 10:31:37 -0700 Subject: [PATCH 2/3] [FSSDK-12369] Fix local holdout ordering: check after forced decision, not before MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move local holdout checks inside getVariationFromExperimentRule() and getVariationFromDeliveryRule(), directly after the forced decision block. Previously the checks were in the outer rule loop, before calling those methods, which caused local holdouts to run before forced decisions — violating the mandatory order: forced decision → local holdout → regular. Add enforcement test: forced decision beats a 100%-traffic local holdout. Co-Authored-By: Claude Sonnet 4.6 --- .../ab/bucketing/DecisionService.java | 47 ++++++++--------- .../ab/bucketing/DecisionServiceTest.java | 50 +++++++++++++++++++ 2 files changed, 71 insertions(+), 26 deletions(-) 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 086aabc7f..92925507f 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 @@ -398,20 +398,6 @@ 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()); @@ -483,18 +469,6 @@ DecisionResponse getVariationForFeatureInRollout(@Nonnull Featu 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, featureFlag.getKey(), @@ -872,6 +846,16 @@ private DecisionResponse getVariationFromExperimentRule(@Nonnull Proj return new DecisionResponse(variation, reasons); } + // Check local holdouts targeting this specific experiment rule (FSSDK-12369) + List localHoldouts = projectConfig.getHoldoutConfig().getHoldoutsForRule(rule.getId()); + for (Holdout holdout : localHoldouts) { + DecisionResponse holdoutDecision = getVariationForHoldout(holdout, user, projectConfig); + reasons.merge(holdoutDecision.getReasons()); + if (holdoutDecision.getResult() != null) { + return new DecisionResponse<>(holdoutDecision.getResult(), reasons); + } + } + //regular decision DecisionResponse decisionResponse = getVariation(rule, user, projectConfig, options, userProfileTracker, null, decisionPath); reasons.merge(decisionResponse.getReasons()); @@ -922,6 +906,17 @@ DecisionResponse getVariationFromDeliveryRule(@Nonnull return new DecisionResponse(variationToSkipToEveryoneElsePair, reasons); } + // Check local holdouts targeting this specific delivery rule (FSSDK-12369) + List localHoldouts = projectConfig.getHoldoutConfig().getHoldoutsForRule(rule.getId()); + for (Holdout holdout : localHoldouts) { + DecisionResponse holdoutDecision = getVariationForHoldout(holdout, user, projectConfig); + reasons.merge(holdoutDecision.getReasons()); + if (holdoutDecision.getResult() != null) { + variationToSkipToEveryoneElsePair = new AbstractMap.SimpleEntry<>(holdoutDecision.getResult(), false); + return new DecisionResponse(variationToSkipToEveryoneElsePair, reasons); + } + } + // Handle a regular decision String bucketingId = getBucketingId(user.getUserId(), user.getAttributes()); Boolean everyoneElse = (ruleIndex == rules.size() - 1); 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 ca164768e..5ea4f79fd 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 @@ -86,7 +86,9 @@ import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_2; import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_3_EVERYONE_ELSE_RULE; import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_3_EVERYONE_ELSE_RULE_ENABLED_VARIATION; +import static com.optimizely.ab.config.ValidProjectConfigV4.EXPERIMENT_MULTIVARIATE_EXPERIMENT_KEY; import static com.optimizely.ab.config.ValidProjectConfigV4.VARIATION_HOLDOUT_VARIATION_OFF; +import static com.optimizely.ab.config.ValidProjectConfigV4.VARIATION_MULTIVARIATE_EXPERIMENT_GRED_KEY; import static com.optimizely.ab.config.ValidProjectConfigV4.generateValidProjectConfigV4_holdout; import static com.optimizely.ab.config.ValidProjectConfigV4.generateValidProjectConfigV4WithHoldouts; import com.optimizely.ab.config.Variation; @@ -1966,6 +1968,54 @@ public void localHoldouts_appliesToDeliveryRules() { assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource); } + /** + * Forced decision takes priority over a 100%-traffic local holdout. + * Even when a local holdout would bucket the user, a forced decision for the same rule must win. + * This is the mandatory ordering enforcement test — if it fails, the per-rule ordering is wrong. + */ + @Test + public void localHoldouts_forcedDecisionTakesPriorityOverLocalHoldout() { + final String experimentRuleId = "3262035800"; // EXPERIMENT_MULTIVARIATE_EXPERIMENT_ID + final String flagKey = FEATURE_FLAG_MULTI_VARIATE_FEATURE.getKey(); // "multi_variate_feature" + final String ruleKey = EXPERIMENT_MULTIVARIATE_EXPERIMENT_KEY; // "multivariate_experiment" + + // 100% traffic local holdout targeting the same experiment rule + Holdout localHoldout = new Holdout( + "local_ho_forced_test", + "local_holdout_forced_decision_test", + Holdout.HoldoutStatus.RUNNING.toString(), + Collections.emptyList(), + null, + DatafileProjectConfigTestUtils.createListOfObjects(VARIATION_HOLDOUT_VARIATION_OFF), + DatafileProjectConfigTestUtils.createListOfObjects( + new TrafficAllocation("$opt_dummy_variation_id", 10000) // 100% — would always bucket + ), + Collections.singletonList(experimentRuleId) + ); + + ProjectConfig config = buildHoldoutProjectConfig(Collections.singletonList(localHoldout)); + Bucketer bucketer = new Bucketer(); + DecisionService svc = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService); + + // Set a forced decision for the same rule + OptimizelyUserContext userCtx = optimizely.createUserContext("anyUser", Collections.emptyMap()); + OptimizelyDecisionContext ctx = new OptimizelyDecisionContext(flagKey, ruleKey); + OptimizelyForcedDecision forcedDecision = new OptimizelyForcedDecision(VARIATION_MULTIVARIATE_EXPERIMENT_GRED_KEY); + userCtx.setForcedDecision(ctx, forcedDecision); + + FeatureDecision decision = svc.getVariationForFeature( + FEATURE_FLAG_MULTI_VARIATE_FEATURE, + userCtx, + config + ).getResult(); + + // Forced decision must win — NOT the holdout + assertNotEquals("Holdout must not win when forced decision is set", + FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource); + assertEquals("Forced variation key must be returned", + VARIATION_MULTIVARIATE_EXPERIMENT_GRED_KEY, decision.variation.getKey()); + } + /** * 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 From 9d5d325ab5db22341d537c9a3e309011d30a0bf5 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Fri, 15 May 2026 10:36:45 -0700 Subject: [PATCH 3/3] Revert "[FSSDK-12369] Fix local holdout ordering: check after forced decision, not before" This reverts commit 59256c178aa7204e161808c5b48d8d1520376e74. --- .../ab/bucketing/DecisionService.java | 47 +++++++++-------- .../ab/bucketing/DecisionServiceTest.java | 50 ------------------- 2 files changed, 26 insertions(+), 71 deletions(-) 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 92925507f..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 @@ -398,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()); @@ -469,6 +483,18 @@ DecisionResponse getVariationForFeatureInRollout(@Nonnull Featu 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, featureFlag.getKey(), @@ -846,16 +872,6 @@ private DecisionResponse getVariationFromExperimentRule(@Nonnull Proj return new DecisionResponse(variation, reasons); } - // Check local holdouts targeting this specific experiment rule (FSSDK-12369) - List localHoldouts = projectConfig.getHoldoutConfig().getHoldoutsForRule(rule.getId()); - for (Holdout holdout : localHoldouts) { - DecisionResponse holdoutDecision = getVariationForHoldout(holdout, user, projectConfig); - reasons.merge(holdoutDecision.getReasons()); - if (holdoutDecision.getResult() != null) { - return new DecisionResponse<>(holdoutDecision.getResult(), reasons); - } - } - //regular decision DecisionResponse decisionResponse = getVariation(rule, user, projectConfig, options, userProfileTracker, null, decisionPath); reasons.merge(decisionResponse.getReasons()); @@ -906,17 +922,6 @@ DecisionResponse getVariationFromDeliveryRule(@Nonnull return new DecisionResponse(variationToSkipToEveryoneElsePair, reasons); } - // Check local holdouts targeting this specific delivery rule (FSSDK-12369) - List localHoldouts = projectConfig.getHoldoutConfig().getHoldoutsForRule(rule.getId()); - for (Holdout holdout : localHoldouts) { - DecisionResponse holdoutDecision = getVariationForHoldout(holdout, user, projectConfig); - reasons.merge(holdoutDecision.getReasons()); - if (holdoutDecision.getResult() != null) { - variationToSkipToEveryoneElsePair = new AbstractMap.SimpleEntry<>(holdoutDecision.getResult(), false); - return new DecisionResponse(variationToSkipToEveryoneElsePair, reasons); - } - } - // Handle a regular decision String bucketingId = getBucketingId(user.getUserId(), user.getAttributes()); Boolean everyoneElse = (ruleIndex == rules.size() - 1); 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 5ea4f79fd..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 @@ -86,9 +86,7 @@ import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_2; import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_3_EVERYONE_ELSE_RULE; import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_3_EVERYONE_ELSE_RULE_ENABLED_VARIATION; -import static com.optimizely.ab.config.ValidProjectConfigV4.EXPERIMENT_MULTIVARIATE_EXPERIMENT_KEY; import static com.optimizely.ab.config.ValidProjectConfigV4.VARIATION_HOLDOUT_VARIATION_OFF; -import static com.optimizely.ab.config.ValidProjectConfigV4.VARIATION_MULTIVARIATE_EXPERIMENT_GRED_KEY; import static com.optimizely.ab.config.ValidProjectConfigV4.generateValidProjectConfigV4_holdout; import static com.optimizely.ab.config.ValidProjectConfigV4.generateValidProjectConfigV4WithHoldouts; import com.optimizely.ab.config.Variation; @@ -1968,54 +1966,6 @@ public void localHoldouts_appliesToDeliveryRules() { assertEquals(FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource); } - /** - * Forced decision takes priority over a 100%-traffic local holdout. - * Even when a local holdout would bucket the user, a forced decision for the same rule must win. - * This is the mandatory ordering enforcement test — if it fails, the per-rule ordering is wrong. - */ - @Test - public void localHoldouts_forcedDecisionTakesPriorityOverLocalHoldout() { - final String experimentRuleId = "3262035800"; // EXPERIMENT_MULTIVARIATE_EXPERIMENT_ID - final String flagKey = FEATURE_FLAG_MULTI_VARIATE_FEATURE.getKey(); // "multi_variate_feature" - final String ruleKey = EXPERIMENT_MULTIVARIATE_EXPERIMENT_KEY; // "multivariate_experiment" - - // 100% traffic local holdout targeting the same experiment rule - Holdout localHoldout = new Holdout( - "local_ho_forced_test", - "local_holdout_forced_decision_test", - Holdout.HoldoutStatus.RUNNING.toString(), - Collections.emptyList(), - null, - DatafileProjectConfigTestUtils.createListOfObjects(VARIATION_HOLDOUT_VARIATION_OFF), - DatafileProjectConfigTestUtils.createListOfObjects( - new TrafficAllocation("$opt_dummy_variation_id", 10000) // 100% — would always bucket - ), - Collections.singletonList(experimentRuleId) - ); - - ProjectConfig config = buildHoldoutProjectConfig(Collections.singletonList(localHoldout)); - Bucketer bucketer = new Bucketer(); - DecisionService svc = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService); - - // Set a forced decision for the same rule - OptimizelyUserContext userCtx = optimizely.createUserContext("anyUser", Collections.emptyMap()); - OptimizelyDecisionContext ctx = new OptimizelyDecisionContext(flagKey, ruleKey); - OptimizelyForcedDecision forcedDecision = new OptimizelyForcedDecision(VARIATION_MULTIVARIATE_EXPERIMENT_GRED_KEY); - userCtx.setForcedDecision(ctx, forcedDecision); - - FeatureDecision decision = svc.getVariationForFeature( - FEATURE_FLAG_MULTI_VARIATE_FEATURE, - userCtx, - config - ).getResult(); - - // Forced decision must win — NOT the holdout - assertNotEquals("Holdout must not win when forced decision is set", - FeatureDecision.DecisionSource.HOLDOUT, decision.decisionSource); - assertEquals("Forced variation key must be returned", - VARIATION_MULTIVARIATE_EXPERIMENT_GRED_KEY, decision.variation.getKey()); - } - /** * 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