From 87afc439fe7451a1178b68b1bbee3efab66766aa Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Jun 2020 09:39:46 +0100 Subject: [PATCH 1/5] Cache serialised template in memory --- app/serialised_models.py | 25 +++++++++++++++++ .../notifications/test_post_notifications.py | 28 +++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/app/serialised_models.py b/app/serialised_models.py index f864171f7..1896c1810 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -1,7 +1,31 @@ from abc import ABC, abstractmethod +from collections import defaultdict +from functools import partial +from threading import RLock + +import cachetools from app.dao import templates_dao +caches = defaultdict(partial(cachetools.TTLCache, maxsize=1024, ttl=2)) +locks = defaultdict(RLock) + + +def memory_cache(func): + @cachetools.cached( + cache=caches[func.__qualname__], + lock=locks[func.__qualname__], + key=ignore_first_argument_cache_key, + ) + def wrapper(*args, **kwargs): + return func(*args, **kwargs) + + return wrapper + + +def ignore_first_argument_cache_key(cls, *args, **kwargs): + return cachetools.keys.hashkey(*args, **kwargs) + class SerialisedModel(ABC): @@ -40,6 +64,7 @@ class SerialisedTemplate(SerialisedModel): } @classmethod + @memory_cache def from_id_and_service_id(cls, template_id, service_id): return cls(cls.get_dict(template_id, service_id)) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 7fe8acc0a..4348ae24d 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -6,6 +6,7 @@ import pytest from freezegun import freeze_time from boto.exception import SQSError +from app.dao import templates_dao from app.dao.service_sms_sender_dao import dao_update_service_sms_sender from app.models import ( ScheduledNotification, @@ -211,6 +212,33 @@ def test_notification_reply_to_text_is_original_value_if_sender_is_changed_after assert notifications[0].reply_to_text == '123456' +def test_should_cache_template_lookups_in_memory(mocker, client, sample_template): + + mock_get_template = mocker.patch( + 'app.dao.templates_dao.dao_get_template_by_id_and_service_id', + wraps=templates_dao.dao_get_template_by_id_and_service_id, + ) + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + + data = { + 'phone_number': '+447700900855', + 'template_id': str(sample_template.id), + } + + for i in range(5): + auth_header = create_authorization_header(service_id=sample_template.service_id) + client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert mock_get_template.call_args_list == [ + call(service_id=sample_template.service_id, template_id=str(sample_template.id)) + ] + assert Notification.query.count() == 5 + + @pytest.mark.parametrize("notification_type, key_send_to, send_to", [("sms", "phone_number", "+447700900855"), ("email", "email_address", "sample@email.com")]) From 2f4470edca40a16d408573209a6802088b2777c8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 18 Jun 2020 15:23:43 +0100 Subject: [PATCH 2/5] Serialise UUID to string not UUID object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We need to serialise the template to JSON to store it in Redis. Python’s built in JSON serialiser doesn’t know what to do with a UUID object, so we need to manually cast it to a string instead. --- app/schemas.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/app/schemas.py b/app/schemas.py index 1dafdba14..dded0b09e 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -13,6 +13,7 @@ from marshmallow import ( post_dump ) from marshmallow_sqlalchemy import field_for +from uuid import UUID from notifications_utils.recipients import ( validate_email_address, @@ -58,6 +59,14 @@ def _validate_datetime_not_in_past(dte, msg="Date cannot be in the past"): raise ValidationError(msg) +class UUIDsAsStringsMixin: + @post_dump() + def __post_dump(self, data): + for key, value in data.items(): + if isinstance(value, UUID): + data[key] = str(value) + + class BaseSchema(ma.ModelSchema): def __init__(self, load_json=False, *args, **kwargs): @@ -341,7 +350,7 @@ class BaseTemplateSchema(BaseSchema): strict = True -class TemplateSchema(BaseTemplateSchema): +class TemplateSchema(BaseTemplateSchema, UUIDsAsStringsMixin): created_by_id = field_for( models.Template, 'created_by_id', dump_to='created_by', dump_only=True From 996800f90da7d4cc407e35f251babc9518c85025 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Jun 2020 09:39:47 +0100 Subject: [PATCH 3/5] Cache serialised template in Redis --- app/serialised_models.py | 4 ++ .../notifications/test_post_notifications.py | 40 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/app/serialised_models.py b/app/serialised_models.py index 1896c1810..019487b0c 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -4,11 +4,14 @@ from functools import partial from threading import RLock import cachetools +from notifications_utils.clients.redis import RequestCache +from app import redis_store from app.dao import templates_dao caches = defaultdict(partial(cachetools.TTLCache, maxsize=1024, ttl=2)) locks = defaultdict(RLock) +redis_cache = RequestCache(redis_store) def memory_cache(func): @@ -69,6 +72,7 @@ class SerialisedTemplate(SerialisedModel): return cls(cls.get_dict(template_id, service_id)) @staticmethod + @redis_cache.set('template-{template_id}-version-None') def get_dict(template_id, service_id): from app.schemas import template_schema diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 4348ae24d..3632e45a0 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -239,6 +239,46 @@ def test_should_cache_template_lookups_in_memory(mocker, client, sample_template assert Notification.query.count() == 5 +def test_should_cache_template_lookups_in_redis(mocker, client, sample_template): + + from app.schemas import template_schema + + mock_redis_get = mocker.patch( + 'app.redis_store.get', + return_value=None, + ) + mock_redis_set = mocker.patch( + 'app.redis_store.set', + ) + + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + + data = { + 'phone_number': '+447700900855', + 'template_id': str(sample_template.id), + } + + auth_header = create_authorization_header(service_id=sample_template.service_id) + client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + expected_key = f'template-{sample_template.id}-version-None' + + assert mock_redis_get.call_args_list == [call( + expected_key, + )] + + template_dict = template_schema.dump(sample_template).data + + assert len(mock_redis_set.call_args_list) == 1 + assert mock_redis_set.call_args[0][0] == expected_key + assert json.loads(mock_redis_set.call_args[0][1]) == template_dict + assert mock_redis_set.call_args[1]['ex'] == 604_800 + + @pytest.mark.parametrize("notification_type, key_send_to, send_to", [("sms", "phone_number", "+447700900855"), ("email", "email_address", "sample@email.com")]) From cb4b809131196d9b7b282cfce6265cecc2937809 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 23 Jun 2020 13:48:55 +0100 Subject: [PATCH 4/5] Add extra assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To be crystal clear 💎 --- tests/app/v2/notifications/test_post_notifications.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 3632e45a0..994517d81 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -233,6 +233,7 @@ def test_should_cache_template_lookups_in_memory(mocker, client, sample_template headers=[('Content-Type', 'application/json'), auth_header] ) + assert mock_get_template.call_count == 1 assert mock_get_template.call_args_list == [ call(service_id=sample_template.service_id, template_id=str(sample_template.id)) ] From af1b021dbe4d5cbbc77228368efde1c35b0edbe8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 23 Jun 2020 14:00:07 +0100 Subject: [PATCH 5/5] Add test for when template is found in Redis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ensures that we’re not calling the dao method when it is --- .../notifications/test_post_notifications.py | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 994517d81..ee1f0c3ac 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -280,6 +280,37 @@ def test_should_cache_template_lookups_in_redis(mocker, client, sample_template) assert mock_redis_set.call_args[1]['ex'] == 604_800 +def test_should_return_template_if_found_in_redis(mocker, client, sample_template): + + from app.schemas import template_schema + template_dict = template_schema.dump(sample_template).data + + mocker.patch( + 'app.redis_store.get', + return_value=json.dumps(template_dict).encode('utf-8') + ) + mock_get_template = mocker.patch( + 'app.dao.templates_dao.dao_get_template_by_id_and_service_id' + ) + + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + + data = { + 'phone_number': '+447700900855', + 'template_id': str(sample_template.id), + } + + auth_header = create_authorization_header(service_id=sample_template.service_id) + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response.status_code == 201 + assert mock_get_template.called is False + + @pytest.mark.parametrize("notification_type, key_send_to, send_to", [("sms", "phone_number", "+447700900855"), ("email", "email_address", "sample@email.com")])