Merge pull request #2026 from alphagov/redis-cache-template

Cache `GET /template…` methods in Redis
This commit is contained in:
Chris Hill-Scott
2018-04-23 14:02:59 +01:00
committed by GitHub
8 changed files with 276 additions and 70 deletions

View File

@@ -29,12 +29,12 @@ def _create_service(service_name, organisation_type, email_from, form):
free_sms_fragment_limit = current_app.config['DEFAULT_FREE_SMS_FRAGMENT_LIMITS'].get(organisation_type)
try:
service_id = service_api_client.create_service(
service_name,
organisation_type,
current_app.config['DEFAULT_SERVICE_LIMIT'],
True,
session['user_id'],
email_from,
service_name=service_name,
organisation_type=organisation_type,
message_limit=current_app.config['DEFAULT_SERVICE_LIMIT'],
restricted=True,
user_id=session['user_id'],
email_from=email_from,
)
session['service_id'] = service_id

View File

@@ -1,27 +1,43 @@
import json
from contextlib import suppress
from datetime import timedelta
from functools import wraps
from inspect import signature
TTL = int(timedelta(hours=24).total_seconds())
def _make_key(prefix, args, key_from_args):
def _get_argument(argument_name, client_method, args, kwargs):
if key_from_args is None:
key_from_args = [0]
with suppress(KeyError):
return kwargs[argument_name]
return '-'.join(
[prefix] + [args[index] for index in key_from_args]
)
with suppress(ValueError, IndexError):
argument_index = list(signature(client_method).parameters).index(argument_name)
return args[argument_index - 1] # -1 because `args` doesnt include `self`
with suppress(KeyError):
return signature(client_method).parameters[argument_name].default
raise TypeError("{}() takes no argument called '{}'".format(
client_method.__name__, argument_name
))
def set(prefix, key_from_args=None):
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 set(key_format):
def _set(client_method):
@wraps(client_method)
def new_client_method(client_instance, *args, **kwargs):
redis_key = _make_key(prefix, args, key_from_args)
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'))
@@ -37,13 +53,13 @@ def set(prefix, key_from_args=None):
return _set
def delete(prefix, key_from_args=None):
def delete(key_format):
def _delete(client_method):
@wraps(client_method)
def new_client_method(client_instance, *args, **kwargs):
redis_key = _make_key(prefix, args, key_from_args)
redis_key = _make_key(key_format, client_method, args, kwargs)
client_instance.redis_client.delete(redis_key)
return client_method(client_instance, *args, **kwargs)

View File

@@ -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')
@cache.delete('user', key_from_args=[1])
@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),

View File

@@ -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')
@cache.delete('service-{service_id}')
def update_service_organisation(self, service_id, org_id):
data = {
'service_id': service_id

View File

@@ -9,7 +9,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
def __init__(self):
super().__init__("a" * 73, "b")
@cache.delete('user', key_from_args=[4])
@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')
@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')
@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')
@cache.delete('service-{service_id}')
def archive_service(self, service_id):
return self.post('/service/{}/archive'.format(service_id), data=None)
@cache.delete('service')
@cache.delete('service-{service_id}')
def suspend_service(self, service_id):
return self.post('/service/{}/suspend'.format(service_id), data=None)
@cache.delete('service')
@cache.delete('service-{service_id}')
def resume_service(self, service_id):
return self.post('/service/{}/resume'.format(service_id), data=None)
@cache.delete('service')
@cache.delete('user', key_from_args=[1])
@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,6 +139,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
data = _attach_current_user({})
return self.delete(endpoint, data)
@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.
@@ -158,6 +159,9 @@ class ServiceAPIClient(NotifyAdminAPIClient):
endpoint = "/service/{0}/template".format(service_id)
return self.post(endpoint, data)
@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.
@@ -181,6 +185,9 @@ class ServiceAPIClient(NotifyAdminAPIClient):
endpoint = "/service/{0}/template/{1}".format(service_id, id_)
return self.post(endpoint, data)
@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_),
@@ -189,6 +196,9 @@ class ServiceAPIClient(NotifyAdminAPIClient):
),
)
@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,
@@ -199,7 +209,8 @@ class ServiceAPIClient(NotifyAdminAPIClient):
data
)
def get_service_template(self, service_id, template_id, version=None, *params):
@cache.set('template-{template_id}-version-{version}')
def get_service_template(self, service_id, template_id, version=None):
"""
Retrieve a service template.
"""
@@ -208,9 +219,10 @@ class ServiceAPIClient(NotifyAdminAPIClient):
template_id=template_id)
if version:
endpoint = '{base}/version/{version}'.format(base=endpoint, version=version)
return self.get(endpoint, *params)
return self.get(endpoint)
def get_service_template_versions(self, service_id, template_id, *params):
@cache.set('template-{template_id}-versions')
def get_service_template_versions(self, service_id, template_id):
"""
Retrieve a list of versions for a template
"""
@@ -218,16 +230,18 @@ class ServiceAPIClient(NotifyAdminAPIClient):
service_id=service_id,
template_id=template_id
)
return self.get(endpoint, *params)
return self.get(endpoint)
def get_service_templates(self, service_id, *params):
@cache.set('service-{service_id}-templates')
def get_service_templates(self, service_id):
"""
Retrieve all templates for service.
"""
endpoint = '/service/{service_id}/template'.format(
service_id=service_id)
return self.get(endpoint, *params)
return self.get(endpoint)
# This doesnt need caching because it calls through to a method which is cached
def count_service_templates(self, service_id, template_type=None):
return len([
template for template in
@@ -238,6 +252,9 @@ class ServiceAPIClient(NotifyAdminAPIClient):
)
])
@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
@@ -267,7 +284,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
def get_whitelist(self, service_id):
return self.get(url='/service/{}/whitelist'.format(service_id))
@cache.delete('service')
@cache.delete('service-{service_id}')
def update_whitelist(self, service_id, data):
return self.put(url='/service/{}/whitelist'.format(service_id), data=data)
@@ -305,7 +322,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
'/service/{}/inbound-sms/summary'.format(service_id)
)
@cache.delete('service')
@cache.delete('service-{service_id}')
def create_service_inbound_api(self, service_id, url, bearer_token, user_id):
data = {
"url": url,
@@ -314,7 +331,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
}
return self.post("/service/{}/inbound-api".format(service_id), data)
@cache.delete('service')
@cache.delete('service-{service_id}')
def update_service_inbound_api(self, service_id, url, bearer_token, user_id, inbound_api_id):
data = {
"url": url,
@@ -346,7 +363,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
)
)
@cache.delete('service')
@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),
@@ -356,7 +373,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
}
)
@cache.delete('service')
@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(
@@ -375,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')
@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),
@@ -385,7 +402,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
}
)
@cache.delete('service')
@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(
@@ -411,7 +428,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
"/service/{}/sms-sender/{}".format(service_id, sms_sender_id)
)
@cache.delete('service')
@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,
@@ -421,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')
@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),
@@ -438,7 +455,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
)
)['data']
@cache.delete('service')
@cache.delete('service-{service_id}')
def update_service_callback_api(self, service_id, url, bearer_token, user_id, callback_api_id):
data = {
"url": url,
@@ -448,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')
@cache.delete('service-{service_id}')
def create_service_callback_api(self, service_id, url, bearer_token, user_id):
data = {
"url": url,

View File

@@ -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')
@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')
@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')
@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')
@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')
@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')
@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')
@cache.delete('user', key_from_args=[1])
@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', key_from_args=[1])
@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')
@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')
@cache.delete('user-{user_id}')
def _activate_user(self, user_id):
return self.post("/user/{}/activate".format(user_id), data=None)

View File

@@ -43,12 +43,12 @@ def test_should_add_service_and_redirect_to_tour_when_no_services(
)
assert mock_get_services_with_no_services.called
mock_create_service.assert_called_once_with(
'testing the post',
'local',
app_.config['DEFAULT_SERVICE_LIMIT'],
True,
api_user_active.id,
'testing.the.post'
service_name='testing the post',
organisation_type='local',
message_limit=app_.config['DEFAULT_SERVICE_LIMIT'],
restricted=True,
user_id=api_user_active.id,
email_from='testing.the.post'
)
mock_create_service_template.assert_called_once_with(
'Example text message template',
@@ -95,12 +95,12 @@ def test_should_add_service_and_redirect_to_dashboard_when_existing_service(
)
assert mock_get_services.called
mock_create_service.assert_called_once_with(
'testing the post',
organisation_type,
app_.config['DEFAULT_SERVICE_LIMIT'],
True,
api_user_active.id,
'testing.the.post'
service_name='testing the post',
organisation_type=organisation_type,
message_limit=app_.config['DEFAULT_SERVICE_LIMIT'],
restricted=True,
user_id=api_user_active.id,
email_from='testing.the.post'
)
mock_create_or_update_free_sms_fragment_limit.assert_called_once_with(101, free_allowance)
assert len(mock_create_service_template.call_args_list) == 0

View File

@@ -6,10 +6,12 @@ from tests.conftest import SERVICE_ONE_ID, fake_uuid
from app import invite_api_client, service_api_client, user_api_client
from app.notify_client.service_api_client import ServiceAPIClient
FAKE_TEMPLATE_ID = fake_uuid()
def test_client_posts_archived_true_when_deleting_template(mocker):
service_id = fake_uuid
template_id = fake_uuid
service_id = fake_uuid()
template_id = fake_uuid()
mocker.patch('app.notify_client.current_user', id='1')
expected_data = {
@@ -139,6 +141,8 @@ def test_client_returns_count_of_service_templates(
@pytest.mark.parametrize(
(
'client_method,'
'extra_args,'
'expected_cache_get_calls,'
'cache_value,'
'expected_api_calls,'
@@ -147,6 +151,8 @@ def test_client_returns_count_of_service_templates(
),
[
(
service_api_client.get_service,
[SERVICE_ONE_ID],
[
call('service-{}'.format(SERVICE_ONE_ID))
],
@@ -156,6 +162,8 @@ def test_client_returns_count_of_service_templates(
{'data_from': 'cache'},
),
(
service_api_client.get_service,
[SERVICE_ONE_ID],
[
call('service-{}'.format(SERVICE_ONE_ID))
],
@@ -172,10 +180,132 @@ def test_client_returns_count_of_service_templates(
],
{'data_from': 'api'},
),
(
service_api_client.get_service_template,
[SERVICE_ONE_ID, FAKE_TEMPLATE_ID],
[
call('template-{}-version-None'.format(FAKE_TEMPLATE_ID))
],
b'{"data_from": "cache"}',
[],
[],
{'data_from': 'cache'},
),
(
service_api_client.get_service_template,
[SERVICE_ONE_ID, FAKE_TEMPLATE_ID],
[
call('template-{}-version-None'.format(FAKE_TEMPLATE_ID))
],
None,
[
call('/service/{}/template/{}'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID))
],
[
call(
'template-{}-version-None'.format(FAKE_TEMPLATE_ID),
'{"data_from": "api"}',
ex=86400
)
],
{'data_from': 'api'},
),
(
service_api_client.get_service_template,
[SERVICE_ONE_ID, FAKE_TEMPLATE_ID, 1],
[
call('template-{}-version-1'.format(FAKE_TEMPLATE_ID))
],
b'{"data_from": "cache"}',
[],
[],
{'data_from': 'cache'},
),
(
service_api_client.get_service_template,
[SERVICE_ONE_ID, FAKE_TEMPLATE_ID, 1],
[
call('template-{}-version-1'.format(FAKE_TEMPLATE_ID))
],
None,
[
call('/service/{}/template/{}/version/1'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID))
],
[
call(
'template-{}-version-1'.format(FAKE_TEMPLATE_ID),
'{"data_from": "api"}',
ex=86400
)
],
{'data_from': 'api'},
),
(
service_api_client.get_service_templates,
[SERVICE_ONE_ID],
[
call('service-{}-templates'.format(SERVICE_ONE_ID))
],
b'{"data_from": "cache"}',
[],
[],
{'data_from': 'cache'},
),
(
service_api_client.get_service_templates,
[SERVICE_ONE_ID],
[
call('service-{}-templates'.format(SERVICE_ONE_ID))
],
None,
[
call('/service/{}/template'.format(SERVICE_ONE_ID))
],
[
call(
'service-{}-templates'.format(SERVICE_ONE_ID),
'{"data_from": "api"}',
ex=86400
)
],
{'data_from': 'api'},
),
(
service_api_client.get_service_template_versions,
[SERVICE_ONE_ID, FAKE_TEMPLATE_ID],
[
call('template-{}-versions'.format(FAKE_TEMPLATE_ID))
],
b'{"data_from": "cache"}',
[],
[],
{'data_from': 'cache'},
),
(
service_api_client.get_service_template_versions,
[SERVICE_ONE_ID, FAKE_TEMPLATE_ID],
[
call('template-{}-versions'.format(FAKE_TEMPLATE_ID))
],
None,
[
call('/service/{}/template/{}/versions'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID))
],
[
call(
'template-{}-versions'.format(FAKE_TEMPLATE_ID),
'{"data_from": "api"}',
ex=86400
)
],
{'data_from': 'api'},
),
]
)
def test_returns_value_from_cache(
mocker,
client_method,
extra_args,
expected_cache_get_calls,
cache_value,
expected_return_value,
@@ -195,7 +325,7 @@ def test_returns_value_from_cache(
'app.notify_client.RedisClient.set',
)
assert service_api_client.get_service(SERVICE_ONE_ID) == expected_return_value
assert client_method(*extra_args) == expected_return_value
assert mock_redis_get.call_args_list == expected_cache_get_calls
assert mock_api_get.call_args_list == expected_api_calls
@@ -240,3 +370,46 @@ def test_deletes_service_cache(
assert call('service-{}'.format(SERVICE_ONE_ID)) in mock_redis_delete.call_args_list
assert len(mock_request.call_args_list) == 1
@pytest.mark.parametrize('method, extra_args, expected_cache_deletes', [
('create_service_template', ['name', 'type_', 'content', SERVICE_ONE_ID], [
'service-{}-templates'.format(SERVICE_ONE_ID),
]),
('update_service_template', [FAKE_TEMPLATE_ID, 'foo', 'sms', 'bar', SERVICE_ONE_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], [
'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'], [
'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], [
'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(
app_,
mock_get_user,
mocker,
method,
extra_args,
expected_cache_deletes,
):
mocker.patch('app.notify_client.current_user', id='1')
mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete')
mock_request = mocker.patch('notifications_python_client.base.BaseAPIClient.request')
getattr(service_api_client, method)(*extra_args)
assert mock_redis_delete.call_args_list == list(map(call, expected_cache_deletes))
assert len(mock_request.call_args_list) == 1