diff --git a/lib/optimizely/config/datafile_project_config.rb b/lib/optimizely/config/datafile_project_config.rb index 4e384175..8c74b9b5 100644 --- a/lib/optimizely/config/datafile_project_config.rb +++ b/lib/optimizely/config/datafile_project_config.rb @@ -33,8 +33,8 @@ 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, :rule_holdouts_map + :public_key_for_odp, :host_for_odp, :all_segments, :region, :holdouts, :local_holdouts, + :holdout_id_map, :global_holdouts, :rule_holdouts_map # Boolean - denotes if Optimizely should remove the last block of visitors' IP address before storing event data attr_reader :anonymize_ip @@ -71,7 +71,10 @@ def initialize(datafile, logger, error_handler) @send_flag_decisions = config.fetch('sendFlagDecisions', false) @integrations = config.fetch('integrations', []) @region = config.fetch('region', 'US') + # `holdouts` carries only global holdouts; `localHoldouts` (new top-level section) carries + # only rule-scoped local holdouts. Section membership is the sole signal for scope. @holdouts = config.fetch('holdouts', []) + @local_holdouts = config.fetch('localHoldouts', []) # Default to US region if not specified @region = 'US' if @region.nil? || @region.empty? @@ -118,25 +121,38 @@ def initialize(datafile, logger, error_handler) @global_holdouts = [] @rule_holdouts_map = {} + # Section membership determines scope: entries in `holdouts` are global, entries in + # `localHoldouts` are local. Any `includedRules` on a `holdouts` entry is stripped/ignored. @holdouts.each do |holdout| next unless holdout['status'] == 'Running' # Ensure holdout has layerId field (holdouts don't have campaigns) holdout['layerId'] ||= '' + # Strip includedRules from global-section entries — section membership is the sole signal. + holdout.delete('includedRules') @holdout_id_map[holdout['id']] = holdout + @global_holdouts << holdout + end - # Build global vs local holdout mappings - # A holdout is global when includedRules is nil/absent (applies to all rules) - # A holdout is local when includedRules is a non-nil array (applies only to specified rules) - if holdout_global?(holdout) - @global_holdouts << holdout - else - included_rules = holdout['includedRules'] || [] - included_rules.each do |rule_id| - @rule_holdouts_map[rule_id] ||= [] - @rule_holdouts_map[rule_id] << holdout - end + @local_holdouts.each do |holdout| + next unless holdout['status'] == 'Running' + + # Local holdouts without includedRules are invalid — log and skip (do not fall back to global). + included_rules = holdout['includedRules'] + if included_rules.nil? || !included_rules.is_a?(Array) || included_rules.empty? + @logger.log( + Logger::ERROR, + "Local holdout '#{holdout['key'] || holdout['id']}' is missing or has empty 'includedRules'; skipping." + ) + next + end + + holdout['layerId'] ||= '' + @holdout_id_map[holdout['id']] = holdout + included_rules.each do |rule_id| + @rule_holdouts_map[rule_id] ||= [] + @rule_holdouts_map[rule_id] << holdout end end @@ -217,10 +233,11 @@ def initialize(datafile, logger, error_handler) # Generate flag_variation_map after injection so it includes everyone-else variations @flag_variation_map = generate_feature_variation_map(@feature_flags) - # Adding Holdout variations in variation id and key maps - return unless @holdouts && !@holdouts.empty? + # Adding Holdout variations in variation id and key maps (both global and local sections). + all_holdouts = (@holdouts || []) + (@local_holdouts || []) + return if all_holdouts.empty? - @holdouts.each do |holdout| + all_holdouts.each do |holdout| next unless holdout['status'] == 'Running' holdout_key = holdout['key'] @@ -659,7 +676,7 @@ def get_holdout(holdout_id) end def get_holdouts_for_rule(rule_id) - # Returns running local holdouts that target a specific rule ID. + # Returns running local holdouts (from the `localHoldouts` section) that target the rule. # Local holdouts apply only to the rules listed in their includedRules array. # # rule_id - String ID of the experiment/delivery rule @@ -669,9 +686,9 @@ def get_holdouts_for_rule(rule_id) end def holdout_global?(holdout) - # Determines whether a holdout is global (applies to all rules) or local (applies to specific rules). - # A holdout is global when includedRules is nil or absent from the datafile. - # A holdout with an empty array [] is a local holdout with no matching rules (NOT global). + # Returns true when the holdout came from the global `holdouts` section. + # Section membership is the sole signal for scope; `ProjectConfig` strips `includedRules` + # from `holdouts`-section entries at parse time, so absence of the key is equivalent. # # holdout - Holdout hash from the datafile # diff --git a/lib/optimizely/helpers/constants.rb b/lib/optimizely/helpers/constants.rb index 4a5e40a4..b23955b1 100644 --- a/lib/optimizely/helpers/constants.rb +++ b/lib/optimizely/helpers/constants.rb @@ -352,6 +352,26 @@ module Constants } } } + }, + 'localHoldouts' => { + 'type' => 'array', + 'items' => { + 'type' => 'object', + 'properties' => { + 'id' => { + 'type' => 'string' + }, + 'key' => { + 'type' => 'string' + }, + 'status' => { + 'type' => 'string' + }, + 'includedRules' => { + 'type' => %w[array null] + } + } + } } }, 'required' => %w[ diff --git a/spec/config/datafile_project_config_spec.rb b/spec/config/datafile_project_config_spec.rb index 61d5370c..bf571ca7 100644 --- a/spec/config/datafile_project_config_spec.rb +++ b/spec/config/datafile_project_config_spec.rb @@ -1583,16 +1583,18 @@ it 'should only include running holdouts in maps' do running_count = config_with_holdouts.holdout_id_map.length - total_count = config_with_holdouts.holdouts.length + total_count = config_with_holdouts.holdouts.length + config_with_holdouts.local_holdouts.length - # Only running holdouts should be in the map + # Only running holdouts (across both sections) should be in the map expect(running_count).to be < total_count expect(config_with_holdouts.holdout_id_map['holdout_1']).not_to be_nil + # holdout_2 has status 'Inactive' so it should not be in the map expect(config_with_holdouts.holdout_id_map['holdout_2']).to be_nil end it 'should handle mixed status holdouts correctly' do - running_holdouts = config_with_holdouts.holdouts.select { |h| h['status'] == 'Running' } + all_holdouts = config_with_holdouts.holdouts + config_with_holdouts.local_holdouts + running_holdouts = all_holdouts.select { |h| h['status'] == 'Running' } running_holdouts.each do |holdout| expect(config_with_holdouts.get_holdout(holdout['id'])).not_to be_nil @@ -1936,7 +1938,8 @@ def build_datafile(experiments: [], rollouts: [], feature_flags: []) end end - # Level 1 — Local Holdouts config/parsing tests (FSSDK-12369) + # Level 1 — Local Holdouts config/parsing tests (FSSDK-12369, FSSDK-12760) + # Section membership (`holdouts` vs `localHoldouts`) is the sole signal for scope. describe 'local holdouts data model and config parsing' do let(:config_with_local_holdouts) do Optimizely::DatafileProjectConfig.new( @@ -1947,66 +1950,84 @@ def build_datafile(experiments: [], rollouts: [], feature_flags: []) end describe '#holdout_global?' do - it 'returns true for holdout with no includedRules key (old datafile format)' do + it 'returns true for entries from the global holdouts section' do global_holdout = config_with_local_holdouts.get_holdout('holdout_1') expect(global_holdout).not_to be_nil + # `includedRules` is stripped at parse time for global-section entries. expect(global_holdout.key?('includedRules')).to be false expect(config_with_local_holdouts.holdout_global?(global_holdout)).to be true end - it 'returns true for holdout with explicit nil includedRules' do + it 'returns true for holdout hash with explicit nil includedRules' do holdout = {'id' => 'test', 'key' => 'test', 'includedRules' => nil} expect(config_with_local_holdouts.holdout_global?(holdout)).to be true end - it 'returns false for holdout with non-nil includedRules array (local holdout)' do + it 'returns false for entries from the localHoldouts section' do local_holdout = config_with_local_holdouts.get_holdout('holdout_local_1') expect(local_holdout).not_to be_nil expect(config_with_local_holdouts.holdout_global?(local_holdout)).to be false end - - it 'returns false for holdout with empty includedRules array (local holdout with no matching rules)' do - local_holdout_empty = config_with_local_holdouts.get_holdout('holdout_local_empty_rules') - expect(local_holdout_empty).not_to be_nil - expect(local_holdout_empty['includedRules']).to eq([]) - # Empty array is local holdout (not global) — DIFFERENT from nil - expect(config_with_local_holdouts.holdout_global?(local_holdout_empty)).to be false - end end describe '#global_holdouts' do - it 'returns only holdouts without includedRules (global holdouts)' do + it 'returns only entries from the global holdouts section' do global_holdouts = config_with_local_holdouts.global_holdouts expect(global_holdouts).not_to be_nil expect(global_holdouts).to be_an(Array) - # All returned holdouts must be global + # All returned holdouts must be global (i.e. came from `holdouts` section). global_holdouts.each do |holdout| expect(config_with_local_holdouts.holdout_global?(holdout)).to be true end end - it 'does not include local holdouts (those with includedRules array)' do + it 'does not include entries from the localHoldouts section' do global_holdouts = config_with_local_holdouts.global_holdouts local_holdout = config_with_local_holdouts.get_holdout('holdout_local_1') expect(global_holdouts).not_to include(local_holdout) end - it 'does not include holdouts with empty includedRules array' do - global_holdouts = config_with_local_holdouts.global_holdouts - empty_local_holdout = config_with_local_holdouts.get_holdout('holdout_local_empty_rules') + it 'ignores includedRules on entries in the global holdouts section' do + # An includedRules field on a `holdouts` entry must NOT narrow its scope — + # section membership is the sole signal. + config = Optimizely::DatafileProjectConfig.new( + JSON.dump( + OptimizelySpec::VALID_CONFIG_BODY.merge( + 'holdouts' => [ + { + 'id' => 'stray_included_rules', + 'key' => 'stray_holdout', + 'status' => 'Running', + 'audiences' => [], + 'includedRules' => ['some_rule_id'], + 'variations' => [{'id' => 'v1', 'key' => 'holdout', 'featureEnabled' => false}], + 'trafficAllocation' => [{'entityId' => 'v1', 'endOfRange' => 10_000}] + } + ] + ) + ), + logger, + error_handler + ) - # Empty [] is local, not global — must not appear in global_holdouts - expect(global_holdouts).not_to include(empty_local_holdout) + holdout = config.get_holdout('stray_included_rules') + # includedRules was stripped at parse time + expect(holdout.key?('includedRules')).to be false + # Still classified as global + expect(config.global_holdouts).to include(holdout) + # And does not appear under any rule + expect(config.get_holdouts_for_rule('some_rule_id')).to eq([]) end it 'returns empty array when no global holdouts are present' do config_no_global = Optimizely::DatafileProjectConfig.new( JSON.dump( OptimizelySpec::VALID_CONFIG_BODY.merge( - 'holdouts' => [ + 'holdouts' => [], + 'localHoldouts' => [ { 'id' => 'only_local', 'key' => 'only_local_holdout', @@ -2039,11 +2060,10 @@ def build_datafile(experiments: [], rollouts: [], feature_flags: []) expect(rule_holdouts).to eq([]) end - it 'does not return global holdouts (those without includedRules)' do + it 'does not return entries from the global holdouts section' do rule_holdouts = config_with_local_holdouts.get_holdouts_for_rule('122227') global_holdout = config_with_local_holdouts.get_holdout('holdout_1') - # Global holdout must not appear in rule-specific list expect(rule_holdouts).not_to include(global_holdout) end @@ -2054,19 +2074,76 @@ def build_datafile(experiments: [], rollouts: [], feature_flags: []) expect(holdouts_for_rule_a).not_to include(local_holdout_2) end + end - it 'returns empty array for holdout with empty includedRules — does not match any rule' do - # holdout_local_empty_rules has includedRules: [] — should not appear for any rule - rule_holdouts = config_with_local_holdouts.get_holdouts_for_rule('122227') - empty_rules_holdout = config_with_local_holdouts.get_holdout('holdout_local_empty_rules') + describe 'invalid local holdouts — missing or empty includedRules' do + it 'logs an error and excludes localHoldouts entries with no includedRules' do + spy_logger = spy('logger') + config = Optimizely::DatafileProjectConfig.new( + JSON.dump( + OptimizelySpec::VALID_CONFIG_BODY.merge( + 'holdouts' => [], + 'localHoldouts' => [ + { + 'id' => 'invalid_no_rules', + 'key' => 'invalid_no_rules_key', + 'status' => 'Running', + 'audiences' => [], + 'variations' => [{'id' => 'v1', 'key' => 'holdout', 'featureEnabled' => false}], + 'trafficAllocation' => [{'entityId' => 'v1', 'endOfRange' => 10_000}] + } + ] + ) + ), + spy_logger, + error_handler + ) + + # Entry must be excluded from the holdout maps and not used for any rule. + expect(config.get_holdout('invalid_no_rules')).to be_nil + expect(config.global_holdouts).to eq([]) + expect(config.get_holdouts_for_rule('any_rule')).to eq([]) + # Error must be logged referencing the holdout key. + expect(spy_logger).to have_received(:log).with( + Logger::ERROR, + a_string_matching(/invalid_no_rules_key.*includedRules/i) + ) + end + + it 'logs an error and excludes localHoldouts entries with empty includedRules' do + spy_logger = spy('logger') + config = Optimizely::DatafileProjectConfig.new( + JSON.dump( + OptimizelySpec::VALID_CONFIG_BODY.merge( + 'holdouts' => [], + 'localHoldouts' => [ + { + 'id' => 'invalid_empty_rules', + 'key' => 'invalid_empty_rules_key', + 'status' => 'Running', + 'audiences' => [], + 'includedRules' => [], + 'variations' => [{'id' => 'v1', 'key' => 'holdout', 'featureEnabled' => false}], + 'trafficAllocation' => [{'entityId' => 'v1', 'endOfRange' => 10_000}] + } + ] + ) + ), + spy_logger, + error_handler + ) - expect(rule_holdouts).not_to include(empty_rules_holdout) + expect(config.get_holdout('invalid_empty_rules')).to be_nil + expect(spy_logger).to have_received(:log).with( + Logger::ERROR, + a_string_matching(/invalid_empty_rules_key.*includedRules/i) + ) end end - describe 'backward compatibility — old datafiles without includedRules' do - it 'parses holdouts without includedRules field as global (backward compatible)' do - # Use the config with only global holdouts (no includedRules key) + describe 'backward compatibility — old datafiles without localHoldouts' do + it 'parses datafiles without a localHoldouts section without errors' do + # Old datafile format: only the `holdouts` (global) section is present. config_global_only = Optimizely::DatafileProjectConfig.new( OptimizelySpec::CONFIG_BODY_WITH_GLOBAL_HOLDOUTS_ONLY_JSON, logger, @@ -2075,9 +2152,8 @@ def build_datafile(experiments: [], rollouts: [], feature_flags: []) holdout = config_global_only.get_holdout('global_holdout_only_1') expect(holdout).not_to be_nil - expect(holdout.key?('includedRules')).to be false - - # Must be classified as global + # No localHoldouts section means @local_holdouts defaults to []. + expect(config_global_only.local_holdouts).to eq([]) expect(config_global_only.holdout_global?(holdout)).to be true expect(config_global_only.global_holdouts).to include(holdout) expect(config_global_only.get_holdouts_for_rule('any_rule')).to eq([]) @@ -2093,5 +2169,24 @@ def build_datafile(experiments: [], rollouts: [], feature_flags: []) end.not_to raise_error end end + + describe 'localHoldouts section parsing (FSSDK-12760)' do + it 'exposes local_holdouts as a top-level attribute' do + expect(config_with_local_holdouts.local_holdouts).to be_an(Array) + expect(config_with_local_holdouts.local_holdouts.size).to eq(2) + end + + it 'parses entries from the localHoldouts section into the holdout maps' do + local_holdout = config_with_local_holdouts.get_holdout('holdout_local_1') + expect(local_holdout).not_to be_nil + expect(local_holdout['key']).to eq('local_holdout_rule_122227') + end + + it 'registers local holdout variations in variation maps' do + # Variation maps must include local holdouts so decision service can resolve their variations. + expect(config_with_local_holdouts.variation_id_map['local_holdout_rule_122227']).not_to be_nil + expect(config_with_local_holdouts.variation_id_map['local_holdout_rule_122227']['local_var_1']).not_to be_nil + end + end end end diff --git a/spec/spec_params.rb b/spec/spec_params.rb index 55982f32..1249f648 100644 --- a/spec/spec_params.rb +++ b/spec/spec_params.rb @@ -1947,7 +1947,7 @@ module OptimizelySpec 'key' => 'global_holdout', 'status' => 'Running', 'audiences' => [], - # No includedRules field => global holdout (applies to all rules) + # Entries in `holdouts` are global by section membership. 'variations' => [ { 'id' => 'var_1', @@ -1991,110 +1991,93 @@ module OptimizelySpec ] }, { - # Local holdout targeting experiment rule 122227 (used in boolean_feature) - 'id' => 'holdout_local_1', - 'key' => 'local_holdout_rule_122227', + 'id' => 'holdout_empty_1', + 'key' => 'holdout_empty_1', 'status' => 'Running', 'audiences' => [], - 'includedRules' => ['122227'], - 'variations' => [ - { - 'id' => 'local_var_1', - 'key' => 'holdout', - 'featureEnabled' => false - } - ], - 'trafficAllocation' => [ - { - 'entityId' => 'local_var_1', - 'endOfRange' => 10_000 - } - ] + 'variations' => [], + 'trafficAllocation' => [] }, { - # Local holdout targeting a different rule (122238), not rule 122227 - 'id' => 'holdout_local_2', - 'key' => 'local_holdout_rule_122238', + 'id' => 'holdout_2', + 'key' => 'specific_holdout', 'status' => 'Running', 'audiences' => [], - 'includedRules' => ['122238'], 'variations' => [ { - 'id' => 'local_var_2', - 'key' => 'holdout', + 'id' => 'var_3', + 'key' => 'control', 'featureEnabled' => false } ], 'trafficAllocation' => [ { - 'entityId' => 'local_var_2', + 'entityId' => 'var_3', 'endOfRange' => 10_000 } ] }, { - # Local holdout with empty includedRules array — local holdout with no matching rules (NOT global) - 'id' => 'holdout_local_empty_rules', - 'key' => 'local_holdout_empty_rules', - 'status' => 'Running', + 'id' => 'holdout_3', + 'key' => 'inactive_holdout', + 'status' => 'Inactive', 'audiences' => [], - 'includedRules' => [], 'variations' => [ { - 'id' => 'local_var_empty', - 'key' => 'holdout', + 'id' => 'var_4', + 'key' => 'off', 'featureEnabled' => false } ], 'trafficAllocation' => [ { - 'entityId' => 'local_var_empty', + 'entityId' => 'var_4', 'endOfRange' => 10_000 } ] - }, - { - 'id' => 'holdout_empty_1', - 'key' => 'holdout_empty_1', - 'status' => 'Running', - 'audiences' => [], - 'variations' => [], - 'trafficAllocation' => [] - }, + } + ], + # Top-level `localHoldouts` section — rule-scoped holdouts (Gen 3 datafile format). + # Older SDKs ignore this section; Gen 3 SDKs apply only to rules in includedRules. + 'localHoldouts' => [ { - 'id' => 'holdout_2', - 'key' => 'specific_holdout', + # Local holdout targeting experiment rule 122227 (used in boolean_feature) + 'id' => 'holdout_local_1', + 'key' => 'local_holdout_rule_122227', 'status' => 'Running', 'audiences' => [], + 'includedRules' => ['122227'], 'variations' => [ { - 'id' => 'var_3', - 'key' => 'control', + 'id' => 'local_var_1', + 'key' => 'holdout', 'featureEnabled' => false } ], 'trafficAllocation' => [ { - 'entityId' => 'var_3', + 'entityId' => 'local_var_1', 'endOfRange' => 10_000 } ] }, { - 'id' => 'holdout_3', - 'key' => 'inactive_holdout', - 'status' => 'Inactive', + # Local holdout targeting a different rule (122238), not rule 122227 + 'id' => 'holdout_local_2', + 'key' => 'local_holdout_rule_122238', + 'status' => 'Running', 'audiences' => [], + 'includedRules' => ['122238'], 'variations' => [ { - 'id' => 'var_4', - 'key' => 'off', + 'id' => 'local_var_2', + 'key' => 'holdout', 'featureEnabled' => false } ], 'trafficAllocation' => [ { - 'entityId' => 'var_4', + 'entityId' => 'local_var_2', 'endOfRange' => 10_000 } ]