feat(attribute_value): Don't target NAN, INF, -INF and > 2^53 (#151)

Author: oakbani

optimizely/helpers/validator.py

@@ -187,20 +187,25 @@ def is_attribute_valid(attribute_key, attribute_value):
   if not isinstance(attribute_key, string_types):
     return False
 
-  if isinstance(attribute_value, string_types) or type(attribute_value) in (int, float, bool):
+  if isinstance(attribute_value, (string_types, bool)):
     return True
 
+  if isinstance(attribute_value, (numbers.Integral, float)):
+    return is_finite_number(attribute_value)
+
   return False
 
 
 def is_finite_number(value):
-  """ Method to validate if the given value is a number and not one of NAN, INF, -INF.
+  """ Validates if the given value is a number, enforces
+   absolute limit of 2^53 and restricts NAN, INF, -INF.
 
   Args:
     value: Value to be validated.
 
   Returns:
-    Boolean: True if value is a number and not NAN, INF or -INF else False.
+    Boolean: True if value is a number and not NAN, INF, -INF or
+             greater than absolute limit of 2^53 else False.
   """
   if not isinstance(value, (numbers.Integral, float)):
       # numbers.Integral instead of int to accomodate long integer in python 2
@@ -214,6 +219,9 @@ def is_finite_number(value):
     if math.isnan(value) or math.isinf(value):
       return False
 
+  if abs(value) > (2**53):
+    return False
+
   return True
 
 

tests/base.py

@@ -13,9 +13,14 @@
 
 import json
 import unittest
+from six import PY3
 
 from optimizely import optimizely
 
+if PY3:
+  def long(a):
+    raise NotImplementedError('Tests should only call `long` if running in PY2')
+
 
 class BaseTest(unittest.TestCase):
 

tests/helpers_tests/test_condition.py

@@ -12,16 +12,12 @@
 # limitations under the License.
 
 import mock
-from six import PY2, PY3
+from six import PY2
 
 from optimizely.helpers import condition as condition_helper
 
 from tests import base
 
-if PY3:
-  def long(a):
-    raise NotImplementedError('Tests should only call `long` if running in PY2')
-
 browserConditionSafari = ['browser_type', 'safari', 'custom_attribute', 'exact']
 booleanCondition = ['is_firefox', True, 'custom_attribute', 'exact']
 integerCondition = ['num_users', 10, 'custom_attribute', 'exact']
@@ -286,6 +282,36 @@ def test_exact_float__returns_null__when_no_user_provided_value(self):
 
     self.assertIsNone(evaluator.evaluate(0))
 
+  def test_exact__given_number_values__calls_is_finite_number(self):
+    """ Test that CustomAttributeConditionEvaluator.evaluate returns True
+        if is_finite_number returns True. Returns None if is_finite_number returns False. """
+
+    evaluator = condition_helper.CustomAttributeConditionEvaluator(
+        exact_int_condition_list, {'lasers_count': 9000}
+      )
+
+    # assert that isFiniteNumber only needs to reject condition value to stop evaluation.
+    with mock.patch('optimizely.helpers.validator.is_finite_number',
+                    side_effect=[False, True]) as mock_is_finite:
+      self.assertIsNone(evaluator.evaluate(0))
+
+    mock_is_finite.assert_called_once_with(9000)
+
+    # assert that isFiniteNumber evaluates user value only if it has accepted condition value.
+    with mock.patch('optimizely.helpers.validator.is_finite_number',
+                    side_effect=[True, False]) as mock_is_finite:
+      self.assertIsNone(evaluator.evaluate(0))
+
+    mock_is_finite.assert_has_calls([mock.call(9000), mock.call(9000)])
+
+    # assert CustomAttributeConditionEvaluator.evaluate returns True only when isFiniteNumber returns
+    # True both for condition and user values.
+    with mock.patch('optimizely.helpers.validator.is_finite_number',
+                    side_effect=[True, True]) as mock_is_finite:
+      self.assertTrue(evaluator.evaluate(0))
+
+    mock_is_finite.assert_has_calls([mock.call(9000), mock.call(9000)])
+
   def test_exact_bool__returns_true__when_user_provided_value_is_equal_to_condition_value(self):
 
     evaluator = condition_helper.CustomAttributeConditionEvaluator(
@@ -594,6 +620,84 @@ def test_less_than_float__returns_null__when_no_user_provided_value(self):
 
     self.assertIsNone(evaluator.evaluate(0))
 
+  def test_greater_than__calls_is_finite_number(self):
+    """ Test that CustomAttributeConditionEvaluator.evaluate returns True
+        if is_finite_number returns True. Returns None if is_finite_number returns False. """
+
+    evaluator = condition_helper.CustomAttributeConditionEvaluator(
+      gt_int_condition_list, {'meters_travelled': 48.1}
+    )
+
+    def is_finite_number__rejecting_condition_value(value):
+      if value == 48:
+        return False
+      return True
+
+    with mock.patch('optimizely.helpers.validator.is_finite_number',
+                    side_effect=is_finite_number__rejecting_condition_value) as mock_is_finite:
+      self.assertIsNone(evaluator.evaluate(0))
+
+    # assert that isFiniteNumber only needs to reject condition value to stop evaluation.
+    mock_is_finite.assert_called_once_with(48)
+
+    def is_finite_number__rejecting_user_attribute_value(value):
+      if value == 48.1:
+        return False
+      return True
+
+    with mock.patch('optimizely.helpers.validator.is_finite_number',
+                    side_effect=is_finite_number__rejecting_user_attribute_value) as mock_is_finite:
+      self.assertIsNone(evaluator.evaluate(0))
+
+    # assert that isFiniteNumber evaluates user value only if it has accepted condition value.
+    mock_is_finite.assert_has_calls([mock.call(48), mock.call(48.1)])
+
+    def is_finite_number__accepting_both_values(value):
+      return True
+
+    with mock.patch('optimizely.helpers.validator.is_finite_number',
+                    side_effect=is_finite_number__accepting_both_values):
+      self.assertTrue(evaluator.evaluate(0))
+
+  def test_less_than__calls_is_finite_number(self):
+    """ Test that CustomAttributeConditionEvaluator.evaluate returns True
+        if is_finite_number returns True. Returns None if is_finite_number returns False. """
+
+    evaluator = condition_helper.CustomAttributeConditionEvaluator(
+      lt_int_condition_list, {'meters_travelled': 47}
+    )
+
+    def is_finite_number__rejecting_condition_value(value):
+      if value == 48:
+        return False
+      return True
+
+    with mock.patch('optimizely.helpers.validator.is_finite_number',
+                    side_effect=is_finite_number__rejecting_condition_value) as mock_is_finite:
+      self.assertIsNone(evaluator.evaluate(0))
+
+    # assert that isFiniteNumber only needs to reject condition value to stop evaluation.
+    mock_is_finite.assert_called_once_with(48)
+
+    def is_finite_number__rejecting_user_attribute_value(value):
+      if value == 47:
+        return False
+      return True
+
+    with mock.patch('optimizely.helpers.validator.is_finite_number',
+                    side_effect=is_finite_number__rejecting_user_attribute_value) as mock_is_finite:
+      self.assertIsNone(evaluator.evaluate(0))
+
+    # assert that isFiniteNumber evaluates user value only if it has accepted condition value.
+    mock_is_finite.assert_has_calls([mock.call(48), mock.call(47)])
+
+    def is_finite_number__accepting_both_values(value):
+      return True
+
+    with mock.patch('optimizely.helpers.validator.is_finite_number',
+                    side_effect=is_finite_number__accepting_both_values):
+      self.assertTrue(evaluator.evaluate(0))
+
 
 class ConditionDecoderTests(base.BaseTest):
 

tests/helpers_tests/test_validator.py

@@ -12,6 +12,9 @@
 # limitations under the License.
 
 import json
+import mock
+
+from six import PY2
 
 from optimizely import error_handler
 from optimizely import event_dispatcher
@@ -168,6 +171,62 @@ def test_is_attribute_valid(self):
     self.assertTrue(validator.is_attribute_valid('test_attribute', ""))
     self.assertTrue(validator.is_attribute_valid('test_attribute', 'test_value'))
 
+    # test if attribute value is a number, it calls is_finite_number and returns it's result
+    with mock.patch('optimizely.helpers.validator.is_finite_number',
+                    return_value=True) as mock_is_finite:
+      self.assertTrue(validator.is_attribute_valid('test_attribute', 5))
+
+    mock_is_finite.assert_called_once_with(5)
+
+    with mock.patch('optimizely.helpers.validator.is_finite_number',
+                    return_value=False) as mock_is_finite:
+      self.assertFalse(validator.is_attribute_valid('test_attribute', 5.5))
+
+    mock_is_finite.assert_called_once_with(5.5)
+
+    if PY2:
+      with mock.patch('optimizely.helpers.validator.is_finite_number',
+                      return_value=None) as mock_is_finite:
+        self.assertIsNone(validator.is_attribute_valid('test_attribute', long(5)))
+
+      mock_is_finite.assert_called_once_with(long(5))
+
+  def test_is_finite_number(self):
+    """ Test that it returns true if value is a number and not NAN, INF, -INF or greater than 2^53.
+        Otherwise False.
+    """
+    # test non number values
+    self.assertFalse(validator.is_finite_number('HelloWorld'))
+    self.assertFalse(validator.is_finite_number(True))
+    self.assertFalse(validator.is_finite_number(False))
+    self.assertFalse(validator.is_finite_number(None))
+    self.assertFalse(validator.is_finite_number({}))
+    self.assertFalse(validator.is_finite_number([]))
+    self.assertFalse(validator.is_finite_number(()))
+
+    # test invalid numbers
+    self.assertFalse(validator.is_finite_number(float('inf')))
+    self.assertFalse(validator.is_finite_number(float('-inf')))
+    self.assertFalse(validator.is_finite_number(float('nan')))
+    self.assertFalse(validator.is_finite_number(int(2**53) + 1))
+    self.assertFalse(validator.is_finite_number(-int(2**53) - 1))
+    self.assertFalse(validator.is_finite_number(float(2**53) + 2.0))
+    self.assertFalse(validator.is_finite_number(-float(2**53) - 2.0))
+    if PY2:
+      self.assertFalse(validator.is_finite_number(long(2**53) + 1))
+      self.assertFalse(validator.is_finite_number(-long(2**53) - 1))
+
+    # test valid numbers
+    self.assertTrue(validator.is_finite_number(0))
+    self.assertTrue(validator.is_finite_number(5))
+    self.assertTrue(validator.is_finite_number(5.5))
+    # float(2**53) + 1.0 evaluates to float(2**53)
+    self.assertTrue(validator.is_finite_number(float(2**53) + 1.0))
+    self.assertTrue(validator.is_finite_number(-float(2**53) - 1.0))
+    self.assertTrue(validator.is_finite_number(int(2**53)))
+    if PY2:
+      self.assertTrue(validator.is_finite_number(long(2**53)))
+
 
 class DatafileValidationTests(base.BaseTest):