darkhelm/notifications-api
1
0
Fork 0
mirror of https://github.com/GSA/notifications-api.git synced 2026-02-02 08:55:15 -05:00
Code Issues Packages Projects Releases Wiki Activity

Correct the daily limits cache.

Browse Source 
Last year we had an issue with the daily limit cache and the query that was populating it. As a result we have not been checking the daily limit properly. This PR should correct all that.

The daily limit cache is not being incremented in app.notifications.process_notifications.persist_notification, this method is and should always be the only method used to create a notification.
We increment the daily limit cache is redis is enabled (and it is always enabled for production) and the key type for the notification is team or normal.

We check if the daily limit is exceed in many places:
 - app.celery.tasks.process_job
 -  app.v2.notifications.post_notifications.post_notification
 - app.v2.notifications.post_notifications.post_precompiled_letter_notification
 - app.service.send_notification.send_one_off_notification
 - app.service.send_notification.send_pdf_letter_notification

If the daily limits cache is not found, set the cache to 0 with an expiry of 24 hours. The daily limit cache key is service_id-yyy-mm-dd-count, so each day a new cache is created.

The best thing about this PR is that the app.service_dao.fetch_todays_total_message_count query has been removed. This query was not performant and had been wrong for ages.
This commit is contained in:
Rebecca Law
2021-06-21 16:03:24 +01:00
parent c44ec57c17
commit 35b20ba363
8 changed files with 196 additions and 230 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
tests/app/celery/test_tasks.py
View File

@@ -160,6 +160,7 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits(
mocker.patch('app.celery.tasks.s3.get_job_and_metadata_from_s3',
return_value=(load_example_csv('multiple_sms'), {'sender_id': None}))
mocker.patch('app.celery.tasks.process_row')
mocker.patch('app.celery.tasks.get_service_daily_limit_cache_value', return_value=8)
process_job(job.id)
@@ -181,6 +182,7 @@ def test_should_not_process_sms_job_if_would_exceed_send_limits_inc_today(
mocker.patch('app.celery.tasks.s3.get_job_and_metadata_from_s3',
return_value=(load_example_csv('sms'), {'sender_id': None}))
mocker.patch('app.celery.tasks.process_row')
mocker.patch('app.celery.tasks.get_service_daily_limit_cache_value', return_value=2)
process_job(job.id)
@@ -200,6 +202,7 @@ def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(noti
mocker.patch('app.celery.tasks.s3.get_job_and_metadata_from_s3')
mocker.patch('app.celery.tasks.process_row')
mocker.patch('app.celery.tasks.get_service_daily_limit_cache_value', return_value=2)
process_job(job.id)
@@ -232,6 +235,7 @@ def test_should_process_email_job_if_exactly_on_send_limits(notify_db_session,
mocker.patch('app.celery.tasks.save_email.apply_async')
mocker.patch('app.encryption.encrypt', return_value="something_encrypted")
mocker.patch('app.celery.tasks.create_uuid', return_value="uuid")
mocker.patch('app.celery.tasks.get_service_daily_limit_cache_value', return_value=0)
process_job(job.id)

26
tests/app/dao/test_services_dao.py
View File

@@ -41,7 +41,6 @@ from app.dao.services_dao import (
dao_suspend_service,
dao_update_service,
delete_service_and_all_associated_db_objects,
fetch_todays_total_message_count,
get_live_services_with_organisation,
get_services_by_partial_name,
)
@@ -984,31 +983,6 @@ def test_fetch_stats_should_not_gather_notifications_older_than_7_days(
assert len(stats) == rows_returned
def test_dao_fetch_todays_total_message_count_returns_count_for_today(notify_db_session):
template = create_template(service=create_service())
notification = create_notification(template=template)
# don't include notifications earlier than today
create_notification(template=template, created_at=datetime.utcnow()-timedelta(days=2))
assert fetch_todays_total_message_count(notification.service.id) == 1
def test_dao_fetch_todays_total_message_count_returns_count_for_all_notification_type_and_selected_service(
notify_db_session
):
service = create_service()
different_service = create_service(service_name='different service')
create_notification(template=create_template(service=service))
create_notification(template=create_template(service=service, template_type='email'))
create_notification(template=create_template(service=service, template_type='letter'))
create_notification(template=create_template(service=different_service))
assert fetch_todays_total_message_count(service.id) == 3
def test_dao_fetch_todays_total_message_count_returns_0_when_no_messages_for_today(notify_db,
notify_db_session):
assert fetch_todays_total_message_count(uuid.uuid4()) == 0
def test_dao_fetch_todays_stats_for_all_services_includes_all_services(notify_db_session):
# two services, each with an email and sms notification
service1 = create_service(service_name='service 1', email_from='service.1')

147
tests/app/notifications/test_process_notification.py
View File

@@ -21,6 +21,7 @@ from app.notifications.process_notifications import (
from app.serialised_models import SerialisedTemplate
from app.v2.errors import BadRequestError
from tests.app.db import create_api_key, create_service, create_template
from tests.conftest import set_config
def test_create_content_for_notification_passes(sample_email_template):
@@ -112,50 +113,47 @@ def test_persist_notification_throws_exception_when_missing_template(sample_api_
assert NotificationHistory.query.count() == 0
def test_cache_is_not_incremented_on_failure_to_persist_notification(sample_api_key, mocker):
mocked_redis = mocker.patch('app.redis_store.get')
mock_service_template_cache = mocker.patch('app.redis_store.get_all_from_hash')
def test_cache_is_not_incremented_on_failure_to_persist_notification(notify_api, sample_api_key, mocker):
mocked_redis = mocker.patch('app.redis_store.incr')
with pytest.raises(SQLAlchemyError):
persist_notification(template_id=None,
template_version=None,
recipient='+447111111111',
service=sample_api_key.service,
personalisation=None,
notification_type='sms',
api_key_id=sample_api_key.id,
key_type=sample_api_key.key_type)
with set_config(notify_api, 'REDIS_ENABLED', True):
persist_notification(template_id=None,
template_version=None,
recipient='+447111111111',
service=sample_api_key.service,
personalisation=None,
notification_type='sms',
api_key_id=sample_api_key.id,
key_type=sample_api_key.key_type)
mocked_redis.assert_not_called()
mock_service_template_cache.assert_not_called()
def test_persist_notification_does_not_increment_cache_if_test_key(
sample_template, sample_job, mocker, sample_test_api_key
notify_api, sample_template, sample_job, mocker, sample_test_api_key
):
mocker.patch('app.notifications.process_notifications.redis_store.get', return_value="cache")
mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value="cache")
daily_limit_cache = mocker.patch('app.notifications.process_notifications.redis_store.incr')
template_usage_cache = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value')
assert Notification.query.count() == 0
assert NotificationHistory.query.count() == 0
persist_notification(
template_id=sample_template.id,
template_version=sample_template.version,
recipient='+447111111111',
service=sample_template.service,
personalisation={},
notification_type='sms',
api_key_id=sample_test_api_key.id,
key_type=sample_test_api_key.key_type,
job_id=sample_job.id,
job_row_number=100,
reference="ref",
)
assert Notification.query.count() == 1
with set_config(notify_api, 'REDIS_ENABLED', True):
persist_notification(
template_id=sample_template.id,
template_version=sample_template.version,
recipient='+447111111111',
service=sample_template.service,
personalisation={},
notification_type='sms',
api_key_id=sample_test_api_key.id,
key_type=sample_test_api_key.key_type,
job_id=sample_job.id,
job_row_number=100,
reference="ref",
)
assert not daily_limit_cache.called
assert not template_usage_cache.called
assert Notification.query.count() == 1
assert not daily_limit_cache.called
@freeze_time("2016-01-01 11:09:00.061258")
@@ -198,77 +196,48 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key):
@freeze_time("2016-01-01 11:09:00.061258")
def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(notify_db_session, mocker):
service = create_service(restricted=True)
template = create_template(service=service)
api_key = create_api_key(service=service)
mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr')
mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=None)
mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value=None)
persist_notification(
template_id=template.id,
template_version=template.version,
recipient='+447111111111',
service=template.service,
personalisation={},
notification_type='sms',
api_key_id=api_key.id,
key_type=api_key.key_type,
reference="ref"
)
mock_incr.assert_not_called()
@freeze_time("2016-01-01 11:09:00.061258")
def test_persist_notification_increments_cache_if_key_exists_and_for_trial_service(
notify_db_session, mocker
def test_persist_notification_increments_cache_for_trial_service(
notify_api, notify_db_session, mocker
):
service = create_service(restricted=True)
template = create_template(service=service)
api_key = create_api_key(service=service)
mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr')
mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1)
mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash',
return_value={template.id, 1})
with set_config(notify_api, 'REDIS_ENABLED', True):
persist_notification(
template_id=template.id,
template_version=template.version,
recipient='+447111111122',
service=template.service,
personalisation={},
notification_type='sms',
api_key_id=api_key.id,
key_type=api_key.key_type,
reference="ref2")
persist_notification(
template_id=template.id,
template_version=template.version,
recipient='+447111111122',
service=template.service,
personalisation={},
notification_type='sms',
api_key_id=api_key.id,
key_type=api_key.key_type,
reference="ref2")
mock_incr.assert_called_once_with(str(service.id) + "-2016-01-01-count", )
mock_incr.assert_called_once_with(str(service.id) + "-2016-01-01-count", )
def test_persist_notification_does_not_increments_cache_live_service(
notify_db_session, mocker
def test_persist_notification_increments_cache_live_service(
notify_api, notify_db_session, mocker
):
service = create_service(restricted=False)
template = create_template(service=service)
api_key = create_api_key(service=service)
mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr')
mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1)
mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash',
return_value={template.id, 1})
with set_config(notify_api, 'REDIS_ENABLED', True):
persist_notification(
template_id=template.id,
template_version=template.version,
recipient='+447111111122',
service=template.service,
personalisation={},
notification_type='sms',
api_key_id=api_key.id,
key_type=api_key.key_type,
reference="ref2")
persist_notification(
template_id=template.id,
template_version=template.version,
recipient='+447111111122',
service=template.service,
personalisation={},
notification_type='sms',
api_key_id=api_key.id,
key_type=api_key.key_type,
reference="ref2")
assert not mock_incr.called
assert mock_incr.called
@pytest.mark.parametrize((

201
tests/app/notifications/test_validators.py
View File

@@ -1,4 +1,4 @@
from unittest.mock import ANY
from datetime import datetime
import pytest
from flask import current_app
@@ -15,6 +15,7 @@ from app.notifications.validators import (
check_if_service_can_send_files_by_email,
check_is_message_too_long,
check_notification_content_is_not_empty,
check_rate_limiting,
check_reply_to,
check_service_email_reply_to_id,
check_service_letter_contact_id,
@@ -23,6 +24,7 @@ from app.notifications.validators import (
check_service_sms_sender_id,
check_template_is_active,
check_template_is_for_notification_type,
get_service_daily_limit_cache_value,
service_can_send_to_recipient,
validate_address,
validate_and_format_recipient,
@@ -38,7 +40,6 @@ from app.v2.errors import BadRequestError, RateLimitError, TooManyRequestsError
from tests.app.db import (
create_api_key,
create_letter_contact,
create_notification,
create_reply_to_email,
create_service,
create_service_guest_list,
@@ -55,59 +56,31 @@ def enable_redis(notify_api):
yield
@pytest.mark.parametrize('key_type', ['test', 'team', 'normal'])
def test_check_service_message_limit_in_cache_with_unrestricted_service_is_allowed(
key_type,
sample_service,
mocker):
mocker.patch('app.notifications.validators.redis_store.get', side_effect=[
None, # The serialised service
1, # The rolling count
])
mocker.patch('app.notifications.validators.redis_store.set')
mocker.patch('app.notifications.validators.services_dao')
serialised_service = SerialisedService.from_id(sample_service.id)
check_service_over_daily_message_limit(key_type, serialised_service)
app.notifications.validators.redis_store.set.assert_called_once_with(
f'service-{serialised_service.id}',
ANY,
ex=ANY,
)
assert not app.notifications.validators.services_dao.mock_calls
@pytest.mark.parametrize('key_type', ['test', 'team', 'normal'])
@pytest.mark.parametrize('key_type', ['team', 'normal'])
def test_check_service_message_limit_in_cache_under_message_limit_passes(
key_type,
sample_service,
mocker):
mocker.patch('app.notifications.validators.redis_store.get', side_effect=[
None, # The serialised service
1, # The rolling count
])
mocker.patch('app.notifications.validators.redis_store.set')
mocker.patch('app.notifications.validators.services_dao')
serialised_service = SerialisedService.from_id(sample_service.id)
mock_get = mocker.patch('app.notifications.validators.redis_store.get', return_value=1)
mock_set = mocker.patch('app.notifications.validators.redis_store.set')
check_service_over_daily_message_limit(key_type, serialised_service)
app.notifications.validators.redis_store.set.assert_called_once_with(
f'service-{serialised_service.id}',
ANY,
ex=ANY,
)
assert not app.notifications.validators.services_dao.mock_calls
mock_get.assert_called_once_with(f'{serialised_service.id}-{datetime.utcnow().strftime("%Y-%m-%d")}-count')
assert mock_get.called
assert not mock_set.called
def test_should_not_interact_with_cache_for_test_key(sample_service, mocker):
mocker.patch('app.notifications.validators.redis_store')
mocker.patch('app.notifications.validators.redis_store.get', side_effect=[None])
mock_get = mocker.patch('app.notifications.validators.redis_store.get', side_effect=[None])
serialised_service = SerialisedService.from_id(sample_service.id)
check_service_over_daily_message_limit('test', serialised_service)
assert not app.notifications.validators.redis_store.mock_calls
assert not mock_get.called
@pytest.mark.parametrize('key_type', ['team', 'normal'])
def test_should_set_cache_value_as_value_from_database_if_cache_not_set(
def test_should_set_cache_value_as_zero_if_cache_not_set(
key_type,
sample_template,
sample_service,
@@ -115,75 +88,34 @@ def test_should_set_cache_value_as_value_from_database_if_cache_not_set(
):
serialised_service = SerialisedService.from_id(sample_service.id)
with freeze_time("2016-01-01 12:00:00.000000"):
for _ in range(5):
create_notification(sample_template)
mocker.patch('app.notifications.validators.redis_store.get', return_value=None)
mocker.patch('app.notifications.validators.redis_store.set')
check_service_over_daily_message_limit(key_type, serialised_service)
app.notifications.validators.redis_store.set.assert_called_with(
str(sample_service.id) + "-2016-01-01-count", 5, ex=3600
str(sample_service.id) + "-2016-01-01-count", 0, ex=86400
)
def test_should_not_access_database_if_redis_disabled(notify_api, sample_service, mocker):
def test_does_nothing_if_redis_disabled(notify_api, sample_service, mocker):
serialised_service = SerialisedService.from_id(sample_service.id)
with set_config(notify_api, 'REDIS_ENABLED', False):
db_mock = mocker.patch('app.notifications.validators.services_dao')
mock_cache_key = mocker.patch('notifications_utils.clients.redis.daily_limit_cache_key')
check_service_over_daily_message_limit('normal', serialised_service)
assert db_mock.method_calls == []
assert mock_cache_key.method_calls == []
@pytest.mark.parametrize('key_type', ['team', 'normal'])
def test_check_service_message_limit_over_message_limit_fails(key_type, sample_service, mocker):
def test_check_service_message_limit_over_message_limit_fails(key_type, mocker, notify_db_session):
with freeze_time("2016-01-01 12:00:00.000000"):
mocker.patch('app.redis_store.get', return_value=None)
mocker.patch('app.notifications.validators.redis_store.set')
sample_service.restricted = True
sample_service.message_limit = 4
template = create_template(sample_service)
serialised_service = SerialisedService.from_id(sample_service.id)
for _ in range(5):
create_notification(template)
with pytest.raises(TooManyRequestsError) as e:
check_service_over_daily_message_limit(key_type, serialised_service)
assert e.value.status_code == 429
assert e.value.message == 'Exceeded send limits (4) for today'
assert e.value.fields == []
app.notifications.validators.redis_store.set.assert_called_with(
str(sample_service.id) + "-2016-01-01-count", 5, ex=3600
)
@pytest.mark.parametrize('key_type', ['team', 'normal'])
def test_check_service_message_limit_in_cache_over_message_limit_fails(
notify_db_session,
key_type,
mocker):
with freeze_time("2016-01-01 12:00:00.000000"):
mocker.patch('app.redis_store.get', side_effect=[
None, # The serialised service
5, # The rolling count
])
mocker.patch('app.notifications.validators.redis_store.set')
mocker.patch('app.notifications.validators.services_dao')
service = create_service(restricted=True, message_limit=4)
serialised_service = SerialisedService.from_id(service.id)
mocker.patch('app.redis_store.get', return_value=5)
with pytest.raises(TooManyRequestsError) as e:
check_service_over_daily_message_limit(key_type, serialised_service)
check_service_over_daily_message_limit(key_type, service)
assert e.value.status_code == 429
assert e.value.message == 'Exceeded send limits (4) for today'
assert e.value.fields == []
app.notifications.validators.redis_store.set.assert_called_once_with(
f'service-{serialised_service.id}',
ANY,
ex=ANY,
)
assert not app.notifications.validators.services_dao.mock_calls
@pytest.mark.parametrize('template_type, notification_type',
@@ -451,7 +383,6 @@ def test_that_when_exceed_rate_limit_request_fails(
api_key_type = key_type
mocker.patch('app.redis_store.exceeded_rate_limit', return_value=True)
mocker.patch('app.notifications.validators.services_dao')
sample_service.restricted = True
api_key = create_api_key(sample_service, key_type=api_key_type)
@@ -478,7 +409,6 @@ def test_that_when_not_exceeded_rate_limit_request_succeeds(
mocker):
with freeze_time("2016-01-01 12:00:00.000000"):
mocker.patch('app.redis_store.exceeded_rate_limit', return_value=False)
mocker.patch('app.notifications.validators.services_dao')
sample_service.restricted = True
api_key = create_api_key(sample_service)
@@ -500,7 +430,6 @@ def test_should_not_rate_limit_if_limiting_is_disabled(
current_app.config['API_RATE_LIMIT_ENABLED'] = False
mocker.patch('app.redis_store.exceeded_rate_limit', return_value=False)
mocker.patch('app.notifications.validators.services_dao')
sample_service.restricted = True
create_api_key(sample_service)
@@ -511,6 +440,96 @@ def test_should_not_rate_limit_if_limiting_is_disabled(
assert not app.redis_store.exceeded_rate_limit.called
def test_check_rate_limiting_checks_daily_limit_if_trial_mode_service(
notify_db_session, mocker
):
mock_daily_limit = mocker.patch('app.notifications.validators.check_service_over_daily_message_limit')
trial_mode_service = create_service(restricted=True, message_limit=5)
team_key = create_api_key(service=trial_mode_service, key_type='team')
check_rate_limiting(trial_mode_service, team_key)
mock_daily_limit.assert_called_once_with('team', trial_mode_service)
def test_check_rate_limiting_checks_daily_limit_if_live_service(
notify_db_session, mocker
):
mock_daily_limit = mocker.patch('app.notifications.validators.check_service_over_daily_message_limit')
live_service = create_service(restricted=False, message_limit=5)
live_key = create_api_key(service=live_service, key_type='normal')
check_rate_limiting(service=live_service, api_key=live_key)
mock_daily_limit.assert_called_once_with('normal', live_service)
def test_check_rate_limiting_checks_rate_limit_if_trial_mode_service(
notify_db_session, mocker
):
mock_rate_limit = mocker.patch('app.notifications.validators.check_service_over_api_rate_limit')
trial_mode_service = create_service(restricted=True, message_limit=5)
team_key = create_api_key(service=trial_mode_service, key_type='team')
check_rate_limiting(trial_mode_service, team_key)
mock_rate_limit.assert_called_once_with(trial_mode_service, team_key)
def test_check_rate_limiting_checks_rate_limit_if_live_service(
notify_db_session, mocker
):
mock_rate_limit = mocker.patch('app.notifications.validators.check_service_over_api_rate_limit')
live_service = create_service(restricted=False, message_limit=5)
live_key = create_api_key(service=live_service, key_type='normal')
check_rate_limiting(service=live_service, api_key=live_key)
mock_rate_limit.assert_called_once_with(live_service, live_key)
def test_get_service_daily_limit_cache_value(
notify_api, notify_db_session, mocker
):
mock_get = mocker.patch('app.redis_store.get', return_value=5)
service = create_service()
with set_config(notify_api, 'REDIS_ENABLED', True):
get_service_daily_limit_cache_value('normal', service)
mock_get.assert_called_once_with(f'{str(service.id)}-{datetime.utcnow().strftime("%Y-%m-%d")}-count')
def test_get_service_daily_limit_cache_value_return_zero_and_sets_cache_if_key_not_found_in_cache(
notify_api, notify_db_session, mocker
):
mock_get = mocker.patch('app.redis_store.get', return_value=None)
mock_set = mocker.patch('app.redis_store.set')
service = create_service()
with set_config(notify_api, 'REDIS_ENABLED', True):
assert get_service_daily_limit_cache_value('normal', service) == 0
mock_get.assert_called_once_with(f'{str(service.id)}-{datetime.utcnow().strftime("%Y-%m-%d")}-count')
mock_set.assert_called_once_with(
f'{str(service.id)}-{datetime.utcnow().strftime("%Y-%m-%d")}-count',
0,
ex=86400
)
def test_get_service_daily_limit_cache_value_returns_zero_if_test_api_key(
notify_api, notify_db_session, mocker
):
mock_get = mocker.patch('app.redis_store.get', return_value=None)
mock_set = mocker.patch('app.redis_store.set')
service = create_service()
with set_config(notify_api, 'REDIS_ENABLED', True):
assert get_service_daily_limit_cache_value('test', service) == 0
assert not mock_get.called
assert not mock_set.called
def test_get_service_daily_limit_cache_value_returns_zero_if_redis_is_not_enabled(
notify_api, notify_db_session, mocker
):
mock_get = mocker.patch('app.redis_store.get', return_value=None)
mock_set = mocker.patch('app.redis_store.set')
service = create_service()
# redis is not enabled for the test environment but is always enabled for production
assert get_service_daily_limit_cache_value('test', service) == 0
assert not mock_get.called
assert not mock_set.called
@pytest.mark.parametrize('key_type', ['test', 'normal'])
def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_int_sms(
key_type,