diff --git a/lib/optimizely/config/datafile_project_config.rb b/lib/optimizely/config/datafile_project_config.rb index cd47d535..cf3a3629 100644 --- a/lib/optimizely/config/datafile_project_config.rb +++ b/lib/optimizely/config/datafile_project_config.rb @@ -33,8 +33,7 @@ class DatafileProjectConfig < ProjectConfig :group_id_map, :rollout_id_map, :rollout_experiment_id_map, :variation_id_map, :variation_id_to_variable_usage_map, :variation_key_map, :variation_id_map_by_experiment_id, :variation_key_map_by_experiment_id, :flag_variation_map, :integration_key_map, :integrations, - :public_key_for_odp, :host_for_odp, :all_segments, :region, :holdouts, :holdout_id_map, - :global_holdouts, :included_holdouts, :excluded_holdouts, :flag_holdouts_map + :public_key_for_odp, :host_for_odp, :all_segments, :region, :holdouts, :holdout_id_map # Boolean - denotes if Optimizely should remove the last block of visitors' IP address before storing event data attr_reader :anonymize_ip @@ -115,10 +114,6 @@ def initialize(datafile, logger, error_handler) @variation_id_to_experiment_map = {} @flag_variation_map = {} @holdout_id_map = {} - @global_holdouts = [] - @included_holdouts = {} - @excluded_holdouts = {} - @flag_holdouts_map = {} @holdouts.each do |holdout| next unless holdout['status'] == 'Running' @@ -127,31 +122,6 @@ def initialize(datafile, logger, error_handler) holdout['layerId'] ||= '' @holdout_id_map[holdout['id']] = holdout - - included_flags = holdout['includedFlags'] || [] - excluded_flags = holdout['excludedFlags'] || [] - - 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 @experiment_id_map.each_value do |exp| @@ -658,46 +628,6 @@ def rollout_experiment?(experiment_id) @rollout_experiment_id_map.key?(experiment_id) end - def get_holdouts_for_flag(flag_id) - # Helper method to get holdouts from an applied feature flag - # - # flag_id - (REQUIRED) ID of the feature flag - # This parameter is required and should not be null/nil - # - # Returns the holdouts that apply for a specific flag - - 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 - def get_holdout(holdout_id) # Helper method to get holdout from holdout ID # diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index 17a97358..38d10ab8 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -169,9 +169,10 @@ def get_variation_for_feature(project_config, feature_flag, user_context, decide # user_context - Optimizely user context instance # # Returns DecisionResult struct. - holdouts = project_config.get_holdouts_for_flag(feature_flag['id']) + # Get running holdouts from the holdout_id_map (all holdouts are global now) + running_holdouts = project_config.holdout_id_map.values - if holdouts && !holdouts.empty? + if running_holdouts && !running_holdouts.empty? # Has holdouts - use get_decision_for_flag which checks holdouts first get_decision_for_flag(feature_flag, user_context, project_config, decide_options) else @@ -195,8 +196,8 @@ def get_decision_for_flag(feature_flag, user_context, project_config, decide_opt reasons = decide_reasons ? decide_reasons.dup : [] user_id = user_context.user_id - # Check holdouts - holdouts = project_config.get_holdouts_for_flag(feature_flag['id']) + # Check holdouts (all holdouts are global now - apply to all flags) + holdouts = project_config.holdout_id_map.values holdouts.each do |holdout| holdout_decision = get_variation_for_holdout(holdout, user_context, project_config) diff --git a/lib/optimizely/helpers/constants.rb b/lib/optimizely/helpers/constants.rb index 86c32aa5..2cfe817b 100644 --- a/lib/optimizely/helpers/constants.rb +++ b/lib/optimizely/helpers/constants.rb @@ -346,14 +346,6 @@ module Constants }, 'status' => { 'type' => 'string' - }, - 'includedFlags' => { - 'type' => 'array', - 'items' => {'type' => 'string'} - }, - 'excludedFlags' => { - 'type' => 'array', - 'items' => {'type' => 'string'} } } } diff --git a/spec/config/datafile_project_config_spec.rb b/spec/config/datafile_project_config_spec.rb index 320d1c29..bb1a917b 100644 --- a/spec/config/datafile_project_config_spec.rb +++ b/spec/config/datafile_project_config_spec.rb @@ -1235,61 +1235,6 @@ end end - describe '#get_holdouts_for_flag' do - let(:config_with_holdouts) do - Optimizely::DatafileProjectConfig.new( - OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS_JSON, - logger, - error_handler - ) - end - - it 'should return empty array for non-existent flag' do - holdouts = config_with_holdouts.get_holdouts_for_flag('non_existent_flag') - expect(holdouts).to eq([]) - end - - it 'should return global holdouts that do not exclude the flag' do - 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' } - expect(global_holdout).not_to be_nil - expect(global_holdout['id']).to eq('holdout_1') - - specific_holdout = holdouts.find { |h| h['key'] == 'specific_holdout' } - expect(specific_holdout).not_to be_nil - expect(specific_holdout['id']).to eq('holdout_2') - end - - it 'should not return global holdouts that exclude the flag' do - 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 - 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 - 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) - expect(holdouts.first['key']).to eq('global_holdout') - end - end - describe '#get_holdout' do let(:config_with_holdouts) do Optimizely::DatafileProjectConfig.new( @@ -1329,9 +1274,7 @@ { 'id' => 'holdout_1', 'key' => 'test_holdout', - 'status' => 'Running', - 'includedFlags' => [], - 'excludedFlags' => [] + 'status' => 'Running' } ] config_json = JSON.dump(config_body_with_holdouts) @@ -1360,71 +1303,33 @@ end describe 'holdout initialization' do - let(:config_with_complex_holdouts) do + let(:config_with_simple_holdouts) do config_body_with_holdouts = config_body.dup - # Use the correct feature flag IDs from the debug output - boolean_feature_id = '155554' - multi_variate_feature_id = '155559' - empty_feature_id = '155564' - string_feature_id = '155557' - config_body_with_holdouts['holdouts'] = [ { 'id' => 'global_holdout', 'key' => 'global', - 'status' => 'Running', - 'includedFlags' => [], - 'excludedFlags' => [boolean_feature_id, string_feature_id] + 'status' => 'Running' }, { 'id' => 'specific_holdout', 'key' => 'specific', - 'status' => 'Running', - 'includedFlags' => [multi_variate_feature_id, empty_feature_id], - 'excludedFlags' => [] + 'status' => 'Running' }, { 'id' => 'inactive_holdout', 'key' => 'inactive', - 'status' => 'Inactive', - 'includedFlags' => [boolean_feature_id], - 'excludedFlags' => [] + 'status' => 'Inactive' } ] config_json = JSON.dump(config_body_with_holdouts) Optimizely::DatafileProjectConfig.new(config_json, logger, error_handler) end - 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.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 = '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 - expect(config_with_complex_holdouts.included_holdouts[empty_feature_id]).not_to be_nil - expect(config_with_complex_holdouts.included_holdouts[empty_feature_id]).not_to be_empty - expect(config_with_complex_holdouts.included_holdouts[boolean_feature_id]).to be_nil - - expect(config_with_complex_holdouts.excluded_holdouts[boolean_feature_id]).not_to be_nil - expect(config_with_complex_holdouts.excluded_holdouts[boolean_feature_id]).not_to be_empty - expect(config_with_complex_holdouts.excluded_holdouts[string_feature_id]).not_to be_nil - expect(config_with_complex_holdouts.excluded_holdouts[string_feature_id]).not_to be_empty - end - 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.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] - expect(included_for_boolean).to be_nil + expect(config_with_simple_holdouts.holdout_id_map.keys).to contain_exactly('global_holdout', 'specific_holdout') + expect(config_with_simple_holdouts.holdout_id_map['inactive_holdout']).to be_nil end end @@ -1454,83 +1359,6 @@ end end - describe '#decide with included flags holdout' do - it 'should return valid decision for included flags' do - feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] - expect(feature_flag).not_to be_nil - - # Check if there's a holdout that includes this flag - included_holdout = config_with_holdouts.holdouts.find do |h| - h['includedFlags']&.include?(feature_flag['id']) - end - - if included_holdout - expect(included_holdout['key']).not_to be_empty - expect(included_holdout['status']).to eq('Running') - end - end - - it 'should properly filter holdouts based on includedFlags' do - 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(feature_flag['id']) - expect(holdouts_for_flag).to be_an(Array) - end - end - - describe '#decide with excluded flags holdout' do - it 'should not return excluded holdout for excluded flag' do - # boolean_feature is excluded by holdout_excluded_1 - feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] - - if feature_flag - 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' } - expect(excluded_holdout).to be_nil - end - end - - it 'should return holdouts for non-excluded flag' do - 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(feature_flag['id']) - expect(holdouts_for_flag).to be_an(Array) - end - end - - describe '#decide with multiple holdouts' do - it 'should handle multiple holdouts for different flags' do - flag_keys = %w[boolean_feature multi_variate_feature string_single_variable_feature empty_feature] - - flag_keys.each do |flag_key| - feature_flag = config_with_holdouts.feature_flag_key_map[flag_key] - next unless feature_flag - - holdouts = config_with_holdouts.get_holdouts_for_flag(flag_key) - expect(holdouts).to be_an(Array) - - # Each holdout should have proper structure - holdouts.each do |holdout| - expect(holdout).to have_key('id') - expect(holdout).to have_key('key') - expect(holdout).to have_key('status') - end - end - end - - it 'should properly cache holdout lookups' do - 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 - end - describe '#decide with inactive holdout' do it 'should not include inactive holdouts in decision process' do # Find a holdout and verify status handling @@ -1579,31 +1407,6 @@ expect(feature_flag['key']).to eq('boolean_feature') end end - - describe '#holdout priority evaluation' do - it 'should evaluate global holdouts for flags without specific targeting' do - feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] - expect(feature_flag).not_to be_nil - - global_holdouts = config_with_holdouts.holdouts.select do |h| - h['includedFlags'].nil? || h['includedFlags'].empty? - end - - included_holdouts = config_with_holdouts.holdouts.select do |h| - h['includedFlags']&.include?(feature_flag['id']) - end - - # Should have either global or included holdouts - expect(global_holdouts.length + included_holdouts.length).to be >= 0 - end - - it 'should handle mixed holdout configurations' do - # Verify the config has properly categorized holdouts - 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 - end end describe 'Holdout Decision Reasons' do @@ -1704,9 +1507,10 @@ 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(feature_flag['id']) + # All holdouts are global now + holdouts = config_with_holdouts.holdout_id_map.values - holdouts_for_flag.each do |holdout| + holdouts.each do |holdout| # Each holdout should have necessary info for decision reasoning expect(holdout['id']).not_to be_empty expect(holdout['key']).not_to be_empty @@ -1735,17 +1539,13 @@ 'id' => 'holdout_1', 'key' => 'test_holdout', 'status' => 'Running', - 'audiences' => [], - 'includedFlags' => [], - 'excludedFlags' => [] + 'audiences' => [] }, { 'id' => 'holdout_2', 'key' => 'paused_holdout', 'status' => 'Paused', - 'audiences' => [], - 'includedFlags' => [], - 'excludedFlags' => [] + 'audiences' => [] } ] config_json = JSON.dump(config_body_with_holdouts) @@ -1759,28 +1559,26 @@ error_handler ) - 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([]) + # All holdouts are global now + holdouts = config_without_holdouts.holdout_id_map.values + expect(holdouts).to eq([]) end - it 'should handle holdouts with nil included/excluded flags' do - config_body_with_nil = config_body.dup - config_body_with_nil['holdouts'] = [ + it 'should handle holdouts correctly' do + config_body_with_holdout = config_body.dup + config_body_with_holdout['holdouts'] = [ { - 'id' => 'holdout_nil', - 'key' => 'nil_holdout', + 'id' => 'holdout_test', + 'key' => 'test_holdout', 'status' => 'Running', - 'audiences' => [], - 'includedFlags' => nil, - 'excludedFlags' => nil + 'audiences' => [] } ] - config_json = JSON.dump(config_body_with_nil) + config_json = JSON.dump(config_body_with_holdout) config = Optimizely::DatafileProjectConfig.new(config_json, logger, error_handler) - # Should treat as global holdout - expect(config.global_holdouts.find { |h| h['id'] == 'holdout_nil' }).not_to be_nil + # Should be in holdout_id_map + expect(config.holdout_id_map['holdout_test']).not_to be_nil end it 'should only include running holdouts in maps' do diff --git a/spec/decision_service_holdout_spec.rb b/spec/decision_service_holdout_spec.rb index 85191ce1..b2fc4571 100644 --- a/spec/decision_service_holdout_spec.rb +++ b/spec/decision_service_holdout_spec.rb @@ -99,20 +99,8 @@ feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] expect(feature_flag).not_to be_nil - # Find the most specific holdout for this flag (prefer explicitly included over global) - applicable_holdout = config_with_holdouts.holdouts.find do |holdout| - # First preference: holdout that explicitly includes this flag - holdout['includedFlags']&.include?(feature_flag['id']) - end - - # If no explicit holdout found, fall back to global holdouts - if applicable_holdout.nil? - applicable_holdout = config_with_holdouts.holdouts.find do |holdout| - # Global holdout (empty/nil includedFlags) that doesn't exclude this flag - (holdout['includedFlags'].nil? || holdout['includedFlags'].empty?) && - !holdout['excludedFlags']&.include?(feature_flag['id']) - end - end + # Get any running holdout (all holdouts are global now) + applicable_holdout = config_with_holdouts.holdout_id_map.values.first expect(applicable_holdout).not_to be_nil, 'No applicable holdout found for boolean_feature' @@ -371,10 +359,8 @@ feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] expect(feature_flag).not_to be_nil - # Get global holdouts - global_holdouts = config_with_holdouts.holdouts.select do |h| - h['includedFlags'].nil? || h['includedFlags'].empty? - end + # All holdouts are global now + global_holdouts = config_with_holdouts.holdout_id_map.values unless global_holdouts.empty? user_context = project_with_holdouts.create_user_context('testUserId', {}) @@ -390,20 +376,6 @@ expect(result).to be_an(Array) end end - - it 'should respect included and excluded flags configuration' do - # Test that flags in excludedFlags are not affected by that holdout - feature_flag = config_with_holdouts.feature_flag_key_map['boolean_feature'] - - if feature_flag - # Get holdouts for this flag - 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' } - expect(excluded_holdout).to be_nil - end - end end describe 'holdout logging and error handling' do diff --git a/spec/spec_params.rb b/spec/spec_params.rb index 6d7ff5c3..789c36df 100644 --- a/spec/spec_params.rb +++ b/spec/spec_params.rb @@ -1947,8 +1947,6 @@ module OptimizelySpec 'key' => 'global_holdout', 'status' => 'Running', 'audiences' => [], - 'includedFlags' => [], - 'excludedFlags' => ['155554'], 'variations' => [ { 'id' => 'var_1', @@ -1977,8 +1975,6 @@ module OptimizelySpec 'key' => 'boolean_feature_holdout', 'status' => 'Running', 'audiences' => [], - 'includedFlags' => ['155549'], - 'excludedFlags' => [], 'variations' => [ { 'id' => 'var_boolean', @@ -1998,8 +1994,6 @@ module OptimizelySpec 'key' => 'holdout_empty_1', 'status' => 'Running', 'audiences' => [], - 'includedFlags' => [], - 'excludedFlags' => [], 'variations' => [], 'trafficAllocation' => [] }, @@ -2008,8 +2002,6 @@ module OptimizelySpec 'key' => 'specific_holdout', 'status' => 'Running', 'audiences' => [], - 'includedFlags' => ['155559'], - 'excludedFlags' => [], 'variations' => [ { 'id' => 'var_3', @@ -2029,8 +2021,6 @@ module OptimizelySpec 'key' => 'inactive_holdout', 'status' => 'Inactive', 'audiences' => [], - 'includedFlags' => ['155554'], - 'excludedFlags' => [], 'variations' => [ { 'id' => 'var_4',