Skip to content

Disable following redirects on UrlConnectionHttpClient #989

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 2, 2019
Merged

Disable following redirects on UrlConnectionHttpClient #989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c279296
Disable following redirects on UrlConnectionHttpClient
Jan 2, 2019
e39efcd
Merge branch 'master' into disableHtppUrlRedirects
abrooksv Jan 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changes/next-release/bugfix-URLConnectionHTTPClient-2357bd0.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"category": "URLConnection HTTP Client",
"type": "bugfix",
"description": "Disable following redirects automatically since doing so causes SDK response handling to fail"
}
4 changes: 4 additions & 0 deletions ...ient/src/main/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ private HttpURLConnection createAndConfigureConnection(HttpExecuteRequest reques
connection.setDoOutput(true);
}

// Disable following redirects since it breaks SDK error handling and matches Apache.
// See: https://github.com/aws/aws-sdk-java-v2/issues/975
connection.setInstanceFollowRedirects(false);
Copy link
Contributor

@millems millems Jan 2, 2019

Choose a reason for hiding this comment

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

Comment why we do this?


return connection;
}

Expand Down
12 changes: 10 additions & 2 deletions test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientTestSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import com.github.tomakehurst.wiremock.client.ResponseDefinitionBuilder;
import com.github.tomakehurst.wiremock.junit.WireMockRule;
import java.io.ByteArrayInputStream;
import java.io.IOException;
Expand Down Expand Up @@ -129,8 +130,15 @@ protected void testForResponseCodeUsingHttps(SdkHttpClient client, int returnCod
}

private void stubForMockRequest(int returnCode) {
stubFor(any(urlPathEqualTo("/")).willReturn(
aResponse().withStatus(returnCode).withHeader("Some-Header", "With Value").withBody("hello")));
ResponseDefinitionBuilder responseBuilder = aResponse().withStatus(returnCode)
.withHeader("Some-Header", "With Value")
.withBody("hello");

if (returnCode >= 300 && returnCode <= 399) {
responseBuilder.withHeader("Location", "Some New Location");
}
Copy link
Contributor

@millems millems Jan 2, 2019

Choose a reason for hiding this comment

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

Do we actually exercise this code path?

Copy link
Contributor Author

@abrooksv abrooksv Jan 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

yes, supportsResponseCode301 and supportsResponseCode302 both trigger it.

supportsResponseCode302 -> testForResponseCode -> stubForMockRequest

Copy link
Contributor Author

@abrooksv abrooksv Jan 2, 2019

Choose a reason for hiding this comment

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

Without disable leads to test failure:

java.lang.NullPointerException: delegate must not be null.

	at software.amazon.awssdk.utils.Validate.paramNotNull(Validate.java:118)
	at software.amazon.awssdk.http.AbortableInputStream.<init>(AbortableInputStream.java:35)

Copy link
Contributor

@kiiadi kiiadi Jan 2, 2019

Choose a reason for hiding this comment

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

👍 awesome that you added a base-test for this so that we catch it for other implementations!


stubFor(any(urlPathEqualTo("/")).willReturn(responseBuilder));
}

private void validateResponse(HttpExecuteResponse response, int returnCode) throws IOException {
Expand Down