[FSSDK-12094] Fix Python holdout implementation which related to FSC failures

.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:

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()

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

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: