diff --git a/lib/optimizely.rb b/lib/optimizely.rb index 38e6a2c1..8750fbac 100644 --- a/lib/optimizely.rb +++ b/lib/optimizely.rb @@ -307,7 +307,11 @@ def get_enabled_features(user_id, attributes = nil) return enabled_features end - return enabled_features unless Optimizely::Helpers::Validator.inputs_valid?({user_id: user_id}, @logger, Logger::ERROR) + return enabled_features unless Optimizely::Helpers::Validator.inputs_valid?( + { + user_id: user_id + }, @logger, Logger::ERROR + ) return enabled_features unless user_inputs_valid?(attributes) @@ -448,8 +452,8 @@ def get_feature_variable_for_type(feature_flag_key, variable_key, variable_type, { feature_flag_key: feature_flag_key, variable_key: variable_key, - variable_type: variable_type, - user_id: user_id + user_id: user_id, + variable_type: variable_type }, @logger, Logger::ERROR ) diff --git a/lib/optimizely/helpers/validator.rb b/lib/optimizely/helpers/validator.rb index 77a2134b..f1185320 100644 --- a/lib/optimizely/helpers/validator.rb +++ b/lib/optimizely/helpers/validator.rb @@ -121,13 +121,22 @@ def inputs_valid?(variables, logger = NoOpLogger.new, level = Logger::ERROR) return false unless variables.respond_to?(:each) && !variables.empty? is_valid = true + if variables.include? :user_id + # Empty str is a valid user ID. + unless variables[:user_id].is_a?(String) + is_valid = false + logger.log(level, "#{Constants::INPUT_VARIABLES['USER_ID']} is invalid") + end + variables.delete :user_id + end + variables.each do |key, value| next if value.is_a?(String) && !value.empty? is_valid = false next unless logger_valid?(logger) && level - logger.log(level, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES[key.to_s.upcase]} is invalid") + logger.log(level, "#{Constants::INPUT_VARIABLES[key.to_s.upcase]} is invalid") end is_valid end diff --git a/lib/optimizely/project_config.rb b/lib/optimizely/project_config.rb index 23a52ca5..3a90c6be 100644 --- a/lib/optimizely/project_config.rb +++ b/lib/optimizely/project_config.rb @@ -329,7 +329,7 @@ def set_forced_variation(experiment_key, user_id, variation_key) # # Returns a boolean value that indicates if the set completed successfully. - input_values = {user_id: user_id, experiment_key: experiment_key} + input_values = {experiment_key: experiment_key, user_id: user_id} input_values[:variation_key] = variation_key unless variation_key.nil? return false unless Optimizely::Helpers::Validator.inputs_valid?(input_values, @logger, Logger::DEBUG) diff --git a/spec/project_config_spec.rb b/spec/project_config_spec.rb index 1172a969..b0fcae3f 100644 --- a/spec/project_config_spec.rb +++ b/spec/project_config_spec.rb @@ -859,12 +859,6 @@ expect(spy_logger).to have_received(:log).with(Logger::DEBUG, 'User ID is invalid') end - # User ID is an empty string - it 'should log a message and return nil when user_id is passed as empty string for get_forced_variation' do - expect(config.get_forced_variation(@valid_experiment[:key], '')).to eq(nil) - expect(spy_logger).to have_received(:log).with(Logger::DEBUG, - 'User ID is invalid') - end # User ID is not defined in the forced variation map it 'should log a message and return nil when user is not in forced variation map' do expect(config.get_forced_variation(@valid_experiment[:key], @user_id)).to eq(nil) @@ -905,12 +899,6 @@ expect(spy_logger).to have_received(:log).with(Logger::DEBUG, 'User ID is invalid') end - # User ID is an empty string - it 'should log a message and return false when user_id is passed as empty string' do - expect(config.set_forced_variation(@valid_experiment[:key], '', @valid_variation[:key])).to eq(false) - expect(spy_logger).to have_received(:log).with(Logger::DEBUG, - 'User ID is invalid') - end # Experiment key is nil it 'should return false when experiment_key is passed as nil' do expect(config.set_forced_variation(nil, @user_id, @valid_variation[:key])).to eq(false) @@ -961,14 +949,24 @@ it 'should call inputs_valid? with the proper arguments in set_forced_variation' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - user_id: @user_id, experiment_key: @valid_experiment[:key], + user_id: @user_id, variation_key: @valid_variation[:key] }, spy_logger, Logger::DEBUG ) config.set_forced_variation(@valid_experiment[:key], @user_id, @valid_variation[:key]) end + it 'should log and return false when user ID is non string in set_forced_variation' do + expect(config.set_forced_variation(@valid_experiment[:key], nil, @valid_variation[:key])).to be false + expect(config.set_forced_variation(@valid_experiment[:key], 5, @valid_variation[:key])).to be false + expect(config.set_forced_variation(@valid_experiment[:key], 5.5, @valid_variation[:key])).to be false + expect(config.set_forced_variation(@valid_experiment[:key], true, @valid_variation[:key])).to be false + expect(config.set_forced_variation(@valid_experiment[:key], {}, @valid_variation[:key])).to be false + expect(config.set_forced_variation(@valid_experiment[:key], [], @valid_variation[:key])).to be false + expect(spy_logger).to have_received(:log).with(Logger::DEBUG, 'User ID is invalid').exactly(6).times + end + it 'should call inputs_valid? with the proper arguments in get_forced_variation' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { @@ -979,6 +977,16 @@ config.get_forced_variation(@valid_experiment[:key], @user_id) end + it 'should log and return nil when user ID is non string in get_forced_variation' do + expect(config.get_forced_variation(@valid_experiment[:key], nil)).to eq(nil) + expect(config.get_forced_variation(@valid_experiment[:key], 5)).to eq(nil) + expect(config.get_forced_variation(@valid_experiment[:key], 5.5)).to eq(nil) + expect(config.get_forced_variation(@valid_experiment[:key], true)).to eq(nil) + expect(config.get_forced_variation(@valid_experiment[:key], {})).to eq(nil) + expect(config.get_forced_variation(@valid_experiment[:key], [])).to eq(nil) + expect(spy_logger).to have_received(:log).with(Logger::DEBUG, 'User ID is invalid').exactly(6).times + end + # Call set variation with different variations on one user/experiment to confirm that each set is expected. it 'should set and return expected variations when different variations are set and removed for one user/experiment' do expect(config.set_forced_variation(@valid_experiment[:key], @user_id, @valid_variation[:key])).to eq(true) diff --git a/spec/project_spec.rb b/spec/project_spec.rb index be3db78d..cbc63a7f 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -364,6 +364,16 @@ class InvalidErrorHandler; end project_instance.activate('test_experiment_with_audience', 'test_user') end + it 'should log return nil when user ID is non string' do + expect(project_instance.activate('test_experiment_with_audience', nil)).to eq(nil) + expect(project_instance.activate('test_experiment_with_audience', 5)).to eq(nil) + expect(project_instance.activate('test_experiment_with_audience', 5.5)).to eq(nil) + expect(project_instance.activate('test_experiment_with_audience', true)).to eq(nil) + expect(project_instance.activate('test_experiment_with_audience', {})).to eq(nil) + expect(project_instance.activate('test_experiment_with_audience', [])).to eq(nil) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(6).times + end + it 'should return false when invalid inputs are passed' do expect(Optimizely::Helpers::Validator.inputs_valid?({})).to eq(false) expect(Optimizely::Helpers::Validator.inputs_valid?([])).to eq(false) @@ -376,38 +386,37 @@ class InvalidErrorHandler; end end it 'should log and return false when non string value inputs are passed' do - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: nil}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: ''}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: []}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: {}}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: 2}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: 2.0}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: true}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: false}, spy_logger, Logger::ERROR)).to eq(false) - expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(8).times + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: nil}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: []}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: {}}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: 2}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: 2.0}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: true}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: false}, spy_logger, Logger::ERROR)).to eq(false) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'Experiment key is invalid').exactly(7).times end it 'should log and return false when multiple non string value inputs are passed' do - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: '', experiment_key: true}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: [], variation_key: 2.0}, spy_logger, Logger::ERROR)).to eq(false) - expect(spy_logger).to have_received(:log).twice.with(Logger::ERROR, 'User ID is invalid') + expect(Optimizely::Helpers::Validator.inputs_valid?({variable_key: nil, experiment_key: true}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({variable_key: [], variation_key: 2.0}, spy_logger, Logger::ERROR)).to eq(false) + expect(spy_logger).to have_received(:log).twice.with(Logger::ERROR, 'Variable key is invalid') expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Experiment key is invalid') expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Variation key is invalid') end it 'should return true when valid input values are passed' do - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: '2'}, spy_logger, Logger::ERROR)).to eq(true) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: '2'}, spy_logger, Logger::ERROR)).to eq(true) expect(Optimizely::Helpers::Validator.inputs_valid?({ - user_id: 'test_user', + variable_key: 'test_variable', experiment_key: 'test_experiment', variation_key: 'test_variation' }, spy_logger, Logger::ERROR)).to eq(true) end it 'should not log when logger or level are nil' do - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: nil}, nil, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: nil}, spy_logger, nil)).to eq(false) - expect(spy_logger).not_to have_received(:log).with(Logger::ERROR, 'User ID is invalid') + expect(Optimizely::Helpers::Validator.inputs_valid?({variable_key: nil}, nil, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({variable_key: nil}, spy_logger, nil)).to eq(false) + expect(spy_logger).not_to have_received(:log).with(Logger::ERROR, 'Variable key is invalid') end it 'should return nil when user is in no variation' do @@ -524,22 +533,31 @@ class InvalidErrorHandler; end } end - it 'should return nil when user_id is empty or nil' do - expect(project_instance.track('test_event', '', nil, 'revenue' => 42)).to eq(nil) + it 'should return nil when user_id is nil' do expect(project_instance.track('test_event', nil, nil, 'revenue' => 42)).to eq(nil) - expect(spy_logger).to have_received(:log).twice.with(Logger::ERROR, 'User ID is invalid') + expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'User ID is invalid') end it 'should call inputs_valid? with the proper arguments in track' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - user_id: 'test_user', - event_key: 'test_event' + event_key: 'test_event', + user_id: 'test_user' }, spy_logger, Logger::ERROR ) project_instance.track('test_event', 'test_user') end + it 'should log and return nil when user ID is non string' do + expect(project_instance.track('test_event', nil)).to eq(nil) + expect(project_instance.track('test_event', 5)).to eq(nil) + expect(project_instance.track('test_event', 5.5)).to eq(nil) + expect(project_instance.track('test_event', true)).to eq(nil) + expect(project_instance.track('test_event', {})).to eq(nil) + expect(project_instance.track('test_event', [])).to eq(nil) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(6).times + end + it 'should properly track an event by calling dispatch_event with right params' do params = @expected_track_event_params @@ -713,22 +731,26 @@ class InvalidErrorHandler; end end describe '#get_variation' do - it 'should return nil when user_id is empty or nil' do - expect(project_instance.get_variation('test_experiment_with_audience', '', nil)).to eq(nil) - expect(project_instance.get_variation('test_experiment_with_audience', nil, nil)).to eq(nil) - expect(spy_logger).to have_received(:log).twice.with(Logger::ERROR, 'User ID is invalid') - end - it 'should call inputs_valid? with the proper arguments in get_variation' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - user_id: 'test_user', - experiment_key: 'test_experiment_with_audience' + experiment_key: 'test_experiment_with_audience', + user_id: 'test_user' }, spy_logger, Logger::ERROR ) project_instance.get_variation('test_experiment_with_audience', 'test_user', nil) end + it 'should log and return nil when user ID is non string' do + expect(project_instance.get_variation('test_experiment_with_audience', nil)).to eq(nil) + expect(project_instance.get_variation('test_experiment_with_audience', 5)).to eq(nil) + expect(project_instance.get_variation('test_experiment_with_audience', 5.5)).to eq(nil) + expect(project_instance.get_variation('test_experiment_with_audience', true)).to eq(nil) + expect(project_instance.get_variation('test_experiment_with_audience', {})).to eq(nil) + expect(project_instance.get_variation('test_experiment_with_audience', [])).to eq(nil) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(6).times + end + it 'should have get_variation return expected variation when there are no audiences' do expect(project_instance.get_variation('test_experiment', 'test_user')) .to eq(config_body['experiments'][0]['variations'][0]['key']) @@ -857,12 +879,6 @@ class InvalidErrorHandler; end expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Feature flag key is invalid') end - it 'should return false when user_id is empty or nil' do - expect(project_instance.is_feature_enabled('boolean_single_variable_feature', '')).to be false - expect(project_instance.is_feature_enabled('boolean_single_variable_feature', nil)).to be false - expect(spy_logger).to have_received(:log).twice.with(Logger::ERROR, 'User ID is invalid') - end - it 'should return false when the feature flag key is invalid' do expect(project_instance.is_feature_enabled('totally_invalid_feature_key', 'test_user')).to be false expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, "Feature flag key 'totally_invalid_feature_key' is not in datafile.") @@ -879,6 +895,22 @@ class InvalidErrorHandler; end project_instance.is_feature_enabled('multi_variate_feature', 'test_user') end + it 'should log and return false when user ID is non string' do + expect(project_instance.is_feature_enabled('multi_variate_feature', nil)).to be(false) + expect(project_instance.is_feature_enabled('multi_variate_feature', 5)).to be(false) + expect(project_instance.is_feature_enabled('multi_variate_feature', 5.5)).to be(false) + expect(project_instance.is_feature_enabled('multi_variate_feature', true)).to be(false) + expect(project_instance.is_feature_enabled('multi_variate_feature', {})).to be(false) + expect(project_instance.is_feature_enabled('multi_variate_feature', [])).to be(false) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(6).times + end + + it 'should return false when attributes are invalid' do + expect(Optimizely::Helpers::Validator).to receive(:attributes_valid?).once.with('invalid') + expect(error_handler).to receive(:handle_error).once.with(Optimizely::InvalidAttributeFormatError) + expect(project_instance.is_feature_enabled('multi_variate_feature', 'test_user', 'invalid')).to be false + end + it 'should log and raise an exception when called with attributes in an invalid format' do expect_any_instance_of(Optimizely::RaiseErrorHandler).to receive(:handle_error).once.with(Optimizely::InvalidAttributeFormatError) expect(project_instance.is_feature_enabled('multi_variate_feature', 'test_user', 'invalid_attributes')).to be false @@ -989,15 +1021,35 @@ class InvalidErrorHandler; end expect(invalid_project.get_enabled_features('test_user')).to be_empty end + it 'should call inputs_valid? with the proper arguments in get_enabled_features' do + expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( + { + user_id: 'test_user' + }, spy_logger, Logger::ERROR + ) + project_instance.get_enabled_features('test_user') + end + it 'should return empty when no feature flag is enabled' do allow(project_instance).to receive(:is_feature_enabled).and_return(false) expect(project_instance.get_enabled_features('test_user')).to be_empty end - it 'should call inputs_valid? with the proper arguments in get_enabled_features' do - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: 'test_user'}, spy_logger, Logger::ERROR)).to eq(true) - expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with({user_id: 'test_user'}, spy_logger, Logger::ERROR) - project_instance.get_enabled_features('test_user', 'browser_type' => 'chrome') + it 'should log and return empty when user ID is non string' do + expect(project_instance.get_enabled_features(nil)).to be_empty + expect(project_instance.get_enabled_features(5)).to be_empty + expect(project_instance.get_enabled_features(5.5)).to be_empty + expect(project_instance.get_enabled_features(true)).to be_empty + expect(project_instance.get_enabled_features({})).to be_empty + expect(project_instance.get_enabled_features([])).to be_empty + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(6).times + end + + it 'should return empty when attributes are invalid' do + expect(Optimizely::Helpers::Validator).to receive(:attributes_valid?).once.with('invalid') + expect(error_handler).to receive(:handle_error).once.with(Optimizely::InvalidAttributeFormatError) + expect(project_instance.get_enabled_features('test_user', 'invalid')).to be_empty + expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Provided attributes are in an invalid format.') end it 'should log and raise an exception when called with attributes in an invalid format' do @@ -1302,30 +1354,47 @@ class InvalidErrorHandler; end expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Variable key is invalid') end - it 'should return nil if user_id is empty or nil' do - expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', nil, user_attributes)) - .to eq(nil) - expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', '', user_attributes)) - .to eq(nil) - expect(spy_logger).to have_received(:log).twice.with(Logger::ERROR, 'User ID is invalid') - end - it 'should call inputs_valid? with the proper arguments in get_feature_variable_for_type' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { feature_flag_key: 'integer_single_variable_feature', variable_key: 'integer_variable', - variable_type: 'integer', - user_id: nil + user_id: 'test_user', + variable_type: 'integer' }, spy_logger, Logger::ERROR ) - expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', nil, user_attributes)) - .to eq(nil) + project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', 'test_user', user_attributes) + end + + it 'should log and return nil when user ID is non string' do + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', nil)).to eq(nil) + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', 5)).to eq(nil) + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', 5.5)).to eq(nil) + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', true)).to eq(nil) + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', {})).to eq(nil) + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', [])).to eq(nil) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(6).times + end + end + + describe '#get_feature_variable_for_type with invalid attributes' do + it 'should return nil when attributes are invalid' do + expect(Optimizely::Helpers::Validator).to receive(:attributes_valid?).once.with('invalid') + expect(error_handler).to receive(:handle_error).once.with(Optimizely::InvalidAttributeFormatError) + expect(project_instance.send( + :get_feature_variable_for_type, + 'integer_single_variable_feature', + 'integer_variable', + 'integer', + 'test_user', + 'invalid' + )).to eq(nil) + expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Provided attributes are in an invalid format.') end it 'should log and raise an exception when called with attributes in an invalid format' do expect_any_instance_of(Optimizely::RaiseErrorHandler).to receive(:handle_error).once.with(Optimizely::InvalidAttributeFormatError) - expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', user_id, 'invalid_attributes')).to eq(nil) + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', 'test_user', 'invalid_attributes')).to eq(nil) expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Provided attributes are in an invalid format.') end end