Add caching of templates in Redis

A lot of the frequently-used pages in the admin app rely on the API to
get templates.

So this commit adds three new caches:
- a single template version (including a key without a version number,
  which is the current version)
- all the templates for a service
- all versions of a template

The first will be the most crucial for performance, but there’s not much
cost to adding the other two.

app/notify_client/cache.py

@@ -12,9 +12,12 @@ def _get_argument(argument_name, args, kwargs, client_method):
     with suppress(KeyError):
         return kwargs[argument_name]
 
-    with suppress(ValueError):
+    with suppress(ValueError, IndexError):
         return args[list(signature(client_method).parameters).index(argument_name) - 1]
 
+    with suppress(KeyError):
+        return signature(client_method).parameters[argument_name].default
+
     raise TypeError("{}() takes no argument called '{}'".format(
         client_method.__name__, argument_name
     ))

app/notify_client/service_api_client.py

@@ -139,6 +139,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
         data = _attach_current_user({})
         return self.delete(endpoint, data)
 
+    @cache.delete('templates', 'service_id')
     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('templates', 'service_id')
+    @cache.delete('template', 'id_')
+    @cache.delete('template_versions', 'id_')
     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('templates', 'service_id')
+    @cache.delete('template', 'id_')
+    @cache.delete('template_versions', 'id_')
     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('templates', 'service_id')
+    @cache.delete('template', 'template_id')
+    @cache.delete('template_versions', 'template_id')
     def update_service_template_sender(self, service_id, template_id, reply_to):
         data = {
             'reply_to': reply_to,
@@ -199,6 +209,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
             data
         )
 
+    @cache.set('template', 'template_id', 'version')
     def get_service_template(self, service_id, template_id, version=None):
         """
         Retrieve a service template.
@@ -210,6 +221,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
             endpoint = '{base}/version/{version}'.format(base=endpoint, version=version)
         return self.get(endpoint)
 
+    @cache.set('template_versions', 'template_id')
     def get_service_template_versions(self, service_id, template_id):
         """
         Retrieve a list of versions for a template
@@ -220,6 +232,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
         )
         return self.get(endpoint)
 
+    @cache.set('templates', 'service_id')
     def get_service_templates(self, service_id):
         """
         Retrieve all templates for service.
@@ -228,6 +241,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
             service_id=service_id)
         return self.get(endpoint)
 
+    # This doesn’t 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('templates', 'service_id')
+    @cache.delete('template', 'template_id')
+    @cache.delete('template_versions', 'template_id')
     def delete_service_template(self, service_id, template_id):
         """
         Set a service template's archived flag to True

tests/app/notify_client/test_service_api_client.py

@@ -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
+    service_id = fake_uuid()
-    template_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,72 @@ 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-{}'.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-{}'.format(FAKE_TEMPLATE_ID))
+            ],
+            None,
+            [
+                call('/service/{}/template/{}'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID))
+            ],
+            [
+                call(
+                    'template-{}'.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-{}-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-{}-1'.format(FAKE_TEMPLATE_ID))
+            ],
+            None,
+            [
+                call('/service/{}/template/{}/version/1'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID))
+            ],
+            [
+                call(
+                    'template-{}-1'.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 +265,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 +310,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], [
+        '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),
+    ]),
+    ('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),
+    ]),
+    ('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),
+    ]),
+    ('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),
+    ]),
+])
+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