diff --git a/lib/optimizely.rb b/lib/optimizely.rb index c64ea794..3d787c88 100644 --- a/lib/optimizely.rb +++ b/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 diff --git a/lib/optimizely/config/datafile_project_config.rb b/lib/optimizely/config/datafile_project_config.rb index d6f1d27b..cfb67f24 100644 --- a/lib/optimizely/config/datafile_project_config.rb +++ b/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 = {} @@ -123,23 +123,34 @@ def initialize(datafile, logger, error_handler) @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 diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index f1bc92e2..051a8b66 100644 --- a/lib/optimizely/decision_service.rb +++ b/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 diff --git a/spec/config/datafile_project_config_spec.rb b/spec/config/datafile_project_config_spec.rb index 84dd4509..ea47fe6f 100644 --- a/spec/config/datafile_project_config_spec.rb +++ b/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,7 +1264,8 @@ 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' } @@ -1271,14 +1273,16 @@ 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 diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index fe2cc881..30ad7d2e 100644 --- a/spec/decision_service_spec.rb +++ b/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