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

[dinooo13]

Should close #409
303 See Other should change the method to GET all other redirects should leave it unchanged
(Source: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#redirection_messages)

I think there are some specialties for 300 and 304 which I ignored for now, but I could look further into it if you wish.

[SimonFrings]

Hey @dinooo13 , thanks for looking into this one 👍

This is an interesting topic and the approach looks nice. It may not seem like it but I think this not only fixes some stuff but also breaks some (for example the new request doesn't contain a body, so when redirecting a POST there is no data). It fixes the problem addressed in #409 but I also think it overloads this PR to use this new rule on every 300 status code (except 303).

I also looked a bit deeper into the HTTP specification. For the majority of the 300 status codes, it says that the request method may change when redirecting, which depends a bit on the use case and who you're talking to. Because of all of this, I think it's best that this PR only focuses on the 307 (and 308) for now and that we tackle the remaining status codes in a following one (if necessary).

Additionally, the specification says, that the method for 303 should always be a GET when redirecting. You already covered this in your current PR but I was thinking: What if the original method was HEAD. The HEAD and GET methods are very similar to each other, the only difference is that the HEAD method doesn't request the body. If I use a HEAD method now and this changes into a GET when redirecting, I'm receiving more data than I originally wanted.

I am curious what your thoughts on this are! Should we add some kind of functionality that HEAD will stay the same when receiving a 303 (which is not explicitly mentioned inside the RFC)?

[dinooo13]

Okay I will update to only handle 307/308, yes it's better to address these when they actually become problems (YAGNI again 😄 ). Actually I found this in the RFC regarding 303 HEAD requests:

Except for responses to a HEAD request, the representation of a 303
response ought to contain a short hypertext note with a hyperlink to
the same URI reference provided in the Location header field.

https://datatracker.ietf.org/doc/html/rfc7231#section-6.4.4

[dinooo13]

Updated this PR to only handle 303, 307 and 308. 303 will be converted to GET. 307 and 308 will preserve their methods and body. All others should have the same behavior as before.

[SimonFrings]

@dinooo13 Looking good, but I still think the logic for 303 should use the default behavior (HEAD stays HEAD and GET stays GET).

The logic behind your code works great, I am not quite sure if we're introducing to much complexity by adding two new methods. I came up with a shorter version of your code, still the same logic:

private function onResponseRedirect(ResponseInterface $response, RequestInterface $request, Deferred $deferred, ClientRequestState $state)
    {
        // resolve location relative to last request URI
        $location = Uri::resolve($request->getUri(), $response->getHeaderLine('Location'));

        $request = $this->makeRedirectRequest($request, $location, $response->getStatusCode());
        $this->progress('redirect', array($request));

        if ($state->numRequests >= $this->maxRedirects) {
            throw new \RuntimeException('Maximum number of redirects (' . $this->maxRedirects . ') exceeded');
        }

        return $this->next($request, $deferred, $state);
    }

The first code block shows the onResponseRedirect() method. I have added $response->getStatusCode() as a parameter to the makeRedirectRequest() function call. The status code is the only part we need, this way we don't have to pass the whole response.

private function makeRedirectRequest(RequestInterface $request, UriInterface $location, Int $statusCode)
    {
        $originalHost = $request->getUri()->getHost();
        $request = $request->withoutHeader('Host');
         
        if ($statusCode !== 307 || $statusCode !== 308) {
            $request = $request
                ->withoutHeader('Content-Type')
                ->withoutHeader('Content-Length');
        }

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

        $body = null;
        if ($statusCode === 307 || $statusCode === 308) {
            $method = $request->getMethod();
            $body = $request->getBody();
        } else {
            // naïve approach..
            $method = ($request->getMethod() === 'HEAD') ? 'HEAD' : 'GET';
        }

        return new Request($method, $location, $request->getHeaders(), $body);
    }

The second block shows the makeRedirectRequest() method. As you can see I've added the status code as an additional argument to this function. The rest follows the same logic as your suggestion, just a little less code. The if's will only check for the 307 and 308 status codes and add the original method and body to the new request. Every other 3xx status code uses the default logic which is already implemented.

The only thing I don't know how to handle yet are streaming bodies. The original request would send the whole body before the redirect response arrives. This means the new request can't contain the same body (data is already sent).

What are your thoughts on this?

[dinooo13]

Nice yes I think this really simplifies it. I found this about how Chrome handles this with the fetch API:

Restricted redirects
Some forms of HTTP redirect require the browser to resend the body of the request to another URL. To support this, the browser would have to buffer the contents of the stream, which sort-of defeats the point, so it doesn't do that.

https://web.dev/fetch-upload-streaming/#restricted-redirects

I think this sums it up pretty good, it just makes no sense to redirect a stream.

[SimonFrings]

Seems like this is the way to go then 👍

I also found this inside the Guzzle documentation, throwing an exception in this case could be the answer for this ticket too. They're also using some kind of repeatable streams, this could be something for a follow up PR maybe ^^

[dinooo13]

Hey I updated the PR with your suggestions and added an exception and a test. Please have a close look at the new test I'm not 100% sure if it's correct. I'm also curious what you think about adding an exception with basically only a default message and a name should we even bother then? I think a hint in the documentation on reactphp.org would also be good to have.

[dinooo13]

I'm not sure if the re-request review button worked you may now have no or quite a few requests 🤣

[WyriHaximus]

I'm not sure if the re-request review button worked you may now have no or quite a few requests rofl

Looks like even I can't re-request @SimonFrings' review 😱

[WyriHaximus]

👍

[SimonFrings]

@WyriHaximus Could be a sign, but I am here anyway ^^

@dinooo13 Changes are looking great, the last thing I want to mention is the amount of commits for this. I don't think it is necessary to have 10 commits for this, can you squash them together into one? After that this is ready to be merged! 👍

[dinooo13]

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests

Can u maybe enable this on the repo or am I just not seeing this option? This would make it way easier to merge PRs. But I can also squash manually in the evening I think.

[dinooo13]

Everything squashed 😄

[SimonFrings]

Nice one 👍

[SimonFrings]

Ping @clue, this is ready for review 👍

[clue]

@dinooo13 Some good progress, would love to get this in! 👍 See minor remarks below, otherwise LGTM 👍

[SimonFrings]

Hey @dinooo13, I think with refactoring @clue meant to bring related code together. I hope my suggestions aren't too confusing, I couldn't make one big diff out of this (GitHub wouldn't let me 😅)

[dinooo13]

LGTM! I applied your suggestions 👍🏼

[SimonFrings]

@dinooo13 Nice, now you only need to squash your commits ;)

[dinooo13]

Oh I think I messed something up squashing I will fix this in the evening

[dinooo13]

Oh I think I messed something up squashing I will fix this in the evening

Or not as it seems 😆 thought the checks failed but looks good

[clue]

@dinooo13 Thanks for the update! Changes LGTM, but perhaps we can add some additional tests like suggested below?

Also, the documentation currently says "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.", should we update this as part of this PR?

Keep it up! 👍

[dinooo13]

Added asserts for the headers in the tests as suggested and 🔥'd the whitespace

[SimonFrings]

Nice one 👍

Have you also seen @clue's comment regarding the README.md?

Also, the documentation currently says "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.", should we update this as part of this PR?

[dinooo13]

Ahh I thought this was about checking the headers have been removed in the test with See Other as status code. I will update the Readme!

[clue]

@dinooo13 Thank you very much for looking into this, changes LGTM! Keep it up!