diff --git a/app/notify_client/cache.py b/app/notify_client/cache.py index 7d622e11e..8bdd6e38c 100644 --- a/app/notify_client/cache.py +++ b/app/notify_client/cache.py @@ -7,13 +7,14 @@ from inspect import signature TTL = int(timedelta(hours=24).total_seconds()) -def _get_argument(argument_name, args, kwargs, client_method): +def _get_argument(argument_name, client_method, args, kwargs): with suppress(KeyError): return kwargs[argument_name] with suppress(ValueError, IndexError): - return args[list(signature(client_method).parameters).index(argument_name) - 1] + argument_index = list(signature(client_method).parameters).index(argument_name) + return args[argument_index - 1] # -1 because `args` doesn’t include `self` with suppress(KeyError): return signature(client_method).parameters[argument_name].default @@ -23,32 +24,20 @@ def _get_argument(argument_name, args, kwargs, client_method): )) -def list_of_strings(list_of_stuff): - return list(map(str, filter(None, list_of_stuff))) +def _make_key(key_format, client_method, args, kwargs): + return key_format.format(**{ + argument_name: _get_argument(argument_name, client_method, args, kwargs) + for argument_name in list(signature(client_method).parameters) + }) -def _make_key(prefix, key_from_args, local_variables): - return '-'.join( - [ - local_variables['prefix'] - ] + list_of_strings( - _get_argument( - argument_name, - local_variables['args'], - local_variables['kwargs'], - local_variables['client_method'] - ) for argument_name in key_from_args - ) - ) - - -def set(prefix, *key_from_args): +def set(key_format): def _set(client_method): @wraps(client_method) def new_client_method(client_instance, *args, **kwargs): - redis_key = _make_key(prefix, key_from_args, locals()) + redis_key = _make_key(key_format, client_method, args, kwargs) cached = client_instance.redis_client.get(redis_key) if cached: return json.loads(cached.decode('utf-8')) @@ -64,13 +53,13 @@ def set(prefix, *key_from_args): return _set -def delete(prefix, *key_from_args): +def delete(key_format): def _delete(client_method): @wraps(client_method) def new_client_method(client_instance, *args, **kwargs): - redis_key = _make_key(prefix, key_from_args, locals()) + redis_key = _make_key(key_format, client_method, args, kwargs) client_instance.redis_client.delete(redis_key) return client_method(client_instance, *args, **kwargs) diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index 45e6840f1..ba7434134 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -44,8 +44,8 @@ class InviteApiClient(NotifyAdminAPIClient): self.post(url='/service/{0}/invite/{1}'.format(service_id, invited_user_id), data=data) - @cache.delete('service', 'service_id') - @cache.delete('user', 'invited_user_id') + @cache.delete('service-{service_id}') + @cache.delete('user-{invited_user_id}') def accept_invite(self, service_id, invited_user_id): data = {'status': 'accepted'} self.post(url='/service/{0}/invite/{1}'.format(service_id, invited_user_id), diff --git a/app/notify_client/organisations_api_client.py b/app/notify_client/organisations_api_client.py index 6bab54e5a..ce7d39a4b 100644 --- a/app/notify_client/organisations_api_client.py +++ b/app/notify_client/organisations_api_client.py @@ -27,7 +27,7 @@ class OrganisationsClient(NotifyAdminAPIClient): def get_service_organisation(self, service_id): return self.get(url="/service/{}/organisation".format(service_id)) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_service_organisation(self, service_id, org_id): data = { 'service_id': service_id diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index a11adde39..11c70a481 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -9,7 +9,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): def __init__(self): super().__init__("a" * 73, "b") - @cache.delete('user', 'user_id') + @cache.delete('user-{user_id}') def create_service( self, service_name, @@ -34,7 +34,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data = _attach_current_user(data) return self.post("/service", data)['data']['id'] - @cache.set('service', 'service_id') + @cache.set('service-{service_id}') def get_service(self, service_id): return self._get_service(service_id, detailed=False, today_only=False) @@ -74,7 +74,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): params_dict['only_active'] = True return self.get_services(params_dict) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_service( self, service_id, @@ -115,20 +115,20 @@ class ServiceAPIClient(NotifyAdminAPIClient): def update_service_with_properties(self, service_id, properties): return self.update_service(service_id, **properties) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def archive_service(self, service_id): return self.post('/service/{}/archive'.format(service_id), data=None) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def suspend_service(self, service_id): return self.post('/service/{}/suspend'.format(service_id), data=None) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def resume_service(self, service_id): return self.post('/service/{}/resume'.format(service_id), data=None) - @cache.delete('service', 'service_id') - @cache.delete('user', 'user_id') + @cache.delete('service-{service_id}') + @cache.delete('user-{user_id}') def remove_user_from_service(self, service_id, user_id): """ Remove a user from a service @@ -139,7 +139,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data = _attach_current_user({}) return self.delete(endpoint, data) - @cache.delete('templates', 'service_id') + @cache.delete('service-{service_id}-templates') def create_service_template(self, name, type_, content, service_id, subject=None, process_type='normal'): """ Create a service template. @@ -159,9 +159,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): endpoint = "/service/{0}/template".format(service_id) return self.post(endpoint, data) - @cache.delete('templates', 'service_id') - @cache.delete('template', 'id_') - @cache.delete('template_versions', 'id_') + @cache.delete('service-{service_id}-templates') + @cache.delete('template-{id_}-version-None') + @cache.delete('template-{id_}-versions') def update_service_template(self, id_, name, type_, content, service_id, subject=None, process_type=None): """ Update a service template. @@ -185,9 +185,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): endpoint = "/service/{0}/template/{1}".format(service_id, id_) return self.post(endpoint, data) - @cache.delete('templates', 'service_id') - @cache.delete('template', 'id_') - @cache.delete('template_versions', 'id_') + @cache.delete('service-{service_id}-templates') + @cache.delete('template-{id_}-version-None') + @cache.delete('template-{id_}-versions') def redact_service_template(self, service_id, id_): return self.post( "/service/{}/template/{}".format(service_id, id_), @@ -196,9 +196,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): ), ) - @cache.delete('templates', 'service_id') - @cache.delete('template', 'template_id') - @cache.delete('template_versions', 'template_id') + @cache.delete('service-{service_id}-templates') + @cache.delete('template-{template_id}-version-None') + @cache.delete('template-{template_id}-versions') def update_service_template_sender(self, service_id, template_id, reply_to): data = { 'reply_to': reply_to, @@ -209,7 +209,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data ) - @cache.set('template', 'template_id', 'version') + @cache.set('template-{template_id}-version-{version}') def get_service_template(self, service_id, template_id, version=None): """ Retrieve a service template. @@ -221,7 +221,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): endpoint = '{base}/version/{version}'.format(base=endpoint, version=version) return self.get(endpoint) - @cache.set('template_versions', 'template_id') + @cache.set('template-{template_id}-versions') def get_service_template_versions(self, service_id, template_id): """ Retrieve a list of versions for a template @@ -232,7 +232,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) return self.get(endpoint) - @cache.set('templates', 'service_id') + @cache.set('service-{service_id}-templates') def get_service_templates(self, service_id): """ Retrieve all templates for service. @@ -252,9 +252,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) ]) - @cache.delete('templates', 'service_id') - @cache.delete('template', 'template_id') - @cache.delete('template_versions', 'template_id') + @cache.delete('service-{service_id}-templates') + @cache.delete('template-{template_id}-version-None') + @cache.delete('template-{template_id}-versions') def delete_service_template(self, service_id, template_id): """ Set a service template's archived flag to True @@ -284,7 +284,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): def get_whitelist(self, service_id): return self.get(url='/service/{}/whitelist'.format(service_id)) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_whitelist(self, service_id, data): return self.put(url='/service/{}/whitelist'.format(service_id), data=data) @@ -322,7 +322,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): '/service/{}/inbound-sms/summary'.format(service_id) ) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def create_service_inbound_api(self, service_id, url, bearer_token, user_id): data = { "url": url, @@ -331,7 +331,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): } return self.post("/service/{}/inbound-api".format(service_id), data) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_service_inbound_api(self, service_id, url, bearer_token, user_id, inbound_api_id): data = { "url": url, @@ -363,7 +363,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) ) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def add_reply_to_email_address(self, service_id, email_address, is_default=False): return self.post( "/service/{}/email-reply-to".format(service_id), @@ -373,7 +373,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): } ) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_reply_to_email_address(self, service_id, reply_to_email_id, email_address, is_default=False): return self.post( "/service/{}/email-reply-to/{}".format( @@ -392,7 +392,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): def get_letter_contact(self, service_id, letter_contact_id): return self.get("/service/{}/letter-contact/{}".format(service_id, letter_contact_id)) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def add_letter_contact(self, service_id, contact_block, is_default=False): return self.post( "/service/{}/letter-contact".format(service_id), @@ -402,7 +402,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): } ) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_letter_contact(self, service_id, letter_contact_id, contact_block, is_default=False): return self.post( "/service/{}/letter-contact/{}".format( @@ -428,7 +428,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): "/service/{}/sms-sender/{}".format(service_id, sms_sender_id) ) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def add_sms_sender(self, service_id, sms_sender, is_default=False, inbound_number_id=None): data = { "sms_sender": sms_sender, @@ -438,7 +438,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data["inbound_number_id"] = inbound_number_id return self.post("/service/{}/sms-sender".format(service_id), data=data) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_sms_sender(self, service_id, sms_sender_id, sms_sender, is_default=False): return self.post( "/service/{}/sms-sender/{}".format(service_id, sms_sender_id), @@ -455,7 +455,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) )['data'] - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_service_callback_api(self, service_id, url, bearer_token, user_id, callback_api_id): data = { "url": url, @@ -465,7 +465,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data['bearer_token'] = bearer_token return self.post("/service/{}/delivery-receipt-api/{}".format(service_id, callback_api_id), data) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def create_service_callback_api(self, service_id, url, bearer_token, user_id): data = { "url": url, diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index e5fbcfae6..c0e3a28ef 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -39,7 +39,7 @@ class UserApiClient(NotifyAdminAPIClient): def get_user(self, user_id): return User(self._get_user(user_id)['data'], max_failed_login_count=self.max_failed_login_count) - @cache.set('user', 'user_id') + @cache.set('user-{user_id}') def _get_user(self, user_id): return self.get("/user/{}".format(user_id)) @@ -61,7 +61,7 @@ class UserApiClient(NotifyAdminAPIClient): users.append(User(user, max_failed_login_count=self.max_failed_login_count)) return users - @cache.delete('user', 'user_id') + @cache.delete('user-{user_id}') def update_user_attribute(self, user_id, **kwargs): data = dict(kwargs) disallowed_attributes = set(data.keys()) - ALLOWED_ATTRIBUTES @@ -75,20 +75,20 @@ class UserApiClient(NotifyAdminAPIClient): user_data = self.post(url, data=data) return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) - @cache.delete('user', 'user_id') + @cache.delete('user-{user_id}') def reset_failed_login_count(self, user_id): url = "/user/{}/reset-failed-login-count".format(user_id) user_data = self.post(url, data={}) return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) - @cache.delete('user', 'user_id') + @cache.delete('user-{user_id}') def update_password(self, user_id, password): data = {"_password": password} url = "/user/{}/update-password".format(user_id) user_data = self.post(url, data=data) return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) - @cache.delete('user', 'user_id') + @cache.delete('user-{user_id}') def verify_password(self, user_id, password): try: url = "/user/{}/verify/password".format(user_id) @@ -118,7 +118,7 @@ class UserApiClient(NotifyAdminAPIClient): endpoint = '/user/{0}/email-already-registered'.format(user_id) self.post(endpoint, data=data) - @cache.delete('user', 'user_id') + @cache.delete('user-{user_id}') def check_verify_code(self, user_id, code, code_type): data = {'code_type': code_type, 'code': code} endpoint = '/user/{}/verify/code'.format(user_id) @@ -154,20 +154,20 @@ class UserApiClient(NotifyAdminAPIClient): resp = self.get(endpoint) return [User(data) for data in resp['data']] - @cache.delete('service', 'service_id') - @cache.delete('user', 'user_id') + @cache.delete('service-{service_id}') + @cache.delete('user-{user_id}') def add_user_to_service(self, service_id, user_id, permissions): # permissions passed in are the combined admin roles, not db permissions endpoint = '/service/{}/users/{}'.format(service_id, user_id) data = [{'permission': x} for x in translate_permissions_from_admin_roles_to_db(permissions)] self.post(endpoint, data=data) - @cache.delete('user', 'user_id') + @cache.delete('user-{user_id}') def add_user_to_organisation(self, org_id, user_id): resp = self.post('/organisations/{}/users/{}'.format(org_id, user_id), data={}) return User(resp['data'], max_failed_login_count=self.max_failed_login_count) - @cache.delete('user', 'user_id') + @cache.delete('user-{user_id}') def set_user_permissions(self, user_id, service_id, permissions): # permissions passed in are the combined admin roles, not db permissions data = [{'permission': x} for x in translate_permissions_from_admin_roles_to_db(permissions)] @@ -191,7 +191,7 @@ class UserApiClient(NotifyAdminAPIClient): else: return user - @cache.delete('user', 'user_id') + @cache.delete('user-{user_id}') def _activate_user(self, user_id): return self.post("/user/{}/activate".format(user_id), data=None) diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index 7b55817c1..60748c818 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -184,7 +184,7 @@ def test_client_returns_count_of_service_templates( service_api_client.get_service_template, [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], [ - call('template-{}'.format(FAKE_TEMPLATE_ID)) + call('template-{}-version-None'.format(FAKE_TEMPLATE_ID)) ], b'{"data_from": "cache"}', [], @@ -195,7 +195,7 @@ def test_client_returns_count_of_service_templates( service_api_client.get_service_template, [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], [ - call('template-{}'.format(FAKE_TEMPLATE_ID)) + call('template-{}-version-None'.format(FAKE_TEMPLATE_ID)) ], None, [ @@ -203,7 +203,7 @@ def test_client_returns_count_of_service_templates( ], [ call( - 'template-{}'.format(FAKE_TEMPLATE_ID), + 'template-{}-version-None'.format(FAKE_TEMPLATE_ID), '{"data_from": "api"}', ex=86400 ) @@ -214,7 +214,7 @@ def test_client_returns_count_of_service_templates( service_api_client.get_service_template, [SERVICE_ONE_ID, FAKE_TEMPLATE_ID, 1], [ - call('template-{}-1'.format(FAKE_TEMPLATE_ID)) + call('template-{}-version-1'.format(FAKE_TEMPLATE_ID)) ], b'{"data_from": "cache"}', [], @@ -225,7 +225,7 @@ def test_client_returns_count_of_service_templates( service_api_client.get_service_template, [SERVICE_ONE_ID, FAKE_TEMPLATE_ID, 1], [ - call('template-{}-1'.format(FAKE_TEMPLATE_ID)) + call('template-{}-version-1'.format(FAKE_TEMPLATE_ID)) ], None, [ @@ -233,7 +233,7 @@ def test_client_returns_count_of_service_templates( ], [ call( - 'template-{}-1'.format(FAKE_TEMPLATE_ID), + 'template-{}-version-1'.format(FAKE_TEMPLATE_ID), '{"data_from": "api"}', ex=86400 ) @@ -314,27 +314,27 @@ def test_deletes_service_cache( @pytest.mark.parametrize('method, extra_args, expected_cache_deletes', [ ('create_service_template', ['name', 'type_', 'content', SERVICE_ONE_ID], [ - 'templates-{}'.format(SERVICE_ONE_ID), + 'service-{}-templates'.format(SERVICE_ONE_ID), ]), ('update_service_template', [FAKE_TEMPLATE_ID, 'foo', 'sms', 'bar', SERVICE_ONE_ID], [ - 'templates-{}'.format(SERVICE_ONE_ID), - 'template-{}'.format(FAKE_TEMPLATE_ID), - 'template_versions-{}'.format(FAKE_TEMPLATE_ID), + 'service-{}-templates'.format(SERVICE_ONE_ID), + 'template-{}-version-None'.format(FAKE_TEMPLATE_ID), + 'template-{}-versions'.format(FAKE_TEMPLATE_ID), ]), ('redact_service_template', [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], [ - 'templates-{}'.format(SERVICE_ONE_ID), - 'template-{}'.format(FAKE_TEMPLATE_ID), - 'template_versions-{}'.format(FAKE_TEMPLATE_ID), + 'service-{}-templates'.format(SERVICE_ONE_ID), + 'template-{}-version-None'.format(FAKE_TEMPLATE_ID), + 'template-{}-versions'.format(FAKE_TEMPLATE_ID), ]), ('update_service_template_sender', [SERVICE_ONE_ID, FAKE_TEMPLATE_ID, 'foo'], [ - 'templates-{}'.format(SERVICE_ONE_ID), - 'template-{}'.format(FAKE_TEMPLATE_ID), - 'template_versions-{}'.format(FAKE_TEMPLATE_ID), + 'service-{}-templates'.format(SERVICE_ONE_ID), + 'template-{}-version-None'.format(FAKE_TEMPLATE_ID), + 'template-{}-versions'.format(FAKE_TEMPLATE_ID), ]), ('delete_service_template', [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], [ - 'templates-{}'.format(SERVICE_ONE_ID), - 'template-{}'.format(FAKE_TEMPLATE_ID), - 'template_versions-{}'.format(FAKE_TEMPLATE_ID), + 'service-{}-templates'.format(SERVICE_ONE_ID), + 'template-{}-version-None'.format(FAKE_TEMPLATE_ID), + 'template-{}-versions'.format(FAKE_TEMPLATE_ID), ]), ]) def test_deletes_caches_when_modifying_templates(