diff --git a/lib/optimizely.rb b/lib/optimizely.rb index 0b095fcf..38e6a2c1 100644 --- a/lib/optimizely.rb +++ b/lib/optimizely.rb @@ -258,6 +258,8 @@ def is_feature_enabled(feature_flag_key, user_id, attributes = nil) }, @logger, Logger::ERROR ) + return false unless user_inputs_valid?(attributes) + feature_flag = @config.get_feature_flag_from_key(feature_flag_key) unless feature_flag @logger.log(Logger::ERROR, "No feature flag was found for key '#{feature_flag_key}'.") @@ -307,6 +309,8 @@ def get_enabled_features(user_id, attributes = nil) return enabled_features unless Optimizely::Helpers::Validator.inputs_valid?({user_id: user_id}, @logger, Logger::ERROR) + return enabled_features unless user_inputs_valid?(attributes) + @config.feature_flags.each do |feature| enabled_features.push(feature['key']) if is_feature_enabled( feature['key'], @@ -450,6 +454,8 @@ def get_feature_variable_for_type(feature_flag_key, variable_key, variable_type, @logger, Logger::ERROR ) + return nil unless user_inputs_valid?(attributes) + feature_flag = @config.get_feature_flag_from_key(feature_flag_key) unless feature_flag @logger.log(Logger::INFO, "No feature flag was found for key '#{feature_flag_key}'.") diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index 2afb3b85..4f0a242d 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -366,17 +366,18 @@ def get_bucketing_id(user_id, attributes) # # user_id - String user ID # attributes - Hash user attributes - # By default, the bucketing ID should be the user ID - bucketing_id = user_id + # Returns String representing bucketing ID if it is a String type in attributes else return user ID - # If the bucketing ID key is defined in attributes, then use that in place of the userID - if attributes && attributes[Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID']].is_a?(String) - unless attributes[Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID']].empty? - bucketing_id = attributes[Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID']] - @config.logger.log(Logger::DEBUG, "Setting the bucketing ID '#{bucketing_id}'") - end + return user_id unless attributes + + bucketing_id = attributes[Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID']] + + if bucketing_id + return bucketing_id if bucketing_id.is_a?(String) + + @config.logger.log(Logger::WARN, 'Bucketing ID attribute is not a string. Defaulted to user ID.') end - bucketing_id + user_id end end end diff --git a/lib/optimizely/event_builder.rb b/lib/optimizely/event_builder.rb index 34fa2176..57cec3c0 100644 --- a/lib/optimizely/event_builder.rb +++ b/lib/optimizely/event_builder.rb @@ -15,10 +15,12 @@ # See the License for the specific language governing permissions and # limitations under the License. # -require_relative './audience' -require_relative './params' -require_relative './version' -require_relative '../optimizely/helpers/event_tag_utils' +require_relative 'audience' +require_relative 'helpers/constants' +require_relative 'helpers/event_tag_utils' +require_relative 'params' +require_relative 'version' + require 'securerandom' module Optimizely @@ -74,9 +76,9 @@ def get_common_params(user_id, attributes) visitor_attributes = [] attributes&.keys&.each do |attribute_key| - # Omit null attribute values + # Omit attribute values that are not supported by the log endpoint. attribute_value = attributes[attribute_key] - unless attribute_value.nil? + if Helpers::Validator.attribute_valid?(attribute_key, attribute_value) attribute_id = @config.get_attribute_id attribute_key if attribute_id visitor_attributes.push( diff --git a/lib/optimizely/helpers/constants.rb b/lib/optimizely/helpers/constants.rb index c35fcd8b..4fd17c44 100644 --- a/lib/optimizely/helpers/constants.rb +++ b/lib/optimizely/helpers/constants.rb @@ -327,6 +327,8 @@ module Constants 'v3' => '3', 'v4' => '4' }.freeze + + ATTRIBUTE_VALID_TYPES = [FalseClass, Float, Integer, String, TrueClass].freeze end end end diff --git a/lib/optimizely/helpers/validator.rb b/lib/optimizely/helpers/validator.rb index 041e31aa..77a2134b 100644 --- a/lib/optimizely/helpers/validator.rb +++ b/lib/optimizely/helpers/validator.rb @@ -34,6 +34,19 @@ def attributes_valid?(attributes) attributes.is_a?(Hash) end + def attribute_valid?(attribute_key, attribute_value) + # Determines if provided attribute_key and attribute_value are valid. + # + # attribute_key - Variable which needs to be validated. + # attribute_value - Variable which needs to be validated. + # + # Returns boolean depending on validity of attribute_key and attribute_value. + + return false unless attribute_key.is_a?(String) || attribute_key.is_a?(Symbol) + + Helpers::Constants::ATTRIBUTE_VALID_TYPES.any? { |type| attribute_value.is_a?(type) } + end + def event_tags_valid?(event_tags) # Determines if provided event tags are valid. # diff --git a/spec/condition_spec.rb b/spec/condition_spec.rb index 8b95e872..f560f231 100644 --- a/spec/condition_spec.rb +++ b/spec/condition_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # -# Copyright 2016-2017, Optimizely and contributors +# Copyright 2016-2018, Optimizely and contributors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -145,4 +145,19 @@ condition = JSON.parse(condition) expect(@condition_evaluator.evaluate(condition)).to be true end + + it 'should evaluate to true when user attributes match the audience conditions' do + user_attributes = { + 'device_type' => 'iPhone', + 'is_firefox' => false, + 'num_users' => 15, + 'pi_value' => 3.14 + } + condition_evaluator = Optimizely::ConditionEvaluator.new(user_attributes) + condition = '["and", ["or", ["or", {"name": "device_type", "type": "custom_attribute", "value": "iPhone"}]], + ["or", ["or", {"name": "is_firefox", "type": "custom_attribute", "value": false}]], ["or", ["or", {"name": "num_users", + "type": "custom_attribute", "value": 15}]], ["or", ["or", {"name": "pi_value", "type": "custom_attribute", "value": 3.14}]]]' + condition = JSON.parse(condition) + expect(condition_evaluator.evaluate(condition)).to be true + end end diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index 975ef5c7..701720df 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -676,4 +676,41 @@ end end end + describe '#get_bucketing_id' do + it 'should log a message and return user ID when bucketing ID is not a String' do + user_attributes = { + 'browser_type' => 'firefox', + Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => 5 + } + expect(decision_service.send(:get_bucketing_id, 'test_user', user_attributes)).to eq('test_user') + expect(spy_logger).to have_received(:log).once.with(Logger::WARN, 'Bucketing ID attribute is not a string. Defaulted to user ID.') + end + + it 'should not log any message and return user ID when bucketing ID is nil' do + user_attributes = { + 'browser_type' => 'firefox', + Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => nil + } + expect(decision_service.send(:get_bucketing_id, 'test_user', user_attributes)).to eq('test_user') + expect(spy_logger).not_to have_received(:log) + end + + it 'should not log any message and return given bucketing ID when bucketing ID is a String' do + user_attributes = { + 'browser_type' => 'firefox', + Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => 'i_am_bucketing_id' + } + expect(decision_service.send(:get_bucketing_id, 'test_user', user_attributes)).to eq('i_am_bucketing_id') + expect(spy_logger).not_to have_received(:log) + end + + it 'should not log any message and return empty String when bucketing ID is empty String' do + user_attributes = { + 'browser_type' => 'firefox', + Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'] => '' + } + expect(decision_service.send(:get_bucketing_id, 'test_user', user_attributes)).to eq('') + expect(spy_logger).not_to have_received(:log) + end + end end diff --git a/spec/event_builder_spec.rb b/spec/event_builder_spec.rb index 23ba282c..6ae4a848 100644 --- a/spec/event_builder_spec.rb +++ b/spec/event_builder_spec.rb @@ -125,6 +125,81 @@ expect(impression_event.http_verb).to eq(:post) end + it 'should create a valid Event when create_impression_event is called with attributes of different valid types' do + @expected_impression_params[:visitors][0][:attributes] = [ + { + entity_id: '111094', + key: 'browser_type', + type: 'custom', + value: 'firefox' + }, { + entity_id: '111095', + key: 'boolean_key', + type: 'custom', + value: true + }, { + entity_id: '111096', + key: 'integer_key', + type: 'custom', + value: 5 + }, { + entity_id: '111097', + key: 'double_key', + type: 'custom', + value: 5.5 + }, + entity_id: Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BOT_FILTERING'], + key: Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BOT_FILTERING'], + type: 'custom', + value: true + ] + + experiment = config.get_experiment_from_key('test_experiment') + attributes = { + 'browser_type' => 'firefox', + 'boolean_key' => true, + 'integer_key' => 5, + 'double_key' => 5.5 + } + impression_event = @event_builder.create_impression_event(experiment, '111128', 'test_user', attributes) + expect(impression_event.params).to eq(@expected_impression_params) + expect(impression_event.url).to eq(@expected_endpoint) + expect(impression_event.http_verb).to eq(:post) + end + + it 'should create a valid Event and exclude attributes of invalid types' do + @expected_impression_params[:visitors][0][:attributes] = [ + { + entity_id: '111094', + key: 'browser_type', + type: 'custom', + value: 'firefox' + }, + { + entity_id: '111096', + key: 'integer_key', + type: 'custom', + value: 5 + }, + entity_id: Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BOT_FILTERING'], + key: Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BOT_FILTERING'], + type: 'custom', + value: true + ] + + experiment = config.get_experiment_from_key('test_experiment') + attributes = { + 'browser_type' => 'firefox', + 'boolean_key' => nil, + 'integer_key' => 5, + 'double_key' => {} + } + impression_event = @event_builder.create_impression_event(experiment, '111128', 'test_user', attributes) + expect(impression_event.params).to eq(@expected_impression_params) + expect(impression_event.url).to eq(@expected_endpoint) + expect(impression_event.http_verb).to eq(:post) + end + it 'should create a valid Event when create_impression_event is called with attributes as a false value' do @expected_impression_params[:visitors][0][:attributes].unshift( entity_id: '111094', diff --git a/spec/project_config_spec.rb b/spec/project_config_spec.rb index b9c79072..1172a969 100644 --- a/spec/project_config_spec.rb +++ b/spec/project_config_spec.rb @@ -42,7 +42,10 @@ expect(project_config.revision).to eq(config_body['revision']) expected_attribute_key_map = { - 'browser_type' => config_body['attributes'][0] + 'browser_type' => config_body['attributes'][0], + 'boolean_key' => config_body['attributes'][1], + 'integer_key' => config_body['attributes'][2], + 'double_key' => config_body['attributes'][3] } expected_audience_id_map = { '11154' => config_body['audiences'][0], diff --git a/spec/project_spec.rb b/spec/project_spec.rb index aa206510..be3db78d 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -228,6 +228,92 @@ class InvalidErrorHandler; end expect(project_instance.decision_service.bucketer).to have_received(:bucket).once end + it 'should properly activate a user, (with attributes of valid types) when there is an audience match' do + params = @expected_activate_params + params[:visitors][0][:attributes].unshift( + { + entity_id: '111094', + key: 'browser_type', + type: 'custom', + value: 'firefox' + }, { + entity_id: '111095', + key: 'boolean_key', + type: 'custom', + value: true + }, { + entity_id: '111096', + key: 'integer_key', + type: 'custom', + value: 5 + }, + entity_id: '111097', + key: 'double_key', + type: 'custom', + value: 5.5 + ) + params[:visitors][0][:snapshots][0][:decisions] = [{ + campaign_id: '3', + experiment_id: '122227', + variation_id: '122228' + }] + params[:visitors][0][:snapshots][0][:events][0][:entity_id] = '3' + + variation_to_return = project_instance.config.get_variation_from_id('test_experiment_with_audience', '122228') + allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return(variation_to_return) + allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event)) + + attributes = { + 'browser_type' => 'firefox', + 'boolean_key' => true, + 'integer_key' => 5, + 'double_key' => 5.5 + } + + expect(project_instance.activate('test_experiment_with_audience', 'test_user', attributes)) + .to eq('control_with_audience') + expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(Optimizely::Event.new(:post, impression_log_url, params, post_headers)).once + expect(project_instance.decision_service.bucketer).to have_received(:bucket).once + end + + it 'should properly activate a user, (with attributes of invalid types) when there is an audience match' do + params = @expected_activate_params + params[:visitors][0][:attributes].unshift( + { + entity_id: '111094', + key: 'browser_type', + type: 'custom', + value: 'firefox' + }, + entity_id: '111095', + key: 'boolean_key', + type: 'custom', + value: true + ) + params[:visitors][0][:snapshots][0][:decisions] = [{ + campaign_id: '3', + experiment_id: '122227', + variation_id: '122228' + }] + params[:visitors][0][:snapshots][0][:events][0][:entity_id] = '3' + + variation_to_return = project_instance.config.get_variation_from_id('test_experiment_with_audience', '122228') + allow(project_instance.decision_service.bucketer).to receive(:bucket).and_return(variation_to_return) + allow(project_instance.event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event)) + + attributes = { + 'browser_type' => 'firefox', + 'boolean_key' => true, + 'integer_key' => nil, + 'double_key' => {} + } + + expect(project_instance.activate('test_experiment_with_audience', 'test_user', attributes)) + .to eq('control_with_audience') + expect(project_instance.event_dispatcher).to have_received(:dispatch_event).with(Optimizely::Event.new(:post, impression_log_url, params, post_headers)).once + expect(project_instance.decision_service.bucketer).to have_received(:bucket).once + end + it 'should properly activate a user, (with attributes provided) when there is an audience match after a force variation call' do params = @expected_activate_params params[:visitors][0][:attributes].unshift( @@ -793,6 +879,12 @@ class InvalidErrorHandler; end project_instance.is_feature_enabled('multi_variate_feature', 'test_user') 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 + expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Provided attributes are in an invalid format.') + end + it 'should return false when the user is not bucketed into any variation' do allow(project_instance.decision_service).to receive(:get_variation_for_feature).and_return(nil) @@ -908,6 +1000,12 @@ class InvalidErrorHandler; end project_instance.get_enabled_features('test_user', 'browser_type' => 'chrome') 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_enabled_features('test_user', 'invalid_attributes')).to be_empty + expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Provided attributes are in an invalid format.') + end + it 'should return only enabled feature flags keys' do # Sets all feature-flags keys with randomly assigned status features_keys = project_instance.config.feature_flags.map do |item| @@ -1224,6 +1322,12 @@ class InvalidErrorHandler; end expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', nil, user_attributes)) .to eq(nil) 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(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Provided attributes are in an invalid format.') + end end describe 'when forced variation is used' do diff --git a/spec/spec_params.rb b/spec/spec_params.rb index caaf8aa6..576d72ce 100644 --- a/spec/spec_params.rb +++ b/spec/spec_params.rb @@ -302,6 +302,15 @@ module OptimizelySpec 'attributes' => [{ 'key' => 'browser_type', 'id' => '111094' + }, { + 'key' => 'boolean_key', + 'id' => '111095' + }, { + 'key' => 'integer_key', + 'id' => '111096' + }, { + 'key' => 'double_key', + 'id' => '111097' }], 'audiences' => [{ 'name' => 'Firefox users', diff --git a/spec/validator_helper_spec.rb b/spec/validator_helper_spec.rb new file mode 100644 index 00000000..8a4736d8 --- /dev/null +++ b/spec/validator_helper_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +# +# Copyright 2018, Optimizely and contributors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +require 'spec_helper' +require 'optimizely' +require 'optimizely/logger' +require 'optimizely/helpers/validator' + +describe 'ValidatorHelper' do + let(:spy_logger) { spy('logger') } + + describe '.attributes_valid?' do + it 'should return true when valid attributes are passed' do + expect(Optimizely::Helpers::Validator.attributes_valid?({})).to eq(true) + expect( + Optimizely::Helpers::Validator.attributes_valid?( + boolean: false, + double: 5.5, + integer: 5, + string: 'value' + ) + ).to eq(true) + end + + it 'should return false when invalid attributes are passed' do + expect(Optimizely::Helpers::Validator.attributes_valid?('key: value')).to eq(false) + expect(Optimizely::Helpers::Validator.attributes_valid?(%w[key value])).to eq(false) + expect(Optimizely::Helpers::Validator.attributes_valid?(42)).to eq(false) + expect(Optimizely::Helpers::Validator.attributes_valid?([])).to eq(false) + expect(Optimizely::Helpers::Validator.attributes_valid?(false)).to eq(false) + end + end + + describe '.attribute_valid?' do + it 'should return true when valid type attribute value is passed' do + expect(Optimizely::Helpers::Validator.attribute_valid?('test_attribute', 'value')).to eq(true) + expect(Optimizely::Helpers::Validator.attribute_valid?('test_attribute', 5)).to eq(true) + expect(Optimizely::Helpers::Validator.attribute_valid?('test_attribute', 5.5)).to eq(true) + expect(Optimizely::Helpers::Validator.attribute_valid?('test_attribute', false)).to eq(true) + expect(Optimizely::Helpers::Validator.attribute_valid?('test_attribute', true)).to eq(true) + expect(Optimizely::Helpers::Validator.attribute_valid?('test_attribute', '')).to eq(true) + expect(Optimizely::Helpers::Validator.attribute_valid?('test_attribute', 0)).to eq(true) + expect(Optimizely::Helpers::Validator.attribute_valid?('test_attribute', 0.0)).to eq(true) + end + + it 'should return false when invalid type attribute value is passed' do + expect(Optimizely::Helpers::Validator.attribute_valid?('test_attribute', {})).to eq(false) + expect(Optimizely::Helpers::Validator.attribute_valid?('test_attribute', [])).to eq(false) + expect(Optimizely::Helpers::Validator.attribute_valid?('test_attribute', nil)).to eq(false) + expect(Optimizely::Helpers::Validator.attribute_valid?('test_attribute', key: 'value')).to eq(false) + expect(Optimizely::Helpers::Validator.attribute_valid?('test_attribute', %w[key value])).to eq(false) + end + + it 'should return true when valid attribute key is passed' do + expect(Optimizely::Helpers::Validator.attribute_valid?('', 'value')).to eq(true) + expect(Optimizely::Helpers::Validator.attribute_valid?(:test_attribute, 'value')).to eq(true) + end + + it 'should return false when invalid attribute key is passed' do + expect(Optimizely::Helpers::Validator.attribute_valid?(5, 'value')).to eq(false) + expect(Optimizely::Helpers::Validator.attribute_valid?(true, 'value')).to eq(false) + expect(Optimizely::Helpers::Validator.attribute_valid?(5.5, 'value')).to eq(false) + end + end +end