fixes variable type bug (#73)

Author: rashidsp

lib/optimizely.rb

@@ -378,51 +378,64 @@ def get_feature_variable_for_type(feature_flag_key, variable_key, variable_type,
       # attributes - Hash representing visitor attributes and values which need to be recorded.
       #
       # Returns the type-casted variable value.
-      # Returns nil if the feature flag or variable are not found.
+      # Returns nil if the feature flag or variable or user ID is empty
+      #             in case of variable type mismatch
+
+      unless feature_flag_key
+        @logger.log(Logger::ERROR, "Feature flag key cannot be empty.")
+        return nil
+      end
+
+      unless variable_key
+        @logger.log(Logger::ERROR, "Variable key cannot be empty.")
+        return nil
+      end
+
+      unless user_id
+        @logger.log(Logger::ERROR, "User ID cannot be empty.")
+        return nil
+      end
 
       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}'.")
         return nil
       end
-
-      variable_value = nil
+      
       variable = @config.get_feature_variable(feature_flag, variable_key)
-      unless variable.nil?
-        variable_value = variable['defaultValue']
 
+      # Error message logged in ProjectConfig- get_feature_flag_from_key
+      return nil if variable.nil?
+
+      # Returns nil if type differs
+      if variable['type'] != variable_type
+        @logger.log(Logger::WARN,
+          "Requested variable as type '#{variable_type}' but variable '#{variable_key}' is of type '#{variable['type']}'.")
+        return nil
+      else
         decision = @decision_service.get_variation_for_feature(feature_flag, user_id, attributes)
-        unless decision
-          @logger.log(Logger::INFO,
-            "User '#{user_id}' was not bucketed into any variation for feature flag '#{feature_flag_key}'. Returning the default variable value '#{variable_value}'.")
-        else
+        variable_value = variable['defaultValue']
+        if decision
           variation = decision['variation']
           variation_variable_usages = @config.variation_id_to_variable_usage_map[variation['id']]
           variable_id = variable['id']
-          unless variation_variable_usages and variation_variable_usages.key?(variable_id)
-            variation_key = variation['key']
-            @logger.log(Logger::DEBUG,
-              "Variable '#{variable_key}' is not used in variation '#{variation_key}'. Returning the default variable value '#{variable_value}'."
-            )
-          else
+          if variation_variable_usages and variation_variable_usages.key?(variable_id)
             variable_value = variation_variable_usages[variable_id]['value']
             @logger.log(Logger::INFO,
               "Got variable value '#{variable_value}' for variable '#{variable_key}' of feature flag '#{feature_flag_key}'.")
+          else
+            @logger.log(Logger::DEBUG,
+              "Variable '#{variable_key}' is not used in variation '#{variation['key']}'. Returning the default variable value '#{variable_value}'.")
           end
+        else
+          @logger.log(Logger::INFO,
+            "User '#{user_id}' was not bucketed into any variation for feature flag '#{feature_flag_key}'. Returning the default variable value '#{variable_value}'.")
         end
       end
 
-      unless variable_value.nil?
-        actual_variable_type = variable['type']
-        unless variable_type == actual_variable_type
-          @logger.log(Logger::WARN,
-            "Requested variable type '#{variable_type}' but variable '#{variable_key}' is of type '#{actual_variable_type}'.")
-        end
+      variable_value = Helpers::VariableType.cast_value_to_type(variable_value, variable_type, @logger)
 
-        variable_value = Helpers::VariableType.cast_value_to_type(variable_value, variable_type, @logger)
-      end
-
-      return variable_value
+      variable_value
     end
 
     def get_valid_experiments_for_event(event_key, user_id, attributes)

lib/optimizely/decision_service.rb

@@ -16,7 +16,6 @@
 require_relative './bucketer'
 
 module Optimizely
-
   RESERVED_ATTRIBUTE_KEY_BUCKETING_ID = "\$opt_bucketing_id".freeze
 
   class DecisionService
@@ -54,9 +53,7 @@ def get_variation(experiment_key, user_id, attributes = nil)
       bucketing_id = get_bucketing_id(user_id, attributes)
       # Check to make sure experiment is active
       experiment = @config.get_experiment_from_key(experiment_key)
-      if experiment.nil?
-        return nil
-      end
+      return nil if experiment.nil?
 
       experiment_id = experiment['id']
       unless @config.experiment_running?(experiment)
@@ -152,34 +149,33 @@ def get_variation_for_feature_experiment(feature_flag, user_id, attributes = nil
           "The feature flag '#{feature_flag_key}' is not used in any experiments."
         )
         return nil
-      else
-        # Evaluate each experiment and return the first bucketed experiment variation
-        feature_flag['experimentIds'].each do |experiment_id|
-          experiment = @config.experiment_id_map[experiment_id]
-          unless experiment
-            @config.logger.log(
-              Logger::DEBUG,
-              "Feature flag experiment with ID '#{experiment_id}' is not in the datafile."
-            )
-            return nil
-          end
-          experiment_key = experiment['key']
-          variation_id = get_variation(experiment_key, user_id, attributes)
-          next unless variation_id
-          variation = @config.variation_id_map[experiment_key][variation_id]
-          @config.logger.log(Logger::INFO,
-                             "The user '#{user_id}' is bucketed into experiment '#{experiment_key}' of feature "\
-                             "'#{feature_flag_key}'.")
-          return {
-            'experiment' => experiment,
-            'variation' => variation
-          }
+      end
+      # Evaluate each experiment and return the first bucketed experiment variation
+      feature_flag['experimentIds'].each do |experiment_id|
+        experiment = @config.experiment_id_map[experiment_id]
+        unless experiment
+          @config.logger.log(
+            Logger::DEBUG,
+            "Feature flag experiment with ID '#{experiment_id}' is not in the datafile."
+          )
+          return nil
         end
-        @config.logger.log(
-          Logger::INFO,
-          "The user '#{user_id}' is not bucketed into any of the experiments on the feature '#{feature_flag_key}'."
-        )
+        experiment_key = experiment['key']
+        variation_id = get_variation(experiment_key, user_id, attributes)
+        next unless variation_id
+        variation = @config.variation_id_map[experiment_key][variation_id]
+        @config.logger.log(Logger::INFO,
+                           "The user '#{user_id}' is bucketed into experiment '#{experiment_key}' of feature "\
+                           "'#{feature_flag_key}'.")
+        return {
+          'experiment' => experiment,
+          'variation' => variation
+        }
       end
+      @config.logger.log(
+        Logger::INFO,
+        "The user '#{user_id}' is not bucketed into any of the experiments on the feature '#{feature_flag_key}'."
+      )
 
       nil
     end
@@ -250,6 +246,7 @@ def get_variation_for_feature_rollout(feature_flag, user_id, attributes = nil)
           Logger::DEBUG,
           "User '#{user_id}' was excluded due to traffic allocation. Checking 'Everyone Else' rule now."
         )
+
         break
       end
 
@@ -262,6 +259,7 @@ def get_variation_for_feature_rollout(feature_flag, user_id, attributes = nil)
         Logger::DEBUG,
         "User '#{user_id}' was excluded from the 'Everyone Else' rule for feature flag"
       )
+
       nil
     end
 
@@ -329,8 +327,8 @@ def get_user_profile(user_id)
       # Returns Hash stored user profile (or a default one if lookup fails or user profile service not provided)
 
       user_profile = {
-        :user_id => user_id,
-        :experiment_bucket_map => {}
+        user_id: user_id,
+        experiment_bucket_map: {}
       }
 
       return user_profile unless @user_profile_service

spec/decision_service_spec.rb

@@ -534,7 +534,6 @@
               .with(Logger::DEBUG, "User '#{user_id}' was excluded due to traffic allocation. Checking 'Everyone Else' rule now.")
             expect(spy_logger).to have_received(:log).once
               .with(Logger::DEBUG, "User '#{user_id}' was excluded from the 'Everyone Else' rule for feature flag")
-
           end
         end
 

spec/project_spec.rb

@@ -713,7 +713,7 @@ class InvalidErrorHandler; end
       end
 
       describe 'and a variable usage instance is found' do
-        describe 'and the variable type is not a string' do
+        describe 'and the variable type boolean is not a string' do
           it 'should log a warning' do
             variation_to_return = project_instance.config.rollout_id_map['166660']['experiments'][0]['variations'][0]
             decision_to_return = {
@@ -723,18 +723,34 @@ class InvalidErrorHandler; end
             allow(project_instance.decision_service).to receive(:get_variation_for_feature).and_return(decision_to_return)
 
             expect(project_instance.get_feature_variable_string('boolean_single_variable_feature', 'boolean_variable', user_id, user_attributes))
-              .to eq('true')
+              .to eq(nil)
 
-            expect(spy_logger).to have_received(:log).twice
             expect(spy_logger).to have_received(:log).once
               .with(
-                Logger::INFO,
-                "Got variable value 'true' for variable 'boolean_variable' of feature flag 'boolean_single_variable_feature'."
+                Logger::WARN,
+                "Requested variable as type 'string' but variable 'boolean_variable' is of type 'boolean'."
               )
+          end
+        end
+
+        describe 'and the variable type integer is not a string' do
+          it 'should log a warning' do
+            integer_feature = project_instance.config.feature_flag_key_map['integer_single_variable_feature']
+            experiment_to_return = project_instance.config.experiment_id_map[integer_feature['experimentIds'][0]]
+            variation_to_return = experiment_to_return['variations'][0]
+            decision_to_return = {
+                'experiment' => experiment_to_return,
+                'variation' => variation_to_return
+            }
+            allow(project_instance.decision_service).to receive(:get_variation_for_feature).and_return(decision_to_return)
+
+            expect(project_instance.get_feature_variable_string('integer_single_variable_feature', 'integer_variable', user_id, user_attributes))
+              .to eq(nil)
+            
             expect(spy_logger).to have_received(:log).once
               .with(
                 Logger::WARN,
-                "Requested variable type 'string' but variable 'boolean_variable' is of type 'boolean'."
+                "Requested variable as type 'string' but variable 'integer_variable' is of type 'integer'."
               )
           end
         end
@@ -888,6 +904,33 @@ class InvalidErrorHandler; end
     end
   end
 
+
+  describe '#get_feature_variable_for_type with empty params' do
+    user_id = 'test_user'
+    user_attributes = {}
+
+    it 'should return nil if feature_flag_key is nil' do
+
+      expect(project_instance.get_feature_variable_integer(nil, 'integer_variable', user_id, user_attributes))
+          .to eq(nil)
+      expect(spy_logger).to have_received(:log).once.with(Logger::ERROR,"Feature flag key cannot be empty.")
+    end
+
+    it 'should return nil if variable_key is nil' do
+
+      expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', nil, user_id, user_attributes))
+          .to eq(nil)
+      expect(spy_logger).to have_received(:log).once.with(Logger::ERROR,"Variable key cannot be empty.")
+    end
+
+    it 'should return nil if user_id is nil' do
+
+      expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', nil, user_attributes))
+          .to eq(nil)
+      expect(spy_logger).to have_received(:log).once.with(Logger::ERROR,"User ID cannot be empty.")
+    end
+  end
+
   describe 'when forced variation is used' do
     # setForcedVariation on a paused experiment and then call getVariation.
     it 'should return null when getVariation is called on a paused experiment after setForcedVariation' do