From 58bbc5a5aa739f1a190d8425bdca75d4f50a4ccd Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 21 Nov 2016 15:11:19 +0000 Subject: [PATCH] Now that we have discovered that the catch all Exception handler doesn't work, I've created a custom exception (SendNotificationQueueError) that handles this case and an error handler for it. Talked these through with @becca as an approach. --- app/errors.py | 6 ++++++ app/notifications/__init__.py | 7 +++++++ app/notifications/process_notifications.py | 6 +++--- app/notifications/rest.py | 5 +---- app/v2/errors.py | 8 ++++++++ tests/app/notifications/test_process_notification.py | 3 ++- 6 files changed, 27 insertions(+), 8 deletions(-) diff --git a/app/errors.py b/app/errors.py index 546f75f0c..1c9c1691d 100644 --- a/app/errors.py +++ b/app/errors.py @@ -6,6 +6,7 @@ from sqlalchemy.exc import SQLAlchemyError, DataError from sqlalchemy.orm.exc import NoResultFound from marshmallow import ValidationError from app.authentication.auth import AuthError +from app.notifications import SendNotificationToQueueError class InvalidRequest(Exception): @@ -83,6 +84,11 @@ def register_errors(blueprint): current_app.logger.exception(e) return jsonify(result='error', message="No result found"), 404 + @blueprint.errorhandler(SendNotificationToQueueError) + def failed_to_create_notification(e): + current_app.logger.exception(e) + return jsonify(result='error', message=e.message), 500 + @blueprint.errorhandler(SQLAlchemyError) def db_error(e): current_app.logger.exception(e) diff --git a/app/notifications/__init__.py b/app/notifications/__init__.py index e69de29bb..a5a575f60 100644 --- a/app/notifications/__init__.py +++ b/app/notifications/__init__.py @@ -0,0 +1,7 @@ + + +class SendNotificationToQueueError(Exception): + status_code = 500 + + def __init__(self): + self.message = "Failed to create the notification" diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 257c3640d..f2bfcfa07 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -9,7 +9,7 @@ from app.celery import provider_tasks from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE from app.notifications.validators import check_sms_content_char_count -from app.v2.errors import BadRequestError +from app.v2.errors import BadRequestError, SendNotificationToQueueError def create_content_for_notification(template, personalisation): @@ -78,9 +78,9 @@ def send_notification_to_queue(notification, research_mode): queue='send-email' if not research_mode else 'research-mode' ) except Exception as e: - current_app.logger.exception("Failed to send to SQS exception") + current_app.logger.exception(e) dao_delete_notifications_and_history_by_id(notification.id) - raise e + raise SendNotificationToQueueError() current_app.logger.info( "{} {} created at {}".format(notification.notification_type, notification.id, notification.created_at) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index c1d579a3c..1859c897e 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -242,10 +242,7 @@ def send_notification(notification_type): notification_type=notification_type, api_key_id=api_user.id, key_type=api_user.key_type) - try: - send_notification_to_queue(saved_notification, service.research_mode) - except Exception as e: - return jsonify(result='error', message="Internal server error"), 500 + send_notification_to_queue(saved_notification, service.research_mode) notification_id = create_uuid() if saved_notification is None else saved_notification.id notification.update({"template_version": template.version}) diff --git a/app/v2/errors.py b/app/v2/errors.py index 26cc3b36e..77958962b 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -5,6 +5,7 @@ from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound from app.authentication.auth import AuthError from app.errors import InvalidRequest +from app.notifications import SendNotificationToQueueError class TooManyRequestsError(InvalidRequest): @@ -47,6 +48,13 @@ def register_errors(blueprint): def auth_error(error): return jsonify(error.to_dict_v2()), error.code + @blueprint.errorhandler(SendNotificationToQueueError) + def failed_to_create_notification(error): + current_app.logger.exception(error) + return jsonify( + status_code=500, + errors=[{"error": error.__class__.__name__, "message": error.message}]), 500 + @blueprint.errorhandler(Exception) def internal_server_error(error): current_app.logger.exception(error) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 851865a6b..9cb0f8887 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -5,6 +5,7 @@ from boto3.exceptions import Boto3Error from sqlalchemy.exc import SQLAlchemyError from app.models import Template, Notification, NotificationHistory +from app.notifications import SendNotificationToQueueError from app.notifications.process_notifications import (create_content_for_notification, persist_notification, send_notification_to_queue) from app.v2.errors import BadRequestError @@ -108,7 +109,7 @@ def test_send_notification_to_queue(notify_db, notify_db_session, def test_send_notification_to_queue_throws_exception_deletes_notification(sample_notification, mocker): mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async', side_effect=Boto3Error("EXPECTED")) - with pytest.raises(Boto3Error): + with pytest.raises(SendNotificationToQueueError): send_notification_to_queue(sample_notification, False) mocked.assert_called_once_with([(str(sample_notification.id))], queue='send-sms')