make remaining changes to manager & update tests

Author: mredolatti

splitio/client/input_validator.py

@@ -365,7 +365,7 @@ def validate_value(value):
     return value
 
 
-def validate_manager_feature_name(feature_name):
+def validate_manager_feature_name(feature_name, should_validate_existance, split_storage):
     """
     Check if feature_name is valid for track.
 
@@ -378,6 +378,15 @@ def validate_manager_feature_name(feature_name):
        (not _check_is_string(feature_name, 'feature_name', 'split')) or \
        (not _check_string_not_empty(feature_name, 'feature_name', 'split')):
         return None
+
+    if should_validate_existance and split_storage.get(feature_name) is None:
+        _LOGGER.error(
+            "split: you passed \"%s\" that does not exist in this environment, "
+            "please double check what Splits exist in the web console.",
+            feature_name
+        )
+        return None
+
     return feature_name
 
 

splitio/client/manager.py

@@ -18,7 +18,7 @@ def __init__(self, factory):
         """
         self._logger = logging.getLogger(self.__class__.__name__)
         self._factory = factory
-        self._storage = factory._get_storage('splits')
+        self._storage = factory._get_storage('splits')  #pylint: disable=protected-access
 
     def split_names(self):
         """
@@ -60,7 +60,12 @@ def split(self, feature_name):
             self._logger.error("Client has already been destroyed - no calls possible.")
             return []
 
-        feature_name = input_validator.validate_manager_feature_name(feature_name)
+        feature_name = input_validator.validate_manager_feature_name(
+            feature_name,
+            self._factory.ready,
+            self._storage
+        )
+
         if feature_name is None:
             return None
 

splitio/storage/adapters/cache_trait.py

@@ -180,7 +180,9 @@ def _decorator(user_function):
         :rtype: callable
         """
         _cache = LocalMemoryCache(key_func, user_function, max_age_seconds, max_size)
-        wrapper = _cache.get
+        # The lambda below IS necessary, otherwise update_wrapper fails since the function
+        # is an instance method and has no reference to the __module__ namespace.
+        wrapper = lambda *args, **kwargs: _cache.get(*args, **kwargs)  #pylint: disable=unnecessary-lambda
         return update_wrapper(wrapper, user_function)
 
     return _decorator

splitio/storage/redis.py

@@ -9,7 +9,7 @@
 from splitio.models import splits, segments
 from splitio.storage import SplitStorage, SegmentStorage, ImpressionStorage, EventStorage
 from splitio.storage.adapters.redis import RedisAdapterException
-
+from splitio.storage.adapters.cache_trait import decorate as add_cache, DEFAULT_MAX_AGE
 
 class RedisSplitStorage(SplitStorage):
     """Redis-based storage for splits."""
@@ -18,7 +18,7 @@ class RedisSplitStorage(SplitStorage):
     _SPLIT_TILL_KEY = 'SPLITIO.splits.till'
     _TRAFFIC_TYPE_KEY = 'SPLITIO.trafficType.{traffic_type_name}'
 
-    def __init__(self, redis_client):
+    def __init__(self, redis_client, enable_caching=False, max_age=DEFAULT_MAX_AGE):
         """
         Class constructor.
 
@@ -27,6 +27,9 @@ def __init__(self, redis_client):
         """
         self._logger = logging.getLogger(self.__class__.__name__)
         self._redis = redis_client
+        if enable_caching:
+            self.get = add_cache(lambda *p, **_: p[0], max_age)(self.get)
+            self.is_valid_traffic_type = add_cache(lambda *p, **_: p[0], max_age)(self.is_valid_traffic_type)  #pylint: disable=line-too-long
 
     def _get_key(self, split_name):
         """
@@ -52,7 +55,7 @@ def _get_traffic_type_key(self, traffic_type_name):
         """
         return self._TRAFFIC_TYPE_KEY.format(traffic_type_name=traffic_type_name)
 
-    def get(self, split_name):
+    def get(self, split_name):  #pylint: disable=method-hidden
         """
         Retrieve a split.
 
@@ -70,7 +73,7 @@ def get(self, split_name):
             self._logger.debug('Error: ', exc_info=True)
             return None
 
-    def is_valid_traffic_type(self, traffic_type_name):
+    def is_valid_traffic_type(self, traffic_type_name):  #pylint: disable=method-hidden
         """
         Return whether the traffic type exists in at least one split in cache.
 

tests/client/test_input_validator.py

@@ -967,6 +967,16 @@ def test_split_(self, mocker):
         assert split_mock.to_split_view.mock_calls == [mocker.call()]
         assert manager._logger.error.mock_calls == []
 
+        manager._logger.reset_mock()
+        split_mock.reset_mock()
+        storage_mock.get.return_value = None
+        manager.split('nonexistant-split')
+        assert split_mock.to_split_view.mock_calls == []
+        assert manager._logger.error.mock_calls == [mocker.call(
+            "split: you passed \"%s\" that does not exist in this environment, "
+            "please double check what Splits exist in the web console.",
+            'nonexistant-split'
+        )]
 
 class FactoryInputValidationTests(object):  #pylint: disable=too-few-public-methods
     """Factory instantiation input validation test cases."""

tests/integration/test_client_e2e.py

@@ -531,6 +531,44 @@ def teardown_method(self):
             redis_client.delete(key)
 
 
+class RedisWithCacheIntegrationTests(RedisIntegrationTests):
+    """Run the same tests as RedisIntegratioTests but with LRU/Expirable cache overlay."""
+
+    def setup_method(self):
+        """Prepare storages with test data."""
+        metadata = SdkMetadata('python-1.2.3', 'some_ip', 'some_name')
+        redis_client = RedisAdapter(StrictRedis())
+        split_storage = RedisSplitStorage(redis_client, True)
+        segment_storage = RedisSegmentStorage(redis_client)
+
+        split_fn = os.path.join(os.path.dirname(__file__), 'files', 'splitChanges.json')
+        with open(split_fn, 'r') as flo:
+            data = json.loads(flo.read())
+        for split in data['splits']:
+            redis_client.set(split_storage._get_key(split['name']), json.dumps(split))
+        redis_client.set(split_storage._SPLIT_TILL_KEY, data['till'])
+
+        segment_fn = os.path.join(os.path.dirname(__file__), 'files', 'segmentEmployeesChanges.json')
+        with open(segment_fn, 'r') as flo:
+            data = json.loads(flo.read())
+        redis_client.sadd(segment_storage._get_key(data['name']), *data['added'])
+        redis_client.set(segment_storage._get_till_key(data['name']), data['till'])
+
+        segment_fn = os.path.join(os.path.dirname(__file__), 'files', 'segmentHumanBeignsChanges.json')
+        with open(segment_fn, 'r') as flo:
+            data = json.loads(flo.read())
+        redis_client.sadd(segment_storage._get_key(data['name']), *data['added'])
+        redis_client.set(segment_storage._get_till_key(data['name']), data['till'])
+
+        self.factory = SplitFactory('some_api_key', {  #pylint:disable=attribute-defined-outside-init
+            'splits': split_storage,
+            'segments': segment_storage,
+            'impressions': RedisImpressionsStorage(redis_client, metadata),
+            'events': RedisEventsStorage(redis_client, metadata),
+            'telemetry': RedisTelemetryStorage(redis_client, metadata)
+        }, True)
+
+
 class LocalhostIntegrationTests(object):  #pylint: disable=too-few-public-methods
     """Client & Manager integration tests."""
 

tests/storage/adapters/test_cache_trait.py

@@ -97,7 +97,7 @@ def test_decorate(self, mocker):
         user_func = mocker.Mock()
 
         cache_trait.decorate(key_func)(user_func)
-        assert update_wrapper_mock.mock_calls == [mocker.call(returned_instance_mock.get, user_func)]
+        assert update_wrapper_mock.mock_calls == [mocker.call(mocker.ANY, user_func)]
         assert local_memory_cache_mock.mock_calls == [
             mocker.call(key_func, user_func, cache_trait.DEFAULT_MAX_AGE, cache_trait.DEFAULT_MAX_SIZE)
         ]

tests/storage/test_redis.py

@@ -2,6 +2,7 @@
 #pylint: disable=no-self-use
 
 import json
+import time
 
 from splitio.client.util import get_metadata
 from splitio.storage.redis import RedisEventsStorage, RedisImpressionsStorage, \
@@ -32,12 +33,42 @@ def test_get_split(self, mocker):
         adapter.reset_mock()
         from_raw.reset_mock()
         adapter.get.return_value = None
-
         result = storage.get('some_split')
         assert result is None
         assert adapter.get.mock_calls == [mocker.call('SPLITIO.split.some_split')]
         assert not from_raw.mock_calls
 
+    def test_get_split_with_cache(self, mocker):
+        """Test retrieving a split works."""
+        adapter = mocker.Mock(spec=RedisAdapter)
+        adapter.get.return_value = '{"name": "some_split"}'
+        from_raw = mocker.Mock()
+        mocker.patch('splitio.models.splits.from_raw', new=from_raw)
+
+        storage = RedisSplitStorage(adapter, True, 1)
+        storage.get('some_split')
+        assert adapter.get.mock_calls == [mocker.call('SPLITIO.split.some_split')]
+        assert from_raw.mock_calls == [mocker.call({"name": "some_split"})]
+
+        # hit the cache:
+        storage.get('some_split')
+        storage.get('some_split')
+        storage.get('some_split')
+        assert adapter.get.mock_calls == [mocker.call('SPLITIO.split.some_split')]
+        assert from_raw.mock_calls == [mocker.call({"name": "some_split"})]
+
+        # Test that a missing split returns None and doesn't call from_raw
+        adapter.reset_mock()
+        from_raw.reset_mock()
+        adapter.get.return_value = None
+
+        result = storage.get('some_split')
+        assert result is not None
+        time.sleep(1)  # wait for expiration
+        result = storage.get('some_split')
+        assert result is None
+        assert adapter.get.mock_calls == [mocker.call('SPLITIO.split.some_split')]
+        assert not from_raw.mock_calls
 
     def test_get_changenumber(self, mocker):
         """Test fetching changenumber."""
@@ -100,6 +131,22 @@ def test_is_valid_traffic_type(self, mocker):
         adapter.get.return_value = None
         assert storage.is_valid_traffic_type('any') is False
 
+    def test_is_valid_traffic_type_with_cache(self, mocker):
+        """Test that traffic type validation works."""
+        adapter = mocker.Mock(spec=RedisAdapter)
+        storage = RedisSplitStorage(adapter, True, 1)
+
+        adapter.get.return_value = '1'
+        assert storage.is_valid_traffic_type('any') is True
+
+        adapter.get.return_value = '0'
+        assert storage.is_valid_traffic_type('any') is True
+        time.sleep(1)
+        assert storage.is_valid_traffic_type('any') is False
+
+        adapter.get.return_value = None
+        time.sleep(1)
+        assert storage.is_valid_traffic_type('any') is False
 
 
 class RedisSegmentStorageTests(object):