Skip to content

Commit 010a828

Browse files
dinooo13dinooo13
committed
Preserve method on redirect
1 parent b5a66a4 commit 010a828

File tree

2 files changed

+138
-9
lines changed

2 files changed

+138
-9
lines changed

‎src/Io/Transaction.php‎

Lines changed: 37 additions & 9 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,39 @@ 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
{
262267
$originalHost = $request->getUri()->getHost();
263-
$request = $request
264-
->withoutHeader('Host')
265-
->withoutHeader('Content-Type')
266-
->withoutHeader('Content-Length');
268+
$request = $request->withoutHeader('Host');
269+
270+
if (!$this->isTemporaryOrPermanentRedirect($statusCode)) {
271+
$request = $request
272+
->withoutHeader('Content-Type')
273+
->withoutHeader('Content-Length');
274+
}
267275

268276
// Remove authorization if changing hostnames (but not if just changing ports or protocols).
269277
if ($location->getHost() !== $originalHost) {
270278
$request = $request->withoutHeader('Authorization');
271279
}
272280

273-
// naïve approach..
274-
$method = ($request->getMethod() === 'HEAD') ? 'HEAD' : 'GET';
281+
$body = null;
282+
if ($this->isTemporaryOrPermanentRedirect($statusCode)) {
283+
$method = $request->getMethod();
284+
if ($request->getBody() instanceof ReadableStreamInterface) {
285+
throw new \RuntimeException('Unable to redirect request with streaming body');
286+
}
287+
$body = $request->getBody();
288+
} else {
289+
// naïve approach..
290+
$method = ($request->getMethod() === 'HEAD') ? 'HEAD' : 'GET';
291+
}
275292

276-
return new Request($method, $location, $request->getHeaders());
293+
return new Request($method, $location, $request->getHeaders(), $body);
277294
}
278295

279296
private function progress($name, array $args = array())
@@ -295,4 +312,15 @@ private function progress($name, array $args = array())
295312

296313
echo PHP_EOL;
297314
}
315+
316+
/**
317+
* @param $statusCode
318+
* @return bool
319+
*/
320+
public function isTemporaryOrPermanentRedirect($statusCode)
321+
{
322+
return
323+
$statusCode === Response::STATUS_TEMPORARY_REDIRECT
324+
|| $statusCode === Response::STATUS_PERMANENT_REDIRECT;
325+
}
298326
}

‎tests/Io/TransactionTest.php‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -671,6 +671,107 @@ public function testSomeRequestHeadersShouldBeRemovedWhenRedirecting()
671671
$transaction->send($requestWithCustomHeaders);
672672
}
673673

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

0 commit comments

Comments
 (0)