From 04b301c509e5507751faf5d00dc2527df66fa1fb Mon Sep 17 00:00:00 2001 From: Thomas Farr Date: Mon, 11 Nov 2024 17:11:29 +1300 Subject: [PATCH 1/5] Add tests --- .../http/SdkHttpClientDefaultTestSuite.java | 65 ++++++++++++++----- 1 file changed, 50 insertions(+), 15 deletions(-) diff --git a/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java b/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java index e0e300b848d4..9ef7e4d1e7a9 100644 --- a/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java +++ b/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java @@ -61,7 +61,7 @@ public void supportsResponseCode200() throws Exception { @Test public void supportsResponseCode200Head() throws Exception { // HEAD is special due to closing of the connection immediately and streams are null - testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD); + testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD, false); } @Test @@ -76,7 +76,7 @@ public void supportsResponseCode403() throws Exception { @Test public void supportsResponseCode403Head() throws Exception { - testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD); + testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD, false); } @Test @@ -98,22 +98,57 @@ public void supportsResponseCode500() throws Exception { public void validatesHttpsCertificateIssuer() { SdkHttpClient client = createSdkHttpClient(); - SdkHttpFullRequest request = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), SdkHttpMethod.POST); + SdkHttpFullRequest request = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), SdkHttpMethod.POST, true); assertThatThrownBy(client.prepareRequest(HttpExecuteRequest.builder().request(request).build())::call) .isInstanceOf(SSLHandshakeException.class); } + @Test + public void supportsRequestBodyOnGetRequest() throws Exception { + testForResponseCode(200, SdkHttpMethod.GET, true); + } + + @Test + public void supportsRequestBodyOnPostRequest() throws Exception { + testForResponseCode(200, SdkHttpMethod.POST, true); + } + + @Test + public void supportsRequestBodyOnPutRequest() throws Exception { + testForResponseCode(200, SdkHttpMethod.PUT, true); + } + + @Test + public void supportsRequestBodyOnDeleteRequest() throws Exception { + testForResponseCode(200, SdkHttpMethod.DELETE, true); + } + + @Test + public void supportsRequestBodyOnHeadRequest() throws Exception { + testForResponseCode(200, SdkHttpMethod.HEAD, true); + } + + @Test + public void supportsRequestBodyOnPatchRequest() throws Exception { + testForResponseCode(200, SdkHttpMethod.PATCH, true); + } + + @Test + public void supportsRequestBodyOnOptionsRequest() throws Exception { + testForResponseCode(200, SdkHttpMethod.OPTIONS, true); + } + private void testForResponseCode(int returnCode) throws Exception { - testForResponseCode(returnCode, SdkHttpMethod.POST); + testForResponseCode(returnCode, SdkHttpMethod.POST, true); } - private void testForResponseCode(int returnCode, SdkHttpMethod method) throws Exception { + private void testForResponseCode(int returnCode, SdkHttpMethod method, boolean includeBody) throws Exception { SdkHttpClient client = createSdkHttpClient(); stubForMockRequest(returnCode); - SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), method); + SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), method, includeBody); HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder() .request(req) .contentStreamProvider(req.contentStreamProvider() @@ -121,14 +156,14 @@ private void testForResponseCode(int returnCode, SdkHttpMethod method) throws Ex .build()) .call(); - validateResponse(rsp, returnCode, method); + validateResponse(rsp, returnCode, method, includeBody); } protected void testForResponseCodeUsingHttps(SdkHttpClient client, int returnCode) throws Exception { SdkHttpMethod sdkHttpMethod = SdkHttpMethod.POST; stubForMockRequest(returnCode); - SdkHttpFullRequest req = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), sdkHttpMethod); + SdkHttpFullRequest req = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), sdkHttpMethod, true); HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder() .request(req) .contentStreamProvider(req.contentStreamProvider() @@ -136,7 +171,7 @@ protected void testForResponseCodeUsingHttps(SdkHttpClient client, int returnCod .build()) .call(); - validateResponse(rsp, returnCode, sdkHttpMethod); + validateResponse(rsp, returnCode, sdkHttpMethod, true); } private void stubForMockRequest(int returnCode) { @@ -151,17 +186,17 @@ private void stubForMockRequest(int returnCode) { mockServer.stubFor(any(urlPathEqualTo("/")).willReturn(responseBuilder)); } - private void validateResponse(HttpExecuteResponse response, int returnCode, SdkHttpMethod method) throws IOException { + private void validateResponse(HttpExecuteResponse response, int returnCode, SdkHttpMethod method, boolean expectBody) throws IOException { RequestMethod requestMethod = RequestMethod.fromString(method.name()); RequestPatternBuilder patternBuilder = RequestPatternBuilder.newRequestPattern(requestMethod, urlMatching("/")) .withHeader("Host", containing("localhost")) .withHeader("User-Agent", equalTo("hello-world!")); - if (method == SdkHttpMethod.HEAD) { - patternBuilder.withRequestBody(absent()); - } else { + if (expectBody) { patternBuilder.withRequestBody(equalTo("Body")); + } else { + patternBuilder.withRequestBody(absent()); } mockServer.verify(1, patternBuilder); @@ -177,14 +212,14 @@ private void validateResponse(HttpExecuteResponse response, int returnCode, SdkH mockServer.resetMappings(); } - private SdkHttpFullRequest mockSdkRequest(String uriString, SdkHttpMethod method) { + private SdkHttpFullRequest mockSdkRequest(String uriString, SdkHttpMethod method, boolean includeBody) { URI uri = URI.create(uriString); SdkHttpFullRequest.Builder requestBuilder = SdkHttpFullRequest.builder() .uri(uri) .method(method) .putHeader("Host", uri.getHost()) .putHeader("User-Agent", "hello-world!"); - if (method != SdkHttpMethod.HEAD) { + if (includeBody) { byte[] content = "Body".getBytes(StandardCharsets.UTF_8); requestBuilder.putHeader("Content-Length", Integer.toString(content.length)); requestBuilder.contentStreamProvider(() -> new ByteArrayInputStream(content)); From fd3f2db061d69290a54b91e0229f2434b74c0992 Mon Sep 17 00:00:00 2001 From: Thomas Farr Date: Mon, 11 Nov 2024 17:28:27 +1300 Subject: [PATCH 2/5] Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests --- .../bugfix-AWSSDKforJavav2-c212259.json | 6 ++++ .../impl/ApacheHttpRequestFactory.java | 34 ++++++++++--------- 2 files changed, 24 insertions(+), 16 deletions(-) create mode 100644 .changes/next-release/bugfix-AWSSDKforJavav2-c212259.json diff --git a/.changes/next-release/bugfix-AWSSDKforJavav2-c212259.json b/.changes/next-release/bugfix-AWSSDKforJavav2-c212259.json new file mode 100644 index 000000000000..aa0eca4ab0fe --- /dev/null +++ b/.changes/next-release/bugfix-AWSSDKforJavav2-c212259.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "AWS SDK for Java v2", + "contributor": "Xtansia", + "description": "Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests" +} diff --git a/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java b/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java index cfb22343ba3f..b6642aeec611 100644 --- a/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java +++ b/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java @@ -24,14 +24,7 @@ import org.apache.http.HttpEntity; import org.apache.http.HttpHeaders; import org.apache.http.client.config.RequestConfig; -import org.apache.http.client.methods.HttpDelete; import org.apache.http.client.methods.HttpEntityEnclosingRequestBase; -import org.apache.http.client.methods.HttpGet; -import org.apache.http.client.methods.HttpHead; -import org.apache.http.client.methods.HttpOptions; -import org.apache.http.client.methods.HttpPatch; -import org.apache.http.client.methods.HttpPost; -import org.apache.http.client.methods.HttpPut; import org.apache.http.client.methods.HttpRequestBase; import software.amazon.awssdk.annotations.SdkInternalApi; import software.amazon.awssdk.http.HttpExecuteRequest; @@ -116,23 +109,18 @@ private void addRequestConfig(final HttpRequestBase base, private HttpRequestBase createApacheRequest(HttpExecuteRequest request, URI uri) { - switch (request.httpRequest().method()) { + SdkHttpMethod method = request.httpRequest().method(); + switch (method) { case HEAD: - return new HttpHead(uri); case GET: - return new HttpGet(uri); case DELETE: - return new HttpDelete(uri); case OPTIONS: - return new HttpOptions(uri); case PATCH: - return wrapEntity(request, new HttpPatch(uri)); case POST: - return wrapEntity(request, new HttpPost(uri)); case PUT: - return wrapEntity(request, new HttpPut(uri)); + return wrapEntity(request, new HttpRequestImpl(method, uri)); default: - throw new RuntimeException("Unknown HTTP method name: " + request.httpRequest().method()); + throw new RuntimeException("Unknown HTTP method name: " + method); } } @@ -192,4 +180,18 @@ private String getHostHeaderValue(SdkHttpRequest request) { ? request.host() + ":" + request.port() : request.host(); } + + private static final class HttpRequestImpl extends HttpEntityEnclosingRequestBase { + private final SdkHttpMethod method; + + private HttpRequestImpl(SdkHttpMethod method, URI uri) { + this.method = method; + setURI(uri); + } + + @Override + public String getMethod() { + return this.method.toString(); + } + } } From 7ca61f30adbd997667b19c2828eba761c415eb83 Mon Sep 17 00:00:00 2001 From: Thomas Farr Date: Mon, 11 Nov 2024 18:08:20 +1300 Subject: [PATCH 3/5] Fix style --- .../amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java b/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java index 9ef7e4d1e7a9..d0bc57261a8b 100644 --- a/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java +++ b/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java @@ -186,7 +186,10 @@ private void stubForMockRequest(int returnCode) { mockServer.stubFor(any(urlPathEqualTo("/")).willReturn(responseBuilder)); } - private void validateResponse(HttpExecuteResponse response, int returnCode, SdkHttpMethod method, boolean expectBody) throws IOException { + private void validateResponse(HttpExecuteResponse response, + int returnCode, + SdkHttpMethod method, + boolean expectBody) throws IOException { RequestMethod requestMethod = RequestMethod.fromString(method.name()); RequestPatternBuilder patternBuilder = RequestPatternBuilder.newRequestPattern(requestMethod, urlMatching("/")) From f935326a790da65157d99fa470f0bf07f2d5ecec Mon Sep 17 00:00:00 2001 From: Thomas Farr Date: Mon, 11 Nov 2024 23:19:43 +1300 Subject: [PATCH 4/5] Handle HttpURLConnection switching GET->POST and not supporting PATCH --- ...nnectionHttpClientDefaultWireMockTest.java | 21 +++++++++++++++++++ .../http/SdkHttpClientDefaultTestSuite.java | 11 ++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientDefaultWireMockTest.java b/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientDefaultWireMockTest.java index 66cbc4e3b814..12960c82dbcf 100644 --- a/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientDefaultWireMockTest.java +++ b/http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientDefaultWireMockTest.java @@ -15,11 +15,16 @@ package software.amazon.awssdk.http.urlconnection; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.net.ProtocolException; import javax.net.ssl.HttpsURLConnection; import javax.net.ssl.SSLSocketFactory; +import org.junit.Test; import org.junit.jupiter.api.AfterEach; import software.amazon.awssdk.http.SdkHttpClient; import software.amazon.awssdk.http.SdkHttpClientDefaultTestSuite; +import software.amazon.awssdk.http.SdkHttpMethod; public class UrlConnectionHttpClientDefaultWireMockTest extends SdkHttpClientDefaultTestSuite { @@ -32,4 +37,20 @@ protected SdkHttpClient createSdkHttpClient() { public void reset() { HttpsURLConnection.setDefaultSSLSocketFactory((SSLSocketFactory) SSLSocketFactory.getDefault()); } + + @Test + @Override + public void supportsRequestBodyOnGetRequest() throws Exception { + // HttpURLConnection is hard-coded to switch GET requests with a body to POST requests, in #getOutputStream0. + testForResponseCode(200, SdkHttpMethod.GET, SdkHttpMethod.POST, true); + } + + @Test + @Override + public void supportsRequestBodyOnPatchRequest() { + // HttpURLConnection does not support PATCH requests. + assertThatThrownBy(super::supportsRequestBodyOnPatchRequest) + .hasRootCauseInstanceOf(ProtocolException.class) + .hasRootCauseMessage("Invalid HTTP method: PATCH"); + } } diff --git a/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java b/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java index d0bc57261a8b..22b55cc6192b 100644 --- a/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java +++ b/test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java @@ -143,7 +143,14 @@ private void testForResponseCode(int returnCode) throws Exception { testForResponseCode(returnCode, SdkHttpMethod.POST, true); } - private void testForResponseCode(int returnCode, SdkHttpMethod method, boolean includeBody) throws Exception { + protected void testForResponseCode(int returnCode, SdkHttpMethod method, boolean includeBody) throws Exception { + testForResponseCode(returnCode, method, method, includeBody); + } + + protected void testForResponseCode(int returnCode, + SdkHttpMethod method, + SdkHttpMethod expectedMethod, + boolean includeBody) throws Exception { SdkHttpClient client = createSdkHttpClient(); stubForMockRequest(returnCode); @@ -156,7 +163,7 @@ private void testForResponseCode(int returnCode, SdkHttpMethod method, boolean i .build()) .call(); - validateResponse(rsp, returnCode, method, includeBody); + validateResponse(rsp, returnCode, expectedMethod, includeBody); } protected void testForResponseCodeUsingHttps(SdkHttpClient client, int returnCode) throws Exception { From 222d1f6c9e4495ad30b154d0d70ae228907126f8 Mon Sep 17 00:00:00 2001 From: Thomas Farr Date: Mon, 25 Nov 2024 11:45:30 +1300 Subject: [PATCH 5/5] Update protocol test --- .../protocol/suites/cases/rest-json-contenttype.json | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/protocol-tests-core/src/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-json-contenttype.json b/test/protocol-tests-core/src/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-json-contenttype.json index 7260c5743e6b..1833bb74f6fc 100644 --- a/test/protocol-tests-core/src/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-json-contenttype.json +++ b/test/protocol-tests-core/src/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-json-contenttype.json @@ -230,9 +230,11 @@ "uri": "/no-payload", "method": "GET", "headers": { + "contains": { + "Content-Length": "0" + }, "doesNotContain": [ - "Content-Type", - "Content-Length" + "Content-Type" ] }, "body": { @@ -258,11 +260,11 @@ "method": "GET", "headers": { "contains": { + "Content-Length": "0", "x-amz-test-id": "t-12345" }, "doesNotContain": [ - "Content-Type", - "Content-Length" + "Content-Type" ] }, "body": {