refactor: improve error and status handling (#1701)

The class falcon.http_error.OptionalRepresentation and the attribute
falcon.HTTPError.has_representation were deprecated. The default error
serializer now generates a representation for every error type that derives from
falcon.HTTPError. In addition, Falcon now ensures that any previously set response body is cleared before handling any raised exception.

docs/_newsfragments/452.breakingchange.rst

@@ -0,0 +1,6 @@
+The class :class:`~.falcon.http_error.OptionalRepresentation` and the attribute
+:attr:`~.falcon.HTTPError.has_representation` were deprecated. The default error
+serializer now generates a representation for every error type that derives from
+:class:`falcon.HTTPError`.
+In addition, Falcon now ensures that any previously set response body is cleared
+before handling any raised exception.

docs/_newsfragments/777.breakingchange.rst

@@ -1,2 +1,2 @@
-The class :class:`~.falcon.http_error.NoRepresentation` was removed since
-all :class:`~.falcon.HTTPError` have optional representation.
+The class :class:`~.falcon.http_error.NoRepresentation` was deprecated. All
+subclasses of :class:`falcon.HTTPError` now have a media type representation.

docs/api/errors.rst

@@ -102,12 +102,6 @@ Base Class
 .. autoclass:: falcon.HTTPError
     :members:
 
-Mixins
-------
-
-.. autoclass:: falcon.http_error.OptionalRepresentation
-    :members:
-
 .. _predefined_errors:
 
 Predefined Errors

falcon/app.py

@@ -733,16 +733,6 @@ def set_error_serializer(self, serializer):
             "+json" or "+xml" suffix, the default serializer will
             convert the error to JSON or XML, respectively.
 
-        Note:
-            The default serializer will not render any response body for
-            :class:`~.HTTPError` instances where the `has_representation`
-            property evaluates to ``False`` (such as in the case of types
-            that subclass :class:`falcon.http_error.OptionalRepresentation`
-            and do not provide a description).
-            However a custom serializer will be called regardless of the
-            property value, and it may choose to override the
-            representation logic.
-
         Note:
             A custom serializer set with this method may not be called if the
             default error handler for :class:`~.HTTPError` has been overriden.
@@ -758,7 +748,7 @@ def my_serializer(req, resp, exception):
                 preferred = req.client_prefers(('application/x-yaml',
                                                 'application/json'))
 
-                if exception.has_representation and preferred is not None:
+                if preferred is not None:
                     if preferred == 'application/json':
                         representation = exception.to_json()
                     else:
@@ -936,6 +926,8 @@ def _handle_exception(self, req, resp, ex, params):
         """
         err_handler = self._find_error_handler(ex)
 
+        # NOTE(caselit): Reset body, data and media before calling the handler
+        resp.body = resp.data = resp.media = None
         if err_handler is not None:
             try:
                 err_handler(req, resp, ex, params)

falcon/app_helpers.py

@@ -158,11 +158,6 @@ def default_serialize_error(req, resp, exception):
         resp: Instance of ``falcon.Response``
         exception: Instance of ``falcon.HTTPError``
     """
-    if not exception.has_representation:
-        return
-
-    representation = None
-
     preferred = req.client_prefers(('application/xml',
                                     'text/xml',
                                     'application/json'))
@@ -188,11 +183,10 @@ def default_serialize_error(req, resp, exception):
 
     if preferred is not None:
         if preferred == 'application/json':
-            representation = exception.to_json()
+            resp.body = exception.to_json()
         else:
-            representation = exception.to_xml()
-
-        resp.body = representation
+            # NOTE(caselit): to_xml already returns bytes
+            resp.data = exception.to_xml()
 
         # NOTE(kgriffs): No need to append the charset param, since
         #   utf-8 is the default for both JSON and XML.

falcon/asgi/app.py

@@ -771,6 +771,8 @@ async def _handle_exception(self, req, resp, ex, params):
         """
         err_handler = self._find_error_handler(ex)
 
+        # NOTE(caselit): Reset body, data and media before calling the handler
+        resp.body = resp.data = resp.media = None
         if err_handler is not None:
             try:
                 await err_handler(req, resp, ex, params)

falcon/errors.py

@@ -38,7 +38,7 @@ def on_get(self, req, resp):
 from datetime import datetime
 
 from falcon import util
-from falcon.http_error import HTTPError, OptionalRepresentation
+from falcon.http_error import HTTPError
 import falcon.status_codes as status
 
 
@@ -268,7 +268,7 @@ def __init__(self, title=None, description=None, headers=None, **kwargs):
         )
 
 
-class HTTPNotFound(OptionalRepresentation, HTTPError):
+class HTTPNotFound(HTTPError):
     """404 Not Found.
 
     The origin server did not find a current representation for the
@@ -381,7 +381,7 @@ class HTTPRouteNotFound(HTTPNotFound):
     """
 
 
-class HTTPMethodNotAllowed(OptionalRepresentation, HTTPError):
+class HTTPMethodNotAllowed(HTTPError):
     """405 Method Not Allowed.
 
     The method received in the request-line is known by the origin
@@ -585,7 +585,7 @@ def __init__(self, title=None, description=None, headers=None, **kwargs):
         )
 
 
-class HTTPGone(OptionalRepresentation, HTTPError):
+class HTTPGone(HTTPError):
     """410 Gone.
 
     The target resource is no longer available at the origin server and
@@ -967,7 +967,7 @@ def __init__(self, title=None, description=None, headers=None, **kwargs):
         )
 
 
-class HTTPRangeNotSatisfiable(OptionalRepresentation, HTTPError):
+class HTTPRangeNotSatisfiable(HTTPError):
     """416 Range Not Satisfiable.
 
     None of the ranges in the request's Range header field overlap the
@@ -1103,7 +1103,7 @@ def __init__(self, title=None, description=None, headers=None, **kwargs):
         )
 
 
-class HTTPLocked(OptionalRepresentation, HTTPError):
+class HTTPLocked(HTTPError):
     """423 Locked.
 
     The 423 (Locked) status code means the source or destination resource
@@ -1159,7 +1159,7 @@ def __init__(self, title=None, description=None, headers=None, **kwargs):
         )
 
 
-class HTTPFailedDependency(OptionalRepresentation, HTTPError):
+class HTTPFailedDependency(HTTPError):
     """424 Failed Dependency.
 
     The 424 (Failed Dependency) status code means that the method could
@@ -1407,7 +1407,7 @@ def __init__(self, title=None, description=None, headers=None, **kwargs):
         )
 
 
-class HTTPUnavailableForLegalReasons(OptionalRepresentation, HTTPError):
+class HTTPUnavailableForLegalReasons(HTTPError):
     """451 Unavailable For Legal Reasons.
 
     The server is denying access to the resource as a consequence of a

falcon/http_error.py

@@ -17,7 +17,7 @@
 from collections import OrderedDict
 import xml.etree.ElementTree as et
 
-from falcon.util import deprecated_args, json, uri
+from falcon.util import deprecated, deprecated_args, json, uri
 
 
 class HTTPError(Exception):
@@ -79,19 +79,6 @@ class HTTPError(Exception):
 
     Attributes:
         status (str): HTTP status line, e.g. '748 Confounded by Ponies'.
-        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 it
-            in order to return ``False`` when an empty HTTP body is desired.
-
-            (See also: :class:`falcon.http_error.OptionalRepresentation`)
-
-            Note:
-                A custom error serializer
-                (see :meth:`~.App.set_error_serializer`) may choose to set a
-                response body regardless of the value of this property.
-
         title (str): Error title to send to the client.
         description (str): Description of the error to send to the client.
         headers (dict): Extra headers to add to the response.
@@ -136,7 +123,11 @@ def __init__(self, status, title=None, description=None, headers=None,
     def __repr__(self):
         return '<%s: %s>' % (self.__class__.__name__, self.status)
 
-    @property
+    @property  # type: ignore
+    @deprecated(
+        'has_representation is deprecated and is currently unused by falcon',
+        is_property=True
+    )
     def has_representation(self):
         return True
 
@@ -186,7 +177,7 @@ def to_xml(self):
         """Return an XML-encoded representation of the error.
 
         Returns:
-            str: An XML document for the error.
+            bytes: An XML document for the error.
 
         """
 
@@ -210,6 +201,39 @@ def to_xml(self):
                 et.tostring(error_element, encoding='utf-8'))
 
 
+class NoRepresentation:
+    """Mixin for ``HTTPError`` child classes that have no representation.
+
+    This class can be mixed in when inheriting from ``HTTPError``, in order
+    to override the `has_representation` property such that it always
+    returns ``False``. This, in turn, will cause Falcon to return an empty
+    response body to the client.
+
+    You can use this mixin when defining errors that either should not have
+    a body (as dictated by HTTP standards or common practice), or in the
+    case that a detailed error response may leak information to an attacker.
+
+    Note:
+        This mixin class must appear before ``HTTPError`` in the base class
+        list when defining the child; otherwise, it will not override the
+        `has_representation` property as expected.
+
+    Warning:
+        As of Falcon 3.0, this mixin class is no longer used since all Falcon
+        errors have a representation. This class is considered deprecated and
+        will be removed in a future release.
+    """
+
+    @property  # type: ignore
+    @deprecated(
+        'has_representation is deprecated and is currently unused by falcon. '
+        'The class NoRepresentation is deprecated and will be removed in a future release',
+        is_property=True
+    )
+    def has_representation(self):
+        return False
+
+
 class OptionalRepresentation:
     """Mixin for ``HTTPError`` child classes that may have a representation.
 
@@ -227,7 +251,17 @@ class OptionalRepresentation:
         list when defining the child; otherwise, it will not override the
         `has_representation` property as expected.
 
+    Warning:
+        As of Falcon 3.0, this mixin class is no longer used since all Falcon
+        errors have a representation. This class is considered deprecated and
+        will be removed in a future release.
     """
-    @property
+
+    @property  # type: ignore
+    @deprecated(
+        'has_representation is deprecated and is currently unused by falcon. '
+        'The class OptionalRepresentation is deprecated and will be removed in a future release',
+        is_property=True
+    )
     def has_representation(self):
-        return super().description is not None
+        return self.description is not None

falcon/util/misc.py

@@ -28,7 +28,6 @@
 import functools
 import http
 import inspect
-import os
 import re
 import sys
 import unicodedata
@@ -120,7 +119,7 @@ class DeprecatedWarning(UserWarning):
     pass
 
 
-def deprecated(instructions):
+def deprecated(instructions, is_property=False):
     """Flags a method as deprecated.
 
     This function returns a decorator which can be used to mark deprecated
@@ -129,23 +128,26 @@ def deprecated(instructions):
 
     Args:
         instructions (str): Specific guidance for the developer, e.g.:
-            'Please migrate to add_proxy(...)''
+            'Please migrate to add_proxy(...)'
+        is_property (bool): If the deprecated object is a property. It
+            will omit the ``(...)`` from the generated documentation
     """
 
     def decorator(func):
+
+        object_name = 'property' if is_property else 'function'
+        post_name = '' if is_property else '(...)'
+        message = 'Call to deprecated {} {}{}. {}'.format(
+            object_name, func.__name__, post_name, instructions)
+
         @functools.wraps(func)
         def wrapper(*args, **kwargs):
-            if 'FALCON_TESTING_SESSION' not in os.environ:
-                message = 'Call to deprecated function {0}(...). {1}'.format(
-                    func.__name__,
-                    instructions)
-
-                frame = inspect.currentframe().f_back
+            frame = inspect.currentframe().f_back
 
-                warnings.warn_explicit(message,
-                                       category=DeprecatedWarning,
-                                       filename=inspect.getfile(frame.f_code),
-                                       lineno=frame.f_lineno)
+            warnings.warn_explicit(message,
+                                   category=DeprecatedWarning,
+                                   filename=inspect.getfile(frame.f_code),
+                                   lineno=frame.f_lineno)
 
             return func(*args, **kwargs)
 

tests/test_custom_router.py

@@ -68,7 +68,7 @@ def find(self, uri, req=None):
 
     for uri in ('/404', '/404/backwards-compat'):
         response = client.simulate_request(path=uri)
-        assert not response.content
+        assert response.text == falcon.HTTPNotFound().to_json()
         assert response.status == falcon.HTTP_404
 
     assert router.reached_backwards_compat