Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes the do not follow redirects bug #944

Conversation

MiguelLavigne
Copy link
Contributor

The HttpEngine would not obey the set state because there was no
correlation between the HttpURLConnection.setInstanceFollowRedirects
method and the OkHttpClient.

Fixed the missing link by adding a new setFollowRedirects method to the
OkHttpClient class. Bridged the gap for the HttpURLConnection.setInstanceFollowRedirects
by forwarding that state into the OkHttpClient.

Now the HttpEngine will always obey the OkHttpClient redirect state when
attempting the followUp phase.

Added necessary test to both OkUrlFactoryTest and CallTest.

#943

@MiguelLavigne
Copy link
Contributor Author

While it's easy to create a bug a figured I could also attempt to fix it. Here's my attempt, hopefully it can be useful!

@joreilly joreilly mentioned this pull request Jun 18, 2014
@MiguelLavigne
Copy link
Contributor Author

Should I be worried of having a broken build?
I noticed in the build history that the oraclejdk8 job often fails. Is that common?

@JakeWharton
Copy link
Member

Java 8 often fails. Flaky tests suck!

@@ -370,6 +371,14 @@ public boolean getFollowSslRedirects() {
return followSslRedirects;
}

public void setFollowRedirects(boolean followRedirects) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you copy the Javadoc from the above setter and modify it to describe this behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, not a problem. It's done!

@JakeWharton
Copy link
Member

Nice tests.

The HttpEngine would not obey the set state because there was no
correlation between the HttpURLConnection.setInstanceFollowRedirects
method and the OkHttpClient.

Fixed the missing link by adding a new setFollowRedirects method to the
OkHttpClient class.  Bridged the gap for the HttpURLConnection.setInstanceFollowRedirects
by forwarding that state into the OkHttpClient.

Now the HttpEngine will always obey the OkHttpClient redirect state when
attempting the followUp phase.

Added necessary test to both OkUrlFactoryTest and CallTest.

square#943
@MiguelLavigne
Copy link
Contributor Author

I thought it might be flakiness.
I updated the code with the comment. Let me know if there's anything else.
Thx!

@JakeWharton
Copy link
Member

LGTM. @swankjesse?

* Configure this client to follow redirects.
*
* <p>If unset, redirects will not be followed. This is the equivalent as the
* built-in {@code HttpURLConnection}'s default.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I might tweak this sentence later once this is landed. I thought in OkHttp following redirects was the default.

swankjesse added a commit that referenced this pull request Jun 28, 2014
…t_follow_redirects

Fixes the do not follow redirects bug
@swankjesse swankjesse merged commit 2cf3885 into square:master Jun 28, 2014
@davidschreiber
Copy link

This PR does not add support for follow-up POST redirects. Currently the redirect is prevented in case of a POST request:

case HTTP_PERM_REDIRECT:
case HTTP_TEMP_REDIRECT:
    // "If the 307 or 308 status code is received in response to a request other than GET
    // or HEAD, the user agent MUST NOT automatically redirect the request"
    if (!userRequest.method().equals("GET") && !userRequest.method().equals("HEAD")) {
        return null;
    }

So only GET and HEAD redirects are supported. Am I wrong, or should POST requests should also fall through?

@danielgomezrico
Copy link

danielgomezrico commented Sep 20, 2016

"If the 307 or 308 status code is received in response to a request other than GET
or HEAD, the user agent MUST NOT automatically redirect the request"

Why?

@swankjesse
Copy link
Member

It’s now-obsolete advice actually.

   The set of request methods that are safe to automatically redirect is
   no longer closed; user agents are able to make that determination
   based upon the request method semantics.  The redirect status codes
   301, 302, and 307 no longer have normative requirements on response
   payloads and user interaction.  (Section 6.4)

https://tools.ietf.org/html/rfc7231#appendix-B

@danielgomezrico
Copy link

@swankjesse then no redirect is done by OkHttp with any http code?

I tried with OkHttp 3.4.1 but:

  • 301 / 302 change POST to GET
  • 307 / 308 there's no redirect, just the body with the redirect Uri

@swankjesse
Copy link
Member

Nope, we follow redirects. Just the spec was recently revised and we could track the changes.

@danielgomezrico
Copy link

@swankjesse will be cool some sample or entry on FAQ for this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants