diff --git a/app/clients/redis/__init__.py b/app/clients/redis/__init__.py index e69de29bb..b85ced64d 100644 --- a/app/clients/redis/__init__.py +++ b/app/clients/redis/__init__.py @@ -0,0 +1,5 @@ +from datetime import datetime + + +def cache_key(service_id): + return "{}-{}-{}".format(str(service_id), datetime.utcnow().strftime("%Y-%m-%d"), "count") diff --git a/app/clients/redis/redis_client.py b/app/clients/redis/redis_client.py index b85450625..b720d9b64 100644 --- a/app/clients/redis/redis_client.py +++ b/app/clients/redis/redis_client.py @@ -1,9 +1,10 @@ from flask.ext.redis import FlaskRedis +from flask import current_app class RedisClient: - active = False redis_store = FlaskRedis() + active = False def init_app(self, app): self.active = app.config.get('REDIS_ENABLED') @@ -11,10 +12,30 @@ class RedisClient: if self.active: self.redis_store.init_app(app) - def set(self, key, value): + def set(self, key, value, ex=None, px=None, nx=False, xx=False, raise_exception=False): if self.active: - self.redis_store.set(key, value) + try: + self.redis_store.set(key, value, ex, px, nx, xx) + except Exception as e: + current_app.logger.exception(e) + if raise_exception: + raise e - def get(self, key): + def inc(self, key, raise_exception=False): if self.active: - self.redis_store.get(key) + try: + return self.redis_store.inc(key) + except Exception as e: + current_app.logger.exception(e) + if raise_exception: + raise e + + def get(self, key, raise_exception=False): + if self.active: + try: + return self.redis_store.get(key) + except Exception as e: + current_app.logger.exception(e) + if raise_exception: + raise e + return None diff --git a/app/notifications/rest.py b/app/notifications/rest.py index eaf6c531a..66b069579 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -206,7 +206,6 @@ def get_notification_statistics_for_day(): @notifications.route('/notifications/', methods=['POST']) def send_notification(notification_type): - redis_store.set('key1', 'value') if notification_type not in ['sms', 'email']: assert False diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 5684dfedb..df0695142 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -4,12 +4,19 @@ from app.dao import services_dao from app.models import KEY_TYPE_TEST, KEY_TYPE_TEAM from app.service.utils import service_allowed_to_send_to from app.v2.errors import TooManyRequestsError, BadRequestError +from app import redis_store +from app.clients import redis def check_service_message_limit(key_type, service): - if all((key_type != KEY_TYPE_TEST, - service.restricted)): - service_stats = services_dao.fetch_todays_total_message_count(service.id) + if key_type != KEY_TYPE_TEST: + cache_key = redis.cache_key(service.id) + service_stats = redis_store.get(cache_key) + if not service_stats: + service_stats = services_dao.fetch_todays_total_message_count(service.id) + redis_store.set(cache_key, service_stats, ex=3600) + print(service_stats) + print(service.message_limit) if service_stats >= service.message_limit: raise TooManyRequestsError(service.message_limit) diff --git a/config.py b/config.py index e009e4b5f..d2b2a77b1 100644 --- a/config.py +++ b/config.py @@ -132,7 +132,7 @@ class Config(object): STATSD_HOST = "statsd.hostedgraphite.com" STATSD_PORT = 8125 - REDIS_ENABLED = False + REDIS_ENABLED = True REDIS_URL = "redis://localhost:6379/0" SENDING_NOTIFICATIONS_TIMEOUT_PERIOD = 259200 @@ -166,6 +166,7 @@ class Development(Config): class Test(Config): + REDIS_ENABLED = False NOTIFY_EMAIL_DOMAIN = 'test.notify.com' FROM_NUMBER = 'testing' NOTIFY_ENVIRONMENT = 'test' diff --git a/tests/app/clients/test_redis_client.py b/tests/app/clients/test_redis_client.py index e5cd78327..f05a3c267 100644 --- a/tests/app/clients/test_redis_client.py +++ b/tests/app/clients/test_redis_client.py @@ -2,36 +2,68 @@ import pytest from unittest.mock import Mock from app.clients.redis.redis_client import RedisClient +from app.clients.redis import cache_key +from freezegun import freeze_time @pytest.fixture(scope='function') -def enabled_redis_client(notify_api): +def enabled_redis_client(notify_api, mocker): notify_api.config['REDIS_ENABLED'] = True - - redis_client = RedisClient() - redis_client.init_app(notify_api) - redis_client.redis_store = Mock() - return redis_client + return build_redis_client(notify_api, mocker) @pytest.fixture(scope='function') -def disabled_redis_client(notify_api): +def disabled_redis_client(notify_api, mocker): notify_api.config['REDIS_ENABLED'] = False + return build_redis_client(notify_api, mocker) + +def build_redis_client(notify_api, mocker): redis_client = RedisClient() redis_client.init_app(notify_api) - redis_client.redis_store = Mock() + mocker.patch.object(redis_client.redis_store, 'get', return_value=100) + mocker.patch.object(redis_client.redis_store, 'set') return redis_client +def test_should_not_raise_exception_if_raise_set_to_false(notify_api): + notify_api.config['REDIS_ENABLED'] = True + redis_client = RedisClient() + redis_client.init_app(notify_api) + redis_client.redis_store.get = Mock(side_effect=Exception()) + redis_client.redis_store.set = Mock(side_effect=Exception()) + redis_client.redis_store.inc = Mock(side_effect=Exception()) + assert redis_client.get('test') is None + assert redis_client.set('test', 'test') is None + assert redis_client.inc('test')is None + + +def test_should_raise_exception_if_raise_set_to_true(notify_api): + notify_api.config['REDIS_ENABLED'] = True + redis_client = RedisClient() + redis_client.init_app(notify_api) + redis_client.redis_store.get = Mock(side_effect=Exception('get failed')) + redis_client.redis_store.set = Mock(side_effect=Exception('set failed')) + redis_client.redis_store.inc = Mock(side_effect=Exception('inc failed')) + with pytest.raises(Exception) as e: + redis_client.get('test', raise_exception=True) + assert str(e.value) == 'get failed' + with pytest.raises(Exception) as e: + redis_client.set('test', 'test', raise_exception=True) + assert str(e.value) == 'set failed' + with pytest.raises(Exception) as e: + redis_client.inc('test', raise_exception=True) + assert str(e.value) == 'inc failed' + + def test_should_not_call_set_if_not_enabled(disabled_redis_client): - disabled_redis_client.set('key', 'value') + assert not disabled_redis_client.set('key', 'value') disabled_redis_client.redis_store.set.assert_not_called() def test_should_call_set_if_enabled(enabled_redis_client): enabled_redis_client.set('key', 'value') - enabled_redis_client.redis_store.set.assert_called_with('key', 'value') + enabled_redis_client.redis_store.set.assert_called_with('key', 'value', None, None, False, False) def test_should_not_call_get_if_not_enabled(disabled_redis_client): @@ -40,5 +72,10 @@ def test_should_not_call_get_if_not_enabled(disabled_redis_client): def test_should_call_get_if_enabled(enabled_redis_client): - enabled_redis_client.get('key') - enabled_redis_client.redis_store.get.assert_called_with('key') \ No newline at end of file + assert enabled_redis_client.get('key') == 100 + enabled_redis_client.redis_store.get.assert_called_with('key') + + +def test_should_build_cache_key_service_and_action(sample_service): + with freeze_time("2016-01-01 12:00:00.000000"): + assert cache_key(sample_service.id) == '{}-2016-01-01-count'.format(sample_service.id) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index a28590c40..b4af5282e 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -343,7 +343,7 @@ def test_should_allow_valid_email_notification(notify_api, sample_email_template @freeze_time("2016-01-01 12:00:00.061258") -def test_should_not_block_api_call_if_over_day_limit_for_live_service( +def test_should_block_api_call_if_over_day_limit_for_live_service( notify_db, notify_db_session, notify_api, @@ -371,8 +371,7 @@ def test_should_not_block_api_call_if_over_day_limit_for_live_service( data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) json.loads(response.get_data(as_text=True)) - - assert response.status_code == 201 + assert response.status_code == 429 @freeze_time("2016-01-01 12:00:00.061258") diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index e41d5f81c..c243dc150 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -1,36 +1,148 @@ import pytest -from app.notifications.validators import check_service_message_limit, check_template_is_for_notification_type, \ - check_template_is_active, service_can_send_to_recipient, check_sms_content_char_count -from app.v2.errors import BadRequestError, TooManyRequestsError -from tests.app.conftest import (sample_notification as create_notification, - sample_service as create_service, sample_service_whitelist) +from freezegun import freeze_time - -@pytest.mark.parametrize('key_type', ['test', 'team', 'normal']) -def test_check_service_message_limit_with_unrestricted_service_passes(key_type, - sample_service, - sample_notification): - assert check_service_message_limit(key_type, sample_service) is None - - -@pytest.mark.parametrize('key_type', ['test', 'team', 'normal']) -def test_check_service_message_limit_under_message_limit_passes(key_type, - sample_service, - sample_notification): - assert check_service_message_limit(key_type, sample_service) is None +import app +from app.notifications.validators import ( + check_service_message_limit, + check_template_is_for_notification_type, + check_template_is_active, + service_can_send_to_recipient, + check_sms_content_char_count +) +from app.v2.errors import ( + BadRequestError, + TooManyRequestsError +) +from tests.app.conftest import ( + sample_notification as create_notification, + sample_service as create_service, + sample_service_whitelist +) @pytest.mark.parametrize('key_type', ['team', 'normal']) -def test_check_service_message_limit_over_message_limit_fails(key_type, notify_db, notify_db_session): +def test_exception_thown_by_redis_store_get_should_not_be_fatal( + notify_db, + notify_db_session, + key_type, + mocker): + mocker.patch('app.notifications.validators.redis_store.redis_store.get', side_effect=Exception("broken redis")) + mocker.patch('app.notifications.validators.redis_store.set') + service = create_service(notify_db, notify_db_session, restricted=True, limit=4) for x in range(5): create_notification(notify_db, notify_db_session, service=service) + with pytest.raises(TooManyRequestsError) as e: check_service_message_limit(key_type, service) assert e.value.status_code == 429 assert e.value.code == '10429' assert e.value.message == 'Exceeded send limits (4) for today' assert e.value.fields == [] + app.notifications.validators.redis_store.set.assert_not_called() + + +@pytest.mark.parametrize('key_type', ['test', 'team', 'normal']) +def test_exception_thown_by_redis_store_set_should_not_be_fatal( + key_type, + sample_service, + mocker): + mocker.patch('app.notifications.validators.redis_store.redis_store.set', side_effect=Exception("broken redis")) + mocker.patch('app.notifications.validators.redis_store.get', return_value=None) + assert not check_service_message_limit(key_type, sample_service) + + +@pytest.mark.parametrize('key_type', ['test', 'team', 'normal']) +def test_check_service_message_limit_in_cache_with_unrestricted_service_passes( + key_type, + sample_service, + mocker): + mocker.patch('app.notifications.validators.redis_store.get', return_value=1) + mocker.patch('app.notifications.validators.redis_store.set') + mocker.patch('app.notifications.validators.services_dao') + assert not check_service_message_limit(key_type, sample_service) + app.notifications.validators.redis_store.set.assert_not_called() + assert not app.notifications.validators.services_dao.mock_calls + + +@pytest.mark.parametrize('key_type', ['test', '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', return_value=1) + mocker.patch('app.notifications.validators.redis_store.set') + mocker.patch('app.notifications.validators.services_dao') + assert not check_service_message_limit(key_type, sample_service) + app.notifications.validators.redis_store.set.assert_not_called() + assert not app.notifications.validators.services_dao.mock_calls + + +def test_should_not_interact_with_cache_for_test_key(sample_service, mocker): + mocker.patch('app.notifications.validators.redis_store') + assert not check_service_message_limit('test', sample_service) + assert not app.notifications.validators.redis_store.mock_calls + + +@pytest.mark.parametrize('key_type', ['team', 'normal']) +def test_should_set_cache_value_as_value_from_database_if_cache_not_set( + key_type, + notify_db, + notify_db_session, + sample_service, + mocker +): + with freeze_time("2016-01-01 12:00:00.000000"): + for x in range(5): + create_notification(notify_db, notify_db_session, service=sample_service) + mocker.patch('app.notifications.validators.redis_store.get', return_value=None) + mocker.patch('app.notifications.validators.redis_store.set') + assert not check_service_message_limit(key_type, sample_service) + 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_over_message_limit_fails(key_type, notify_db, notify_db_session, mocker): + 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') + + service = create_service(notify_db, notify_db_session, restricted=True, limit=4) + for x in range(5): + create_notification(notify_db, notify_db_session, service=service) + with pytest.raises(TooManyRequestsError) as e: + check_service_message_limit(key_type, service) + assert e.value.status_code == 429 + assert e.value.code == '10429' + assert e.value.message == 'Exceeded send limits (4) for today' + assert e.value.fields == [] + app.notifications.validators.redis_store.set.assert_called_with( + str(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, + notify_db_session, + key_type, + mocker): + with freeze_time("2016-01-01 12:00:00.000000"): + mocker.patch('app.redis_store.get', return_value=5) + mocker.patch('app.notifications.validators.redis_store.set') + mocker.patch('app.notifications.validators.services_dao') + + service = create_service(notify_db, notify_db_session, restricted=True, limit=4) + with pytest.raises(TooManyRequestsError) as e: + check_service_message_limit(key_type, service) + assert e.value.status_code == 429 + assert e.value.code == '10429' + assert e.value.message == 'Exceeded send limits (4) for today' + assert e.value.fields == [] + app.notifications.validators.redis_store.set.assert_not_called() + assert not app.notifications.validators.services_dao.mock_calls @pytest.mark.parametrize('template_type, notification_type',