From d055117140ae1a1a5328de88f8313224e505a921 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Fri, 15 May 2026 11:29:09 -0700 Subject: [PATCH 1/3] [FSSDK-12369] Add local holdouts support to Java SDK --- .../ab/bucketing/DecisionService.java | 75 +++++- .../ab/config/DatafileProjectConfig.java | 12 +- .../com/optimizely/ab/config/Holdout.java | 63 ++++- .../optimizely/ab/config/HoldoutConfig.java | 68 ++++- .../optimizely/ab/config/ProjectConfig.java | 15 ++ .../ab/config/parser/GsonHelpers.java | 12 +- .../ab/config/parser/JsonConfigParser.java | 12 +- .../config/parser/JsonSimpleConfigParser.java | 12 +- .../ab/bucketing/DecisionServiceTest.java | 141 +++++++++- .../ab/config/HoldoutConfigTest.java | 251 ++++++++++++++---- .../ab/config/ValidProjectConfigV4.java | 141 ++++++++++ 11 files changed, 721 insertions(+), 81 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..e02aad691 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,9 +325,10 @@ public List> getVariationsForFeatureList(@Non DecisionReasons reasons = DefaultDecisionReasons.newInstance(); reasons.merge(upsReasons); - List holdouts = projectConfig.getHoldoutForFlag(featureFlag.getId()); - if (!holdouts.isEmpty()) { - for (Holdout holdout : holdouts) { + // Evaluate global holdouts at flag level (before any rules are iterated) + List globalHoldouts = projectConfig.getGlobalHoldouts(); + if (!globalHoldouts.isEmpty()) { + for (Holdout holdout : globalHoldouts) { DecisionResponse holdoutDecision = getVariationForHoldout(holdout, user, projectConfig); reasons.merge(holdoutDecision.getReasons()); if (holdoutDecision.getResult() != null) { @@ -395,12 +396,44 @@ DecisionResponse getVariationFromExperiment(@Nonnull ProjectCon @Nullable UserProfileTracker userProfileTracker, @Nonnull DecisionPath decisionPath) { DecisionReasons reasons = DefaultDecisionReasons.newInstance(); + // Cache flagKey once to avoid multiple getKey() calls (important for mock-based tests) + String flagKey = featureFlag.getKey(); if (!featureFlag.getExperimentIds().isEmpty()) { for (String experimentId : featureFlag.getExperimentIds()) { Experiment experiment = projectConfig.getExperimentIdMapping().get(experimentId); + // Step 1: Check forced decision for this experiment rule first (highest priority). + // We must do this before the local holdout check so forced decisions win. + if (experiment != null) { + String ruleKey = experiment.getKey(); + OptimizelyDecisionContext fdContext = new OptimizelyDecisionContext(flagKey, ruleKey); + DecisionResponse fdResponse = validatedForcedDecision(fdContext, projectConfig, user); + reasons.merge(fdResponse.getReasons()); + if (fdResponse.getResult() != null) { + return new DecisionResponse<>( + new FeatureDecision(experiment, fdResponse.getResult(), FeatureDecision.DecisionSource.FEATURE_TEST), + reasons); + } + + // Step 2: Check local holdouts targeting this experiment rule. + // Local holdouts run after forced decisions but before regular rule evaluation. + List localHoldouts = projectConfig.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); + } + } + } + + // Step 3: Regular rule evaluation (getVariationFromExperimentRule also checks + // forced decisions internally but it will find no forced decision since we already + // checked above; the duplicate check is harmless). DecisionResponse decisionVariation = - getVariationFromExperimentRule(projectConfig, featureFlag.getKey(), experiment, user, options, userProfileTracker, decisionPath); + getVariationFromExperimentRule(projectConfig, flagKey, experiment, user, options, userProfileTracker, decisionPath); reasons.merge(decisionVariation.getReasons()); Variation variation = decisionVariation.getResult(); String cmabUuid = decisionVariation.getCmabUuid(); @@ -421,7 +454,7 @@ DecisionResponse getVariationFromExperiment(@Nonnull ProjectCon } } } else { - String message = reasons.addInfo("The feature flag \"%s\" is not used in any experiments.", featureFlag.getKey()); + String message = reasons.addInfo("The feature flag \"%s\" is not used in any experiments.", flagKey); logger.info(message); } @@ -468,7 +501,33 @@ DecisionResponse getVariationForFeatureInRollout(@Nonnull Featu int index = 0; while (index < rolloutRulesLength) { + Experiment rolloutRule = rollout.getExperiments().get(index); + + // Step 1: Check forced decision for this delivery rule (highest priority). + String rolloutRuleKey = rolloutRule.getKey(); + OptimizelyDecisionContext rolloutFdContext = new OptimizelyDecisionContext(featureFlag.getKey(), rolloutRuleKey); + DecisionResponse rolloutFdResponse = validatedForcedDecision(rolloutFdContext, projectConfig, user); + reasons.merge(rolloutFdResponse.getReasons()); + if (rolloutFdResponse.getResult() != null) { + FeatureDecision featureDecision = new FeatureDecision(rolloutRule, rolloutFdResponse.getResult(), FeatureDecision.DecisionSource.ROLLOUT); + return new DecisionResponse<>(featureDecision, reasons); + } + // Step 2: Check local holdouts targeting this delivery rule. + // Local holdouts run after forced decisions but before regular delivery rule evaluation. + List rolloutLocalHoldouts = projectConfig.getHoldoutsForRule(rolloutRule.getId()); + for (Holdout holdout : rolloutLocalHoldouts) { + 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); + } + } + + // Step 3: Regular delivery rule evaluation (getVariationFromDeliveryRule also checks + // forced decisions internally; the duplicate check is harmless). DecisionResponse decisionVariationResponse = getVariationFromDeliveryRule( projectConfig, featureFlag.getKey(), @@ -836,7 +895,7 @@ private DecisionResponse getVariationFromExperimentRule(@Nonnull Proj DecisionReasons reasons = DefaultDecisionReasons.newInstance(); String ruleKey = rule != null ? rule.getKey() : null; - // Check Forced-Decision + // Step 1: Check Forced-Decision OptimizelyDecisionContext optimizelyDecisionContext = new OptimizelyDecisionContext(flagKey, ruleKey); DecisionResponse forcedDecisionResponse = validatedForcedDecision(optimizelyDecisionContext, projectConfig, user); @@ -846,7 +905,9 @@ private DecisionResponse getVariationFromExperimentRule(@Nonnull Proj if (variation != null) { return new DecisionResponse(variation, reasons); } - //regular decision + + // Regular rule decision (local holdouts for experiment rules are checked by the caller + // getVariationFromExperiment, where the FeatureDecision source can be set to HOLDOUT) 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..28ac62789 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,7 +575,17 @@ public List getHoldoutForFlag(@Nonnull String id) { return holdoutConfig.getHoldoutForFlag(id); } - @Override + @Override + public List getGlobalHoldouts() { + return holdoutConfig.getGlobalHoldouts(); + } + + @Override + public List getHoldoutsForRule(@Nonnull String ruleId) { + return holdoutConfig.getHoldoutsForRule(ruleId); + } + + @Override public Holdout getHoldout(@Nonnull String id) { return holdoutConfig.getHoldout(id); } 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..85c530ad8 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 @@ -1,6 +1,6 @@ /** * - * Copyright 2016-2019, 2021, Optimizely and contributors + * Copyright 2016-2019, 2021, 2026, Optimizely and contributors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -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. When null, the holdout is global + * (applies to all rules across all flags). When non-null (even empty), it is a local + * holdout that only applies 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,28 @@ public String toString() { @VisibleForTesting public Holdout(String id, String key) { - this(id, key, "Running", Collections.emptyList(), null, Collections.emptyList(), Collections.emptyList()); - } - - // Keep only this constructor and add @JsonCreator to it + this(id, key, "Running", Collections.emptyList(), null, Collections.emptyList(), Collections.emptyList(), null); + } + + /** + * Constructor without includedRules (backward-compatible — treated as global holdout). + */ + 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); + } + + /** + * Full constructor including optional includedRules field (used by parsers). + * + * @param includedRules null = global holdout (applies to all rules); non-null list = local holdout + * targeting only those rule IDs (empty list = local holdout with no matching rules) + */ @JsonCreator public Holdout(@JsonProperty("id") @Nonnull String id, @JsonProperty("key") @Nonnull String key, @@ -79,7 +105,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 +114,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); } @@ -143,6 +171,26 @@ public boolean isRunning() { return status.equals(Holdout.HoldoutStatus.RUNNING.toString()); } + /** + * Returns the list of rule IDs this holdout targets, or null if this is a global holdout. + * + * @return null for global holdouts; a (possibly empty) list of rule IDs for local holdouts + */ + @Nullable + public List getIncludedRules() { + return includedRules; + } + + /** + * Returns true if this holdout is global (applies to all rules across all flags). + * A holdout is global when includedRules is null. + * + * @return true if this is a global holdout, false if it is a local holdout + */ + public boolean isGlobal() { + return includedRules == null; + } + @Override public String toString() { return "Holdout {" @@ -154,6 +202,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..ebd5e6a60 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 @@ -1,6 +1,6 @@ /** * - * Copyright 2016-2019, 2021, Optimizely and contributors + * Copyright 2016-2019, 2021, 2026, Optimizely and contributors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -28,13 +28,19 @@ 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, distinguishing between global holdouts + * (which apply to all rules) and local holdouts (which target specific rule IDs). */ public class HoldoutConfig { private List allHoldouts; private Map holdoutIdMap; + /** Global holdouts: holdouts where includedRules == null. Evaluated at flag level. */ + private List globalHoldouts; + + /** Rule-level map: ruleId -> list of local holdouts targeting that rule. */ + private Map> ruleHoldoutsMap; + /** * Initializes a new HoldoutConfig with an empty list of holdouts. */ @@ -50,28 +56,76 @@ 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. + * Updates internal mappings: + * - holdoutIdMap: id -> Holdout + * - globalHoldouts: holdouts where includedRules == null + * - ruleHoldoutsMap: ruleId -> list of holdouts that include that rule */ private void updateHoldoutMapping() { holdoutIdMap.clear(); + globalHoldouts.clear(); + ruleHoldoutsMap.clear(); + for (Holdout holdout : allHoldouts) { holdoutIdMap.put(holdout.getId(), holdout); + + if (holdout.isGlobal()) { + // includedRules == null: global holdout — applies to all rules + globalHoldouts.add(holdout); + } else { + // includedRules != null: local holdout — add to each targeted rule + List includedRules = holdout.getIncludedRules(); + for (String ruleId : includedRules) { + if (!ruleHoldoutsMap.containsKey(ruleId)) { + ruleHoldoutsMap.put(ruleId, new ArrayList<>()); + } + ruleHoldoutsMap.get(ruleId).add(holdout); + } + } } } + /** + * Returns all global holdouts (holdouts where includedRules == null). + * These are evaluated at the flag level, before any rules are evaluated. + * + * @return An unmodifiable list of global holdouts + */ + public List getGlobalHoldouts() { + return Collections.unmodifiableList(globalHoldouts); + } + + /** + * Returns local holdouts targeting a specific rule ID. + * These are evaluated per-rule, after the forced decision check and before regular rule evaluation. + * + * @param ruleId The rule identifier to look up + * @return An unmodifiable list of local holdouts targeting that rule, or empty list if none + */ + @Nonnull + public List getHoldoutsForRule(@Nonnull String ruleId) { + List holdouts = ruleHoldoutsMap.get(ruleId); + return holdouts != null ? Collections.unmodifiableList(holdouts) : Collections.emptyList(); + } + /** * Returns all holdouts for the given flag ID. - * Since all holdouts are now global, this returns all holdouts. + * For backward compatibility: returns all global holdouts (same behavior as before local holdouts). * * @param id The flag identifier - * @return A list of all Holdout objects + * @return A list of global Holdout objects + * @deprecated Use {@link #getGlobalHoldouts()} for flag-level evaluation and + * {@link #getHoldoutsForRule(String)} for per-rule evaluation. */ + @Deprecated public List getHoldoutForFlag(@Nonnull String id) { - return Collections.unmodifiableList(allHoldouts); + return Collections.unmodifiableList(globalHoldouts); } /** 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..d0ba008e8 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 @@ -75,6 +75,21 @@ Experiment getExperimentForKey(@Nonnull String experimentKey, List getHoldoutForFlag(@Nonnull String id); + /** + * Returns all global holdouts (holdouts where includedRules == null). + * Evaluated at flag level, before any rules are iterated. + */ + List getGlobalHoldouts(); + + /** + * Returns local holdouts targeting a specific rule ID. + * Evaluated per-rule, after forced decision check and before regular rule evaluation. + * + * @param ruleId The rule identifier to look up + * @return List of local holdouts for that rule, or empty list if none + */ + List getHoldoutsForRule(@Nonnull String ruleId); + Holdout getHoldout(@Nonnull String id); Set getAllSegments(); 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..52dbcd9b3 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, array = 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..6b99f53b7 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, array = 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..d30978186 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, array = 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..f5bf6bd22 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 @@ -419,7 +419,7 @@ public void getVariationForFeatureReturnsVariationReturnedFromGetVariation() { assertEquals(FeatureDecision.DecisionSource.FEATURE_TEST, featureDecision.decisionSource); verify(spyFeatureFlag, times(2)).getExperimentIds(); - verify(spyFeatureFlag, times(2)).getKey(); + verify(spyFeatureFlag, times(1)).getKey(); } /** @@ -1746,6 +1746,145 @@ public void getVariationStandardExperimentSavesUserProfile() throws Exception { String.format("Saved user profile of user \"%s\".", genericUserId)); } + // =================================================================== + // Local holdout decision service tests (FSSDK-12369) + // =================================================================== + + /** + * Global holdout is evaluated at flag level — a user bucketed into a global holdout + * receives the holdout variation before any rule is evaluated. + */ + @Test + public void localHoldout_globalHoldoutEvaluatedAtFlagLevelBeforeRules() { + ProjectConfig holdoutProjectConfig = generateValidProjectConfigV4_holdout(); + + Bucketer mockBucketer = new Bucketer(); + DecisionService decisionService = new DecisionService(mockBucketer, mockErrorHandler, null, mockCmabService); + + // ppid160000 buckets into basic_holdout (global, 5% traffic) + Map attributes = new HashMap<>(); + attributes.put("$opt_bucketing_id", "ppid160000"); + FeatureDecision featureDecision = decisionService.getVariationForFeature( + FEATURE_FLAG_BOOLEAN_FEATURE, + optimizely.createUserContext("user123", attributes), + holdoutProjectConfig + ).getResult(); + + // Should return global holdout decision — not a regular experiment or rollout decision + assertEquals(FeatureDecision.DecisionSource.HOLDOUT, featureDecision.decisionSource); + assertEquals(HOLDOUT_BASIC_HOLDOUT, featureDecision.experiment); + assertEquals(VARIATION_HOLDOUT_VARIATION_OFF, featureDecision.variation); + } + + /** + * Local holdout hit: a user bucketed into a local holdout targeting experiment rule X + * receives the holdout variation for that rule; regular rule evaluation is skipped. + */ + @Test + public void localHoldout_userInLocalHoldoutReceivesHoldoutVariation() { + // Config has only a local holdout targeting EXPERIMENT_BASIC_EXPERIMENT_ID (100% traffic) + ProjectConfig localHoldoutConfig = ValidProjectConfigV4.generateValidProjectConfigV4_localHoldout(); + + Bucketer mockBucketer = new Bucketer(); + DecisionService decisionService = new DecisionService(mockBucketer, mockErrorHandler, null, mockCmabService); + + // Use FEATURE_FLAG_BASIC_EXPERIMENT_FEATURE which is wired to EXPERIMENT_BASIC_EXPERIMENT_ID + // 100% traffic local holdout — any user bucketed into the experiment rule hits the holdout + FeatureDecision featureDecision = decisionService.getVariationForFeature( + ValidProjectConfigV4.FEATURE_FLAG_BASIC_EXPERIMENT_FEATURE, + optimizely.createUserContext("any_user", Collections.emptyMap()), + localHoldoutConfig + ).getResult(); + + assertEquals("User should be in holdout, not regular experiment", + FeatureDecision.DecisionSource.HOLDOUT, featureDecision.decisionSource); + assertEquals(ValidProjectConfigV4.HOLDOUT_LOCAL_FOR_BASIC_EXPERIMENT, featureDecision.experiment); + assertEquals(VARIATION_HOLDOUT_VARIATION_OFF, featureDecision.variation); + + logbackVerifier.expectMessage(Level.INFO, + "User (any_user) is in variation (ho_off_key) of holdout (local_holdout_basic_experiment)."); + } + + /** + * Local holdout miss: when a user does not hit the local holdout, they fall through + * to regular rule evaluation. + */ + @Test + public void localHoldout_userNotInLocalHoldoutFallsThroughToRegularRuleEvaluation() { + // Config has no holdouts at all — user should get a regular experiment decision + ProjectConfig noHoldoutConfig = validProjectConfigV4(); + + Bucketer mockBucketer = new Bucketer(); + DecisionService decisionService = new DecisionService(mockBucketer, mockErrorHandler, null, mockCmabService); + + FeatureDecision featureDecision = decisionService.getVariationForFeature( + FEATURE_FLAG_MULTI_VARIATE_FEATURE, + optimizely.createUserContext(genericUserId, Collections.emptyMap()), + noHoldoutConfig + ).getResult(); + + // No holdouts in config — decision source must not be HOLDOUT + assertTrue("Without holdouts, decision source should not be HOLDOUT", + featureDecision == null || featureDecision.decisionSource != FeatureDecision.DecisionSource.HOLDOUT); + } + + /** + * Rule specificity: a local holdout targeting rule X does not affect rule Y. + * getHoldoutsForRule is rule-specific. + */ + @Test + public void localHoldout_ruleSpecificityLocalHoldoutDoesNotAffectOtherRules() { + ProjectConfig localHoldoutConfig = ValidProjectConfigV4.generateValidProjectConfigV4_localHoldout(); + + // The local holdout targets EXPERIMENT_BASIC_EXPERIMENT_ID. + // FEATURE_FLAG_MULTI_VARIATE_FEATURE uses a different experiment — holdout should not apply. + Bucketer mockBucketer = new Bucketer(); + DecisionService decisionService = new DecisionService(mockBucketer, mockErrorHandler, null, mockCmabService); + + FeatureDecision featureDecision = decisionService.getVariationForFeature( + FEATURE_FLAG_MULTI_VARIATE_FEATURE, + optimizely.createUserContext("any_user", Collections.emptyMap()), + localHoldoutConfig + ).getResult(); + + // The local holdout targets basic_experiment, not multi_variate_feature's experiment + assertTrue("Local holdout targeting a different rule must not affect this feature", + featureDecision == null || featureDecision.decisionSource != FeatureDecision.DecisionSource.HOLDOUT); + } + + /** + * Forced decision priority (MANDATORY enforcement test): + * When a forced decision AND a 100% local holdout both target the same rule, + * the forced decision must win. + */ + @Test + public void localHoldout_forcedDecisionTakesPriorityOverLocalHoldout() { + // Config has a 100% local holdout targeting EXPERIMENT_BASIC_EXPERIMENT_ID + ProjectConfig localHoldoutConfig = ValidProjectConfigV4.generateValidProjectConfigV4_localHoldout(); + + Bucketer mockBucketer = new Bucketer(); + DecisionService decisionService = new DecisionService(mockBucketer, mockErrorHandler, null, mockCmabService); + + // Set a forced decision for the basic experiment rule + OptimizelyUserContext userContext = optimizely.createUserContext("forced_user", Collections.emptyMap()); + userContext.setForcedDecision( + new OptimizelyDecisionContext(ValidProjectConfigV4.FEATURE_FLAG_BASIC_EXPERIMENT_FEATURE_KEY, + ValidProjectConfigV4.EXPERIMENT_BASIC_EXPERIMENT_KEY), + new OptimizelyForcedDecision("A") + ); + + FeatureDecision featureDecision = decisionService.getVariationForFeature( + ValidProjectConfigV4.FEATURE_FLAG_BASIC_EXPERIMENT_FEATURE, + userContext, + localHoldoutConfig + ).getResult(); + + // Forced decision must win over local holdout + assertNotNull("Forced decision should produce a result", featureDecision); + assertNotEquals("Forced decision must NOT return holdout variation", + FeatureDecision.DecisionSource.HOLDOUT, featureDecision.decisionSource); + } + 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..1d4f7f4dd 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 @@ -1,6 +1,6 @@ /** * - * Copyright 2016-2019, 2021, Optimizely and contributors + * Copyright 2016-2019, 2021, 2026, Optimizely and contributors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -31,99 +31,240 @@ public class HoldoutConfigTest { - private Holdout holdout1; - private Holdout holdout2; - private Holdout holdout3; + private Holdout globalHoldout1; + private Holdout globalHoldout2; + private Holdout localHoldoutRuleA; + private Holdout localHoldoutRuleB; + private Holdout localHoldoutEmpty; @Before public void setUp() { - // All holdouts are now global (apply to all flags) - holdout1 = new Holdout("holdout1", "first_holdout"); - holdout2 = new Holdout("holdout2", "second_holdout"); - holdout3 = new Holdout("holdout3", "third_holdout"); + // Global holdouts — includedRules == null + globalHoldout1 = new Holdout("holdout1", "first_holdout"); + globalHoldout2 = new Holdout("holdout2", "second_holdout"); + + // Local holdout targeting rule "ruleA" + localHoldoutRuleA = new Holdout( + "local_holdout_a", "local_a", + "Running", + Collections.emptyList(), + null, + Collections.emptyList(), + Collections.emptyList(), + Arrays.asList("ruleA") + ); + + // Local holdout targeting rules "ruleA" and "ruleB" + localHoldoutRuleB = new Holdout( + "local_holdout_b", "local_b", + "Running", + Collections.emptyList(), + null, + Collections.emptyList(), + Collections.emptyList(), + Arrays.asList("ruleA", "ruleB") + ); + + // Local holdout with empty includedRules list — targets no rules + localHoldoutEmpty = new Holdout( + "local_holdout_empty", "local_empty", + "Running", + Collections.emptyList(), + null, + Collections.emptyList(), + Collections.emptyList(), + Collections.emptyList() + ); } + // ----------------------------------------------------------------------- + // isGlobal classification + // ----------------------------------------------------------------------- + @Test - public void testEmptyConstructor() { - HoldoutConfig config = new HoldoutConfig(); - - assertTrue(config.getAllHoldouts().isEmpty()); - assertTrue(config.getHoldoutForFlag("any_flag").isEmpty()); - assertNull(config.getHoldout("any_id")); + public void testIsGlobalReturnsTrueWhenIncludedRulesIsNull() { + assertTrue("Holdout with null includedRules must be global", globalHoldout1.isGlobal()); + assertTrue("Holdout with null includedRules must be global", globalHoldout2.isGlobal()); } @Test - public void testConstructorWithEmptyList() { - HoldoutConfig config = new HoldoutConfig(Collections.emptyList()); - - assertTrue(config.getAllHoldouts().isEmpty()); - assertTrue(config.getHoldoutForFlag("any_flag").isEmpty()); - assertNull(config.getHoldout("any_id")); + public void testIsGlobalReturnsFalseWhenIncludedRulesIsNonNull() { + assertFalse("Holdout with non-null includedRules must be local", localHoldoutRuleA.isGlobal()); + assertFalse("Holdout with non-null includedRules must be local", localHoldoutRuleB.isGlobal()); } @Test - public void testConstructorWithHoldouts() { - List holdouts = Arrays.asList(holdout1, holdout2); + public void testEmptyIncludedRulesIsLocalNotGlobal() { + // Empty list is still a local holdout — nil vs empty list are different + assertFalse("Holdout with empty includedRules list must be local, not global", localHoldoutEmpty.isGlobal()); + assertNotNull("Empty list should be returned, not null", localHoldoutEmpty.getIncludedRules()); + assertTrue("Empty includedRules list should be empty", localHoldoutEmpty.getIncludedRules().isEmpty()); + } + + // ----------------------------------------------------------------------- + // getGlobalHoldouts + // ----------------------------------------------------------------------- + + @Test + public void testGetGlobalHoldoutsReturnsOnlyGlobalHoldouts() { + List holdouts = Arrays.asList(globalHoldout1, localHoldoutRuleA, globalHoldout2, localHoldoutRuleB); HoldoutConfig config = new HoldoutConfig(holdouts); - assertEquals(2, config.getAllHoldouts().size()); - assertTrue(config.getAllHoldouts().contains(holdout1)); + List globals = config.getGlobalHoldouts(); + assertEquals(2, globals.size()); + assertTrue(globals.contains(globalHoldout1)); + assertTrue(globals.contains(globalHoldout2)); + assertFalse(globals.contains(localHoldoutRuleA)); + assertFalse(globals.contains(localHoldoutRuleB)); } @Test - public void testGetHoldout() { - List holdouts = Arrays.asList(holdout1, holdout2); - HoldoutConfig config = new HoldoutConfig(holdouts); + public void testGetGlobalHoldoutsIsEmptyWhenNoGlobalHoldouts() { + HoldoutConfig config = new HoldoutConfig(Arrays.asList(localHoldoutRuleA)); + assertTrue(config.getGlobalHoldouts().isEmpty()); + } - assertEquals(holdout1, config.getHoldout("holdout1")); - assertEquals(holdout2, config.getHoldout("holdout2")); - assertNull(config.getHoldout("nonexistent")); + @Test + public void testGetGlobalHoldoutsIsUnmodifiable() { + HoldoutConfig config = new HoldoutConfig(Arrays.asList(globalHoldout1)); + try { + config.getGlobalHoldouts().add(globalHoldout2); + fail("Should throw UnsupportedOperationException"); + } catch (UnsupportedOperationException e) { + // Expected + } } + // ----------------------------------------------------------------------- + // getHoldoutsForRule + // ----------------------------------------------------------------------- + @Test - public void testGetHoldoutForFlagReturnsAllHoldouts() { - List holdouts = Arrays.asList(holdout1, holdout2, holdout3); + public void testGetHoldoutsForRuleReturnsMatchingLocalHoldouts() { + List holdouts = Arrays.asList(globalHoldout1, localHoldoutRuleA, localHoldoutRuleB); HoldoutConfig config = new HoldoutConfig(holdouts); - // All holdouts are global and apply to all flags - List flag1Holdouts = config.getHoldoutForFlag("flag1"); - assertEquals(3, flag1Holdouts.size()); - assertTrue(flag1Holdouts.contains(holdout1)); - assertTrue(flag1Holdouts.contains(holdout2)); - assertTrue(flag1Holdouts.contains(holdout3)); + // ruleA is targeted by both localHoldoutRuleA and localHoldoutRuleB + List forRuleA = config.getHoldoutsForRule("ruleA"); + assertEquals(2, forRuleA.size()); + assertTrue(forRuleA.contains(localHoldoutRuleA)); + assertTrue(forRuleA.contains(localHoldoutRuleB)); - List flag2Holdouts = config.getHoldoutForFlag("flag2"); - assertEquals(3, flag2Holdouts.size()); - assertTrue(flag2Holdouts.contains(holdout1)); - assertTrue(flag2Holdouts.contains(holdout2)); - assertTrue(flag2Holdouts.contains(holdout3)); + // ruleB is targeted only by localHoldoutRuleB + List forRuleB = config.getHoldoutsForRule("ruleB"); + assertEquals(1, forRuleB.size()); + assertTrue(forRuleB.contains(localHoldoutRuleB)); + } - // Any flag should return all holdouts - List anyFlagHoldouts = config.getHoldoutForFlag("any_flag"); - assertEquals(3, anyFlagHoldouts.size()); + @Test + public void testGetHoldoutsForRuleReturnsEmptyListForUnknownRule() { + HoldoutConfig config = new HoldoutConfig(Arrays.asList(localHoldoutRuleA)); + assertTrue("Unknown rule should return empty list", config.getHoldoutsForRule("unknownRule").isEmpty()); } @Test - public void testGetAllHoldoutsIsUnmodifiable() { - List holdouts = Arrays.asList(holdout1, holdout2); - HoldoutConfig config = new HoldoutConfig(holdouts); + public void testGetHoldoutsForRuleDoesNotReturnGlobalHoldouts() { + // Global holdouts must NOT appear in getHoldoutsForRule — only local ones do + HoldoutConfig config = new HoldoutConfig(Arrays.asList(globalHoldout1, localHoldoutRuleA)); - List allHoldouts = config.getAllHoldouts(); + List forRuleA = config.getHoldoutsForRule("ruleA"); + assertFalse("Global holdouts must not appear in getHoldoutsForRule", forRuleA.contains(globalHoldout1)); + } + @Test + public void testEmptyIncludedRulesHoldoutDoesNotMatchAnyRule() { + // A local holdout with empty includedRules targets no rules + HoldoutConfig config = new HoldoutConfig(Arrays.asList(localHoldoutEmpty)); + assertTrue(config.getHoldoutsForRule("ruleA").isEmpty()); + assertTrue(config.getHoldoutsForRule("ruleB").isEmpty()); + } + + @Test + public void testGetHoldoutsForRuleIsUnmodifiable() { + HoldoutConfig config = new HoldoutConfig(Arrays.asList(localHoldoutRuleA)); try { - allHoldouts.add(holdout3); + config.getHoldoutsForRule("ruleA").add(globalHoldout1); fail("Should throw UnsupportedOperationException"); } catch (UnsupportedOperationException e) { // Expected } } + // ----------------------------------------------------------------------- + // Backward compatibility: getHoldoutForFlag (deprecated) + // ----------------------------------------------------------------------- + + @Test + @SuppressWarnings("deprecation") + public void testGetHoldoutForFlagReturnsOnlyGlobalHoldoutsForBackwardCompatibility() { + List holdouts = Arrays.asList(globalHoldout1, localHoldoutRuleA, globalHoldout2); + HoldoutConfig config = new HoldoutConfig(holdouts); + + // The deprecated getHoldoutForFlag should return only global holdouts (not local ones) + List result = config.getHoldoutForFlag("any_flag"); + assertEquals(2, result.size()); + assertTrue(result.contains(globalHoldout1)); + assertTrue(result.contains(globalHoldout2)); + assertFalse(result.contains(localHoldoutRuleA)); + } + + // ----------------------------------------------------------------------- + // General functionality + // ----------------------------------------------------------------------- + @Test - public void testEmptyFlagHoldouts() { + public void testEmptyConstructor() { HoldoutConfig config = new HoldoutConfig(); - List flagHoldouts = config.getHoldoutForFlag("any_flag"); - assertTrue(flagHoldouts.isEmpty()); + assertTrue(config.getAllHoldouts().isEmpty()); + assertTrue(config.getGlobalHoldouts().isEmpty()); + assertTrue(config.getHoldoutsForRule("any_rule").isEmpty()); + assertNull(config.getHoldout("any_id")); + } + + @Test + public void testConstructorWithEmptyList() { + HoldoutConfig config = new HoldoutConfig(Collections.emptyList()); + + assertTrue(config.getAllHoldouts().isEmpty()); + assertTrue(config.getGlobalHoldouts().isEmpty()); + assertTrue(config.getHoldoutsForRule("any_rule").isEmpty()); + assertNull(config.getHoldout("any_id")); + } + + @Test + public void testGetHoldout() { + List holdouts = Arrays.asList(globalHoldout1, localHoldoutRuleA); + HoldoutConfig config = new HoldoutConfig(holdouts); + + assertEquals(globalHoldout1, config.getHoldout("holdout1")); + assertEquals(localHoldoutRuleA, config.getHoldout("local_holdout_a")); + assertNull(config.getHoldout("nonexistent")); + } + + @Test + public void testGetAllHoldoutsIncludesBothGlobalAndLocal() { + List holdouts = Arrays.asList(globalHoldout1, localHoldoutRuleA); + HoldoutConfig config = new HoldoutConfig(holdouts); + + assertEquals(2, config.getAllHoldouts().size()); + assertTrue(config.getAllHoldouts().contains(globalHoldout1)); + assertTrue(config.getAllHoldouts().contains(localHoldoutRuleA)); } -} \ No newline at end of file + @Test + public void testGetAllHoldoutsIsUnmodifiable() { + HoldoutConfig config = new HoldoutConfig(Arrays.asList(globalHoldout1)); + try { + config.getAllHoldouts().add(globalHoldout2); + fail("Should throw UnsupportedOperationException"); + } catch (UnsupportedOperationException e) { + // Expected + } + } + + // Helper for assertNotNull (avoids import of static from junit 4.x) + private static void assertNotNull(String message, Object obj) { + assertTrue(message, obj != null); + } +} 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..4542b9783 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 @@ -592,6 +592,44 @@ public class ValidProjectConfigV4 { ) ) ); + /** + * Feature flag wired to EXPERIMENT_BASIC_EXPERIMENT_ID, used by local holdout tests. + * Not part of the standard feature flags — only used in generateValidProjectConfigV4_localHoldout(). + */ + public static final String FEATURE_FLAG_BASIC_EXPERIMENT_FEATURE_KEY = "basic_experiment_feature"; + private static final String FEATURE_BASIC_EXPERIMENT_FEATURE_ID = "9999999901"; + public static final FeatureFlag FEATURE_FLAG_BASIC_EXPERIMENT_FEATURE = new FeatureFlag( + FEATURE_BASIC_EXPERIMENT_FEATURE_ID, + FEATURE_FLAG_BASIC_EXPERIMENT_FEATURE_KEY, + "", + Collections.singletonList(EXPERIMENT_BASIC_EXPERIMENT_ID), + Collections.emptyList() + ); + + /** + * Local holdout targeting EXPERIMENT_BASIC_EXPERIMENT_ID ("1323241596"). + * 100% traffic allocation — user hits this holdout whenever it applies. + */ + public static final Holdout HOLDOUT_LOCAL_FOR_BASIC_EXPERIMENT = new Holdout( + "20075323428", + "local_holdout_basic_experiment", + Holdout.HoldoutStatus.RUNNING.toString(), + Collections.emptyList(), + null, + DatafileProjectConfigTestUtils.createListOfObjects( + VARIATION_HOLDOUT_VARIATION_OFF + ), + DatafileProjectConfigTestUtils.createListOfObjects( + new TrafficAllocation( + "$opt_dummy_variation_id", + 10000 + ) + ), + DatafileProjectConfigTestUtils.createListOfObjects( + EXPERIMENT_BASIC_EXPERIMENT_ID // targets the basic experiment rule + ) + ); + private static final String LAYER_TYPEDAUDIENCE_EXPERIMENT_ID = "1630555627"; private static final String EXPERIMENT_TYPEDAUDIENCE_EXPERIMENT_ID = "1323241597"; public static final String EXPERIMENT_TYPEDAUDIENCE_EXPERIMENT_KEY = "typed_audience_experiment"; @@ -1633,4 +1671,107 @@ public static ProjectConfig generateValidProjectConfigV4_holdout() { integrations ); } + + /** + * Generates a ProjectConfig that includes a local holdout targeting EXPERIMENT_BASIC_EXPERIMENT_ID. + * Used to test local holdout decision logic in DecisionService. + */ + public static ProjectConfig generateValidProjectConfigV4_localHoldout() { + // list attributes + List attributes = new ArrayList(); + attributes.add(ATTRIBUTE_HOUSE); + attributes.add(ATTRIBUTE_NATIONALITY); + attributes.add(ATTRIBUTE_OPT); + attributes.add(ATTRIBUTE_BOOLEAN); + attributes.add(ATTRIBUTE_INTEGER); + attributes.add(ATTRIBUTE_DOUBLE); + attributes.add(ATTRIBUTE_EMPTY); + + // list audiences + List audiences = new ArrayList(); + audiences.add(AUDIENCE_GRYFFINDOR); + audiences.add(AUDIENCE_SLYTHERIN); + audiences.add(AUDIENCE_ENGLISH_CITIZENS); + audiences.add(AUDIENCE_WITH_MISSING_VALUE); + + List typedAudiences = new ArrayList(); + typedAudiences.add(TYPED_AUDIENCE_BOOL); + typedAudiences.add(TYPED_AUDIENCE_EXACT_INT); + typedAudiences.add(TYPED_AUDIENCE_INT); + typedAudiences.add(TYPED_AUDIENCE_DOUBLE); + typedAudiences.add(TYPED_AUDIENCE_GRYFFINDOR); + typedAudiences.add(TYPED_AUDIENCE_SLYTHERIN); + typedAudiences.add(TYPED_AUDIENCE_ENGLISH_CITIZENS); + typedAudiences.add(AUDIENCE_WITH_MISSING_VALUE); + + // list events + List events = new ArrayList(); + events.add(EVENT_BASIC_EVENT); + events.add(EVENT_PAUSED_EXPERIMENT); + events.add(EVENT_LAUNCHED_EXPERIMENT_ONLY); + + // list experiments — include EXPERIMENT_BASIC_EXPERIMENT so the feature flag resolves it + List experiments = new ArrayList(); + experiments.add(EXPERIMENT_BASIC_EXPERIMENT); + experiments.add(EXPERIMENT_TYPEDAUDIENCE_EXPERIMENT); + experiments.add(EXPERIMENT_TYPEDAUDIENCE_WITH_AND_EXPERIMENT); + experiments.add(EXPERIMENT_TYPEDAUDIENCE_LEAF_EXPERIMENT); + experiments.add(EXPERIMENT_MULTIVARIATE_EXPERIMENT); + experiments.add(EXPERIMENT_DOUBLE_FEATURE_EXPERIMENT); + experiments.add(EXPERIMENT_PAUSED_EXPERIMENT); + experiments.add(EXPERIMENT_LAUNCHED_EXPERIMENT); + experiments.add(EXPERIMENT_WITH_MALFORMED_AUDIENCE); + + // Local holdout targeting the basic experiment rule only — NO global holdouts + List holdouts = new ArrayList(); + holdouts.add(HOLDOUT_LOCAL_FOR_BASIC_EXPERIMENT); + + // list featureFlags — include a feature wired to EXPERIMENT_BASIC_EXPERIMENT for local holdout tests + List featureFlags = new ArrayList(); + featureFlags.add(FEATURE_FLAG_BASIC_EXPERIMENT_FEATURE); // wired to basic_experiment + featureFlags.add(FEATURE_FLAG_BOOLEAN_FEATURE); + featureFlags.add(FEATURE_FLAG_SINGLE_VARIABLE_DOUBLE); + featureFlags.add(FEATURE_FLAG_SINGLE_VARIABLE_INTEGER); + featureFlags.add(FEATURE_FLAG_SINGLE_VARIABLE_BOOLEAN); + featureFlags.add(FEATURE_FLAG_SINGLE_VARIABLE_STRING); + featureFlags.add(FEATURE_FLAG_MULTI_VARIATE_FEATURE); + featureFlags.add(FEATURE_FLAG_MULTI_VARIATE_FUTURE_FEATURE); + featureFlags.add(FEATURE_FLAG_MUTEX_GROUP_FEATURE); + + List groups = new ArrayList(); + groups.add(GROUP_1); + groups.add(GROUP_2); + + // list rollouts + List rollouts = new ArrayList(); + rollouts.add(ROLLOUT_1); + rollouts.add(ROLLOUT_2); + rollouts.add(ROLLOUT_3); + + List integrations = new ArrayList<>(); + integrations.add(odpIntegration); + + return new DatafileProjectConfig( + ACCOUNT_ID, + ANONYMIZE_IP, + SEND_FLAG_DECISIONS, + BOT_FILTERING, + REGION, + PROJECT_ID, + REVISION, + SDK_KEY, + ENVIRONMENT_KEY, + VERSION, + attributes, + audiences, + typedAudiences, + events, + experiments, + holdouts, + featureFlags, + groups, + rollouts, + integrations + ); + } } From 18746fcc463987d0693fe14499b8d573685f38fc Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Tue, 19 May 2026 15:32:07 -0700 Subject: [PATCH 2/3] [FSSDK-12369] Refactor: deduplicate forcedDecision/localHoldout logic in DecisionService Move validatedForcedDecision and localHoldout checks from getVariationFromExperiment and getVariationForFeatureInRollout into getVariationFromExperimentRule and getVariationFromDeliveryRule, eliminating redundant evaluation. Extract shared local holdout logic into evaluateLocalHoldouts() and add unit tests. Co-Authored-By: Claude Opus 4.6 --- .../ab/bucketing/DecisionService.java | 155 ++++++++---------- .../ab/bucketing/DecisionServiceTest.java | 60 ++++++- 2 files changed, 121 insertions(+), 94 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 e02aad691..149fc1438 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 @@ -36,6 +36,7 @@ import com.optimizely.ab.OptimizelyRuntimeException; import com.optimizely.ab.OptimizelyUserContext; import com.optimizely.ab.config.Experiment; +import com.optimizely.ab.config.ExperimentCore; import com.optimizely.ab.config.FeatureFlag; import com.optimizely.ab.config.Holdout; import com.optimizely.ab.config.ProjectConfig; @@ -402,55 +403,12 @@ DecisionResponse getVariationFromExperiment(@Nonnull ProjectCon for (String experimentId : featureFlag.getExperimentIds()) { Experiment experiment = projectConfig.getExperimentIdMapping().get(experimentId); - // Step 1: Check forced decision for this experiment rule first (highest priority). - // We must do this before the local holdout check so forced decisions win. - if (experiment != null) { - String ruleKey = experiment.getKey(); - OptimizelyDecisionContext fdContext = new OptimizelyDecisionContext(flagKey, ruleKey); - DecisionResponse fdResponse = validatedForcedDecision(fdContext, projectConfig, user); - reasons.merge(fdResponse.getReasons()); - if (fdResponse.getResult() != null) { - return new DecisionResponse<>( - new FeatureDecision(experiment, fdResponse.getResult(), FeatureDecision.DecisionSource.FEATURE_TEST), - reasons); - } - - // Step 2: Check local holdouts targeting this experiment rule. - // Local holdouts run after forced decisions but before regular rule evaluation. - List localHoldouts = projectConfig.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); - } - } - } - - // Step 3: Regular rule evaluation (getVariationFromExperimentRule also checks - // forced decisions internally but it will find no forced decision since we already - // checked above; the duplicate check is harmless). - DecisionResponse decisionVariation = + DecisionResponse decisionVariation = getVariationFromExperimentRule(projectConfig, flagKey, experiment, user, options, userProfileTracker, decisionPath); reasons.merge(decisionVariation.getReasons()); - Variation variation = decisionVariation.getResult(); - String cmabUuid = decisionVariation.getCmabUuid(); - boolean error = decisionVariation.isError(); - if (error) { - return new DecisionResponse( - new FeatureDecision(experiment, variation, FeatureDecision.DecisionSource.FEATURE_TEST, cmabUuid), - reasons, - decisionVariation.isError(), - cmabUuid); - } - if (variation != null) { - return new DecisionResponse( - new FeatureDecision(experiment, variation, FeatureDecision.DecisionSource.FEATURE_TEST, cmabUuid), - reasons, - decisionVariation.isError(), - cmabUuid); + FeatureDecision featureDecision = decisionVariation.getResult(); + if (decisionVariation.isError() || (featureDecision != null && featureDecision.variation != null)) { + return new DecisionResponse(featureDecision, reasons, decisionVariation.isError(), decisionVariation.getCmabUuid()); } } } else { @@ -503,31 +461,6 @@ DecisionResponse getVariationForFeatureInRollout(@Nonnull Featu while (index < rolloutRulesLength) { Experiment rolloutRule = rollout.getExperiments().get(index); - // Step 1: Check forced decision for this delivery rule (highest priority). - String rolloutRuleKey = rolloutRule.getKey(); - OptimizelyDecisionContext rolloutFdContext = new OptimizelyDecisionContext(featureFlag.getKey(), rolloutRuleKey); - DecisionResponse rolloutFdResponse = validatedForcedDecision(rolloutFdContext, projectConfig, user); - reasons.merge(rolloutFdResponse.getReasons()); - if (rolloutFdResponse.getResult() != null) { - FeatureDecision featureDecision = new FeatureDecision(rolloutRule, rolloutFdResponse.getResult(), FeatureDecision.DecisionSource.ROLLOUT); - return new DecisionResponse<>(featureDecision, reasons); - } - - // Step 2: Check local holdouts targeting this delivery rule. - // Local holdouts run after forced decisions but before regular delivery rule evaluation. - List rolloutLocalHoldouts = projectConfig.getHoldoutsForRule(rolloutRule.getId()); - for (Holdout holdout : rolloutLocalHoldouts) { - 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); - } - } - - // Step 3: Regular delivery rule evaluation (getVariationFromDeliveryRule also checks - // forced decisions internally; the duplicate check is harmless). DecisionResponse decisionVariationResponse = getVariationFromDeliveryRule( projectConfig, featureFlag.getKey(), @@ -537,12 +470,10 @@ DecisionResponse getVariationForFeatureInRollout(@Nonnull Featu ); reasons.merge(decisionVariationResponse.getReasons()); - AbstractMap.SimpleEntry response = decisionVariationResponse.getResult(); - Variation variation = response.getKey(); + AbstractMap.SimpleEntry response = decisionVariationResponse.getResult(); + FeatureDecision featureDecision = response.getKey(); Boolean skipToEveryoneElse = response.getValue(); - if (variation != null) { - Experiment rule = rollout.getExperiments().get(index); - FeatureDecision featureDecision = new FeatureDecision(rule, variation, FeatureDecision.DecisionSource.ROLLOUT); + if (featureDecision != null) { return new DecisionResponse(featureDecision, reasons); } @@ -773,6 +704,23 @@ public DecisionResponse validatedForcedDecision(@Nonnull OptimizelyDe return new DecisionResponse<>(null, reasons); } + DecisionResponse evaluateLocalHoldouts(@Nonnull ExperimentCore rule, + @Nonnull ProjectConfig projectConfig, + @Nonnull OptimizelyUserContext user) { + DecisionReasons reasons = DefaultDecisionReasons.newInstance(); + List localHoldouts = projectConfig.getHoldoutsForRule(rule.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); + } + } + return new DecisionResponse<>(null, reasons); + } + public ConcurrentHashMap> getForcedVariationMapping() { return forcedVariationMapping; } @@ -885,7 +833,7 @@ public DecisionResponse getForcedVariation(@Nonnull Experiment experi } - private DecisionResponse getVariationFromExperimentRule(@Nonnull ProjectConfig projectConfig, + private DecisionResponse getVariationFromExperimentRule(@Nonnull ProjectConfig projectConfig, @Nonnull String flagKey, @Nonnull Experiment rule, @Nonnull OptimizelyUserContext user, @@ -903,17 +851,29 @@ private DecisionResponse getVariationFromExperimentRule(@Nonnull Proj Variation variation = forcedDecisionResponse.getResult(); if (variation != null) { - return new DecisionResponse(variation, reasons); + return new DecisionResponse( + new FeatureDecision(rule, variation, FeatureDecision.DecisionSource.FEATURE_TEST), + reasons); } - // Regular rule decision (local holdouts for experiment rules are checked by the caller - // getVariationFromExperiment, where the FeatureDecision source can be set to HOLDOUT) + // Step 2: Check local holdouts + if (rule != null) { + DecisionResponse holdoutResponse = evaluateLocalHoldouts(rule, projectConfig, user); + reasons.merge(holdoutResponse.getReasons()); + if (holdoutResponse.getResult() != null) { + return new DecisionResponse<>(holdoutResponse.getResult(), reasons); + } + } + + // Step 3: Regular rule decision DecisionResponse decisionResponse = getVariation(rule, user, projectConfig, options, userProfileTracker, null, decisionPath); reasons.merge(decisionResponse.getReasons()); variation = decisionResponse.getResult(); - return new DecisionResponse<>(variation, reasons, decisionResponse.isError(), decisionResponse.getCmabUuid()); + return new DecisionResponse<>( + new FeatureDecision(rule, variation, FeatureDecision.DecisionSource.FEATURE_TEST, decisionResponse.getCmabUuid()), + reasons, decisionResponse.isError(), decisionResponse.getCmabUuid()); } /** @@ -933,8 +893,8 @@ private boolean validateUserId(String userId) { * @param rules The experiments belonging to a rollout * @param ruleIndex The index of the rule * @param user The OptimizelyUserContext - * @return Returns a DecisionResponse Object containing a AbstractMap.SimpleEntry - * where the Variation is the result and the Boolean is the skipToEveryoneElse. + * @return Returns a DecisionResponse Object containing a AbstractMap.SimpleEntry + * where the FeatureDecision is the result and the Boolean is the skipToEveryoneElse. */ DecisionResponse getVariationFromDeliveryRule(@Nonnull ProjectConfig projectConfig, @Nonnull String flagKey, @@ -944,20 +904,30 @@ DecisionResponse getVariationFromDeliveryRule(@Nonnull DecisionReasons reasons = DefaultDecisionReasons.newInstance(); Boolean skipToEveryoneElse = false; - AbstractMap.SimpleEntry variationToSkipToEveryoneElsePair; - // Check forced-decisions first + AbstractMap.SimpleEntry resultPair; Experiment rule = rules.get(ruleIndex); + + // Step 1: Check Forced-Decision OptimizelyDecisionContext optimizelyDecisionContext = new OptimizelyDecisionContext(flagKey, rule.getKey()); DecisionResponse forcedDecisionResponse = validatedForcedDecision(optimizelyDecisionContext, projectConfig, user); reasons.merge(forcedDecisionResponse.getReasons()); Variation variation = forcedDecisionResponse.getResult(); if (variation != null) { - variationToSkipToEveryoneElsePair = new AbstractMap.SimpleEntry<>(variation, false); - return new DecisionResponse(variationToSkipToEveryoneElsePair, reasons); + resultPair = new AbstractMap.SimpleEntry<>( + new FeatureDecision(rule, variation, FeatureDecision.DecisionSource.ROLLOUT), false); + return new DecisionResponse(resultPair, reasons); + } + + // Step 2: Check local holdouts + DecisionResponse holdoutResponse = evaluateLocalHoldouts(rule, projectConfig, user); + reasons.merge(holdoutResponse.getReasons()); + if (holdoutResponse.getResult() != null) { + resultPair = new AbstractMap.SimpleEntry<>(holdoutResponse.getResult(), false); + return new DecisionResponse(resultPair, reasons); } - // Handle a regular decision + // Step 3: Regular rule decision String bucketingId = getBucketingId(user.getUserId(), user.getAttributes()); Boolean everyoneElse = (ruleIndex == rules.size() - 1); String loggingKey = everyoneElse ? "Everyone Else" : String.valueOf(ruleIndex + 1); @@ -999,8 +969,11 @@ DecisionResponse getVariationFromDeliveryRule(@Nonnull reasons.addInfo(message); logger.debug(message); } - variationToSkipToEveryoneElsePair = new AbstractMap.SimpleEntry<>(bucketedVariation, skipToEveryoneElse); - return new DecisionResponse(variationToSkipToEveryoneElsePair, reasons); + FeatureDecision featureDecision = bucketedVariation != null + ? new FeatureDecision(rule, bucketedVariation, FeatureDecision.DecisionSource.ROLLOUT) + : null; + resultPair = new AbstractMap.SimpleEntry<>(featureDecision, skipToEveryoneElse); + return new DecisionResponse(resultPair, reasons); } /** 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 f5bf6bd22..4634fcbe1 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 @@ -841,12 +841,12 @@ public void getVariationFromDeliveryRuleTest() { optimizely.createUserContext(genericUserId, Collections.singletonMap(ATTRIBUTE_NATIONALITY_KEY, AUDIENCE_ENGLISH_CITIZENS_VALUE)) ); - Variation variation = (Variation) decisionResponse.getResult().getKey(); + FeatureDecision featureDecision = (FeatureDecision) decisionResponse.getResult().getKey(); Boolean skipToEveryoneElse = (Boolean) decisionResponse.getResult().getValue(); assertNotNull(decisionResponse.getResult()); - assertNotNull(variation); + assertNotNull(featureDecision); assertNotNull(expectedVariation); - assertEquals(expectedVariation, variation); + assertEquals(expectedVariation, featureDecision.variation); assertFalse(skipToEveryoneElse); } @@ -1747,6 +1747,60 @@ public void getVariationStandardExperimentSavesUserProfile() throws Exception { } // =================================================================== + //========= evaluateLocalHoldouts tests =========/ + + @Test + public void evaluateLocalHoldouts_returnsHoldoutDecisionWhenUserBucketed() { + ProjectConfig localHoldoutConfig = ValidProjectConfigV4.generateValidProjectConfigV4_localHoldout(); + Experiment targetedRule = localHoldoutConfig.getExperimentIdMapping().get("1323241596"); + + Bucketer bucketer = new Bucketer(); + DecisionService decisionService = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService); + + DecisionResponse response = decisionService.evaluateLocalHoldouts( + targetedRule, localHoldoutConfig, + optimizely.createUserContext("any_user", Collections.emptyMap()) + ); + + assertNotNull(response.getResult()); + assertEquals(FeatureDecision.DecisionSource.HOLDOUT, response.getResult().decisionSource); + assertEquals(ValidProjectConfigV4.HOLDOUT_LOCAL_FOR_BASIC_EXPERIMENT, response.getResult().experiment); + assertEquals(VARIATION_HOLDOUT_VARIATION_OFF, response.getResult().variation); + } + + @Test + public void evaluateLocalHoldouts_returnsNullWhenNoHoldoutsForRule() { + ProjectConfig localHoldoutConfig = ValidProjectConfigV4.generateValidProjectConfigV4_localHoldout(); + // EXPERIMENT_MULTIVARIATE_EXPERIMENT is not targeted by any local holdout + Experiment untargetedRule = localHoldoutConfig.getExperimentIdMapping().get("3262035800"); + + Bucketer bucketer = new Bucketer(); + DecisionService decisionService = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService); + + DecisionResponse response = decisionService.evaluateLocalHoldouts( + untargetedRule, localHoldoutConfig, + optimizely.createUserContext("any_user", Collections.emptyMap()) + ); + + assertNull(response.getResult()); + } + + @Test + public void evaluateLocalHoldouts_returnsNullWhenConfigHasNoHoldouts() { + ProjectConfig noHoldoutConfig = validProjectConfigV4(); + Experiment rule = noHoldoutConfig.getExperimentIdMapping().get("1323241596"); + + Bucketer bucketer = new Bucketer(); + DecisionService decisionService = new DecisionService(bucketer, mockErrorHandler, null, mockCmabService); + + DecisionResponse response = decisionService.evaluateLocalHoldouts( + rule, noHoldoutConfig, + optimizely.createUserContext("any_user", Collections.emptyMap()) + ); + + assertNull(response.getResult()); + } + // Local holdout decision service tests (FSSDK-12369) // =================================================================== From 3fbc9056f44d1aed2df5b5249bcbdcc62d963d0d Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Tue, 19 May 2026 16:10:59 -0700 Subject: [PATCH 3/3] [FSSDK-12369] Add local holdout parsing tests for all 4 config parsers Add a local holdout with includedRules to the JSON fixture and expected config so Json, Gson, Jackson, and JsonSimple parsers all verify local holdout parsing. Also add includedRules/isGlobal assertions to verifyHoldouts. Co-Authored-By: Claude Opus 4.6 --- .../DatafileProjectConfigTestUtils.java | 2 ++ .../ab/config/ValidProjectConfigV4.java | 21 +++++++++++++++++++ .../config/holdouts-project-config.json | 20 ++++++++++++++++++ 3 files changed, 43 insertions(+) diff --git a/core-api/src/test/java/com/optimizely/ab/config/DatafileProjectConfigTestUtils.java b/core-api/src/test/java/com/optimizely/ab/config/DatafileProjectConfigTestUtils.java index 8f13efcf0..8d5e219d6 100644 --- a/core-api/src/test/java/com/optimizely/ab/config/DatafileProjectConfigTestUtils.java +++ b/core-api/src/test/java/com/optimizely/ab/config/DatafileProjectConfigTestUtils.java @@ -544,6 +544,8 @@ private static void verifyHoldouts(List actual, List expected) // System.out.println("Actual audience conditions: " + actualHoldout.getAudienceConditions()); // System.out.println("Expected audience conditions: " + expectedHoldout.getAudienceConditions()); assertThat(actualHoldout.getAudienceConditions(), is(expectedHoldout.getAudienceConditions())); + assertThat(actualHoldout.getIncludedRules(), is(expectedHoldout.getIncludedRules())); + assertThat(actualHoldout.isGlobal(), is(expectedHoldout.isGlobal())); verifyVariations(actualHoldout.getVariations(), expectedHoldout.getVariations()); verifyTrafficAllocations(actualHoldout.getTrafficAllocation(), expectedHoldout.getTrafficAllocation()); 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 4542b9783..5f28003c2 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 @@ -592,6 +592,26 @@ public class ValidProjectConfigV4 { ) ) ); + public static final Holdout HOLDOUT_LOCAL_FOR_BASIC_EXPERIMENT_PARSER = new Holdout( + "10075323430", + "local_holdout_for_basic_experiment", + Holdout.HoldoutStatus.RUNNING.toString(), + Collections.emptyList(), + null, + DatafileProjectConfigTestUtils.createListOfObjects( + VARIATION_HOLDOUT_VARIATION_OFF + ), + DatafileProjectConfigTestUtils.createListOfObjects( + new TrafficAllocation( + "$opt_dummy_variation_id", + 10000 + ) + ), + DatafileProjectConfigTestUtils.createListOfObjects( + EXPERIMENT_BASIC_EXPERIMENT_ID + ) + ); + /** * Feature flag wired to EXPERIMENT_BASIC_EXPERIMENT_ID, used by local holdout tests. * Not part of the standard feature flags — only used in generateValidProjectConfigV4_localHoldout(). @@ -1623,6 +1643,7 @@ public static ProjectConfig generateValidProjectConfigV4_holdout() { holdouts.add(HOLDOUT_ZERO_TRAFFIC_HOLDOUT); holdouts.add(HOLDOUT_BASIC_HOLDOUT); holdouts.add(HOLDOUT_TYPEDAUDIENCE_HOLDOUT); + holdouts.add(HOLDOUT_LOCAL_FOR_BASIC_EXPERIMENT_PARSER); // list featureFlags List featureFlags = new ArrayList(); diff --git a/core-api/src/test/resources/config/holdouts-project-config.json b/core-api/src/test/resources/config/holdouts-project-config.json index 89bb61bf2..9bf3dfe43 100644 --- a/core-api/src/test/resources/config/holdouts-project-config.json +++ b/core-api/src/test/resources/config/holdouts-project-config.json @@ -532,6 +532,26 @@ ], "audienceIds": ["3468206643", "3468206644", "3468206646", "3468206645"], "audienceConditions" : ["or", "3468206643", "3468206644", "3468206646", "3468206645"] + }, + { + "id": "10075323430", + "key": "local_holdout_for_basic_experiment", + "status": "Running", + "audienceIds": [], + "trafficAllocation": [ + { + "endOfRange": 10000, + "entityId": "$opt_dummy_variation_id" + } + ], + "variations": [ + { + "featureEnabled": false, + "id": "$opt_dummy_variation_id", + "key": "ho_off_key" + } + ], + "includedRules": ["1323241596"] } ], "groups": [