From 2c2eb2abadafd4026eaa64a95d2c8f44a5ed909c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 26 Jul 2019 13:26:20 +0100 Subject: [PATCH] handle 405s Handle werkzeug http errors separately. Also, improve the generic `Exception` handler to be more flexible when exceptions don't look like http errors. note: because they're 405s the route isn't matched, so the v2 error handlers don't trip properly. this might be an issue because the internal endpoints expect a different return format. Might have to think about how to handle this. --- app/__init__.py | 15 +++++++++++++-- tests/app/v2/test_errors.py | 32 ++++++++++++++++++++++++-------- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 9c6405ef0..6c3d982be 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -3,7 +3,7 @@ import random import string import uuid -from flask import _request_ctx_stack, request, g, jsonify +from flask import _request_ctx_stack, request, g, jsonify, make_response from flask_sqlalchemy import SQLAlchemy as _SQLAlchemy from flask_marshmallow import Marshmallow from flask_migrate import Migrate @@ -12,6 +12,7 @@ from notifications_utils.clients.zendesk.zendesk_client import ZendeskClient from notifications_utils.clients.statsd.statsd_client import StatsdClient from notifications_utils.clients.redis.redis_client import RedisClient from notifications_utils import logging, request_helper +from werkzeug.exceptions import HTTPException as WerkzeugHTTPException from werkzeug.local import LocalProxy from app.celery.celery import NotifyCelery @@ -263,7 +264,17 @@ def init_app(app): def exception(error): app.logger.exception(error) # error.code is set for our exception types. - return jsonify(result='error', message=error.message), error.code or 500 + msg = getattr(error, 'message', str(error)) + code = getattr(error, 'code', 500) + return jsonify(result='error', message=msg), code + + @app.errorhandler(WerkzeugHTTPException) + def werkzeug_exception(e): + return make_response( + jsonify(result='error', message=e.description), + e.code, + e.get_headers() + ) @app.errorhandler(404) def page_not_found(e): diff --git a/tests/app/v2/test_errors.py b/tests/app/v2/test_errors.py index f08367f81..1b3416bce 100644 --- a/tests/app/v2/test_errors.py +++ b/tests/app/v2/test_errors.py @@ -1,4 +1,3 @@ -import json import pytest from flask import url_for from sqlalchemy.exc import DataError @@ -10,9 +9,13 @@ def app_for_test(mocker): from flask import Blueprint from app.authentication.auth import AuthError from app.v2.errors import BadRequestError, TooManyRequestsError, JobIncompleteError + from app import init_app app = flask.Flask(__name__) app.config['TESTING'] = True + init_app(app) + from app import statsd_client + statsd_client.init_app(app) from app.v2.errors import register_errors blue = Blueprint("v2_under_test", __name__, url_prefix='/v2/under_test') @@ -58,7 +61,7 @@ def test_auth_error(app_for_test): with app_for_test.test_client() as client: response = client.get(url_for('v2_under_test.raising_auth_error')) assert response.status_code == 403 - error = json.loads(response.get_data(as_text=True)) + error = response.json assert error == {"status_code": 403, "errors": [{"error": "AuthError", "message": "some message"}]} @@ -69,7 +72,7 @@ def test_bad_request_error(app_for_test): with app_for_test.test_client() as client: response = client.get(url_for('v2_under_test.raising_bad_request')) assert response.status_code == 400 - error = json.loads(response.get_data(as_text=True)) + error = response.json assert error == {"status_code": 400, "errors": [{"error": "BadRequestError", "message": "you forgot the thing"}]} @@ -80,7 +83,7 @@ def test_too_many_requests_error(app_for_test): with app_for_test.test_client() as client: response = client.get(url_for('v2_under_test.raising_too_many_requests')) assert response.status_code == 429 - error = json.loads(response.get_data(as_text=True)) + error = response.json assert error == {"status_code": 429, "errors": [{"error": "TooManyRequestsError", "message": "Exceeded send limits (452) for today"}]} @@ -91,7 +94,7 @@ def test_validation_error(app_for_test): with app_for_test.test_client() as client: response = client.get(url_for('v2_under_test.raising_validation_error')) assert response.status_code == 400 - error = json.loads(response.get_data(as_text=True)) + error = response.json assert len(error.keys()) == 2 assert error['status_code'] == 400 assert len(error['errors']) == 2 @@ -106,7 +109,7 @@ def test_data_errors(app_for_test): with app_for_test.test_client() as client: response = client.get(url_for('v2_under_test.raising_data_error')) assert response.status_code == 404 - error = json.loads(response.get_data(as_text=True)) + error = response.json assert error == {"status_code": 404, "errors": [{"error": "DataError", "message": "No result found"}]} @@ -116,7 +119,7 @@ def test_job_incomplete_errors(app_for_test): with app_for_test.test_client() as client: response = client.get(url_for('v2_under_test.raising_job_incomplete_error')) assert response.status_code == 500 - error = json.loads(response.get_data(as_text=True)) + error = response.json assert error == {"status_code": 500, "errors": [{"error": "JobIncompleteError", "message": "Raising job incomplete error"}]} @@ -126,6 +129,19 @@ def test_internal_server_error_handler(app_for_test): with app_for_test.test_client() as client: response = client.get(url_for("v2_under_test.raising_exception")) assert response.status_code == 500 - error = json.loads(response.get_data(as_text=True)) + error = response.json assert error == {"status_code": 500, "errors": [{"error": "AssertionError", "message": "Internal server error"}]} + + +def test_bad_method(app_for_test): + with app_for_test.test_request_context(): + with app_for_test.test_client() as client: + response = client.post(url_for("v2_under_test.raising_exception")) + + assert response.status_code == 405 + + assert response.json == { + "result": "error", + "message": "The method is not allowed for the requested URL." + }