Preserve method on redirect

Author: dinooo13

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,32 @@ 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';
+        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())
@@ -295,4 +305,5 @@ private function progress($name, array $args = array())
 
         echo PHP_EOL;
     }
+
 }

tests/Io/TransactionTest.php

@@ -671,6 +671,112 @@ 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());
+                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());
+                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();