Fix holdout mapping to return single holdout per flag with explicit precedence

lib/optimizely/config/datafile_project_config.rb

@@ -115,7 +115,7 @@ def initialize(datafile, logger, error_handler)
       @variation_id_to_experiment_map = {}
       @flag_variation_map = {}
       @holdout_id_map = {}
-      @global_holdouts = {}
+      @global_holdouts = []
       @included_holdouts = {}
       @excluded_holdouts = {}
       @flag_holdouts_map = {}
@@ -126,7 +126,7 @@ def initialize(datafile, logger, error_handler)
         @holdout_id_map[holdout['id']] = holdout
 
         if holdout['includedFlags'].nil? || holdout['includedFlags'].empty?
-          @global_holdouts[holdout['id']] = holdout
+          @global_holdouts << holdout
 
           excluded_flags = holdout['excludedFlags']
           if excluded_flags && !excluded_flags.empty?
@@ -196,16 +196,28 @@ def initialize(datafile, logger, error_handler)
         end
 
         flag_id = feature_flag['id']
-        applicable_holdouts = []
 
-        applicable_holdouts.concat(@included_holdouts[flag_id]) if @included_holdouts[flag_id]
+        # Collect all applicable holdouts for this flag
+        # Priority: global holdouts (excluding excluded flags) + included holdouts
+        applicable_holdouts = []
 
-        @global_holdouts.each_value do |holdout|
-          excluded_flag_ids = holdout['excludedFlags'] || []
-          applicable_holdouts << holdout unless excluded_flag_ids.include?(flag_id)
+        # Add all global holdouts that don't exclude this flag
+        excluded_for_flag = @excluded_holdouts[flag_id] || []
+        if excluded_for_flag.empty?
+          # No exclusions, add all global holdouts
+          applicable_holdouts.concat(@global_holdouts)
+        else
+          # Filter out excluded holdouts
+          @global_holdouts.each do |holdout|
+            applicable_holdouts << holdout unless excluded_for_flag.include?(holdout)
+          end
         end
 
-        @flag_holdouts_map[key] = applicable_holdouts unless applicable_holdouts.empty?
+        # Add all included holdouts for this flag
+        included = @included_holdouts[flag_id]
+        applicable_holdouts.concat(included) if included && !included.empty?
+
+        @flag_holdouts_map[flag_id] = applicable_holdouts unless applicable_holdouts.empty?
       end
 
       # Adding Holdout variations in variation id and key maps

lib/optimizely/decision_service.rb

@@ -312,14 +312,9 @@ def get_variations_for_feature_list(project_config, feature_flags, user_context,
 
       decisions = []
       feature_flags.each do |feature_flag|
-        # check if the feature is being experiment on and whether the user is bucketed into the experiment
-        decision_result = get_variation_for_feature_experiment(project_config, feature_flag, user_context, user_profile_tracker, decide_options)
-        # Only process rollout if no experiment decision was found and no error
-        if decision_result.decision.nil? && !decision_result.error
-          decision_result_rollout = get_variation_for_feature_rollout(project_config, feature_flag, user_context) unless decision_result.decision
-          decision_result.decision = decision_result_rollout.decision
-          decision_result.reasons.push(*decision_result_rollout.reasons)
-        end
+        # Always use get_decision_for_flag which checks holdouts, experiments, and rollouts in order
+        # This matches Swift SDK's getDecisionForFlag behavior (DefaultDecisionService.swift:381-419)
+        decision_result = get_decision_for_flag(feature_flag, user_context, project_config, decide_options, user_profile_tracker)
         decisions << decision_result
       end
       user_profile_tracker&.save_user_profile

spec/config/datafile_project_config_spec.rb

@@ -1249,40 +1249,60 @@
       expect(holdouts).to eq([])
     end
 
-    it 'should return global holdouts that do not exclude the flag' do
-      holdouts = config_with_holdouts.get_holdouts_for_flag('multi_variate_feature')
+    it 'should return all applicable holdouts for the flag' do
+      # get_holdouts_for_flag expects flag ID, not key
+      feature_flag = config_with_holdouts.feature_flag_key_map['multi_variate_feature']
+      holdouts = config_with_holdouts.get_holdouts_for_flag(feature_flag['id'])
+      # Should return global holdouts (not excluded) + included holdouts
+      # global_holdout (not excluded), holdout_empty_1 (global), specific_holdout (included)
       expect(holdouts.length).to eq(3)
 
-      global_holdout = holdouts.find { |h| h['key'] == 'global_holdout' }
-      expect(global_holdout).not_to be_nil
-      expect(global_holdout['id']).to eq('holdout_1')
-
+      # Should include the explicit holdout (specific_holdout)
       specific_holdout = holdouts.find { |h| h['key'] == 'specific_holdout' }
       expect(specific_holdout).not_to be_nil
       expect(specific_holdout['id']).to eq('holdout_2')
+
+      # Global holdouts should also be included (Swift SDK alignment)
+      global_holdout = holdouts.find { |h| h['key'] == 'global_holdout' }
+      expect(global_holdout).not_to be_nil
     end
 
     it 'should not return global holdouts that exclude the flag' do
-      holdouts = config_with_holdouts.get_holdouts_for_flag('boolean_single_variable_feature')
+      feature_flag = config_with_holdouts.feature_flag_key_map['boolean_single_variable_feature']
+      holdouts = config_with_holdouts.get_holdouts_for_flag(feature_flag['id'])
+      # Should only return holdout_empty_1 (global, not excluded)
+      # holdout_1 excludes this flag, so it shouldn't be included
       expect(holdouts.length).to eq(1)
 
       global_holdout = holdouts.find { |h| h['key'] == 'global_holdout' }
       expect(global_holdout).to be_nil
+
+      empty_holdout = holdouts.find { |h| h['key'] == 'holdout_empty_1' }
+      expect(empty_holdout).not_to be_nil
     end
 
-    it 'should cache results for subsequent calls' do
-      holdouts1 = config_with_holdouts.get_holdouts_for_flag('multi_variate_feature')
-      holdouts2 = config_with_holdouts.get_holdouts_for_flag('multi_variate_feature')
+    it 'should return consistent results for subsequent calls' do
+      feature_flag = config_with_holdouts.feature_flag_key_map['multi_variate_feature']
+      holdouts1 = config_with_holdouts.get_holdouts_for_flag(feature_flag['id'])
+      holdouts2 = config_with_holdouts.get_holdouts_for_flag(feature_flag['id'])
+      # Results are cached in @flag_holdouts_map
       expect(holdouts1).to equal(holdouts2)
       expect(holdouts1.length).to eq(3)
     end
 
-    it 'should return only global holdouts for flags not specifically targeted' do
-      holdouts = config_with_holdouts.get_holdouts_for_flag('string_single_variable_feature')
+    it 'should return all global holdouts for flags not specifically targeted' do
+      feature_flag = config_with_holdouts.feature_flag_key_map['string_single_variable_feature']
+      holdouts = config_with_holdouts.get_holdouts_for_flag(feature_flag['id'])
 
-      # Should only include global holdout (not excluded and no specific targeting)
+      # Should include all global holdouts (not excluded and no specific targeting)
+      # global_holdout + holdout_empty_1
       expect(holdouts.length).to eq(2)
-      expect(holdouts.first['key']).to eq('global_holdout')
+
+      global_holdout = holdouts.find { |h| h['key'] == 'global_holdout' }
+      expect(global_holdout).not_to be_nil
+
+      empty_holdout = holdouts.find { |h| h['key'] == 'holdout_empty_1' }
+      expect(empty_holdout).not_to be_nil
     end
   end
 
@@ -1394,7 +1414,11 @@
 
     it 'should properly categorize holdouts during initialization' do
       expect(config_with_complex_holdouts.holdout_id_map.keys).to contain_exactly('global_holdout', 'specific_holdout')
-      expect(config_with_complex_holdouts.global_holdouts.keys).to contain_exactly('global_holdout')
+
+      # global_holdouts is now an Array (Swift alignment)
+      expect(config_with_complex_holdouts.global_holdouts).to be_an(Array)
+      expect(config_with_complex_holdouts.global_holdouts.length).to eq(1)
+      expect(config_with_complex_holdouts.global_holdouts.first['id']).to eq('global_holdout')
 
       # Use the correct feature flag IDs
       boolean_feature_id = '155554'
@@ -1416,7 +1440,10 @@
 
     it 'should only process running holdouts during initialization' do
       expect(config_with_complex_holdouts.holdout_id_map['inactive_holdout']).to be_nil
-      expect(config_with_complex_holdouts.global_holdouts['inactive_holdout']).to be_nil
+
+      # global_holdouts is now an Array - check it doesn't contain inactive holdout
+      inactive_in_global = config_with_complex_holdouts.global_holdouts.any? { |h| h['id'] == 'inactive_holdout' }
+      expect(inactive_in_global).to be false
 
       boolean_feature_id = '155554'
       included_for_boolean = config_with_complex_holdouts.included_holdouts[boolean_feature_id]
@@ -1519,8 +1546,9 @@
       end
 
       it 'should properly cache holdout lookups' do
-        holdouts_1 = config_with_holdouts.get_holdouts_for_flag('boolean_feature')
-        holdouts_2 = config_with_holdouts.get_holdouts_for_flag('boolean_feature')
+        feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature']
+        holdouts_1 = config_with_holdouts.get_holdouts_for_flag(feature_flag['id'])
+        holdouts_2 = config_with_holdouts.get_holdouts_for_flag(feature_flag['id'])
 
         expect(holdouts_1).to equal(holdouts_2)
       end
@@ -1594,7 +1622,8 @@
 
       it 'should handle mixed holdout configurations' do
         # Verify the config has properly categorized holdouts
-        expect(config_with_holdouts.global_holdouts).to be_a(Hash)
+        # global_holdouts is an Array (Swift alignment), others are Hashes
+        expect(config_with_holdouts.global_holdouts).to be_an(Array)
         expect(config_with_holdouts.included_holdouts).to be_a(Hash)
         expect(config_with_holdouts.excluded_holdouts).to be_a(Hash)
       end
@@ -1773,8 +1802,9 @@
       config_json = JSON.dump(config_body_with_nil)
       config = Optimizely::DatafileProjectConfig.new(config_json, logger, error_handler)
 
-      # Should treat as global holdout
-      expect(config.global_holdouts['holdout_nil']).not_to be_nil
+      # Should treat as global holdout (global_holdouts is an Array)
+      holdout_found = config.global_holdouts.any? { |h| h['id'] == 'holdout_nil' }
+      expect(holdout_found).to be true
     end
 
     it 'should only include running holdouts in maps' do