[FSSDK-11571] Python: Add holdout support and refactor decision logic in DefaultDecisionService

esrakartalOpt · esrakartalOpt · commit c22bfd94f712 · 2025-11-05T14:47:08.000-06:00
diff --git a/optimizely/bucketer.py b/optimizely/bucketer.py
@@ -119,16 +119,41 @@ def bucket(
             and array of log messages representing decision making.
      */.
         """
+        # Check if experiment is None first
+        if not experiment:
+            message = 'Invalid entity key provided for bucketing. Returning nil.'
+            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
+
+        if not experiment_key or not experiment_key.strip():
+            message = 'Invalid entity key provided for bucketing. Returning nil.'
+            project_config.logger.debug(message)
+            return None, []
+
         variation_id, decide_reasons = self.bucket_to_entity_id(project_config, experiment, user_id, bucketing_id)
         if variation_id:
-            variation = project_config.get_variation_from_id_by_experiment_id(experiment.id, 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)
             return variation, decide_reasons
-
-        else:
-            message = 'Bucketed into an empty traffic range. Returning nil.'
-            project_config.logger.info(message)
-            decide_reasons.append(message)
-
+        
+        # No variation found - log message for empty traffic range
+        message = 'Bucketed into an empty traffic range. Returning nil.'
+        project_config.logger.info(message)
+        decide_reasons.append(message)
         return None, decide_reasons
 
     def bucket_to_entity_id(
@@ -151,9 +176,25 @@ 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)
+            group_policy = getattr(experiment, 'groupPolicy', None)
+
         # Determine if experiment is in a mutually exclusive group.
-        # This will not affect evaluation of rollout rules.
-        if experiment.groupPolicy in GROUP_POLICIES:
+        # 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:
@@ -169,26 +210,26 @@ def bucket_to_entity_id(
                 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}.'
+            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
 
-            message = f'User "{user_id}" is 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)
 
-        traffic_allocations: list[TrafficAllocation] = experiment.trafficAllocation
-        if experiment.cmab:
+        if has_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,
-                                        experiment.id, traffic_allocations)
+                                        experiment_id, traffic_allocations)
 
         return variation_id, decide_reasons
diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py
@@ -670,7 +670,160 @@ 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.
         """
-        return self.get_variations_for_feature_list(project_config, [feature], user_context, options)[0]
+        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)
+        else:
+            return self.get_variations_for_feature_list(project_config, [feature], user_context, options)[0]
+
+    def get_decision_for_flag(
+        self,
+        feature_flag: entities.FeatureFlag,
+        user_context: OptimizelyUserContext,
+        project_config: ProjectConfig,
+        decide_options: Optional[Sequence[str]] = None,
+        user_profile_tracker: Optional[UserProfileTracker] = None,
+        decide_reasons: Optional[list[str]] = None
+    ) -> DecisionResult:
+        """
+        Get the decision for a single feature flag.
+        Processes holdouts, experiments, and rollouts in that order.
+
+        Args:
+            feature_flag: The feature flag to get a decision for.
+            user_context: The user context.
+            project_config: The project config.
+            decide_options: Sequence of decide options.
+            user_profile_tracker: The user profile tracker.
+            decide_reasons: List of decision reasons to merge.
+
+        Returns:
+            A DecisionResult for the feature flag.
+        """
+        reasons = decide_reasons.copy() if decide_reasons else []
+        user_id = user_context.user_id
+
+        # Check holdouts
+        holdouts = project_config.get_holdouts_for_flag(feature_flag.key)
+        for holdout in holdouts:
+            holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config)
+            reasons.extend(holdout_decision['reasons'])
+
+            if not holdout_decision['decision']:
+                continue
+
+            message = f"The user '{user_id}' is bucketed into holdout '{holdout['key']}' " \
+                    f"for feature flag '{feature_flag.key}'."
+            self.logger.info(message)
+            reasons.append(message)
+            return {
+                'decision': holdout_decision['decision'],
+                'error': False,
+                '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'])
+        
+        return {
+            'decision': fallback_result.get('decision'),
+            'error': fallback_result.get('error', False),
+            'reasons': reasons
+        }
+
+    def get_variation_for_holdout(
+        self,
+        holdout: dict[str, str],
+        user_context: OptimizelyUserContext,
+        project_config: ProjectConfig
+    ) -> DecisionResult:
+        """
+        Get the variation for holdout.
+
+        Args:
+            holdout: The holdout configuration.
+            user_context: The user context.
+            project_config: The project config.
+
+        Returns:
+            A DecisionResult for the holdout.
+        """
+        from optimizely.helpers.enums import ExperimentAudienceEvaluationLogs
+        
+        decide_reasons: list[str] = []
+        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."
+            self.logger.info(message)
+            decide_reasons.append(message)
+            return {
+                'decision': None,
+                'error': False,
+                'reasons': decide_reasons
+            }
+
+        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')
+        user_meets_audience_conditions, reasons_received = audience_helper.does_user_meet_audience_conditions(
+            project_config, 
+            audience_conditions,
+            ExperimentAudienceEvaluationLogs,
+            holdout.get('key', 'unknown'),
+            user_context,
+            self.logger
+        )
+        decide_reasons.extend(reasons_received)
+
+        if not user_meets_audience_conditions:
+            message = f"User '{user_id}' does not meet the conditions for holdout '{holdout['key']}'."
+            self.logger.debug(message)
+            decide_reasons.append(message)
+            return {
+                'decision': None,
+                'error': False,
+                'reasons': decide_reasons
+            }
+
+        # Bucket user into holdout variation
+        variation, bucket_reasons = self.bucketer.bucket(project_config, holdout, user_id, bucketing_id)
+        decide_reasons.extend(bucket_reasons)
+
+        if variation:
+            message = f"The user '{user_id}' is bucketed into variation '{variation['key']}' " \
+                    f"of holdout '{holdout['key']}'."
+            self.logger.info(message)
+            decide_reasons.append(message)
+
+            holdout_decision = Decision(holdout, variation, enums.DecisionSources.HOLDOUT, None)
+            return {
+                'decision': holdout_decision,
+                'error': False,
+                'reasons': decide_reasons
+            }
+
+        message = f"User '{user_id}' is not bucketed into any variation for holdout '{holdout['key']}'."
+        self.logger.info(message)
+        decide_reasons.append(message)
+        return {
+            'decision': None,
+            'error': False,
+            'reasons': decide_reasons
+        }
 
     def validated_forced_decision(
         self,
diff --git a/optimizely/helpers/enums.py b/optimizely/helpers/enums.py
@@ -99,6 +99,7 @@ class DecisionSources:
     EXPERIMENT: Final = 'experiment'
     FEATURE_TEST: Final = 'feature-test'
     ROLLOUT: Final = 'rollout'
+    HOLDOUT: Final = 'holdout'
 
 
 class Errors:
diff --git a/optimizely/project_config.py b/optimizely/project_config.py
@@ -218,6 +218,21 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any):
                 # Add this experiment in experiment-feature map.
                 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:
@@ -231,6 +246,30 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any):
                         variations.append(rule_variation)
             self.flag_variations_map[feature.key] = 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')
+                        variation_id = variation.get('id')
+                        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
+
     @staticmethod
     def _generate_key_map(
         entity_list: Iterable[Any], key: str, entity_class: Type[EntityClass], first_value: bool = False
@@ -794,38 +833,10 @@ def get_holdouts_for_flag(self, flag_key: str) -> list[Any]:
         Returns:
             The holdouts that apply for a specific flag.
         """
-        feature_flag = self.feature_key_map.get(flag_key)
-        if not feature_flag:
+        if not self.holdouts:
             return []
 
-        flag_id = feature_flag.id
-
-        # Check cache first
-        if flag_id in self.flag_holdouts_map:
-            return self.flag_holdouts_map[flag_id]
-
-        holdouts = []
-
-        # Add global holdouts that don't exclude this flag
-        for holdout in self.global_holdouts.values():
-            is_excluded = False
-            excluded_flags = holdout.get('excludedFlags')
-            if excluded_flags:
-                for excluded_flag_id in excluded_flags:
-                    if excluded_flag_id == flag_id:
-                        is_excluded = True
-                        break
-            if not is_excluded:
-                holdouts.append(holdout)
-
-        # Add holdouts that specifically include this flag
-        if flag_id in self.included_holdouts:
-            holdouts.extend(self.included_holdouts[flag_id])
-
-        # Cache the result
-        self.flag_holdouts_map[flag_id] = holdouts
-
-        return holdouts
+        return self.flag_holdouts_map.get(flag_key, [])
 
     def get_holdout(self, holdout_id: str) -> Optional[dict[str, Any]]:
         """ Helper method to get holdout from holdout ID.
diff --git a/tests/test_bucketing_holdout.py b/tests/test_bucketing_holdout.py
diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py
