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 c757c072c..8144b0d7d 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 @@ -43,8 +43,6 @@ public class Holdout implements ExperimentCore { private final Condition audienceConditions; private final List variations; private final List trafficAllocation; - private final List includedFlags; - private final List excludedFlags; private final Map variationKeyToVariationMap; private final Map variationIdToVariationMap; @@ -70,7 +68,7 @@ public String toString() { @VisibleForTesting public Holdout(String id, String key) { - this(id, key, "Running", Collections.emptyList(), null, Collections.emptyList(), Collections.emptyList(), null, null); + this(id, key, "Running", Collections.emptyList(), null, Collections.emptyList(), Collections.emptyList()); } // Keep only this constructor and add @JsonCreator to it @@ -81,9 +79,7 @@ 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("includedFlags") @Nullable List includedFlags, - @JsonProperty("excludedFlags") @Nullable List excludedFlags) { + @JsonProperty("trafficAllocation") @Nonnull List trafficAllocation) { this.id = id; this.key = key; this.status = status; @@ -91,8 +87,6 @@ public Holdout(@JsonProperty("id") @Nonnull String id, this.audienceConditions = audienceConditions; this.variations = variations; this.trafficAllocation = trafficAllocation; - this.includedFlags = includedFlags == null ? Collections.emptyList() : Collections.unmodifiableList(includedFlags); - this.excludedFlags = excludedFlags == null ? Collections.emptyList() : Collections.unmodifiableList(excludedFlags); this.variationKeyToVariationMap = ProjectConfigUtils.generateNameMapping(this.variations); this.variationIdToVariationMap = ProjectConfigUtils.generateIdMapping(this.variations); } @@ -141,14 +135,6 @@ public String getGroupId() { return ""; } - public List getIncludedFlags() { - return includedFlags; - } - - public List getExcludedFlags() { - return excludedFlags; - } - public boolean isActive() { return status.equals(Holdout.HoldoutStatus.RUNNING.toString()); } 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 69635b1ae..77b9ba30f 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 @@ -21,25 +21,19 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import javax.annotation.Nonnull; import javax.annotation.Nullable; /** - * HoldoutConfig manages collections of Holdout objects and their relationships to flags. + * HoldoutConfig manages collections of Holdout objects. + * All holdouts are global and apply to all flags. */ public class HoldoutConfig { private List allHoldouts; - private List global; private Map holdoutIdMap; - private Map> flagHoldoutsMap; - private Map> includedHoldouts; - private Map> excludedHoldouts; /** * Initializes a new HoldoutConfig with an empty list of holdouts. @@ -55,91 +49,29 @@ public HoldoutConfig() { */ public HoldoutConfig(@Nonnull List allHoldouts) { this.allHoldouts = new ArrayList<>(allHoldouts); - this.global = new ArrayList<>(); this.holdoutIdMap = new HashMap<>(); - this.flagHoldoutsMap = new ConcurrentHashMap<>(); - this.includedHoldouts = new HashMap<>(); - this.excludedHoldouts = new HashMap<>(); updateHoldoutMapping(); } /** - * Updates internal mappings of holdouts including the id map, global list, - * and per-flag inclusion/exclusion maps. + * Updates internal mapping of holdout IDs to holdout objects. */ private void updateHoldoutMapping() { holdoutIdMap.clear(); for (Holdout holdout : allHoldouts) { holdoutIdMap.put(holdout.getId(), holdout); } - - flagHoldoutsMap.clear(); - global.clear(); - includedHoldouts.clear(); - excludedHoldouts.clear(); - - for (Holdout holdout : allHoldouts) { - boolean hasIncludedFlags = !holdout.getIncludedFlags().isEmpty(); - boolean hasExcludedFlags = !holdout.getExcludedFlags().isEmpty(); - - if (!hasIncludedFlags && !hasExcludedFlags) { - // Global holdout (applies to all flags) - global.add(holdout); - } else if (hasIncludedFlags) { - // Holdout only applies to specific included flags - for (String flagId : holdout.getIncludedFlags()) { - includedHoldouts.computeIfAbsent(flagId, k -> new ArrayList<>()).add(holdout); - } - } else { - // Global holdout with specific exclusions - global.add(holdout); - - for (String flagId : holdout.getExcludedFlags()) { - excludedHoldouts.computeIfAbsent(flagId, k -> new HashSet<>()).add(holdout); - } - } - } } /** - * Returns the applicable holdouts for the given flag ID by combining global holdouts - * (excluding any specified) and included holdouts, in that order. - * Caches the result for future calls. + * Returns all holdouts for the given flag ID. + * Since all holdouts are now global, this returns all holdouts. * * @param id The flag identifier - * @return A list of Holdout objects relevant to the given flag + * @return A list of all Holdout objects */ public List getHoldoutForFlag(@Nonnull String id) { - if (allHoldouts.isEmpty()) { - return Collections.emptyList(); - } - - // Check cache and return persistent holdouts - if (flagHoldoutsMap.containsKey(id)) { - return flagHoldoutsMap.get(id); - } - - // Prioritize global holdouts first - List activeHoldouts = new ArrayList<>(); - Set excluded = excludedHoldouts.getOrDefault(id, Collections.emptySet()); - - if (!excluded.isEmpty()) { - for (Holdout holdout : global) { - if (!excluded.contains(holdout)) { - activeHoldouts.add(holdout); - } - } - } else { - activeHoldouts.addAll(global); - } - - // Add included holdouts - activeHoldouts.addAll(includedHoldouts.getOrDefault(id, Collections.emptyList())); - - // Cache the result - flagHoldoutsMap.put(id, activeHoldouts); - - return activeHoldouts; + return Collections.unmodifiableList(allHoldouts); } /** 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 cfebbd9c8..8bd82dc0f 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,23 +202,7 @@ static Holdout parseHoldout(JsonObject holdoutJson, JsonDeserializationContext c List trafficAllocations = parseTrafficAllocation(holdoutJson.getAsJsonArray("trafficAllocation")); - List includedFlags = new ArrayList<>(); - if (holdoutJson.has("includedFlags")) { - JsonArray includedIdsJson = holdoutJson.getAsJsonArray("includedFlags"); - for (JsonElement hoIdObj : includedIdsJson) { - includedFlags.add(hoIdObj.getAsString()); - } - } - - List excludedFlags = new ArrayList<>(); - if (holdoutJson.has("excludedFlags")) { - JsonArray excludedIdsJson = holdoutJson.getAsJsonArray("excludedFlags"); - for (JsonElement hoIdObj : excludedIdsJson) { - excludedFlags.add(hoIdObj.getAsString()); - } - } - - return new Holdout(id, key, status, audienceIds, conditions, variations, trafficAllocations, includedFlags, excludedFlags); + return new Holdout(id, key, status, audienceIds, conditions, variations, trafficAllocations); } 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 8d2c005b5..b361031e2 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,34 +218,8 @@ private List parseHoldouts(JSONArray holdoutJson) { List trafficAllocations = parseTrafficAllocation(holdoutObject.getJSONArray("trafficAllocation")); - List includedFlags; - if (holdoutObject.has("includedFlags")) { - JSONArray includedIdsJson = holdoutObject.getJSONArray("includedFlags"); - includedFlags = new ArrayList<>(includedIdsJson.length()); - - for (int j = 0; j < includedIdsJson.length(); j++) { - Object idObj = includedIdsJson.get(j); - includedFlags.add((String) idObj); - } - } else { - includedFlags = Collections.emptyList(); - } - - List excludedFlags; - if (holdoutObject.has("excludedFlags")) { - JSONArray excludedIdsJson = holdoutObject.getJSONArray("excludedFlags"); - excludedFlags = new ArrayList<>(excludedIdsJson.length()); - - for (int j = 0; j < excludedIdsJson.length(); j++) { - Object idObj = excludedIdsJson.get(j); - excludedFlags.add((String) idObj); - } - } else { - excludedFlags = Collections.emptyList(); - } - holdouts.add(new Holdout(id, key, status, audienceIds, conditions, variations, - trafficAllocations, includedFlags, excludedFlags)); + trafficAllocations)); } 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 2accb9813..8491f1e3e 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,22 +237,8 @@ private List parseHoldouts(JSONArray holdoutJson) { List trafficAllocations = parseTrafficAllocation((JSONArray) hoObject.get("trafficAllocation")); - List includedFlags; - if (hoObject.containsKey("includedFlags")) { - includedFlags = new ArrayList((JSONArray) hoObject.get("includedFlags")); - } else { - includedFlags = Collections.emptyList(); - } - - List excludedFlags; - if (hoObject.containsKey("excludedFlags")) { - excludedFlags = new ArrayList((JSONArray) hoObject.get("excludedFlags")); - } else { - excludedFlags = Collections.emptyList(); - } - holdouts.add(new Holdout(id, key, status, audienceIds, conditions, variations, - trafficAllocations, includedFlags, excludedFlags)); + trafficAllocations)); } return holdouts; diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyUserContextTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyUserContextTest.java index c7ed5af89..0a1829297 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyUserContextTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyUserContextTest.java @@ -2199,69 +2199,4 @@ public void decide_for_keys_with_holdout() throws Exception { logbackVerifier.expectMessage(Level.INFO, expectedReason); } - @Test - public void decide_all_with_holdout() throws Exception { - - Optimizely optWithHoldout = createOptimizelyWithHoldouts(); - String userId = "user123"; - Map attrs = new HashMap<>(); - // ppid120000 buckets user into holdout_included_flags - attrs.put("$opt_bucketing_id", "ppid120000"); - OptimizelyUserContext user = optWithHoldout.createUserContext(userId, attrs); - - // All flag keys present in holdouts-project-config.json - List allFlagKeys = Arrays.asList( - "boolean_feature", - "double_single_variable_feature", - "integer_single_variable_feature", - "boolean_single_variable_feature", - "string_single_variable_feature", - "multi_variate_feature", - "multi_variate_future_feature", - "mutex_group_feature" - ); - - // Flags INCLUDED in holdout_included_flags (only these should be holdout decisions) - List includedInHoldout = Arrays.asList( - "boolean_feature", - "double_single_variable_feature", - "integer_single_variable_feature" - ); - - Map decisions = user.decideAll(Arrays.asList( - OptimizelyDecideOption.INCLUDE_REASONS, - OptimizelyDecideOption.DISABLE_DECISION_EVENT - )); - assertEquals(allFlagKeys.size(), decisions.size()); - - String holdoutExperimentId = "1007543323427"; // holdout_included_flags id - String variationId = "$opt_dummy_variation_id"; - String variationKey = "ho_off_key"; - String expectedReason = "User (" + userId + ") is in variation (" + variationKey + ") of holdout (holdout_included_flags)."; - - int holdoutCount = 0; - for (String flagKey : allFlagKeys) { - OptimizelyDecision d = decisions.get(flagKey); - assertNotNull("Missing decision for flag " + flagKey, d); - if (includedInHoldout.contains(flagKey)) { - // Should be holdout decision - assertEquals(variationKey, d.getVariationKey()); - assertFalse(d.getEnabled()); - assertTrue("Expected holdout reason for flag " + flagKey, d.getReasons().contains(expectedReason)); - DecisionMetadata metadata = new DecisionMetadata.Builder() - .setFlagKey(flagKey) - .setRuleKey("holdout_included_flags") - .setRuleType("holdout") - .setVariationKey(variationKey) - .setEnabled(false) - .build(); - holdoutCount++; - } else { - // Should NOT be a holdout decision - assertFalse("Non-included flag should not have holdout reason: " + flagKey, d.getReasons().contains(expectedReason)); - } - } - assertEquals("Expected exactly the included flags to be in holdout", includedInHoldout.size(), holdoutCount); - logbackVerifier.expectMessage(Level.INFO, expectedReason); - } } 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 c2f41d400..c33bf95d7 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 @@ -81,8 +81,6 @@ import static com.optimizely.ab.config.ValidProjectConfigV4.FEATURE_FLAG_SINGLE_VARIABLE_INTEGER; import static com.optimizely.ab.config.ValidProjectConfigV4.FEATURE_MULTI_VARIATE_FEATURE_KEY; import static com.optimizely.ab.config.ValidProjectConfigV4.HOLDOUT_BASIC_HOLDOUT; -import static com.optimizely.ab.config.ValidProjectConfigV4.HOLDOUT_EXCLUDED_FLAGS_HOLDOUT; -import static com.optimizely.ab.config.ValidProjectConfigV4.HOLDOUT_INCLUDED_FLAGS_HOLDOUT; import static com.optimizely.ab.config.ValidProjectConfigV4.HOLDOUT_TYPEDAUDIENCE_HOLDOUT; import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_2; import static com.optimizely.ab.config.ValidProjectConfigV4.ROLLOUT_3_EVERYONE_ELSE_RULE; @@ -1324,59 +1322,6 @@ public void getVariationForFeatureReturnHoldoutDecisionForGlobalHoldout() { logbackVerifier.expectMessage(Level.INFO, "User (user123) is in variation (ho_off_key) of holdout (basic_holdout)."); } - @Test - public void includedFlagsHoldoutOnlyAppliestoSpecificFlags() { - ProjectConfig holdoutProjectConfig = generateValidProjectConfigV4_holdout(); - - Bucketer mockBucketer = new Bucketer(); - CmabService cmabService = mock(CmabService.class); - DecisionService decisionService = new DecisionService(mockBucketer, mockErrorHandler, null, cmabService); - - Map attributes = new HashMap<>(); - attributes.put("$opt_bucketing_id", "ppid120000"); - FeatureDecision featureDecision = decisionService.getVariationForFeature( - FEATURE_FLAG_BOOLEAN_FEATURE, - optimizely.createUserContext("user123", attributes), - holdoutProjectConfig - ).getResult(); - - assertEquals(HOLDOUT_INCLUDED_FLAGS_HOLDOUT, featureDecision.experiment); - assertEquals(VARIATION_HOLDOUT_VARIATION_OFF, featureDecision.variation); - assertEquals(FeatureDecision.DecisionSource.HOLDOUT, featureDecision.decisionSource); - - logbackVerifier.expectMessage(Level.INFO, "User (user123) is in variation (ho_off_key) of holdout (holdout_included_flags)."); - } - - @Test - public void excludedFlagsHoldoutAppliesToAllExceptSpecified() { - ProjectConfig holdoutProjectConfig = generateValidProjectConfigV4_holdout(); - - Bucketer mockBucketer = new Bucketer(); - - DecisionService decisionService = new DecisionService(mockBucketer, mockErrorHandler, null, mockCmabService); - - Map attributes = new HashMap<>(); - attributes.put("$opt_bucketing_id", "ppid300002"); - FeatureDecision excludedDecision = decisionService.getVariationForFeature( - FEATURE_FLAG_SINGLE_VARIABLE_BOOLEAN, // excluded from ho (holdout_excluded_flags) - optimizely.createUserContext("user123", attributes), - holdoutProjectConfig - ).getResult(); - - assertNotEquals(FeatureDecision.DecisionSource.HOLDOUT, excludedDecision.decisionSource); - - FeatureDecision featureDecision = decisionService.getVariationForFeature( - FEATURE_FLAG_SINGLE_VARIABLE_INTEGER, // excluded from ho (holdout_excluded_flags) - optimizely.createUserContext("user123", attributes), - holdoutProjectConfig - ).getResult(); - - assertEquals(HOLDOUT_EXCLUDED_FLAGS_HOLDOUT, featureDecision.experiment); - assertEquals(VARIATION_HOLDOUT_VARIATION_OFF, featureDecision.variation); - assertEquals(FeatureDecision.DecisionSource.HOLDOUT, featureDecision.decisionSource); - - logbackVerifier.expectMessage(Level.INFO, "User (user123) is in variation (ho_off_key) of holdout (holdout_excluded_flags)."); - } @Test public void userMeetsHoldoutAudienceConditions() { 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 6908623b0..8f13efcf0 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,8 +544,6 @@ 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.getIncludedFlags(), is(expectedHoldout.getIncludedFlags())); - assertThat(actualHoldout.getExcludedFlags(), is(expectedHoldout.getExcludedFlags())); verifyVariations(actualHoldout.getVariations(), expectedHoldout.getVariations()); verifyTrafficAllocations(actualHoldout.getTrafficAllocation(), expectedHoldout.getTrafficAllocation()); 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 5c0b2fef1..c0ddf7c71 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 @@ -31,28 +31,16 @@ public class HoldoutConfigTest { - private Holdout globalHoldout; - private Holdout includedHoldout; - private Holdout excludedHoldout; - private Holdout mixedHoldout; + private Holdout holdout1; + private Holdout holdout2; + private Holdout holdout3; @Before public void setUp() { - // Global holdout (no included/excluded flags) - globalHoldout = new Holdout("global1", "global_holdout"); - - // Holdout with included flags - includedHoldout = new Holdout("included1", "included_holdout", "Running", - Collections.emptyList(), null, Collections.emptyList(), - Collections.emptyList(), Arrays.asList("flag1", "flag2"), null); - - // Global holdout with excluded flags - excludedHoldout = new Holdout("excluded1", "excluded_holdout", "Running", - Collections.emptyList(), null, Collections.emptyList(), - Collections.emptyList(), null, Arrays.asList("flag3")); - - // Another global holdout for testing - mixedHoldout = new Holdout("mixed1", "mixed_holdout"); + // 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"); } @Test @@ -74,121 +62,56 @@ public void testConstructorWithEmptyList() { } @Test - public void testConstructorWithGlobalHoldouts() { - List holdouts = Arrays.asList(globalHoldout, mixedHoldout); + public void testConstructorWithHoldouts() { + List holdouts = Arrays.asList(holdout1, holdout2); HoldoutConfig config = new HoldoutConfig(holdouts); - + assertEquals(2, config.getAllHoldouts().size()); - assertTrue(config.getAllHoldouts().contains(globalHoldout)); + assertTrue(config.getAllHoldouts().contains(holdout1)); } @Test public void testGetHoldout() { - List holdouts = Arrays.asList(globalHoldout, includedHoldout); + List holdouts = Arrays.asList(holdout1, holdout2); HoldoutConfig config = new HoldoutConfig(holdouts); - - assertEquals(globalHoldout, config.getHoldout("global1")); - assertEquals(includedHoldout, config.getHoldout("included1")); + + assertEquals(holdout1, config.getHoldout("holdout1")); + assertEquals(holdout2, config.getHoldout("holdout2")); assertNull(config.getHoldout("nonexistent")); } @Test - public void testGetHoldoutForFlagWithGlobalHoldouts() { - List holdouts = Arrays.asList(globalHoldout, mixedHoldout); + public void testGetHoldoutForFlagReturnsAllHoldouts() { + List holdouts = Arrays.asList(holdout1, holdout2, holdout3); HoldoutConfig config = new HoldoutConfig(holdouts); - - List flagHoldouts = config.getHoldoutForFlag("any_flag"); - assertEquals(2, flagHoldouts.size()); - assertTrue(flagHoldouts.contains(globalHoldout)); - assertTrue(flagHoldouts.contains(mixedHoldout)); - } - @Test - public void testGetHoldoutForFlagWithIncludedHoldouts() { - List holdouts = Arrays.asList(globalHoldout, includedHoldout); - HoldoutConfig config = new HoldoutConfig(holdouts); - - // Flag included in holdout - List flag1Holdouts = config.getHoldoutForFlag("flag1"); - assertEquals(2, flag1Holdouts.size()); - assertTrue(flag1Holdouts.contains(globalHoldout)); // Global first - assertTrue(flag1Holdouts.contains(includedHoldout)); // Included second - - List flag2Holdouts = config.getHoldoutForFlag("flag2"); - assertEquals(2, flag2Holdouts.size()); - assertTrue(flag2Holdouts.contains(globalHoldout)); - assertTrue(flag2Holdouts.contains(includedHoldout)); - - // Flag not included in holdout - List flag3Holdouts = config.getHoldoutForFlag("flag3"); - assertEquals(1, flag3Holdouts.size()); - assertTrue(flag3Holdouts.contains(globalHoldout)); // Only global - } - - @Test - public void testGetHoldoutForFlagWithExcludedHoldouts() { - List holdouts = Arrays.asList(globalHoldout, excludedHoldout); - HoldoutConfig config = new HoldoutConfig(holdouts); - - // Flag excluded from holdout - List flag3Holdouts = config.getHoldoutForFlag("flag3"); - assertEquals(1, flag3Holdouts.size()); - assertTrue(flag3Holdouts.contains(globalHoldout)); // excludedHoldout should be filtered out - - // Flag not excluded - List flag1Holdouts = config.getHoldoutForFlag("flag1"); - assertEquals(2, flag1Holdouts.size()); - assertTrue(flag1Holdouts.contains(globalHoldout)); - assertTrue(flag1Holdouts.contains(excludedHoldout)); - } - - @Test - public void testGetHoldoutForFlagWithMixedHoldouts() { - List holdouts = Arrays.asList(globalHoldout, includedHoldout, excludedHoldout); - HoldoutConfig config = new HoldoutConfig(holdouts); - - // flag1 is included in includedHoldout + // All holdouts are global and apply to all flags List flag1Holdouts = config.getHoldoutForFlag("flag1"); assertEquals(3, flag1Holdouts.size()); - assertTrue(flag1Holdouts.contains(globalHoldout)); - assertTrue(flag1Holdouts.contains(excludedHoldout)); - assertTrue(flag1Holdouts.contains(includedHoldout)); - - // flag3 is excluded from excludedHoldout - List flag3Holdouts = config.getHoldoutForFlag("flag3"); - assertEquals(1, flag3Holdouts.size()); - assertTrue(flag3Holdouts.contains(globalHoldout)); // Only global, excludedHoldout filtered out - - // flag4 has no specific inclusion/exclusion - List flag4Holdouts = config.getHoldoutForFlag("flag4"); - assertEquals(2, flag4Holdouts.size()); - assertTrue(flag4Holdouts.contains(globalHoldout)); - assertTrue(flag4Holdouts.contains(excludedHoldout)); - } + assertTrue(flag1Holdouts.contains(holdout1)); + assertTrue(flag1Holdouts.contains(holdout2)); + assertTrue(flag1Holdouts.contains(holdout3)); - @Test - public void testCachingBehavior() { - List holdouts = Arrays.asList(globalHoldout, includedHoldout); - HoldoutConfig config = new HoldoutConfig(holdouts); - - // First call - List firstCall = config.getHoldoutForFlag("flag1"); - // Second call should return cached result (same object reference) - List secondCall = config.getHoldoutForFlag("flag1"); - - assertSame(firstCall, secondCall); - assertEquals(2, firstCall.size()); + List flag2Holdouts = config.getHoldoutForFlag("flag2"); + assertEquals(3, flag2Holdouts.size()); + assertTrue(flag2Holdouts.contains(holdout1)); + assertTrue(flag2Holdouts.contains(holdout2)); + assertTrue(flag2Holdouts.contains(holdout3)); + + // Any flag should return all holdouts + List anyFlagHoldouts = config.getHoldoutForFlag("any_flag"); + assertEquals(3, anyFlagHoldouts.size()); } @Test public void testGetAllHoldoutsIsUnmodifiable() { - List holdouts = Arrays.asList(globalHoldout, includedHoldout); + List holdouts = Arrays.asList(holdout1, holdout2); HoldoutConfig config = new HoldoutConfig(holdouts); - + List allHoldouts = config.getAllHoldouts(); - + try { - allHoldouts.add(mixedHoldout); + allHoldouts.add(holdout3); fail("Should throw UnsupportedOperationException"); } catch (UnsupportedOperationException e) { // Expected @@ -198,36 +121,9 @@ public void testGetAllHoldoutsIsUnmodifiable() { @Test public void testEmptyFlagHoldouts() { HoldoutConfig config = new HoldoutConfig(); - + List flagHoldouts = config.getHoldoutForFlag("any_flag"); assertTrue(flagHoldouts.isEmpty()); - - // Should return same empty list for subsequent calls (caching) - List secondCall = config.getHoldoutForFlag("any_flag"); - assertSame(flagHoldouts, secondCall); - } - - @Test - public void testHoldoutWithBothIncludedAndExcluded() { - // Create a holdout with both included and excluded flags (included takes precedence) - Holdout bothHoldout = new Holdout("both1", "both_holdout", "Running", - Collections.emptyList(), null, Collections.emptyList(), - Collections.emptyList(), Arrays.asList("flag1"), Arrays.asList("flag2")); - - List holdouts = Arrays.asList(globalHoldout, bothHoldout); - HoldoutConfig config = new HoldoutConfig(holdouts); - - // flag1 should include bothHoldout (included takes precedence) - List flag1Holdouts = config.getHoldoutForFlag("flag1"); - assertEquals(2, flag1Holdouts.size()); - assertTrue(flag1Holdouts.contains(globalHoldout)); - assertTrue(flag1Holdouts.contains(bothHoldout)); - - // flag2 should not include bothHoldout (not in included list) - List flag2Holdouts = config.getHoldoutForFlag("flag2"); - assertEquals(1, flag2Holdouts.size()); - assertTrue(flag2Holdouts.contains(globalHoldout)); - assertFalse(flag2Holdouts.contains(bothHoldout)); } } \ No newline at end of file diff --git a/core-api/src/test/java/com/optimizely/ab/config/HoldoutTest.java b/core-api/src/test/java/com/optimizely/ab/config/HoldoutTest.java index f61925137..aff4a288e 100644 --- a/core-api/src/test/java/com/optimizely/ab/config/HoldoutTest.java +++ b/core-api/src/test/java/com/optimizely/ab/config/HoldoutTest.java @@ -203,9 +203,7 @@ private Holdout makeMockHoldoutWithStatus(Holdout.HoldoutStatus status, Conditio Collections.emptyList(), audienceConditions, Collections.emptyList(), - Collections.emptyList(), - Collections.emptyList(), - Collections.emptyList() + Collections.emptyList() ); } } 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 0291c0ce1..df62f048c 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 @@ -550,9 +550,7 @@ public class ValidProjectConfigV4 { "$opt_dummy_variation_id", 500 ) - ), - null, - null + ) ); private static final Holdout HOLDOUT_ZERO_TRAFFIC_HOLDOUT = new Holdout( @@ -569,57 +567,10 @@ public class ValidProjectConfigV4 { "$opt_dummy_variation_id", 0 ) - ), - null, - null - ); - - public static final Holdout HOLDOUT_INCLUDED_FLAGS_HOLDOUT = new Holdout( - "1007543323427", - "holdout_included_flags", - Holdout.HoldoutStatus.RUNNING.toString(), - Collections.emptyList(), - null, - DatafileProjectConfigTestUtils.createListOfObjects( - VARIATION_HOLDOUT_VARIATION_OFF - ), - DatafileProjectConfigTestUtils.createListOfObjects( - new TrafficAllocation( - "$opt_dummy_variation_id", - 2000 - ) - ), - DatafileProjectConfigTestUtils.createListOfObjects( - "4195505407", - "3926744821", - "3281420120" - ), - null - ); - - public static final Holdout HOLDOUT_EXCLUDED_FLAGS_HOLDOUT = new Holdout( - "100753234214", - "holdout_excluded_flags", - Holdout.HoldoutStatus.RUNNING.toString(), - Collections.emptyList(), - null, - DatafileProjectConfigTestUtils.createListOfObjects( - VARIATION_HOLDOUT_VARIATION_OFF - ), - DatafileProjectConfigTestUtils.createListOfObjects( - new TrafficAllocation( - "$opt_dummy_variation_id", - 1500 - ) - ), - null, - DatafileProjectConfigTestUtils.createListOfObjects( - "2591051011", - "2079378557", - "3263342226" ) ); + public static final Holdout HOLDOUT_TYPEDAUDIENCE_HOLDOUT = new Holdout( "10075323429", "typed_audience_holdout", @@ -639,9 +590,7 @@ public class ValidProjectConfigV4 { "$opt_dummy_variation_id", 1000 ) - ), - Collections.emptyList(), - Collections.emptyList() + ) ); private static final String LAYER_TYPEDAUDIENCE_EXPERIMENT_ID = "1630555627"; private static final String EXPERIMENT_TYPEDAUDIENCE_EXPERIMENT_ID = "1323241597"; @@ -1634,10 +1583,8 @@ public static ProjectConfig generateValidProjectConfigV4_holdout() { // list holdouts List holdouts = new ArrayList(); holdouts.add(HOLDOUT_ZERO_TRAFFIC_HOLDOUT); - holdouts.add(HOLDOUT_INCLUDED_FLAGS_HOLDOUT); holdouts.add(HOLDOUT_BASIC_HOLDOUT); holdouts.add(HOLDOUT_TYPEDAUDIENCE_HOLDOUT); - holdouts.add(HOLDOUT_EXCLUDED_FLAGS_HOLDOUT); // 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 585ae8572..89bb61bf2 100644 --- a/core-api/src/test/resources/config/holdouts-project-config.json +++ b/core-api/src/test/resources/config/holdouts-project-config.json @@ -494,30 +494,6 @@ } ] }, - { - "audienceIds": [], - "id": "1007543323427", - "key": "holdout_included_flags", - "status": "Running", - "trafficAllocation": [ - { - "endOfRange": 2000, - "entityId": "$opt_dummy_variation_id" - } - ], - "variations": [ - { - "featureEnabled": false, - "id": "$opt_dummy_variation_id", - "key": "ho_off_key" - } - ], - "includedFlags": [ - "4195505407", - "3926744821", - "3281420120" - ] - }, { "audienceIds": [], "id": "10075323428", @@ -556,30 +532,6 @@ ], "audienceIds": ["3468206643", "3468206644", "3468206646", "3468206645"], "audienceConditions" : ["or", "3468206643", "3468206644", "3468206646", "3468206645"] - }, - { - "audienceIds": [], - "id": "100753234214", - "key": "holdout_excluded_flags", - "status": "Running", - "trafficAllocation": [ - { - "endOfRange": 1500, - "entityId": "$opt_dummy_variation_id" - } - ], - "variations": [ - { - "featureEnabled": false, - "id": "$opt_dummy_variation_id", - "key": "ho_off_key" - } - ], - "excludedFlags": [ - "2591051011", - "2079378557", - "3263342226" - ] } ], "groups": [