add comments addressed

Author: ozayr-zaviar

lib/optimizely/config/datafile_project_config.rb

@@ -142,16 +142,7 @@ def initialize(datafile, logger, error_handler)
         @rollout_experiment_id_map = @rollout_experiment_id_map.merge(generate_key_map(exps, 'id'))
       end
 
-      @feature_flags.each do |flag|
-        variations = []
-        get_rules_for_flag(flag).each do |rule|
-          rule['variations'].each do |rule_variation|
-            variations.push(rule_variation) if variations.select { |variation| variation['id'] == rule_variation['id'] }
-          end
-        end
-        @flag_variation_map[flag['key']] = variations
-      end
-
+      @flag_variation_map = generate_feature_variation_map(@feature_flags)
       @all_experiments = @experiment_id_map.merge(@rollout_experiment_id_map)
       @all_experiments.each do |id, exp|
         variations = exp.fetch('variations')
@@ -533,6 +524,20 @@ def rollout_experiment?(experiment_id)
 
     private
 
+    def generate_feature_variation_map(feature_flags)
+      flag_variation_map = {}
+      feature_flags.each do |flag|
+        variations = []
+        get_rules_for_flag(flag).each do |rule|
+          rule['variations'].each do |rule_variation|
+            variations.push(rule_variation) if variations.select { |variation| variation['id'] == rule_variation['id'] }.empty?
+          end
+        end
+        flag_variation_map[flag['key']] = variations
+      end
+      flag_variation_map
+    end
+
     def generate_key_map(array, key)
       # Helper method to generate map from key to hash in array of hashes
       #

lib/optimizely/optimizely_user_context.rb

@@ -29,6 +29,7 @@ class OptimizelyUserContext
     ForcedDecision = Struct.new(:flag_key, :rule_key)
     def initialize(optimizely_client, user_id, user_attributes)
       @attr_mutex = Mutex.new
+      @forced_decision_mutex = Mutex.new
       @optimizely_client = optimizely_client
       @user_id = user_id
       @user_attributes = user_attributes.nil? ? {} : user_attributes.clone
@@ -37,7 +38,7 @@ def initialize(optimizely_client, user_id, user_attributes)
 
     def clone
       user_context = OptimizelyUserContext.new(@optimizely_client, @user_id, user_attributes)
-      user_context.instance_variable_set('@forced_decisions', @forced_decisions.dup) unless @forced_decisions.empty?
+      @forced_decision_mutex.synchronize { user_context.instance_variable_set('@forced_decisions', @forced_decisions.dup) unless @forced_decisions.empty? }
       user_context
     end
 
@@ -104,19 +105,18 @@ def set_forced_decision(flag_key, rule_key, variation_key)
       return false if flag_key.empty? || flag_key.nil?
 
       forced_decision_key = ForcedDecision.new(flag_key, rule_key)
-      @forced_decisions[forced_decision_key] = variation_key
+      @forced_decision_mutex.synchronize { @forced_decisions[forced_decision_key] = variation_key }
 
       true
     end
 
     def find_forced_decision(flag_key, rule_key = nil)
       return nil if @forced_decisions.empty?
 
+      variation_key = nil
       forced_decision_key = ForcedDecision.new(flag_key, rule_key)
-      variation_key = @forced_decisions[forced_decision_key]
-      return variation_key if variation_key
-
-      nil
+      @forced_decision_mutex.synchronize { variation_key = @forced_decisions[forced_decision_key] }
+      variation_key
     end
 
     # Returns the forced decision for a given flag and an optional rule.
@@ -143,12 +143,14 @@ def remove_forced_decision(flag_key, rule_key = nil)
       return false if @optimizely_client&.get_optimizely_config.nil?
 
       forced_decision_key = ForcedDecision.new(flag_key, rule_key)
-      if @forced_decisions.key?(forced_decision_key)
-        @forced_decisions.delete(forced_decision_key)
-        return true
+      deleted = false
+      @forced_decision_mutex.synchronize do
+        if @forced_decisions.key?(forced_decision_key)
+          @forced_decisions.delete(forced_decision_key)
+          deleted = true
+        end
       end
-
-      false
+      deleted
     end
 
     # Removes all forced decisions bound to this user context.
@@ -158,7 +160,7 @@ def remove_forced_decision(flag_key, rule_key = nil)
     def remove_all_forced_decision
       return false if @optimizely_client&.get_optimizely_config.nil?
 
-      @forced_decisions.clear
+      @forced_decision_mutex.synchronize { @forced_decisions.clear }
       true
     end
 

spec/config/datafile_project_config_spec.rb

@@ -23,6 +23,7 @@
 describe Optimizely::DatafileProjectConfig do
   let(:config_body) { OptimizelySpec::VALID_CONFIG_BODY }
   let(:config_body_JSON) { OptimizelySpec::VALID_CONFIG_BODY_JSON }
+  let(:decision_JSON) { OptimizelySpec::DECIDE_FORCED_DECISION_JSON }
   let(:error_handler) { Optimizely::NoOpErrorHandler.new }
   let(:logger) { Optimizely::NoOpLogger.new }
   let(:config) { Optimizely::DatafileProjectConfig.new(config_body_JSON, logger, error_handler) }
@@ -1073,4 +1074,56 @@
       expect(config.rollout_experiment?('177771')).to eq(false)
     end
   end
+
+  describe '#feature variation map' do
+    let(:config) { Optimizely::DatafileProjectConfig.new(decision_JSON, logger, error_handler) }
+
+    it 'should return valid flag variation map without duplicates' do
+      # variation '3324490634' is repeated in datafile but should appear once in map
+      expected_feature_variation_map = {
+        'feature_1' => [{
+          'variables' => [],
+          'featureEnabled' => true,
+          'id' => '10389729780',
+          'key' => 'a'
+        }, {
+          'variables' => [],
+          'id' => '10416523121',
+          'key' => 'b',
+          'featureEnabled' => false
+        }, {
+          'featureEnabled' => true,
+          'id' => '3324490633',
+          'key' => '3324490633',
+          'variables' => []
+        }, {
+          'featureEnabled' => true,
+          'id' => '3324490634',
+          'key' => '3324490634',
+          'variables' => []
+        }, {
+          'featureEnabled' => true,
+          'id' => '3324490562',
+          'key' => '3324490562',
+          'variables' => []
+        }, {
+          'variables' => [],
+          'id' => '18257766532',
+          'key' => '18257766532',
+          'featureEnabled' => true
+        }], 'feature_2' => [{
+          'variables' => [],
+          'featureEnabled' => true,
+          'id' => '10418551353',
+          'key' => 'variation_with_traffic'
+        }, {
+          'variables' => [],
+          'featureEnabled' => false,
+          'id' => '10418510624',
+          'key' => 'variation_no_traffic'
+        }], 'feature_3' => []
+      }
+      expect(config.send(:generate_feature_variation_map, config.feature_flags)).to eq(expected_feature_variation_map)
+    end
+  end
 end