Merge pull request #294 from clue-labs/happy-middleware

Avoid promise wrapping for middleware next request handlers and add documentation for consuming response from next request handler

Author: jsor

README.md

@@ -798,10 +798,27 @@ As such, this project supports the concept of middleware request handlers.
 
 A middleware request handler is expected to adhere the following rules:
 
-* It is a `callable`.
-* It accepts `ServerRequestInterface` as first argument and optional `callable` as second argument.
-* It returns a `ResponseInterface` (or any promise which can be consumed by [`Promise\resolve`](http://reactphp.org/promise/#resolve) resolving to a `ResponseInterface`)
-* It calls `$next($request)` to continue processing the next middleware function or returns explicitly to abort the chain
+* It is a valid `callable`.
+* It accepts `ServerRequestInterface` as first argument and an optional
+  `callable` as second argument.
+* It returns either:
+  * An instance implementing `ResponseInterface` for direct consumption.
+  * Any promise which can be consumed by
+    [`Promise\resolve()`](http://reactphp.org/promise/#resolve) resolving to a
+  `ResponseInterface` for deferred consumption.
+  * It MAY throw an `Exception` (or return a rejected promise) in order to
+    signal an error condition and abort the chain.
+* It calls `$next($request)` to continue processing the next middleware
+  request handler function or returns explicitly without calling `$next` to
+  abort the chain.
+  * The `$next` request handler function (recursively) invokes the next request
+    handler from the chain with the same logic as above and returns (or throws)
+    as above.
+  * The `$request` may be modified prior to calling `$next($request)` to
+    change the incoming request the next middleware operates on.
+  * The `$next` return value may be consumed to modify the outgoing response.
+  * The `$next` request handler MAY be called more than once if you want to
+    implement custom "retry" logic etc.
 
 Note that this very simple definition allows you to use either anonymous
 functions or any classes that use the magic `__invoke()` method.
@@ -828,9 +845,62 @@ $server = new Server(array(
         $request = $request->withHeader('Request-Time', time());
         return $next($request);
     },
+    function (ServerRequestInterface $request) {
+        return new Response(200);
+    }
+));
+```
+
+Similarly, you can use the result of the `$next` middleware request handler
+function to modify the outgoing response.
+Note that as per the above documentation, the `$next` method may return a
+`ResponseInterface` directly or one wrapped in a promise for deferred
+resolution.
+In order to simplify handling both paths, you can simply wrap this in a
+[`Promise\resolve()`](http://reactphp.org/promise/#resolve) call like this:
+
+```php
+$server = new Server(array(
     function (ServerRequestInterface $request, callable $next) {
+        $promise = React\Promise\resolve($next($request));
+        return $promise->then(function (ResponseInterface $response) {
+            return $response->withHeader('Content-Type', 'text/html');
+        });
+    },
+    function (ServerRequestInterface $request) {
         return new Response(200);
+    }
+));
+```
+
+Note that the `$next` middleware request handler function may also throw an
+`Exception` (or return a rejected promise) as described above.
+The previous example does not catch any exceptions and would thus signal an
+error condition to the `Server`.
+Alternatively, you can also catch any `Exception` to implement custom error
+handling logic (or logging etc.) by wrapping this in a
+[`Promise`](http://reactphp.org/promise/#promise) like this:
+
+```php
+$server = new Server(array(
+    function (ServerRequestInterface $request, callable $next) {
+        $promise = new React\Promise\Promise(function ($resolve) use ($next, $request) {
+            $resolve($next($request));
+        });
+        return $promise->then(null, function (Exception $e) {
+            return new Response(
+                500,
+                array(),
+                'Internal error: ' . $e->getMessage()
+            );
+        });
     },
+    function (ServerRequestInterface $request) {
+        if (mt_rand(0, 1) === 1) {
+            throw new RuntimeException('Database error');
+        }
+        return new Response(200);
+    }
 ));
 ```
 

src/Io/MiddlewareRunner.php

@@ -4,7 +4,6 @@
 
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
-use React\Promise;
 use React\Promise\PromiseInterface;
 
 /**
@@ -29,12 +28,13 @@ public function __construct(array $middleware)
 
     /**
      * @param ServerRequestInterface $request
-     * @return PromiseInterface<ResponseInterface>
+     * @return ResponseInterface|PromiseInterface<ResponseInterface>
+     * @throws Exception
      */
     public function __invoke(ServerRequestInterface $request)
     {
         if (count($this->middleware) === 0) {
-            return Promise\reject(new \RuntimeException('No middleware to run'));
+            throw new \RuntimeException('No middleware to run');
         }
 
         return $this->call($request, 0);
@@ -49,14 +49,6 @@ public function call(ServerRequestInterface $request, $position)
         };
 
         $handler = $this->middleware[$position];
-        try {
-            return Promise\resolve($handler($request, $next));
-        } catch (\Exception $error) {
-            // request handler callback throws an Exception
-            return Promise\reject($error);
-        } catch (\Throwable $error) { // @codeCoverageIgnoreStart
-            // request handler callback throws a PHP7+ Error
-            return Promise\reject($error); // @codeCoverageIgnoreEnd
-        }
+        return $handler($request, $next);
     }
 }

tests/Io/MiddlewareRunnerTest.php

@@ -14,23 +14,28 @@
 use RingCentral\Psr7\Response;
 use Psr\Http\Message\RequestInterface;
 use React\Promise\CancellablePromiseInterface;
+use React\Promise\PromiseInterface;
 
 final class MiddlewareRunnerTest extends TestCase
 {
     /**
      * @expectedException RuntimeException
      * @expectedExceptionMessage No middleware to run
      */
-    public function testDefaultResponse()
+    public function testEmptyMiddlewareStackThrowsException()
     {
         $request = new ServerRequest('GET', 'https://example.com/');
         $middlewares = array();
         $middlewareStack = new MiddlewareRunner($middlewares);
 
-        Block\await($middlewareStack($request), Factory::create());
+        $middlewareStack($request);
     }
 
-    public function testReturnsRejectedPromiseIfHandlerThrowsException()
+    /**
+     * @expectedException RuntimeException
+     * @expectedExceptionMessage hello
+     */
+    public function testThrowsIfHandlerThrowsException()
     {
         $middleware = new MiddlewareRunner(array(
             function (ServerRequestInterface $request) {
@@ -40,15 +45,15 @@ function (ServerRequestInterface $request) {
 
         $request = new ServerRequest('GET', 'http://example.com/');
 
-        $promise = $middleware($request);
-
-        $promise->then(null, $this->expectCallableOnceWith($this->isInstanceOf('RuntimeException')));
+        $middleware($request);
     }
 
     /**
      * @requires PHP 7
+     * @expectedException Throwable
+     * @expectedExceptionMessage hello
      */
-    public function testReturnsRejectedPromiseIfHandlerThrowsThrowable()
+    public function testThrowsIfHandlerThrowsThrowable()
     {
         $middleware = new MiddlewareRunner(array(
             function (ServerRequestInterface $request) {
@@ -58,9 +63,7 @@ function (ServerRequestInterface $request) {
 
         $request = new ServerRequest('GET', 'http://example.com/');
 
-        $promise = $middleware($request);
-
-        $promise->then(null, $this->expectCallableOnceWith($this->isInstanceOf('Throwable')));
+        $middleware($request);
     }
 
     public function provideProcessStackMiddlewares()
@@ -126,9 +129,14 @@ public function testProcessStack(array $middlewares, $expectedCallCount)
         $request = new ServerRequest('GET', 'https://example.com/');
         $middlewareStack = new MiddlewareRunner($middlewares);
 
-        /** @var ResponseInterface $result */
-        $result = Block\await($middlewareStack($request), Factory::create());
-        $this->assertSame(200, $result->getStatusCode());
+        $response = $middlewareStack($request);
+
+        $this->assertTrue($response instanceof PromiseInterface);
+        $response = Block\await($response, Factory::create());
+
+        $this->assertTrue($response instanceof ResponseInterface);
+        $this->assertSame(200, $response->getStatusCode());
+
         foreach ($middlewares as $middleware) {
             if (!($middleware instanceof ProcessStack)) {
                 continue;
@@ -163,7 +171,11 @@ public function testNextCanBeRunMoreThanOnceWithoutCorruptingTheMiddlewareStack(
         $retryCalled = 0;
         $error = null;
         $retry = function ($request, $next) use (&$error, &$retryCalled) {
-            return $next($request)->then(null, function ($et) use (&$error, $request, $next, &$retryCalled) {
+            $promise = new \React\Promise\Promise(function ($resolve) use ($request, $next) {
+                $resolve($next($request));
+            });
+
+            return $promise->then(null, function ($et) use (&$error, $request, $next, &$retryCalled) {
                 $retryCalled++;
                 $error = $et;
                 // the $next failed. discard $error and retry once again:
@@ -214,7 +226,7 @@ function (ServerRequestInterface $request, $next) use (&$receivedRequests) {
             },
             function (ServerRequestInterface $request, $next) use (&$receivedRequests) {
                 $receivedRequests[] = 'middleware3: ' . $request->getUri();
-                return $next($request);
+                return new \React\Promise\Promise(function () { });
             }
         ));
 
@@ -372,9 +384,9 @@ public function testUncommonMiddlewareArrayFormats($middlewareFactory, $expected
         $request = new ServerRequest('GET', 'https://example.com/');
         $middlewareStack = new MiddlewareRunner($middlewareFactory());
 
-        /** @var ResponseInterface $response */
-        $response = Block\await($middlewareStack($request), Factory::create());
+        $response = $middlewareStack($request);
 
+        $this->assertTrue($response instanceof ResponseInterface);
         $this->assertSame($expectedSequence, (string) $response->getBody());
     }
 
@@ -397,7 +409,7 @@ function (RequestInterface $request) use (&$called) {
 
         $request = new ServerRequest('GET', 'http://example.com/');
 
-        $response = Block\await($middleware($request), Factory::create());
+        $response = $middleware($request);
 
         $this->assertTrue($response instanceof ResponseInterface);
         $this->assertEquals(2, $called);