Minor changes for clarity

* ServiceManager check method renamed as assert_within_limits
* Legacy classes moved to separate files

server/extension/sql/20_geocode_street.sql

@@ -77,7 +77,7 @@ RETURNS Geometry AS $$
 
   plpy.execute("SELECT cdb_dataservices_server._get_logger_config()")
   service_manager = LegacyServiceManager('geocoder', username, orgname, GD)
-  service_manager.check()
+  service_manager.assert_within_limits()
 
   try:
     geocoder = HereMapsGeocoder(service_manager.config.heremaps_app_id, service_manager.config.heremaps_app_code, service_manager.logger, service_manager.config.heremaps_service_params)
@@ -106,7 +106,7 @@ RETURNS Geometry AS $$
 
   plpy.execute("SELECT cdb_dataservices_server._get_logger_config()")
   service_manager = LegacyServiceManager('geocoder', username, orgname, GD)
-  service_manager.check(quota=False)
+  service_manager.assert_within_limits(quota=False)
 
   try:
     geocoder = GoogleMapsGeocoder(service_manager.config.google_client_id, service_manager.config.google_api_key, service_manager.logger)
@@ -139,7 +139,7 @@ RETURNS Geometry AS $$
   cartodb_services.init(plpy, GD)
 
   service_manager = ServiceManager('geocoder', MapzenGeocoderConfigBuilder, username, orgname)
-  service_manager.check()
+  service_manager.assert_within_limits()
 
   try:
     geocoder = MapzenGeocoder(service_manager.config.mapzen_api_key, service_manager.logger, service_manager.config.service_params)

server/lib/python/cartodb_services/cartodb_services/refactor/config/__init__.py

@@ -0,0 +1,2 @@
+from rate_limits import RateLimitsConfig, RateLimitsConfigBuilder
+from legacy_rate_limits import RateLimitsConfigLegacyBuilder

server/lib/python/cartodb_services/cartodb_services/refactor/config/legacy_rate_limits.py

@@ -0,0 +1,46 @@
+import json
+from rate_limits import RateLimitsConfig
+
+class RateLimitsConfigLegacyBuilder(object):
+    """
+    Build a RateLimitsConfig object using the *legacy* configuration classes
+    """
+
+    def __init__(self, redis_connection, db_conn, service, user, org):
+        self._service = service
+        self._username = user
+        self._orgname = org
+        self._redis_connection = redis_connection
+        self._db_conn = db_conn
+
+    def get(self):
+        rate_limit = self.__get_rate_limit()
+        return RateLimitsConfig(self._service,
+                                self._username,
+                                rate_limit.get('limit', None),
+                                rate_limit.get('period', None))
+
+    def __get_rate_limit(self):
+        rate_limit = {}
+        rate_limit_key = "{0}_rate_limit".format(self._service)
+        user_key = "rails:users:{0}".format(self._username)
+        rate_limit_json = self.__get_redis_config(user_key, rate_limit_key)
+        if not rate_limit_json and self._orgname:
+            org_key = "rails:orgs:{0}".format(self._orgname)
+            rate_limit_json = self.__get_redis_config(org_key, rate_limit_key)
+        if rate_limit_json:
+            rate_limit = json.loads(rate_limit_json)
+        else:
+            conf_key = 'rate_limits'
+            sql = "SELECT cartodb.CDB_Conf_GetConf('{0}') as conf".format(conf_key)
+            try:
+                conf = self._db_conn.execute(sql, 1)[0]['conf']
+            except Exception:
+                conf = None
+            if conf:
+                rate_limit = json.loads(conf).get(self._service)
+        return rate_limit or {}
+
+    def __get_redis_config(self, basekey, param):
+        config = self._redis_connection.hgetall(basekey)
+        return config and config.get(param)

server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py

@@ -37,8 +37,11 @@ class RateLimitsConfig(object):
     def is_limited(self):
         return self._limit and self._limit > 0 and self._period and self._period > 0
 
-
 class RateLimitsConfigBuilder(object):
+    """
+    Build a rate limits configuration obtaining the parameters
+    from the user/org/server configuration.
+    """
 
     def __init__(self, server_conf, user_conf, org_conf, service, user, org):
         self._server_conf = server_conf
@@ -62,49 +65,3 @@ class RateLimitsConfigBuilder(object):
                                 self._username,
                                 rate_limit.get('limit', None),
                                 rate_limit.get('period', None))
-
-class RateLimitsConfigLegacyBuilder(object):
-    """
-    Build a RateLimitsConfig object using the legacy configuration
-    classes ...
-    instead of the refactored ...
-    """
-
-    def __init__(self, redis_connection, db_conn, service, user, org):
-        self._service = service
-        self._username = user
-        self._orgname = org
-        self._redis_connection = redis_connection
-        self._db_conn = db_conn
-
-    def get(self):
-        rate_limit = self.__get_rate_limit()
-        return RateLimitsConfig(self._service,
-                                self._username,
-                                rate_limit.get('limit', None),
-                                rate_limit.get('period', None))
-
-    def __get_rate_limit(self):
-        rate_limit = {}
-        rate_limit_key = "{0}_rate_limit".format(self._service)
-        user_key = "rails:users:{0}".format(self._username)
-        rate_limit_json = self.__get_redis_config(user_key, rate_limit_key)
-        if not rate_limit_json and self._orgname:
-            org_key = "rails:orgs:{0}".format(self._orgname)
-            rate_limit_json = self.__get_redis_config(org_key, rate_limit_key)
-        if rate_limit_json:
-            rate_limit = json.loads(rate_limit_json)
-        else:
-            conf_key = 'rate_limits'
-            sql = "SELECT cartodb.CDB_Conf_GetConf('{0}') as conf".format(conf_key)
-            try:
-                conf = self._db_conn.execute(sql, 1)[0]['conf']
-            except Exception:
-                conf = None
-            if conf:
-                rate_limit = json.loads(conf).get(self._service)
-        return rate_limit or {}
-
-    def __get_redis_config(self, basekey, param):
-        config = self._redis_connection.hgetall(basekey)
-        return config and config.get(param)

server/lib/python/cartodb_services/cartodb_services/tools/__init__.py

@@ -3,5 +3,5 @@ from coordinates import Coordinate
 from polyline import PolyLine
 from log import Logger, LoggerConfig
 from rate_limiter import RateLimiter
-from exceptions import RateLimitExceeded
+from service_manager import ServiceManager, RateLimitExceeded
-from service_manager import ServiceManager, LegacyServiceManager
+from legacy_service_manager import  LegacyServiceManager

server/lib/python/cartodb_services/cartodb_services/tools/exceptions.py

@@ -1,3 +0,0 @@
-class RateLimitExceeded(Exception):
-    def __str__(self):
-            return repr('Rate limit exceeded')

server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py

@@ -0,0 +1,23 @@
+from cartodb_services.metrics import QuotaService
+from cartodb_services.tools import Logger,LoggerConfig
+from cartodb_services.tools import RateLimiter
+from cartodb_services.refactor.config import RateLimitsConfigLegacyBuilder
+from cartodb_services.tools.service_manager import ServiceManagerBase
+import plpy
+
+class LegacyServiceManager(ServiceManagerBase):
+    """
+    This service manager relies on cached configuration (in gd) stored in *legacy* configuration objects
+    It's intended for use by the *legacy* configuration objects (in use prior to the configuration refactor).
+    """
+
+    def __init__(self, service, username, orgname, gd):
+        redis_conn = gd["redis_connection_{0}".format(username)]['redis_metrics_connection']
+        self.config = gd["user_{0}_config_{1}".format(service, username)]
+        logger_config = gd["logger_config"]
+        self.logger = Logger(logger_config)
+
+        self.quota_service = QuotaService(self.config, redis_conn)
+
+        rate_limit_config = RateLimitsConfigLegacyBuilder(redis_conn, plpy, service=service, user=username, org=orgname).get()
+        self.rate_limiter = RateLimiter(rate_limit_config, redis_conn)

server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py

@@ -1,7 +1,41 @@
-from exceptions import RateLimitExceeded
+from cartodb_services.metrics import QuotaService
+from cartodb_services.tools import Logger
+from cartodb_services.tools import RateLimiter
+from cartodb_services.refactor.tools.logger import LoggerConfigBuilder
+from cartodb_services.refactor.core.environment import ServerEnvironmentBuilder
+from cartodb_services.refactor.backend.server_config import ServerConfigBackendFactory
+from cartodb_services.refactor.backend.user_config import UserConfigBackendFactory
+from cartodb_services.refactor.backend.org_config import OrgConfigBackendFactory
+from cartodb_services.refactor.backend.redis_metrics_connection import RedisMetricsConnectionFactory
+from cartodb_services.refactor.config import RateLimitsConfigBuilder
+
+class RateLimitExceeded(Exception):
+    def __str__(self):
+            return repr('Rate limit exceeded')
 
 class ServiceManagerBase:
-    def check(self, quota=True, rate=True):
+    """
+    A Service manager collects the configuration needed to use a service,
+    including thir-party services parameters.
+
+    This abstract class serves as the base for concrete service manager classes;
+    derived class must provide and initialize attributes for ``config``,
+    ``quota_service``, ``logger`` and ``rate_limiter`` (which can be None
+    for no limits).
+
+    It provides an `assert_within_limits` method to check quota and rate limits
+    which raises exceptions when limits are exceeded.
+
+    It exposes properties containing:
+
+    * ``config`` : a configuration object containing the configuration parameters for
+      a given service and provider.
+    * ``quota_service`` a QuotaService object to for quota accounting
+    * ``logger``
+
+    """
+
+    def assert_within_limits(self, quota=True, rate=True):
         if rate and not self.rate_limiter.check():
             raise RateLimitExceeded()
         if quota and not self.quota_service.check_user_quota():
@@ -19,18 +53,12 @@ class ServiceManagerBase:
     def logger(self):
         return self.logger
 
-from cartodb_services.metrics import QuotaService
-from cartodb_services.tools import Logger
-from cartodb_services.tools import RateLimiter
-from cartodb_services.refactor.tools.logger import LoggerConfigBuilder
-from cartodb_services.refactor.core.environment import ServerEnvironmentBuilder
-from cartodb_services.refactor.backend.server_config import ServerConfigBackendFactory
-from cartodb_services.refactor.backend.user_config import UserConfigBackendFactory
-from cartodb_services.refactor.backend.org_config import OrgConfigBackendFactory
-from cartodb_services.refactor.backend.redis_metrics_connection import RedisMetricsConnectionFactory
-from cartodb_services.refactor.config.rate_limits import RateLimitsConfigBuilder
-
 class ServiceManager(ServiceManagerBase):
+    """
+    This service manager delegates the configuration parameter details,
+    and the policies about configuration precedence to a configuration-builder class.
+    It uses the refactored configuration classes.
+    """
 
     def __init__(self, service, config_builder, username, orgname):
         server_config_backend = ServerConfigBackendFactory().get()
@@ -48,23 +76,3 @@ class ServiceManager(ServiceManagerBase):
 
         self.rate_limiter = RateLimiter(rate_limit_config, redis_metrics_connection)
         self.quota_service = QuotaService(self.config, redis_metrics_connection)
-
-from cartodb_services.metrics import QuotaService
-from cartodb_services.tools import Logger,LoggerConfig
-from cartodb_services.tools import RateLimiter
-from cartodb_services.refactor.config.rate_limits import RateLimitsConfigLegacyBuilder
-import plpy
-
-class LegacyServiceManager(ServiceManagerBase):
-
-    def __init__(self, service, username, orgname, gd):
-        redis_conn = gd["redis_connection_{0}".format(username)]['redis_metrics_connection']
-        self.config = gd["user_{0}_config_{1}".format(service, username)]
-        logger_config = gd["logger_config"]
-        self.logger = Logger(logger_config)
-
-        self.quota_service = QuotaService(self.config, redis_conn)
-
-        rate_limit_config = RateLimitsConfigLegacyBuilder(redis_conn, plpy, service=service, user=username, org=orgname).get()
-        self.rate_limiter = RateLimiter(rate_limit_config, redis_conn)
-

server/lib/python/cartodb_services/test/test_servicemanager.py

@@ -36,12 +36,12 @@ class TestServiceManager(TestCase):
     def check_rate_limit(self, service_manager, n, active=True):
         if LUA_AVAILABLE_FOR_MOCKREDIS:
             for _ in xrange(n):
-                service_manager.check()
+                service_manager.assert_within_limits()
             if active:
                 with assert_raises(RateLimitExceeded):
-                    service_manager.check()
+                    service_manager.assert_within_limits()
             else:
-                service_manager.check()
+                service_manager.assert_within_limits()
         else:
             # rratelimit doesn't work with MockRedis because it needs Lua support
             # so, we'll simply perform some sanity check on the configuration of the rate limiter
@@ -59,14 +59,14 @@ class TestServiceManager(TestCase):
           'logger_config' : Mock(min_log_level='debug', log_file_path=None, rollbar_api_key=None, environment=self.environment)
         }
         service_manager = LegacyServiceManager('geocoder', self.username, self.orgname, config_cache)
-        service_manager.check()
+        service_manager.assert_within_limits()
         assert_equal(service_manager.config.service_type, 'geocoder_mapzen')
 
     def test_service_manager(self):
         with patch.object(RedisConnectionBuilder,'get') as get_fn:
             get_fn.return_value = self.redis_conn
             service_manager = ServiceManager('geocoder', MapzenGeocoderConfigBuilder, self.username, self.orgname)
-            service_manager.check()
+            service_manager.assert_within_limits()
             assert_equal(service_manager.config.service_type, 'geocoder_mapzen')
 
     def test_no_rate_limit_by_default(self):