From d6c9a5e1396eaf05cec317512a106d6beeaaac0f Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Fri, 12 Jun 2026 10:59:40 -0700 Subject: [PATCH 1/3] [AI-FSSDK] [FSSDK-12760] Add localHoldouts datafile section for backward compat --- optimizely/project_config.py | 78 +++-- tests/test_decision_service_holdout.py | 28 +- tests/test_holdout_config.py | 438 ++++++++++++++++++++++--- 3 files changed, 470 insertions(+), 74 deletions(-) diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 5ec5f85b..b234a202 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -89,18 +89,58 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): region_value = config.get('region') self.region: str = region_value or 'US' - # Parse holdouts from datafile and convert to Holdout entities - holdouts_data: list[types.HoldoutDict] = config.get('holdouts', []) + # Parse holdouts from datafile and convert to Holdout entities. + # + # Two top-level datafile sections drive holdout scoping (Gen 3+): + # - 'holdouts' → ALL entries are global holdouts (applied to every flag). + # Any 'includedRules' field on these entries is IGNORED; + # section membership alone determines scope. + # - 'localHoldouts' → ALL entries are local holdouts (rule-scoped via + # 'includedRules'). Entries missing 'includedRules' are + # invalid and skipped with an error log. + # + # Backward compatibility: older datafiles that only emit the 'holdouts' section + # continue to work — every entry is treated as global, matching pre-localHoldouts + # behavior. The 'localHoldouts' key is simply absent and parsed as an empty list. + global_holdouts_data: list[types.HoldoutDict] = config.get('holdouts', []) + local_holdouts_data: list[types.HoldoutDict] = config.get('localHoldouts', []) self.holdouts: list[entities.Holdout] = [] self.holdout_id_map: dict[str, entities.Holdout] = {} - # Global holdouts (includedRules is None) — evaluated at flag level before any rule + # Global holdouts — evaluated at flag level before any rule self.global_holdouts: list[entities.Holdout] = [] # Rule-level holdouts — map from rule ID to holdouts targeting that rule self.rule_holdouts_map: dict[str, list[entities.Holdout]] = {} - # Convert holdout dicts to Holdout entities - for holdout_data in holdouts_data: - # Create Holdout entity + # Process global holdouts: section membership is the sole signal for scope. + # Drop any 'includedRules' field on entries here so the entity is unambiguously + # global (is_global → True), even if the datafile incorrectly includes one. + for holdout_data in global_holdouts_data: + # Copy the typed dict and remove includedRules without losing its shape. + sanitized = cast(types.HoldoutDict, dict(holdout_data)) + sanitized.pop('includedRules', None) # type: ignore[misc] + holdout = entities.Holdout(**sanitized) + self.holdouts.append(holdout) + + # Only process Running holdouts + if not holdout.is_activated: + continue + + # Map by ID for quick lookup + self.holdout_id_map[holdout.id] = holdout + self.global_holdouts.append(holdout) + + # Process local holdouts: every entry must carry 'includedRules' (list of rule IDs). + # Entries without 'includedRules' are invalid per spec — log an error and exclude + # them from evaluation (do NOT fall back to global application). + for holdout_data in local_holdouts_data: + if 'includedRules' not in holdout_data or holdout_data.get('includedRules') is None: + holdout_id = holdout_data.get('id', '') + self.logger.error( + f'Local holdout with ID "{holdout_id}" is missing required "includedRules" ' + f'field and will be excluded from evaluation.' + ) + continue + holdout = entities.Holdout(**holdout_data) self.holdouts.append(holdout) @@ -111,15 +151,11 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): # Map by ID for quick lookup self.holdout_id_map[holdout.id] = holdout - # Classify holdout as global or local based on includedRules - if holdout.is_global: - self.global_holdouts.append(holdout) - else: - # Local holdout — register for each targeted rule ID - for rule_id in (holdout.included_rules or []): - if rule_id not in self.rule_holdouts_map: - self.rule_holdouts_map[rule_id] = [] - self.rule_holdouts_map[rule_id].append(holdout) + # Register for each targeted rule ID + for rule_id in (holdout.included_rules or []): + if rule_id not in self.rule_holdouts_map: + self.rule_holdouts_map[rule_id] = [] + self.rule_holdouts_map[rule_id].append(holdout) # Utility maps for quick lookup self.group_id_map: dict[str, entities.Group] = self._generate_key_map(self.groups, 'id', entities.Group) @@ -887,9 +923,11 @@ def get_flag_variation( return None def get_global_holdouts(self) -> list[entities.Holdout]: - """Return all global holdouts (includedRules is None). + """Return all global holdouts (parsed from the top-level 'holdouts' section). Global holdouts are evaluated at flag level before any rule is checked. + Section membership in 'holdouts' is the sole signal for global scope — + any 'includedRules' field on these entries is ignored. Returns: List of global Holdout entities that are currently running. @@ -899,9 +937,11 @@ def get_global_holdouts(self) -> list[entities.Holdout]: def get_holdouts_for_rule(self, rule_id: str) -> list[entities.Holdout]: """Return local holdouts that target a specific rule. - Local holdouts are evaluated per-rule, before the rule's audience and - traffic allocation checks. A rule ID not present in any holdout's - includedRules simply returns an empty list — silently skipped. + Local holdouts come from the top-level 'localHoldouts' datafile section and + are scoped per-rule via their 'includedRules' field. They are evaluated + before the rule's audience and traffic allocation checks. A rule ID not + present in any holdout's includedRules simply returns an empty list — + silently skipped. Args: rule_id: The experiment or delivery rule ID to look up. diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index 1bf81c93..21a269b2 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -1380,9 +1380,33 @@ def tearDown(self): if hasattr(self, 'opt_obj'): self.opt_obj.close() - def _make_opt(self, holdouts): + def _make_opt(self, holdouts, local_holdouts=None): + """Build an Optimizely instance from holdout entries. + + Per FSSDK-12760, the datafile partitions holdouts into two top-level sections: + - 'holdouts' → global holdouts (every entry is global, regardless of includedRules) + - 'localHoldouts' → local holdouts (rule-scoped via includedRules) + + For convenience, this helper auto-routes entries: any entry in `holdouts` + whose `includedRules` is a non-None list is moved to the `localHoldouts` + section. Tests that explicitly want to control sectioning can pass both + `holdouts` and `local_holdouts` lists. + """ cfg = self.config_dict_with_features.copy() - cfg['holdouts'] = holdouts + if local_holdouts is None: + globals_list: list = [] + locals_list: list = [] + for h in holdouts: + if h.get('includedRules') is not None: + locals_list.append(h) + else: + globals_list.append(h) + cfg['holdouts'] = globals_list + if locals_list: + cfg['localHoldouts'] = locals_list + else: + cfg['holdouts'] = holdouts + cfg['localHoldouts'] = local_holdouts self.opt_obj = optimizely_module.Optimizely(json.dumps(cfg)) return self.opt_obj diff --git a/tests/test_holdout_config.py b/tests/test_holdout_config.py index 3d8cd08c..6420e42c 100644 --- a/tests/test_holdout_config.py +++ b/tests/test_holdout_config.py @@ -12,13 +12,25 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""Level 1 tests for holdout config parsing and data model (FSSDK-12369). +"""Level 1 tests for holdout config parsing and data model. + +Originally added under FSSDK-12369 to cover holdout config classification. +Updated under FSSDK-12760 to align with the backward-compatible local holdouts +design: local holdouts now live in a new top-level 'localHoldouts' datafile +section. The 'holdouts' section carries ONLY global holdouts — section membership +(not 'includedRules') is the sole signal for scope. Tests cover: -- isGlobal classification (includedRules == None vs list) -- get_global_holdouts() returns only global holdouts -- get_holdouts_for_rule() returns local holdouts for a specific rule -- Backward compatibility with old datafiles (no includedRules field) +- isGlobal classification at the entity level (includedRules == None vs list) +- get_global_holdouts() returns entries from the 'holdouts' section +- get_holdouts_for_rule() returns entries from the 'localHoldouts' section +- Backward compatibility: + * Old datafiles without 'localHoldouts' continue to work unchanged + * Any 'includedRules' field on entries in the 'holdouts' section is ignored + * Datafiles missing both 'holdouts' and 'localHoldouts' sections produce + empty global/local lists +- localHoldouts section: invalid entries (missing includedRules) are logged + and excluded from evaluation - Edge cases: empty array vs None, unknown rule IDs, cross-flag targeting """ @@ -59,10 +71,24 @@ def _make_holdout( return h -def _build_config(holdouts: list, base_test: 'base.BaseTest') -> 'optimizely_module.Optimizely': - """Create an Optimizely instance with the given holdouts list.""" +def _build_config( + holdouts: list, + base_test: 'base.BaseTest', + local_holdouts: 'list | None' = None, +) -> 'optimizely_module.Optimizely': + """Create an Optimizely instance with the given top-level holdout sections. + + Args: + holdouts: Entries for the top-level 'holdouts' section (treated as global). + base_test: Test fixture providing config_dict_with_features. + local_holdouts: Optional entries for the top-level 'localHoldouts' section + (treated as local). When omitted, the 'localHoldouts' key is NOT added + to the datafile, exercising backward-compatible behavior. + """ config_dict = base_test.config_dict_with_features.copy() config_dict['holdouts'] = holdouts + if local_holdouts is not None: + config_dict['localHoldouts'] = local_holdouts return optimizely_module.Optimizely(json.dumps(config_dict)) @@ -131,13 +157,23 @@ def tearDown(self): if hasattr(self, 'opt_obj'): self.opt_obj.close() - def test_get_global_holdouts_returns_all_global_holdouts(self): - """All holdouts without includedRules should be returned.""" - self.opt_obj = _build_config([ - _make_holdout('h1', 'global_1'), # field absent → global - _make_holdout('h2', 'global_2', None), # explicitly None → global - _make_holdout('h3', 'local_1', ['rule_x']), # local - ], self) + def test_get_global_holdouts_returns_all_entries_from_holdouts_section(self): + """Every entry in the 'holdouts' datafile section is treated as global. + + Section membership is the sole signal for scope — any 'includedRules' + field on these entries is ignored. Local holdouts must live in the + separate 'localHoldouts' section. + """ + self.opt_obj = _build_config( + holdouts=[ + _make_holdout('h1', 'global_1'), + _make_holdout('h2', 'global_2', None), + ], + base_test=self, + local_holdouts=[ + _make_holdout('h3', 'local_1', ['rule_x']), + ], + ) config = self.opt_obj.config_manager.get_config() global_holdouts = config.get_global_holdouts() global_ids = {h.id for h in global_holdouts} @@ -164,14 +200,47 @@ def test_get_global_holdouts_excludes_non_running_holdouts(self): self.assertNotIn('h_draft', ids) def test_get_global_holdouts_returns_empty_when_all_local(self): - """If all holdouts are local, global list is empty.""" - self.opt_obj = _build_config([ - _make_holdout('h1', 'local_a', ['rule_1']), - _make_holdout('h2', 'local_b', ['rule_2']), - ], self) + """If all holdouts live in the localHoldouts section, global list is empty.""" + self.opt_obj = _build_config( + holdouts=[], + base_test=self, + local_holdouts=[ + _make_holdout('h1', 'local_a', ['rule_1']), + _make_holdout('h2', 'local_b', ['rule_2']), + ], + ) config = self.opt_obj.config_manager.get_config() self.assertEqual(config.get_global_holdouts(), []) + def test_global_holdouts_ignore_included_rules_on_entries(self): + """Any 'includedRules' field on entries in the 'holdouts' section is ignored. + + Section membership in 'holdouts' alone determines global scope. Even if the + datafile incorrectly includes an 'includedRules' field on a global holdout, + the entity must still be classified as global and never registered against + any rule's local-holdout map. + """ + self.opt_obj = _build_config( + holdouts=[ + _make_holdout('h_global_with_rules', 'g', ['rule_should_be_ignored']), + ], + base_test=self, + ) + config = self.opt_obj.config_manager.get_config() + + global_holdouts = config.get_global_holdouts() + global_ids = {h.id for h in global_holdouts} + self.assertIn('h_global_with_rules', global_ids) + + # The ignored rule ID must not be present in the rule map + self.assertEqual(config.get_holdouts_for_rule('rule_should_be_ignored'), []) + + # The entity itself must report is_global (includedRules was stripped) + for h in global_holdouts: + if h.id == 'h_global_with_rules': + self.assertTrue(h.is_global) + self.assertIsNone(h.included_rules) + class HoldoutConfigGetHoldoutsForRuleTest(unittest.TestCase): """Tests for ProjectConfig.get_holdouts_for_rule().""" @@ -185,9 +254,13 @@ def tearDown(self): def test_get_holdouts_for_rule_returns_matching_local_holdouts(self): """Holdout targeting rule_x should be returned for rule_x.""" - self.opt_obj = _build_config([ - _make_holdout('h1', 'local_h', ['rule_x', 'rule_y']), - ], self) + self.opt_obj = _build_config( + holdouts=[], + base_test=self, + local_holdouts=[ + _make_holdout('h1', 'local_h', ['rule_x', 'rule_y']), + ], + ) config = self.opt_obj.config_manager.get_config() holdouts = config.get_holdouts_for_rule('rule_x') self.assertEqual(len(holdouts), 1) @@ -195,9 +268,13 @@ def test_get_holdouts_for_rule_returns_matching_local_holdouts(self): def test_get_holdouts_for_rule_returns_empty_for_unknown_rule(self): """Rule ID not found in any holdout's includedRules returns empty list.""" - self.opt_obj = _build_config([ - _make_holdout('h1', 'local_h', ['rule_a']), - ], self) + self.opt_obj = _build_config( + holdouts=[], + base_test=self, + local_holdouts=[ + _make_holdout('h1', 'local_h', ['rule_a']), + ], + ) config = self.opt_obj.config_manager.get_config() # Silently skip unknown rule IDs holdouts = config.get_holdouts_for_rule('nonexistent_rule') @@ -211,10 +288,14 @@ def test_get_holdouts_for_rule_returns_empty_when_no_holdouts(self): def test_get_holdouts_for_rule_multiple_holdouts_for_same_rule(self): """Multiple holdouts can target the same rule.""" - self.opt_obj = _build_config([ - _make_holdout('h1', 'local_h1', ['rule_x']), - _make_holdout('h2', 'local_h2', ['rule_x', 'rule_y']), - ], self) + self.opt_obj = _build_config( + holdouts=[], + base_test=self, + local_holdouts=[ + _make_holdout('h1', 'local_h1', ['rule_x']), + _make_holdout('h2', 'local_h2', ['rule_x', 'rule_y']), + ], + ) config = self.opt_obj.config_manager.get_config() holdouts = config.get_holdouts_for_rule('rule_x') ids = {h.id for h in holdouts} @@ -223,10 +304,15 @@ def test_get_holdouts_for_rule_multiple_holdouts_for_same_rule(self): def test_get_holdouts_for_rule_does_not_return_global_holdouts(self): """Global holdouts should not appear in get_holdouts_for_rule results.""" - self.opt_obj = _build_config([ - _make_holdout('global', 'global_holdout'), # field absent → global - _make_holdout('local', 'local_holdout', ['rule_x']), - ], self) + self.opt_obj = _build_config( + holdouts=[ + _make_holdout('global', 'global_holdout'), + ], + base_test=self, + local_holdouts=[ + _make_holdout('local', 'local_holdout', ['rule_x']), + ], + ) config = self.opt_obj.config_manager.get_config() holdouts = config.get_holdouts_for_rule('rule_x') ids = {h.id for h in holdouts} @@ -235,17 +321,25 @@ def test_get_holdouts_for_rule_does_not_return_global_holdouts(self): def test_get_holdouts_for_rule_rule_specificity(self): """A holdout targeting rule_x must not appear for rule_y.""" - self.opt_obj = _build_config([ - _make_holdout('h1', 'local_for_x', ['rule_x']), - ], self) + self.opt_obj = _build_config( + holdouts=[], + base_test=self, + local_holdouts=[ + _make_holdout('h1', 'local_for_x', ['rule_x']), + ], + ) config = self.opt_obj.config_manager.get_config() self.assertEqual(config.get_holdouts_for_rule('rule_y'), []) def test_get_holdouts_for_rule_cross_flag_targeting(self): """One holdout can target rules from multiple different flags.""" - self.opt_obj = _build_config([ - _make_holdout('h1', 'cross_flag_holdout', ['rule_flag_a', 'rule_flag_b']), - ], self) + self.opt_obj = _build_config( + holdouts=[], + base_test=self, + local_holdouts=[ + _make_holdout('h1', 'cross_flag_holdout', ['rule_flag_a', 'rule_flag_b']), + ], + ) config = self.opt_obj.config_manager.get_config() for rule_id in ['rule_flag_a', 'rule_flag_b']: holdouts = config.get_holdouts_for_rule(rule_id) @@ -263,9 +357,13 @@ def tearDown(self): if hasattr(self, 'opt_obj'): self.opt_obj.close() - def test_old_datafile_without_included_rules_treated_as_global(self): - """Datafiles without includedRules field must default to global holdout.""" - # Simulate old datafile — no 'includedRules' key at all + def test_old_datafile_without_local_holdouts_section_works(self): + """Old datafiles that only emit the 'holdouts' section continue to work. + + Per FSSDK-12760: backward compatibility requires that datafiles produced + before the 'localHoldouts' section existed are parsed exactly like before. + Every entry in 'holdouts' is global; no errors, no log noise. + """ old_holdout = { 'id': 'old_h', 'key': 'legacy_holdout', @@ -283,7 +381,7 @@ def test_old_datafile_without_included_rules_treated_as_global(self): self.assertTrue(global_holdouts[0].is_global) def test_old_datafile_holdout_does_not_appear_in_rule_map(self): - """Legacy holdouts with no includedRules field must not pollute rule map.""" + """Legacy holdouts (no localHoldouts section) must not pollute rule map.""" old_holdout = { 'id': 'old_h', 'key': 'legacy_holdout', @@ -296,17 +394,251 @@ def test_old_datafile_holdout_does_not_appear_in_rule_map(self): config = self.opt_obj.config_manager.get_config() self.assertEqual(config.get_holdouts_for_rule('any_rule'), []) - def test_empty_included_rules_is_local_not_global(self): - """[] (empty array) is a local holdout targeting no rules — not the same as None.""" - holdout_empty = _make_holdout('h_empty', 'empty_local', included_rules=[]) - holdout_none = _make_holdout('h_none', 'global_none', included_rules=None) + def test_datafile_missing_both_holdouts_sections(self): + """Datafiles without 'holdouts' or 'localHoldouts' keys produce empty lists.""" + config_dict = self.config_dict_with_features.copy() + # Explicitly do not set either holdouts key + self.opt_obj = optimizely_module.Optimizely(json.dumps(config_dict)) + config = self.opt_obj.config_manager.get_config() + + self.assertEqual(config.get_global_holdouts(), []) + self.assertEqual(config.get_holdouts_for_rule('any_rule'), []) + + def test_is_global_entity_property_independent_of_section(self): + """The Holdout.is_global entity property still reflects includedRules. + + At the entity level, is_global is computed from the included_rules attribute + (None → True, [] or non-empty list → False). The new datafile sections affect + how the entity is BUILT (via includedRules stripping) but not how the property + itself is computed. + """ + # Build an entity directly (bypassing project config) — includedRules controls is_global + h_none = entities.Holdout( + id='h_none', key='g', status='Running', + variations=HOLDOUT_VARIATION, trafficAllocation=FULL_TRAFFIC, + audienceIds=[], includedRules=None, + ) + h_empty = entities.Holdout( + id='h_empty', key='l', status='Running', + variations=HOLDOUT_VARIATION, trafficAllocation=FULL_TRAFFIC, + audienceIds=[], includedRules=[], + ) + self.assertTrue(h_none.is_global) + self.assertFalse(h_empty.is_global) + + +class LocalHoldoutsSectionTest(unittest.TestCase): + """Tests for the new top-level 'localHoldouts' datafile section (FSSDK-12760). + + Local holdouts now live in a dedicated 'localHoldouts' section, separate from + 'holdouts' which carries only global holdouts. Older SDK versions (Gen 1/Gen 2) + will ignore this unknown top-level key entirely — that's the whole point of the + backward-compatible design — but Gen 3 SDKs must parse it as the source of truth + for local (rule-scoped) holdouts. + """ + + def setUp(self): + base.BaseTest.setUp(self) + + def tearDown(self): + if hasattr(self, 'opt_obj'): + self.opt_obj.close() - self.opt_obj = _build_config([holdout_empty, holdout_none], self) + def test_local_holdouts_section_registers_in_rule_map(self): + """Entries in 'localHoldouts' are registered under each rule in includedRules.""" + self.opt_obj = _build_config( + holdouts=[], + base_test=self, + local_holdouts=[ + _make_holdout('h_local', 'local_h', ['rule_x']), + ], + ) config = self.opt_obj.config_manager.get_config() + holdouts = config.get_holdouts_for_rule('rule_x') + self.assertEqual(len(holdouts), 1) + self.assertEqual(holdouts[0].id, 'h_local') + + def test_local_holdouts_section_entries_excluded_from_global_list(self): + """Entries in 'localHoldouts' must not appear in get_global_holdouts().""" + self.opt_obj = _build_config( + holdouts=[], + base_test=self, + local_holdouts=[ + _make_holdout('h_local', 'local_h', ['rule_x']), + ], + ) + config = self.opt_obj.config_manager.get_config() + self.assertEqual(config.get_global_holdouts(), []) - global_holdouts = config.get_global_holdouts() - global_ids = {h.id for h in global_holdouts} + def test_local_holdouts_missing_included_rules_logged_and_excluded(self): + """Entries in 'localHoldouts' without 'includedRules' are invalid per spec. + + SDK must log an error and exclude the entry from evaluation. It must NOT + fall back to global application (the partition between sections is hard). + """ + invalid_local = { + 'id': 'h_invalid', + 'key': 'invalid_local', + 'status': 'Running', + 'audienceIds': [], + 'variations': HOLDOUT_VARIATION, + 'trafficAllocation': FULL_TRAFFIC, + # Note: no 'includedRules' field + } + self.opt_obj = _build_config( + holdouts=[], + base_test=self, + local_holdouts=[invalid_local], + ) + config = self.opt_obj.config_manager.get_config() + + # Invalid entry must not be applied as global + self.assertEqual(config.get_global_holdouts(), []) + # Invalid entry must not be applied as local for any rule + self.assertEqual(config.get_holdouts_for_rule('any_rule'), []) + # Invalid entry must not be retrievable by ID either + self.assertIsNone(config.holdout_id_map.get('h_invalid')) + + def test_local_holdouts_missing_included_rules_logs_error(self): + """Verify an error log is emitted for an invalid local holdout entry.""" + invalid_local = { + 'id': 'h_invalid', + 'key': 'invalid_local', + 'status': 'Running', + 'audienceIds': [], + 'variations': HOLDOUT_VARIATION, + 'trafficAllocation': FULL_TRAFFIC, + } + config_dict = self.config_dict_with_features.copy() + config_dict['localHoldouts'] = [invalid_local] + + # Verify the error is logged through the standard logging framework. + with self.assertLogs('optimizely.logger', level='ERROR') as captured: + self.opt_obj = optimizely_module.Optimizely(json.dumps(config_dict)) + + matching = [ + msg for msg in captured.output + if 'h_invalid' in msg and 'includedRules' in msg + ] + self.assertGreaterEqual( + len(matching), 1, + f'Expected error log for invalid local holdout; got: {captured.output}' + ) + + def test_local_holdouts_with_null_included_rules_logged_and_excluded(self): + """includedRules=None in 'localHoldouts' is invalid (same as missing). + + Distinct from 'holdouts' section where None signals a global holdout — + in 'localHoldouts' the field is REQUIRED and a None value is invalid. + """ + invalid_local = _make_holdout('h_null', 'null_local', included_rules=None) + self.opt_obj = _build_config( + holdouts=[], + base_test=self, + local_holdouts=[invalid_local], + ) + config = self.opt_obj.config_manager.get_config() + self.assertEqual(config.get_holdouts_for_rule('any_rule'), []) + self.assertEqual(config.get_global_holdouts(), []) + + def test_local_holdouts_empty_included_rules_targets_no_rules(self): + """includedRules=[] is valid but targets no rules (entity is stored, not invalid). + + An entry with an empty includedRules list is treated as a local holdout + that simply does not apply to any rule. It is NOT invalid (no error log), + and it is NOT promoted to global. This matches the existing entity-level + semantics where [] != None. + """ + empty_local = _make_holdout('h_empty', 'empty_local', included_rules=[]) + self.opt_obj = _build_config( + holdouts=[], + base_test=self, + local_holdouts=[empty_local], + ) + config = self.opt_obj.config_manager.get_config() + # Not in any rule map (no rules to target) + self.assertEqual(config.get_holdouts_for_rule('any_rule'), []) + # Not global either + self.assertEqual(config.get_global_holdouts(), []) + # But the entity itself was created and tracked in holdout_id_map + stored = config.holdout_id_map.get('h_empty') + self.assertIsNotNone(stored) + self.assertFalse(stored.is_global) + + def test_local_holdouts_excludes_non_running_entries(self): + """Non-Running entries in 'localHoldouts' must not be evaluated.""" + self.opt_obj = _build_config( + holdouts=[], + base_test=self, + local_holdouts=[ + _make_holdout('h_running', 'lr', ['rule_x'], status='Running'), + _make_holdout('h_draft', 'ld', ['rule_x'], status='Draft'), + ], + ) + config = self.opt_obj.config_manager.get_config() + ids = {h.id for h in config.get_holdouts_for_rule('rule_x')} + self.assertIn('h_running', ids) + self.assertNotIn('h_draft', ids) - # None → global; [] → local (does not appear in global list) - self.assertIn('h_none', global_ids) - self.assertNotIn('h_empty', global_ids) + def test_both_sections_present_partition_correctly(self): + """When both 'holdouts' and 'localHoldouts' are present, scope is enforced + by section membership — entries never cross over.""" + self.opt_obj = _build_config( + holdouts=[ + _make_holdout('g1', 'global_1'), + _make_holdout('g2', 'global_2'), + ], + base_test=self, + local_holdouts=[ + _make_holdout('l1', 'local_1', ['rule_a']), + _make_holdout('l2', 'local_2', ['rule_b']), + ], + ) + config = self.opt_obj.config_manager.get_config() + + global_ids = {h.id for h in config.get_global_holdouts()} + self.assertEqual(global_ids, {'g1', 'g2'}) + + self.assertEqual({h.id for h in config.get_holdouts_for_rule('rule_a')}, {'l1'}) + self.assertEqual({h.id for h in config.get_holdouts_for_rule('rule_b')}, {'l2'}) + + def test_local_holdouts_id_uniqueness_across_sections(self): + """Entries from both sections are tracked together in holdout_id_map. + + get_holdout(id) must work for both global and local holdouts regardless of + which section they came from. + """ + self.opt_obj = _build_config( + holdouts=[ + _make_holdout('g1', 'global_1'), + ], + base_test=self, + local_holdouts=[ + _make_holdout('l1', 'local_1', ['rule_x']), + ], + ) + config = self.opt_obj.config_manager.get_config() + + self.assertIsNotNone(config.get_holdout('g1')) + self.assertIsNotNone(config.get_holdout('l1')) + + def test_local_holdouts_section_with_real_traffic_allocation(self): + """Local holdouts in 'localHoldouts' carry real trafficAllocation directly. + + Per spec: no more dummy-zero-allocation workaround — the trafficAllocation + field is used as-is for bucketing. + """ + local_h = _make_holdout('h_local', 'local_h', ['rule_x']) + # Override with a custom traffic allocation + local_h['trafficAllocation'] = [{'entityId': 'holdout_var_1', 'endOfRange': 5000}] + + self.opt_obj = _build_config( + holdouts=[], + base_test=self, + local_holdouts=[local_h], + ) + config = self.opt_obj.config_manager.get_config() + holdouts = config.get_holdouts_for_rule('rule_x') + self.assertEqual(len(holdouts), 1) + self.assertEqual(holdouts[0].trafficAllocation, + [{'entityId': 'holdout_var_1', 'endOfRange': 5000}]) From 3754fcdd84cee8b5d758b60396bdbe465bf16c36 Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Fri, 12 Jun 2026 11:34:45 -0700 Subject: [PATCH 2/3] =?UTF-8?q?[FSSDK-12760]=20Clarify=20holdout=20scope?= =?UTF-8?q?=20docs=20(spec=20=C2=A75=20cleanup)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update stale docstrings/comments that described holdout scope as being determined by the includedRules field. After FSSDK-12760, scope is determined by which top-level datafile section the entity is parsed from ('holdouts' = global, 'localHoldouts' = local). The entity-level is_global property continues to compute from included_rules for code holding direct entity references. No behavior change. --- optimizely/entities.py | 19 ++++++++++++++++--- optimizely/helpers/types.py | 7 ++++++- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/optimizely/entities.py b/optimizely/entities.py index e15595b4..f971bd91 100644 --- a/optimizely/entities.py +++ b/optimizely/entities.py @@ -233,7 +233,11 @@ def __init__( self.trafficAllocation = trafficAllocation self.audienceIds = audienceIds self.audienceConditions = audienceConditions - # None = global holdout (applies to all rules); list of rule IDs = local holdout + # Per-rule targeting list for local holdouts. Scope is NOT determined by this + # field — it is determined by which datafile section the entity was parsed + # from ('holdouts' = global, 'localHoldouts' = local). ProjectConfig strips + # this field on entries parsed from the 'holdouts' section so that entities + # built from that section always satisfy is_global. See FSSDK-12760. self.included_rules: Optional[list[str]] = includedRules def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]: @@ -248,8 +252,17 @@ def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]: def is_global(self) -> bool: """Check if this is a global holdout (applies to all rules across all flags). - A holdout is global when includedRules is None (absent from datafile). - An empty list [] is a local holdout that targets no rules (different from global). + Authoritative scope is the datafile section the entity came from: + the top-level 'holdouts' section is global, 'localHoldouts' is local. + ProjectConfig enforces this by stripping 'includedRules' from entries + parsed out of the 'holdouts' section before constructing the entity, + so for entities built via ProjectConfig this property is consistent + with section membership. + + At the entity level the property is still computed from + ``included_rules`` for code holding entity references directly: + None → global, [] or non-empty list → local. An empty list is a + local holdout that targets no rules (distinct from None/global). Returns: True if included_rules is None (global), False otherwise (local). diff --git a/optimizely/helpers/types.py b/optimizely/helpers/types.py index 7ee51373..8e084eb1 100644 --- a/optimizely/helpers/types.py +++ b/optimizely/helpers/types.py @@ -130,4 +130,9 @@ class HoldoutDict(ExperimentDict): Extends ExperimentDict with holdout-specific properties. """ holdoutStatus: HoldoutStatus - includedRules: Optional[list[str]] # None = global holdout; list of rule IDs = local holdout + # Per-rule targeting list for local holdouts. Holdout scope is determined by which + # top-level datafile section the entry appears in ('holdouts' vs 'localHoldouts'), + # NOT by this field. On 'holdouts'-section entries this field is ignored and + # stripped at parse time (see ProjectConfig). On 'localHoldouts'-section entries + # the field is required; missing or None is an error and the entry is excluded. + includedRules: Optional[list[str]] From 2c0971d1d07e7e0350945f591a32214a450e31fa Mon Sep 17 00:00:00 2001 From: Jae Kim Date: Fri, 12 Jun 2026 11:39:17 -0700 Subject: [PATCH 3/3] [FSSDK-12760] Tighten holdout scope docstrings/comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Condense the spec §5 cleanup comments to one-line/short-docstring form per updated spec guidance. Same content, more compact. --- optimizely/entities.py | 26 ++++++-------------------- optimizely/helpers/types.py | 7 ++----- 2 files changed, 8 insertions(+), 25 deletions(-) diff --git a/optimizely/entities.py b/optimizely/entities.py index f971bd91..7b0b09e5 100644 --- a/optimizely/entities.py +++ b/optimizely/entities.py @@ -233,11 +233,8 @@ def __init__( self.trafficAllocation = trafficAllocation self.audienceIds = audienceIds self.audienceConditions = audienceConditions - # Per-rule targeting list for local holdouts. Scope is NOT determined by this - # field — it is determined by which datafile section the entity was parsed - # from ('holdouts' = global, 'localHoldouts' = local). ProjectConfig strips - # this field on entries parsed from the 'holdouts' section so that entities - # built from that section always satisfy is_global. See FSSDK-12760. + # Per-rule targeting for local holdouts. Scope comes from the datafile + # section, not this field; ProjectConfig strips it on 'holdouts' entries. self.included_rules: Optional[list[str]] = includedRules def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]: @@ -250,22 +247,11 @@ def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]: @property def is_global(self) -> bool: - """Check if this is a global holdout (applies to all rules across all flags). + """True if this is a global holdout. - Authoritative scope is the datafile section the entity came from: - the top-level 'holdouts' section is global, 'localHoldouts' is local. - ProjectConfig enforces this by stripping 'includedRules' from entries - parsed out of the 'holdouts' section before constructing the entity, - so for entities built via ProjectConfig this property is consistent - with section membership. - - At the entity level the property is still computed from - ``included_rules`` for code holding entity references directly: - None → global, [] or non-empty list → local. An empty list is a - local holdout that targets no rules (distinct from None/global). - - Returns: - True if included_rules is None (global), False otherwise (local). + Scope is set by the datafile section ('holdouts' vs 'localHoldouts'). + ProjectConfig strips 'includedRules' on 'holdouts' entries, so this + property stays consistent with section membership. """ return self.included_rules is None diff --git a/optimizely/helpers/types.py b/optimizely/helpers/types.py index 8e084eb1..7b65ab13 100644 --- a/optimizely/helpers/types.py +++ b/optimizely/helpers/types.py @@ -130,9 +130,6 @@ class HoldoutDict(ExperimentDict): Extends ExperimentDict with holdout-specific properties. """ holdoutStatus: HoldoutStatus - # Per-rule targeting list for local holdouts. Holdout scope is determined by which - # top-level datafile section the entry appears in ('holdouts' vs 'localHoldouts'), - # NOT by this field. On 'holdouts'-section entries this field is ignored and - # stripped at parse time (see ProjectConfig). On 'localHoldouts'-section entries - # the field is required; missing or None is an error and the entry is excluded. + # Per-rule targeting for local holdouts. Scope comes from the datafile section, + # not this field; required on 'localHoldouts' entries, stripped on 'holdouts'. includedRules: Optional[list[str]]