Skip to content

refact(API): Adds missing input validations in all API methods and validates empty user Id. #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Nov 27, 2018
Merged

refact(API): Adds missing input validations in all API methods and validates empty user Id. #127

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e925856
refact(API): Adds missing input validations in all API methods and va…
rashidsp Sep 28, 2018
2080c3a
Resolves conflicts
rashidsp Nov 14, 2018
14dcb12
Merge branch 'master' into rashid/input-validations
rashidsp Nov 15, 2018
1bd4112
refact(API): validates user_id inside validator.
rashidsp Nov 15, 2018
b7d9c58
Merge branch 'master' into rashid/input-validations
msohailhussain Nov 26, 2018
9a053be
fix: master branch update issues
rashidsp Nov 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 40 additions & 11 deletions lib/optimizely.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,15 @@ def activate(experiment_key, user_id, attributes = nil)

return nil unless Optimizely::Helpers::Validator.inputs_valid?(
{
experiment_key: experiment_key,
user_id: user_id
experiment_key: experiment_key
}, @logger, Logger::ERROR
)

unless user_id.is_a?(String)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather we refactor a bit and avoid using this same logic everywhere in the code. Let's mimic what the C# SDK does here: https://github.com/optimizely/csharp-sdk/blob/master/OptimizelySDK/Optimizely.cs#L642

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid")
return nil
end

variation_key = get_variation(experiment_key, user_id, attributes)

if variation_key.nil?
Expand Down Expand Up @@ -132,11 +136,15 @@ def get_variation(experiment_key, user_id, attributes = nil)

return nil unless Optimizely::Helpers::Validator.inputs_valid?(
{
experiment_key: experiment_key,
user_id: user_id
experiment_key: experiment_key
}, @logger, Logger::ERROR
)

unless user_id.is_a?(String)
@logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid")
return nil
end

unless user_inputs_valid?(attributes)
@logger.log(Logger::INFO, "Not activating user '#{user_id}.")
return nil
Expand Down Expand Up @@ -194,11 +202,15 @@ def track(event_key, user_id, attributes = nil, event_tags = nil)

return nil unless Optimizely::Helpers::Validator.inputs_valid?(
{
event_key: event_key,
user_id: user_id
event_key: event_key
}, @logger, Logger::ERROR
)

unless user_id.is_a?(String)
@logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid")
return nil
end

return nil unless user_inputs_valid?(attributes, event_tags)

experiment_ids = @config.get_experiment_ids_for_event(event_key)
Expand Down Expand Up @@ -253,11 +265,17 @@ def is_feature_enabled(feature_flag_key, user_id, attributes = nil)

return false unless Optimizely::Helpers::Validator.inputs_valid?(
{
feature_flag_key: feature_flag_key,
user_id: user_id
feature_flag_key: feature_flag_key
}, @logger, Logger::ERROR
)

unless user_id.is_a?(String)
@logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid")
return false
end

return false unless user_inputs_valid?(attributes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we combine Optimizely::Helpers::Validator.inputs_valid? with user_inputs_valid?? Seems like we could just put all user inputs into one dictionary and have them all validated

Copy link
Contributor Author

@rashidsp rashidsp Nov 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combined validations: rashid/input-validations...rashid/input-validations-2
It resulted in a big change plz confirm to continue with this PR?


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}'.")
Expand Down Expand Up @@ -305,7 +323,12 @@ 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)
unless user_id.is_a?(String)
@logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid")
return enabled_features
end

return enabled_features unless user_inputs_valid?(attributes)

@config.feature_flags.each do |feature|
enabled_features.push(feature['key']) if is_feature_enabled(
Expand Down Expand Up @@ -444,12 +467,18 @@ 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
variable_type: variable_type
},
@logger, Logger::ERROR
)

unless user_id.is_a?(String)
@logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid")
return nil
end

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}'.")
Expand Down
15 changes: 12 additions & 3 deletions lib/optimizely/project_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,11 +282,15 @@ def get_forced_variation(experiment_key, user_id)

return nil unless Optimizely::Helpers::Validator.inputs_valid?(
{
experiment_key: experiment_key,
user_id: user_id
experiment_key: experiment_key
}, @logger, Logger::DEBUG
)

unless user_id.is_a?(String)
@logger.log(Logger::DEBUG, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid")
return nil
end

unless @forced_variation_map.key? user_id
@logger.log(Logger::DEBUG, "User '#{user_id}' is not in the forced variation map.")
return nil
Expand Down Expand Up @@ -329,10 +333,15 @@ 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}
input_values[:variation_key] = variation_key unless variation_key.nil?
return false unless Optimizely::Helpers::Validator.inputs_valid?(input_values, @logger, Logger::DEBUG)

unless user_id.is_a?(String)
@logger.log(Logger::DEBUG, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid")
return false
end

experiment = get_experiment_from_key(experiment_key)
experiment_id = experiment['id'] if experiment
# check if the experiment exists in the datafile
Expand Down
36 changes: 21 additions & 15 deletions spec/project_config_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,12 +856,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)
Expand Down Expand Up @@ -902,12 +896,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)
Expand Down Expand Up @@ -958,24 +946,42 @@
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],
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(
{
experiment_key: @valid_experiment[:key],
user_id: @user_id
experiment_key: @valid_experiment[:key]
}, spy_logger, Logger::DEBUG
)
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)
Expand Down
Loading