feat(api): Accept all types of Attribute values (#142)

oakbani · mikeproeng37 · commit 42d74b2646ea · 2018-09-19T12:11:38.000-07:00
diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py
@@ -12,6 +12,7 @@
 # limitations under the License.
 
 from collections import namedtuple
+from six import string_types
 
 from . import bucketer
 from .helpers import audience as audience_helper
@@ -34,20 +35,27 @@ def __init__(self, config, user_profile_service):
     self.config = config
     self.logger = config.logger
 
-  @staticmethod
-  def _get_bucketing_id(user_id, attributes):
+  def _get_bucketing_id(self, user_id, attributes):
     """ Helper method to determine bucketing ID for the user.
 
     Args:
       user_id: ID for user.
       attributes: Dict representing user attributes. May consist of bucketing ID to be used.
 
     Returns:
-      String representing bucketing ID for the user. Fallback to user's ID if not provided.
+      String representing bucketing ID if it is a String type in attributes else return user ID.
     """
 
     attributes = attributes or {}
-    return attributes.get(enums.ControlAttributes.BUCKETING_ID, user_id)
+    bucketing_id = attributes.get(enums.ControlAttributes.BUCKETING_ID)
+
+    if bucketing_id is not None:
+      if isinstance(bucketing_id, string_types):
+        return bucketing_id
+
+      self.logger.warning('Bucketing ID attribute is not a string. Defaulted to user_id.')
+
+    return user_id
 
   def get_forced_variation(self, experiment, user_id):
     """ Determine if a user is forced into a variation for the given experiment and return that variation.
diff --git a/optimizely/event_builder.py b/optimizely/event_builder.py
@@ -19,6 +19,7 @@
 from . import version
 from .helpers import enums
 from .helpers import event_tag_utils
+from .helpers import validator
 
 
 class Event(object):
@@ -182,8 +183,8 @@ def _get_attributes(self, attributes):
     if isinstance(attributes, dict):
       for attribute_key in attributes.keys():
         attribute_value = attributes.get(attribute_key)
-        # Omit falsy attribute values
-        if attribute_value:
+        # Omit attribute values that are not supported by the log endpoint.
+        if validator.is_attribute_valid(attribute_key, attribute_value):
           attribute_id = self.config.get_attribute_id(attribute_key)
           if attribute_id:
             params.append({
diff --git a/optimizely/helpers/validator.py b/optimizely/helpers/validator.py
@@ -167,3 +167,25 @@ def is_non_empty_string(input_id_key):
     return True
 
   return False
+
+
+def is_attribute_valid(attribute_key, attribute_value):
+  """ Determine if given attribute is valid.
+
+  Args:
+    attribute_key: Variable which needs to be validated
+    attribute_value: Variable which needs to be validated
+
+  Returns:
+    False if attribute_key is not a string
+    False if attribute_value is not one of the supported attribute types
+    True otherwise
+  """
+
+  if not isinstance(attribute_key, string_types):
+    return False
+
+  if isinstance(attribute_value, string_types) or type(attribute_value) in (int, float, bool):
+    return True
+
+  return False
diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py
@@ -212,6 +212,9 @@ def _get_feature_variable_for_type(self, feature_key, variable_key, variable_typ
       self.logger.error(enums.Errors.NONE_USER_ID_PARAMETER)
       return None
 
+    if not self._validate_user_inputs(attributes):
+      return None
+
     feature_flag = self.config.get_feature_from_key(feature_key)
     if not feature_flag:
       return None
@@ -407,6 +410,9 @@ def is_feature_enabled(self, feature_key, user_id, attributes=None):
       self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('user_id'))
       return False
 
+    if not self._validate_user_inputs(attributes):
+      return False
+
     feature = self.config.get_feature_from_key(feature_key)
     if not feature:
       return False
@@ -447,6 +453,9 @@ def get_enabled_features(self, user_id, attributes=None):
       self.logger.error(enums.Errors.INVALID_INPUT_ERROR.format('user_id'))
       return enabled_features
 
+    if not self._validate_user_inputs(attributes):
+      return enabled_features
+
     for feature in self.config.feature_key_map.values():
       if self.is_feature_enabled(feature.key, user_id, attributes):
         enabled_features.append(feature.key)
diff --git a/tests/base.py b/tests/base.py
@@ -124,6 +124,15 @@ def setUp(self, config_dict='config_dict'):
       'attributes': [{
         'key': 'test_attribute',
         'id': '111094'
+      }, {
+        'key': 'boolean_key',
+        'id': '111196'
+      }, {
+        'key': 'integer_key',
+        'id': '111197'
+      }, {
+        'key': 'double_key',
+        'id': '111198'
       }],
       'audiences': [{
         'name': 'Test attribute users 1',
@@ -506,6 +515,15 @@ def setUp(self, config_dict='config_dict'):
       'attributes': [{
         'key': 'test_attribute',
         'id': '111094'
+      }, {
+        'key': 'boolean_key',
+        'id': '111196'
+      }, {
+        'key': 'integer_key',
+        'id': '111197'
+      }, {
+        'key': 'double_key',
+        'id': '111198'
       }],
       'audiences': [{
         'name': 'Test attribute users 1',
diff --git a/tests/helpers_tests/test_condition.py b/tests/helpers_tests/test_condition.py
@@ -1,4 +1,4 @@
-# Copyright 2016-2017, Optimizely
+# Copyright 2016-2018, Optimizely
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
@@ -33,10 +33,29 @@ def setUp(self):
     self.condition_evaluator = condition_helper.ConditionEvaluator(self.condition_list, attributes)
 
   def test_evaluator__returns_true(self):
-    """ Test that evaluator correctly returns True when there is a match. """
+    """ Test that evaluator correctly returns True when there is an exact match.
+    Also test that evaluator works for falsy values. """
 
+    # string attribute value
+    condition_list = [['test_attribute', '']]
+    condition_evaluator = condition_helper.ConditionEvaluator(condition_list, {'test_attribute': ''})
     self.assertTrue(self.condition_evaluator.evaluator(0))
 
+    # boolean attribute value
+    condition_list = [['boolean_key', False]]
+    condition_evaluator = condition_helper.ConditionEvaluator(condition_list, {'boolean_key': False})
+    self.assertTrue(condition_evaluator.evaluator(0))
+
+    # integer attribute value
+    condition_list = [['integer_key', 0]]
+    condition_evaluator = condition_helper.ConditionEvaluator(condition_list, {'integer_key': 0})
+    self.assertTrue(condition_evaluator.evaluator(0))
+
+    # double attribute value
+    condition_list = [['double_key', 0.0]]
+    condition_evaluator = condition_helper.ConditionEvaluator(condition_list, {'double_key': 0.0})
+    self.assertTrue(condition_evaluator.evaluator(0))
+
   def test_evaluator__returns_false(self):
     """ Test that evaluator correctly returns False when there is no match. """
 
diff --git a/tests/helpers_tests/test_validator.py b/tests/helpers_tests/test_validator.py
@@ -146,6 +146,28 @@ def test_is_non_empty_string(self):
     self.assertTrue(validator.is_non_empty_string('0'))
     self.assertTrue(validator.is_non_empty_string('test_user'))
 
+  def test_is_attribute_valid(self):
+    """ Test that non-string attribute key or unsupported attribute value returns False."""
+
+    # test invalid attribute keys
+    self.assertFalse(validator.is_attribute_valid(5, 'test_value'))
+    self.assertFalse(validator.is_attribute_valid(True, 'test_value'))
+    self.assertFalse(validator.is_attribute_valid(5.5, 'test_value'))
+
+    # test invalid attribute values
+    self.assertFalse(validator.is_attribute_valid('test_attribute', None))
+    self.assertFalse(validator.is_attribute_valid('test_attribute', {}))
+    self.assertFalse(validator.is_attribute_valid('test_attribute', []))
+    self.assertFalse(validator.is_attribute_valid('test_attribute', ()))
+
+    # test valid attribute values
+    self.assertTrue(validator.is_attribute_valid('test_attribute', False))
+    self.assertTrue(validator.is_attribute_valid('test_attribute', True))
+    self.assertTrue(validator.is_attribute_valid('test_attribute', 0))
+    self.assertTrue(validator.is_attribute_valid('test_attribute', 0.0))
+    self.assertTrue(validator.is_attribute_valid('test_attribute', ""))
+    self.assertTrue(validator.is_attribute_valid('test_attribute', 'test_value'))
+
 
 class DatafileValidationTests(base.BaseTest):
 
diff --git a/tests/test_config.py b/tests/test_config.py
@@ -110,6 +110,9 @@ def test_init(self):
       'Total Revenue': entities.Event('111096', 'Total Revenue', ['111127'])
     }
     expected_attribute_key_map = {
+      'boolean_key': entities.Attribute('111196', 'boolean_key'),
+      'double_key': entities.Attribute('111198', 'double_key'),
+      'integer_key': entities.Attribute('111197', 'integer_key'),
       'test_attribute': entities.Attribute('111094', 'test_attribute', segmentId='11133')
     }
     expected_audience_id_map = {
diff --git a/tests/test_decision_service.py b/tests/test_decision_service.py
@@ -34,18 +34,42 @@ def test_get_bucketing_id__no_bucketing_id_attribute(self):
     """ Test that _get_bucketing_id returns correct bucketing ID when there is no bucketing ID attribute. """
 
     # No attributes
-    self.assertEqual('test_user', decision_service.DecisionService._get_bucketing_id('test_user', None))
+    self.assertEqual('test_user', self.decision_service._get_bucketing_id('test_user', None))
 
     # With attributes, but no bucketing ID
-    self.assertEqual('test_user', decision_service.DecisionService._get_bucketing_id('test_user',
+    self.assertEqual('test_user', self.decision_service._get_bucketing_id('test_user',
                                                                                      {'random_key': 'random_value'}))
 
   def test_get_bucketing_id__bucketing_id_attribute(self):
     """ Test that _get_bucketing_id returns correct bucketing ID when there is bucketing ID attribute. """
+    with mock.patch.object(self.decision_service, 'logger') as mock_decision_logging:
+      self.assertEqual('user_bucket_value',
+                       self.decision_service._get_bucketing_id('test_user',
+                                                                          {'$opt_bucketing_id': 'user_bucket_value'}))
+      mock_decision_logging.debug.assert_not_called()
 
-    self.assertEqual('user_bucket_value',
-                     decision_service.DecisionService._get_bucketing_id('test_user',
-                                                                        {'$opt_bucketing_id': 'user_bucket_value'}))
+  def test_get_bucketing_id__bucketing_id_attribute_not_a_string(self):
+    """ Test that _get_bucketing_id returns user ID as  bucketing ID when bucketing ID attribute is not a string"""
+    with mock.patch.object(self.decision_service, 'logger') as mock_decision_logging:
+      self.assertEqual('test_user',
+                       self.decision_service._get_bucketing_id('test_user',
+                                                                          {'$opt_bucketing_id': True}))
+      mock_decision_logging.warning.assert_called_once_with(
+        'Bucketing ID attribute is not a string. Defaulted to user_id.')
+      mock_decision_logging.reset_mock()
+
+      self.assertEqual('test_user',
+                       self.decision_service._get_bucketing_id('test_user',
+                                                                          {'$opt_bucketing_id': 5.9}))
+      mock_decision_logging.warning.assert_called_once_with(
+        'Bucketing ID attribute is not a string. Defaulted to user_id.')
+      mock_decision_logging.reset_mock()
+
+      self.assertEqual('test_user',
+                       self.decision_service._get_bucketing_id('test_user',
+                                                                          {'$opt_bucketing_id': 5}))
+      mock_decision_logging.warning.assert_called_once_with(
+        'Bucketing ID attribute is not a string. Defaulted to user_id.')
 
   def test_get_forced_variation__user_in_forced_variation(self):
     """ Test that expected variation is returned if user is forced in a variation. """
diff --git a/tests/test_event_builder.py b/tests/test_event_builder.py
@@ -189,6 +189,75 @@ def test_create_impression_event_when_attribute_is_not_in_datafile(self):
                                   event_builder.EventBuilder.HTTP_VERB,
                                   event_builder.EventBuilder.HTTP_HEADERS)
 
+  def test_create_impression_event_calls_is_attribute_valid(self):
+    """ Test that create_impression_event calls is_attribute_valid and
+    creates Event object with only those attributes for which is_attribute_valid is True."""
+
+    expected_params = {
+        'account_id': '12001',
+        'project_id': '111001',
+        'visitors': [{
+            'visitor_id': 'test_user',
+            'attributes': [{
+                'type': 'custom',
+                'value': 5.5,
+                'entity_id': '111198',
+                'key': 'double_key'
+            }, {
+                'type': 'custom',
+                'value': True,
+                'entity_id': '111196',
+                'key': 'boolean_key'
+            }],
+            'snapshots': [{
+                'decisions': [{
+                    'variation_id': '111129',
+                    'experiment_id': '111127',
+                    'campaign_id': '111182'
+                }],
+                'events': [{
+                    'timestamp': 42123,
+                    'entity_id': '111182',
+                    'uuid': 'a68cf1ad-0393-4e18-af87-efe8f01a7c9c',
+                    'key': 'campaign_activated'
+                }]
+            }]
+        }],
+        'client_name': 'python-sdk',
+        'client_version': version.__version__,
+        'anonymize_ip': False,
+        'revision': '42'
+    }
+
+    def side_effect(*args, **kwargs):
+      attribute_key = args[0]
+      if attribute_key == 'boolean_key' or attribute_key == 'double_key':
+        return True
+
+      return False
+
+    attributes = {
+        'test_attribute': 'test_value',
+        'boolean_key': True,
+        'integer_key': 0,
+        'double_key': 5.5
+      }
+
+    with mock.patch('time.time', return_value=42.123), \
+         mock.patch('uuid.uuid4', return_value='a68cf1ad-0393-4e18-af87-efe8f01a7c9c'),\
+         mock.patch('optimizely.helpers.validator.is_attribute_valid', side_effect=side_effect):
+
+      event_obj = self.event_builder.create_impression_event(
+        self.project_config.get_experiment_from_key('test_experiment'),
+        '111129', 'test_user', attributes
+      )
+
+    self._validate_event_object(event_obj,
+                                event_builder.EventBuilder.EVENTS_URL,
+                                expected_params,
+                                event_builder.EventBuilder.HTTP_VERB,
+                                event_builder.EventBuilder.HTTP_HEADERS)
+
   def test_create_impression_event__with_user_agent_when_bot_filtering_is_enabled(self):
     """ Test that create_impression_event creates Event object
     with right params when user agent attribute is provided and
diff --git a/tests/test_optimizely.py b/tests/test_optimizely.py

Original file line number	Diff line number	Diff line change
`@@ -110,6 +110,9 @@ def test_init(self):`
`110`	`110`	`'Total Revenue': entities.Event('111096', 'Total Revenue', ['111127'])`
`111`	`111`	`}`
`112`	`112`	`expected_attribute_key_map = {`
	`113`	`+ 'boolean_key': entities.Attribute('111196', 'boolean_key'),`
	`114`	`+ 'double_key': entities.Attribute('111198', 'double_key'),`
	`115`	`+ 'integer_key': entities.Attribute('111197', 'integer_key'),`
`113`	`116`	`'test_attribute': entities.Attribute('111094', 'test_attribute', segmentId='11133')`
`114`	`117`	`}`
`115`	`118`	`expected_audience_id_map = {`