From 4be86d1a60d86bc5555db5d5641ca0388148f10f Mon Sep 17 00:00:00 2001 From: Kurt Griffiths Date: Thu, 16 Oct 2014 18:22:57 -0500 Subject: [PATCH] fix(HTTPError): Remove dependency on blank title to determine serialization This patch attempts to make the code easier to understand and maintain going forward by removing the dependency between the title attribute and the "serializable" property. The "serializable" property was renamed to be more indicitive of the context in which it is actually used. Closes #265 --- falcon/api.py | 2 +- falcon/exceptions.py | 28 ++++++++++++++----- falcon/http_error.py | 57 +++++++++++++++++---------------------- tests/test_after_hooks.py | 6 +++-- tests/test_httperror.py | 6 ++--- 5 files changed, 53 insertions(+), 46 deletions(-) diff --git a/falcon/api.py b/falcon/api.py index 73ee726..4a0b05f 100644 --- a/falcon/api.py +++ b/falcon/api.py @@ -371,7 +371,7 @@ class API(object): if error.headers is not None: resp.set_headers(error.headers) - if error.serializable: + if error.has_representation: media_type, body = self._serialize_error(req, error) if body is not None: diff --git a/falcon/exceptions.py b/falcon/exceptions.py index 7d26c0c..856a5e7 100644 --- a/falcon/exceptions.py +++ b/falcon/exceptions.py @@ -94,7 +94,11 @@ class HTTPNotFound(HTTPError): """ def __init__(self): - HTTPError.__init__(self, status.HTTP_404, None, None) + HTTPError.__init__(self, status.HTTP_404) + + @property + def has_representation(self): + return False class HTTPMethodNotAllowed(HTTPError): @@ -116,21 +120,27 @@ class HTTPMethodNotAllowed(HTTPError): HTTPError.__init__(self, status.HTTP_405, 'Method not allowed', **kwargs) + # NOTE(kgriffs): Trigger an empty body in the response; 405 + # responses don't usually have bodies, so we only send one + # if the caller indicates they want one by providing some + # details in the kwargs. if kwargs: title = 'Method not allowed' + self._has_representation = True else: - # NOTE(kgriffs): Trigger an empty body in the response; 405 - # responses don't usually have bodies, so we only send one - # if the caller indicates they want one, by way of specifying - # a description, href, and/or other details. title = None + self._has_representation = False # NOTE(kgriffs): Inject the "Allow" header so it will be included # in the HTTP response. headers = kwargs.setdefault('headers', {}) headers['Allow'] = ', '.join(allowed_methods) - HTTPError.__init__(self, status.HTTP_405, title, **kwargs) + HTTPError.__init__(self, status.HTTP_405, title=title, **kwargs) + + @property + def has_representation(self): + return self._has_representation class HTTPNotAcceptable(HTTPError): @@ -262,7 +272,11 @@ class HTTPRangeNotSatisfiable(HTTPError): def __init__(self, resource_length): headers = {'Content-Range': 'bytes */' + str(resource_length)} - HTTPError.__init__(self, status.HTTP_416, None, headers=headers) + HTTPError.__init__(self, status.HTTP_416, headers=headers) + + @property + def has_representation(self): + return False class HTTPInternalServerError(HTTPError): diff --git a/falcon/http_error.py b/falcon/http_error.py index 4bc4090..fb2de0c 100644 --- a/falcon/http_error.py +++ b/falcon/http_error.py @@ -35,14 +35,11 @@ class HTTPError(Exception): Attributes: status (str): HTTP status line, such as "748 Confounded by Ponies". - serializable (bool): Returns *True* IFF the error should be - serialized when composing the HTTP response. - - Note: - If an app sets a custom error serializer, it will only - be called when the error's `serializable` property is - ``True``. - + has_representation (bool): Read-only property that determines + whether error details will be serialized when composing + the HTTP response. In ``HTTPError`` this property always + returns ``True``, but child classes may override this property + in order to return ``False`` when an empty HTTP body is desired. title (str): Error title to send to the client. Will be ``None`` if the error should result in an HTTP response with an empty body. description (str): Description of the error to send to the client. @@ -54,19 +51,9 @@ class HTTPError(Exception): Args: status (str): HTTP status code and text, such as "400 Bad Request" - title (str): Human-friendly error title. Set to ``None`` if you wish - Falcon to return an empty response body (all remaining args will - be ignored except for headers.) This will set the error's - `serializable` property to ``False``. - - Note: - Set `title` to ``None`` when you don't wish to disclose - sensitive information about why a request was refused, - when the status and headers are self-descriptive, or when - the HTTP specification forbids returning a body for the - status code in question. Keyword Args: + title (str): Human-friendly error title (default ``None``). description (str): Human-friendly description of the error, along with a helpful suggestion or two (default ``None``). headers (dict or list): A dictionary of header names and values @@ -95,7 +82,7 @@ class HTTPError(Exception): for this error"). code (int): An internal code that customers can reference in their support request or to help them when searching for knowledge - base articles related to this error. + base articles related to this error (default ``None``). """ __slots__ = ( @@ -104,7 +91,7 @@ class HTTPError(Exception): 'description', 'headers', 'link', - 'code' + 'code', ) def __init__(self, status, title=None, description=None, headers=None, @@ -124,8 +111,8 @@ class HTTPError(Exception): self.link = None @property - def serializable(self): - return self.title is not None + def has_representation(self): + return True def raw(self, obj_type=dict): """Returns a raw dictionary representing the error. @@ -142,18 +129,20 @@ class HTTPError(Exception): """ - assert self.serializable + assert self.has_representation obj = obj_type() - obj['title'] = self.title - if self.description: + if self.title is not None: + obj['title'] = self.title + + if self.description is not None: obj['description'] = self.description - if self.code: + if self.code is not None: obj['code'] = self.code - if self.link: + if self.link is not None: obj['link'] = self.link return obj @@ -178,18 +167,20 @@ class HTTPError(Exception): """ - assert self.serializable + assert self.has_representation error_element = et.Element('error') - et.SubElement(error_element, 'title').text = self.title - if self.description: + if self.title is not None: + et.SubElement(error_element, 'title').text = self.title + + if self.description is not None: et.SubElement(error_element, 'description').text = self.description - if self.code: + if self.code is not None: et.SubElement(error_element, 'code').text = str(self.code) - if self.link: + if self.link is not None: link_element = et.SubElement(error_element, 'link') for key in ('text', 'href', 'rel'): diff --git a/tests/test_after_hooks.py b/tests/test_after_hooks.py index 0f89c65..f1988da 100644 --- a/tests/test_after_hooks.py +++ b/tests/test_after_hooks.py @@ -5,7 +5,7 @@ import falcon.testing as testing def validate_output(req, resp): - raise falcon.HTTPError(falcon.HTTP_723, title=None) + raise falcon.HTTPError(falcon.HTTP_723, 'Tricky') def serialize_body(req, resp): @@ -257,7 +257,9 @@ class TestHooks(testing.TestBase): def test_output_validator(self): self.simulate_request(self.test_route) self.assertEqual(falcon.HTTP_723, self.srmock.status) - self.assertEqual(None, self.resource.resp.body_encoded) + + expected = b'{\n "title": "Tricky"\n}' + self.assertEqual(expected, self.resource.resp.body_encoded) def test_serializer(self): self.simulate_request(self.test_route, method='PUT') diff --git a/tests/test_httperror.py b/tests/test_httperror.py index 841e8d5..03eb775 100644 --- a/tests/test_httperror.py +++ b/tests/test_httperror.py @@ -37,7 +37,7 @@ class FaultyResource: code=8733224) def on_patch(self, req, resp): - raise falcon.HTTPError(falcon.HTTP_400, 'No-can-do') + raise falcon.HTTPError(falcon.HTTP_400) class UnicodeFaultyResource(object): @@ -166,7 +166,7 @@ class TestHTTPError(testing.TestBase): def test_no_description_json(self): body = self.simulate_request('/fail', method='PATCH') self.assertEqual(self.srmock.status, falcon.HTTP_400) - self.assertEqual(body, [b'{\n "title": "No-can-do"\n}']) + self.assertEqual(body, [b'{}']) def test_no_description_xml(self): body = self.simulate_request('/fail', method='PATCH', @@ -174,7 +174,7 @@ class TestHTTPError(testing.TestBase): self.assertEqual(self.srmock.status, falcon.HTTP_400) expected_xml = (b'' - b'No-can-do') + b'') self.assertEqual(body, [expected_xml])