From 7f6c19b2929b9ce0556175a6b5f1efb7912750f6 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 16 Mar 2017 17:59:22 +0100 Subject: [PATCH] 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 | 6 +- .../refactor/config/__init__.py | 2 + .../refactor/config/legacy_rate_limits.py | 46 ++++++++++++ .../refactor/config/rate_limits.py | 51 +------------ .../cartodb_services/tools/__init__.py | 4 +- .../cartodb_services/tools/exceptions.py | 3 - .../tools/legacy_service_manager.py | 23 ++++++ .../cartodb_services/tools/service_manager.py | 74 ++++++++++--------- .../test/test_servicemanager.py | 10 +-- 9 files changed, 126 insertions(+), 93 deletions(-) create mode 100644 server/lib/python/cartodb_services/cartodb_services/refactor/config/legacy_rate_limits.py create mode 100644 server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py diff --git a/server/extension/sql/20_geocode_street.sql b/server/extension/sql/20_geocode_street.sql index 5fec700..b3daec9 100644 --- a/server/extension/sql/20_geocode_street.sql +++ b/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) diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/__init__.py b/server/lib/python/cartodb_services/cartodb_services/refactor/config/__init__.py index e69de29..b9559c6 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/config/__init__.py +++ b/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 diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/legacy_rate_limits.py b/server/lib/python/cartodb_services/cartodb_services/refactor/config/legacy_rate_limits.py new file mode 100644 index 0000000..d4208ec --- /dev/null +++ b/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) diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py b/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py index 6a465f5..6f6d0df 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py +++ b/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) diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/__init__.py b/server/lib/python/cartodb_services/cartodb_services/tools/__init__.py index 0658841..16072f8 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/__init__.py +++ b/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, LegacyServiceManager +from service_manager import ServiceManager, RateLimitExceeded +from legacy_service_manager import LegacyServiceManager diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/exceptions.py b/server/lib/python/cartodb_services/cartodb_services/tools/exceptions.py index f8b79c1..e69de29 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/exceptions.py +++ b/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') diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py b/server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py new file mode 100644 index 0000000..fca642f --- /dev/null +++ b/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) diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py index 5f140a5..06005d7 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py +++ b/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) - diff --git a/server/lib/python/cartodb_services/test/test_servicemanager.py b/server/lib/python/cartodb_services/test/test_servicemanager.py index b7874ab..2131815 100644 --- a/server/lib/python/cartodb_services/test/test_servicemanager.py +++ b/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):