validate parameters in feature APIs (#98)

* add additional error messages around invalid inputs to feature APIs
* add parameter validation to isFeatureEnabled
* add parameter validation and logs to getFeatureVariable
* return None if user ID is None
* add null parameter tests for get_feature_variable_*
* add null parameter tests for is_feature_enabled

Author: wangjoshuah

optimizely/helpers/enums.py

@@ -29,16 +29,21 @@ class LogLevels(object):
 
 
 class Errors(object):
-  INVALID_INPUT_ERROR = 'Provided "{}" is in an invalid format.'
   INVALID_ATTRIBUTE_ERROR = 'Provided attribute is not in datafile.'
   INVALID_ATTRIBUTE_FORMAT = 'Attributes provided are in an invalid format.'
-  INVALID_EVENT_TAG_FORMAT = 'Event tags provided are in an invalid format.'
   INVALID_AUDIENCE_ERROR = 'Provided audience is not in datafile.'
+  INVALID_DATAFILE = 'Datafile has invalid format. Failing "{}".'
+  INVALID_EVENT_TAG_FORMAT = 'Event tags provided are in an invalid format.'
   INVALID_EXPERIMENT_KEY_ERROR = 'Provided experiment is not in datafile.'
   INVALID_EVENT_KEY_ERROR = 'Provided event is not in datafile.'
-  INVALID_DATAFILE = 'Datafile has invalid format. Failing "{}".'
+  INVALID_FEATURE_KEY_ERROR = 'Provided feature key is not in the datafile.'
   INVALID_GROUP_ID_ERROR = 'Provided group is not in datafile.'
+  INVALID_INPUT_ERROR = 'Provided "{}" is in an invalid format.'
   INVALID_VARIATION_ERROR = 'Provided variation is not in datafile.'
+  INVALID_VARIABLE_KEY_ERROR = 'Provided variable key is not in the feature flag.'
+  NONE_FEATURE_KEY_PARAMETER = '"None" is an invalid value for feature key.'
+  NONE_USER_ID_PARAMETER = '"None" is an invalid value for user ID.'
+  NONE_VARIABLE_KEY_PARAMETER = '"None" is an invalid value for variable key.'
   UNSUPPORTED_DATAFILE_VERSION = 'Provided datafile has unsupported version. ' \
                                  'Please use SDK version 1.1.0 or earlier for datafile version 1.'
 

optimizely/optimizely.py

@@ -198,6 +198,17 @@ 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.log(enums.LogLevels.ERROR, enums.Errors.NONE_FEATURE_KEY_PARAMETER)
+      return None
+
+    if variable_key is None:
+      self.logger.log(enums.LogLevels.ERROR, enums.Errors.NONE_VARIABLE_KEY_PARAMETER)
+      return None
+
+    if user_id is None:
+      self.logger.log(enums.LogLevels.ERROR, enums.Errors.NONE_USER_ID_PARAMETER)
+      return None
 
     feature_flag = self.config.get_feature_from_key(feature_key)
     if not feature_flag:
@@ -375,6 +386,14 @@ def is_feature_enabled(self, feature_key, user_id, attributes=None):
       self.logger.log(enums.LogLevels.ERROR, enums.Errors.INVALID_DATAFILE.format('is_feature_enabled'))
       return False
 
+    if feature_key is None:
+      self.logger.log(enums.LogLevels.ERROR, enums.Errors.NONE_FEATURE_KEY_PARAMETER)
+      return False
+
+    if user_id is None:
+      self.logger.log(enums.LogLevels.ERROR, enums.Errors.NONE_USER_ID_PARAMETER)
+      return False
+
     feature = self.config.get_feature_from_key(feature_key)
     if not feature:
       return False

tests/test_optimizely.py

@@ -1173,6 +1173,26 @@ def test_get_variation__invalid_object(self):
 
     mock_logging.assert_called_once_with(enums.LogLevels.ERROR, 'Datafile has invalid format. Failing "get_variation".')
 
+  def test_is_feature_enabled__returns_false_for_none_feature_key(self):
+    """ Test that is_feature_enabled returns false if the provided feature key is None. """
+
+    opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features))
+
+    with mock.patch('optimizely.logger.NoOpLogger.log') as mock_logger:
+      self.assertFalse(opt_obj.is_feature_enabled(None, 'test_user'))
+
+    mock_logger.assert_called_once_with(enums.LogLevels.ERROR, enums.Errors.NONE_FEATURE_KEY_PARAMETER)
+
+  def test_is_feature_enabled__returns_false_for_none_user_id(self):
+    """ Test that is_feature_enabled returns false if the provided user ID is None. """
+
+    opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features))
+
+    with mock.patch('optimizely.logger.NoOpLogger.log') as mock_logger:
+      self.assertFalse(opt_obj.is_feature_enabled('feature_key', None))
+
+    mock_logger.assert_called_once_with(enums.LogLevels.ERROR, enums.Errors.NONE_USER_ID_PARAMETER)
+
   def test_is_feature_enabled__returns_false_for_invalid_feature(self):
     """ Test that the feature is not enabled for the user if the provided feature key is invalid. """
 
@@ -1449,6 +1469,54 @@ def test_get_feature_variable__returns_default_value(self):
       'Returning default value for variable "environment" of feature flag "test_feature_in_experiment".'
     )
 
+  def test_get_feature_variable__returns_none_if_none_feature_key(self):
+    """ Test that get_feature_variable_* returns None for None feature key. """
+
+    opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features))
+    with mock.patch('optimizely.logger.NoOpLogger.log') as mock_logger:
+      self.assertIsNone(opt_obj.get_feature_variable_boolean(None, 'variable_key', 'test_user'))
+      mock_logger.assert_called_with(enums.LogLevels.ERROR, enums.Errors.NONE_FEATURE_KEY_PARAMETER)
+      self.assertIsNone(opt_obj.get_feature_variable_double(None, 'variable_key', 'test_user'))
+      mock_logger.assert_called_with(enums.LogLevels.ERROR, enums.Errors.NONE_FEATURE_KEY_PARAMETER)
+      self.assertIsNone(opt_obj.get_feature_variable_integer(None, 'variable_key', 'test_user'))
+      mock_logger.assert_called_with(enums.LogLevels.ERROR, enums.Errors.NONE_FEATURE_KEY_PARAMETER)
+      self.assertIsNone(opt_obj.get_feature_variable_string(None, 'variable_key', 'test_user'))
+      mock_logger.assert_called_with(enums.LogLevels.ERROR, enums.Errors.NONE_FEATURE_KEY_PARAMETER)
+
+    self.assertEqual(4, mock_logger.call_count)
+
+  def test_get_feature_variable__returns_none_if_none_variable_key(self):
+    """ Test that get_feature_variable_* returns None for None variable key. """
+
+    opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features))
+    with mock.patch('optimizely.logger.NoOpLogger.log') as mock_logger:
+      self.assertIsNone(opt_obj.get_feature_variable_boolean('feature_key', None, 'test_user'))
+      mock_logger.assert_called_with(enums.LogLevels.ERROR, enums.Errors.NONE_VARIABLE_KEY_PARAMETER)
+      self.assertIsNone(opt_obj.get_feature_variable_double('feature_key', None, 'test_user'))
+      mock_logger.assert_called_with(enums.LogLevels.ERROR, enums.Errors.NONE_VARIABLE_KEY_PARAMETER)
+      self.assertIsNone(opt_obj.get_feature_variable_integer('feature_key', None, 'test_user'))
+      mock_logger.assert_called_with(enums.LogLevels.ERROR, enums.Errors.NONE_VARIABLE_KEY_PARAMETER)
+      self.assertIsNone(opt_obj.get_feature_variable_string('feature_key', None, 'test-User'))
+      mock_logger.assert_called_with(enums.LogLevels.ERROR, enums.Errors.NONE_VARIABLE_KEY_PARAMETER)
+
+    self.assertEqual(4, mock_logger.call_count)
+
+  def test_get_feature_variable__returns_none_if_none_user_id(self):
+    """ Test that get_feature_variable_* returns None for None user ID. """
+
+    opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features))
+    with mock.patch('optimizely.logger.NoOpLogger.log') as mock_logger:
+      self.assertIsNone(opt_obj.get_feature_variable_boolean('feature_key', 'variable_key', None))
+      mock_logger.assert_called_with(enums.LogLevels.ERROR, enums.Errors.NONE_USER_ID_PARAMETER)
+      self.assertIsNone(opt_obj.get_feature_variable_double('feature_key', 'variable_key', None))
+      mock_logger.assert_called_with(enums.LogLevels.ERROR, enums.Errors.NONE_USER_ID_PARAMETER)
+      self.assertIsNone(opt_obj.get_feature_variable_integer('feature_key', 'variable_key', None))
+      mock_logger.assert_called_with(enums.LogLevels.ERROR, enums.Errors.NONE_USER_ID_PARAMETER)
+      self.assertIsNone(opt_obj.get_feature_variable_string('feature_key', 'variable_key', None))
+      mock_logger.assert_called_with(enums.LogLevels.ERROR, enums.Errors.NONE_USER_ID_PARAMETER)
+
+    self.assertEqual(4, mock_logger.call_count)
+
   def test_get_feature_variable__returns_none_if_invalid_feature_key(self):
     """ Test that get_feature_variable_* returns None for invalid feature key. """