Skip to content

Commit 6d8ad82

Browse files
dinooo13dinooo13
committed
Preserve method on redirect
1 parent 14e9c6b commit 6d8ad82

File tree

2 files changed

+139
-12
lines changed

2 files changed

+139
-12
lines changed

‎src/Io/Transaction.php‎

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use Psr\Http\Message\RequestInterface;
66
use Psr\Http\Message\ResponseInterface;
77
use Psr\Http\Message\UriInterface;
8+
use React\Http\Message\Response;
89
use RingCentral\Psr7\Request;
910
use RingCentral\Psr7\Uri;
1011
use React\EventLoop\LoopInterface;
@@ -234,6 +235,8 @@ public function onResponse(ResponseInterface $response, RequestInterface $reques
234235
/**
235236
* @param ResponseInterface $response
236237
* @param RequestInterface $request
238+
* @param Deferred $deferred
239+
* @param ClientRequestState $state
237240
* @return PromiseInterface
238241
* @throws \RuntimeException
239242
*/
@@ -242,7 +245,7 @@ private function onResponseRedirect(ResponseInterface $response, RequestInterfac
242245
// resolve location relative to last request URI
243246
$location = Uri::resolve($request->getUri(), $response->getHeaderLine('Location'));
244247

245-
$request = $this->makeRedirectRequest($request, $location);
248+
$request = $this->makeRedirectRequest($request, $location, $response->getStatusCode());
246249
$this->progress('redirect', array($request));
247250

248251
if ($state->numRequests >= $this->maxRedirects) {
@@ -255,25 +258,33 @@ private function onResponseRedirect(ResponseInterface $response, RequestInterfac
255258
/**
256259
* @param RequestInterface $request
257260
* @param UriInterface $location
261+
* @param int $statusCode
258262
* @return RequestInterface
263+
* @throws \RuntimeException
259264
*/
260-
private function makeRedirectRequest(RequestInterface $request, UriInterface $location)
265+
private function makeRedirectRequest(RequestInterface $request, UriInterface $location, $statusCode)
261266
{
262-
$originalHost = $request->getUri()->getHost();
263-
$request = $request
264-
->withoutHeader('Host')
265-
->withoutHeader('Content-Type')
266-
->withoutHeader('Content-Length');
267-
268267
// Remove authorization if changing hostnames (but not if just changing ports or protocols).
268+
$originalHost = $request->getUri()->getHost();
269269
if ($location->getHost() !== $originalHost) {
270270
$request = $request->withoutHeader('Authorization');
271271
}
272272

273-
// naïve approach..
274-
$method = ($request->getMethod() === 'HEAD') ? 'HEAD' : 'GET';
273+
$request = $request->withoutHeader('Host')->withUri($location);
274+
275+
if ($statusCode === Response::STATUS_TEMPORARY_REDIRECT || $statusCode === Response::STATUS_PERMANENT_REDIRECT) {
276+
if ($request->getBody() instanceof ReadableStreamInterface) {
277+
throw new \RuntimeException('Unable to redirect request with streaming body');
278+
}
279+
} else {
280+
$request = $request
281+
->withMethod($request->getMethod() === 'HEAD' ? 'HEAD' : 'GET')
282+
->withoutHeader('Content-Type')
283+
->withoutHeader('Content-Length')
284+
->withBody(new EmptyBodyStream());
285+
}
275286

276-
return new Request($method, $location, $request->getHeaders());
287+
return $request;
277288
}
278289

279290
private function progress($name, array $args = array())

‎tests/Io/TransactionTest.php‎

Lines changed: 117 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -663,7 +663,7 @@ public function testSomeRequestHeadersShouldBeRemovedWhenRedirecting()
663663
array($this->callback(function (RequestInterface $request) use ($that) {
664664
$that->assertFalse($request->hasHeader('Content-Type'));
665665
$that->assertFalse($request->hasHeader('Content-Length'));
666-
return true;;
666+
return true;
667667
}))
668668
)->willReturnOnConsecutiveCalls(
669669
Promise\resolve($redirectResponse),
@@ -674,6 +674,122 @@ public function testSomeRequestHeadersShouldBeRemovedWhenRedirecting()
674674
$transaction->send($requestWithCustomHeaders);
675675
}
676676

677+
public function testRequestMethodShouldBeChangedWhenRedirectingWithSeeOther()
678+
{
679+
$loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
680+
681+
$customHeaders = array(
682+
'Content-Type' => 'text/html; charset=utf-8',
683+
'Content-Length' => '111',
684+
);
685+
686+
$request = new Request('POST', 'http://example.com', $customHeaders);
687+
$sender = $this->makeSenderMock();
688+
689+
// mock sender to resolve promise with the given $redirectResponse in
690+
// response to the given $request
691+
$redirectResponse = new Response(303, array('Location' => 'http://example.com/new'));
692+
693+
// mock sender to resolve promise with the given $okResponse in
694+
// response to the given $request
695+
$okResponse = new Response(200);
696+
$that = $this;
697+
$sender->expects($this->exactly(2))->method('send')->withConsecutive(
698+
array($this->anything()),
699+
array($this->callback(function (RequestInterface $request) use ($that) {
700+
$that->assertEquals('GET', $request->getMethod());
701+
$that->assertFalse($request->hasHeader('Content-Type'));
702+
$that->assertFalse($request->hasHeader('Content-Length'));
703+
return true;
704+
}))
705+
)->willReturnOnConsecutiveCalls(
706+
Promise\resolve($redirectResponse),
707+
Promise\resolve($okResponse)
708+
);
709+
710+
$transaction = new Transaction($sender, $loop);
711+
$transaction->send($request);
712+
}
713+
714+
public function testRequestMethodAndBodyShouldNotBeChangedWhenRedirectingWith307Or308()
715+
{
716+
$loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
717+
718+
$customHeaders = array(
719+
'Content-Type' => 'text/html; charset=utf-8',
720+
'Content-Length' => '111',
721+
);
722+
723+
$request = new Request('POST', 'http://example.com', $customHeaders, '{"key":"value"}');
724+
$sender = $this->makeSenderMock();
725+
726+
// mock sender to resolve promise with the given $redirectResponse in
727+
// response to the given $request
728+
$redirectResponse = new Response(307, array('Location' => 'http://example.com/new'));
729+
730+
// mock sender to resolve promise with the given $okResponse in
731+
// response to the given $request
732+
$okResponse = new Response(200);
733+
$that = $this;
734+
$sender->expects($this->exactly(2))->method('send')->withConsecutive(
735+
array($this->anything()),
736+
array($this->callback(function (RequestInterface $request) use ($that) {
737+
$that->assertEquals('POST', $request->getMethod());
738+
$that->assertEquals('{"key":"value"}', (string)$request->getBody());
739+
$that->assertEquals(
740+
array(
741+
'Content-Type' => array('text/html; charset=utf-8'),
742+
'Content-Length' => array('111'),
743+
'Host' => array('example.com')
744+
),
745+
$request->getHeaders()
746+
);
747+
return true;
748+
}))
749+
)->willReturnOnConsecutiveCalls(
750+
Promise\resolve($redirectResponse),
751+
Promise\resolve($okResponse)
752+
);
753+
754+
$transaction = new Transaction($sender, $loop);
755+
$transaction->send($request);
756+
}
757+
758+
public function testRedirectingStreamingBodyWith307Or308ShouldThrowCantRedirectStreamException()
759+
{
760+
$loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
761+
762+
$customHeaders = array(
763+
'Content-Type' => 'text/html; charset=utf-8',
764+
'Content-Length' => '111',
765+
);
766+
767+
$stream = new ThroughStream();
768+
$request = new Request('POST', 'http://example.com', $customHeaders, new ReadableBodyStream($stream));
769+
$sender = $this->makeSenderMock();
770+
771+
// mock sender to resolve promise with the given $redirectResponse in
772+
// response to the given $request
773+
$redirectResponse = new Response(307, array('Location' => 'http://example.com/new'));
774+
775+
$sender->expects($this->once())->method('send')->withConsecutive(
776+
array($this->anything())
777+
)->willReturnOnConsecutiveCalls(
778+
Promise\resolve($redirectResponse)
779+
);
780+
781+
$transaction = new Transaction($sender, $loop);
782+
$promise = $transaction->send($request);
783+
784+
$exception = null;
785+
$promise->then(null, function ($reason) use (&$exception) {
786+
$exception = $reason;
787+
});
788+
789+
assert($exception instanceof \RuntimeException);
790+
$this->assertEquals('Unable to redirect request with streaming body', $exception->getMessage());
791+
}
792+
677793
public function testCancelTransactionWillCancelRequest()
678794
{
679795
$loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();

0 commit comments

Comments
 (0)