[FSSDK-12094] Fix Ruby holdout implementation which related to FSC failures (#382)

* Fix FSC tests

* Update holdout login on datafile_project_config

* Fix lint

* Update decision service

* Update optimizely

* Fix unit tests

* Fix unit tests

* Fix unit test failures

* Clear the duplicate code

* Fix unit and FSC errors

* Fix test

Author: esrakartalOpt

lib/optimizely.rb

@@ -220,7 +220,7 @@ def create_optimizely_decision(user_context, flag_key, decision, reasons, decide
         decision_source = decision.source
       end
 
-      if !decide_options.include?(OptimizelyDecideOption::DISABLE_DECISION_EVENT) && (decision_source == Optimizely::DecisionService::DECISION_SOURCES['FEATURE_TEST'] || config.send_flag_decisions)
+      if !decide_options.include?(OptimizelyDecideOption::DISABLE_DECISION_EVENT) && (decision_source == Optimizely::DecisionService::DECISION_SOURCES['FEATURE_TEST'] || decision_source == Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT'] || config.send_flag_decisions)
         send_impression(config, experiment, variation_key || '', flag_key, rule_key || '', feature_enabled, decision_source, user_id, attributes, decision&.cmab_uuid)
         decision_event_dispatched = true
       end

lib/optimizely/config/datafile_project_config.rb

@@ -115,31 +115,42 @@ 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 = {}
 
       @holdouts.each do |holdout|
         next unless holdout['status'] == 'Running'
 
+        # Ensure holdout has layerId field (holdouts don't have campaigns)
+        holdout['layerId'] ||= ''
+
         @holdout_id_map[holdout['id']] = holdout
 
-        if holdout['includedFlags'].nil? || holdout['includedFlags'].empty?
-          @global_holdouts[holdout['id']] = holdout
+        included_flags = holdout['includedFlags'] || []
+        excluded_flags = holdout['excludedFlags'] || []
 
-          excluded_flags = holdout['excludedFlags']
-          if excluded_flags && !excluded_flags.empty?
-            excluded_flags.each do |flag_id|
-              @excluded_holdouts[flag_id] ||= []
-              @excluded_holdouts[flag_id] << holdout
-            end
-          end
-        else
-          holdout['includedFlags'].each do |flag_id|
+        case [included_flags.empty?, excluded_flags.empty?]
+        when [true, true]
+          # No included or excluded flags - this is a global holdout
+          @global_holdouts << holdout
+
+        when [false, true], [false, false]
+          # Has included flags - add to included_holdouts map
+          included_flags.each do |flag_id|
             @included_holdouts[flag_id] ||= []
             @included_holdouts[flag_id] << holdout
           end
+
+        when [true, false]
+          # No included flags but has excluded flags - global with exclusions
+          @global_holdouts << holdout
+
+          excluded_flags.each do |flag_id|
+            @excluded_holdouts[flag_id] ||= []
+            @excluded_holdouts[flag_id] << holdout
+          end
         end
       end
 
@@ -194,18 +205,6 @@ def initialize(datafile, logger, error_handler)
         feature_flag['experimentIds'].each do |experiment_id|
           @experiment_feature_map[experiment_id] = [feature_flag['id']]
         end
-
-        flag_id = feature_flag['id']
-        applicable_holdouts = []
-
-        applicable_holdouts.concat(@included_holdouts[flag_id]) if @included_holdouts[flag_id]
-
-        @global_holdouts.each_value do |holdout|
-          excluded_flag_ids = holdout['excludedFlags'] || []
-          applicable_holdouts << holdout unless excluded_flag_ids.include?(flag_id)
-        end
-
-        @flag_holdouts_map[key] = applicable_holdouts unless applicable_holdouts.empty?
       end
 
       # Adding Holdout variations in variation id and key maps
@@ -645,6 +644,33 @@ def get_holdouts_for_flag(flag_id)
 
       return [] if @holdouts.nil? || @holdouts.empty?
 
+      # Check cache first (before validation, so we cache the validation result too)
+      return @flag_holdouts_map[flag_id] if @flag_holdouts_map.key?(flag_id)
+
+      # Validate that the flag exists in the datafile
+      flag_exists = @feature_flags.any? { |flag| flag['id'] == flag_id }
+      unless flag_exists
+        # Cache the empty result for non-existent flags
+        @flag_holdouts_map[flag_id] = []
+        return []
+      end
+
+      # Prioritize global holdouts first
+      excluded = @excluded_holdouts[flag_id] || []
+
+      active_holdouts = if excluded.any?
+                          @global_holdouts.reject { |holdout| excluded.include?(holdout) }
+                        else
+                          @global_holdouts.dup
+                        end
+
+      # Append included holdouts
+      included = @included_holdouts[flag_id] || []
+      active_holdouts.concat(included)
+
+      # Cache the result
+      @flag_holdouts_map[flag_id] = active_holdouts
+
       @flag_holdouts_map[flag_id] || []
     end
 

lib/optimizely/decision_service.rb

@@ -214,6 +214,9 @@ def get_decision_for_flag(feature_flag, user_context, project_config, decide_opt
 
       return DecisionResult.new(experiment_decision.decision, experiment_decision.error, reasons) if experiment_decision.decision
 
+      # If there's an error (e.g., CMAB error), return immediately without falling back to rollout
+      return DecisionResult.new(nil, experiment_decision.error, reasons) if experiment_decision.error
+
       # Check if the feature flag has a rollout and the user is bucketed into that rollout
       rollout_decision = get_variation_for_feature_rollout(project_config, feature_flag, user_context)
       reasons.push(*rollout_decision.reasons)
@@ -312,15 +315,8 @@ 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
-        decisions << decision_result
+        decision = get_decision_for_flag(feature_flag, user_context, project_config, decide_options, user_profile_tracker)
+        decisions << decision
       end
       user_profile_tracker&.save_user_profile
       decisions

spec/config/datafile_project_config_spec.rb

@@ -1250,7 +1250,8 @@
     end
 
     it 'should return global holdouts that do not exclude the flag' do
-      holdouts = config_with_holdouts.get_holdouts_for_flag('multi_variate_feature')
+      multi_variate_feature_id = '155559'
+      holdouts = config_with_holdouts.get_holdouts_for_flag(multi_variate_feature_id)
       expect(holdouts.length).to eq(3)
 
       global_holdout = holdouts.find { |h| h['key'] == 'global_holdout' }
@@ -1263,22 +1264,25 @@
     end
 
     it 'should not return global holdouts that exclude the flag' do
-      holdouts = config_with_holdouts.get_holdouts_for_flag('boolean_single_variable_feature')
+      boolean_feature_id = '155554'
+      holdouts = config_with_holdouts.get_holdouts_for_flag(boolean_feature_id)
       expect(holdouts.length).to eq(1)
 
       global_holdout = holdouts.find { |h| h['key'] == 'global_holdout' }
       expect(global_holdout).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')
+      multi_variate_feature_id = '155559'
+      holdouts1 = config_with_holdouts.get_holdouts_for_flag(multi_variate_feature_id)
+      holdouts2 = config_with_holdouts.get_holdouts_for_flag(multi_variate_feature_id)
       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')
+      string_feature_id = '155557'
+      holdouts = config_with_holdouts.get_holdouts_for_flag(string_feature_id)
 
       # Should only include global holdout (not excluded and no specific targeting)
       expect(holdouts.length).to eq(2)
@@ -1362,8 +1366,8 @@
       # Use the correct feature flag IDs from the debug output
       boolean_feature_id = '155554'
       multi_variate_feature_id = '155559'
-      empty_feature_id = '594032'
-      string_feature_id = '594060'
+      empty_feature_id = '155564'
+      string_feature_id = '155557'
 
       config_body_with_holdouts['holdouts'] = [
         {
@@ -1394,13 +1398,13 @@
 
     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')
+      expect(config_with_complex_holdouts.global_holdouts.map { |h| h['id'] }).to contain_exactly('global_holdout')
 
       # Use the correct feature flag IDs
       boolean_feature_id = '155554'
       multi_variate_feature_id = '155559'
-      empty_feature_id = '594032'
-      string_feature_id = '594060'
+      empty_feature_id = '155564'
+      string_feature_id = '155557'
 
       expect(config_with_complex_holdouts.included_holdouts[multi_variate_feature_id]).not_to be_nil
       expect(config_with_complex_holdouts.included_holdouts[multi_variate_feature_id]).not_to be_empty
@@ -1416,7 +1420,7 @@
 
     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
+      expect(config_with_complex_holdouts.global_holdouts.find { |h| h['id'] == 'inactive_holdout' }).to be_nil
 
       boolean_feature_id = '155554'
       included_for_boolean = config_with_complex_holdouts.included_holdouts[boolean_feature_id]
@@ -1470,7 +1474,7 @@
         feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature']
         expect(feature_flag).not_to be_nil
 
-        holdouts_for_flag = config_with_holdouts.get_holdouts_for_flag('boolean_feature')
+        holdouts_for_flag = config_with_holdouts.get_holdouts_for_flag(feature_flag['id'])
         expect(holdouts_for_flag).to be_an(Array)
       end
     end
@@ -1481,7 +1485,7 @@
         feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature']
 
         if feature_flag
-          holdouts_for_flag = config_with_holdouts.get_holdouts_for_flag('boolean_feature')
+          holdouts_for_flag = config_with_holdouts.get_holdouts_for_flag(feature_flag['id'])
 
           # Should not include holdouts that exclude this flag
           excluded_holdout = holdouts_for_flag.find { |h| h['key'] == 'excluded_holdout' }
@@ -1493,7 +1497,7 @@
         feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature']
         expect(feature_flag).not_to be_nil
 
-        holdouts_for_flag = config_with_holdouts.get_holdouts_for_flag('boolean_feature')
+        holdouts_for_flag = config_with_holdouts.get_holdouts_for_flag(feature_flag['id'])
         expect(holdouts_for_flag).to be_an(Array)
       end
     end
@@ -1519,8 +1523,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 +1599,7 @@
 
       it 'should handle mixed holdout configurations' do
         # Verify the config has properly categorized holdouts
-        expect(config_with_holdouts.global_holdouts).to be_a(Hash)
+        expect(config_with_holdouts.global_holdouts).to be_a(Array)
         expect(config_with_holdouts.included_holdouts).to be_a(Hash)
         expect(config_with_holdouts.excluded_holdouts).to be_a(Hash)
       end
@@ -1699,7 +1704,7 @@
         feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature']
         expect(feature_flag).not_to be_nil
 
-        holdouts_for_flag = config_with_holdouts.get_holdouts_for_flag('boolean_feature')
+        holdouts_for_flag = config_with_holdouts.get_holdouts_for_flag(feature_flag['id'])
 
         holdouts_for_flag.each do |holdout|
           # Each holdout should have necessary info for decision reasoning
@@ -1754,7 +1759,8 @@
         error_handler
       )
 
-      holdouts_for_flag = config_without_holdouts.get_holdouts_for_flag('boolean_feature')
+      feature_flag = config_without_holdouts.feature_flag_key_map['boolean_feature']
+      holdouts_for_flag = config_without_holdouts.get_holdouts_for_flag(feature_flag['id'])
       expect(holdouts_for_flag).to eq([])
     end
 
@@ -1774,7 +1780,7 @@
       config = Optimizely::DatafileProjectConfig.new(config_json, logger, error_handler)
 
       # Should treat as global holdout
-      expect(config.global_holdouts['holdout_nil']).not_to be_nil
+      expect(config.global_holdouts.find { |h| h['id'] == 'holdout_nil' }).not_to be_nil
     end
 
     it 'should only include running holdouts in maps' do

spec/decision_service_spec.rb

@@ -775,7 +775,7 @@
 
           decision_result = decision_service.get_variation_for_feature(config, feature_flag, user_context)
           expect(decision_result.decision).to eq(expected_decision)
-          expect(decision_result.reasons).to eq([])
+          expect(decision_result.reasons).to eq(["The user 'user_1' is bucketed into a rollout for feature flag 'string_single_variable_feature'."])
         end
       end