Merge pull request #137 from splitio/feature/input-validation-v2-multiple-factories

Feature/input validation v2 multiple factories

Author: mredolatti

splitio/client/factory.py

@@ -4,8 +4,9 @@
 
 import logging
 import threading
-from enum import Enum
+from collections import Counter
 
+from enum import Enum
 import six
 
 from splitio.client.client import Client
@@ -45,6 +46,11 @@
     LocalhostSplitSynchronizationTask, LocalhostTelemetryStorage
 
 
+_LOGGER = logging.getLogger(__name__)
+_INSTANTIATED_FACTORIES = Counter()
+_INSTANTIATED_FACTORIES_LOCK = threading.RLock()
+
+
 class Status(Enum):
     """Factory Status."""
 
@@ -64,6 +70,7 @@ class SplitFactory(object):  #pylint: disable=too-many-instance-attributes
 
     def __init__(  #pylint: disable=too-many-arguments
             self,
+            apikey,
             storages,
             labels_enabled,
             apis=None,
@@ -87,6 +94,7 @@ def __init__(  #pylint: disable=too-many-arguments
         :param impression_listener: User custom listener to handle impressions locally.
         :type impression_listener: splitio.client.listener.ImpressionListener
         """
+        self._apikey = apikey
         self._logger = logging.getLogger(self.__class__.__name__)
         self._storages = storages
         self._labels_enabled = labels_enabled
@@ -197,6 +205,8 @@ def _wait_for_tasks_to_stop():
                     task.stop()
         finally:
             self._status = Status.DESTROYED
+            with _INSTANTIATED_FACTORIES_LOCK:
+                _INSTANTIATED_FACTORIES.subtract([self._apikey])
 
     @property
     def destroyed(self):
@@ -321,6 +331,7 @@ def segment_ready_task():
     segment_completion_thread.setDaemon(True)
     segment_completion_thread.start()
     return SplitFactory(
+        api_key,
         storages,
         cfg['labelsEnabled'],
         apis,
@@ -330,7 +341,7 @@ def segment_ready_task():
     )
 
 
-def _build_redis_factory(config):
+def _build_redis_factory(api_key, config):
     """Build and return a split factory with redis-based storage."""
     cfg = DEFAULT_CONFIG.copy()
     cfg.update(config)
@@ -344,13 +355,14 @@ def _build_redis_factory(config):
         'telemetry': RedisTelemetryStorage(redis_adapter, sdk_metadata)
     }
     return SplitFactory(
+        api_key,
         storages,
         cfg['labelsEnabled'],
         impression_listener=_wrap_impression_listener(cfg['impressionListener'], sdk_metadata)
     )
 
 
-def _build_uwsgi_factory(config):
+def _build_uwsgi_factory(api_key, config):
     """Build and return a split factory with redis-based storage."""
     cfg = DEFAULT_CONFIG.copy()
     cfg.update(config)
@@ -364,6 +376,7 @@ def _build_uwsgi_factory(config):
         'telemetry': UWSGITelemetryStorage(uwsgi_adapter)
     }
     return SplitFactory(
+        api_key,
         storages,
         cfg['labelsEnabled'],
         impression_listener=_wrap_impression_listener(cfg['impressionListener'], sdk_metadata)
@@ -390,25 +403,47 @@ def _build_localhost_factory(config):
         ready_event
     )}
     tasks['splits'].start()
-    return SplitFactory(storages, False, None, tasks, ready_event)
+    return SplitFactory('localhost', storages, False, None, tasks, ready_event)
 
 
 def get_factory(api_key, **kwargs):
     """Build and return the appropriate factory."""
-    config = kwargs.get('config', {})
+    try:
+        _INSTANTIATED_FACTORIES_LOCK.acquire()
+        if _INSTANTIATED_FACTORIES:
+            if api_key in _INSTANTIATED_FACTORIES:
+                _LOGGER.warning(
+                    "factory instantiation: You already have %d %s with this API Key. "
+                    "We recommend keeping only one instance of the factory at all times "
+                    "(Singleton pattern) and reusing it throughout your application.",
+                    _INSTANTIATED_FACTORIES[api_key],
+                    'factory' if _INSTANTIATED_FACTORIES[api_key] == 1 else 'factories'
+                )
+            else:
+                _LOGGER.warning(
+                    "factory instantiation: You already have an instance of the Split factory. "
+                    "Make sure you definitely want this additional instance. "
+                    "We recommend keeping only one instance of the factory at all times "
+                    "(Singleton pattern) and reusing it throughout your application."
+                )
 
-    if api_key == 'localhost':
-        return _build_localhost_factory(config)
+        config = kwargs.get('config', {})
 
-    if 'redisHost' in config or 'redisSentinels' in config:
-        return _build_redis_factory(config)
+        if api_key == 'localhost':
+            return _build_localhost_factory(config)
 
-    if 'uwsgiClient' in config:
-        return _build_uwsgi_factory(config)
+        if 'redisHost' in config or 'redisSentinels' in config:
+            return _build_redis_factory(api_key, config)
 
-    return _build_in_memory_factory(
-        api_key,
-        config,
-        kwargs.get('sdk_api_base_url'),
-        kwargs.get('events_api_base_url')
-    )
+        if 'uwsgiClient' in config:
+            return _build_uwsgi_factory(api_key, config)
+
+        return _build_in_memory_factory(
+            api_key,
+            config,
+            kwargs.get('sdk_api_base_url'),
+            kwargs.get('events_api_base_url')
+        )
+    finally:
+        _INSTANTIATED_FACTORIES.update([api_key])
+        _INSTANTIATED_FACTORIES_LOCK.release()

tests/client/test_factory.py

@@ -1,10 +1,11 @@
 """Split factory test module."""
-#pylint: disable=no-self-use,protected-access
+#pylint: disable=no-self-use,protected-access,line-too-long,too-many-statements
+#pylint: disable=too-many-locals, too-many-arguments
 
 import time
 import threading
 from splitio.client.listener import ImpressionListenerWrapper
-from splitio.client.factory import get_factory
+from splitio.client.factory import get_factory, SplitFactory, _INSTANTIATED_FACTORIES
 from splitio.client.config import DEFAULT_CONFIG
 from splitio.storage import redis, inmemmory, uwsgi
 from splitio.tasks import events_sync, impressions_sync, split_sync, segment_sync, telemetry_sync
@@ -338,3 +339,67 @@ def _telemetry_task_init_mock(self, api, storage, refresh_rate):
 
         assert event.is_set()
         assert factory.destroyed
+
+    def test_multiple_factories(self, mocker):
+        """Test multiple factories instantiation and tracking."""
+        def _make_factory_with_apikey(apikey, *_, **__):
+            return SplitFactory(apikey, {}, True)
+
+        factory_module_logger = mocker.Mock()
+        build_in_memory = mocker.Mock()
+        build_in_memory.side_effect = _make_factory_with_apikey
+        build_redis = mocker.Mock()
+        build_redis.side_effect = _make_factory_with_apikey
+        build_uwsgi = mocker.Mock()
+        build_uwsgi.side_effect = _make_factory_with_apikey
+        build_localhost = mocker.Mock()
+        build_localhost.side_effect = _make_factory_with_apikey
+        mocker.patch('splitio.client.factory._LOGGER', new=factory_module_logger)
+        mocker.patch('splitio.client.factory._build_in_memory_factory', new=build_in_memory)
+        mocker.patch('splitio.client.factory._build_redis_factory', new=build_redis)
+        mocker.patch('splitio.client.factory._build_uwsgi_factory', new=build_uwsgi)
+        mocker.patch('splitio.client.factory._build_localhost_factory', new=build_localhost)
+
+        _INSTANTIATED_FACTORIES.clear()  # Clear all factory counters for testing purposes
+
+        factory1 = get_factory('some_api_key')
+        assert _INSTANTIATED_FACTORIES['some_api_key'] == 1
+        assert factory_module_logger.warning.mock_calls == []
+
+        get_factory('some_api_key')
+        assert _INSTANTIATED_FACTORIES['some_api_key'] == 2
+        assert factory_module_logger.warning.mock_calls == [mocker.call(
+            "factory instantiation: You already have %d %s with this API Key. "
+            "We recommend keeping only one instance of the factory at all times "
+            "(Singleton pattern) and reusing it throughout your application.",
+            1,
+            'factory'
+        )]
+
+        factory_module_logger.reset_mock()
+        get_factory('some_api_key')
+        assert _INSTANTIATED_FACTORIES['some_api_key'] == 3
+        assert factory_module_logger.warning.mock_calls == [mocker.call(
+            "factory instantiation: You already have %d %s with this API Key. "
+            "We recommend keeping only one instance of the factory at all times "
+            "(Singleton pattern) and reusing it throughout your application.",
+            2,
+            'factories'
+        )]
+
+        factory_module_logger.reset_mock()
+        get_factory('some_other_api_key')
+        assert _INSTANTIATED_FACTORIES['some_api_key'] == 3
+        assert _INSTANTIATED_FACTORIES['some_other_api_key'] == 1
+        assert factory_module_logger.warning.mock_calls == [mocker.call(
+            "factory instantiation: You already have an instance of the Split factory. "
+            "Make sure you definitely want this additional instance. "
+            "We recommend keeping only one instance of the factory at all times "
+            "(Singleton pattern) and reusing it throughout your application."
+        )]
+
+        event = threading.Event()
+        factory1.destroy(event)
+        event.wait()
+        assert _INSTANTIATED_FACTORIES['some_other_api_key'] == 1
+        assert _INSTANTIATED_FACTORIES['some_api_key'] == 2

tests/integration/test_client_e2e.py

@@ -39,7 +39,7 @@ def setup_method(self):
             data = json.loads(flo.read())
         segment_storage.put(segments.from_raw(data))
 
-        self.factory = SplitFactory({  #pylint:disable=attribute-defined-outside-init
+        self.factory = SplitFactory('some_api_key', {  #pylint:disable=attribute-defined-outside-init
             'splits': split_storage,
             'segments': segment_storage,
             'impressions': InMemoryImpressionStorage(5000),
@@ -286,7 +286,7 @@ def setup_method(self):
         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({  #pylint:disable=attribute-defined-outside-init
+        self.factory = SplitFactory('some_api_key', {  #pylint:disable=attribute-defined-outside-init
             'splits': split_storage,
             'segments': segment_storage,
             'impressions': RedisImpressionsStorage(redis_client, metadata),
@@ -535,7 +535,7 @@ def teardown_method(self):
             redis_client.delete(key)
 
 
-class LocalhostIntegrationTests(object):
+class LocalhostIntegrationTests(object):  #pylint: disable=too-few-public-methods
     """Client & Manager integration tests."""
 
     def test_localhost_e2e(self):