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

Author: rashidsp

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
       )

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

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)
 

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)