Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEA…

…D & OPTIONS requests (#5704)

* Add tests

* Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests

* Fix style

* Handle HttpURLConnection switching GET->POST and not supporting PATCH

* Update protocol test

(cherry picked from commit b2fcb7e)

.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"
+}

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,28 +109,29 @@ 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);
+                if (hasContentStream(request)) {
+                    return createEntityEnclosingRequest(request, method, uri);
+                }
+                return new HttpRequestImpl(method, 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 createEntityEnclosingRequest(request, method, uri);
             default:
-                throw new RuntimeException("Unknown HTTP method name: " + request.httpRequest().method());
+                throw new RuntimeException("Unknown HTTP method name: " + method);
         }
     }
 
-    private HttpRequestBase wrapEntity(HttpExecuteRequest request,
-                                       HttpEntityEnclosingRequestBase entityEnclosingRequest) {
+
+
+    private HttpRequestBase createEntityEnclosingRequest(HttpExecuteRequest request, SdkHttpMethod method, URI uri) {
+        HttpEntityEnclosingRequestBase entityEnclosingRequest = new HttpEntityEnclosingRequestImpl(method, uri);
 
         /*
          * We should never reuse the entity of the previous request, since
@@ -149,7 +143,7 @@ private HttpRequestBase wrapEntity(HttpExecuteRequest request,
          * preparation for the retry. Eventually, these wrappers would
          * return incorrect validation result.
          */
-        if (request.contentStreamProvider().isPresent()) {
+        if (hasContentStream(request)) {
             HttpEntity entity = new RepeatableInputStreamRequestEntity(request);
             if (!request.httpRequest().firstMatchingHeader(HttpHeaders.CONTENT_LENGTH).isPresent() && !entity.isChunked()) {
                 entity = ApacheUtils.newBufferedHttpEntity(entity);
@@ -160,6 +154,10 @@ private HttpRequestBase wrapEntity(HttpExecuteRequest request,
         return entityEnclosingRequest;
     }
 
+    private static boolean hasContentStream(HttpExecuteRequest request) {
+        return request.contentStreamProvider().isPresent();
+    }
+
     /**
      * Configures the headers in the specified Apache HTTP request.
      */
@@ -192,4 +190,32 @@ private String getHostHeaderValue(SdkHttpRequest request) {
                 ? request.host() + ":" + request.port()
                 : request.host();
     }
+
+    private static final class HttpRequestImpl extends HttpRequestBase {
+        private final SdkHttpMethod method;
+
+        private HttpRequestImpl(SdkHttpMethod method, URI uri) {
+            this.method = method;
+            setURI(uri);
+        }
+
+        @Override
+        public String getMethod() {
+            return this.method.toString();
+        }
+    }
+
+    private static final class HttpEntityEnclosingRequestImpl extends HttpEntityEnclosingRequestBase {
+        private final SdkHttpMethod method;
+
+        private HttpEntityEnclosingRequestImpl(SdkHttpMethod method, URI uri) {
+            this.method = method;
+            setURI(uri);
+        }
+
+        @Override
+        public String getMethod() {
+            return this.method.toString();
+        }
+    }
 }

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,29 @@ protected SdkHttpClient createSdkHttpClient() {
     public void reset() {
         HttpsURLConnection.setDefaultSSLSocketFactory((SSLSocketFactory) SSLSocketFactory.getDefault());
     }
+
+    @Test
+    @Override
+    public void supportsGetRequestWithRequestBody() 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 supportsPatchRequestWithoutRequestBody() {
+        // HttpURLConnection does not support PATCH requests.
+        assertThatThrownBy(super::supportsPatchRequestWithoutRequestBody)
+            .hasRootCauseInstanceOf(ProtocolException.class)
+            .hasRootCauseMessage("Invalid HTTP method: PATCH");
+    }
+
+    @Test
+    @Override
+    public void supportsPatchRequestWithRequestBody() {
+        // HttpURLConnection does not support PATCH requests.
+        assertThatThrownBy(super::supportsPatchRequestWithRequestBody)
+            .hasRootCauseInstanceOf(ProtocolException.class)
+            .hasRootCauseMessage("Invalid HTTP method: PATCH");
+    }
 }

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,45 +98,122 @@ 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 supportsDeleteRequestWithoutRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.DELETE, false);
+    }
+
+    @Test
+    public void supportsDeleteRequestWithRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.DELETE, true);
+    }
+
+    @Test
+    public void supportsGetRequestWithoutRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.GET, false);
+    }
+
+    @Test
+    public void supportsGetRequestWithRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.GET, true);
+    }
+
+    @Test
+    public void supportsHeadRequestWithoutRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.HEAD, false);
+    }
+
+    @Test
+    public void supportsHeadRequestWithRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.HEAD, true);
+    }
+
+    @Test
+    public void supportsOptionsRequestWithoutRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.OPTIONS, false);
+    }
+
+    @Test
+    public void supportsOptionsRequestWithRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.OPTIONS, true);
+    }
+
+    @Test
+    public void supportsPatchRequestWithoutRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.PATCH, false);
+    }
+
+    @Test
+    public void supportsPatchRequestWithRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.PATCH, true);
+    }
+
+    @Test
+    public void supportsPostRequestWithoutRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.POST, false);
+    }
+
+    @Test
+    public void supportsPostRequestWithRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.POST, true);
+    }
+
+    @Test
+    public void supportsPutRequestWithoutRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.PUT, false);
+    }
+
+    @Test
+    public void supportsPutRequestWithRequestBody() throws Exception {
+        testForResponseCode(200, SdkHttpMethod.PUT, 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 {
+    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);
 
-        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()
                                                                                                     .orElse(null))
                                                                           .build())
                                         .call();
 
-        validateResponse(rsp, returnCode, method);
+        validateResponse(rsp, returnCode, expectedMethod, 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()
                                                                                                     .orElse(null))
                                                                           .build())
                                         .call();
 
-        validateResponse(rsp, returnCode, sdkHttpMethod);
+        validateResponse(rsp, returnCode, sdkHttpMethod, true);
     }
 
     private void stubForMockRequest(int returnCode) {
@@ -151,17 +228,24 @@ 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());
+        if (expectBody) {
+            patternBuilder.withRequestBody(equalTo("Body"))
+                .withHeader("Content-Length", equalTo("4"));
         } else {
-            patternBuilder.withRequestBody(equalTo("Body"));
+            patternBuilder.withRequestBody(absent());
+            if (method != SdkHttpMethod.PATCH && method != SdkHttpMethod.POST && method != SdkHttpMethod.PUT) {
+                patternBuilder.withoutHeader("Content-Length");
+            }
         }
 
         mockServer.verify(1, patternBuilder);
@@ -177,14 +261,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));