Repository: falconry/falcon

Issue:
Middleware post process error handling
When middleware is added to the api and we have a post_process on it, any exceptions thrown in the responder are not aggregated to an appropriate response. Instead, they are ignored and the post_process attempts to run on as though nothing happened.

It does not matter whether an exception handler is added to the api or not.

Let me know if I just misunderstood how to handle these

Discussion:
codecov-io: ## [Current coverage](https://codecov.io/gh/falconry/falcon/pull/890?src=pr) is 100% (diff: 100%)
> Merging [#890](https://codecov.io/gh/falconry/falcon/pull/890?src=pr) into [master](https://codecov.io/gh/falconry/falcon/branch/master?src=pr) will not change coverage

```diff
@@           master   #890   diff @@
====================================
  Files          30     30          
  Lines        1897   1910    +13   
  Methods         0      0          
  Messages        0      0          
  Branches      314    316     +2   
====================================
+ Hits         1897   1910    +13   
  Misses          0      0          
  Partials        0      0          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last update [d9d1aed...bdae40c](https://codecov.io/gh/falconry/falcon/compare/d9d1aed01ef5e85d3164bd0e6b83f52293647c58...bdae40c242f2e4949a591973e09d7cf1a78033f8?src=pr)
jmvrbanac: LGTM :+1: 
jmvrbanac: :+1: 

Patch:
diff --git a/docs/api/middleware.rst b/docs/api/middleware.rst
index f947b7f..b6e9036 100644
--- a/docs/api/middleware.rst
+++ b/docs/api/middleware.rst
@@ -47,7 +47,7 @@ Falcon's middleware interface is defined as follows:
                     method as keyword arguments.
             """
 
-        def process_response(self, req, resp, resource):
+        def process_response(self, req, resp, resource, req_succeeded):
             """Post-processing of the response (after routing).
 
             Args:
@@ -56,6 +56,9 @@ Falcon's middleware interface is defined as follows:
                 resource: Resource object to which the request was
                     routed. May be None if no route was found
                     for the request.
+                req_succeeded: True if no exceptions were raised while
+                    the framework processed and routed the request;
+                    otherwise False.
             """
 
 .. Tip::
diff --git a/falcon/api.py b/falcon/api.py
index 853abb1..e1e985a 100644
--- a/falcon/api.py
+++ b/falcon/api.py
@@ -73,7 +73,7 @@ class API(object):
                                 arguments.
                         \"\"\"
 
-                    def process_response(self, req, resp, resource)
+                    def process_response(self, req, resp, resource, req_succeeded)
                         \"\"\"Post-processing of the response (after routing).
 
                         Args:
@@ -82,6 +82,9 @@ class API(object):
                             resource: Resource object to which the request was
                                 routed. May be None if no route was found
                                 for the request.
+                            req_succeeded: True if no exceptions were raised
+                                while the framework processed and routed the
+                                request; otherwise False.
                         \"\"\"
 
             See also :ref:`Middleware <middleware>`.
@@ -161,9 +164,11 @@ class API(object):
         req = self._request_type(env, options=self.req_options)
         resp = self._response_type()
         resource = None
-        mw_pr_stack = []  # Keep track of executed middleware components
         params = {}
 
+        mw_pr_stack = []  # Keep track of executed middleware components
+        req_succeeded = False
+
         try:
             try:
                 # NOTE(ealogar): The execution of request middleware
@@ -202,6 +207,7 @@ class API(object):
                                 process_resource(req, resp, resource, params)
 
                     responder(req, resp, **params)
+                    req_succeeded = True
                 except Exception as ex:
                     if not self._handle_exception(ex, req, resp, params):
                         raise
@@ -217,11 +223,13 @@ class API(object):
             while mw_pr_stack:
                 process_response = mw_pr_stack.pop()
                 try:
-                    process_response(req, resp, resource)
+                    process_response(req, resp, resource, req_succeeded)
                 except Exception as ex:
                     if not self._handle_exception(ex, req, resp, params):
                         raise
 
+                    req_succeeded = False
+
         #
         # Set status and headers
         #
diff --git a/falcon/api_helpers.py b/falcon/api_helpers.py
index 9f91608..04a5979 100644
--- a/falcon/api_helpers.py
+++ b/falcon/api_helpers.py
@@ -14,6 +14,9 @@
 
 """Utilities for the API class."""
 
+from functools import wraps
+import inspect
+
 from falcon import util
 
 
@@ -49,6 +52,21 @@ def prepare_middleware(middleware=None):
             msg = '{0} does not implement the middleware interface'
             raise TypeError(msg.format(component))
 
+        if process_response:
+            # NOTE(kgriffs): Shim older implementations to ensure
+            # backwards-compatibility.
+            spec = inspect.getargspec(process_response)
+
+            if len(spec.args) == 4:  # (self, req, resp, resource)
+                def let(process_response=process_response):
+                    @wraps(process_response)
+                    def shim(req, resp, resource, req_succeeded):
+                        process_response(req, resp, resource)
+
+                    return shim
+
+                process_response = let()
+
         prepared_middleware.append((process_request, process_resource,
                                     process_response))
 


Test patch:
diff --git a/tests/test_middleware.py b/tests/test_middleware.py
index 652306f..9d48c4c 100644
--- a/tests/test_middleware.py
+++ b/tests/test_middleware.py
@@ -11,8 +11,11 @@ context = {'executed_methods': []}
 
 class CaptureResponseMiddleware(object):
 
-    def process_response(self, req, resp, resource):
+    def process_response(self, req, resp, resource, req_succeeded):
+        self.req = req
         self.resp = resp
+        self.resource = resource
+        self.req_succeeded = req_succeeded
 
 
 class RequestTimeMiddleware(object):
@@ -25,9 +28,10 @@ class RequestTimeMiddleware(object):
         global context
         context['mid_time'] = datetime.utcnow()
 
-    def process_response(self, req, resp, resource):
+    def process_response(self, req, resp, resource, req_succeeded):
         global context
         context['end_time'] = datetime.utcnow()
+        context['req_succeeded'] = req_succeeded
 
 
 class TransactionIdMiddleware(object):
@@ -36,6 +40,9 @@ class TransactionIdMiddleware(object):
         global context
         context['transaction_id'] = 'unique-req-id'
 
+    def process_response(self, req, resp, resource):
+        pass
+
 
 class ExecutedFirstMiddleware(object):
 
@@ -49,11 +56,18 @@ class ExecutedFirstMiddleware(object):
         context['executed_methods'].append(
             '{0}.{1}'.format(self.__class__.__name__, 'process_resource'))
 
+    # NOTE(kgriffs): This also tests that the framework can continue to
+    # call process_response() methods that do not have a 'req_succeeded'
+    # arg.
     def process_response(self, req, resp, resource):
         global context
         context['executed_methods'].append(
             '{0}.{1}'.format(self.__class__.__name__, 'process_response'))
 
+        context['req'] = req
+        context['resp'] = resp
+        context['resource'] = resource
+
 
 class ExecutedLastMiddleware(ExecutedFirstMiddleware):
     pass
@@ -110,6 +124,7 @@ class TestRequestTimeMiddleware(TestMiddleware):
         self.assertIn('start_time', context)
         self.assertNotIn('mid_time', context)
         self.assertIn('end_time', context)
+        self.assertFalse(context['req_succeeded'])
 
     def test_add_invalid_middleware(self):
         """Test than an invalid class can not be added as middleware"""
@@ -147,6 +162,7 @@ class TestRequestTimeMiddleware(TestMiddleware):
         body = self.simulate_json_request(self.test_route)
         self.assertEqual(_EXPECTED_BODY, body)
         self.assertEqual(self.srmock.status, falcon.HTTP_200)
+
         self.assertIn('start_time', context)
         self.assertIn('mid_time', context)
         self.assertIn('end_time', context)
@@ -155,6 +171,8 @@ class TestRequestTimeMiddleware(TestMiddleware):
         self.assertTrue(context['end_time'] >= context['start_time'],
                         'process_response not executed after request')
 
+        self.assertTrue(context['req_succeeded'])
+
 
 class TestTransactionIdMiddleware(TestMiddleware):
 
@@ -194,6 +212,16 @@ class TestSeveralMiddlewares(TestMiddleware):
         self.assertTrue(context['end_time'] >= context['start_time'],
                         'process_response not executed after request')
 
+    def test_legacy_middleware_called_with_correct_args(self):
+        global context
+        self.api = falcon.API(middleware=[ExecutedFirstMiddleware()])
+        self.api.add_route(self.test_route, MiddlewareClassResource())
+
+        self.simulate_request(self.test_route)
+        self.assertIsInstance(context['req'], falcon.Request)
+        self.assertIsInstance(context['resp'], falcon.Response)
+        self.assertIsInstance(context['resource'], MiddlewareClassResource)
+
     def test_middleware_execution_order(self):
         global context
         self.api = falcon.API(middleware=[ExecutedFirstMiddleware(),
@@ -220,6 +248,8 @@ class TestSeveralMiddlewares(TestMiddleware):
         """Test that error in inner middleware leaves"""
         global context
 
+        context['req_succeeded'] = []
+
         class RaiseStatusMiddleware(object):
             def process_response(self, req, resp, resource):
                 raise falcon.HTTPStatus(falcon.HTTP_201)
@@ -229,10 +259,12 @@ class TestSeveralMiddlewares(TestMiddleware):
                 raise falcon.HTTPError(falcon.HTTP_748)
 
         class ProcessResponseMiddleware(object):
-            def process_response(self, req, resp, resource):
+            def process_response(self, req, resp, resource, req_succeeded):
                 context['executed_methods'].append('process_response')
+                context['req_succeeded'].append(req_succeeded)
 
-        self.api = falcon.API(middleware=[RaiseErrorMiddleware(),
+        self.api = falcon.API(middleware=[ProcessResponseMiddleware(),
+                                          RaiseErrorMiddleware(),
                                           ProcessResponseMiddleware(),
                                           RaiseStatusMiddleware(),
                                           ProcessResponseMiddleware()])
@@ -242,8 +274,9 @@ class TestSeveralMiddlewares(TestMiddleware):
 
         self.assertEqual(self.srmock.status, falcon.HTTP_748)
 
-        expected_methods = ['process_response', 'process_response']
+        expected_methods = ['process_response'] * 3
         self.assertEqual(context['executed_methods'], expected_methods)
+        self.assertEqual(context['req_succeeded'], [True, False, False])
 
     def test_inner_mw_throw_exception(self):
         """Test that error in inner middleware leaves"""
@@ -475,6 +508,13 @@ class TestErrorHandling(TestMiddleware):
         composed_body = json.loads(self.mw.resp.body)
         self.assertEqual(composed_body['title'], self.srmock.status)
 
+        self.assertFalse(self.mw.req_succeeded)
+
+        # NOTE(kgriffs): Sanity-check the other params passed to
+        # process_response()
+        self.assertIsInstance(self.mw.req, falcon.Request)
+        self.assertIsInstance(self.mw.resource, MiddlewareClassResource)
+
     def test_http_status_raised_from_error_handler(self):
         def _http_error_handler(error, req, resp, params):
             raise falcon.HTTPStatus(falcon.HTTP_201)