From f4c176ff9622c041591f44f9c5910040cd93b071 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Guillot?= Date: Thu, 3 Nov 2016 11:18:27 -0400 Subject: [PATCH] Fix 500 error when using bad date format or invalid payload - Simplify exception handling in API handler. - The API code absolutely need to be refactored to use the same date format everywhere and to use the same way of handling JSON parsing/serialization, basically validation of incoming data must consistent across the application. Change-Id: Ie490fac8867086e7f3e719a04993f9803e48b350 --- almanach/api/v1/routes.py | 32 +++++++++++++++----------------- almanach/core/exception.py | 12 ++++++++++-- tests/api/test_api_entity.py | 21 +++++++++++++++++++++ tests/api/test_api_instance.py | 2 +- tests/api/test_api_volume.py | 2 +- 5 files changed, 48 insertions(+), 21 deletions(-) diff --git a/almanach/api/v1/routes.py b/almanach/api/v1/routes.py index db49d2e..9a76b6e 100644 --- a/almanach/api/v1/routes.py +++ b/almanach/api/v1/routes.py @@ -30,42 +30,40 @@ auth_adapter = None def to_json(api_call): - def encode(data): + def encode_result(data): return jsonpickle.encode(data, unpicklable=False) + def send_response(data, status_code): + return flask.Response(encode_result(data), status_code, {"Content-Type": "application/json"}) + @wraps(api_call) def decorator(*args, **kwargs): try: result = api_call(*args, **kwargs) - return result if isinstance(result, wrappers.BaseResponse) \ - else flask.Response(encode(result), 200, {"Content-Type": "application/json"}) - except exception.DateFormatException as e: + return result if isinstance(result, wrappers.BaseResponse) else send_response(result, 200) + except (exception.DateFormatException, exception.MultipleEntitiesMatchingQueryException) as e: LOG.warning(e.message) - return flask.Response(encode({"error": e.message}), 400, {"Content-Type": "application/json"}) + return send_response({"error": e.message}, 400) except KeyError as e: message = "The {param} param is mandatory for the request you have made.".format(param=e) LOG.warning(message) - return encode({"error": message}), 400, {"Content-Type": "application/json"} - except TypeError: - message = "The request you have made must have data. None was given." + return send_response({"error": message}, 400) + except (TypeError, ValueError): + message = "Invalid parameter or payload" LOG.warning(message) - return encode({"error": message}), 400, {"Content-Type": "application/json"} + return send_response({"error": message}, 400) except exception.InvalidAttributeException as e: LOG.warning(e.get_error_message()) - return encode({"error": e.get_error_message()}), 400, {"Content-Type": "application/json"} - except exception.MultipleEntitiesMatchingQueryException as e: - LOG.warning(e.message) - return encode({"error": "Multiple entities found while updating closed"}), 400, { - "Content-Type": "application/json"} + return send_response({"error": e.get_error_message()}, 400) except exception.AlmanachEntityNotFoundException as e: LOG.warning(e.message) - return encode({"error": "Entity not found"}), 404, {"Content-Type": "application/json"} + return send_response({"error": e.message}, 404) except exception.AlmanachException as e: LOG.exception(e) - return flask.Response(encode({"error": e.message}), 500, {"Content-Type": "application/json"}) + return send_response({"error": e.message}, 500) except Exception as e: LOG.exception(e) - return flask.Response(encode({"error": e}), 500, {"Content-Type": "application/json"}) + return send_response({"error": e}, 500) return decorator diff --git a/almanach/core/exception.py b/almanach/core/exception.py index 22714a0..d8c44ae 100644 --- a/almanach/core/exception.py +++ b/almanach/core/exception.py @@ -19,7 +19,11 @@ class AlmanachException(Exception): class AlmanachEntityNotFoundException(AlmanachException): - pass + def __init__(self, message=None): + if not message: + message = "Entity not found" + + super(AlmanachEntityNotFoundException, self).__init__(message) class AuthenticationFailureException(AlmanachException): @@ -36,7 +40,11 @@ class DateFormatException(AlmanachException): class MultipleEntitiesMatchingQueryException(AlmanachException): - pass + def __init__(self, message=None): + if not message: + message = "Multiple entities found while updating a closed entity" + + super(MultipleEntitiesMatchingQueryException, self).__init__(message) class InvalidAttributeException(AlmanachException): diff --git a/tests/api/test_api_entity.py b/tests/api/test_api_entity.py index 6af49fb..41c7d66 100644 --- a/tests/api/test_api_entity.py +++ b/tests/api/test_api_entity.py @@ -50,6 +50,27 @@ class ApiEntityTest(BaseApi): assert_that(result, has_key('end')) assert_that(result['start'], is_("2014-01-01 00:00:00+00:00")) + def test_update_active_instance_entity_with_bad_payload(self): + instance_id = 'INSTANCE_ID' + data = { + 'flavor': 'A_FLAVOR', + } + + self.controller.should_receive('update_active_instance_entity') \ + .with_args(instance_id=instance_id, **data) \ + .once() \ + .and_raise(ValueError('Expecting object: line 1 column 15 (char 14)')) + + code, result = self.api_put( + '/entity/instance/INSTANCE_ID', + data=data, + headers={'X-Auth-Token': 'some token value'} + ) + assert_that(result, has_entries({ + "error": 'Invalid parameter or payload' + })) + assert_that(code, equal_to(400)) + def test_update_active_instance_entity_with_wrong_attribute_raise_exception(self): errors = [ Invalid(message="error message1", path=["my_attribute1"]), diff --git a/tests/api/test_api_instance.py b/tests/api/test_api_instance.py index ab3dcca..1069329 100644 --- a/tests/api/test_api_instance.py +++ b/tests/api/test_api_instance.py @@ -207,7 +207,7 @@ class ApiInstanceTest(BaseApi): .never() code, result = self.api_delete('/instance/INSTANCE_ID', headers={'X-Auth-Token': 'some token value'}) - assert_that(result, has_entries({"error": "The request you have made must have data. None was given."})) + assert_that(result, has_entries({"error": "Invalid parameter or payload"})) assert_that(code, equal_to(400)) def test_instance_delete_bad_date_format_returns_bad_request_code(self): diff --git a/tests/api/test_api_volume.py b/tests/api/test_api_volume.py index a6110c8..bf68b4f 100644 --- a/tests/api/test_api_volume.py +++ b/tests/api/test_api_volume.py @@ -116,7 +116,7 @@ class ApiVolumeTest(BaseApi): .never() code, result = self.api_delete('/volume/VOLUME_ID', headers={'X-Auth-Token': 'some token value'}) - assert_that(result, has_entries({"error": "The request you have made must have data. None was given."})) + assert_that(result, has_entries({"error": "Invalid parameter or payload"})) assert_that(code, equal_to(400)) def test_volume_delete_bad_date_format_returns_bad_request_code(self):