From c0c62e02b72b8a7b10e199d2d4f82e722ae1515a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 13 Oct 2016 14:17:17 +0100 Subject: [PATCH 1/4] move statsd call out of generic tryexcept we shouldn't try and use statsd to log an error if they fail, for example [we also shouldn't retry sending the message but that's a problem for another time] --- app/clients/email/aws_ses.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index 63536ae43..4607d5fda 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -90,13 +90,15 @@ class AwsSesClient(EmailClient): }, 'Body': body }, - ReplyToAddresses=reply_to_addresses) - elapsed_time = monotonic() - start_time - current_app.logger.info("AWS SES request finished in {}".format(elapsed_time)) - self.statsd_client.timing("clients.ses.request-time", elapsed_time) - self.statsd_client.incr("clients.ses.success") - return response['MessageId'] + ReplyToAddresses=reply_to_addresses + ) except Exception as e: # TODO logging exceptions self.statsd_client.incr("clients.ses.error") raise AwsSesClientException(str(e)) + + elapsed_time = monotonic() - start_time + current_app.logger.info("AWS SES request finished in {}".format(elapsed_time)) + self.statsd_client.timing("clients.ses.request-time", elapsed_time) + self.statsd_client.incr("clients.ses.success") + return response['MessageId'] From a095aa41f3b3eb2cab607000b838061ed4ffd1f1 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 13 Oct 2016 15:27:47 +0100 Subject: [PATCH 2/4] don't retry task if InvalidEmailError just record it as a technical error - retrying wont fix a bad email --- app/celery/provider_tasks.py | 9 +++++++-- app/clients/email/aws_ses.py | 22 +++++++++++++++------- tests/app/celery/test_provider_tasks.py | 14 +++++++++++++- 3 files changed, 35 insertions(+), 10 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index c94120e79..b2e3a346d 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -1,12 +1,12 @@ from flask import current_app +from notifications_utils.recipients import InvalidEmailError +from sqlalchemy.orm.exc import NoResultFound from app import notify_celery from app.dao import notifications_dao from app.dao.notifications_dao import update_notification_status_by_id from app.statsd_decorators import statsd - from app.delivery import send_to_providers -from sqlalchemy.orm.exc import NoResultFound def retry_iteration_to_delay(retry=0): @@ -64,6 +64,9 @@ def deliver_email(self, notification_id): if not notification: raise NoResultFound() send_to_providers.send_email_to_provider(notification) + except InvalidEmailError as e: + current_app.logger.exception(e) + update_notification_status_by_id(notification_id, 'technical-failure') except Exception as e: try: current_app.logger.error( @@ -82,6 +85,7 @@ def deliver_email(self, notification_id): @notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5) @statsd(namespace="tasks") def send_sms_to_provider(self, service_id, notification_id): + # todo: delete this task? try: notification = notifications_dao.get_notification_by_id(notification_id) if not notification: @@ -105,6 +109,7 @@ def send_sms_to_provider(self, service_id, notification_id): @notify_celery.task(bind=True, name="send-email-to-provider", max_retries=5, default_retry_delay=5) @statsd(namespace="tasks") def send_email_to_provider(self, service_id, notification_id): + # todo: delete this task? try: notification = notifications_dao.get_notification_by_id(notification_id) if not notification: diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index 4607d5fda..aeab927cd 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -1,6 +1,9 @@ import boto3 +import botocore from flask import current_app from monotonic import monotonic +from notifications_utils.recipients import InvalidEmailError + from app.clients import STATISTICS_DELIVERED, STATISTICS_FAILURE from app.clients.email import (EmailClientException, EmailClient) @@ -92,13 +95,18 @@ class AwsSesClient(EmailClient): }, ReplyToAddresses=reply_to_addresses ) + except botocore.exceptions.ClientError as e: + if e.response['Error']['Code'] == 'InvalidParameterValue': + raise InvalidEmailError('email: "{}" message: "{}"'.format( + to_addresses[0], + e.response['Error']['Message'] + )) except Exception as e: - # TODO logging exceptions self.statsd_client.incr("clients.ses.error") raise AwsSesClientException(str(e)) - - elapsed_time = monotonic() - start_time - current_app.logger.info("AWS SES request finished in {}".format(elapsed_time)) - self.statsd_client.timing("clients.ses.request-time", elapsed_time) - self.statsd_client.incr("clients.ses.success") - return response['MessageId'] + else: + elapsed_time = monotonic() - start_time + current_app.logger.info("AWS SES request finished in {}".format(elapsed_time)) + self.statsd_client.timing("clients.ses.request-time", elapsed_time) + self.statsd_client.incr("clients.ses.success") + return response['MessageId'] diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 7ab7a9c20..290b30722 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -1,10 +1,13 @@ from celery.exceptions import MaxRetriesExceededError +from notifications_utils.recipients import InvalidEmailError + +import app from app.celery import provider_tasks from app.celery.provider_tasks import send_sms_to_provider, send_email_to_provider, deliver_sms, deliver_email from app.clients.email import EmailClientException from app.models import Notification + from tests.app.conftest import sample_notification -import app def test_should_have_decorated_tasks_functions(): @@ -229,3 +232,12 @@ def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task db_notification = Notification.query.filter_by(id=notification.id).one() assert db_notification.status == 'technical-failure' + +def test_should_technical_error_and_not_retry_if_invalid_email(sample_notification, mocker): + mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=InvalidEmailError('bad email')) + mocker.patch('app.celery.provider_tasks.deliver_email.retry') + + deliver_email(sample_notification.id) + + assert provider_tasks.deliver_email.retry.called is False + assert sample_notification.status == 'technical-failure' From a2c3d265deeb136db968ce7f71cfb51448e8924e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 13 Oct 2016 15:53:01 +0100 Subject: [PATCH 3/4] remove unused former send_sms_to_provider and send_sms_to_email functions they were superceded by deliver_sms and deliver_email in the same file 3 wks ago --- app/celery/provider_tasks.py | 48 --------- tests/app/celery/test_provider_tasks.py | 136 ++---------------------- tests/app/celery/test_tasks.py | 4 +- 3 files changed, 10 insertions(+), 178 deletions(-) diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py index b2e3a346d..ef98ec5e3 100644 --- a/app/celery/provider_tasks.py +++ b/app/celery/provider_tasks.py @@ -80,51 +80,3 @@ def deliver_email(self, notification_id): e ) update_notification_status_by_id(notification_id, 'technical-failure') - - -@notify_celery.task(bind=True, name="send-sms-to-provider", max_retries=5, default_retry_delay=5) -@statsd(namespace="tasks") -def send_sms_to_provider(self, service_id, notification_id): - # todo: delete this task? - try: - notification = notifications_dao.get_notification_by_id(notification_id) - if not notification: - raise NoResultFound() - send_to_providers.send_sms_to_provider(notification) - except Exception as e: - try: - current_app.logger.error( - "RETRY: SMS notification {} failed".format(notification_id) - ) - current_app.logger.exception(e) - self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) - except self.MaxRetriesExceededError: - current_app.logger.error( - "RETRY FAILED: task send_sms_to_provider failed for notification {}".format(notification_id), - e - ) - update_notification_status_by_id(notification_id, 'technical-failure') - - -@notify_celery.task(bind=True, name="send-email-to-provider", max_retries=5, default_retry_delay=5) -@statsd(namespace="tasks") -def send_email_to_provider(self, service_id, notification_id): - # todo: delete this task? - try: - notification = notifications_dao.get_notification_by_id(notification_id) - if not notification: - raise NoResultFound() - send_to_providers.send_email_to_provider(notification) - except Exception as e: - try: - current_app.logger.error( - "RETRY: Email notification {} failed".format(notification_id) - ) - current_app.logger.exception(e) - self.retry(queue="retry", countdown=retry_iteration_to_delay(self.request.retries)) - except self.MaxRetriesExceededError: - current_app.logger.error( - "RETRY FAILED: task send_email_to_provider failed for notification {}".format(notification_id), - e - ) - update_notification_status_by_id(notification_id, 'technical-failure') diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 290b30722..7271e9a4c 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -3,7 +3,7 @@ from notifications_utils.recipients import InvalidEmailError import app from app.celery import provider_tasks -from app.celery.provider_tasks import send_sms_to_provider, send_email_to_provider, deliver_sms, deliver_email +from app.celery.provider_tasks import deliver_sms, deliver_email from app.clients.email import EmailClientException from app.models import Notification @@ -68,32 +68,6 @@ def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_sms_task app.celery.provider_tasks.deliver_sms.retry.assert_called_with(queue="retry", countdown=10) -def test_should_call_send_sms_to_provider_from_send_sms_to_provider_task( - notify_db, - notify_db_session, - sample_notification, - mocker): - mocker.patch('app.delivery.send_to_providers.send_sms_to_provider') - - send_sms_to_provider(sample_notification.service_id, sample_notification.id) - app.delivery.send_to_providers.send_sms_to_provider.assert_called_with(sample_notification) - - -def test_should_add_to_retry_queue_if_notification_not_found_in_send_sms_to_provider_task( - notify_db, - notify_db_session, - mocker): - mocker.patch('app.delivery.send_to_providers.send_sms_to_provider') - mocker.patch('app.celery.provider_tasks.send_sms_to_provider.retry') - - notification_id = app.create_uuid() - service_id = app.create_uuid() - - send_sms_to_provider(service_id, notification_id) - app.delivery.send_to_providers.send_sms_to_provider.assert_not_called() - app.celery.provider_tasks.send_sms_to_provider.retry.assert_called_with(queue="retry", countdown=10) - - def test_should_call_send_email_to_provider_from_deliver_email_task( notify_db, notify_db_session, @@ -105,10 +79,7 @@ def test_should_call_send_email_to_provider_from_deliver_email_task( app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_notification) -def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_email_task( - notify_db, - notify_db_session, - mocker): +def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_email_task(mocker): mocker.patch('app.delivery.send_to_providers.send_email_to_provider') mocker.patch('app.celery.provider_tasks.deliver_email.retry') @@ -119,119 +90,28 @@ def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_email_ta app.celery.provider_tasks.deliver_email.retry.assert_called_with(queue="retry", countdown=10) -def test_should_call_send_email_to_provider_from_email_task( - notify_db, - notify_db_session, - sample_notification, - mocker): - mocker.patch('app.delivery.send_to_providers.send_email_to_provider') - - send_email_to_provider(sample_notification.service_id, sample_notification.id) - app.delivery.send_to_providers.send_email_to_provider.assert_called_with(sample_notification) - - -def test_should_add_to_retry_queue_if_notification_not_found_in_send_email_to_provider_task( - notify_db, - notify_db_session, - mocker): - mocker.patch('app.delivery.send_to_providers.send_email_to_provider') - mocker.patch('app.celery.provider_tasks.send_email_to_provider.retry') - - notification_id = app.create_uuid() - service_id = app.create_uuid() - - send_email_to_provider(service_id, notification_id) - app.delivery.send_to_providers.send_email_to_provider.assert_not_called() - app.celery.provider_tasks.send_email_to_provider.retry.assert_called_with(queue="retry", countdown=10) - - # DO THESE FOR THE 4 TYPES OF TASK -def test_should_go_into_technical_error_if_exceeds_retries_on_send_sms_to_provider_task( - notify_db, - notify_db_session, - sample_service, - mocker): - notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - service=sample_service, status='created') - - mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=Exception("EXPECTED")) - mocker.patch('app.celery.provider_tasks.send_sms_to_provider.retry', side_effect=MaxRetriesExceededError()) - - send_sms_to_provider( - notification.service_id, - notification.id - ) - - provider_tasks.send_sms_to_provider.retry.assert_called_with(queue='retry', countdown=10) - - db_notification = Notification.query.filter_by(id=notification.id).one() - assert db_notification.status == 'technical-failure' - - -def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task( - notify_db, - notify_db_session, - sample_service, - mocker): - notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - service=sample_service, status='created') - +def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task(sample_notification, mocker): mocker.patch('app.delivery.send_to_providers.send_sms_to_provider', side_effect=Exception("EXPECTED")) mocker.patch('app.celery.provider_tasks.deliver_sms.retry', side_effect=MaxRetriesExceededError()) - deliver_sms( - notification.id - ) + deliver_sms(sample_notification.id) provider_tasks.deliver_sms.retry.assert_called_with(queue='retry', countdown=10) - db_notification = Notification.query.filter_by(id=notification.id).one() - assert db_notification.status == 'technical-failure' + assert sample_notification.status == 'technical-failure' -def test_send_email_to_provider_should_go_into_technical_error_if_exceeds_retries_on_send_email_to_provider_task( - notify_db, - notify_db_session, - sample_service, - sample_email_template, - mocker): - notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - service=sample_service, status='created', template=sample_email_template) - - mocker.patch('app.aws_ses_client.send_email', side_effect=EmailClientException("EXPECTED")) - mocker.patch('app.celery.provider_tasks.send_email_to_provider.retry', side_effect=MaxRetriesExceededError()) - - send_email_to_provider( - notification.service_id, - notification.id - ) - - provider_tasks.send_email_to_provider.retry.assert_called_with(queue='retry', countdown=10) - - db_notification = Notification.query.filter_by(id=notification.id).one() - assert db_notification.status == 'technical-failure' - - -def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task( - notify_db, - notify_db_session, - sample_service, - mocker): - notification = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, - service=sample_service, status='created') - +def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task(sample_notification, mocker): mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=Exception("EXPECTED")) mocker.patch('app.celery.provider_tasks.deliver_email.retry', side_effect=MaxRetriesExceededError()) - deliver_email( - notification.id - ) + deliver_email(sample_notification.id) provider_tasks.deliver_email.retry.assert_called_with(queue='retry', countdown=10) + assert sample_notification.status == 'technical-failure' - db_notification = Notification.query.filter_by(id=notification.id).one() - assert db_notification.status == 'technical-failure' def test_should_technical_error_and_not_retry_if_invalid_email(sample_notification, mocker): mocker.patch('app.delivery.send_to_providers.send_email_to_provider', side_effect=InvalidEmailError('bad email')) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 0f11ecf42..4191857c5 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -634,7 +634,7 @@ def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, no assert "07890 300000" not in team_members notification = _notification_json(template, "07700 900849") - mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() send_sms( @@ -643,7 +643,7 @@ def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, no encryption.encrypt(notification), datetime.utcnow().strftime(DATETIME_FORMAT) ) - assert provider_tasks.send_sms_to_provider.apply_async.called is False + assert provider_tasks.deliver_sms.apply_async.called is False with pytest.raises(NoResultFound): Notification.query.filter_by(id=notification_id).one() From c69d2aa77815151a7b40266c3e83aed5be1ec984 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 13 Oct 2016 16:07:32 +0100 Subject: [PATCH 4/4] add tests for ses client --- app/clients/email/aws_ses.py | 6 ++++ tests/app/clients/test_aws_ses.py | 53 ++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index aeab927cd..b7d24b245 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -96,11 +96,17 @@ class AwsSesClient(EmailClient): ReplyToAddresses=reply_to_addresses ) except botocore.exceptions.ClientError as e: + self.statsd_client.incr("clients.ses.error") + + # http://docs.aws.amazon.com/ses/latest/DeveloperGuide/api-error-codes.html if e.response['Error']['Code'] == 'InvalidParameterValue': raise InvalidEmailError('email: "{}" message: "{}"'.format( to_addresses[0], e.response['Error']['Message'] )) + else: + self.statsd_client.incr("clients.ses.error") + raise AwsSesClientException(str(e)) except Exception as e: self.statsd_client.incr("clients.ses.error") raise AwsSesClientException(str(e)) diff --git a/tests/app/clients/test_aws_ses.py b/tests/app/clients/test_aws_ses.py index e00d65edf..47b5ce9d0 100644 --- a/tests/app/clients/test_aws_ses.py +++ b/tests/app/clients/test_aws_ses.py @@ -1,8 +1,10 @@ +import botocore import pytest from unittest.mock import Mock, ANY +from notifications_utils.recipients import InvalidEmailError from app import aws_ses_client -from app.clients.email.aws_ses import get_aws_responses +from app.clients.email.aws_ses import get_aws_responses, AwsSesClientException def test_should_return_correct_details_for_delivery(): @@ -67,3 +69,52 @@ def test_send_email_handles_reply_to_address(notify_api, mocker, reply_to_addres Message=ANY, ReplyToAddresses=expected_value ) + + +def test_send_email_raises_bad_email_as_InvalidEmailError(mocker): + boto_mock = mocker.patch.object(aws_ses_client, '_client', create=True) + mocker.patch.object(aws_ses_client, 'statsd_client', create=True) + error_response = { + 'Error': { + 'Code': 'InvalidParameterValue', + 'Message': 'some error message from amazon', + 'Type': 'Sender' + } + } + boto_mock.send_email.side_effect = botocore.exceptions.ClientError(error_response, 'opname') + mocker.patch.object(aws_ses_client, 'statsd_client', create=True) + + with pytest.raises(InvalidEmailError) as excinfo: + aws_ses_client.send_email( + source=Mock(), + to_addresses='clearly@invalid@email.com', + subject=Mock(), + body=Mock() + ) + + assert 'some error message from amazon' in excinfo.value.message + assert 'clearly@invalid@email.com' in excinfo.value.message + + +def test_send_email_raises_other_errs_as_AwsSesClientException(mocker): + boto_mock = mocker.patch.object(aws_ses_client, '_client', create=True) + mocker.patch.object(aws_ses_client, 'statsd_client', create=True) + error_response = { + 'Error': { + 'Code': 'ServiceUnavailable', + 'Message': 'some error message from amazon', + 'Type': 'Sender' + } + } + boto_mock.send_email.side_effect = botocore.exceptions.ClientError(error_response, 'opname') + mocker.patch.object(aws_ses_client, 'statsd_client', create=True) + + with pytest.raises(AwsSesClientException) as excinfo: + aws_ses_client.send_email( + source=Mock(), + to_addresses=Mock(), + subject=Mock(), + body=Mock() + ) + + assert 'some error message from amazon' in str(excinfo.value)