Merge pull request #442 from dinooo13/redirectMethod

HTTP client: Preserve request method and body for `307 Temporary Redirect` and `308 Permanent Redirect`

README.md

@@ -342,9 +342,10 @@ $browser->get($url, $headers)->then(function (Psr\Http\Message\ResponseInterface
 Any redirected requests will follow the semantics of the original request and
 will include the same request headers as the original request except for those
 listed below.
-If the original request contained a request body, this request body will never
-be passed to the redirected request. Accordingly, each redirected request will
-remove any `Content-Length` and `Content-Type` request headers.
+If the original request is a temporary (307) or a permanent (308) redirect, request
+body and headers will be passed to the redirected request. Otherwise, the request 
+body will never be passed to the redirected request. Accordingly, each redirected 
+request will remove any `Content-Length` and `Content-Type` request headers.
 
 If the original request used HTTP authentication with an `Authorization` request
 header, this request header will only be passed as part of the redirected

src/Io/Transaction.php

@@ -5,6 +5,7 @@
 use Psr\Http\Message\RequestInterface;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\UriInterface;
+use React\Http\Message\Response;
 use RingCentral\Psr7\Request;
 use RingCentral\Psr7\Uri;
 use React\EventLoop\LoopInterface;
@@ -234,6 +235,8 @@ public function onResponse(ResponseInterface $response, RequestInterface $reques
     /**
      * @param ResponseInterface $response
      * @param RequestInterface $request
+     * @param Deferred $deferred
+     * @param ClientRequestState $state
      * @return PromiseInterface
      * @throws \RuntimeException
      */
@@ -242,7 +245,7 @@ private function onResponseRedirect(ResponseInterface $response, RequestInterfac
         // resolve location relative to last request URI
         $location = Uri::resolve($request->getUri(), $response->getHeaderLine('Location'));
 
-        $request = $this->makeRedirectRequest($request, $location);
+        $request = $this->makeRedirectRequest($request, $location, $response->getStatusCode());
         $this->progress('redirect', array($request));
 
         if ($state->numRequests >= $this->maxRedirects) {
@@ -255,25 +258,33 @@ private function onResponseRedirect(ResponseInterface $response, RequestInterfac
     /**
      * @param RequestInterface $request
      * @param UriInterface $location
+     * @param int $statusCode
      * @return RequestInterface
+     * @throws \RuntimeException
      */
-    private function makeRedirectRequest(RequestInterface $request, UriInterface $location)
+    private function makeRedirectRequest(RequestInterface $request, UriInterface $location, $statusCode)
     {
-        $originalHost = $request->getUri()->getHost();
-        $request = $request
-            ->withoutHeader('Host')
-            ->withoutHeader('Content-Type')
-            ->withoutHeader('Content-Length');
-
         // Remove authorization if changing hostnames (but not if just changing ports or protocols).
+        $originalHost = $request->getUri()->getHost();
         if ($location->getHost() !== $originalHost) {
             $request = $request->withoutHeader('Authorization');
         }
 
-        // naïve approach..
-        $method = ($request->getMethod() === 'HEAD') ? 'HEAD' : 'GET';
+        $request = $request->withoutHeader('Host')->withUri($location);
+
+        if ($statusCode === Response::STATUS_TEMPORARY_REDIRECT || $statusCode === Response::STATUS_PERMANENT_REDIRECT) {
+            if ($request->getBody() instanceof ReadableStreamInterface) {
+                throw new \RuntimeException('Unable to redirect request with streaming body');
+            }
+        } else {
+            $request = $request
+                ->withMethod($request->getMethod() === 'HEAD' ? 'HEAD' : 'GET')
+                ->withoutHeader('Content-Type')
+                ->withoutHeader('Content-Length')
+                ->withBody(new EmptyBodyStream());
+        }
 
-        return new Request($method, $location, $request->getHeaders());
+        return $request;
     }
 
     private function progress($name, array $args = array())

tests/Io/TransactionTest.php

@@ -663,7 +663,7 @@ public function testSomeRequestHeadersShouldBeRemovedWhenRedirecting()
             array($this->callback(function (RequestInterface $request) use ($that) {
                 $that->assertFalse($request->hasHeader('Content-Type'));
                 $that->assertFalse($request->hasHeader('Content-Length'));
-                return true;;
+                return true;
             }))
         )->willReturnOnConsecutiveCalls(
             Promise\resolve($redirectResponse),
@@ -674,6 +674,122 @@ public function testSomeRequestHeadersShouldBeRemovedWhenRedirecting()
         $transaction->send($requestWithCustomHeaders);
     }
 
+    public function testRequestMethodShouldBeChangedWhenRedirectingWithSeeOther()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $customHeaders = array(
+            'Content-Type' => 'text/html; charset=utf-8',
+            'Content-Length' => '111',
+        );
+
+        $request = new Request('POST', 'http://example.com', $customHeaders);
+        $sender = $this->makeSenderMock();
+
+        // mock sender to resolve promise with the given $redirectResponse in
+        // response to the given $request
+        $redirectResponse = new Response(303, array('Location' => 'http://example.com/new'));
+
+        // mock sender to resolve promise with the given $okResponse in
+        // response to the given $request
+        $okResponse = new Response(200);
+        $that = $this;
+        $sender->expects($this->exactly(2))->method('send')->withConsecutive(
+            array($this->anything()),
+            array($this->callback(function (RequestInterface $request) use ($that) {
+                $that->assertEquals('GET', $request->getMethod());
+                $that->assertFalse($request->hasHeader('Content-Type'));
+                $that->assertFalse($request->hasHeader('Content-Length'));
+                return true;
+            }))
+        )->willReturnOnConsecutiveCalls(
+            Promise\resolve($redirectResponse),
+            Promise\resolve($okResponse)
+        );
+
+        $transaction = new Transaction($sender, $loop);
+        $transaction->send($request);
+    }
+
+    public function testRequestMethodAndBodyShouldNotBeChangedWhenRedirectingWith307Or308()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $customHeaders = array(
+            'Content-Type' => 'text/html; charset=utf-8',
+            'Content-Length' => '111',
+        );
+
+        $request = new Request('POST', 'http://example.com', $customHeaders, '{"key":"value"}');
+        $sender = $this->makeSenderMock();
+
+        // mock sender to resolve promise with the given $redirectResponse in
+        // response to the given $request
+        $redirectResponse = new Response(307, array('Location' => 'http://example.com/new'));
+
+        // mock sender to resolve promise with the given $okResponse in
+        // response to the given $request
+        $okResponse = new Response(200);
+        $that = $this;
+        $sender->expects($this->exactly(2))->method('send')->withConsecutive(
+            array($this->anything()),
+            array($this->callback(function (RequestInterface $request) use ($that) {
+                $that->assertEquals('POST', $request->getMethod());
+                $that->assertEquals('{"key":"value"}', (string)$request->getBody());
+                $that->assertEquals(
+                    array(
+                        'Content-Type' => array('text/html; charset=utf-8'),
+                        'Content-Length' => array('111'),
+                        'Host' => array('example.com')
+                    ),
+                    $request->getHeaders()
+                );
+                return true;
+            }))
+        )->willReturnOnConsecutiveCalls(
+            Promise\resolve($redirectResponse),
+            Promise\resolve($okResponse)
+        );
+
+        $transaction = new Transaction($sender, $loop);
+        $transaction->send($request);
+    }
+
+    public function testRedirectingStreamingBodyWith307Or308ShouldThrowCantRedirectStreamException()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $customHeaders = array(
+            'Content-Type' => 'text/html; charset=utf-8',
+            'Content-Length' => '111',
+        );
+
+        $stream = new ThroughStream();
+        $request = new Request('POST', 'http://example.com', $customHeaders, new ReadableBodyStream($stream));
+        $sender = $this->makeSenderMock();
+
+        // mock sender to resolve promise with the given $redirectResponse in
+        // response to the given $request
+        $redirectResponse = new Response(307, array('Location' => 'http://example.com/new'));
+
+        $sender->expects($this->once())->method('send')->withConsecutive(
+            array($this->anything())
+        )->willReturnOnConsecutiveCalls(
+            Promise\resolve($redirectResponse)
+        );
+
+        $transaction = new Transaction($sender, $loop);
+        $promise = $transaction->send($request);
+
+        $exception = null;
+        $promise->then(null, function ($reason) use (&$exception) {
+            $exception = $reason;
+        });
+
+        assert($exception instanceof \RuntimeException);
+        $this->assertEquals('Unable to redirect request with streaming body', $exception->getMessage());
+    }
+
     public function testCancelTransactionWillCancelRequest()
     {
         $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();