Duplicate experiment Key issue with multi feature flag (#347)

* Changing lookup of experiment in decision service to use experiment ID instead of key to resolve bug

* [BUGFIX] - Update to bucketer and added proper ID map to project_config.

* Switch to generate experiments_key_map from experiment_id_map rather than the other way around.

* Updated with unit tests.

* Change line break before operator to avoid W504 Flake8 issue.

* Flake8 changes to follow best practice and ignore the W504

Author: The-inside-man

.flake8

@@ -1,5 +1,8 @@
 [flake8]
 # E722 - do not use bare 'except'
-ignore = E722
+# W504 - Either W503 (Line break after Operand) or W503 (
+# Line break before operand needs to be ignored for line lengths
+# greater than max-line-length. Best practice shows W504
+ignore = E722, W504
 exclude = optimizely/lib/pymmh3.py,*virtualenv*
 max-line-length = 120

optimizely/bucketer.py

@@ -142,7 +142,7 @@ def bucket(self, project_config, experiment, user_id, bucketing_id):
         variation_id = self.find_bucket(project_config, bucketing_id,
                                         experiment.id, experiment.trafficAllocation)
         if 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
 
         else:

optimizely/decision_service.py

@@ -342,7 +342,7 @@ def get_variation_for_rollout(self, project_config, rollout, user_id, attributes
         if rollout and len(rollout.experiments) > 0:
             for idx in range(len(rollout.experiments) - 1):
                 logging_key = str(idx + 1)
-                rollout_rule = project_config.get_experiment_from_key(rollout.experiments[idx].get('key'))
+                rollout_rule = project_config.get_experiment_from_id(rollout.experiments[idx].get('id'))
 
                 # Check if user meets audience conditions for targeting rule
                 audience_conditions = rollout_rule.get_audience_conditions_or_ids()
@@ -387,7 +387,7 @@ def get_variation_for_rollout(self, project_config, rollout, user_id, attributes
                     break
 
             # Evaluate last rule i.e. "Everyone Else" rule
-            everyone_else_rule = project_config.get_experiment_from_key(rollout.experiments[-1].get('key'))
+            everyone_else_rule = project_config.get_experiment_from_id(rollout.experiments[-1].get('id'))
             audience_conditions = everyone_else_rule.get_audience_conditions_or_ids()
             audience_eval, audience_reasons = audience_helper.does_user_meet_audience_conditions(
                 project_config,

optimizely/project_config.py

@@ -68,7 +68,7 @@ def __init__(self, datafile, logger, error_handler):
 
         # Utility maps for quick lookup
         self.group_id_map = self._generate_key_map(self.groups, 'id', entities.Group)
-        self.experiment_key_map = self._generate_key_map(self.experiments, 'key', entities.Experiment)
+        self.experiment_id_map = self._generate_key_map(self.experiments, 'id', entities.Experiment)
         self.event_key_map = self._generate_key_map(self.events, 'key', entities.Event)
         self.attribute_key_map = self._generate_key_map(self.attributes, 'key', entities.Attribute)
 
@@ -84,27 +84,36 @@ def __init__(self, datafile, logger, error_handler):
         self.rollout_id_map = self._generate_key_map(self.rollouts, 'id', entities.Layer)
         for layer in self.rollout_id_map.values():
             for experiment in layer.experiments:
-                self.experiment_key_map[experiment['key']] = entities.Experiment(**experiment)
+                self.experiment_id_map[experiment['id']] = entities.Experiment(**experiment)
 
         self.audience_id_map = self._deserialize_audience(self.audience_id_map)
         for group in self.group_id_map.values():
-            experiments_in_group_key_map = self._generate_key_map(group.experiments, 'key', entities.Experiment)
-            for experiment in experiments_in_group_key_map.values():
+            experiments_in_group_id_map = self._generate_key_map(group.experiments, 'id', entities.Experiment)
+            for experiment in experiments_in_group_id_map.values():
                 experiment.__dict__.update({'groupId': group.id, 'groupPolicy': group.policy})
-            self.experiment_key_map.update(experiments_in_group_key_map)
+            self.experiment_id_map.update(experiments_in_group_id_map)
 
-        self.experiment_id_map = {}
+        self.experiment_key_map = {}
         self.variation_key_map = {}
         self.variation_id_map = {}
         self.variation_variable_usage_map = {}
-        for experiment in self.experiment_key_map.values():
-            self.experiment_id_map[experiment.id] = experiment
+        self.variation_id_map_by_experiment_id = {}
+        self.variation_key_map_by_experiment_id = {}
+
+        for experiment in self.experiment_id_map.values():
+            self.experiment_key_map[experiment.key] = experiment
             self.variation_key_map[experiment.key] = self._generate_key_map(
                 experiment.variations, 'key', entities.Variation
             )
+
             self.variation_id_map[experiment.key] = {}
+            self.variation_id_map_by_experiment_id[experiment.id] = {}
+            self.variation_key_map_by_experiment_id[experiment.id] = {}
+
             for variation in self.variation_key_map.get(experiment.key).values():
                 self.variation_id_map[experiment.key][variation.id] = variation
+                self.variation_id_map_by_experiment_id[experiment.id][variation.id] = variation
+                self.variation_key_map_by_experiment_id[experiment.id][variation.key] = variation
                 self.variation_variable_usage_map[variation.id] = self._generate_key_map(
                     variation.variables, 'id', entities.Variation.VariableUsage
                 )
@@ -557,3 +566,35 @@ def is_feature_experiment(self, experiment_id):
         """
 
         return experiment_id in self.experiment_feature_map
+
+    def get_variation_from_id_by_experiment_id(self, experiment_id, variation_id):
+        """ Gets variation from variation id and specific experiment id
+
+            Returns:
+                The variation for the experiment id and variation id
+                or empty dict if not found
+        """
+        if (experiment_id in self.variation_id_map_by_experiment_id and
+                variation_id in self.variation_id_map_by_experiment_id[experiment_id]):
+            return self.variation_id_map_by_experiment_id[experiment_id][variation_id]
+
+        self.logger.error('Variation with id "%s" not defined in the datafile for experiment "%s".',
+                          variation_id, experiment_id)
+
+        return {}
+
+    def get_variation_from_key_by_experiment_id(self, experiment_id, variation_key):
+        """ Gets variation from variation key and specific experiment id
+
+            Returns:
+                The variation for the experiment id and variation key
+                or empty dict if not found
+        """
+        if (experiment_id in self.variation_key_map_by_experiment_id and
+                variation_key in self.variation_key_map_by_experiment_id[experiment_id]):
+            return self.variation_key_map_by_experiment_id[experiment_id][variation_key]
+
+        self.logger.error('Variation with key "%s" not defined in the datafile for experiment "%s".',
+                          variation_key, experiment_id)
+
+        return {}

tests/test_config.py

@@ -41,6 +41,7 @@ def test_init(self):
                 self.config_dict['groups'][0]['trafficAllocation'],
             )
         }
+
         expected_experiment_key_map = {
             'test_experiment': entities.Experiment(
                 '111127',
@@ -1213,3 +1214,27 @@ def test_is_feature_experiment(self):
 
         self.assertStrictFalse(project_config.is_feature_experiment(experiment.id))
         self.assertStrictTrue(project_config.is_feature_experiment(feature_experiment.id))
+
+    def test_get_variation_from_id_by_experiment_id(self):
+
+        opt_obj = optimizely.Optimizely(json.dumps(self.config_dict))
+        project_config = opt_obj.config_manager.get_config()
+
+        experiment_id = '111127'
+        variation_id = '111128'
+
+        variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)
+
+        self.assertIsInstance(variation, entities.Variation)
+
+    def test_get_variation_from_key_by_experiment_id(self):
+
+        opt_obj = optimizely.Optimizely(json.dumps(self.config_dict))
+        project_config = opt_obj.config_manager.get_config()
+
+        experiment_id = '111127'
+        variation_key = 'control'
+
+        variation = project_config.get_variation_from_key_by_experiment_id(experiment_id, variation_key)
+
+        self.assertIsInstance(variation, entities.Variation)