Merge pull request #2014 from alphagov/redis-spike-CHS

Use Redis to cache API calls in admin app

app/config.py

@@ -74,6 +74,9 @@ class Config(object):
     ROUTE_SECRET_KEY_2 = os.environ.get('ROUTE_SECRET_KEY_2', '')
     CHECK_PROXY_HEADER = False
 
+    REDIS_URL = os.environ.get('REDIS_URL')
+    REDIS_ENABLED = os.environ.get('REDIS_ENABLED') == '1'
+
 
 class Development(Config):
     NOTIFY_LOG_PATH = 'application.log'

app/notify_client/__init__.py

@@ -2,6 +2,7 @@ from flask_login import current_user
 from flask import has_request_context, request, abort
 from notifications_python_client.base import BaseAPIClient
 from notifications_python_client import __version__
+from notifications_utils.clients.redis.redis_client import RedisClient
 
 
 def _attach_current_user(data):
@@ -12,11 +13,16 @@ def _attach_current_user(data):
 
 
 class NotifyAdminAPIClient(BaseAPIClient):
+
+    redis_client = RedisClient()
+
     def init_app(self, app):
         self.base_url = app.config['API_HOST_NAME']
         self.service_id = app.config['ADMIN_CLIENT_USER_NAME']
         self.api_key = app.config['ADMIN_CLIENT_SECRET']
         self.route_secret = app.config['ROUTE_SECRET_KEY_1']
+        self.redis_client.init_app(app)
+        self.redis_client.redis_store.decode_responses = True
 
     def generate_headers(self, api_token):
         headers = {

app/notify_client/cache.py

@@ -0,0 +1,52 @@
+import json
+from datetime import timedelta
+from functools import wraps
+
+TTL = int(timedelta(hours=24).total_seconds())
+
+
+def _make_key(prefix, args, key_from_args):
+
+    if key_from_args is None:
+        key_from_args = [0]
+
+    return '-'.join(
+        [prefix] + [args[index] for index in key_from_args]
+    )
+
+
+def set(prefix, key_from_args=None):
+
+    def _set(client_method):
+
+        @wraps(client_method)
+        def new_client_method(client_instance, *args, **kwargs):
+            redis_key = _make_key(prefix, args, key_from_args)
+            cached = client_instance.redis_client.get(redis_key)
+            if cached:
+                return json.loads(cached.decode('utf-8'))
+            api_response = client_method(client_instance, *args, **kwargs)
+            client_instance.redis_client.set(
+                redis_key,
+                json.dumps(api_response),
+                ex=TTL,
+            )
+            return api_response
+
+        return new_client_method
+    return _set
+
+
+def delete(prefix, key_from_args=None):
+
+    def _delete(client_method):
+
+        @wraps(client_method)
+        def new_client_method(client_instance, *args, **kwargs):
+            redis_key = _make_key(prefix, args, key_from_args)
+            client_instance.redis_client.delete(redis_key)
+            return client_method(client_instance, *args, **kwargs)
+
+        return new_client_method
+
+    return _delete

app/notify_client/organisations_api_client.py

@@ -1,4 +1,4 @@
-from app.notify_client import NotifyAdminAPIClient, _attach_current_user
+from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache
 
 
 class OrganisationsClient(NotifyAdminAPIClient):
@@ -27,6 +27,7 @@ class OrganisationsClient(NotifyAdminAPIClient):
     def get_service_organisation(self, service_id):
         return self.get(url="/service/{}/organisation".format(service_id))
 
+    @cache.delete('service')
     def update_service_organisation(self, service_id, org_id):
         data = {
             'service_id': service_id

app/notify_client/service_api_client.py

@@ -1,6 +1,6 @@
 from __future__ import unicode_literals
 
-from app.notify_client import NotifyAdminAPIClient, _attach_current_user
+from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache
 
 
 class ServiceAPIClient(NotifyAdminAPIClient):
@@ -33,6 +33,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
         data = _attach_current_user(data)
         return self.post("/service", data)['data']['id']
 
+    @cache.set('service')
     def get_service(self, service_id):
         return self._get_service(service_id, detailed=False, today_only=False)
 
@@ -72,6 +73,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
         params_dict['only_active'] = True
         return self.get_services(params_dict)
 
+    @cache.delete('service')
     def update_service(
         self,
         service_id,
@@ -108,18 +110,23 @@ class ServiceAPIClient(NotifyAdminAPIClient):
         endpoint = "/service/{0}".format(service_id)
         return self.post(endpoint, data)
 
+    # This method is not cached because it calls through to one which is
     def update_service_with_properties(self, service_id, properties):
         return self.update_service(service_id, **properties)
 
+    @cache.delete('service')
     def archive_service(self, service_id):
         return self.post('/service/{}/archive'.format(service_id), data=None)
 
+    @cache.delete('service')
     def suspend_service(self, service_id):
         return self.post('/service/{}/suspend'.format(service_id), data=None)
 
+    @cache.delete('service')
     def resume_service(self, service_id):
         return self.post('/service/{}/resume'.format(service_id), data=None)
 
+    @cache.delete('service')
     def remove_user_from_service(self, service_id, user_id):
         """
         Remove a user from a service
@@ -258,6 +265,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
     def get_whitelist(self, service_id):
         return self.get(url='/service/{}/whitelist'.format(service_id))
 
+    @cache.delete('service')
     def update_whitelist(self, service_id, data):
         return self.put(url='/service/{}/whitelist'.format(service_id), data=data)
 
@@ -295,6 +303,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
             '/service/{}/inbound-sms/summary'.format(service_id)
         )
 
+    @cache.delete('service')
     def create_service_inbound_api(self, service_id, url, bearer_token, user_id):
         data = {
             "url": url,
@@ -303,6 +312,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
         }
         return self.post("/service/{}/inbound-api".format(service_id), data)
 
+    @cache.delete('service')
     def update_service_inbound_api(self, service_id, url, bearer_token, user_id, inbound_api_id):
         data = {
             "url": url,
@@ -334,6 +344,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
             )
         )
 
+    @cache.delete('service')
     def add_reply_to_email_address(self, service_id, email_address, is_default=False):
         return self.post(
             "/service/{}/email-reply-to".format(service_id),
@@ -343,6 +354,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
             }
         )
 
+    @cache.delete('service')
     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(
@@ -361,6 +373,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')
     def add_letter_contact(self, service_id, contact_block, is_default=False):
         return self.post(
             "/service/{}/letter-contact".format(service_id),
@@ -370,6 +383,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
             }
         )
 
+    @cache.delete('service')
     def update_letter_contact(self, service_id, letter_contact_id, contact_block, is_default=False):
         return self.post(
             "/service/{}/letter-contact/{}".format(
@@ -395,6 +409,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
             "/service/{}/sms-sender/{}".format(service_id, sms_sender_id)
         )
 
+    @cache.delete('service')
     def add_sms_sender(self, service_id, sms_sender, is_default=False, inbound_number_id=None):
         data = {
             "sms_sender": sms_sender,
@@ -404,6 +419,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')
     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),
@@ -420,6 +436,7 @@ class ServiceAPIClient(NotifyAdminAPIClient):
             )
         )['data']
 
+    @cache.delete('service')
     def update_service_callback_api(self, service_id, url, bearer_token, user_id, callback_api_id):
         data = {
             "url": url,
@@ -429,6 +446,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')
     def create_service_callback_api(self, service_id, url, bearer_token, user_id):
         data = {
             "url": url,

app/notify_client/user_api_client.py

@@ -1,6 +1,6 @@
 from notifications_python_client.errors import HTTPError
 
-from app.notify_client import NotifyAdminAPIClient
+from app.notify_client import NotifyAdminAPIClient, cache
 from app.notify_client.models import (
     User,
     roles,
@@ -147,6 +147,7 @@ class UserApiClient(NotifyAdminAPIClient):
         resp = self.get(endpoint)
         return [User(data) for data in resp['data']]
 
+    @cache.delete('service')
     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)

manifest-base.yml

@@ -36,5 +36,8 @@ env:
   TEMPLATE_PREVIEW_API_HOST: null
   TEMPLATE_PREVIEW_API_KEY: null
 
+  REDIS_ENABLED: null
+  REDIS_URL: null
+
 applications:
   - name: notify-admin

pytest.ini

@@ -9,3 +9,4 @@ env =
     DESKPRO_API_HOST=test
     DESKPRO_API_KEY=test
     STATSD_PREFIX=stats-prefix
+    REDIS_ENABLED=0

requirements.txt

@@ -18,4 +18,4 @@ notifications-python-client==4.8.1
 # PaaS
 awscli-cwlogs>=1.4,<1.5
 
-git+https://github.com/alphagov/notifications-utils.git@25.2.3#egg=notifications-utils==25.2.3
+git+https://github.com/alphagov/notifications-utils.git@26.2.0#egg=notifications-utils==26.2.0

tests/app/main/views/test_service_settings.py

@@ -2123,7 +2123,7 @@ def test_updates_sms_prefixing(
         )
     )
     mock_update_service.assert_called_once_with(
-        service_id=SERVICE_ONE_ID,
+        SERVICE_ONE_ID,
         prefix_sms=expected_api_argument,
     )
 

tests/app/notify_client/test_service_api_client.py

@@ -1,7 +1,9 @@
+from unittest.mock import call
+
 import pytest
 from tests.conftest import SERVICE_ONE_ID, fake_uuid
 
-from app import service_api_client
+from app import service_api_client, user_api_client
 from app.notify_client.service_api_client import ServiceAPIClient
 
 
@@ -33,7 +35,7 @@ def test_client_posts_archived_true_when_deleting_template(mocker):
 )
 def test_client_gets_service(mocker, function, params):
     client = ServiceAPIClient()
-    mock_get = mocker.patch.object(client, 'get')
+    mock_get = mocker.patch.object(client, 'get', return_value={})
 
     function(client, 'foo')
     mock_get.assert_called_once_with('/service/foo', params=params)
@@ -52,7 +54,7 @@ def test_client_creates_service_with_correct_data(
     fake_uuid,
 ):
     client = ServiceAPIClient()
-    mock_post = mocker.patch.object(client, 'post')
+    mock_post = mocker.patch.object(client, 'post', return_value={'data': {'id': None}})
     mocker.patch('app.notify_client.current_user', id='123')
 
     client.create_service(
@@ -133,3 +135,109 @@ def test_client_returns_count_of_service_templates(
     assert service_api_client.count_service_templates(
         SERVICE_ONE_ID, **extra_args
     ) == expected_count
+
+
+@pytest.mark.parametrize(
+    (
+        'expected_cache_get_calls,'
+        'cache_value,'
+        'expected_api_calls,'
+        'expected_cache_set_calls,'
+        'expected_return_value,'
+    ),
+    [
+        (
+            [
+                call('service-{}'.format(SERVICE_ONE_ID))
+            ],
+            b'{"data_from": "cache"}',
+            [],
+            [],
+            {'data_from': 'cache'},
+        ),
+        (
+            [
+                call('service-{}'.format(SERVICE_ONE_ID))
+            ],
+            None,
+            [
+                call('/service/{}'.format(SERVICE_ONE_ID), params={})
+            ],
+            [
+                call(
+                    'service-{}'.format(SERVICE_ONE_ID),
+                    '{"data_from": "api"}',
+                    ex=86400
+                )
+            ],
+            {'data_from': 'api'},
+        ),
+    ]
+)
+def test_returns_value_from_cache(
+    mocker,
+    expected_cache_get_calls,
+    cache_value,
+    expected_return_value,
+    expected_api_calls,
+    expected_cache_set_calls,
+):
+
+    mock_redis_get = mocker.patch(
+        'app.notify_client.RedisClient.get',
+        return_value=cache_value,
+    )
+    mock_api_get = mocker.patch(
+        'app.notify_client.NotifyAdminAPIClient.get',
+        return_value={'data_from': 'api'},
+    )
+    mock_redis_set = mocker.patch(
+        'app.notify_client.RedisClient.set',
+    )
+
+    assert service_api_client.get_service(SERVICE_ONE_ID) == expected_return_value
+
+    assert mock_redis_get.call_args_list == expected_cache_get_calls
+    assert mock_api_get.call_args_list == expected_api_calls
+    assert mock_redis_set.call_args_list == expected_cache_set_calls
+
+
+@pytest.mark.parametrize('client, method, extra_args, extra_kwargs', [
+    (service_api_client, 'update_service', [SERVICE_ONE_ID], {'name': 'foo'}),
+    (service_api_client, 'update_service_with_properties', [SERVICE_ONE_ID], {'properties': {}}),
+    (service_api_client, 'archive_service', [SERVICE_ONE_ID], {}),
+    (service_api_client, 'suspend_service', [SERVICE_ONE_ID], {}),
+    (service_api_client, 'resume_service', [SERVICE_ONE_ID], {}),
+    (service_api_client, 'remove_user_from_service', [SERVICE_ONE_ID, ''], {}),
+    (service_api_client, 'update_whitelist', [SERVICE_ONE_ID, {}], {}),
+    (service_api_client, 'create_service_inbound_api', [SERVICE_ONE_ID] + [''] * 3, {}),
+    (service_api_client, 'update_service_inbound_api', [SERVICE_ONE_ID] + [''] * 4, {}),
+    (service_api_client, 'add_reply_to_email_address', [SERVICE_ONE_ID, ''], {}),
+    (service_api_client, 'update_reply_to_email_address', [SERVICE_ONE_ID] + [''] * 2, {}),
+    (service_api_client, 'add_letter_contact', [SERVICE_ONE_ID, ''], {}),
+    (service_api_client, 'update_letter_contact', [SERVICE_ONE_ID] + [''] * 2, {}),
+    (service_api_client, 'add_sms_sender', [SERVICE_ONE_ID, ''], {}),
+    (service_api_client, 'update_sms_sender', [SERVICE_ONE_ID] + [''] * 2, {}),
+    (service_api_client, 'update_service_callback_api', [SERVICE_ONE_ID] + [''] * 4, {}),
+    (service_api_client, 'create_service_callback_api', [SERVICE_ONE_ID] + [''] * 3, {}),
+    (user_api_client, 'add_user_to_service', [SERVICE_ONE_ID, fake_uuid(), []], {}),
+])
+def test_deletes_service_cache(
+    app_,
+    mock_get_user,
+    mocker,
+    client,
+    method,
+    extra_args,
+    extra_kwargs,
+):
+    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(client, method)(*extra_args, **extra_kwargs)
+
+    assert mock_redis_delete.call_args_list == [
+        call('service-{}'.format(SERVICE_ONE_ID))
+    ]
+    assert len(mock_request.call_args_list) == 1