Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Dec 28, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7320a3c
feat(attribute_value): Don't target NAN, INF, -INF and Integers > 1e53
oakbani Dec 10, 2018
c532525
nit. blank lines
oakbani Dec 10, 2018
5b96ff2
Merge remote-tracking branch 'remotes/origin/master' into oakbani/-do…
oakbani Dec 20, 2018
66159a8
:pen: Revise number
oakbani Dec 20, 2018
da583c8
tests: refact
oakbani Dec 21, 2018
24a6350
docstrings/namings
oakbani Dec 24, 2018
8c6fbb4
Update exact_match testcase
oakbani Dec 27, 2018
3137fd4
Merge branch 'master' into oakbani/-dont-target-nan-inf-1e53
aliabbasrizvi Dec 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions optimizely/helpers/validator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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


Expand Down
5 changes: 5 additions & 0 deletions tests/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):

Expand Down
114 changes: 109 additions & 5 deletions tests/helpers_tests/test_condition.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Expand Down Expand Up @@ -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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. Please be explicit in naming the mocks. ...as mock_is_finite

side_effect=[False, True]) as mock_is_finite:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nchilada In exact match need to send exactly same values to ensure that it's the mocked is_finite that is really driving the result. Therefore, can't make a value-base side-effect method as in gt/lt methods.
This returns False on 1st call and True on 2nd.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right. Good solution. Thanks @oakbani!

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(
Expand Down Expand Up @@ -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):

Expand Down
59 changes: 59 additions & 0 deletions tests/helpers_tests/test_validator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, thanks for figuring out the need for + 2!

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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://stackoverflow.com/questions/1848700/biggest-integer-that-can-be-stored-in-a-double

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this is really testing the behavior of floating-point arithmetic, so I'm not sure we need this assertion. But if we keep it, maybe we could include a code comment like

# float(2**53) + 1.0 evaluates to float(2**53)

so that code readers aren't confused by this scenario

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):

Expand Down