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

rashidsp · mikeproeng37 · commit e5ec1188b2c5 · 2018-11-26T10:15:42.000-08:00
diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py
@@ -10,6 +10,8 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+from six import string_types
+
 from . import decision_service
 from . import entities
 from . import event_builder
@@ -200,16 +202,16 @@ def _get_feature_variable_for_type(self, feature_key, variable_key, variable_typ
       - Variable key is invalid.
       - Mismatch with type of variable.
     """
-    if feature_key is None:
-      self.logger.error(enums.Errors.NONE_FEATURE_KEY_PARAMETER)
+    if not validator.is_non_empty_string(feature_key):
+      self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('feature_key'))
       return None
 
-    if variable_key is None:
-      self.logger.error(enums.Errors.NONE_VARIABLE_KEY_PARAMETER)
+    if not validator.is_non_empty_string(variable_key):
+      self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('variable_key'))
       return None
 
-    if user_id is None:
-      self.logger.error(enums.Errors.NONE_USER_ID_PARAMETER)
+    if not isinstance(user_id, string_types):
+      self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('user_id'))
       return None
 
     if not self._validate_user_inputs(attributes):
@@ -271,7 +273,7 @@ def activate(self, experiment_key, user_id, attributes=None):
       self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('experiment_key'))
       return None
 
-    if not validator.is_non_empty_string(user_id):
+    if not isinstance(user_id, string_types):
       self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('user_id'))
       return None
 
@@ -308,7 +310,7 @@ def track(self, event_key, user_id, attributes=None, event_tags=None):
       self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('event_key'))
       return
 
-    if not validator.is_non_empty_string(user_id):
+    if not isinstance(user_id, string_types):
       self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('user_id'))
       return
 
@@ -364,7 +366,7 @@ def get_variation(self, experiment_key, user_id, attributes=None):
       self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('experiment_key'))
       return None
 
-    if not validator.is_non_empty_string(user_id):
+    if not isinstance(user_id, string_types):
       self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('user_id'))
       return None
 
@@ -406,7 +408,7 @@ def is_feature_enabled(self, feature_key, user_id, attributes=None):
       self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('feature_key'))
       return False
 
-    if not validator.is_non_empty_string(user_id):
+    if not isinstance(user_id, string_types):
       self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('user_id'))
       return False
 
@@ -449,7 +451,7 @@ def get_enabled_features(self, user_id, attributes=None):
       self.logger.error(enums.Errors.INVALID_DATAFILE.format('get_enabled_features'))
       return enabled_features
 
-    if not validator.is_non_empty_string(user_id):
+    if not isinstance(user_id, string_types):
       self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('user_id'))
       return enabled_features
 
@@ -551,6 +553,18 @@ def set_forced_variation(self, experiment_key, user_id, variation_key):
       A boolean value that indicates if the set completed successfully.
     """
 
+    if not self.is_valid:
+      self.logger.error(enums.Errors.INVALID_DATAFILE.format('set_forced_variation'))
+      return False
+
+    if not validator.is_non_empty_string(experiment_key):
+      self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('experiment_key'))
+      return False
+
+    if not isinstance(user_id, string_types):
+      self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('user_id'))
+      return False
+
     return self.config.set_forced_variation(experiment_key, user_id, variation_key)
 
   def get_forced_variation(self, experiment_key, user_id):
@@ -564,5 +578,17 @@ def get_forced_variation(self, experiment_key, user_id):
       The forced variation key. None if no forced variation key.
     """
 
+    if not self.is_valid:
+      self.logger.error(enums.Errors.INVALID_DATAFILE.format('get_forced_variation'))
+      return None
+
+    if not validator.is_non_empty_string(experiment_key):
+      self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('experiment_key'))
+      return None
+
+    if not isinstance(user_id, string_types):
+      self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('user_id'))
+      return None
+
     forced_variation = self.config.get_forced_variation(experiment_key, user_id)
     return forced_variation.key if forced_variation else None
diff --git a/optimizely/project_config.py b/optimizely/project_config.py
@@ -490,10 +490,6 @@ def set_forced_variation(self, experiment_key, user_id, variation_key):
       Returns:
         A boolean value that indicates if the set completed successfully.
     """
-    if not user_id:
-      self.logger.debug('User ID is invalid.')
-      return False
-
     experiment = self.get_experiment_from_key(experiment_key)
     if not experiment:
       # The invalid experiment key will be logged inside this call.
@@ -551,9 +547,6 @@ def get_forced_variation(self, experiment_key, user_id):
       Returns:
         The variation which the given user and experiment should be forced into.
     """
-    if not user_id:
-      self.logger.debug('User ID is invalid.')
-      return None
 
     if user_id not in self.forced_variation_map:
       self.logger.debug('User "%s" is not in the forced variation map.' % user_id)
diff --git a/tests/test_config.py b/tests/test_config.py
@@ -1035,13 +1035,6 @@ def test_get_forced_variation_missing_variation_mapped_to_experiment(self):
       'No variation mapped to experiment "test_experiment" in the forced variation map.'
     )
 
-  # set_forced_variation tests
-  def test_set_forced_variation__invalid_user_id(self):
-    """ Test invalid user IDs set fail to set a forced variation """
-
-    self.assertFalse(self.project_config.set_forced_variation('test_experiment', None, 'variation'))
-    self.assertFalse(self.project_config.set_forced_variation('test_experiment', '', 'variation'))
-
   def test_set_forced_variation__invalid_experiment_key(self):
     """ Test invalid experiment keys set fail to set a forced variation """
 
diff --git a/tests/test_optimizely.py b/tests/test_optimizely.py
@@ -1285,12 +1285,8 @@ def test_track__invalid_user_id(self):
     """ Test that None is returned and expected log messages are logged during track \
     when user_id is in invalid format. """
 
-    with mock.patch.object(self.optimizely, 'logger') as mock_client_logging, \
-         mock.patch('optimizely.helpers.validator.is_non_empty_string', side_effect=[True, False]) as mock_validator:
+    with mock.patch.object(self.optimizely, 'logger') as mock_client_logging:
       self.assertIsNone(self.optimizely.track('test_event', 99))
-
-    mock_validator.assert_any_call(99)
-
     mock_client_logging.error.assert_called_once_with('Provided "user_id" is in an invalid format.')
 
   def test_get_variation__invalid_object(self):
@@ -1329,11 +1325,8 @@ def test_is_feature_enabled__returns_false_for_invalid_user_id(self):
 
     opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features))
 
-    with mock.patch.object(opt_obj, 'logger') as mock_client_logging,\
-         mock.patch('optimizely.helpers.validator.is_non_empty_string', side_effect=[True, False]) as mock_validator:
+    with mock.patch.object(opt_obj, 'logger') as mock_client_logging:
       self.assertFalse(opt_obj.is_feature_enabled('feature_key', 1.2))
-
-    mock_validator.assert_any_call(1.2)
     mock_client_logging.error.assert_called_with('Provided "user_id" is in an invalid format.')
 
   def test_is_feature_enabled__returns_false_for__invalid_attributes(self):
@@ -1628,11 +1621,9 @@ def side_effect(*args, **kwargs):
     mock_is_feature_enabled.assert_any_call('test_feature_in_experiment_and_rollout', 'user_1', None)
 
   def test_get_enabled_features_invalid_user_id(self):
-    with mock.patch.object(self.optimizely, 'logger') as mock_client_logging, \
-            mock.patch('optimizely.helpers.validator.is_non_empty_string', return_value=False) as mock_validator:
-      self.optimizely.get_enabled_features(1.2)
+    with mock.patch.object(self.optimizely, 'logger') as mock_client_logging:
+      self.assertEqual([], self.optimizely.get_enabled_features(1.2))
 
-    mock_validator.assert_any_call(1.2)
     mock_client_logging.error.assert_called_once_with('Provided "user_id" is in an invalid format.')
 
   def test_get_enabled_features__invalid_attributes(self):
@@ -1853,22 +1844,22 @@ def test_get_feature_variable__returns_none_if_none_feature_key(self):
     with mock.patch.object(opt_obj, 'logger') as mock_client_logger:
       # Check for booleans
       self.assertIsNone(opt_obj.get_feature_variable_boolean(None, 'variable_key', 'test_user'))
-      mock_client_logger.error.assert_called_with(enums.Errors.NONE_FEATURE_KEY_PARAMETER)
+      mock_client_logger.error.assert_called_with('Provided "feature_key" is in an invalid format.')
       mock_client_logger.reset_mock()
 
       # Check for doubles
       self.assertIsNone(opt_obj.get_feature_variable_double(None, 'variable_key', 'test_user'))
-      mock_client_logger.error.assert_called_with(enums.Errors.NONE_FEATURE_KEY_PARAMETER)
+      mock_client_logger.error.assert_called_with('Provided "feature_key" is in an invalid format.')
       mock_client_logger.reset_mock()
 
       # Check for integers
       self.assertIsNone(opt_obj.get_feature_variable_integer(None, 'variable_key', 'test_user'))
-      mock_client_logger.error.assert_called_with(enums.Errors.NONE_FEATURE_KEY_PARAMETER)
+      mock_client_logger.error.assert_called_with('Provided "feature_key" is in an invalid format.')
       mock_client_logger.reset_mock()
 
       # Check for strings
       self.assertIsNone(opt_obj.get_feature_variable_string(None, 'variable_key', 'test_user'))
-      mock_client_logger.error.assert_called_with(enums.Errors.NONE_FEATURE_KEY_PARAMETER)
+      mock_client_logger.error.assert_called_with('Provided "feature_key" is in an invalid format.')
       mock_client_logger.reset_mock()
 
   def test_get_feature_variable__returns_none_if_none_variable_key(self):
@@ -1878,22 +1869,22 @@ def test_get_feature_variable__returns_none_if_none_variable_key(self):
     with mock.patch.object(opt_obj, 'logger') as mock_client_logger:
       # Check for booleans
       self.assertIsNone(opt_obj.get_feature_variable_boolean('feature_key', None, 'test_user'))
-      mock_client_logger.error.assert_called_with(enums.Errors.NONE_VARIABLE_KEY_PARAMETER)
+      mock_client_logger.error.assert_called_with('Provided "variable_key" is in an invalid format.')
       mock_client_logger.reset_mock()
 
       # Check for doubles
       self.assertIsNone(opt_obj.get_feature_variable_double('feature_key', None, 'test_user'))
-      mock_client_logger.error.assert_called_with(enums.Errors.NONE_VARIABLE_KEY_PARAMETER)
+      mock_client_logger.error.assert_called_with('Provided "variable_key" is in an invalid format.')
       mock_client_logger.reset_mock()
 
       # Check for integers
       self.assertIsNone(opt_obj.get_feature_variable_integer('feature_key', None, 'test_user'))
-      mock_client_logger.error.assert_called_with(enums.Errors.NONE_VARIABLE_KEY_PARAMETER)
+      mock_client_logger.error.assert_called_with('Provided "variable_key" is in an invalid format.')
       mock_client_logger.reset_mock()
 
       # Check for strings
       self.assertIsNone(opt_obj.get_feature_variable_string('feature_key', None, 'test-User'))
-      mock_client_logger.error.assert_called_with(enums.Errors.NONE_VARIABLE_KEY_PARAMETER)
+      mock_client_logger.error.assert_called_with('Provided "variable_key" is in an invalid format.')
       mock_client_logger.reset_mock()
 
   def test_get_feature_variable__returns_none_if_none_user_id(self):
@@ -1903,22 +1894,22 @@ def test_get_feature_variable__returns_none_if_none_user_id(self):
     with mock.patch.object(opt_obj, 'logger') as mock_client_logger:
       # Check for booleans
       self.assertIsNone(opt_obj.get_feature_variable_boolean('feature_key', 'variable_key', None))
-      mock_client_logger.error.assert_called_with(enums.Errors.NONE_USER_ID_PARAMETER)
+      mock_client_logger.error.assert_called_with('Provided "user_id" is in an invalid format.')
       mock_client_logger.reset_mock()
 
       # Check for doubles
       self.assertIsNone(opt_obj.get_feature_variable_double('feature_key', 'variable_key', None))
-      mock_client_logger.error.assert_called_with(enums.Errors.NONE_USER_ID_PARAMETER)
+      mock_client_logger.error.assert_called_with('Provided "user_id" is in an invalid format.')
       mock_client_logger.reset_mock()
 
       # Check for integers
       self.assertIsNone(opt_obj.get_feature_variable_integer('feature_key', 'variable_key', None))
-      mock_client_logger.error.assert_called_with(enums.Errors.NONE_USER_ID_PARAMETER)
+      mock_client_logger.error.assert_called_with('Provided "user_id" is in an invalid format.')
       mock_client_logger.reset_mock()
 
       # Check for strings
       self.assertIsNone(opt_obj.get_feature_variable_string('feature_key', 'variable_key', None))
-      mock_client_logger.error.assert_called_with(enums.Errors.NONE_USER_ID_PARAMETER)
+      mock_client_logger.error.assert_called_with('Provided "user_id" is in an invalid format.')
       mock_client_logger.reset_mock()
 
   def test_get_feature_variable__invalid_attributes(self):
@@ -2246,11 +2237,8 @@ def test_get_variation__invalid_user_id(self):
     """ Test that None is returned and expected log messages are logged during get_variation \
     when user_id is in invalid format. """
 
-    with mock.patch.object(self.optimizely, 'logger') as mock_client_logging,\
-         mock.patch('optimizely.helpers.validator.is_non_empty_string', side_effect=[True, False]) as mock_validator:
+    with mock.patch.object(self.optimizely, 'logger') as mock_client_logging:
       self.assertIsNone(self.optimizely.get_variation('test_experiment', 99))
-
-    mock_validator.assert_any_call(99)
     mock_client_logging.error.assert_called_once_with('Provided "user_id" is in an invalid format.')
 
   def test_activate__invalid_experiment_key(self):
@@ -2269,14 +2257,35 @@ def test_activate__invalid_user_id(self):
     """ Test that None is returned and expected log messages are logged during activate \
     when user_id is in invalid format. """
 
-    with mock.patch.object(self.optimizely, 'logger') as mock_client_logging,\
-         mock.patch('optimizely.helpers.validator.is_non_empty_string', side_effect=[True, False]) as mock_validator:
+    with mock.patch.object(self.optimizely, 'logger') as mock_client_logging:
       self.assertIsNone(self.optimizely.activate('test_experiment', 99))
 
-    mock_validator.assert_any_call(99)
-
     mock_client_logging.error.assert_called_once_with('Provided "user_id" is in an invalid format.')
 
+  def test_activate__empty_user_id(self):
+    """ Test that expected log messages are logged during activate. """
+
+    variation_key = 'variation'
+    experiment_key = 'test_experiment'
+    user_id = ''
+
+    with mock.patch('optimizely.decision_service.DecisionService.get_variation',
+                    return_value=self.project_config.get_variation_from_id(
+                      'test_experiment', '111129')), \
+         mock.patch('time.time', return_value=42), \
+         mock.patch('optimizely.event_dispatcher.EventDispatcher.dispatch_event'), \
+         mock.patch.object(self.optimizely, 'logger') as mock_client_logging:
+      self.assertEqual(variation_key, self.optimizely.activate(experiment_key, user_id))
+
+    mock_client_logging.info.assert_called_once_with(
+      'Activating user "" in experiment "test_experiment".'
+    )
+    debug_message = mock_client_logging.debug.call_args_list[0][0][0]
+    self.assertRegexpMatches(
+      debug_message,
+      'Dispatching impression event to URL https://logx.optimizely.com/v1/events with params'
+    )
+
   def test_activate__invalid_attributes(self):
     """ Test that expected log messages are logged during activate when attributes are in invalid format. """
     with mock.patch.object(self.optimizely, 'logger') as mock_client_logging:
@@ -2373,3 +2382,64 @@ def test_get_variation__invalid_attributes__forced_bucketing(self):
                                                   'test_user',
                                                   attributes={'test_attribute': 'test_value_invalid'})
     self.assertEqual('variation', variation_key)
+
+  def test_set_forced_variation__invalid_object(self):
+    """ Test that set_forced_variation logs error if Optimizely object is not created correctly. """
+
+    opt_obj = optimizely.Optimizely('invalid_datafile')
+
+    with mock.patch.object(opt_obj, 'logger') as mock_client_logging:
+      self.assertFalse(opt_obj.set_forced_variation('test_experiment', 'test_user', 'test_variation'))
+
+    mock_client_logging.error.assert_called_once_with('Datafile has invalid format. Failing "set_forced_variation".')
+
+  def test_set_forced_variation__invalid_experiment_key(self):
+    """ Test that None is returned and expected log messages are logged during set_forced_variation \
+    when exp_key is in invalid format. """
+
+    with mock.patch.object(self.optimizely, 'logger') as mock_client_logging, \
+            mock.patch('optimizely.helpers.validator.is_non_empty_string', return_value=False) as mock_validator:
+      self.assertFalse(self.optimizely.set_forced_variation(99, 'test_user', 'variation'))
+
+    mock_validator.assert_any_call(99)
+
+    mock_client_logging.error.assert_called_once_with('Provided "experiment_key" is in an invalid format.')
+
+  def test_set_forced_variation__invalid_user_id(self):
+    """ Test that None is returned and expected log messages are logged during set_forced_variation \
+    when user_id is in invalid format. """
+
+    with mock.patch.object(self.optimizely, 'logger') as mock_client_logging:
+      self.assertFalse(self.optimizely.set_forced_variation('test_experiment', 99, 'variation'))
+    mock_client_logging.error.assert_called_once_with('Provided "user_id" is in an invalid format.')
+
+  def test_get_forced_variation__invalid_object(self):
+    """ Test that get_forced_variation logs error if Optimizely object is not created correctly. """
+
+    opt_obj = optimizely.Optimizely('invalid_datafile')
+
+    with mock.patch.object(opt_obj, 'logger') as mock_client_logging:
+      self.assertIsNone(opt_obj.get_forced_variation('test_experiment', 'test_user'))
+
+    mock_client_logging.error.assert_called_once_with('Datafile has invalid format. Failing "get_forced_variation".')
+
+  def test_get_forced_variation__invalid_experiment_key(self):
+    """ Test that None is returned and expected log messages are logged during get_forced_variation \
+    when exp_key is in invalid format. """
+
+    with mock.patch.object(self.optimizely, 'logger') as mock_client_logging, \
+            mock.patch('optimizely.helpers.validator.is_non_empty_string', return_value=False) as mock_validator:
+      self.assertIsNone(self.optimizely.get_forced_variation(99, 'test_user'))
+
+    mock_validator.assert_any_call(99)
+
+    mock_client_logging.error.assert_called_once_with('Provided "experiment_key" is in an invalid format.')
+
+  def test_get_forced_variation__invalid_user_id(self):
+    """ Test that None is returned and expected log messages are logged during get_forced_variation \
+    when user_id is in invalid format. """
+
+    with mock.patch.object(self.optimizely, 'logger') as mock_client_logging:
+      self.assertIsNone(self.optimizely.get_forced_variation('test_experiment', 99))
+
+    mock_client_logging.error.assert_called_once_with('Provided "user_id" is in an invalid format.')
