From b397b3bd03d8b4e1963afcd42edd4020446a1d78 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 7 May 2026 15:21:06 -0700 Subject: [PATCH] [AI-FSSDK] [FSSDK-12369] Add local holdouts support (includedRules, rule-level evaluation) --- optimizely/decision_service.py | 43 +++- optimizely/entities.py | 15 ++ optimizely/helpers/types.py | 1 + optimizely/project_config.py | 49 +++- tests/test_decision_service_holdout.py | 343 +++++++++++++++++++++++++ 5 files changed, 443 insertions(+), 8 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index be2be2c5..ab8fc540 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -599,8 +599,25 @@ def get_variation_for_rollout( while index < len(rollout_rules): skip_to_everyone_else = False - # check forced decision first rule = rollout_rules[index] + + # Check local holdouts targeting this delivery rule (NEW - local holdouts) + local_holdouts = project_config.get_holdouts_for_rule(rule.id) + for local_holdout in local_holdouts: + local_holdout_decision = self.get_variation_for_holdout( + local_holdout, user_context, project_config + ) + decide_reasons.extend(local_holdout_decision['reasons']) + if local_holdout_decision['decision'].variation is not None: + message = ( + f"The user '{user_id}' is bucketed into local holdout " + f"'{local_holdout.key}' for rollout rule '{rule.key}'." + ) + self.logger.info(message) + decide_reasons.append(message) + return local_holdout_decision['decision'], decide_reasons + + # check forced decision first optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(feature.key, rule.key) forced_decision_variation, reasons_received = self.validated_forced_decision( project_config, optimizely_decision_context, user_context) @@ -733,8 +750,8 @@ def get_decision_for_flag( reasons = decide_reasons.copy() if decide_reasons else [] user_id = user_context.user_id - # Check holdouts - holdouts = project_config.get_holdouts_for_flag(feature_flag.key) + # Check global holdouts (evaluated at flag level, before forced decisions) + holdouts = project_config.get_global_holdouts() for holdout in holdouts: holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config) reasons.extend(holdout_decision['reasons']) @@ -762,6 +779,26 @@ def get_decision_for_flag( experiment = project_config.get_experiment_from_id(experiment_id) if experiment: + # Check local holdouts targeting this experiment rule (NEW - local holdouts) + local_holdouts = project_config.get_holdouts_for_rule(experiment.id) + for local_holdout in local_holdouts: + local_holdout_decision = self.get_variation_for_holdout( + local_holdout, user_context, project_config + ) + reasons.extend(local_holdout_decision['reasons']) + if local_holdout_decision['decision'].variation is not None: + message = ( + f"The user '{user_id}' is bucketed into local holdout " + f"'{local_holdout.key}' for experiment rule '{experiment.key}'." + ) + self.logger.info(message) + reasons.append(message) + return { + 'decision': local_holdout_decision['decision'], + 'error': False, + 'reasons': reasons + } + # Check for forced decision optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext( feature_flag.key, experiment.key) diff --git a/optimizely/entities.py b/optimizely/entities.py index f67dee89..75e441ec 100644 --- a/optimizely/entities.py +++ b/optimizely/entities.py @@ -223,6 +223,7 @@ def __init__( trafficAllocation: list[TrafficAllocation], audienceIds: list[str], audienceConditions: Optional[Sequence[str | list[str]]] = None, + includedRules: Optional[list[str]] = None, **kwargs: Any ): self.id = id @@ -232,6 +233,8 @@ def __init__( self.trafficAllocation = trafficAllocation self.audienceIds = audienceIds self.audienceConditions = audienceConditions + # None = global holdout (applies to all rules), list of rule IDs = local holdout + self.included_rules: Optional[list[str]] = includedRules def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]: """Returns audienceConditions if present, otherwise audienceIds. @@ -253,6 +256,18 @@ def is_activated(self) -> bool: """ return self.status == self.Status.RUNNING + @property + def is_global(self) -> bool: + """Check if this is a global holdout (applies to all rules). + + A holdout is global when includedRules is None. + An empty list [] means a local holdout with no matching rules (not global). + + Returns: + True if includedRules is None (global), False if includedRules is a list (local). + """ + return self.included_rules is None + def __str__(self) -> str: return self.key diff --git a/optimizely/helpers/types.py b/optimizely/helpers/types.py index 6b704c36..6dea8b09 100644 --- a/optimizely/helpers/types.py +++ b/optimizely/helpers/types.py @@ -130,3 +130,4 @@ class HoldoutDict(ExperimentDict): Extends ExperimentDict with holdout-specific properties. """ holdoutStatus: HoldoutStatus + includedRules: Optional[list[str]] # None = global holdout, list of rule IDs = local holdout diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 0fb0f35f..495e354f 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -93,7 +93,12 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): holdouts_data: list[types.HoldoutDict] = config.get('holdouts', []) self.holdouts: list[entities.Holdout] = [] self.holdout_id_map: dict[str, entities.Holdout] = {} + # Legacy flag-level map kept for backward compatibility self.flag_holdouts_map: dict[str, list[entities.Holdout]] = {} + # Global holdouts (includedRules is None) — evaluated at flag level + self.global_holdouts: list[entities.Holdout] = [] + # Rule-level holdouts map: rule_id -> [Holdout] for local holdouts + self.rule_holdouts_map: dict[str, list[entities.Holdout]] = {} # Convert holdout dicts to Holdout entities for holdout_data in holdouts_data: @@ -108,6 +113,16 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): # Map by ID for quick lookup self.holdout_id_map[holdout.id] = holdout + # Categorize holdout as global or local + if holdout.is_global: + self.global_holdouts.append(holdout) + else: + # Local holdout: map each included rule ID to this holdout + 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) self.experiment_id_map: dict[str, entities.Experiment] = self._generate_key_map( @@ -240,10 +255,9 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): everyone_else_variation.variables, 'id', entities.Variation.VariableUsage ) - # Map all running holdouts to this flag - applicable_holdouts = list(self.holdout_id_map.values()) - if applicable_holdouts: - self.flag_holdouts_map[feature.key] = applicable_holdouts + # Map global holdouts to this flag (for legacy flag-level access) + if self.global_holdouts: + self.flag_holdouts_map[feature.key] = list(self.global_holdouts) rollout = None if len(feature.rolloutId) == 0 else self.rollout_id_map[feature.rolloutId] if rollout: @@ -881,17 +895,42 @@ def get_flag_variation( def get_holdouts_for_flag(self, flag_key: str) -> list[entities.Holdout]: """ Helper method to get holdouts from an applied feature flag. + Returns global holdouts for the given flag (backward-compatible). + Args: flag_key: Key of the feature flag. Returns: - The holdouts that apply for a specific flag as Holdout entity objects. + The global holdouts that apply for a specific flag as Holdout entity objects. """ if not self.holdouts: return [] return self.flag_holdouts_map.get(flag_key, []) + def get_global_holdouts(self) -> list[entities.Holdout]: + """Return all global holdouts (holdouts with includedRules == None). + + Global holdouts are evaluated at flag level before forced decisions. + + Returns: + List of global Holdout entities. + """ + return self.global_holdouts + + def get_holdouts_for_rule(self, rule_id: str) -> list[entities.Holdout]: + """Return local holdouts targeting a specific rule. + + Local holdouts are evaluated per-rule before audience and traffic checks. + + Args: + rule_id: The experiment or delivery rule ID to look up. + + Returns: + List of Holdout entities targeting the given rule ID. + """ + return self.rule_holdouts_map.get(rule_id, []) + def get_holdout(self, holdout_id: str) -> Optional[entities.Holdout]: """ Helper method to get holdout from holdout ID. diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index b5584a37..5e5109b5 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -1331,3 +1331,346 @@ def test_holdout_with_audience_match(self): self.assertIsNotNone(decision_no_match) finally: opt.close() + + +# --------------------------------------------------------------------------- +# Local Holdout Tests (FSSDK-12369) +# --------------------------------------------------------------------------- + +class LocalHoldoutTest(base.BaseTest): + """Tests for local holdout support (includedRules field). + + Covers: + - Data model: is_global property, includedRules parsing + - Mapping logic: get_global_holdouts(), get_holdouts_for_rule() + - Decision flow: local holdout evaluated before rule audience/traffic + - Source tracking: source and experiment_id correct + - Edge cases: backward compat, empty array vs None, unknown rule IDs + """ + + def setUp(self): + base.BaseTest.setUp(self) + self.error_handler = error_handler.NoOpErrorHandler() + self.spy_logger = mock.MagicMock(spec=logger.SimpleLogger) + self.spy_logger.logger = self.spy_logger + self.spy_user_profile_service = mock.MagicMock() + self.spy_cmab_service = mock.MagicMock() + + def tearDown(self): + if hasattr(self, 'opt_obj'): + self.opt_obj.close() + + def _make_holdout( + self, + holdout_id: str, + key: str, + status: str = 'Running', + included_rules=None, + traffic_end: int = 10000, + ) -> dict: + """Build a minimal holdout dict for tests.""" + holdout: dict = { + 'id': holdout_id, + 'key': key, + 'status': status, + 'audienceIds': [], + 'variations': [ + {'id': f'{holdout_id}_var', 'key': 'holdout_v', 'variables': []} + ], + 'trafficAllocation': [ + {'entityId': f'{holdout_id}_var', 'endOfRange': traffic_end} + ], + } + if included_rules is not None: + holdout['includedRules'] = included_rules + return holdout + + def _build_opt(self, holdouts: list) -> 'optimizely_module.Optimizely': + cfg = self.config_dict_with_features.copy() + cfg['holdouts'] = holdouts + self.opt_obj = optimizely_module.Optimizely(json.dumps(cfg)) + return self.opt_obj + + # ------------------------------------------------------------------ + # Data model tests + # ------------------------------------------------------------------ + + def test_holdout_entity_is_global_when_included_rules_absent(self): + """Holdout with no includedRules field is global (is_global == True).""" + from optimizely import entities + h = entities.Holdout( + id='h1', key='h1', status='Running', + variations=[], trafficAllocation=[], audienceIds=[] + ) + self.assertTrue(h.is_global) + self.assertIsNone(h.included_rules) + + def test_holdout_entity_is_not_global_when_included_rules_is_list(self): + """Holdout with includedRules list is local (is_global == False).""" + from optimizely import entities + h = entities.Holdout( + id='h2', key='h2', status='Running', + variations=[], trafficAllocation=[], audienceIds=[], + includedRules=['rule_1', 'rule_2'] + ) + self.assertFalse(h.is_global) + self.assertEqual(h.included_rules, ['rule_1', 'rule_2']) + + def test_holdout_entity_empty_included_rules_is_not_global(self): + """Holdout with empty includedRules [] is local (not global).""" + from optimizely import entities + h = entities.Holdout( + id='h3', key='h3', status='Running', + variations=[], trafficAllocation=[], audienceIds=[], + includedRules=[] + ) + self.assertFalse(h.is_global) + self.assertEqual(h.included_rules, []) + + # ------------------------------------------------------------------ + # ProjectConfig mapping tests + # ------------------------------------------------------------------ + + def test_get_global_holdouts_returns_only_global(self): + """get_global_holdouts() returns only holdouts with includedRules == None.""" + opt = self._build_opt([ + self._make_holdout('gh1', 'global_h1'), # global (no includedRules) + self._make_holdout('lh1', 'local_h1', included_rules=['111127']), # local + ]) + config = opt.config_manager.get_config() + global_holdouts = config.get_global_holdouts() + self.assertEqual(len(global_holdouts), 1) + self.assertEqual(global_holdouts[0].id, 'gh1') + + def test_get_holdouts_for_rule_returns_local_holdouts_for_rule(self): + """get_holdouts_for_rule() returns local holdouts targeting a given rule ID.""" + # '111127' is the experiment ID for test_feature_in_experiment + opt = self._build_opt([ + self._make_holdout('lh1', 'local_h1', included_rules=['111127']), + self._make_holdout('lh2', 'local_h2', included_rules=['other_rule']), + ]) + config = opt.config_manager.get_config() + holdouts_for_rule = config.get_holdouts_for_rule('111127') + self.assertEqual(len(holdouts_for_rule), 1) + self.assertEqual(holdouts_for_rule[0].id, 'lh1') + + def test_get_holdouts_for_rule_returns_empty_for_unknown_rule(self): + """get_holdouts_for_rule() returns [] for a rule ID not in any holdout.""" + opt = self._build_opt([ + self._make_holdout('lh1', 'local_h1', included_rules=['111127']), + ]) + config = opt.config_manager.get_config() + self.assertEqual(config.get_holdouts_for_rule('nonexistent_rule'), []) + + def test_holdout_targeting_multiple_rules_registered_for_each(self): + """A single local holdout with multiple includedRules appears in each rule's list.""" + opt = self._build_opt([ + self._make_holdout('lh_multi', 'local_multi', included_rules=['rule_a', 'rule_b', '111127']), + ]) + config = opt.config_manager.get_config() + self.assertEqual(len(config.get_holdouts_for_rule('rule_a')), 1) + self.assertEqual(len(config.get_holdouts_for_rule('rule_b')), 1) + self.assertEqual(len(config.get_holdouts_for_rule('111127')), 1) + self.assertEqual(config.get_holdouts_for_rule('rule_a')[0].id, 'lh_multi') + + def test_local_holdout_not_added_to_global_holdouts(self): + """Local holdouts are NOT included in get_global_holdouts().""" + opt = self._build_opt([ + self._make_holdout('lh1', 'local_h1', included_rules=['111127']), + ]) + config = opt.config_manager.get_config() + self.assertEqual(config.get_global_holdouts(), []) + + def test_empty_included_rules_holdout_not_registered_in_rule_map(self): + """A holdout with includedRules=[] is local but targets no rules.""" + opt = self._build_opt([ + self._make_holdout('lh_empty', 'local_empty', included_rules=[]), + ]) + config = opt.config_manager.get_config() + # empty includedRules → not global, and not in any rule map + self.assertEqual(config.get_global_holdouts(), []) + self.assertEqual(config.get_holdouts_for_rule('111127'), []) + + def test_non_running_local_holdout_not_in_rule_map(self): + """Non-running local holdouts are not registered in rule_holdouts_map.""" + opt = self._build_opt([ + self._make_holdout('lh_draft', 'local_draft', status='Draft', included_rules=['111127']), + ]) + config = opt.config_manager.get_config() + self.assertEqual(config.get_holdouts_for_rule('111127'), []) + + # ------------------------------------------------------------------ + # Backward compatibility + # ------------------------------------------------------------------ + + def test_backward_compat_old_datafile_without_included_rules(self): + """Old datafiles without includedRules parse correctly as global holdouts.""" + cfg = self.config_dict_with_features.copy() + cfg['holdouts'] = [ + { + 'id': 'old_h1', + 'key': 'old_holdout', + 'status': 'Running', + 'audienceIds': [], + 'variations': [{'id': 'old_v1', 'key': 'old_control', 'variables': []}], + 'trafficAllocation': [{'entityId': 'old_v1', 'endOfRange': 10000}], + # No includedRules key — simulates old datafile + } + ] + self.opt_obj = optimizely_module.Optimizely(json.dumps(cfg)) + config = self.opt_obj.config_manager.get_config() + + global_holdouts = config.get_global_holdouts() + self.assertEqual(len(global_holdouts), 1) + self.assertEqual(global_holdouts[0].id, 'old_h1') + self.assertTrue(global_holdouts[0].is_global) + + def test_global_and_local_holdouts_coexist(self): + """Global and local holdouts can coexist; each is mapped correctly.""" + opt = self._build_opt([ + self._make_holdout('gh1', 'global_h'), # global + self._make_holdout('lh1', 'local_h', included_rules=['111127']), # local + ]) + config = opt.config_manager.get_config() + self.assertEqual(len(config.get_global_holdouts()), 1) + self.assertEqual(config.get_global_holdouts()[0].id, 'gh1') + self.assertEqual(len(config.get_holdouts_for_rule('111127')), 1) + self.assertEqual(config.get_holdouts_for_rule('111127')[0].id, 'lh1') + + # ------------------------------------------------------------------ + # Decision flow: local holdout evaluated before rule + # ------------------------------------------------------------------ + + def test_local_holdout_hit_returns_holdout_decision_for_experiment_rule(self): + """When user hits local holdout, decision source is HOLDOUT for experiment rule.""" + # '111127' is the experiment ID for test_feature_in_experiment + opt = self._build_opt([ + self._make_holdout('lh1', 'local_h1', included_rules=['111127'], traffic_end=10000), + ]) + config = opt.config_manager.get_config() + ds = decision_service.DecisionService( + self.spy_logger, self.spy_user_profile_service, self.spy_cmab_service + ) + + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + self.assertIsNotNone(feature_flag) + user_context = opt.create_user_context('testUserId', {}) + + result = ds.get_decision_for_flag(feature_flag, user_context, config) + + self.assertIsNotNone(result) + decision = result['decision'] + # Decision source must be HOLDOUT + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + # Experiment on the decision must be the holdout + self.assertEqual(decision.experiment.id, 'lh1') + + def test_local_holdout_miss_falls_through_to_rule_evaluation(self): + """When user misses local holdout (traffic=0%), decision falls through to experiment.""" + # 0% traffic => user never hits local holdout + opt = self._build_opt([ + self._make_holdout('lh1', 'local_h1', included_rules=['111127'], traffic_end=0), + ]) + config = opt.config_manager.get_config() + ds = decision_service.DecisionService( + self.spy_logger, self.spy_user_profile_service, self.spy_cmab_service + ) + + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + user_context = opt.create_user_context('testUserId', {}) + + result = ds.get_decision_for_flag(feature_flag, user_context, config) + self.assertIsNotNone(result) + decision = result['decision'] + # Should not be a holdout decision (fell through to experiment or rollout) + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + + def test_local_holdout_only_applies_to_its_rule(self): + """Local holdout targeting rule X does not affect other rules.""" + opt = self._build_opt([ + # Local holdout targets only 'other_rule', NOT '111127' + self._make_holdout('lh1', 'local_h1', included_rules=['other_rule'], traffic_end=10000), + ]) + config = opt.config_manager.get_config() + ds = decision_service.DecisionService( + self.spy_logger, self.spy_user_profile_service, self.spy_cmab_service + ) + + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + user_context = opt.create_user_context('testUserId', {}) + + result = ds.get_decision_for_flag(feature_flag, user_context, config) + self.assertIsNotNone(result) + # The local holdout targets 'other_rule', not '111127', so it should NOT apply + decision = result['decision'] + self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) + + def test_global_holdout_evaluated_before_local_holdout(self): + """Global holdout is evaluated at flag level before local holdouts per rule.""" + opt = self._build_opt([ + # Global holdout with 100% traffic + self._make_holdout('gh1', 'global_h', traffic_end=10000), + # Local holdout also with 100% traffic + self._make_holdout('lh1', 'local_h', included_rules=['111127'], traffic_end=10000), + ]) + config = opt.config_manager.get_config() + ds = decision_service.DecisionService( + self.spy_logger, self.spy_user_profile_service, self.spy_cmab_service + ) + + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + user_context = opt.create_user_context('testUserId', {}) + + result = ds.get_decision_for_flag(feature_flag, user_context, config) + decision = result['decision'] + # Global holdout evaluated first — result must come from global holdout 'gh1' + self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertEqual(decision.experiment.id, 'gh1') + + def test_local_holdout_logs_reason_for_experiment_rule(self): + """Local holdout hit populates decision reasons for experiment rule.""" + opt = self._build_opt([ + self._make_holdout('lh1', 'local_h1', included_rules=['111127'], traffic_end=10000), + ]) + config = opt.config_manager.get_config() + ds = decision_service.DecisionService( + self.spy_logger, self.spy_user_profile_service, self.spy_cmab_service + ) + + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + user_context = opt.create_user_context('testUserId', {}) + + result = ds.get_decision_for_flag(feature_flag, user_context, config) + self.assertGreater(len(result['reasons']), 0) + # There should be a reason mentioning local holdout + reasons_text = ' '.join(result['reasons']) + self.assertIn('local holdout', reasons_text) + + def test_local_holdout_with_unknown_rule_id_does_not_crash(self): + """Holdout with rule ID not in datafile is silently skipped (no crash).""" + opt = self._build_opt([ + self._make_holdout('lh_unknown', 'local_unknown', included_rules=['nonexistent_rule_999']), + ]) + config = opt.config_manager.get_config() + ds = decision_service.DecisionService( + self.spy_logger, self.spy_user_profile_service, self.spy_cmab_service + ) + + feature_flag = config.get_feature_from_key('test_feature_in_experiment') + user_context = opt.create_user_context('testUserId', {}) + + # Should not raise any exception + result = ds.get_decision_for_flag(feature_flag, user_context, config) + self.assertIsNotNone(result) + self.assertIn('decision', result) + + def test_decide_api_with_local_holdout(self): + """End-to-end: decide() API returns HOLDOUT source when user hits local holdout.""" + opt = self._build_opt([ + self._make_holdout('lh1', 'local_h1', included_rules=['111127'], traffic_end=10000), + ]) + user_context = opt.create_user_context('testUserId', {}) + decision = user_context.decide('test_feature_in_experiment', [OptimizelyDecideOption.INCLUDE_REASONS]) + self.assertIsNotNone(decision) + # Reasons should be populated + self.assertGreater(len(decision.reasons), 0)