From 65197a6c91fe7cce62b981bb9d455a2329a80bfb Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 17 Oct 2016 17:41:39 +0100 Subject: [PATCH 1/2] handle Exception and remove duplication in errors.py ensure that if unexpected Exceptions are thrown, we handle them correctly (log and then return JSON) also remove some branches that will never trip, and combine a couple of identical functions --- app/errors.py | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/app/errors.py b/app/errors.py index 046f21690..3aa0b2831 100644 --- a/app/errors.py +++ b/app/errors.py @@ -42,10 +42,7 @@ def register_errors(blueprint): @blueprint.app_errorhandler(400) def bad_request(e): - if isinstance(e, str): - msg = e - else: - msg = e.description or "Invalid request parameters" + msg = e.description or "Invalid request parameters" current_app.logger.exception(msg) return jsonify(result='error', message=str(msg)), 400 @@ -61,10 +58,7 @@ def register_errors(blueprint): @blueprint.app_errorhandler(404) def page_not_found(e): - if isinstance(e, str): - msg = e - else: - msg = e.description or "Not found" + msg = e.description or "Not found" current_app.logger.exception(msg) return jsonify(result='error', message=msg), 404 @@ -73,18 +67,9 @@ def register_errors(blueprint): current_app.logger.exception(e) return jsonify(result='error', message=str(e.description)), 429 - @blueprint.app_errorhandler(500) - def internal_server_error(e): - current_app.logger.exception(e) - return jsonify(result='error', message="Internal server error"), 500 - @blueprint.app_errorhandler(NoResultFound) - def no_result_found(e): - current_app.logger.exception(e) - return jsonify(result='error', message="No result found"), 404 - @blueprint.app_errorhandler(DataError) - def data_error(e): + def no_result_found(e): current_app.logger.exception(e) return jsonify(result='error', message="No result found"), 404 @@ -101,3 +86,10 @@ def register_errors(blueprint): )]} ), 400 return jsonify(result='error', message="Internal server error"), 500 + + # this must be defined after all other error handlers since it catches the generic Exception object + @blueprint.app_errorhandler(500) + @blueprint.app_errorhandler(Exception) + def internal_server_error(e): + current_app.logger.exception(e) + return jsonify(result='error', message="Internal server error"), 500 From a1cc092d3bfa670cfa4f67cc6e41ae31671ffea3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 17 Oct 2016 17:44:17 +0100 Subject: [PATCH 2/2] fix logger.exception syntax when given any log function with multiple parameters, the python logging utils assume the first param is a format string and the rest are arguments to pass in - we were passing in the exception object to `logger.exception`, however, the purpose of .exception is to add the exception object itself - so we didn't need to --- app/celery/scheduled_tasks.py | 10 +++++----- app/notifications/rest.py | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index f9ee251b8..399cc8f2f 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -31,7 +31,7 @@ def run_scheduled_jobs(): process_job.apply_async([str(job.id)], queue="process-job") current_app.logger.info("Job ID {} added to process job queue".format(job.id)) except SQLAlchemyError as e: - current_app.logger.exception("Failed to run scheduled jobs", e) + current_app.logger.exception("Failed to run scheduled jobs") raise @@ -45,7 +45,7 @@ def delete_verify_codes(): "Delete job started {} finished {} deleted {} verify codes".format(start, datetime.utcnow(), deleted) ) except SQLAlchemyError as e: - current_app.logger.exception("Failed to delete verify codes", e) + current_app.logger.exception("Failed to delete verify codes") raise @@ -63,7 +63,7 @@ def delete_successful_notifications(): ) ) except SQLAlchemyError as e: - current_app.logger.exception("Failed to delete successful notifications", e) + current_app.logger.exception("Failed to delete successful notifications") raise @@ -84,7 +84,7 @@ def delete_failed_notifications(): ) ) except SQLAlchemyError as e: - current_app.logger.exception("Failed to delete failed notifications", e) + current_app.logger.exception("Failed to delete failed notifications") raise @@ -98,7 +98,7 @@ def delete_invitations(): "Delete job started {} finished {} deleted {} invitations".format(start, datetime.utcnow(), deleted) ) except SQLAlchemyError as e: - current_app.logger.exception("Failed to delete invitations", e) + current_app.logger.exception("Failed to delete invitations") raise diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 9285cf17d..2a0e9319e 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -309,7 +309,7 @@ def persist_notification( queue='send-email' if not research_mode else 'research-mode' ) except Exception as e: - current_app.logger.exception("Failed to send to SQS exception", e) + current_app.logger.exception("Failed to send to SQS exception") dao_delete_notifications_and_history_by_id(notification_id) raise InvalidRequest(message="Internal server error", status_code=500)