From 52d8ffa212c27ced0f482938b423424d03a7e580 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro@alejandrocelaya.com>
Date: Sun, 6 May 2018 12:28:22 +0200
Subject: [PATCH] Improved CreateShortCodeContentNegotiationMiddleware sho that
 it takes into account the case in which an error is returned from next
 middleware

---
 ...eShortCodeContentNegotiationMiddleware.php | 15 ++++-
 ...rtCodeContentNegotiationMiddlewareTest.php | 60 +++++++++++++++----
 2 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/module/Rest/src/Middleware/ShortCode/CreateShortCodeContentNegotiationMiddleware.php b/module/Rest/src/Middleware/ShortCode/CreateShortCodeContentNegotiationMiddleware.php
index 6de16ae7..6a76795e 100644
--- a/module/Rest/src/Middleware/ShortCode/CreateShortCodeContentNegotiationMiddleware.php
+++ b/module/Rest/src/Middleware/ShortCode/CreateShortCodeContentNegotiationMiddleware.php
@@ -23,8 +23,13 @@ class CreateShortCodeContentNegotiationMiddleware implements MiddlewareInterface
      */
     public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
     {
-        /** @var JsonResponse $response */
         $response = $handler->handle($request);
+
+        // If the response is not JSON, return it as is
+        if (! $response instanceof JsonResponse) {
+            return $response;
+        }
+
         $acceptedType = $request->hasHeader('Accept')
             ? $this->determineAcceptTypeFromHeader($request->getHeaderLine('Accept'))
             : $this->determineAcceptTypeFromQuery($request->getQueryParams());
@@ -37,7 +42,7 @@ class CreateShortCodeContentNegotiationMiddleware implements MiddlewareInterface
         // If requested, return a plain text response containing the short URL only
         $resp = (new Response())->withHeader('Content-Type', 'text/plain');
         $body = $resp->getBody();
-        $body->write($response->getPayload()['shortUrl'] ?? '');
+        $body->write($this->determineBody($response));
         $body->rewind();
         return $resp;
     }
@@ -58,4 +63,10 @@ class CreateShortCodeContentNegotiationMiddleware implements MiddlewareInterface
         $accept = \strtolower(\array_shift($accepts));
         return \strpos($accept, 'text/plain') !== false ? self::PLAIN_TEXT : self::JSON;
     }
+
+    private function determineBody(JsonResponse $resp): string
+    {
+        $payload = $resp->getPayload();
+        return $payload['shortUrl'] ?? $payload['error'] ?? '';
+    }
 }
diff --git a/module/Rest/test/Middleware/ShortCode/CreateShortCodeContentNegotiationMiddlewareTest.php b/module/Rest/test/Middleware/ShortCode/CreateShortCodeContentNegotiationMiddlewareTest.php
index b61f06bf..a4feb7d5 100644
--- a/module/Rest/test/Middleware/ShortCode/CreateShortCodeContentNegotiationMiddlewareTest.php
+++ b/module/Rest/test/Middleware/ShortCode/CreateShortCodeContentNegotiationMiddlewareTest.php
@@ -4,10 +4,12 @@ declare(strict_types=1);
 namespace ShlinkioTest\Shlink\Rest\Middleware\ShortCode;
 
 use PHPUnit\Framework\TestCase;
+use Prophecy\Argument;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Server\RequestHandlerInterface;
 use Shlinkio\Shlink\Rest\Middleware\ShortCode\CreateShortCodeContentNegotiationMiddleware;
+use Zend\Diactoros\Response;
 use Zend\Diactoros\Response\JsonResponse;
 use Zend\Diactoros\ServerRequestFactory;
 
@@ -25,15 +27,20 @@ class CreateShortCodeContentNegotiationMiddlewareTest extends TestCase
     public function setUp()
     {
         $this->middleware = new CreateShortCodeContentNegotiationMiddleware();
-        $this->requestHandler = new class implements RequestHandlerInterface {
-            /**
-             * Handle the request and return a response.
-             */
-            public function handle(ServerRequestInterface $request): ResponseInterface
-            {
-                return new JsonResponse(['shortUrl' => 'http://doma.in/foo']);
-            }
-        };
+        $this->requestHandler = $this->prophesize(RequestHandlerInterface::class);
+    }
+
+    /**
+     * @test
+     */
+    public function whenNoJsonResponseIsReturnedNoFurtherOperationsArePerformed()
+    {
+        $expectedResp = new Response();
+        $this->requestHandler->handle(Argument::type(ServerRequestInterface::class))->willReturn($expectedResp);
+
+        $resp = $this->middleware->process(ServerRequestFactory::fromGlobals(), $this->requestHandler->reveal());
+
+        $this->assertSame($expectedResp, $resp);
     }
 
     /**
@@ -48,9 +55,14 @@ class CreateShortCodeContentNegotiationMiddlewareTest extends TestCase
             $request = $request->withHeader('Accept', $accept);
         }
 
-        $response = $this->middleware->process($request, $this->requestHandler);
+        $handle = $this->requestHandler->handle(Argument::type(ServerRequestInterface::class))->willReturn(
+            new JsonResponse(['shortUrl' => 'http://doma.in/foo'])
+        );
+
+        $response = $this->middleware->process($request, $this->requestHandler->reveal());
 
         $this->assertEquals($expectedContentType, $response->getHeaderLine('Content-type'));
+        $handle->shouldHaveBeenCalled();
     }
 
     public function provideData(): array
@@ -65,4 +77,32 @@ class CreateShortCodeContentNegotiationMiddlewareTest extends TestCase
             ['text/plain', [], 'text/plain'],
         ];
     }
+
+    /**
+     * @test
+     * @dataProvider provideTextBodies
+     * @param array $json
+     */
+    public function properBodyIsReturnedInPlainTextResponses(array $json, string $expectedBody)
+    {
+        $request = ServerRequestFactory::fromGlobals()->withQueryParams(['format' => 'txt']);
+
+        $handle = $this->requestHandler->handle(Argument::type(ServerRequestInterface::class))->willReturn(
+            new JsonResponse($json)
+        );
+
+        $response = $this->middleware->process($request, $this->requestHandler->reveal());
+
+        $this->assertEquals($expectedBody, (string) $response->getBody());
+        $handle->shouldHaveBeenCalled();
+    }
+
+    public function provideTextBodies(): array
+    {
+        return [
+            [['shortUrl' => 'foobar'], 'foobar'],
+            [['error' => 'FOO_BAR'], 'FOO_BAR'],
+            [[], ''],
+        ];
+    }
 }