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 78be89d8..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, @@ -824,9 +887,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 @@ -946,83 +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 - - # 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) + 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/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 7b27d849..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 ) @@ -1252,7 +1266,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 @@ -1262,7 +1281,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, @@ -1301,9 +1322,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: diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 23508bb6..32cd7e32 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,33 @@ def get_holdouts_for_flag(self, flag_key: str) -> list[HoldoutDict]: if not self.holdouts: return [] - return self.flag_holdouts_map.get(flag_key, []) + # 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] 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..dd183312 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 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_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..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)