Merge pull request #3501 from aws/joviegas/revert_apache_wrapentity

Revert "Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests (#5704)
aws · Dec 3, 2024 · 31f0c49 · 31f0c49
2 parents b19b9d3 + 094ddae
commit 31f0c49
Show file tree

Hide file tree

Showing 6 changed files with 45 additions and 105 deletions.
diff --git a/.changes/2.29.26.json b/.changes/2.29.26.json
@@ -109,6 +109,12 @@
             "category": "Redshift Serverless",
             "contributor": "",
             "description": "Adds support for the ListManagedWorkgroups API to get an overview of existing managed workgroups."
+        },
+        {
+            "type": "bugfix",
+            "category": "AWS SDK for Java v2",
+            "contributor": "",
+            "description": "Reverted PR https://github.com/aws/aws-sdk-java-v2/pull/5704, which modified all HTTP methods to include the request body entity, as it caused regression issues for some services."
         }
     ]
 }
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -72,6 +72,10 @@
   - ### Features
     - Adds support for the ListManagedWorkgroups API to get an overview of existing managed workgroups.
 
+## __AWS SDK for Java v2__
+- ### Bugfixes
+    - Reverted PR https://github.com/aws/aws-sdk-java-v2/pull/5704, which modified all HTTP methods to include the request body entity, as it caused regression issues for some services.
+
 # __2.29.25__ __2024-12-02__
 ## __AWS End User Messaging Social__
   - ### Features

diff --git a/.../main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java b/.../main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java
@@ -24,7 +24,14 @@
 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;
@@ -109,18 +116,23 @@ private void addRequestConfig(final HttpRequestBase base,
 
 
     private HttpRequestBase createApacheRequest(HttpExecuteRequest request, URI uri) {
-        SdkHttpMethod method = request.httpRequest().method();
-        switch (method) {
+        switch (request.httpRequest().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 HttpRequestImpl(method, uri));
+                return wrapEntity(request, new HttpPut(uri));
             default:
-                throw new RuntimeException("Unknown HTTP method name: " + method);
+                throw new RuntimeException("Unknown HTTP method name: " + request.httpRequest().method());
         }
     }
 
@@ -180,18 +192,4 @@ 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();
-        }
-    }
 }
diff --git a/...software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientDefaultWireMockTest.java b/...software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientDefaultWireMockTest.java
@@ -15,16 +15,11 @@
 
 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 {
 
@@ -37,20 +32,4 @@ 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/...client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java b/...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, false);
+        testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD);
     }
 
     @Test
@@ -76,7 +76,7 @@ public void supportsResponseCode403() throws Exception {
 
     @Test
     public void supportsResponseCode403Head() throws Exception {
-        testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD, false);
+        testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD);
     }
 
     @Test
@@ -98,87 +98,45 @@ public void supportsResponseCode500() throws Exception {
     public void validatesHttpsCertificateIssuer() {
         SdkHttpClient client = createSdkHttpClient();
 
-        SdkHttpFullRequest request = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), SdkHttpMethod.POST, true);
+        SdkHttpFullRequest request = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), SdkHttpMethod.POST);
 
         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, true);
+        testForResponseCode(returnCode, SdkHttpMethod.POST);
     }
 
-    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 {
+    private void testForResponseCode(int returnCode, SdkHttpMethod method) throws Exception {
         SdkHttpClient client = createSdkHttpClient();
 
         stubForMockRequest(returnCode);
 
-        SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), method, includeBody);
+        SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), method);
         HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder()
                                                                           .request(req)
                                                                           .contentStreamProvider(req.contentStreamProvider()
                                                                                                     .orElse(null))
                                                                           .build())
                                         .call();
 
-        validateResponse(rsp, returnCode, expectedMethod, includeBody);
+        validateResponse(rsp, returnCode, method);
     }
 
     protected void testForResponseCodeUsingHttps(SdkHttpClient client, int returnCode) throws Exception {
         SdkHttpMethod sdkHttpMethod = SdkHttpMethod.POST;
         stubForMockRequest(returnCode);
 
-        SdkHttpFullRequest req = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), sdkHttpMethod, true);
+        SdkHttpFullRequest req = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), sdkHttpMethod);
         HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder()
                                                                           .request(req)
                                                                           .contentStreamProvider(req.contentStreamProvider()
                                                                                                     .orElse(null))
                                                                           .build())
                                         .call();
 
-        validateResponse(rsp, returnCode, sdkHttpMethod, true);
+        validateResponse(rsp, returnCode, sdkHttpMethod);
     }
 
     private void stubForMockRequest(int returnCode) {
@@ -193,20 +151,17 @@ 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) 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 (expectBody) {
-            patternBuilder.withRequestBody(equalTo("Body"));
-        } else {
+        if (method == SdkHttpMethod.HEAD) {
             patternBuilder.withRequestBody(absent());
+        } else {
+            patternBuilder.withRequestBody(equalTo("Body"));
         }
 
         mockServer.verify(1, patternBuilder);
@@ -222,14 +177,14 @@ private void validateResponse(HttpExecuteResponse response,
         mockServer.resetMappings();
     }
 
-    private SdkHttpFullRequest mockSdkRequest(String uriString, SdkHttpMethod method, boolean includeBody) {
+    private SdkHttpFullRequest mockSdkRequest(String uriString, SdkHttpMethod method) {
         URI uri = URI.create(uriString);
         SdkHttpFullRequest.Builder requestBuilder = SdkHttpFullRequest.builder()
                                                                       .uri(uri)
                                                                       .method(method)
                                                                       .putHeader("Host", uri.getHost())
                                                                       .putHeader("User-Agent", "hello-world!");
-        if (includeBody) {
+        if (method != SdkHttpMethod.HEAD) {
             byte[] content = "Body".getBytes(StandardCharsets.UTF_8);
             requestBuilder.putHeader("Content-Length", Integer.toString(content.length));
             requestBuilder.contentStreamProvider(() -> new ByteArrayInputStream(content));

diff --git a/...rc/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-json-contenttype.json b/...rc/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-json-contenttype.json
@@ -230,11 +230,9 @@
       "uri": "/no-payload",
       "method": "GET",
       "headers": {
-        "contains": {
-          "Content-Length": "0"
-        },
         "doesNotContain": [
-          "Content-Type"
+          "Content-Type",
+          "Content-Length"
         ]
       },
       "body": {
@@ -260,11 +258,11 @@
       "method": "GET",
       "headers": {
         "contains": {
-          "Content-Length": "0",
           "x-amz-test-id": "t-12345"
         },
         "doesNotContain": [
-          "Content-Type"
+          "Content-Type",
+          "Content-Length"
         ]
       },
       "body": {