From 6a9818b5fdd3dad6dda764ef795a3f19442533e6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 18 Jun 2020 15:04:59 +0100 Subject: [PATCH] Cache services and API keys in memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same as we’ve done for templates. For high volume services this should mean avoiding calls to external services, either the database or Redis. TTL is set to 2 seconds, so that’s the maximum time it will take for revoking an API key or renaming a service to propagate. Some of the tests created services with the same service ID. This caused intermittent failures because the cache relies on unique service IDs (like we have in the real world) to key itself. --- app/config.py | 7 +++- app/serialised_models.py | 2 ++ requirements-app.txt | 1 + requirements.txt | 6 ++-- .../app/authentication/test_authentication.py | 36 ++++++++++++++++++- .../notifications/test_post_notifications.py | 21 ++++++++--- 6 files changed, 63 insertions(+), 10 deletions(-) diff --git a/app/config.py b/app/config.py index 4e13a3732..0a539864f 100644 --- a/app/config.py +++ b/app/config.py @@ -400,7 +400,12 @@ class Test(Development): NOTIFY_ENVIRONMENT = 'test' TESTING = True - HIGH_VOLUME_SERVICE = ['941b6f9a-50d7-4742-8d50-f365ca74bf27'] + HIGH_VOLUME_SERVICE = [ + '941b6f9a-50d7-4742-8d50-f365ca74bf27', + '63f95b86-2d19-4497-b8b2-ccf25457df4e', + '7e5950cb-9954-41f5-8376-962b8c8555cf', + '10d1b9c9-0072-4fa9-ae1c-595e333841da', + ] CSV_UPLOAD_BUCKET_NAME = 'test-notifications-csv-upload' CONTACT_LIST_BUCKET_NAME = 'test-contact-list' diff --git a/app/serialised_models.py b/app/serialised_models.py index 319017dcb..db8d0a3fe 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -126,6 +126,7 @@ class SerialisedService(SerialisedModel): } @classmethod + @memory_cache def from_id(cls, service_id): return cls(cls.get_dict(service_id)) @@ -156,6 +157,7 @@ class SerialisedAPIKeyCollection(SerialisedModelCollection): model = SerialisedAPIKey @classmethod + @memory_cache def from_service_id(cls, service_id): keys = [ {k: getattr(key, k) for k in SerialisedAPIKey.ALLOWED_PROPERTIES} diff --git a/requirements-app.txt b/requirements-app.txt index 57b04538e..444164308 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -20,6 +20,7 @@ marshmallow==2.21.0 # pyup: <3 # v3 throws errors psycopg2-binary==2.8.5 PyJWT==1.7.1 SQLAlchemy==1.3.17 +cachetools==4.1.0 notifications-python-client==5.5.1 diff --git a/requirements.txt b/requirements.txt index c12e5dc4b..c70959d8c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,6 +22,7 @@ marshmallow==2.21.0 # pyup: <3 # v3 throws errors psycopg2-binary==2.8.5 PyJWT==1.7.1 SQLAlchemy==1.3.17 +cachetools==4.1.0 notifications-python-client==5.5.1 @@ -39,15 +40,14 @@ alembic==1.4.2 amqp==1.4.9 anyjson==0.3.3 attrs==19.3.0 -awscli==1.18.84 +awscli==1.18.85 bcrypt==3.1.7 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.17.7 -cachetools==4.1.0 +botocore==1.17.8 certifi==2020.6.20 chardet==3.0.4 click==7.1.2 diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 4cae2acd1..24b212f4c 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -8,9 +8,18 @@ import pytest from flask import json, current_app, request from freezegun import freeze_time from notifications_python_client.authentication import create_jwt_token +from unittest.mock import call from app import api_user -from app.dao.api_key_dao import get_unsigned_secrets, save_model_api_key, get_unsigned_secret, expire_api_key +from app.dao.api_key_dao import ( + get_unsigned_secrets, + save_model_api_key, + get_unsigned_secret, + expire_api_key, + get_model_api_keys, +) +from app.dao.services_dao import dao_fetch_service_by_id + from app.models import ApiKey, KEY_TYPE_NORMAL from app.authentication.auth import AuthError, requires_admin_auth, requires_auth, GENERAL_TOKEN_ERROR_MESSAGE @@ -457,3 +466,28 @@ def test_proxy_key_on_admin_auth_endpoint(notify_api, check_proxy_header, header ] ) assert response.status_code == expected_status + + +def test_should_cache_service_and_api_key_lookups(mocker, client, sample_api_key): + + mock_get_api_keys = mocker.patch( + 'app.serialised_models.get_model_api_keys', + wraps=get_model_api_keys, + ) + mock_get_service = mocker.patch( + 'app.serialised_models.dao_fetch_service_by_id', + wraps=dao_fetch_service_by_id, + ) + + for i in range(5): + token = __create_token(sample_api_key.service_id) + client.get('/notifications', headers={ + 'Authorization': f'Bearer {token}' + }) + + assert mock_get_api_keys.call_args_list == [ + call(str(sample_api_key.service_id)) + ] + assert mock_get_service.call_args_list == [ + call(str(sample_api_key.service_id)) + ] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index fcff4191d..76a342801 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -1070,7 +1070,10 @@ def test_post_notifications_saves_email_to_queue(client, notify_db_session, mock save_email_task = mocker.patch("app.celery.tasks.save_api_email.apply_async") mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - service = create_service(service_id='941b6f9a-50d7-4742-8d50-f365ca74bf27', service_name='high volume service') + service = create_service( + service_id=current_app.config['HIGH_VOLUME_SERVICE'][0], + service_name='high volume service', + ) template = create_template(service=service, content='((message))', template_type=EMAIL_TYPE) data = { "email_address": "joe.citizen@example.com", @@ -1101,7 +1104,10 @@ def test_post_notifications_saves_email_normally_if_save_email_to_queue_fails(cl ) mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - service = create_service(service_id='941b6f9a-50d7-4742-8d50-f365ca74bf27', service_name='high volume service') + service = create_service( + service_id=current_app.config['HIGH_VOLUME_SERVICE'][1], + service_name='high volume service', + ) template = create_template(service=service, content='((message))', template_type=EMAIL_TYPE) data = { "email_address": "joe.citizen@example.com", @@ -1130,8 +1136,10 @@ def test_post_notifications_doesnt_save_email_to_queue_for_test_emails(client, n save_email_task = mocker.patch("app.celery.tasks.save_api_email.apply_async") mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - service = create_service(service_id='941b6f9a-50d7-4742-8d50-f365ca74bf27', service_name='high volume service') - # create_api_key(service=service, key_type='test') + service = create_service( + service_id=current_app.config['HIGH_VOLUME_SERVICE'][2], + service_name='high volume service', + ) template = create_template(service=service, content='((message))', template_type=EMAIL_TYPE) data = { "email_address": "joe.citizen@example.com", @@ -1160,7 +1168,10 @@ def test_post_notifications_doesnt_save_email_to_queue_for_sms(client, notify_db save_email_task = mocker.patch("app.celery.tasks.save_api_email.apply_async") mock_send_task = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - service = create_service(service_id='941b6f9a-50d7-4742-8d50-f365ca74bf27', service_name='high volume service') + service = create_service( + service_id=current_app.config['HIGH_VOLUME_SERVICE'][3], + service_name='high volume service', + ) template = create_template(service=service, content='((message))', template_type=SMS_TYPE) data = { "phone_number": '+447700900855',