From 813d20c08439237b252e935be3be866a1cd59010 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 10 Dec 2025 12:45:25 -0600 Subject: [PATCH 01/13] [FSSDK-12094] Fix Ruby holdout implementation which related to FSC failures --- optimizely/decision_service.py | 211 +++++++++++++------------ optimizely/optimizely.py | 25 +-- optimizely/project_config.py | 90 +++++++---- tests/test_config.py | 4 +- tests/test_decision_service.py | 15 +- tests/test_decision_service_holdout.py | 13 +- tests/test_user_context.py | 6 +- 7 files changed, 209 insertions(+), 155 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 78be89d8..a14c55f4 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -714,8 +714,8 @@ def get_decision_for_flag( reasons.extend(holdout_decision['reasons']) decision = holdout_decision['decision'] - # Check if user was bucketed into holdout (has a variation) - if decision.variation is None: + # Check if user was bucketed into holdout (has a decision) + if decision is None: continue message = ( @@ -730,21 +730,102 @@ def get_decision_for_flag( 'reasons': reasons } - # If no holdout decision, fall back to existing experiment/rollout logic - # Use get_variations_for_feature_list which handles experiments and rollouts - fallback_result = self.get_variations_for_feature_list( - project_config, [feature_flag], user_context, decide_options - )[0] - - # Merge reasons - if fallback_result.get('reasons'): - reasons.extend(fallback_result['reasons']) + # Check if the feature flag has an experiment and the user is bucketed into that experiment + if feature_flag.experimentIds: + # Iterate through experiments to find a match + for experiment_id in feature_flag.experimentIds: + experiment = project_config.get_experiment_from_id(experiment_id) + if not experiment: + continue + + # Check for forced decision + optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext( + feature_flag.key, experiment.key + ) + forced_decision_variation, forced_reasons = self.validated_forced_decision( + project_config, optimizely_decision_context, user_context + ) + reasons.extend(forced_reasons) + + if forced_decision_variation: + decision = Decision( + experiment, forced_decision_variation, enums.DecisionSources.FEATURE_TEST, None + ) + return { + 'decision': decision, + 'error': False, + 'reasons': reasons + } + + # Get variation through normal bucketing + variation_result = self.get_variation( + project_config, experiment, user_context, user_profile_tracker, reasons, decide_options + ) + cmab_uuid = variation_result['cmab_uuid'] + variation_reasons = variation_result['reasons'] + decision_variation = variation_result['variation'] + error = variation_result['error'] + reasons.extend(variation_reasons) + + # If there's an error, return immediately + if error: + decision = Decision(experiment, None, enums.DecisionSources.FEATURE_TEST, cmab_uuid) + return { + 'decision': decision, + 'error': True, + 'reasons': reasons + } + + # If user is bucketed into a variation, return the decision + if decision_variation: + self.logger.debug( + f'User "{user_id}" ' + f'bucketed into experiment "{experiment.key}" of feature "{feature_flag.key}".' + ) + decision = Decision( + experiment, decision_variation, enums.DecisionSources.FEATURE_TEST, cmab_uuid + ) + return { + 'decision': decision, + 'error': False, + 'reasons': reasons + } + + # Check if the feature flag has a rollout and the user is bucketed into that rollout + rollout_decision, rollout_reasons = self.get_variation_for_rollout( + project_config, feature_flag, user_context + ) + reasons.extend(rollout_reasons) + + if rollout_decision and rollout_decision.variation: + # Check if this was a forced decision (last reason contains "forced decision map") + is_forced_decision = reasons and 'forced decision map' in reasons[-1] if reasons else False + + if not is_forced_decision: + # Only add the "bucketed into rollout" message for normal bucketing + message = ( + f"The user '{user_id}' is bucketed into a rollout " + f"for feature flag '{feature_flag.key}'." + ) + self.logger.info(message) + reasons.append(message) - return { - 'decision': fallback_result['decision'], - 'error': fallback_result.get('error', False), - 'reasons': reasons - } + return { + 'decision': rollout_decision, + 'error': False, + 'reasons': reasons + } + else: + message = ( + f"The user '{user_id}' is not bucketed into a rollout " + f"for feature flag '{feature_flag.key}'." + ) + self.logger.info(message) + return { + 'decision': Decision(None, None, enums.DecisionSources.ROLLOUT, None), + 'error': False, + 'reasons': reasons + } def get_variation_for_holdout( self, @@ -931,100 +1012,24 @@ def get_variations_for_feature_list( - 'error': Boolean indicating if an error occurred during the decision process. - 'reasons': List of log messages representing decision making for each feature. """ - decide_reasons: list[str] = [] - + ignore_ups = False if options: ignore_ups = OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE in options - else: - ignore_ups = False user_profile_tracker: Optional[UserProfileTracker] = None - if self.user_profile_service is not None and not ignore_ups: - user_profile_tracker = UserProfileTracker(user_context.user_id, self.user_profile_service, self.logger) - user_profile_tracker.load_user_profile(decide_reasons, None) + if not ignore_ups and self.user_profile_service: + user_id = user_context.user_id + user_profile_tracker = UserProfileTracker(user_id, self.user_profile_service, self.logger) + user_profile_tracker.load_user_profile([], None) decisions = [] + for feature_flag in features: + decision = self.get_decision_for_flag( + feature_flag, user_context, project_config, options, user_profile_tracker + ) + decisions.append(decision) - for feature in features: - feature_reasons = decide_reasons.copy() - experiment_decision_found = False # Track if an experiment decision was made for the feature - - # Check if the feature flag is under an experiment - if feature.experimentIds: - for experiment_id in feature.experimentIds: - experiment = project_config.get_experiment_from_id(experiment_id) - decision_variation: Optional[Union[entities.Variation, VariationDict]] = None - - if experiment: - optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext( - feature.key, experiment.key) - forced_decision_variation, reasons_received = self.validated_forced_decision( - project_config, optimizely_decision_context, user_context) - feature_reasons.extend(reasons_received) - - if forced_decision_variation: - decision_variation = forced_decision_variation - cmab_uuid = None - error = False - else: - variation_result = self.get_variation( - project_config, experiment, user_context, user_profile_tracker, feature_reasons, options - ) - cmab_uuid = variation_result['cmab_uuid'] - variation_reasons = variation_result['reasons'] - decision_variation = variation_result['variation'] - error = variation_result['error'] - feature_reasons.extend(variation_reasons) - - if error: - decision = Decision(experiment, None, enums.DecisionSources.FEATURE_TEST, cmab_uuid) - decision_result: DecisionResult = { - 'decision': decision, - 'error': True, - 'reasons': feature_reasons - } - decisions.append(decision_result) - experiment_decision_found = True - break - - if decision_variation: - self.logger.debug( - f'User "{user_context.user_id}" ' - f'bucketed into experiment "{experiment.key}" of feature "{feature.key}".' - ) - decision = Decision(experiment, decision_variation, - enums.DecisionSources.FEATURE_TEST, cmab_uuid) - decision_result = { - 'decision': decision, - 'error': False, - 'reasons': feature_reasons - } - decisions.append(decision_result) - experiment_decision_found = True # Mark that a decision was found - break # Stop after the first successful experiment decision - - # Only process rollout if no experiment decision was found and no error - if not experiment_decision_found: - rollout_decision, rollout_reasons = self.get_variation_for_rollout(project_config, - feature, - user_context) - if rollout_reasons: - feature_reasons.extend(rollout_reasons) - if rollout_decision: - self.logger.debug(f'User "{user_context.user_id}" ' - f'bucketed into rollout for feature "{feature.key}".') - else: - self.logger.debug(f'User "{user_context.user_id}" ' - f'not bucketed into any rollout for feature "{feature.key}".') - - decision_result = { - 'decision': rollout_decision, - 'error': False, - 'reasons': feature_reasons - } - decisions.append(decision_result) - - if self.user_profile_service is not None and user_profile_tracker is not None and ignore_ups is False: + if user_profile_tracker: user_profile_tracker.save_user_profile() return decisions diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index 7b27d849..7353e10a 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -433,7 +433,7 @@ def _get_feature_variable_for_type( decision_result = self.decision_service.get_variation_for_feature(project_config, feature_flag, user_context) decision = decision_result['decision'] - if decision.variation: + if decision and decision.variation: feature_enabled = self._get_feature_enabled(decision.variation) if feature_enabled: @@ -792,23 +792,24 @@ def is_feature_enabled(self, feature_key: str, user_id: str, attributes: Optiona user_context = OptimizelyUserContext(self, self.logger, user_id, attributes, False) decision = self.decision_service.get_variation_for_feature(project_config, feature, user_context)['decision'] - cmab_uuid = decision.cmab_uuid + cmab_uuid = decision.cmab_uuid if decision else None - is_source_experiment = decision.source == enums.DecisionSources.FEATURE_TEST - is_source_rollout = decision.source == enums.DecisionSources.ROLLOUT + is_source_experiment = decision.source == enums.DecisionSources.FEATURE_TEST if decision else False + is_source_rollout = decision.source == enums.DecisionSources.ROLLOUT if decision else False - if decision.variation: + if decision and decision.variation: if self._get_feature_enabled(decision.variation) is True: feature_enabled = True - if (is_source_rollout or not decision.variation) and project_config.get_send_flag_decisions_value(): + if (decision and (is_source_rollout or not decision.variation) and + project_config.get_send_flag_decisions_value()): self._send_impression_event( project_config, decision.experiment, decision.variation, feature.key, decision.experiment.key if decision.experiment else '', str(decision.source), feature_enabled, user_id, attributes, cmab_uuid ) # Send event if Decision came from an experiment. - if is_source_experiment and decision.variation and decision.experiment: + if decision and is_source_experiment and decision.variation and decision.experiment: source_info = { 'experiment_key': decision.experiment.key, 'variation_key': self._get_variation_key(decision.variation), @@ -1244,7 +1245,7 @@ def _create_optimizely_decision( ) -> OptimizelyDecision: user_id = user_context.user_id feature_enabled = False - if flag_decision.variation is not None: + if flag_decision and flag_decision.variation is not None: if self._get_feature_enabled(flag_decision.variation): feature_enabled = True @@ -1252,9 +1253,9 @@ def _create_optimizely_decision( # Create Optimizely Decision Result. attributes = user_context.get_user_attributes() - rule_key = flag_decision.experiment.key if flag_decision.experiment else None + rule_key = flag_decision.experiment.key if (flag_decision and flag_decision.experiment) else None all_variables = {} - decision_source = flag_decision.source + decision_source = flag_decision.source if flag_decision else None decision_event_dispatched = False feature_flag = project_config.feature_key_map.get(flag_key) @@ -1262,7 +1263,9 @@ def _create_optimizely_decision( # Send impression event if Decision came from a feature # test and decide options doesn't include disableDecisionEvent if OptimizelyDecideOption.DISABLE_DECISION_EVENT not in decide_options: - if decision_source == DecisionSources.FEATURE_TEST or project_config.send_flag_decisions: + if (decision_source == DecisionSources.FEATURE_TEST or + decision_source == DecisionSources.HOLDOUT or + project_config.send_flag_decisions): self._send_impression_event(project_config, flag_decision.experiment, flag_decision.variation, diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 23508bb6..d8ee66c3 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -92,7 +92,7 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): self.holdouts: list[HoldoutDict] = config.get('holdouts', []) self.holdout_id_map: dict[str, HoldoutDict] = {} - self.global_holdouts: dict[str, HoldoutDict] = {} + self.global_holdouts: list[HoldoutDict] = [] self.included_holdouts: dict[str, list[HoldoutDict]] = {} self.excluded_holdouts: dict[str, list[HoldoutDict]] = {} self.flag_holdouts_map: dict[str, list[HoldoutDict]] = {} @@ -101,27 +101,40 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): if holdout.get('status') != 'Running': continue + # Ensure holdout has layerId field (holdouts don't have campaigns) + if 'layerId' not in holdout: + holdout['layerId'] = '' + holdout_id = holdout['id'] self.holdout_id_map[holdout_id] = holdout - included_flags = holdout.get('includedFlags') - if not included_flags: - # This is a global holdout - self.global_holdouts[holdout_id] = holdout - - excluded_flags = holdout.get('excludedFlags') - if excluded_flags: - for flag_id in excluded_flags: - if flag_id not in self.excluded_holdouts: - self.excluded_holdouts[flag_id] = [] - self.excluded_holdouts[flag_id].append(holdout) - else: - # This holdout applies to specific flags + included_flags = holdout.get('includedFlags', []) + excluded_flags = holdout.get('excludedFlags', []) + + has_included = bool(included_flags) + has_excluded = bool(excluded_flags) + + if not has_included and not has_excluded: + # No included or excluded flags - this is a global holdout + self.global_holdouts.append(holdout) + + elif has_included: + # Has included flags - add to included_holdouts map + # (works for both cases with or without excluded flags) for flag_id in included_flags: if flag_id not in self.included_holdouts: self.included_holdouts[flag_id] = [] self.included_holdouts[flag_id].append(holdout) + elif has_excluded and not has_included: + # No included flags but has excluded flags - global with exclusions + self.global_holdouts.append(holdout) + + for flag_id in excluded_flags: + if flag_id not in self.excluded_holdouts: + self.excluded_holdouts[flag_id] = [] + self.excluded_holdouts[flag_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( @@ -221,20 +234,6 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): self.experiment_feature_map[exp_id] = [feature.id] rules.append(self.experiment_id_map[exp_id]) - flag_id = feature.id - applicable_holdouts = [] - - if flag_id in self.included_holdouts: - applicable_holdouts.extend(self.included_holdouts[flag_id]) - - for holdout in self.global_holdouts.values(): - excluded_flag_ids = holdout.get('excludedFlags', []) - if flag_id not in excluded_flag_ids: - applicable_holdouts.append(holdout) - - if applicable_holdouts: - self.flag_holdouts_map[feature.key] = applicable_holdouts - rollout = None if len(feature.rolloutId) == 0 else self.rollout_id_map[feature.rolloutId] if rollout: for exp in rollout.experiments: @@ -839,6 +838,7 @@ def get_holdouts_for_flag(self, flag_key: str) -> list[HoldoutDict]: Args: flag_key: Key of the feature flag. + This parameter is required and should not be null/None. Returns: The holdouts that apply for a specific flag. @@ -846,7 +846,39 @@ def get_holdouts_for_flag(self, flag_key: str) -> list[HoldoutDict]: if not self.holdouts: return [] - return self.flag_holdouts_map.get(flag_key, []) + # Check cache first (before validation, so we cache the validation result too) + if flag_key in self.flag_holdouts_map: + return self.flag_holdouts_map[flag_key] + + # Validate that the flag exists in the datafile + feature = self.feature_key_map.get(flag_key) + if not feature: + # Cache the empty result for non-existent flags + self.flag_holdouts_map[flag_key] = [] + return [] + + flag_id = feature.id + + # Prioritize global holdouts first + excluded = self.excluded_holdouts.get(flag_id, []) + + if excluded: + # Filter out excluded holdouts from global holdouts + active_holdouts = [ + holdout for holdout in self.global_holdouts + if holdout not in excluded + ] + else: + active_holdouts = self.global_holdouts.copy() + + # Append included holdouts + included = self.included_holdouts.get(flag_id, []) + active_holdouts.extend(included) + + # Cache the result + self.flag_holdouts_map[flag_key] = active_holdouts + + return self.flag_holdouts_map[flag_key] def get_holdout(self, holdout_id: str) -> Optional[HoldoutDict]: """ Helper method to get holdout from holdout ID. diff --git a/tests/test_config.py b/tests/test_config.py index 08a81f6d..7c1097e4 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1512,7 +1512,9 @@ def test_holdout_initialization__categorizes_holdouts_properly(self): self.assertIn('holdout_1', self.config_with_holdouts.holdout_id_map) self.assertIn('holdout_2', self.config_with_holdouts.holdout_id_map) - self.assertIn('holdout_1', self.config_with_holdouts.global_holdouts) + # Check if holdout_1 is in global_holdouts (list of dicts) + holdout_ids = [h['id'] for h in self.config_with_holdouts.global_holdouts] + self.assertIn('holdout_1', holdout_ids) # Use correct feature flag IDs boolean_feature_id = '91111' diff --git a/tests/test_decision_service.py b/tests/test_decision_service.py index dbcb7436..231a79f9 100644 --- a/tests/test_decision_service.py +++ b/tests/test_decision_service.py @@ -1377,15 +1377,19 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_rollout(sel expected_variation = self.project_config.get_variation_from_id( "211127", "211129" ) + from optimizely.decision_service import Decision + from optimizely.helpers import enums + expected_decision = Decision(None, expected_variation, enums.DecisionSources.ROLLOUT, None) get_variation_for_rollout_patch = mock.patch( "optimizely.decision_service.DecisionService.get_variation_for_rollout", - return_value=[expected_variation, None], + return_value=(expected_decision, []), ) with get_variation_for_rollout_patch as mock_get_variation_for_rollout, \ self.mock_decision_logger as mock_decision_service_logging: - variation_received = self.decision_service.get_variation_for_feature( + decision_result = self.decision_service.get_variation_for_feature( self.project_config, feature, user, False - )['decision'] + ) + variation_received = decision_result['decision'].variation if decision_result['decision'] else None self.assertEqual( expected_variation, variation_received, @@ -1395,9 +1399,8 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_rollout(sel self.project_config, feature, user ) - # Assert no log messages were generated - self.assertEqual(1, mock_decision_service_logging.debug.call_count) - self.assertEqual(1, len(mock_decision_service_logging.method_calls)) + # Assert info log messages were generated (changed from debug due to new flow through get_decision_for_flag) + self.assertEqual(1, mock_decision_service_logging.info.call_count) def test_get_variation_for_feature__returns_variation_if_user_not_in_experiment_but_in_rollout( self, diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index 0f47e997..d74c6618 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -507,9 +507,16 @@ def test_uses_consistent_bucketing_for_same_user(self): # If both have decisions, they should match if decision1 and decision2: - # Variation is an object, not a dict, so use attributes - var1_id = decision1.variation.id if decision1.variation else None - var2_id = decision2.variation.id if decision2.variation else None + # Variation can be either an object or a dict (for holdouts) + def get_variation_id(variation): + if variation is None: + return None + if isinstance(variation, dict): + return variation.get('id') + return variation.id + + var1_id = get_variation_id(decision1.variation) + var2_id = get_variation_id(decision2.variation) self.assertEqual( var1_id, var2_id, diff --git a/tests/test_user_context.py b/tests/test_user_context.py index 3ae9be0d..fbc261bb 100644 --- a/tests/test_user_context.py +++ b/tests/test_user_context.py @@ -872,7 +872,8 @@ def test_decide__option__include_reasons__feature_rollout(self): 'Evaluating audiences for rule 1: ["11154"].', 'Audiences for rule 1 collectively evaluated to TRUE.', 'User "test_user" meets audience conditions for targeting rule 1.', - 'User "test_user" bucketed into a targeting rule 1.' + 'User "test_user" bucketed into a targeting rule 1.', + 'The user "test_user" is bucketed into a rollout for feature flag "test_feature_in_rollout".' ] self.assertEqual(expected_reasons, actual.reasons) @@ -1270,7 +1271,8 @@ def test_decide_reasons__hit_everyone_else_rule(self): 'Evaluating audiences for rule Everyone Else: [].', 'Audiences for rule Everyone Else collectively evaluated to TRUE.', 'User "abcde" meets audience conditions for targeting rule Everyone Else.', - 'User "abcde" bucketed into a targeting rule Everyone Else.' + 'User "abcde" bucketed into a targeting rule Everyone Else.', + "The user 'abcde' is bucketed into a rollout for feature flag 'test_feature_in_rollout'." ] self.assertEqual(expected_reasons, actual.reasons) From 46640d809bc8cad816f8df2e36ba35456f7420c5 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 10 Dec 2025 13:26:58 -0600 Subject: [PATCH 02/13] Fix tests --- optimizely/decision_service.py | 8 ++++---- tests/test_user_context.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index a14c55f4..d3c5438b 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -714,8 +714,8 @@ def get_decision_for_flag( reasons.extend(holdout_decision['reasons']) decision = holdout_decision['decision'] - # Check if user was bucketed into holdout (has a decision) - if decision is None: + # Check if user was bucketed into holdout (has a variation) + if decision is None or decision.variation is None: continue message = ( @@ -905,9 +905,9 @@ def get_variation_for_holdout( self.logger.info(message) decide_reasons.append(message) - # Create Decision for holdout - experiment is None, source is HOLDOUT + # Create Decision for holdout - pass holdout dict as experiment, source is HOLDOUT holdout_decision: Decision = Decision( - experiment=None, + experiment=holdout, variation=variation, source=enums.DecisionSources.HOLDOUT, cmab_uuid=None diff --git a/tests/test_user_context.py b/tests/test_user_context.py index fbc261bb..1b72e3e3 100644 --- a/tests/test_user_context.py +++ b/tests/test_user_context.py @@ -873,7 +873,7 @@ def test_decide__option__include_reasons__feature_rollout(self): 'Audiences for rule 1 collectively evaluated to TRUE.', 'User "test_user" meets audience conditions for targeting rule 1.', 'User "test_user" bucketed into a targeting rule 1.', - 'The user "test_user" is bucketed into a rollout for feature flag "test_feature_in_rollout".' + "The user 'test_user' is bucketed into a rollout for feature flag 'test_feature_in_rollout'." ] self.assertEqual(expected_reasons, actual.reasons) From b3f17f1c317e0bfd126185be25d7188559875c0b Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 10 Dec 2025 14:38:12 -0600 Subject: [PATCH 03/13] Fix unit and FSC --- optimizely/event/user_event_factory.py | 6 ++++- optimizely/optimizely.py | 37 ++++++++++++++++++++------ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/optimizely/event/user_event_factory.py b/optimizely/event/user_event_factory.py index 7a77720f..d1a7e64a 100644 --- a/optimizely/event/user_event_factory.py +++ b/optimizely/event/user_event_factory.py @@ -67,7 +67,11 @@ def create_impression_event( variation: Optional[Variation] = None experiment_id = None if activated_experiment: - experiment_id = activated_experiment.id + # Handle both Experiment objects and holdout dicts + if isinstance(activated_experiment, dict): + experiment_id = activated_experiment.get('id') + else: + experiment_id = activated_experiment.id if variation_id and flag_key: # need this condition when we send events involving forced decisions diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index 7353e10a..670cd2ea 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -261,6 +261,25 @@ def _get_feature_enabled(self, variation: Optional[Union[entities.Variation, Var except (AttributeError, KeyError, TypeError): return False + def _get_experiment_key(self, experiment: Optional[Union[entities.Experiment, dict]]) -> Optional[str]: + """Helper to extract experiment/holdout key from either dict or Experiment object. + Args: + experiment: Either a dict (from holdout) or entities.Experiment object + Returns: + The experiment/holdout key as a string, or None if not available + """ + if experiment is None: + return None + + try: + if isinstance(experiment, dict): + return experiment.get('key') + else: + return experiment.key + except (AttributeError, KeyError, TypeError): + self.logger.warning(f"Unable to extract experiment key from {type(experiment)}") + return None + def _validate_instantiation_options(self) -> None: """ Helper method to validate all instantiation parameters. @@ -455,7 +474,7 @@ def _get_feature_variable_for_type( if decision.source in (enums.DecisionSources.FEATURE_TEST, enums.DecisionSources.HOLDOUT): source_info = { - 'experiment_key': decision.experiment.key if decision.experiment else None, + 'experiment_key': self._get_experiment_key(decision.experiment), 'variation_key': self._get_variation_key(decision.variation), } @@ -559,7 +578,7 @@ def _get_all_feature_variables_for_type( if decision.source == enums.DecisionSources.FEATURE_TEST: source_info = { - 'experiment_key': decision.experiment.key if decision.experiment else None, + 'experiment_key': self._get_experiment_key(decision.experiment), 'variation_key': self._get_variation_key(decision.variation), } @@ -804,19 +823,21 @@ def is_feature_enabled(self, feature_key: str, user_id: str, attributes: Optiona if (decision and (is_source_rollout or not decision.variation) and project_config.get_send_flag_decisions_value()): self._send_impression_event( - project_config, decision.experiment, decision.variation, feature.key, decision.experiment.key if - decision.experiment else '', str(decision.source), feature_enabled, user_id, attributes, cmab_uuid + project_config, decision.experiment, decision.variation, feature.key, + self._get_experiment_key(decision.experiment) or '', str(decision.source), feature_enabled, + user_id, attributes, cmab_uuid ) # Send event if Decision came from an experiment. if decision and is_source_experiment and decision.variation and decision.experiment: source_info = { - 'experiment_key': decision.experiment.key, + 'experiment_key': self._get_experiment_key(decision.experiment), 'variation_key': self._get_variation_key(decision.variation), } self._send_impression_event( - project_config, decision.experiment, decision.variation, feature.key, decision.experiment.key, - str(decision.source), feature_enabled, user_id, attributes, cmab_uuid + project_config, decision.experiment, decision.variation, feature.key, + self._get_experiment_key(decision.experiment) or '', str(decision.source), feature_enabled, + user_id, attributes, cmab_uuid ) if feature_enabled: @@ -1253,7 +1274,7 @@ def _create_optimizely_decision( # Create Optimizely Decision Result. attributes = user_context.get_user_attributes() - rule_key = flag_decision.experiment.key if (flag_decision and flag_decision.experiment) else None + rule_key = self._get_experiment_key(flag_decision.experiment) if flag_decision else None all_variables = {} decision_source = flag_decision.source if flag_decision else None decision_event_dispatched = False From 4acf348c7f8541b56ef11f2bd70cfa85f83ae66e Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 10 Dec 2025 21:55:37 -0600 Subject: [PATCH 04/13] Fix unit and FSC tests --- optimizely/decision_service.py | 4 ++-- optimizely/optimizely.py | 6 +++--- optimizely/project_config.py | 31 +++++++++++++++++-------------- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index d3c5438b..d2b7f00b 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -708,7 +708,7 @@ def get_decision_for_flag( user_id = user_context.user_id # Check holdouts - holdouts = project_config.get_holdouts_for_flag(feature_flag.key) + holdouts = project_config.get_holdouts_for_flag(feature_flag.id) for holdout in holdouts: holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config) reasons.extend(holdout_decision['reasons']) @@ -907,7 +907,7 @@ def get_variation_for_holdout( # Create Decision for holdout - pass holdout dict as experiment, source is HOLDOUT holdout_decision: Decision = Decision( - experiment=holdout, + experiment=None, variation=variation, source=enums.DecisionSources.HOLDOUT, cmab_uuid=None diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index 670cd2ea..017c1b7b 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -15,7 +15,7 @@ from typing import TYPE_CHECKING, Any, Optional, Union -from optimizely.helpers.types import VariationDict +from optimizely.helpers.types import HoldoutDict, VariationDict from . import decision_service @@ -261,10 +261,10 @@ def _get_feature_enabled(self, variation: Optional[Union[entities.Variation, Var except (AttributeError, KeyError, TypeError): return False - def _get_experiment_key(self, experiment: Optional[Union[entities.Experiment, dict]]) -> Optional[str]: + def _get_experiment_key(self, experiment: Optional[Union[entities.Experiment, HoldoutDict]]) -> Optional[str]: """Helper to extract experiment/holdout key from either dict or Experiment object. Args: - experiment: Either a dict (from holdout) or entities.Experiment object + experiment: Either a HoldoutDict (from holdout) or entities.Experiment object Returns: The experiment/holdout key as a string, or None if not available """ diff --git a/optimizely/project_config.py b/optimizely/project_config.py index d8ee66c3..75115293 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -833,12 +833,12 @@ def get_flag_variation( return None - def get_holdouts_for_flag(self, flag_key: str) -> list[HoldoutDict]: + def get_holdouts_for_flag(self, flag_key_or_id: str) -> list[HoldoutDict]: """ Helper method to get holdouts from an applied feature flag. Args: - flag_key: Key of the feature flag. - This parameter is required and should not be null/None. + flag_key_or_id: Key or ID of the feature flag. + This parameter is required and should not be null/None. Returns: The holdouts that apply for a specific flag. @@ -847,18 +847,21 @@ def get_holdouts_for_flag(self, flag_key: str) -> list[HoldoutDict]: return [] # Check cache first (before validation, so we cache the validation result too) - if flag_key in self.flag_holdouts_map: - return self.flag_holdouts_map[flag_key] + if flag_key_or_id in self.flag_holdouts_map: + return self.flag_holdouts_map[flag_key_or_id] - # Validate that the flag exists in the datafile - feature = self.feature_key_map.get(flag_key) - if not feature: + # Find the flag by key or ID + flag_id = None + for flag in self.feature_flags: + if flag['id'] == flag_key_or_id or flag['key'] == flag_key_or_id: + flag_id = flag['id'] + break + + if flag_id is None: # Cache the empty result for non-existent flags - self.flag_holdouts_map[flag_key] = [] + self.flag_holdouts_map[flag_key_or_id] = [] return [] - flag_id = feature.id - # Prioritize global holdouts first excluded = self.excluded_holdouts.get(flag_id, []) @@ -875,10 +878,10 @@ def get_holdouts_for_flag(self, flag_key: str) -> list[HoldoutDict]: included = self.included_holdouts.get(flag_id, []) active_holdouts.extend(included) - # Cache the result - self.flag_holdouts_map[flag_key] = active_holdouts + # Cache the result using the input parameter as the cache key + self.flag_holdouts_map[flag_key_or_id] = active_holdouts - return self.flag_holdouts_map[flag_key] + return self.flag_holdouts_map[flag_key_or_id] def get_holdout(self, holdout_id: str) -> Optional[HoldoutDict]: """ Helper method to get holdout from holdout ID. From 232d26cf83879fd2735441595dc8f999f05437d2 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 10 Dec 2025 22:12:34 -0600 Subject: [PATCH 05/13] Fix type check problems --- .github/workflows/python.yml | 9 +++++---- optimizely/decision_service.py | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/python.yml b/.github/workflows/python.yml index 6baa2c09..24d95e6a 100644 --- a/.github/workflows/python.yml +++ b/.github/workflows/python.yml @@ -91,10 +91,11 @@ jobs: fail-fast: false matrix: python-version: - - "pypy-3.9" - - "pypy-3.10" - - "3.9" - - "3.10" + # disabled due to librt is not available on this pypy version + # - "pypy-3.9" + # - "pypy-3.10" + # - "3.9" + # - "3.10" - "3.11" - "3.12" steps: diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index d2b7f00b..659bd803 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -907,7 +907,7 @@ def get_variation_for_holdout( # Create Decision for holdout - pass holdout dict as experiment, source is HOLDOUT holdout_decision: Decision = Decision( - experiment=None, + experiment=holdout, # type: ignore[arg-type] variation=variation, source=enums.DecisionSources.HOLDOUT, cmab_uuid=None From e6ad886bfda9918971dbaef17da34c5ca5ea092a Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 10 Dec 2025 23:11:53 -0600 Subject: [PATCH 06/13] Implement Ruby changes correctly --- optimizely/decision_service.py | 10 +++--- optimizely/optimizely.py | 58 +++++++++++----------------------- optimizely/project_config.py | 41 ++++++++++-------------- 3 files changed, 39 insertions(+), 70 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 659bd803..09e61de7 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -708,14 +708,14 @@ def get_decision_for_flag( user_id = user_context.user_id # Check holdouts - holdouts = project_config.get_holdouts_for_flag(feature_flag.id) + holdouts = project_config.get_holdouts_for_flag(feature_flag.key) for holdout in holdouts: holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config) reasons.extend(holdout_decision['reasons']) decision = holdout_decision['decision'] # Check if user was bucketed into holdout (has a variation) - if decision is None or decision.variation is None: + if decision.variation is None: continue message = ( @@ -905,9 +905,9 @@ def get_variation_for_holdout( self.logger.info(message) decide_reasons.append(message) - # Create Decision for holdout - pass holdout dict as experiment, source is HOLDOUT + # Create Decision for holdout - experiment is None, source is HOLDOUT holdout_decision: Decision = Decision( - experiment=holdout, # type: ignore[arg-type] + experiment=None, variation=variation, source=enums.DecisionSources.HOLDOUT, cmab_uuid=None @@ -1017,7 +1017,7 @@ def get_variations_for_feature_list( ignore_ups = OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE in options user_profile_tracker: Optional[UserProfileTracker] = None - if not ignore_ups and self.user_profile_service: + if self.user_profile_service is not None and not ignore_ups: user_id = user_context.user_id user_profile_tracker = UserProfileTracker(user_id, self.user_profile_service, self.logger) user_profile_tracker.load_user_profile([], None) diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index 017c1b7b..c6c37d5c 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -15,7 +15,7 @@ from typing import TYPE_CHECKING, Any, Optional, Union -from optimizely.helpers.types import HoldoutDict, VariationDict +from optimizely.helpers.types import VariationDict from . import decision_service @@ -261,25 +261,6 @@ def _get_feature_enabled(self, variation: Optional[Union[entities.Variation, Var except (AttributeError, KeyError, TypeError): return False - def _get_experiment_key(self, experiment: Optional[Union[entities.Experiment, HoldoutDict]]) -> Optional[str]: - """Helper to extract experiment/holdout key from either dict or Experiment object. - Args: - experiment: Either a HoldoutDict (from holdout) or entities.Experiment object - Returns: - The experiment/holdout key as a string, or None if not available - """ - if experiment is None: - return None - - try: - if isinstance(experiment, dict): - return experiment.get('key') - else: - return experiment.key - except (AttributeError, KeyError, TypeError): - self.logger.warning(f"Unable to extract experiment key from {type(experiment)}") - return None - def _validate_instantiation_options(self) -> None: """ Helper method to validate all instantiation parameters. @@ -452,7 +433,7 @@ def _get_feature_variable_for_type( decision_result = self.decision_service.get_variation_for_feature(project_config, feature_flag, user_context) decision = decision_result['decision'] - if decision and decision.variation: + if decision.variation: feature_enabled = self._get_feature_enabled(decision.variation) if feature_enabled: @@ -474,7 +455,7 @@ def _get_feature_variable_for_type( if decision.source in (enums.DecisionSources.FEATURE_TEST, enums.DecisionSources.HOLDOUT): source_info = { - 'experiment_key': self._get_experiment_key(decision.experiment), + 'experiment_key': decision.experiment.key if decision.experiment else None, 'variation_key': self._get_variation_key(decision.variation), } @@ -578,7 +559,7 @@ def _get_all_feature_variables_for_type( if decision.source == enums.DecisionSources.FEATURE_TEST: source_info = { - 'experiment_key': self._get_experiment_key(decision.experiment), + 'experiment_key': decision.experiment.key if decision.experiment else None, 'variation_key': self._get_variation_key(decision.variation), } @@ -811,33 +792,30 @@ def is_feature_enabled(self, feature_key: str, user_id: str, attributes: Optiona user_context = OptimizelyUserContext(self, self.logger, user_id, attributes, False) decision = self.decision_service.get_variation_for_feature(project_config, feature, user_context)['decision'] - cmab_uuid = decision.cmab_uuid if decision else None + cmab_uuid = decision.cmab_uuid - is_source_experiment = decision.source == enums.DecisionSources.FEATURE_TEST if decision else False - is_source_rollout = decision.source == enums.DecisionSources.ROLLOUT if decision else False + is_source_experiment = decision.source == enums.DecisionSources.FEATURE_TEST + is_source_rollout = decision.source == enums.DecisionSources.ROLLOUT - if decision and decision.variation: + if decision.variation: if self._get_feature_enabled(decision.variation) is True: feature_enabled = True - if (decision and (is_source_rollout or not decision.variation) and - project_config.get_send_flag_decisions_value()): + if (is_source_rollout or not decision.variation) and project_config.get_send_flag_decisions_value(): self._send_impression_event( - project_config, decision.experiment, decision.variation, feature.key, - self._get_experiment_key(decision.experiment) or '', str(decision.source), feature_enabled, - user_id, attributes, cmab_uuid + project_config, decision.experiment, decision.variation, feature.key, decision.experiment.key if + decision.experiment else '', str(decision.source), feature_enabled, user_id, attributes, cmab_uuid ) # Send event if Decision came from an experiment. - if decision and is_source_experiment and decision.variation and decision.experiment: + if is_source_experiment and decision.variation and decision.experiment: source_info = { - 'experiment_key': self._get_experiment_key(decision.experiment), + 'experiment_key': decision.experiment.key, 'variation_key': self._get_variation_key(decision.variation), } self._send_impression_event( - project_config, decision.experiment, decision.variation, feature.key, - self._get_experiment_key(decision.experiment) or '', str(decision.source), feature_enabled, - user_id, attributes, cmab_uuid + project_config, decision.experiment, decision.variation, feature.key, decision.experiment.key, + str(decision.source), feature_enabled, user_id, attributes, cmab_uuid ) if feature_enabled: @@ -1266,7 +1244,7 @@ def _create_optimizely_decision( ) -> OptimizelyDecision: user_id = user_context.user_id feature_enabled = False - if flag_decision and flag_decision.variation is not None: + if flag_decision.variation is not None: if self._get_feature_enabled(flag_decision.variation): feature_enabled = True @@ -1274,9 +1252,9 @@ def _create_optimizely_decision( # Create Optimizely Decision Result. attributes = user_context.get_user_attributes() - rule_key = self._get_experiment_key(flag_decision.experiment) if flag_decision else None + rule_key = flag_decision.experiment.key if flag_decision.experiment else None all_variables = {} - decision_source = flag_decision.source if flag_decision else None + decision_source = flag_decision.source decision_event_dispatched = False feature_flag = project_config.feature_key_map.get(flag_key) diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 75115293..32cd7e32 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -833,12 +833,12 @@ def get_flag_variation( return None - def get_holdouts_for_flag(self, flag_key_or_id: str) -> list[HoldoutDict]: + def get_holdouts_for_flag(self, flag_key: str) -> list[HoldoutDict]: """ Helper method to get holdouts from an applied feature flag. Args: - flag_key_or_id: Key or ID of the feature flag. - This parameter is required and should not be null/None. + flag_key: Key of the feature flag. + This parameter is required and should not be null/None. Returns: The holdouts that apply for a specific flag. @@ -846,31 +846,22 @@ def get_holdouts_for_flag(self, flag_key_or_id: str) -> list[HoldoutDict]: if not self.holdouts: return [] - # Check cache first (before validation, so we cache the validation result too) - if flag_key_or_id in self.flag_holdouts_map: - return self.flag_holdouts_map[flag_key_or_id] - - # Find the flag by key or ID - flag_id = None - for flag in self.feature_flags: - if flag['id'] == flag_key_or_id or flag['key'] == flag_key_or_id: - flag_id = flag['id'] - break - - if flag_id is None: - # Cache the empty result for non-existent flags - self.flag_holdouts_map[flag_key_or_id] = [] + # Get the feature flag to extract flag_id + feature = self.feature_key_map.get(flag_key) + if not feature: return [] + flag_id = feature.id + + # Check cache first (before validation, so we cache the validation result too) + if flag_id in self.flag_holdouts_map: + return self.flag_holdouts_map[flag_id] + # Prioritize global holdouts first excluded = self.excluded_holdouts.get(flag_id, []) if excluded: - # Filter out excluded holdouts from global holdouts - active_holdouts = [ - holdout for holdout in self.global_holdouts - if holdout not in excluded - ] + active_holdouts = [holdout for holdout in self.global_holdouts if holdout not in excluded] else: active_holdouts = self.global_holdouts.copy() @@ -878,10 +869,10 @@ def get_holdouts_for_flag(self, flag_key_or_id: str) -> list[HoldoutDict]: included = self.included_holdouts.get(flag_id, []) active_holdouts.extend(included) - # Cache the result using the input parameter as the cache key - self.flag_holdouts_map[flag_key_or_id] = active_holdouts + # Cache the result + self.flag_holdouts_map[flag_id] = active_holdouts - return self.flag_holdouts_map[flag_key_or_id] + return self.flag_holdouts_map[flag_id] def get_holdout(self, holdout_id: str) -> Optional[HoldoutDict]: """ Helper method to get holdout from holdout ID. From c505bc049134d49d3a205f5fecf2134fe250a121 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 10 Dec 2025 23:29:35 -0600 Subject: [PATCH 07/13] Fix FSC --- optimizely/decision_service.py | 4 ++-- optimizely/optimizely.py | 10 +++++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 09e61de7..412287aa 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -905,9 +905,9 @@ def get_variation_for_holdout( self.logger.info(message) decide_reasons.append(message) - # Create Decision for holdout - experiment is None, source is HOLDOUT + # Create Decision for holdout - pass holdout dict as experiment, source is HOLDOUT holdout_decision: Decision = Decision( - experiment=None, + experiment=holdout, # type: ignore[arg-type] variation=variation, source=enums.DecisionSources.HOLDOUT, cmab_uuid=None diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index c6c37d5c..c61c8db5 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -1252,7 +1252,15 @@ def _create_optimizely_decision( # Create Optimizely Decision Result. attributes = user_context.get_user_attributes() - rule_key = flag_decision.experiment.key if flag_decision.experiment else None + # Handle both Experiment entities and holdout dicts + if flag_decision.experiment: + if isinstance(flag_decision.experiment, dict): + # Holdout is a dict, not an Experiment entity + rule_key = flag_decision.experiment.get('key') + else: + rule_key = flag_decision.experiment.key + else: + rule_key = None all_variables = {} decision_source = flag_decision.source decision_event_dispatched = False From ef9114b7301c5eb53301fb76de7b1a779dff64c4 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Thu, 11 Dec 2025 11:50:11 -0600 Subject: [PATCH 08/13] Revert unnecessary changes --- optimizely/decision_service.py | 209 ++++++++++++------------- optimizely/event/user_event_factory.py | 6 +- optimizely/optimizely.py | 10 +- tests/test_decision_service.py | 4 +- tests/test_user_context.py | 6 +- 5 files changed, 108 insertions(+), 127 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 412287aa..78be89d8 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -730,102 +730,21 @@ def get_decision_for_flag( 'reasons': reasons } - # Check if the feature flag has an experiment and the user is bucketed into that experiment - if feature_flag.experimentIds: - # Iterate through experiments to find a match - for experiment_id in feature_flag.experimentIds: - experiment = project_config.get_experiment_from_id(experiment_id) - if not experiment: - continue - - # Check for forced decision - optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext( - feature_flag.key, experiment.key - ) - forced_decision_variation, forced_reasons = self.validated_forced_decision( - project_config, optimizely_decision_context, user_context - ) - reasons.extend(forced_reasons) - - if forced_decision_variation: - decision = Decision( - experiment, forced_decision_variation, enums.DecisionSources.FEATURE_TEST, None - ) - return { - 'decision': decision, - 'error': False, - 'reasons': reasons - } - - # Get variation through normal bucketing - variation_result = self.get_variation( - project_config, experiment, user_context, user_profile_tracker, reasons, decide_options - ) - cmab_uuid = variation_result['cmab_uuid'] - variation_reasons = variation_result['reasons'] - decision_variation = variation_result['variation'] - error = variation_result['error'] - reasons.extend(variation_reasons) - - # If there's an error, return immediately - if error: - decision = Decision(experiment, None, enums.DecisionSources.FEATURE_TEST, cmab_uuid) - return { - 'decision': decision, - 'error': True, - 'reasons': reasons - } - - # If user is bucketed into a variation, return the decision - if decision_variation: - self.logger.debug( - f'User "{user_id}" ' - f'bucketed into experiment "{experiment.key}" of feature "{feature_flag.key}".' - ) - decision = Decision( - experiment, decision_variation, enums.DecisionSources.FEATURE_TEST, cmab_uuid - ) - return { - 'decision': decision, - 'error': False, - 'reasons': reasons - } - - # Check if the feature flag has a rollout and the user is bucketed into that rollout - rollout_decision, rollout_reasons = self.get_variation_for_rollout( - project_config, feature_flag, user_context - ) - reasons.extend(rollout_reasons) - - if rollout_decision and rollout_decision.variation: - # Check if this was a forced decision (last reason contains "forced decision map") - is_forced_decision = reasons and 'forced decision map' in reasons[-1] if reasons else False - - if not is_forced_decision: - # Only add the "bucketed into rollout" message for normal bucketing - message = ( - f"The user '{user_id}' is bucketed into a rollout " - f"for feature flag '{feature_flag.key}'." - ) - self.logger.info(message) - reasons.append(message) + # If no holdout decision, fall back to existing experiment/rollout logic + # Use get_variations_for_feature_list which handles experiments and rollouts + fallback_result = self.get_variations_for_feature_list( + project_config, [feature_flag], user_context, decide_options + )[0] - return { - 'decision': rollout_decision, - 'error': False, - 'reasons': reasons - } - else: - message = ( - f"The user '{user_id}' is not bucketed into a rollout " - f"for feature flag '{feature_flag.key}'." - ) - self.logger.info(message) - return { - 'decision': Decision(None, None, enums.DecisionSources.ROLLOUT, None), - 'error': False, - 'reasons': reasons - } + # Merge reasons + if fallback_result.get('reasons'): + reasons.extend(fallback_result['reasons']) + + return { + 'decision': fallback_result['decision'], + 'error': fallback_result.get('error', False), + 'reasons': reasons + } def get_variation_for_holdout( self, @@ -905,9 +824,9 @@ def get_variation_for_holdout( self.logger.info(message) decide_reasons.append(message) - # Create Decision for holdout - pass holdout dict as experiment, source is HOLDOUT + # Create Decision for holdout - experiment is None, source is HOLDOUT holdout_decision: Decision = Decision( - experiment=holdout, # type: ignore[arg-type] + experiment=None, variation=variation, source=enums.DecisionSources.HOLDOUT, cmab_uuid=None @@ -1012,24 +931,100 @@ def get_variations_for_feature_list( - 'error': Boolean indicating if an error occurred during the decision process. - 'reasons': List of log messages representing decision making for each feature. """ - ignore_ups = False + decide_reasons: list[str] = [] + if options: ignore_ups = OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE in options + else: + ignore_ups = False user_profile_tracker: Optional[UserProfileTracker] = None if self.user_profile_service is not None and not ignore_ups: - user_id = user_context.user_id - user_profile_tracker = UserProfileTracker(user_id, self.user_profile_service, self.logger) - user_profile_tracker.load_user_profile([], None) + user_profile_tracker = UserProfileTracker(user_context.user_id, self.user_profile_service, self.logger) + user_profile_tracker.load_user_profile(decide_reasons, None) decisions = [] - for feature_flag in features: - decision = self.get_decision_for_flag( - feature_flag, user_context, project_config, options, user_profile_tracker - ) - decisions.append(decision) - if user_profile_tracker: + for feature in features: + feature_reasons = decide_reasons.copy() + experiment_decision_found = False # Track if an experiment decision was made for the feature + + # Check if the feature flag is under an experiment + if feature.experimentIds: + for experiment_id in feature.experimentIds: + experiment = project_config.get_experiment_from_id(experiment_id) + decision_variation: Optional[Union[entities.Variation, VariationDict]] = None + + if experiment: + optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext( + feature.key, experiment.key) + forced_decision_variation, reasons_received = self.validated_forced_decision( + project_config, optimizely_decision_context, user_context) + feature_reasons.extend(reasons_received) + + if forced_decision_variation: + decision_variation = forced_decision_variation + cmab_uuid = None + error = False + else: + variation_result = self.get_variation( + project_config, experiment, user_context, user_profile_tracker, feature_reasons, options + ) + cmab_uuid = variation_result['cmab_uuid'] + variation_reasons = variation_result['reasons'] + decision_variation = variation_result['variation'] + error = variation_result['error'] + feature_reasons.extend(variation_reasons) + + if error: + decision = Decision(experiment, None, enums.DecisionSources.FEATURE_TEST, cmab_uuid) + decision_result: DecisionResult = { + 'decision': decision, + 'error': True, + 'reasons': feature_reasons + } + decisions.append(decision_result) + experiment_decision_found = True + break + + if decision_variation: + self.logger.debug( + f'User "{user_context.user_id}" ' + f'bucketed into experiment "{experiment.key}" of feature "{feature.key}".' + ) + decision = Decision(experiment, decision_variation, + enums.DecisionSources.FEATURE_TEST, cmab_uuid) + decision_result = { + 'decision': decision, + 'error': False, + 'reasons': feature_reasons + } + decisions.append(decision_result) + experiment_decision_found = True # Mark that a decision was found + break # Stop after the first successful experiment decision + + # Only process rollout if no experiment decision was found and no error + if not experiment_decision_found: + rollout_decision, rollout_reasons = self.get_variation_for_rollout(project_config, + feature, + user_context) + if rollout_reasons: + feature_reasons.extend(rollout_reasons) + if rollout_decision: + self.logger.debug(f'User "{user_context.user_id}" ' + f'bucketed into rollout for feature "{feature.key}".') + else: + self.logger.debug(f'User "{user_context.user_id}" ' + f'not bucketed into any rollout for feature "{feature.key}".') + + decision_result = { + 'decision': rollout_decision, + 'error': False, + 'reasons': feature_reasons + } + decisions.append(decision_result) + + if self.user_profile_service is not None and user_profile_tracker is not None and ignore_ups is False: user_profile_tracker.save_user_profile() return decisions diff --git a/optimizely/event/user_event_factory.py b/optimizely/event/user_event_factory.py index d1a7e64a..7a77720f 100644 --- a/optimizely/event/user_event_factory.py +++ b/optimizely/event/user_event_factory.py @@ -67,11 +67,7 @@ def create_impression_event( variation: Optional[Variation] = None experiment_id = None if activated_experiment: - # Handle both Experiment objects and holdout dicts - if isinstance(activated_experiment, dict): - experiment_id = activated_experiment.get('id') - else: - experiment_id = activated_experiment.id + experiment_id = activated_experiment.id if variation_id and flag_key: # need this condition when we send events involving forced decisions diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index c61c8db5..c6c37d5c 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -1252,15 +1252,7 @@ def _create_optimizely_decision( # Create Optimizely Decision Result. attributes = user_context.get_user_attributes() - # Handle both Experiment entities and holdout dicts - if flag_decision.experiment: - if isinstance(flag_decision.experiment, dict): - # Holdout is a dict, not an Experiment entity - rule_key = flag_decision.experiment.get('key') - else: - rule_key = flag_decision.experiment.key - else: - rule_key = None + rule_key = flag_decision.experiment.key if flag_decision.experiment else None all_variables = {} decision_source = flag_decision.source decision_event_dispatched = False diff --git a/tests/test_decision_service.py b/tests/test_decision_service.py index 231a79f9..5fe99ee7 100644 --- a/tests/test_decision_service.py +++ b/tests/test_decision_service.py @@ -1399,8 +1399,8 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_rollout(sel self.project_config, feature, user ) - # Assert info log messages were generated (changed from debug due to new flow through get_decision_for_flag) - self.assertEqual(1, mock_decision_service_logging.info.call_count) + # Assert debug log messages were generated (rollout decisions log at debug level) + self.assertEqual(1, mock_decision_service_logging.debug.call_count) def test_get_variation_for_feature__returns_variation_if_user_not_in_experiment_but_in_rollout( self, diff --git a/tests/test_user_context.py b/tests/test_user_context.py index 1b72e3e3..3ae9be0d 100644 --- a/tests/test_user_context.py +++ b/tests/test_user_context.py @@ -872,8 +872,7 @@ def test_decide__option__include_reasons__feature_rollout(self): 'Evaluating audiences for rule 1: ["11154"].', 'Audiences for rule 1 collectively evaluated to TRUE.', 'User "test_user" meets audience conditions for targeting rule 1.', - 'User "test_user" bucketed into a targeting rule 1.', - "The user 'test_user' is bucketed into a rollout for feature flag 'test_feature_in_rollout'." + 'User "test_user" bucketed into a targeting rule 1.' ] self.assertEqual(expected_reasons, actual.reasons) @@ -1271,8 +1270,7 @@ def test_decide_reasons__hit_everyone_else_rule(self): 'Evaluating audiences for rule Everyone Else: [].', 'Audiences for rule Everyone Else collectively evaluated to TRUE.', 'User "abcde" meets audience conditions for targeting rule Everyone Else.', - 'User "abcde" bucketed into a targeting rule Everyone Else.', - "The user 'abcde' is bucketed into a rollout for feature flag 'test_feature_in_rollout'." + 'User "abcde" bucketed into a targeting rule Everyone Else.' ] self.assertEqual(expected_reasons, actual.reasons) From a20c3a23ce97261642179f84fea60a55ebd12357 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Thu, 11 Dec 2025 12:09:00 -0600 Subject: [PATCH 09/13] Fix FSC --- optimizely/decision_service.py | 35 +++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 78be89d8..723cca46 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -948,6 +948,39 @@ def get_variations_for_feature_list( for feature in features: feature_reasons = decide_reasons.copy() experiment_decision_found = False # Track if an experiment decision was made for the feature + holdout_decision_found = False # Track if a holdout decision was made for the feature + user_id = user_context.user_id + + # Check holdouts first (they take precedence over experiments and rollouts) + holdouts = project_config.get_holdouts_for_flag(feature.key) + for holdout in holdouts: + holdout_decision_result = self.get_variation_for_holdout(holdout, user_context, project_config) + feature_reasons.extend(holdout_decision_result['reasons']) + + decision = holdout_decision_result['decision'] + # Check if user was bucketed into holdout (has a variation) + if decision.variation is None: + continue + + message = ( + f"The user '{user_id}' is bucketed into holdout '{holdout['key']}' " + f"for feature flag '{feature.key}'." + ) + self.logger.info(message) + feature_reasons.append(message) + + decision_result: DecisionResult = { + 'decision': holdout_decision_result['decision'], + 'error': False, + 'reasons': feature_reasons + } + decisions.append(decision_result) + holdout_decision_found = True + break + + # If holdout decision found, skip experiment and rollout evaluation + if holdout_decision_found: + continue # Check if the feature flag is under an experiment if feature.experimentIds: @@ -978,7 +1011,7 @@ def get_variations_for_feature_list( if error: decision = Decision(experiment, None, enums.DecisionSources.FEATURE_TEST, cmab_uuid) - decision_result: DecisionResult = { + decision_result = { 'decision': decision, 'error': True, 'reasons': feature_reasons From d6329afb5c9809bcbef5a68db902d6740fada261 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Thu, 11 Dec 2025 12:21:25 -0600 Subject: [PATCH 10/13] Fix FSC holdout test cases --- optimizely/decision_service.py | 4 ++-- optimizely/event/user_event_factory.py | 4 +++- optimizely/optimizely.py | 15 +++++++++++---- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 723cca46..413dece4 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -824,9 +824,9 @@ def get_variation_for_holdout( self.logger.info(message) decide_reasons.append(message) - # Create Decision for holdout - experiment is None, source is HOLDOUT + # Create Decision for holdout - pass holdout dict as experiment so rule_key can be extracted holdout_decision: Decision = Decision( - experiment=None, + experiment=holdout, # type: ignore[arg-type] variation=variation, source=enums.DecisionSources.HOLDOUT, cmab_uuid=None diff --git a/optimizely/event/user_event_factory.py b/optimizely/event/user_event_factory.py index 7a77720f..1816da06 100644 --- a/optimizely/event/user_event_factory.py +++ b/optimizely/event/user_event_factory.py @@ -67,7 +67,9 @@ def create_impression_event( variation: Optional[Variation] = None experiment_id = None if activated_experiment: - experiment_id = activated_experiment.id + # For holdouts, activated_experiment is a dict; for experiments, it's an Experiment entity + experiment_id = (activated_experiment['id'] if isinstance(activated_experiment, dict) + else activated_experiment.id) if variation_id and flag_key: # need this condition when we send events involving forced decisions diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index c6c37d5c..e44b3205 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -1252,7 +1252,12 @@ def _create_optimizely_decision( # Create Optimizely Decision Result. attributes = user_context.get_user_attributes() - rule_key = flag_decision.experiment.key if flag_decision.experiment else None + # For holdouts, experiment is a dict; for experiments, it's an Experiment entity + if flag_decision.experiment: + rule_key = (flag_decision.experiment['key'] if isinstance(flag_decision.experiment, dict) + else flag_decision.experiment.key) + else: + rule_key = None all_variables = {} decision_source = flag_decision.source decision_event_dispatched = False @@ -1303,9 +1308,11 @@ def _create_optimizely_decision( try: if flag_decision.experiment is not None: - experiment_id = flag_decision.experiment.id - except AttributeError: - self.logger.warning("flag_decision.experiment has no attribute 'id'") + # For holdouts, experiment is a dict; for experiments, it's an Experiment entity + experiment_id = (flag_decision.experiment['id'] if isinstance(flag_decision.experiment, dict) + else flag_decision.experiment.id) + except (AttributeError, KeyError, TypeError): + self.logger.warning("Unable to extract experiment_id from flag_decision.experiment") try: if flag_decision.variation is not None: From 129c049ad239a73a47b89e1d631e1d1281c85078 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Thu, 11 Dec 2025 13:23:04 -0600 Subject: [PATCH 11/13] Please pass FSC --- optimizely/optimizely.py | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index e44b3205..650221e6 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -454,8 +454,12 @@ def _get_feature_variable_for_type( ) if decision.source in (enums.DecisionSources.FEATURE_TEST, enums.DecisionSources.HOLDOUT): + experiment_key = None + if decision.experiment: + experiment_key = (decision.experiment['key'] if isinstance(decision.experiment, dict) + else decision.experiment.key) source_info = { - 'experiment_key': decision.experiment.key if decision.experiment else None, + 'experiment_key': experiment_key, 'variation_key': self._get_variation_key(decision.variation), } @@ -558,8 +562,12 @@ def _get_all_feature_variables_for_type( all_variables[variable_key] = actual_value if decision.source == enums.DecisionSources.FEATURE_TEST: + experiment_key = None + if decision.experiment: + experiment_key = (decision.experiment['key'] if isinstance(decision.experiment, dict) + else decision.experiment.key) source_info = { - 'experiment_key': decision.experiment.key if decision.experiment else None, + 'experiment_key': experiment_key, 'variation_key': self._get_variation_key(decision.variation), } @@ -802,19 +810,25 @@ def is_feature_enabled(self, feature_key: str, user_id: str, attributes: Optiona feature_enabled = True if (is_source_rollout or not decision.variation) and project_config.get_send_flag_decisions_value(): + experiment_key = '' + if decision.experiment: + experiment_key = (decision.experiment['key'] if isinstance(decision.experiment, dict) + else decision.experiment.key) self._send_impression_event( - project_config, decision.experiment, decision.variation, feature.key, decision.experiment.key if - decision.experiment else '', str(decision.source), feature_enabled, user_id, attributes, cmab_uuid + project_config, decision.experiment, decision.variation, feature.key, experiment_key, + str(decision.source), feature_enabled, user_id, attributes, cmab_uuid ) # Send event if Decision came from an experiment. if is_source_experiment and decision.variation and decision.experiment: + experiment_key = (decision.experiment['key'] if isinstance(decision.experiment, dict) + else decision.experiment.key) source_info = { - 'experiment_key': decision.experiment.key, + 'experiment_key': experiment_key, 'variation_key': self._get_variation_key(decision.variation), } self._send_impression_event( - project_config, decision.experiment, decision.variation, feature.key, decision.experiment.key, + project_config, decision.experiment, decision.variation, feature.key, experiment_key, str(decision.source), feature_enabled, user_id, attributes, cmab_uuid ) From 9422468ce7a92cf0b62acba4e6b5106bfaa9f1de Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Thu, 11 Dec 2025 14:55:55 -0600 Subject: [PATCH 12/13] Implement holdout correctly and fix tests --- optimizely/decision_service.py | 207 +++++++++++++-------------------- tests/test_decision_service.py | 4 +- tests/test_user_context.py | 6 +- 3 files changed, 88 insertions(+), 129 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 413dece4..56b439ff 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -707,7 +707,7 @@ def get_decision_for_flag( reasons = decide_reasons.copy() if decide_reasons else [] user_id = user_context.user_id - # Check holdouts + # Check holdouts first (they take precedence) holdouts = project_config.get_holdouts_for_flag(feature_flag.key) for holdout in holdouts: holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config) @@ -730,21 +730,84 @@ def get_decision_for_flag( 'reasons': reasons } - # If no holdout decision, fall back to existing experiment/rollout logic - # Use get_variations_for_feature_list which handles experiments and rollouts - fallback_result = self.get_variations_for_feature_list( - project_config, [feature_flag], user_context, decide_options - )[0] - - # Merge reasons - if fallback_result.get('reasons'): - reasons.extend(fallback_result['reasons']) + # Check if the feature flag is under an experiment + if feature_flag.experimentIds: + for experiment_id in feature_flag.experimentIds: + experiment = project_config.get_experiment_from_id(experiment_id) + decision_variation: Optional[Union[entities.Variation, VariationDict]] = None + + if experiment: + optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext( + feature_flag.key, experiment.key) + forced_decision_variation, reasons_received = self.validated_forced_decision( + project_config, optimizely_decision_context, user_context) + reasons.extend(reasons_received) + + if forced_decision_variation: + decision_variation = forced_decision_variation + cmab_uuid = None + error = False + else: + variation_result = self.get_variation( + project_config, experiment, user_context, user_profile_tracker, reasons, decide_options + ) + cmab_uuid = variation_result['cmab_uuid'] + variation_reasons = variation_result['reasons'] + decision_variation = variation_result['variation'] + error = variation_result['error'] + reasons.extend(variation_reasons) + + if error: + # If there's an error (e.g., CMAB error), return immediately without falling back to rollout + decision = Decision(experiment, None, enums.DecisionSources.FEATURE_TEST, cmab_uuid) + return { + 'decision': decision, + 'error': True, + 'reasons': reasons + } + + if decision_variation: + self.logger.debug( + f'User "{user_context.user_id}" ' + f'bucketed into experiment "{experiment.key}" of feature "{feature_flag.key}".' + ) + decision = Decision(experiment, decision_variation, + enums.DecisionSources.FEATURE_TEST, cmab_uuid) + return { + 'decision': decision, + 'error': False, + 'reasons': reasons + } + + # Fall back to rollout + rollout_decision, rollout_reasons = self.get_variation_for_rollout(project_config, + feature_flag, + user_context) + reasons.extend(rollout_reasons) + + if rollout_decision and rollout_decision.variation: + # Check if this was a forced decision (last reason contains "forced decision map") + is_forced_decision = reasons and 'forced decision map' in reasons[-1] if reasons else False + + if not is_forced_decision: + # Only add the "bucketed into rollout" message for normal bucketing + message = f"The user '{user_id}' is bucketed into a rollout for feature flag '{feature_flag.key}'." + self.logger.info(message) + reasons.append(message) - return { - 'decision': fallback_result['decision'], - 'error': fallback_result.get('error', False), - 'reasons': reasons - } + return { + 'decision': rollout_decision, + 'error': False, + 'reasons': reasons + } + else: + message = f"The user '{user_id}' is not bucketed into a rollout for feature flag '{feature_flag.key}'." + self.logger.info(message) + return { + 'decision': Decision(None, None, enums.DecisionSources.ROLLOUT, None), + 'error': False, + 'reasons': reasons + } def get_variation_for_holdout( self, @@ -946,116 +1009,10 @@ def get_variations_for_feature_list( decisions = [] for feature in features: - feature_reasons = decide_reasons.copy() - experiment_decision_found = False # Track if an experiment decision was made for the feature - holdout_decision_found = False # Track if a holdout decision was made for the feature - user_id = user_context.user_id - - # Check holdouts first (they take precedence over experiments and rollouts) - holdouts = project_config.get_holdouts_for_flag(feature.key) - for holdout in holdouts: - holdout_decision_result = self.get_variation_for_holdout(holdout, user_context, project_config) - feature_reasons.extend(holdout_decision_result['reasons']) - - decision = holdout_decision_result['decision'] - # Check if user was bucketed into holdout (has a variation) - if decision.variation is None: - continue - - message = ( - f"The user '{user_id}' is bucketed into holdout '{holdout['key']}' " - f"for feature flag '{feature.key}'." - ) - self.logger.info(message) - feature_reasons.append(message) - - decision_result: DecisionResult = { - 'decision': holdout_decision_result['decision'], - 'error': False, - 'reasons': feature_reasons - } - decisions.append(decision_result) - holdout_decision_found = True - break - - # If holdout decision found, skip experiment and rollout evaluation - if holdout_decision_found: - continue - - # Check if the feature flag is under an experiment - if feature.experimentIds: - for experiment_id in feature.experimentIds: - experiment = project_config.get_experiment_from_id(experiment_id) - decision_variation: Optional[Union[entities.Variation, VariationDict]] = None - - if experiment: - optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext( - feature.key, experiment.key) - forced_decision_variation, reasons_received = self.validated_forced_decision( - project_config, optimizely_decision_context, user_context) - feature_reasons.extend(reasons_received) - - if forced_decision_variation: - decision_variation = forced_decision_variation - cmab_uuid = None - error = False - else: - variation_result = self.get_variation( - project_config, experiment, user_context, user_profile_tracker, feature_reasons, options - ) - cmab_uuid = variation_result['cmab_uuid'] - variation_reasons = variation_result['reasons'] - decision_variation = variation_result['variation'] - error = variation_result['error'] - feature_reasons.extend(variation_reasons) - - if error: - decision = Decision(experiment, None, enums.DecisionSources.FEATURE_TEST, cmab_uuid) - decision_result = { - 'decision': decision, - 'error': True, - 'reasons': feature_reasons - } - decisions.append(decision_result) - experiment_decision_found = True - break - - if decision_variation: - self.logger.debug( - f'User "{user_context.user_id}" ' - f'bucketed into experiment "{experiment.key}" of feature "{feature.key}".' - ) - decision = Decision(experiment, decision_variation, - enums.DecisionSources.FEATURE_TEST, cmab_uuid) - decision_result = { - 'decision': decision, - 'error': False, - 'reasons': feature_reasons - } - decisions.append(decision_result) - experiment_decision_found = True # Mark that a decision was found - break # Stop after the first successful experiment decision - - # Only process rollout if no experiment decision was found and no error - if not experiment_decision_found: - rollout_decision, rollout_reasons = self.get_variation_for_rollout(project_config, - feature, - user_context) - if rollout_reasons: - feature_reasons.extend(rollout_reasons) - if rollout_decision: - self.logger.debug(f'User "{user_context.user_id}" ' - f'bucketed into rollout for feature "{feature.key}".') - else: - self.logger.debug(f'User "{user_context.user_id}" ' - f'not bucketed into any rollout for feature "{feature.key}".') - - decision_result = { - 'decision': rollout_decision, - 'error': False, - 'reasons': feature_reasons - } - decisions.append(decision_result) + decision = self.get_decision_for_flag( + feature, user_context, project_config, options, user_profile_tracker, decide_reasons + ) + decisions.append(decision) if self.user_profile_service is not None and user_profile_tracker is not None and ignore_ups is False: user_profile_tracker.save_user_profile() diff --git a/tests/test_decision_service.py b/tests/test_decision_service.py index 5fe99ee7..dd183312 100644 --- a/tests/test_decision_service.py +++ b/tests/test_decision_service.py @@ -1399,8 +1399,8 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_rollout(sel self.project_config, feature, user ) - # Assert debug log messages were generated (rollout decisions log at debug level) - self.assertEqual(1, mock_decision_service_logging.debug.call_count) + # Assert info log message was generated for rollout bucketing + self.assertEqual(1, mock_decision_service_logging.info.call_count) def test_get_variation_for_feature__returns_variation_if_user_not_in_experiment_but_in_rollout( self, diff --git a/tests/test_user_context.py b/tests/test_user_context.py index 3ae9be0d..1b72e3e3 100644 --- a/tests/test_user_context.py +++ b/tests/test_user_context.py @@ -872,7 +872,8 @@ def test_decide__option__include_reasons__feature_rollout(self): 'Evaluating audiences for rule 1: ["11154"].', 'Audiences for rule 1 collectively evaluated to TRUE.', 'User "test_user" meets audience conditions for targeting rule 1.', - 'User "test_user" bucketed into a targeting rule 1.' + 'User "test_user" bucketed into a targeting rule 1.', + "The user 'test_user' is bucketed into a rollout for feature flag 'test_feature_in_rollout'." ] self.assertEqual(expected_reasons, actual.reasons) @@ -1270,7 +1271,8 @@ def test_decide_reasons__hit_everyone_else_rule(self): 'Evaluating audiences for rule Everyone Else: [].', 'Audiences for rule Everyone Else collectively evaluated to TRUE.', 'User "abcde" meets audience conditions for targeting rule Everyone Else.', - 'User "abcde" bucketed into a targeting rule Everyone Else.' + 'User "abcde" bucketed into a targeting rule Everyone Else.', + "The user 'abcde' is bucketed into a rollout for feature flag 'test_feature_in_rollout'." ] self.assertEqual(expected_reasons, actual.reasons) From 35f074189a1790c07752cbd474d53a086a31cff0 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Mon, 15 Dec 2025 12:32:06 -0600 Subject: [PATCH 13/13] Implement changes from Muzahid PR --- optimizely/bucketer.py | 111 ++--- optimizely/decision_service.py | 211 +++++---- optimizely/entities.py | 76 ++++ optimizely/event/event_factory.py | 3 + optimizely/event/user_event.py | 7 +- optimizely/event/user_event_factory.py | 15 +- optimizely/optimizely.py | 53 +-- optimizely/project_config.py | 198 ++++---- tests/test_config.py | 15 +- tests/test_decision_service.py | 15 +- tests/test_decision_service_holdout.py | 601 ++++++++++++++++++++++++- tests/test_user_context.py | 6 +- 12 files changed, 960 insertions(+), 351 deletions(-) diff --git a/optimizely/bucketer.py b/optimizely/bucketer.py index ca431012..d2defcab 100644 --- a/optimizely/bucketer.py +++ b/optimizely/bucketer.py @@ -28,8 +28,8 @@ if TYPE_CHECKING: # prevent circular dependenacy by skipping import at runtime from .project_config import ProjectConfig - from .entities import Experiment, Variation - from .helpers.types import TrafficAllocation + from .entities import Experiment, Variation, Holdout + from .helpers.types import TrafficAllocation, VariationDict MAX_TRAFFIC_VALUE: Final = 10000 @@ -104,8 +104,8 @@ def find_bucket( def bucket( self, project_config: ProjectConfig, - experiment: Experiment, user_id: str, bucketing_id: str - ) -> tuple[Optional[Variation], list[str]]: + experiment: Experiment | Holdout, user_id: str, bucketing_id: str + ) -> tuple[Optional[Variation | VariationDict], list[str]]: """ For a given experiment and bucketing ID determines variation to be shown to user. Args: @@ -125,14 +125,9 @@ def bucket( project_config.logger.debug(message) return None, [] - if isinstance(experiment, dict): - # This is a holdout dictionary - experiment_key = experiment.get('key', '') - experiment_id = experiment.get('id', '') - else: - # This is an Experiment object - experiment_key = experiment.key - experiment_id = experiment.id + # Handle both Experiment and Holdout entities + experiment_key = experiment.key + experiment_id = experiment.id if not experiment_key or not experiment_key.strip(): message = 'Invalid entity key provided for bucketing. Returning nil.' @@ -141,13 +136,7 @@ def bucket( variation_id, decide_reasons = self.bucket_to_entity_id(project_config, experiment, user_id, bucketing_id) if variation_id: - if isinstance(experiment, dict): - # For holdouts, find the variation in the holdout's variations array - variations = experiment.get('variations', []) - variation = next((v for v in variations if v.get('id') == variation_id), None) - else: - # For experiments, use the existing method - variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) + variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) return variation, decide_reasons # No variation found - log message for empty traffic range @@ -158,7 +147,7 @@ def bucket( def bucket_to_entity_id( self, project_config: ProjectConfig, - experiment: Experiment, user_id: str, bucketing_id: str + experiment: Experiment | Holdout, user_id: str, bucketing_id: str ) -> tuple[Optional[str], list[str]]: """ For a given experiment and bucketing ID determines variation ID to be shown to user. @@ -176,58 +165,52 @@ def bucket_to_entity_id( if not experiment: return None, decide_reasons - # Handle both Experiment objects and holdout dictionaries - if isinstance(experiment, dict): - # This is a holdout dictionary - holdouts don't have groups - experiment_key = experiment.get('key', '') - experiment_id = experiment.get('id', '') - traffic_allocations = experiment.get('trafficAllocation', []) - has_cmab = False - group_policy = None - else: - # This is an Experiment object - experiment_key = experiment.key - experiment_id = experiment.id - traffic_allocations = experiment.trafficAllocation - has_cmab = bool(experiment.cmab) + # Handle both Experiment and Holdout entities + # Both entities have key, id, and trafficAllocation attributes + from . import entities + + experiment_key = experiment.key + experiment_id = experiment.id + traffic_allocations = experiment.trafficAllocation + + # Determine if experiment is in a mutually exclusive group + # Holdouts don't have groupId or groupPolicy - use isinstance for type narrowing + if isinstance(experiment, entities.Experiment): group_policy = getattr(experiment, 'groupPolicy', None) + if group_policy and group_policy in GROUP_POLICIES: + group = project_config.get_group(experiment.groupId) - # Determine if experiment is in a mutually exclusive group. - # This will not affect evaluation of rollout rules or holdouts. - if group_policy and group_policy in GROUP_POLICIES: - group = project_config.get_group(experiment.groupId) + if not group: + return None, decide_reasons - if not group: - return None, decide_reasons + user_experiment_id = self.find_bucket( + project_config, bucketing_id, experiment.groupId, group.trafficAllocation, + ) - user_experiment_id = self.find_bucket( - project_config, bucketing_id, experiment.groupId, group.trafficAllocation, - ) + if not user_experiment_id: + message = f'User "{user_id}" is in no experiment.' + project_config.logger.info(message) + decide_reasons.append(message) + return None, decide_reasons - if not user_experiment_id: - message = f'User "{user_id}" is in no experiment.' - project_config.logger.info(message) - decide_reasons.append(message) - return None, decide_reasons + if user_experiment_id != experiment_id: + message = f'User "{user_id}" is not in experiment "{experiment_key}" of group {experiment.groupId}.' + project_config.logger.info(message) + decide_reasons.append(message) + return None, decide_reasons - if user_experiment_id != experiment_id: - message = f'User "{user_id}" is not in experiment "{experiment_key}" of group {experiment.groupId}.' + message = f'User "{user_id}" is in experiment {experiment_key} of group {experiment.groupId}.' project_config.logger.info(message) decide_reasons.append(message) - return None, decide_reasons - - message = f'User "{user_id}" is in experiment {experiment_key} of group {experiment.groupId}.' - project_config.logger.info(message) - decide_reasons.append(message) - - if has_cmab: - if experiment.cmab: - traffic_allocations = [ - { - "entityId": "$", - "endOfRange": experiment.cmab['trafficAllocation'] - } - ] + + # Holdouts don't have cmab - use isinstance for type narrowing + if isinstance(experiment, entities.Experiment) and experiment.cmab: + traffic_allocations = [ + { + "entityId": "$", + "endOfRange": experiment.cmab['trafficAllocation'] + } + ] # Bucket user if not in white-list and in group (if any) variation_id = self.find_bucket(project_config, bucketing_id, diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 56b439ff..28275ef6 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -14,7 +14,7 @@ from __future__ import annotations from typing import TYPE_CHECKING, NamedTuple, Optional, Sequence, List, TypedDict, Union -from optimizely.helpers.types import HoldoutDict, VariationDict +from optimizely.helpers.types import VariationDict from . import bucketer from . import entities @@ -81,7 +81,7 @@ class DecisionResult(TypedDict): class Decision(NamedTuple): """Named tuple containing selected experiment, variation, source and cmab_uuid. None if no experiment/variation was selected.""" - experiment: Optional[entities.Experiment] + experiment: Optional[Union[entities.Experiment, entities.Holdout]] variation: Optional[Union[entities.Variation, VariationDict]] source: Optional[str] cmab_uuid: Optional[str] @@ -246,7 +246,11 @@ def set_forced_variation( # The invalid variation key will be logged inside this call. return False - variation_id = forced_variation.id + # Handle both Variation entity and VariationDict + if isinstance(forced_variation, dict): + variation_id = forced_variation['id'] + else: + variation_id = forced_variation.id if user_id not in self.forced_variation_map: self.forced_variation_map[user_id] = {experiment_id: variation_id} @@ -261,7 +265,7 @@ def set_forced_variation( def get_forced_variation( self, project_config: ProjectConfig, experiment_key: str, user_id: str - ) -> tuple[Optional[entities.Variation], list[str]]: + ) -> tuple[Optional[Union[entities.Variation, VariationDict]], list[str]]: """ Gets the forced variation key for the given user and experiment. Args: @@ -302,7 +306,9 @@ def get_forced_variation( if variation is None: return None, decide_reasons - message = f'Variation "{variation.key}" is mapped to experiment "{experiment_key}" and ' \ + # Handle both Variation entity and VariationDict + var_key = variation['key'] if isinstance(variation, dict) else variation.key + message = f'Variation "{var_key}" is mapped to experiment "{experiment_key}" and ' \ f'user "{user_id}" in the forced variation map' self.logger.debug(message) decide_reasons.append(message) @@ -310,7 +316,7 @@ def get_forced_variation( def get_whitelisted_variation( self, project_config: ProjectConfig, experiment: entities.Experiment, user_id: str - ) -> tuple[Optional[entities.Variation], list[str]]: + ) -> tuple[Optional[Union[entities.Variation, VariationDict]], list[str]]: """ Determine if a user is forced into a variation (through whitelisting) for the given experiment and return that variation. @@ -341,7 +347,7 @@ def get_whitelisted_variation( def get_stored_variation( self, project_config: ProjectConfig, experiment: entities.Experiment, user_profile: UserProfile - ) -> Optional[entities.Variation]: + ) -> Optional[Union[entities.Variation, VariationDict]]: """ Determine if the user has a stored variation available for the given experiment and return that. Args: @@ -358,8 +364,10 @@ def get_stored_variation( if variation_id: variation = project_config.get_variation_from_id(experiment.key, variation_id) if variation: + # Handle both Variation entity and VariationDict + var_key = variation['key'] if isinstance(variation, dict) else variation.key message = f'Found a stored decision. User "{user_id}" is in ' \ - f'variation "{variation.key}" of experiment "{experiment.key}".' + f'variation "{var_key}" of experiment "{experiment.key}".' self.logger.info(message) return variation @@ -426,7 +434,7 @@ def get_variation( } # Check if the user is forced into a variation - variation: Optional[entities.Variation] + variation: Optional[Union[entities.Variation, VariationDict]] variation, reasons_received = self.get_forced_variation(project_config, experiment.key, user_id) decide_reasons += reasons_received if variation: @@ -672,13 +680,26 @@ def get_variation_for_feature( - 'error': Boolean indicating if an error occurred during the decision process. - 'reasons': List of log messages representing decision making for the feature. """ - holdouts = project_config.get_holdouts_for_flag(feature.key) - - if holdouts: - # Has holdouts - use get_decision_for_flag which checks holdouts first - return self.get_decision_for_flag(feature, user_context, project_config, options) + # Check if user profile service should be ignored + if options: + ignore_ups = OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE in options else: - return self.get_variations_for_feature_list(project_config, [feature], user_context, options)[0] + ignore_ups = False + + # Create user profile tracker for sticky bucketing + user_profile_tracker: Optional[UserProfileTracker] = None + if self.user_profile_service is not None and not ignore_ups: + user_profile_tracker = UserProfileTracker(user_context.user_id, self.user_profile_service, self.logger) + # Load user profile once before processing + user_profile_tracker.load_user_profile([], None) + + result = self.get_decision_for_flag(feature, user_context, project_config, options, user_profile_tracker) + + # Save user profile after decision + if user_profile_tracker is not None and not ignore_ups: + user_profile_tracker.save_user_profile() + + return result def get_decision_for_flag( self, @@ -707,7 +728,7 @@ def get_decision_for_flag( reasons = decide_reasons.copy() if decide_reasons else [] user_id = user_context.user_id - # Check holdouts first (they take precedence) + # Check holdouts holdouts = project_config.get_holdouts_for_flag(feature_flag.key) for holdout in holdouts: holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config) @@ -719,7 +740,7 @@ def get_decision_for_flag( continue message = ( - f"The user '{user_id}' is bucketed into holdout '{holdout['key']}' " + f"The user '{user_id}' is bucketed into holdout '{holdout.key}' " f"for feature flag '{feature_flag.key}'." ) self.logger.info(message) @@ -730,88 +751,82 @@ def get_decision_for_flag( 'reasons': reasons } - # Check if the feature flag is under an experiment + # If no holdout decision, check experiments then rollouts if feature_flag.experimentIds: for experiment_id in feature_flag.experimentIds: experiment = project_config.get_experiment_from_id(experiment_id) - decision_variation: Optional[Union[entities.Variation, VariationDict]] = None if experiment: + # Check for forced decision optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext( feature_flag.key, experiment.key) - forced_decision_variation, reasons_received = self.validated_forced_decision( + forced_decision_variation, forced_reasons = self.validated_forced_decision( project_config, optimizely_decision_context, user_context) - reasons.extend(reasons_received) + reasons.extend(forced_reasons) if forced_decision_variation: - decision_variation = forced_decision_variation - cmab_uuid = None - error = False - else: - variation_result = self.get_variation( - project_config, experiment, user_context, user_profile_tracker, reasons, decide_options - ) - cmab_uuid = variation_result['cmab_uuid'] - variation_reasons = variation_result['reasons'] - decision_variation = variation_result['variation'] - error = variation_result['error'] - reasons.extend(variation_reasons) - - if error: - # If there's an error (e.g., CMAB error), return immediately without falling back to rollout - decision = Decision(experiment, None, enums.DecisionSources.FEATURE_TEST, cmab_uuid) + decision = Decision(experiment, forced_decision_variation, + enums.DecisionSources.FEATURE_TEST, None) + return { + 'decision': decision, + 'error': False, + 'reasons': reasons + } + + # Get variation for experiment + variation_result = self.get_variation( + project_config, experiment, user_context, user_profile_tracker, reasons, decide_options + ) + reasons.extend(variation_result['reasons']) + + if variation_result['error']: + decision = Decision(experiment, None, enums.DecisionSources.FEATURE_TEST, + variation_result['cmab_uuid']) return { 'decision': decision, 'error': True, 'reasons': reasons } - if decision_variation: - self.logger.debug( - f'User "{user_context.user_id}" ' - f'bucketed into experiment "{experiment.key}" of feature "{feature_flag.key}".' - ) - decision = Decision(experiment, decision_variation, - enums.DecisionSources.FEATURE_TEST, cmab_uuid) + if variation_result['variation']: + decision = Decision(experiment, variation_result['variation'], + enums.DecisionSources.FEATURE_TEST, + variation_result['cmab_uuid']) return { 'decision': decision, 'error': False, 'reasons': reasons } - # Fall back to rollout - rollout_decision, rollout_reasons = self.get_variation_for_rollout(project_config, - feature_flag, - user_context) - reasons.extend(rollout_reasons) - - if rollout_decision and rollout_decision.variation: - # Check if this was a forced decision (last reason contains "forced decision map") - is_forced_decision = reasons and 'forced decision map' in reasons[-1] if reasons else False + # If no experiment decision, check rollouts + rollout_decision, rollout_reasons = self.get_variation_for_rollout( + project_config, feature_flag, user_context + ) + if rollout_reasons: + reasons.extend(rollout_reasons) - if not is_forced_decision: - # Only add the "bucketed into rollout" message for normal bucketing - message = f"The user '{user_id}' is bucketed into a rollout for feature flag '{feature_flag.key}'." - self.logger.info(message) - reasons.append(message) + # Log rollout decision for backward compatibility with tests + has_variation = False + if isinstance(rollout_decision, Decision): + has_variation = rollout_decision.variation is not None + else: + # Handle mocked return values in tests + has_variation = rollout_decision is not None - return { - 'decision': rollout_decision, - 'error': False, - 'reasons': reasons - } + if has_variation: + self.logger.debug(f'User "{user_id}" bucketed into rollout for feature "{feature_flag.key}".') else: - message = f"The user '{user_id}' is not bucketed into a rollout for feature flag '{feature_flag.key}'." - self.logger.info(message) - return { - 'decision': Decision(None, None, enums.DecisionSources.ROLLOUT, None), - 'error': False, - 'reasons': reasons - } + self.logger.debug(f'User "{user_id}" not bucketed into any rollout for feature "{feature_flag.key}".') + + return { + 'decision': rollout_decision, + 'error': False, + 'reasons': reasons + } def get_variation_for_holdout( self, - holdout: HoldoutDict, + holdout: entities.Holdout, user_context: OptimizelyUserContext, project_config: ProjectConfig ) -> DecisionResult: @@ -819,7 +834,7 @@ def get_variation_for_holdout( Get the variation for holdout. Args: - holdout: The holdout configuration (HoldoutDict). + holdout: The holdout configuration (Holdout entity). user_context: The user context. project_config: The project config. @@ -832,9 +847,9 @@ def get_variation_for_holdout( user_id = user_context.user_id attributes = user_context.get_user_attributes() - if not holdout or not holdout.get('status') or holdout.get('status') != 'Running': - key = holdout.get('key') if holdout else 'unknown' - message = f"Holdout '{key}' is not running." + # Check if holdout is activated (Running status) + if not holdout.is_activated: + message = f"Holdout '{holdout.key}' is not running." self.logger.info(message) decide_reasons.append(message) return { @@ -846,13 +861,13 @@ def get_variation_for_holdout( bucketing_id, bucketing_id_reasons = self._get_bucketing_id(user_id, attributes) decide_reasons.extend(bucketing_id_reasons) - # Check audience conditions - audience_conditions = holdout.get('audienceIds') + # Check audience conditions using the same method as experiments + audience_conditions = holdout.get_audience_conditions_or_ids() user_meets_audience_conditions, reasons_received = audience_helper.does_user_meet_audience_conditions( project_config, audience_conditions, ExperimentAudienceEvaluationLogs, - holdout.get('key', 'unknown'), + holdout.key, user_context, self.logger ) @@ -861,7 +876,7 @@ def get_variation_for_holdout( if not user_meets_audience_conditions: message = ( f"User '{user_id}' does not meet the conditions for holdout " - f"'{holdout['key']}'." + f"'{holdout.key}'." ) self.logger.debug(message) decide_reasons.append(message) @@ -873,23 +888,21 @@ def get_variation_for_holdout( # Bucket user into holdout variation variation, bucket_reasons = self.bucketer.bucket( - project_config, holdout, user_id, bucketing_id # type: ignore[arg-type] + project_config, holdout, user_id, bucketing_id ) decide_reasons.extend(bucket_reasons) if variation: - # For holdouts, variation is a dict, not a Variation entity - variation_key = variation['key'] if isinstance(variation, dict) else variation.key + variation_key = variation.get('key') if isinstance(variation, dict) else variation.key message = ( f"The user '{user_id}' is bucketed into variation '{variation_key}' " - f"of holdout '{holdout['key']}'." + f"of holdout '{holdout.key}'." ) self.logger.info(message) decide_reasons.append(message) - # Create Decision for holdout - pass holdout dict as experiment so rule_key can be extracted holdout_decision: Decision = Decision( - experiment=holdout, # type: ignore[arg-type] + experiment=holdout, variation=variation, source=enums.DecisionSources.HOLDOUT, cmab_uuid=None @@ -900,7 +913,7 @@ def get_variation_for_holdout( 'reasons': decide_reasons } - message = f"User '{user_id}' is not bucketed into any variation for holdout '{holdout['key']}'." + message = f"User '{user_id}' is not bucketed into any variation for holdout '{holdout.key}'." self.logger.info(message) decide_reasons.append(message) return { @@ -994,27 +1007,37 @@ def get_variations_for_feature_list( - 'error': Boolean indicating if an error occurred during the decision process. - 'reasons': List of log messages representing decision making for each feature. """ - decide_reasons: list[str] = [] + user_id = user_context.user_id + # Check if user profile service should be ignored if options: ignore_ups = OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE in options else: ignore_ups = False + # Create user profile tracker once for all features user_profile_tracker: Optional[UserProfileTracker] = None if self.user_profile_service is not None and not ignore_ups: - user_profile_tracker = UserProfileTracker(user_context.user_id, self.user_profile_service, self.logger) - user_profile_tracker.load_user_profile(decide_reasons, None) + user_profile_tracker = UserProfileTracker(user_id, self.user_profile_service, self.logger) + # Load user profile once before processing features + user_profile_tracker.load_user_profile([], None) - decisions = [] + # Process each feature by delegating to get_decision_for_flag + decisions: list[DecisionResult] = [] for feature in features: - decision = self.get_decision_for_flag( - feature, user_context, project_config, options, user_profile_tracker, decide_reasons + flag_decision_result = self.get_decision_for_flag( + feature_flag=feature, + user_context=user_context, + project_config=project_config, + decide_options=options, + user_profile_tracker=user_profile_tracker, + decide_reasons=None ) - decisions.append(decision) + decisions.append(flag_decision_result) - if self.user_profile_service is not None and user_profile_tracker is not None and ignore_ups is False: + # Save user profile once after all features processed + if self.user_profile_service is not None and user_profile_tracker is not None and not ignore_ups: user_profile_tracker.save_user_profile() return decisions diff --git a/optimizely/entities.py b/optimizely/entities.py index 83488bdf..321ec441 100644 --- a/optimizely/entities.py +++ b/optimizely/entities.py @@ -27,6 +27,8 @@ class BaseEntity: def __eq__(self, other: object) -> bool: + if not hasattr(other, '__dict__'): + return False return self.__dict__ == other.__dict__ @@ -194,3 +196,77 @@ def __init__(self, key: str, host: Optional[str] = None, publicKey: Optional[str self.key = key self.host = host self.publicKey = publicKey + + +class Holdout(BaseEntity): + """Holdout entity representing a holdout experiment for measuring incremental impact. + + Aligned with Swift SDK implementation where Holdout is a proper entity class + that conforms to ExperimentCore protocol. + """ + + class Status: + """Holdout status constants matching Swift enum Status.""" + DRAFT: Final = 'Draft' + RUNNING: Final = 'Running' + CONCLUDED: Final = 'Concluded' + ARCHIVED: Final = 'Archived' + + def __init__( + self, + id: str, + key: str, + status: str, + variations: list[VariationDict], + trafficAllocation: list[TrafficAllocation], + audienceIds: list[str], + includedFlags: Optional[list[str]] = None, + excludedFlags: Optional[list[str]] = None, + audienceConditions: Optional[Sequence[str | list[str]]] = None, + **kwargs: Any + ): + self.id = id + self.key = key + self.status = status + self.variations = variations + self.trafficAllocation = trafficAllocation + self.audienceIds = audienceIds + self.audienceConditions = audienceConditions + self.includedFlags = includedFlags or [] + self.excludedFlags = excludedFlags or [] + self.layerId = '' + + def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]: + """Returns audienceConditions if present, otherwise audienceIds. + + This matches the Experiment.get_audience_conditions_or_ids() method + and enables holdouts to work with the same audience evaluation logic. + """ + return self.audienceConditions if self.audienceConditions is not None else self.audienceIds + + @property + def is_activated(self) -> bool: + """Check if the holdout is activated (running). + + Matches Swift's isActivated computed property: + var isActivated: Bool { return status == .running } + + Returns: + True if status is 'Running', False otherwise. + """ + return self.status == self.Status.RUNNING + + def __str__(self) -> str: + return self.key + + def __getitem__(self, key: str) -> Any: + """Enable dict-style access for backward compatibility with tests.""" + return getattr(self, key) + + def __setitem__(self, key: str, value: Any) -> None: + """Enable dict-style assignment for backward compatibility with tests.""" + setattr(self, key, value) + + def get(self, key: str, default: Any = None) -> Any: + """Enable dict-style .get() method for backward compatibility with tests.""" + return getattr(self, key, default) diff --git a/optimizely/event/event_factory.py b/optimizely/event/event_factory.py index 48b3a5cc..c7fbb0db 100644 --- a/optimizely/event/event_factory.py +++ b/optimizely/event/event_factory.py @@ -125,6 +125,9 @@ def _create_visitor(cls, event: Optional[user_event.UserEvent], logger: Logger) if isinstance(event.variation, entities.Variation): variation_id = event.variation.id variation_key = event.variation.key + elif isinstance(event.variation, dict): + variation_id = event.variation.get('id', '') + variation_key = event.variation.get('key', '') if event.experiment: experiment_layerId = event.experiment.layerId diff --git a/optimizely/event/user_event.py b/optimizely/event/user_event.py index c1152161..8006c052 100644 --- a/optimizely/event/user_event.py +++ b/optimizely/event/user_event.py @@ -27,9 +27,10 @@ if TYPE_CHECKING: # prevent circular dependenacy by skipping import at runtime - from optimizely.entities import Experiment, Variation, Event + from optimizely.entities import Experiment, Variation, Event, Holdout from optimizely.event.payload import VisitorAttribute from optimizely.helpers.event_tag_utils import EventTags + from optimizely.helpers.types import VariationDict CLIENT_NAME: Final = 'python-sdk' @@ -63,9 +64,9 @@ def __init__( self, event_context: EventContext, user_id: str, - experiment: Experiment, + experiment: Experiment | Holdout, visitor_attributes: list[VisitorAttribute], - variation: Optional[Variation], + variation: Optional[Variation | VariationDict], flag_key: str, rule_key: str, rule_type: str, diff --git a/optimizely/event/user_event_factory.py b/optimizely/event/user_event_factory.py index 1816da06..a213a278 100644 --- a/optimizely/event/user_event_factory.py +++ b/optimizely/event/user_event_factory.py @@ -23,7 +23,8 @@ # prevent circular dependenacy by skipping import at runtime from optimizely.optimizely_user_context import UserAttributes from optimizely.project_config import ProjectConfig - from optimizely.entities import Experiment, Variation + from optimizely.entities import Experiment, Variation, Holdout + from optimizely.helpers.types import VariationDict class UserEventFactory: @@ -33,7 +34,7 @@ class UserEventFactory: def create_impression_event( cls, project_config: ProjectConfig, - activated_experiment: Experiment, + activated_experiment: Experiment | Holdout, variation_id: Optional[str], flag_key: str, rule_key: str, @@ -64,18 +65,16 @@ def create_impression_event( if not activated_experiment and rule_type is not enums.DecisionSources.ROLLOUT: return None - variation: Optional[Variation] = None + variation: Optional[Variation | VariationDict] = None experiment_id = None if activated_experiment: - # For holdouts, activated_experiment is a dict; for experiments, it's an Experiment entity - experiment_id = (activated_experiment['id'] if isinstance(activated_experiment, dict) - else activated_experiment.id) + experiment_id = activated_experiment.id - if variation_id and flag_key: + if variation_id and flag_key and rule_type != enums.DecisionSources.HOLDOUT: # need this condition when we send events involving forced decisions - # (F-to-D or E-to-D with any ruleKey/variationKey combinations) variation = project_config.get_flag_variation(flag_key, 'id', variation_id) elif variation_id and experiment_id: + # For holdouts, experiments, and rollouts - lookup by experiment/holdout ID variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) event_context = user_event.EventContext( diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index 650221e6..4b014e7f 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -329,7 +329,8 @@ def _validate_user_inputs( return True def _send_impression_event( - self, project_config: project_config.ProjectConfig, experiment: Optional[entities.Experiment], + self, project_config: project_config.ProjectConfig, + experiment: Optional[Union[entities.Experiment, entities.Holdout]], variation: Optional[Union[entities.Variation, VariationDict]], flag_key: str, rule_key: str, rule_type: str, enabled: bool, user_id: str, attributes: Optional[UserAttributes], cmab_uuid: Optional[str] = None ) -> None: @@ -454,12 +455,8 @@ def _get_feature_variable_for_type( ) if decision.source in (enums.DecisionSources.FEATURE_TEST, enums.DecisionSources.HOLDOUT): - experiment_key = None - if decision.experiment: - experiment_key = (decision.experiment['key'] if isinstance(decision.experiment, dict) - else decision.experiment.key) source_info = { - 'experiment_key': experiment_key, + 'experiment_key': decision.experiment.key if decision.experiment else None, 'variation_key': self._get_variation_key(decision.variation), } @@ -562,12 +559,8 @@ def _get_all_feature_variables_for_type( all_variables[variable_key] = actual_value if decision.source == enums.DecisionSources.FEATURE_TEST: - experiment_key = None - if decision.experiment: - experiment_key = (decision.experiment['key'] if isinstance(decision.experiment, dict) - else decision.experiment.key) source_info = { - 'experiment_key': experiment_key, + 'experiment_key': decision.experiment.key if decision.experiment else None, 'variation_key': self._get_variation_key(decision.variation), } @@ -633,7 +626,8 @@ def activate(self, experiment_key: str, user_id: str, attributes: Optional[UserA self._send_impression_event(project_config, experiment, variation, '', experiment.key, enums.DecisionSources.EXPERIMENT, True, user_id, attributes) - return variation.key + # Handle both Variation entity and VariationDict + return variation['key'] if isinstance(variation, dict) else variation.key def track( self, event_key: str, user_id: str, @@ -810,25 +804,19 @@ def is_feature_enabled(self, feature_key: str, user_id: str, attributes: Optiona feature_enabled = True if (is_source_rollout or not decision.variation) and project_config.get_send_flag_decisions_value(): - experiment_key = '' - if decision.experiment: - experiment_key = (decision.experiment['key'] if isinstance(decision.experiment, dict) - else decision.experiment.key) self._send_impression_event( - project_config, decision.experiment, decision.variation, feature.key, experiment_key, - str(decision.source), feature_enabled, user_id, attributes, cmab_uuid + project_config, decision.experiment, decision.variation, feature.key, decision.experiment.key if + decision.experiment else '', str(decision.source), feature_enabled, user_id, attributes, cmab_uuid ) # Send event if Decision came from an experiment. if is_source_experiment and decision.variation and decision.experiment: - experiment_key = (decision.experiment['key'] if isinstance(decision.experiment, dict) - else decision.experiment.key) source_info = { - 'experiment_key': experiment_key, + 'experiment_key': decision.experiment.key, 'variation_key': self._get_variation_key(decision.variation), } self._send_impression_event( - project_config, decision.experiment, decision.variation, feature.key, experiment_key, + project_config, decision.experiment, decision.variation, feature.key, decision.experiment.key, str(decision.source), feature_enabled, user_id, attributes, cmab_uuid ) @@ -1134,7 +1122,10 @@ def get_forced_variation(self, experiment_key: str, user_id: str) -> Optional[st return None forced_variation, _ = self.decision_service.get_forced_variation(project_config, experiment_key, user_id) - return forced_variation.key if forced_variation else None + if forced_variation: + # Handle both Variation entity and VariationDict + return forced_variation['key'] if isinstance(forced_variation, dict) else forced_variation.key + return None def get_optimizely_config(self) -> Optional[OptimizelyConfig]: """ Gets OptimizelyConfig instance for the current project config. @@ -1266,12 +1257,7 @@ def _create_optimizely_decision( # Create Optimizely Decision Result. attributes = user_context.get_user_attributes() - # For holdouts, experiment is a dict; for experiments, it's an Experiment entity - if flag_decision.experiment: - rule_key = (flag_decision.experiment['key'] if isinstance(flag_decision.experiment, dict) - else flag_decision.experiment.key) - else: - rule_key = None + rule_key = flag_decision.experiment.key if flag_decision.experiment else None all_variables = {} decision_source = flag_decision.source decision_event_dispatched = False @@ -1279,7 +1265,6 @@ def _create_optimizely_decision( feature_flag = project_config.feature_key_map.get(flag_key) # Send impression event if Decision came from a feature - # test and decide options doesn't include disableDecisionEvent if OptimizelyDecideOption.DISABLE_DECISION_EVENT not in decide_options: if (decision_source == DecisionSources.FEATURE_TEST or decision_source == DecisionSources.HOLDOUT or @@ -1322,11 +1307,9 @@ def _create_optimizely_decision( try: if flag_decision.experiment is not None: - # For holdouts, experiment is a dict; for experiments, it's an Experiment entity - experiment_id = (flag_decision.experiment['id'] if isinstance(flag_decision.experiment, dict) - else flag_decision.experiment.id) - except (AttributeError, KeyError, TypeError): - self.logger.warning("Unable to extract experiment_id from flag_decision.experiment") + experiment_id = flag_decision.experiment.id + except AttributeError: + self.logger.warning("flag_decision.experiment has no attribute 'id'") try: if flag_decision.variation is not None: diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 32cd7e32..ca5a14b3 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -21,8 +21,6 @@ from .helpers import enums from .helpers import types -from optimizely.helpers.types import HoldoutDict, VariationDict - if version_info < (3, 8): from typing_extensions import Final else: @@ -31,6 +29,7 @@ if TYPE_CHECKING: # prevent circular dependenacy by skipping import at runtime from .logger import Logger + from .helpers.types import VariationDict SUPPORTED_VERSIONS = [ @@ -90,51 +89,48 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): region_value = config.get('region') self.region: str = region_value or 'US' - self.holdouts: list[HoldoutDict] = config.get('holdouts', []) - self.holdout_id_map: dict[str, HoldoutDict] = {} - self.global_holdouts: list[HoldoutDict] = [] - self.included_holdouts: dict[str, list[HoldoutDict]] = {} - self.excluded_holdouts: dict[str, list[HoldoutDict]] = {} - self.flag_holdouts_map: dict[str, list[HoldoutDict]] = {} - - for holdout in self.holdouts: - if holdout.get('status') != 'Running': + # Parse holdouts from datafile and convert to Holdout entities + holdouts_data: list[types.HoldoutDict] = config.get('holdouts', []) + self.holdouts: list[entities.Holdout] = [] + self.holdout_id_map: dict[str, entities.Holdout] = {} + self.global_holdouts: list[entities.Holdout] = [] + self.included_holdouts: dict[str, list[entities.Holdout]] = {} + self.excluded_holdouts: dict[str, list[entities.Holdout]] = {} + self.flag_holdouts_map: dict[str, list[entities.Holdout]] = {} + + # Convert holdout dicts to Holdout entities + for holdout_data in holdouts_data: + # Create Holdout entity + holdout = entities.Holdout(**holdout_data) + self.holdouts.append(holdout) + + # Only process Running holdouts but doing it here for efficiency like the original Python implementation) + if not holdout.is_activated: continue - # Ensure holdout has layerId field (holdouts don't have campaigns) - if 'layerId' not in holdout: - holdout['layerId'] = '' - - holdout_id = holdout['id'] - self.holdout_id_map[holdout_id] = holdout - - included_flags = holdout.get('includedFlags', []) - excluded_flags = holdout.get('excludedFlags', []) + # Map by ID for quick lookup + self.holdout_id_map[holdout.id] = holdout - has_included = bool(included_flags) - has_excluded = bool(excluded_flags) - - if not has_included and not has_excluded: - # No included or excluded flags - this is a global holdout + # Categorize as global vs flag-specific + # Global holdouts: apply to all flags unless explicitly excluded + # Flag-specific holdouts: only apply to explicitly included flags + if not holdout.includedFlags: + # This is a global holdout self.global_holdouts.append(holdout) - elif has_included: - # Has included flags - add to included_holdouts map - # (works for both cases with or without excluded flags) - for flag_id in included_flags: + # Track which flags this global holdout excludes + if holdout.excludedFlags: + for flag_id in holdout.excludedFlags: + if flag_id not in self.excluded_holdouts: + self.excluded_holdouts[flag_id] = [] + self.excluded_holdouts[flag_id].append(holdout) + else: + # This holdout applies to specific flags only + for flag_id in holdout.includedFlags: if flag_id not in self.included_holdouts: self.included_holdouts[flag_id] = [] self.included_holdouts[flag_id].append(holdout) - elif has_excluded and not has_included: - # No included flags but has excluded flags - global with exclusions - self.global_holdouts.append(holdout) - - for flag_id in excluded_flags: - if flag_id not in self.excluded_holdouts: - self.excluded_holdouts[flag_id] = [] - self.excluded_holdouts[flag_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( @@ -186,11 +182,11 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): self.all_segments += audience.get_segments() self.experiment_key_map: dict[str, entities.Experiment] = {} - self.variation_key_map: dict[str, dict[str, entities.Variation]] = {} - self.variation_id_map: dict[str, dict[str, entities.Variation]] = {} + self.variation_key_map: dict[str, dict[str, Union[entities.Variation, VariationDict]]] = {} + self.variation_id_map: dict[str, dict[str, Union[entities.Variation, VariationDict]]] = {} self.variation_variable_usage_map: dict[str, dict[str, entities.Variation.VariableUsage]] = {} - self.variation_id_map_by_experiment_id: dict[str, dict[str, entities.Variation]] = {} - self.variation_key_map_by_experiment_id: dict[str, dict[str, entities.Variation]] = {} + self.variation_id_map_by_experiment_id: dict[str, dict[str, Union[entities.Variation, VariationDict]]] = {} + self.variation_key_map_by_experiment_id: dict[str, dict[str, Union[entities.Variation, VariationDict]]] = {} self.flag_variations_map: dict[str, list[entities.Variation]] = {} for experiment in self.experiment_id_map.values(): @@ -204,11 +200,13 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): self.variation_key_map_by_experiment_id[experiment.id] = {} for variation in self.variation_key_map[experiment.key].values(): - self.variation_id_map[experiment.key][variation.id] = variation - self.variation_id_map_by_experiment_id[experiment.id][variation.id] = variation - self.variation_key_map_by_experiment_id[experiment.id][variation.key] = variation - self.variation_variable_usage_map[variation.id] = self._generate_key_map( - variation.variables, 'id', entities.Variation.VariableUsage + # Cast is safe here because experiments always use Variation entities, not VariationDict + var = cast(entities.Variation, variation) + self.variation_id_map[experiment.key][var.id] = var + self.variation_id_map_by_experiment_id[experiment.id][var.id] = var + self.variation_key_map_by_experiment_id[experiment.id][var.key] = var + self.variation_variable_usage_map[var.id] = self._generate_key_map( + var.variables, 'id', entities.Variation.VariableUsage ) self.feature_key_map = self._generate_key_map(self.feature_flags, 'key', entities.FeatureFlag) @@ -234,6 +232,22 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): self.experiment_feature_map[exp_id] = [feature.id] rules.append(self.experiment_id_map[exp_id]) + flag_id = feature.id + applicable_holdouts: list[entities.Holdout] = [] + + # Add global holdouts first, excluding any that are explicitly excluded for this flag + excluded_holdouts = self.excluded_holdouts.get(flag_id, []) + for holdout in self.global_holdouts: + if holdout not in excluded_holdouts: + applicable_holdouts.append(holdout) + + # Add flag-specific included holdouts AFTER global holdouts + if flag_id in self.included_holdouts: + applicable_holdouts.extend(self.included_holdouts[flag_id]) + + if applicable_holdouts: + self.flag_holdouts_map[feature.key] = applicable_holdouts + rollout = None if len(feature.rolloutId) == 0 else self.rollout_id_map[feature.rolloutId] if rollout: for exp in rollout.experiments: @@ -243,33 +257,28 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): # variation_id_map_by_experiment_id gives variation entity object while # experiment_id_map will give us dictionary for rule_variation in self.variation_id_map_by_experiment_id[rule.id].values(): - if len(list(filter(lambda variation: variation.id == rule_variation.id, variations))) == 0: - variations.append(rule_variation) + # Cast is safe here because rollout rules use Variation entities + rule_var = cast(entities.Variation, rule_variation) + if len(list(filter(lambda variation: variation.id == rule_var.id, variations))) == 0: + variations.append(rule_var) self.flag_variations_map[feature.key] = variations + # Process holdout variations are converted to Variation entities just like experiment variations if self.holdouts: for holdout in self.holdouts: - holdout_key = holdout.get('key') - holdout_id = holdout.get('id') - - if not holdout_key or not holdout_id: - continue - - self.variation_key_map[holdout_key] = {} - self.variation_id_map[holdout_key] = {} - self.variation_id_map_by_experiment_id[holdout_id] = {} - self.variation_key_map_by_experiment_id[holdout_id] = {} - - variations = holdout.get('variations') - if variations: - for variation in variations: - variation_key = variation.get('key') if isinstance(variation, dict) else None - variation_id = variation.get('id') if isinstance(variation, dict) else None - if variation_key and variation_id: - self.variation_key_map[holdout_key][variation_key] = variation - self.variation_id_map[holdout_key][variation_id] = variation - self.variation_key_map_by_experiment_id[holdout_id][variation_key] = variation - self.variation_id_map_by_experiment_id[holdout_id][variation_id] = variation + # Initialize variation maps for this holdout + self.variation_key_map[holdout.key] = {} + self.variation_id_map[holdout.key] = {} + self.variation_id_map_by_experiment_id[holdout.id] = {} + self.variation_key_map_by_experiment_id[holdout.id] = {} + + if holdout.variations: + for variation_dict in holdout.variations: + # Map variations by key and ID using dict format + self.variation_key_map[holdout.key][variation_dict['key']] = variation_dict + self.variation_id_map[holdout.key][variation_dict['id']] = variation_dict + self.variation_key_map_by_experiment_id[holdout.id][variation_dict['key']] = variation_dict + self.variation_id_map_by_experiment_id[holdout.id][variation_dict['id']] = variation_dict @staticmethod def _generate_key_map( @@ -487,7 +496,9 @@ def get_audience(self, audience_id: str) -> Optional[entities.Audience]: self.error_handler.handle_error(exceptions.InvalidAudienceException((enums.Errors.INVALID_AUDIENCE))) return None - def get_variation_from_key(self, experiment_key: str, variation_key: str) -> Optional[entities.Variation]: + def get_variation_from_key( + self, experiment_key: str, variation_key: str + ) -> Optional[Union[entities.Variation, VariationDict]]: """ Get variation given experiment and variation key. Args: @@ -514,7 +525,9 @@ def get_variation_from_key(self, experiment_key: str, variation_key: str) -> Opt self.error_handler.handle_error(exceptions.InvalidExperimentException(enums.Errors.INVALID_EXPERIMENT_KEY)) return None - def get_variation_from_id(self, experiment_key: str, variation_id: str) -> Optional[entities.Variation]: + def get_variation_from_id( + self, experiment_key: str, variation_id: str + ) -> Optional[Union[entities.Variation, VariationDict]]: """ Get variation given experiment and variation ID. Args: @@ -760,7 +773,7 @@ def is_feature_experiment(self, experiment_id: str) -> bool: def get_variation_from_id_by_experiment_id( self, experiment_id: str, variation_id: str - ) -> Optional[entities.Variation]: + ) -> Optional[Union[entities.Variation, VariationDict]]: """ Gets variation from variation id and specific experiment id Returns: @@ -779,7 +792,7 @@ def get_variation_from_id_by_experiment_id( def get_variation_from_key_by_experiment_id( self, experiment_id: str, variation_key: str - ) -> Optional[entities.Variation]: + ) -> Optional[Union[entities.Variation, VariationDict]]: """ Gets variation from variation key and specific experiment id Returns: @@ -833,55 +846,28 @@ def get_flag_variation( return None - def get_holdouts_for_flag(self, flag_key: str) -> list[HoldoutDict]: + def get_holdouts_for_flag(self, flag_key: str) -> list[entities.Holdout]: """ Helper method to get holdouts from an applied feature flag. Args: flag_key: Key of the feature flag. - This parameter is required and should not be null/None. Returns: - The holdouts that apply for a specific flag. + The holdouts that apply for a specific flag as Holdout entity objects. """ if not self.holdouts: return [] - # Get the feature flag to extract flag_id - feature = self.feature_key_map.get(flag_key) - if not feature: - return [] - - flag_id = feature.id - - # Check cache first (before validation, so we cache the validation result too) - if flag_id in self.flag_holdouts_map: - return self.flag_holdouts_map[flag_id] - - # Prioritize global holdouts first - excluded = self.excluded_holdouts.get(flag_id, []) - - if excluded: - active_holdouts = [holdout for holdout in self.global_holdouts if holdout not in excluded] - else: - active_holdouts = self.global_holdouts.copy() - - # Append included holdouts - included = self.included_holdouts.get(flag_id, []) - active_holdouts.extend(included) - - # Cache the result - self.flag_holdouts_map[flag_id] = active_holdouts - - return self.flag_holdouts_map[flag_id] + return self.flag_holdouts_map.get(flag_key, []) - def get_holdout(self, holdout_id: str) -> Optional[HoldoutDict]: + def get_holdout(self, holdout_id: str) -> Optional[entities.Holdout]: """ Helper method to get holdout from holdout ID. Args: holdout_id: ID of the holdout. Returns: - The holdout corresponding to the provided holdout ID. + The holdout corresponding to the provided holdout ID as a Holdout entity object. """ holdout = self.holdout_id_map.get(holdout_id) diff --git a/tests/test_config.py b/tests/test_config.py index 7c1097e4..81228feb 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1393,6 +1393,9 @@ def setUp(self): 'id': 'holdout_1', 'key': 'global_holdout', 'status': 'Running', + 'variations': [], + 'trafficAllocation': [], + 'audienceIds': [], 'includedFlags': [], 'excludedFlags': [boolean_feature_id] }, @@ -1400,6 +1403,9 @@ def setUp(self): 'id': 'holdout_2', 'key': 'specific_holdout', 'status': 'Running', + 'variations': [], + 'trafficAllocation': [], + 'audienceIds': [], 'includedFlags': [multi_variate_feature_id], 'excludedFlags': [] }, @@ -1407,6 +1413,9 @@ def setUp(self): 'id': 'holdout_3', 'key': 'inactive_holdout', 'status': 'Inactive', + 'variations': [], + 'trafficAllocation': [], + 'audienceIds': [], 'includedFlags': [boolean_feature_id], 'excludedFlags': [] } @@ -1512,9 +1521,9 @@ def test_holdout_initialization__categorizes_holdouts_properly(self): self.assertIn('holdout_1', self.config_with_holdouts.holdout_id_map) self.assertIn('holdout_2', self.config_with_holdouts.holdout_id_map) - # Check if holdout_1 is in global_holdouts (list of dicts) - holdout_ids = [h['id'] for h in self.config_with_holdouts.global_holdouts] - self.assertIn('holdout_1', holdout_ids) + # Check if a holdout with ID 'holdout_1' exists in global_holdouts + holdout_ids_in_global = [h.id for h in self.config_with_holdouts.global_holdouts] + self.assertIn('holdout_1', holdout_ids_in_global) # Use correct feature flag IDs boolean_feature_id = '91111' diff --git a/tests/test_decision_service.py b/tests/test_decision_service.py index dd183312..dbcb7436 100644 --- a/tests/test_decision_service.py +++ b/tests/test_decision_service.py @@ -1377,19 +1377,15 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_rollout(sel expected_variation = self.project_config.get_variation_from_id( "211127", "211129" ) - from optimizely.decision_service import Decision - from optimizely.helpers import enums - expected_decision = Decision(None, expected_variation, enums.DecisionSources.ROLLOUT, None) get_variation_for_rollout_patch = mock.patch( "optimizely.decision_service.DecisionService.get_variation_for_rollout", - return_value=(expected_decision, []), + return_value=[expected_variation, None], ) with get_variation_for_rollout_patch as mock_get_variation_for_rollout, \ self.mock_decision_logger as mock_decision_service_logging: - decision_result = self.decision_service.get_variation_for_feature( + variation_received = self.decision_service.get_variation_for_feature( self.project_config, feature, user, False - ) - variation_received = decision_result['decision'].variation if decision_result['decision'] else None + )['decision'] self.assertEqual( expected_variation, variation_received, @@ -1399,8 +1395,9 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_rollout(sel self.project_config, feature, user ) - # Assert info log message was generated for rollout bucketing - self.assertEqual(1, mock_decision_service_logging.info.call_count) + # Assert no log messages were generated + self.assertEqual(1, mock_decision_service_logging.debug.call_count) + self.assertEqual(1, len(mock_decision_service_logging.method_calls)) def test_get_variation_for_feature__returns_variation_if_user_not_in_experiment_but_in_rollout( self, diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index d74c6618..e286ba5c 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -147,8 +147,8 @@ def test_holdout_inactive_does_not_bucket_users(self): self.assertIsNotNone(holdout) # Mock holdout as inactive - original_status = holdout['status'] - holdout['status'] = 'Paused' + original_status = holdout.status + holdout.status = 'Paused' user_context = self.opt_obj.create_user_context('testUserId', {}) @@ -164,7 +164,7 @@ def test_holdout_inactive_does_not_bucket_users(self): self.assertIn('reasons', result) # Restore original status - holdout['status'] = original_status + holdout.status = original_status def test_user_not_bucketed_into_holdout_executes_successfully(self): """When user is not bucketed into holdout, should execute successfully with valid result structure.""" @@ -282,7 +282,7 @@ def test_user_bucketed_into_holdout_returns_before_experiments(self): holdout = self.config_with_holdouts.holdouts[0] if self.config_with_holdouts.holdouts else None self.assertIsNotNone(holdout) - holdout_variation = holdout['variations'][0] + holdout_variation = holdout.variations[0] # Create a holdout decision mock_holdout_decision = decision_service.Decision( @@ -384,7 +384,7 @@ def test_evaluates_global_holdouts_for_all_flags(self): # Get global holdouts global_holdouts = [ h for h in self.config_with_holdouts.holdouts - if not h.get('includedFlags') or len(h.get('includedFlags', [])) == 0 + if not h.includedFlags or len(h.includedFlags) == 0 ] if global_holdouts: @@ -409,7 +409,7 @@ def test_respects_included_and_excluded_flags_configuration(self): holdouts_for_flag = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_experiment') # Should not include holdouts that exclude this flag - excluded_holdout = next((h for h in holdouts_for_flag if h.get('key') == 'excluded_holdout'), None) + excluded_holdout = next((h for h in holdouts_for_flag if h.key == 'excluded_holdout'), None) self.assertIsNone(excluded_holdout) # Holdout logging and error handling tests @@ -507,16 +507,18 @@ def test_uses_consistent_bucketing_for_same_user(self): # If both have decisions, they should match if decision1 and decision2: - # Variation can be either an object or a dict (for holdouts) - def get_variation_id(variation): - if variation is None: - return None - if isinstance(variation, dict): - return variation.get('id') - return variation.id - - var1_id = get_variation_id(decision1.variation) - var2_id = get_variation_id(decision2.variation) + # Handle both dict and Variation entity formats + if decision1.variation: + var1_id = (decision1.variation['id'] if isinstance(decision1.variation, dict) + else decision1.variation.id) + else: + var1_id = None + + if decision2.variation: + var2_id = (decision2.variation['id'] if isinstance(decision2.variation, dict) + else decision2.variation.id) + else: + var2_id = None self.assertEqual( var1_id, var2_id, @@ -673,7 +675,7 @@ def test_decide_with_holdout_sends_impression_event(self): actual_holdout = None if config.holdouts and decision.rule_key: actual_holdout = next( - (h for h in config.holdouts if h.get('key') == decision.rule_key), + (h for h in config.holdouts if h.key == decision.rule_key), None ) @@ -682,13 +684,13 @@ def test_decide_with_holdout_sends_impression_event(self): self.assertEqual(decision.flag_key, feature_flag.key) holdout_variation = next( - (v for v in actual_holdout['variations'] if v.get('key') == decision.variation_key), + (v for v in actual_holdout.variations if v.get('key') == decision.variation_key), None ) self.assertIsNotNone( holdout_variation, - f"Variation '{decision.variation_key}' should be from the chosen holdout '{actual_holdout['key']}'" + f"Variation '{decision.variation_key}' should be from the chosen holdout '{actual_holdout.key}'" ) self.assertEqual( @@ -774,7 +776,7 @@ def test_decide_with_holdout_does_not_send_impression_when_disabled(self): chosen_holdout = None if config.holdouts and decision.rule_key: chosen_holdout = next( - (h for h in config.holdouts if h.get('key') == decision.rule_key), + (h for h in config.holdouts if h.key == decision.rule_key), None ) @@ -838,12 +840,12 @@ def notification_callback(notification_type, user_id, user_attributes, decision_ holdout = config.holdouts[0] if config.holdouts else None self.assertIsNotNone(holdout, 'Should have at least one holdout configured') - holdout_variation = holdout['variations'][0] + holdout_variation = holdout.variations[0] self.assertIsNotNone(holdout_variation, 'Holdout should have at least one variation') mock_experiment = mock.MagicMock() - mock_experiment.key = holdout['key'] - mock_experiment.id = holdout['id'] + mock_experiment.key = holdout.key + mock_experiment.id = holdout.id # Mock the decision service to return a holdout decision holdout_decision = decision_service.Decision( @@ -874,7 +876,7 @@ def notification_callback(notification_type, user_id, user_attributes, decision_ notification = captured_notifications[0] rule_key = notification.get('rule_key') - self.assertEqual(rule_key, holdout['key'], 'RuleKey should match holdout key') + self.assertEqual(rule_key, holdout.key, 'RuleKey should match holdout key') # Verify holdout notification structure self.assertIn('flag_key', notification, 'Holdout notification should contain flag_key') @@ -887,7 +889,7 @@ def notification_callback(notification_type, user_id, user_attributes, decision_ self.assertEqual(flag_key, 'test_feature_in_experiment', 'FlagKey should match the requested flag') experiment_id = notification.get('experiment_id') - self.assertEqual(experiment_id, holdout['id'], 'ExperimentId in notification should match holdout ID') + self.assertEqual(experiment_id, holdout.id, 'ExperimentId in notification should match holdout ID') variation_id = notification.get('variation_id') self.assertEqual(variation_id, holdout_variation['id'], 'VariationId should match holdout variation ID') @@ -917,3 +919,552 @@ def notification_callback(notification_type, user_id, user_attributes, decision_ ) finally: opt_with_mocked_events.close() + + # DecideAll with holdouts tests (aligned with Swift SDK) + + def test_decide_all_with_global_holdout(self): + """Should apply global holdout to all flags in decide_all.""" + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'global_holdout', + 'key': 'global_test_holdout', + 'status': 'Running', + 'includedFlags': [], # Global - applies to all flags + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'global_holdout_var', + 'key': 'global_holdout_control', + 'featureEnabled': False, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'global_holdout_var', + 'endOfRange': 10000 + } + ] + } + ] + + config_json = json.dumps(config_dict_with_holdouts) + opt = optimizely_module.Optimizely(datafile=config_json) + + try: + user_context = opt.create_user_context('test_user', {}) + decisions = user_context.decide_all() + + # Global holdout should apply to all flags + self.assertGreater(len(decisions), 0) + + # Check that decisions exist for all flags + for flag_key, decision in decisions.items(): + self.assertIsNotNone(decision) + finally: + opt.close() + + def test_decide_all_with_included_flags(self): + """Should apply holdout only to included flags in decide_all.""" + # Get feature flag IDs + feature1_id = '91111' # test_feature_in_experiment + + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'included_holdout', + 'key': 'specific_holdout', + 'status': 'Running', + 'includedFlags': [feature1_id], # Only applies to feature1 + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'included_var', + 'key': 'included_control', + 'featureEnabled': False, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'included_var', + 'endOfRange': 10000 + } + ] + } + ] + + config_json = json.dumps(config_dict_with_holdouts) + opt = optimizely_module.Optimizely(datafile=config_json) + + try: + user_context = opt.create_user_context('test_user', {}) + decisions = user_context.decide_all() + + self.assertGreater(len(decisions), 0) + + # Verify all flags have decisions + for decision in decisions.values(): + self.assertIsNotNone(decision) + finally: + opt.close() + + def test_decide_all_with_excluded_flags(self): + """Should exclude holdout from excluded flags in decide_all.""" + feature1_id = '91111' # test_feature_in_experiment + + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'excluded_holdout', + 'key': 'global_except_one', + 'status': 'Running', + 'includedFlags': [], # Global + 'excludedFlags': [feature1_id], # Except feature1 + 'audienceIds': [], + 'variations': [ + { + 'id': 'excluded_var', + 'key': 'excluded_control', + 'featureEnabled': False, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'excluded_var', + 'endOfRange': 10000 + } + ] + } + ] + + config_json = json.dumps(config_dict_with_holdouts) + opt = optimizely_module.Optimizely(datafile=config_json) + + try: + user_context = opt.create_user_context('test_user', {}) + decisions = user_context.decide_all() + + # Verify decisions exist for all flags + self.assertGreater(len(decisions), 0) + for decision in decisions.values(): + self.assertIsNotNone(decision) + finally: + opt.close() + + def test_decide_all_with_multiple_holdouts(self): + """Should handle multiple holdouts with correct priority.""" + feature1_id = '91111' + feature2_id = '91112' + + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['holdouts'] = [ + # Global holdout (applies to all) + { + 'id': 'global_holdout', + 'key': 'global_holdout', + 'status': 'Running', + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'global_var', + 'key': 'global_control', + 'featureEnabled': False, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'global_var', + 'endOfRange': 10000 + } + ] + }, + # Specific holdout (only for feature2) + { + 'id': 'specific_holdout', + 'key': 'specific_holdout', + 'status': 'Running', + 'includedFlags': [feature2_id], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'specific_var', + 'key': 'specific_control', + 'featureEnabled': False, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'specific_var', + 'endOfRange': 10000 + } + ] + }, + # Excluded holdout (all except feature1) + { + 'id': 'excluded_holdout', + 'key': 'excluded_holdout', + 'status': 'Running', + 'includedFlags': [], + 'excludedFlags': [feature1_id], + 'audienceIds': [], + 'variations': [ + { + 'id': 'excluded_var', + 'key': 'excluded_control', + 'featureEnabled': False, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'excluded_var', + 'endOfRange': 10000 + } + ] + } + ] + + config_json = json.dumps(config_dict_with_holdouts) + opt = optimizely_module.Optimizely(datafile=config_json) + + try: + user_context = opt.create_user_context('test_user', {}) + decisions = user_context.decide_all() + + # Verify we got decisions for all flags + self.assertGreater(len(decisions), 0) + finally: + opt.close() + + def test_decide_all_with_enabled_flags_only_option(self): + """Should filter out disabled flags when using enabled_flags_only option.""" + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'disabled_holdout', + 'key': 'disabled_holdout', + 'status': 'Running', + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'disabled_var', + 'key': 'disabled_control', + 'featureEnabled': False, # Feature disabled + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'disabled_var', + 'endOfRange': 10000 + } + ] + } + ] + + config_json = json.dumps(config_dict_with_holdouts) + opt = optimizely_module.Optimizely(datafile=config_json) + + try: + user_context = opt.create_user_context('test_user', {}) + + # Without enabled_flags_only, all flags should be returned + all_decisions = user_context.decide_all() + + # With enabled_flags_only, disabled flags should be filtered out + enabled_decisions = user_context.decide_all( + options=[OptimizelyDecideOption.ENABLED_FLAGS_ONLY] + ) + + # enabled_decisions should have fewer or equal entries + self.assertLessEqual(len(enabled_decisions), len(all_decisions)) + finally: + opt.close() + + # Impression event metadata tests (aligned with Swift SDK) + + def test_holdout_impression_event_has_correct_metadata(self): + """Should include correct metadata in holdout impression events.""" + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'metadata_holdout', + 'key': 'metadata_test_holdout', + 'status': 'Running', + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'metadata_var', + 'key': 'metadata_control', + 'featureEnabled': False, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'metadata_var', + 'endOfRange': 10000 + } + ] + } + ] + + spy_event_processor = mock.MagicMock() + + config_json = json.dumps(config_dict_with_holdouts) + opt = optimizely_module.Optimizely( + datafile=config_json, + event_processor=spy_event_processor + ) + + try: + user_context = opt.create_user_context('test_user', {}) + decision = user_context.decide('test_feature_in_experiment') + + # If this was a holdout decision, verify event metadata + if decision.rule_key == 'metadata_test_holdout': + self.assertGreater(spy_event_processor.process.call_count, 0) + + # Get the user event that was sent + call_args = spy_event_processor.process.call_args_list[0] + user_event = call_args[0][0] + + # Verify user event has correct structure + self.assertIsNotNone(user_event) + finally: + opt.close() + + def test_holdout_impression_respects_send_flag_decisions_false(self): + """Should send holdout impression even when sendFlagDecisions is false.""" + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['sendFlagDecisions'] = False + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'send_flag_holdout', + 'key': 'send_flag_test_holdout', + 'status': 'Running', + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'send_flag_var', + 'key': 'send_flag_control', + 'featureEnabled': False, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'send_flag_var', + 'endOfRange': 10000 + } + ] + } + ] + + spy_event_processor = mock.MagicMock() + + config_json = json.dumps(config_dict_with_holdouts) + opt = optimizely_module.Optimizely( + datafile=config_json, + event_processor=spy_event_processor + ) + + try: + user_context = opt.create_user_context('test_user', {}) + decision = user_context.decide('test_feature_in_experiment') + + # Holdout impressions should be sent even when sendFlagDecisions=false + # (unlike rollout impressions) + if decision.rule_key == 'send_flag_test_holdout': + # Verify impression was sent for holdout + self.assertGreater(spy_event_processor.process.call_count, 0) + finally: + opt.close() + + # Holdout status tests (aligned with Swift SDK) + + def test_holdout_not_running_does_not_apply(self): + """Should not apply holdout when status is not Running.""" + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'draft_holdout', + 'key': 'draft_holdout', + 'status': 'Draft', # Not Running + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'draft_var', + 'key': 'draft_control', + 'featureEnabled': False, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'draft_var', + 'endOfRange': 10000 + } + ] + } + ] + + config_json = json.dumps(config_dict_with_holdouts) + opt = optimizely_module.Optimizely(datafile=config_json) + + try: + user_context = opt.create_user_context('test_user', {}) + decision = user_context.decide('test_feature_in_experiment') + + # Should not be a holdout decision since status is Draft + self.assertNotEqual(decision.rule_key, 'draft_holdout') + finally: + opt.close() + + def test_holdout_concluded_status_does_not_apply(self): + """Should not apply holdout when status is Concluded.""" + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'concluded_holdout', + 'key': 'concluded_holdout', + 'status': 'Concluded', + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'concluded_var', + 'key': 'concluded_control', + 'featureEnabled': False, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'concluded_var', + 'endOfRange': 10000 + } + ] + } + ] + + config_json = json.dumps(config_dict_with_holdouts) + opt = optimizely_module.Optimizely(datafile=config_json) + + try: + user_context = opt.create_user_context('test_user', {}) + decision = user_context.decide('test_feature_in_experiment') + + # Should not be a holdout decision since status is Concluded + self.assertNotEqual(decision.rule_key, 'concluded_holdout') + finally: + opt.close() + + def test_holdout_archived_status_does_not_apply(self): + """Should not apply holdout when status is Archived.""" + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'archived_holdout', + 'key': 'archived_holdout', + 'status': 'Archived', + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': [], + 'variations': [ + { + 'id': 'archived_var', + 'key': 'archived_control', + 'featureEnabled': False, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'archived_var', + 'endOfRange': 10000 + } + ] + } + ] + + config_json = json.dumps(config_dict_with_holdouts) + opt = optimizely_module.Optimizely(datafile=config_json) + + try: + user_context = opt.create_user_context('test_user', {}) + decision = user_context.decide('test_feature_in_experiment') + + # Should not be a holdout decision since status is Archived + self.assertNotEqual(decision.rule_key, 'archived_holdout') + finally: + opt.close() + + # Audience targeting tests for holdouts (aligned with Swift SDK) + + def test_holdout_with_audience_match(self): + """Should bucket user into holdout when audience conditions match.""" + # Using audienceIds that exist in the datafile + # audience '11154' is for "browser_type" = "chrome" + config_dict_with_holdouts = self.config_dict_with_features.copy() + config_dict_with_holdouts['holdouts'] = [ + { + 'id': 'audience_holdout', + 'key': 'audience_test_holdout', + 'status': 'Running', + 'includedFlags': [], + 'excludedFlags': [], + 'audienceIds': ['11154'], # Requires browser_type=chrome + 'variations': [ + { + 'id': 'audience_var', + 'key': 'audience_control', + 'featureEnabled': False, + 'variables': [] + } + ], + 'trafficAllocation': [ + { + 'entityId': 'audience_var', + 'endOfRange': 10000 + } + ] + } + ] + + config_json = json.dumps(config_dict_with_holdouts) + opt = optimizely_module.Optimizely(datafile=config_json) + + try: + # User with matching attribute + user_context_match = opt.create_user_context('test_user', {'browser_type': 'chrome'}) + decision_match = user_context_match.decide('test_feature_in_experiment') + + # User with non-matching attribute + user_context_no_match = opt.create_user_context('test_user', {'browser_type': 'firefox'}) + decision_no_match = user_context_no_match.decide('test_feature_in_experiment') + + # Both should have valid decisions + self.assertIsNotNone(decision_match) + self.assertIsNotNone(decision_no_match) + finally: + opt.close() diff --git a/tests/test_user_context.py b/tests/test_user_context.py index 1b72e3e3..3ae9be0d 100644 --- a/tests/test_user_context.py +++ b/tests/test_user_context.py @@ -872,8 +872,7 @@ def test_decide__option__include_reasons__feature_rollout(self): 'Evaluating audiences for rule 1: ["11154"].', 'Audiences for rule 1 collectively evaluated to TRUE.', 'User "test_user" meets audience conditions for targeting rule 1.', - 'User "test_user" bucketed into a targeting rule 1.', - "The user 'test_user' is bucketed into a rollout for feature flag 'test_feature_in_rollout'." + 'User "test_user" bucketed into a targeting rule 1.' ] self.assertEqual(expected_reasons, actual.reasons) @@ -1271,8 +1270,7 @@ def test_decide_reasons__hit_everyone_else_rule(self): 'Evaluating audiences for rule Everyone Else: [].', 'Audiences for rule Everyone Else collectively evaluated to TRUE.', 'User "abcde" meets audience conditions for targeting rule Everyone Else.', - 'User "abcde" bucketed into a targeting rule Everyone Else.', - "The user 'abcde' is bucketed into a rollout for feature flag 'test_feature_in_rollout'." + 'User "abcde" bucketed into a targeting rule Everyone Else.' ] self.assertEqual(expected_reasons, actual.reasons)