feat: Duplicate experiment key issue with multi feature flag (#282)

Author: ozayr-zaviar

lib/optimizely.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 #
-#    Copyright 2016-2020, Optimizely and contributors
+#    Copyright 2016-2021, 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.
@@ -877,12 +877,14 @@ def get_variation_with_config(experiment_key, user_id, attributes, config)
       experiment = config.get_experiment_from_key(experiment_key)
       return nil if experiment.nil?
 
+      experiment_id = experiment['id']
+
       return nil unless user_inputs_valid?(attributes)
 
-      variation_id, = @decision_service.get_variation(config, experiment_key, user_id, attributes)
+      variation_id, = @decision_service.get_variation(config, experiment_id, user_id, attributes)
       variation = config.get_variation_from_id(experiment_key, variation_id) unless variation_id.nil?
       variation_key = variation['key'] if variation
-      decision_notification_type = if config.feature_experiment?(experiment['id'])
+      decision_notification_type = if config.feature_experiment?(experiment_id)
                                      Helpers::Constants::DECISION_NOTIFICATION_TYPES['FEATURE_TEST']
                                    else
                                      Helpers::Constants::DECISION_NOTIFICATION_TYPES['AB_TEST']
@@ -1078,10 +1080,11 @@ def send_impression(config, experiment, variation_key, flag_key, rule_key, enabl
         }
       end
 
+      experiment_id = experiment['id']
       experiment_key = experiment['key']
 
       variation_id = ''
-      variation_id = config.get_variation_id_from_key(experiment_key, variation_key) if experiment_key != ''
+      variation_id = config.get_variation_id_from_key_by_experiment_id(experiment_id, variation_key) if experiment_id != ''
 
       metadata = {
         flag_key: flag_key,
@@ -1097,9 +1100,9 @@ def send_impression(config, experiment, variation_key, flag_key, rule_key, enabl
 
       @logger.log(Logger::INFO, "Activating user '#{user_id}' in experiment '#{experiment_key}'.")
 
-      experiment = nil if experiment_key == ''
+      experiment = nil if experiment_id == ''
       variation = nil
-      variation = config.get_variation_from_id(experiment_key, variation_id) unless experiment.nil?
+      variation = config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) unless experiment.nil?
       log_event = EventFactory.create_log_event(user_event, @logger)
       @notification_center.send_notifications(
         NotificationCenter::NOTIFICATION_TYPES[:ACTIVATE],

lib/optimizely/bucketer.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 #
-#    Copyright 2016-2017, 2019-2020 Optimizely and contributors
+#    Copyright 2016-2017, 2019-2021 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.
@@ -88,7 +88,7 @@ def bucket(project_config, experiment, bucketing_id, user_id)
       decide_reasons.push(*find_bucket_reasons)
 
       if variation_id && variation_id != ''
-        variation = project_config.get_variation_from_id(experiment_key, variation_id)
+        variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)
         return variation, decide_reasons
       end
 

lib/optimizely/config/datafile_project_config.rb

@@ -52,10 +52,12 @@ class DatafileProjectConfig < ProjectConfig
     attr_reader :feature_variable_key_map
     attr_reader :group_id_map
     attr_reader :rollout_id_map
-    attr_reader :rollout_experiment_key_map
+    attr_reader :rollout_experiment_id_map
     attr_reader :variation_id_map
     attr_reader :variation_id_to_variable_usage_map
     attr_reader :variation_key_map
+    attr_reader :variation_id_map_by_experiment_id
+    attr_reader :variation_key_map_by_experiment_id
 
     def initialize(datafile, logger, error_handler)
       # ProjectConfig init method to fetch and set project config data
@@ -113,9 +115,11 @@ def initialize(datafile, logger, error_handler)
       @audience_id_map = @audience_id_map.merge(generate_key_map(@typed_audiences, 'id')) unless @typed_audiences.empty?
       @variation_id_map = {}
       @variation_key_map = {}
+      @variation_id_map_by_experiment_id = {}
+      @variation_key_map_by_experiment_id = {}
       @variation_id_to_variable_usage_map = {}
       @variation_id_to_experiment_map = {}
-      @experiment_key_map.each_value do |exp|
+      @experiment_id_map.each_value do |exp|
         # Excludes experiments from rollouts
         variations = exp.fetch('variations')
         variations.each do |variation|
@@ -125,13 +129,13 @@ def initialize(datafile, logger, error_handler)
       end
       @rollout_id_map = generate_key_map(@rollouts, 'id')
       # split out the experiment key map for rollouts
-      @rollout_experiment_key_map = {}
+      @rollout_experiment_id_map = {}
       @rollout_id_map.each_value do |rollout|
         exps = rollout.fetch('experiments')
-        @rollout_experiment_key_map = @rollout_experiment_key_map.merge(generate_key_map(exps, 'key'))
+        @rollout_experiment_id_map = @rollout_experiment_id_map.merge(generate_key_map(exps, 'id'))
       end
-      @all_experiments = @experiment_key_map.merge(@rollout_experiment_key_map)
-      @all_experiments.each do |key, exp|
+      @all_experiments = @experiment_id_map.merge(@rollout_experiment_id_map)
+      @all_experiments.each do |id, exp|
         variations = exp.fetch('variations')
         variations.each do |variation|
           variation_id = variation['id']
@@ -141,8 +145,10 @@ def initialize(datafile, logger, error_handler)
 
           @variation_id_to_variable_usage_map[variation_id] = generate_key_map(variation_variables, 'id')
         end
-        @variation_id_map[key] = generate_key_map(variations, 'id')
-        @variation_key_map[key] = generate_key_map(variations, 'key')
+        @variation_id_map[exp['key']] = generate_key_map(variations, 'id')
+        @variation_key_map[exp['key']] = generate_key_map(variations, 'key')
+        @variation_id_map_by_experiment_id[id] = generate_key_map(variations, 'id')
+        @variation_key_map_by_experiment_id[id] = generate_key_map(variations, 'key')
       end
       @feature_flag_key_map = generate_key_map(@feature_flags, 'key')
       @experiment_feature_map = {}
@@ -209,6 +215,21 @@ def get_experiment_from_key(experiment_key)
       nil
     end
 
+    def get_experiment_from_id(experiment_id)
+      # Retrieves experiment ID for a given key
+      #
+      # experiment_id - String id representing the experiment
+      #
+      # Returns Experiment or nil if not found
+
+      experiment = @experiment_id_map[experiment_id]
+      return experiment if experiment
+
+      @logger.log Logger::ERROR, "Experiment id '#{experiment_id}' is not in datafile."
+      @error_handler.handle_error InvalidExperimentError
+      nil
+    end
+
     def get_experiment_key(experiment_id)
       # Retrieves experiment key for a given ID.
       #
@@ -277,6 +298,52 @@ def get_variation_from_id(experiment_key, variation_id)
       nil
     end
 
+    def get_variation_from_id_by_experiment_id(experiment_id, variation_id)
+      # Get variation given experiment ID and variation ID
+      #
+      # experiment_id - ID representing parent experiment of variation
+      # variation_id - ID of the variation
+      #
+      # Returns the variation or nil if not found
+
+      variation_id_map_by_experiment_id = @variation_id_map_by_experiment_id[experiment_id]
+      if variation_id_map_by_experiment_id
+        variation = variation_id_map_by_experiment_id[variation_id]
+        return variation if variation
+
+        @logger.log Logger::ERROR, "Variation id '#{variation_id}' is not in datafile."
+        @error_handler.handle_error InvalidVariationError
+        return nil
+      end
+
+      @logger.log Logger::ERROR, "Experiment id '#{experiment_id}' is not in datafile."
+      @error_handler.handle_error InvalidExperimentError
+      nil
+    end
+
+    def get_variation_id_from_key_by_experiment_id(experiment_id, variation_key)
+      # Get variation given experiment ID and variation key
+      #
+      # experiment_id - ID representing parent experiment of variation
+      # variation_key - Key of the variation
+      #
+      # Returns the variation or nil if not found
+
+      variation_key_map = @variation_key_map_by_experiment_id[experiment_id]
+      if variation_key_map
+        variation = variation_key_map[variation_key]
+        return variation['id'] if variation
+
+        @logger.log Logger::ERROR, "Variation key '#{variation_key}' is not in datafile."
+        @error_handler.handle_error InvalidVariationError
+        return nil
+      end
+
+      @logger.log Logger::ERROR, "Experiment id '#{experiment_id}' is not in datafile."
+      @error_handler.handle_error InvalidExperimentError
+      nil
+    end
+
     def get_variation_id_from_key(experiment_key, variation_key)
       # Get variation ID given experiment key and variation key
       #
@@ -300,17 +367,17 @@ def get_variation_id_from_key(experiment_key, variation_key)
       nil
     end
 
-    def get_whitelisted_variations(experiment_key)
-      # Retrieves whitelisted variations for a given experiment Key
+    def get_whitelisted_variations(experiment_id)
+      # Retrieves whitelisted variations for a given experiment id
       #
-      # experiment_key - String Key representing the experiment
+      # experiment_id - String id representing the experiment
       #
       # Returns whitelisted variations for the experiment or nil
 
-      experiment = @experiment_key_map[experiment_key]
+      experiment = @experiment_id_map[experiment_id]
       return experiment['forcedVariations'] if experiment
 
-      @logger.log Logger::ERROR, "Experiment key '#{experiment_key}' is not in datafile."
+      @logger.log Logger::ERROR, "Experiment ID '#{experiment_id}' is not in datafile."
       @error_handler.handle_error InvalidExperimentError
     end
 

lib/optimizely/decision_service.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 #
-#    Copyright 2017-2020, Optimizely and contributors
+#    Copyright 2017-2021, 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.
@@ -52,11 +52,11 @@ def initialize(logger, user_profile_service = nil)
       @forced_variation_map = {}
     end
 
-    def get_variation(project_config, experiment_key, user_id, attributes = nil, decide_options = [])
+    def get_variation(project_config, experiment_id, user_id, attributes = nil, decide_options = [])
       # Determines variation into which user will be bucketed.
       #
       # project_config - project_config - Instance of ProjectConfig
-      # experiment_key - Experiment for which visitor variation needs to be determined
+      # experiment_id - Experiment for which visitor variation needs to be determined
       # user_id - String ID for user
       # attributes - Hash representing user attributes
       #
@@ -68,10 +68,10 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec
       bucketing_id, bucketing_id_reasons = get_bucketing_id(user_id, attributes)
       decide_reasons.push(*bucketing_id_reasons)
       # Check to make sure experiment is active
-      experiment = project_config.get_experiment_from_key(experiment_key)
+      experiment = project_config.get_experiment_from_id(experiment_id)
       return nil, decide_reasons if experiment.nil?
 
-      experiment_id = experiment['id']
+      experiment_key = experiment['key']
       unless project_config.experiment_running?(experiment)
         message = "Experiment '#{experiment_key}' is not running."
         @logger.log(Logger::INFO, message)
@@ -80,12 +80,12 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec
       end
 
       # Check if a forced variation is set for the user
-      forced_variation, reasons_received = get_forced_variation(project_config, experiment_key, user_id)
+      forced_variation, reasons_received = get_forced_variation(project_config, experiment['key'], user_id)
       decide_reasons.push(*reasons_received)
       return forced_variation['id'], decide_reasons if forced_variation
 
       # Check if user is in a white-listed variation
-      whitelisted_variation_id, reasons_received = get_whitelisted_variation_id(project_config, experiment_key, user_id)
+      whitelisted_variation_id, reasons_received = get_whitelisted_variation_id(project_config, experiment_id, user_id)
       decide_reasons.push(*reasons_received)
       return whitelisted_variation_id, decide_reasons if whitelisted_variation_id
 
@@ -122,7 +122,7 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec
       message = ''
       if variation_id
         variation_key = variation['key']
-        message = "User '#{user_id}' is in variation '#{variation_key}' of experiment '#{experiment_key}'."
+        message = "User '#{user_id}' is in variation '#{variation_key}' of experiment '#{experiment_id}'."
       else
         message = "User '#{user_id}' is in no variation."
       end
@@ -186,13 +186,13 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_id,
           return nil, decide_reasons
         end
 
-        experiment_key = experiment['key']
-        variation_id, reasons_received = get_variation(project_config, experiment_key, user_id, attributes, decide_options)
+        experiment_id = experiment['id']
+        variation_id, reasons_received = get_variation(project_config, experiment_id, user_id, attributes, decide_options)
         decide_reasons.push(*reasons_received)
 
         next unless variation_id
 
-        variation = project_config.variation_id_map[experiment_key][variation_id]
+        variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)
 
         return Decision.new(experiment, variation, DECISION_SOURCES['FEATURE_TEST']), decide_reasons
       end
@@ -315,7 +315,7 @@ def set_forced_variation(project_config, experiment_key, user_id, variation_key)
         return true
       end
 
-      variation_id = project_config.get_variation_id_from_key(experiment_key, variation_key)
+      variation_id = project_config.get_variation_id_from_key_by_experiment_id(experiment_id, variation_key)
 
       #  check if the variation exists in the datafile
       unless variation_id
@@ -334,7 +334,7 @@ def get_forced_variation(project_config, experiment_key, user_id)
       # Gets the forced variation for the given user and experiment.
       #
       # project_config - Instance of ProjectConfig
-      # experiment_key - String Key for experiment
+      # experiment_key - String key for experiment
       # user_id - String ID for user
       #
       # Returns Variation The variation which the given user and experiment should be forced into
@@ -354,22 +354,22 @@ def get_forced_variation(project_config, experiment_key, user_id)
       return nil, decide_reasons if experiment_id.nil? || experiment_id.empty?
 
       unless experiment_to_variation_map.key? experiment_id
-        message = "No experiment '#{experiment_key}' mapped to user '#{user_id}' in the forced variation map."
+        message = "No experiment '#{experiment_id}' mapped to user '#{user_id}' in the forced variation map."
         @logger.log(Logger::DEBUG, message)
         decide_reasons.push(message)
         return nil, decide_reasons
       end
 
       variation_id = experiment_to_variation_map[experiment_id]
       variation_key = ''
-      variation = project_config.get_variation_from_id(experiment_key, variation_id)
+      variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)
       variation_key = variation['key'] if variation
 
       # check if the variation exists in the datafile
       # this case is logged in get_variation_from_id
       return nil, decide_reasons if variation_key.empty?
 
-      message = "Variation '#{variation_key}' is mapped to experiment '#{experiment_key}' and user '#{user_id}' in the forced variation map"
+      message = "Variation '#{variation_key}' is mapped to experiment '#{experiment_id}' and user '#{user_id}' in the forced variation map"
       @logger.log(Logger::DEBUG, message)
       decide_reasons.push(message)
 
@@ -378,7 +378,7 @@ def get_forced_variation(project_config, experiment_key, user_id)
 
     private
 
-    def get_whitelisted_variation_id(project_config, experiment_key, user_id)
+    def get_whitelisted_variation_id(project_config, experiment_id, user_id)
       # Determine if a user is whitelisted into a variation for the given experiment and return the ID of that variation
       #
       # project_config - project_config - Instance of ProjectConfig
@@ -387,23 +387,23 @@ def get_whitelisted_variation_id(project_config, experiment_key, user_id)
       #
       # Returns variation ID into which user_id is whitelisted (nil if no variation)
 
-      whitelisted_variations = project_config.get_whitelisted_variations(experiment_key)
+      whitelisted_variations = project_config.get_whitelisted_variations(experiment_id)
 
       return nil, nil unless whitelisted_variations
 
       whitelisted_variation_key = whitelisted_variations[user_id]
 
       return nil, nil unless whitelisted_variation_key
 
-      whitelisted_variation_id = project_config.get_variation_id_from_key(experiment_key, whitelisted_variation_key)
+      whitelisted_variation_id = project_config.get_variation_id_from_key_by_experiment_id(experiment_id, whitelisted_variation_key)
 
       unless whitelisted_variation_id
         message = "User '#{user_id}' is whitelisted into variation '#{whitelisted_variation_key}', which is not in the datafile."
         @logger.log(Logger::INFO, message)
         return nil, message
       end
 
-      message = "User '#{user_id}' is whitelisted into variation '#{whitelisted_variation_key}' of experiment '#{experiment_key}'."
+      message = "User '#{user_id}' is whitelisted into variation '#{whitelisted_variation_key}' of experiment '#{experiment_id}'."
       @logger.log(Logger::INFO, message)
 
       [whitelisted_variation_id, message]