Skip to content

Commit 5f6ca8f

Browse files
XtansiaXtansia
committed
Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & 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)
1 parent 73d13ed commit 5f6ca8f

File tree

4 files changed

+180
-34
lines changed

4 files changed

+180
-34
lines changed

‎.changes/next-release/bugfix-AWSSDKforJavav2-c212259.json

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"type": "bugfix",
3+
"category": "AWS SDK for Java v2",
4+
"contributor": "Xtansia",
5+
"description": "Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests"
6+
}

‎http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java

+45-19
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,7 @@
2424
import org.apache.http.HttpEntity;
2525
import org.apache.http.HttpHeaders;
2626
import org.apache.http.client.config.RequestConfig;
27-
import org.apache.http.client.methods.HttpDelete;
2827
import org.apache.http.client.methods.HttpEntityEnclosingRequestBase;
29-
import org.apache.http.client.methods.HttpGet;
30-
import org.apache.http.client.methods.HttpHead;
31-
import org.apache.http.client.methods.HttpOptions;
32-
import org.apache.http.client.methods.HttpPatch;
33-
import org.apache.http.client.methods.HttpPost;
34-
import org.apache.http.client.methods.HttpPut;
3528
import org.apache.http.client.methods.HttpRequestBase;
3629
import software.amazon.awssdk.annotations.SdkInternalApi;
3730
import software.amazon.awssdk.http.HttpExecuteRequest;
@@ -116,28 +109,29 @@ private void addRequestConfig(final HttpRequestBase base,
116109

117110

118111
private HttpRequestBase createApacheRequest(HttpExecuteRequest request, URI uri) {
119-
switch (request.httpRequest().method()) {
112+
SdkHttpMethod method = request.httpRequest().method();
113+
switch (method) {
120114
case HEAD:
121-
return new HttpHead(uri);
122115
case GET:
123-
return new HttpGet(uri);
124116
case DELETE:
125-
return new HttpDelete(uri);
126117
case OPTIONS:
127-
return new HttpOptions(uri);
118+
if (hasContentStream(request)) {
119+
return createEntityEnclosingRequest(request, method, uri);
120+
}
121+
return new HttpRequestImpl(method, uri);
128122
case PATCH:
129-
return wrapEntity(request, new HttpPatch(uri));
130123
case POST:
131-
return wrapEntity(request, new HttpPost(uri));
132124
case PUT:
133-
return wrapEntity(request, new HttpPut(uri));
125+
return createEntityEnclosingRequest(request, method, uri);
134126
default:
135-
throw new RuntimeException("Unknown HTTP method name: " + request.httpRequest().method());
127+
throw new RuntimeException("Unknown HTTP method name: " + method);
136128
}
137129
}
138130

139-
private HttpRequestBase wrapEntity(HttpExecuteRequest request,
140-
HttpEntityEnclosingRequestBase entityEnclosingRequest) {
131+
132+
133+
private HttpRequestBase createEntityEnclosingRequest(HttpExecuteRequest request, SdkHttpMethod method, URI uri) {
134+
HttpEntityEnclosingRequestBase entityEnclosingRequest = new HttpEntityEnclosingRequestImpl(method, uri);
141135

142136
/*
143137
* We should never reuse the entity of the previous request, since
@@ -149,7 +143,7 @@ private HttpRequestBase wrapEntity(HttpExecuteRequest request,
149143
* preparation for the retry. Eventually, these wrappers would
150144
* return incorrect validation result.
151145
*/
152-
if (request.contentStreamProvider().isPresent()) {
146+
if (hasContentStream(request)) {
153147
HttpEntity entity = new RepeatableInputStreamRequestEntity(request);
154148
if (!request.httpRequest().firstMatchingHeader(HttpHeaders.CONTENT_LENGTH).isPresent() && !entity.isChunked()) {
155149
entity = ApacheUtils.newBufferedHttpEntity(entity);
@@ -160,6 +154,10 @@ private HttpRequestBase wrapEntity(HttpExecuteRequest request,
160154
return entityEnclosingRequest;
161155
}
162156

157+
private static boolean hasContentStream(HttpExecuteRequest request) {
158+
return request.contentStreamProvider().isPresent();
159+
}
160+
163161
/**
164162
* Configures the headers in the specified Apache HTTP request.
165163
*/
@@ -192,4 +190,32 @@ private String getHostHeaderValue(SdkHttpRequest request) {
192190
? request.host() + ":" + request.port()
193191
: request.host();
194192
}
193+
194+
private static final class HttpRequestImpl extends HttpRequestBase {
195+
private final SdkHttpMethod method;
196+
197+
private HttpRequestImpl(SdkHttpMethod method, URI uri) {
198+
this.method = method;
199+
setURI(uri);
200+
}
201+
202+
@Override
203+
public String getMethod() {
204+
return this.method.toString();
205+
}
206+
}
207+
208+
private static final class HttpEntityEnclosingRequestImpl extends HttpEntityEnclosingRequestBase {
209+
private final SdkHttpMethod method;
210+
211+
private HttpEntityEnclosingRequestImpl(SdkHttpMethod method, URI uri) {
212+
this.method = method;
213+
setURI(uri);
214+
}
215+
216+
@Override
217+
public String getMethod() {
218+
return this.method.toString();
219+
}
220+
}
195221
}

‎http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientDefaultWireMockTest.java

+30
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,16 @@
1515

1616
package software.amazon.awssdk.http.urlconnection;
1717

18+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
19+
20+
import java.net.ProtocolException;
1821
import javax.net.ssl.HttpsURLConnection;
1922
import javax.net.ssl.SSLSocketFactory;
23+
import org.junit.Test;
2024
import org.junit.jupiter.api.AfterEach;
2125
import software.amazon.awssdk.http.SdkHttpClient;
2226
import software.amazon.awssdk.http.SdkHttpClientDefaultTestSuite;
27+
import software.amazon.awssdk.http.SdkHttpMethod;
2328

2429
public class UrlConnectionHttpClientDefaultWireMockTest extends SdkHttpClientDefaultTestSuite {
2530

@@ -32,4 +37,29 @@ protected SdkHttpClient createSdkHttpClient() {
3237
public void reset() {
3338
HttpsURLConnection.setDefaultSSLSocketFactory((SSLSocketFactory) SSLSocketFactory.getDefault());
3439
}
40+
41+
@Test
42+
@Override
43+
public void supportsGetRequestWithRequestBody() throws Exception {
44+
// HttpURLConnection is hard-coded to switch GET requests with a body to POST requests, in #getOutputStream0.
45+
testForResponseCode(200, SdkHttpMethod.GET, SdkHttpMethod.POST, true);
46+
}
47+
48+
@Test
49+
@Override
50+
public void supportsPatchRequestWithoutRequestBody() {
51+
// HttpURLConnection does not support PATCH requests.
52+
assertThatThrownBy(super::supportsPatchRequestWithoutRequestBody)
53+
.hasRootCauseInstanceOf(ProtocolException.class)
54+
.hasRootCauseMessage("Invalid HTTP method: PATCH");
55+
}
56+
57+
@Test
58+
@Override
59+
public void supportsPatchRequestWithRequestBody() {
60+
// HttpURLConnection does not support PATCH requests.
61+
assertThatThrownBy(super::supportsPatchRequestWithRequestBody)
62+
.hasRootCauseInstanceOf(ProtocolException.class)
63+
.hasRootCauseMessage("Invalid HTTP method: PATCH");
64+
}
3565
}

‎test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientDefaultTestSuite.java

+99-15
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ public void supportsResponseCode200() throws Exception {
6161
@Test
6262
public void supportsResponseCode200Head() throws Exception {
6363
// HEAD is special due to closing of the connection immediately and streams are null
64-
testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD);
64+
testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD, false);
6565
}
6666

6767
@Test
@@ -76,7 +76,7 @@ public void supportsResponseCode403() throws Exception {
7676

7777
@Test
7878
public void supportsResponseCode403Head() throws Exception {
79-
testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD);
79+
testForResponseCode(HttpURLConnection.HTTP_FORBIDDEN, SdkHttpMethod.HEAD, false);
8080
}
8181

8282
@Test
@@ -98,45 +98,122 @@ public void supportsResponseCode500() throws Exception {
9898
public void validatesHttpsCertificateIssuer() {
9999
SdkHttpClient client = createSdkHttpClient();
100100

101-
SdkHttpFullRequest request = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), SdkHttpMethod.POST);
101+
SdkHttpFullRequest request = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), SdkHttpMethod.POST, true);
102102

103103
assertThatThrownBy(client.prepareRequest(HttpExecuteRequest.builder().request(request).build())::call)
104104
.isInstanceOf(SSLHandshakeException.class);
105105
}
106106

107+
@Test
108+
public void supportsDeleteRequestWithoutRequestBody() throws Exception {
109+
testForResponseCode(200, SdkHttpMethod.DELETE, false);
110+
}
111+
112+
@Test
113+
public void supportsDeleteRequestWithRequestBody() throws Exception {
114+
testForResponseCode(200, SdkHttpMethod.DELETE, true);
115+
}
116+
117+
@Test
118+
public void supportsGetRequestWithoutRequestBody() throws Exception {
119+
testForResponseCode(200, SdkHttpMethod.GET, false);
120+
}
121+
122+
@Test
123+
public void supportsGetRequestWithRequestBody() throws Exception {
124+
testForResponseCode(200, SdkHttpMethod.GET, true);
125+
}
126+
127+
@Test
128+
public void supportsHeadRequestWithoutRequestBody() throws Exception {
129+
testForResponseCode(200, SdkHttpMethod.HEAD, false);
130+
}
131+
132+
@Test
133+
public void supportsHeadRequestWithRequestBody() throws Exception {
134+
testForResponseCode(200, SdkHttpMethod.HEAD, true);
135+
}
136+
137+
@Test
138+
public void supportsOptionsRequestWithoutRequestBody() throws Exception {
139+
testForResponseCode(200, SdkHttpMethod.OPTIONS, false);
140+
}
141+
142+
@Test
143+
public void supportsOptionsRequestWithRequestBody() throws Exception {
144+
testForResponseCode(200, SdkHttpMethod.OPTIONS, true);
145+
}
146+
147+
@Test
148+
public void supportsPatchRequestWithoutRequestBody() throws Exception {
149+
testForResponseCode(200, SdkHttpMethod.PATCH, false);
150+
}
151+
152+
@Test
153+
public void supportsPatchRequestWithRequestBody() throws Exception {
154+
testForResponseCode(200, SdkHttpMethod.PATCH, true);
155+
}
156+
157+
@Test
158+
public void supportsPostRequestWithoutRequestBody() throws Exception {
159+
testForResponseCode(200, SdkHttpMethod.POST, false);
160+
}
161+
162+
@Test
163+
public void supportsPostRequestWithRequestBody() throws Exception {
164+
testForResponseCode(200, SdkHttpMethod.POST, true);
165+
}
166+
167+
@Test
168+
public void supportsPutRequestWithoutRequestBody() throws Exception {
169+
testForResponseCode(200, SdkHttpMethod.PUT, false);
170+
}
171+
172+
@Test
173+
public void supportsPutRequestWithRequestBody() throws Exception {
174+
testForResponseCode(200, SdkHttpMethod.PUT, true);
175+
}
176+
107177
private void testForResponseCode(int returnCode) throws Exception {
108-
testForResponseCode(returnCode, SdkHttpMethod.POST);
178+
testForResponseCode(returnCode, SdkHttpMethod.POST, true);
109179
}
110180

111-
private void testForResponseCode(int returnCode, SdkHttpMethod method) throws Exception {
181+
protected void testForResponseCode(int returnCode, SdkHttpMethod method, boolean includeBody) throws Exception {
182+
testForResponseCode(returnCode, method, method, includeBody);
183+
}
184+
185+
protected void testForResponseCode(int returnCode,
186+
SdkHttpMethod method,
187+
SdkHttpMethod expectedMethod,
188+
boolean includeBody) throws Exception {
112189
SdkHttpClient client = createSdkHttpClient();
113190

114191
stubForMockRequest(returnCode);
115192

116-
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), method);
193+
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), method, includeBody);
117194
HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder()
118195
.request(req)
119196
.contentStreamProvider(req.contentStreamProvider()
120197
.orElse(null))
121198
.build())
122199
.call();
123200

124-
validateResponse(rsp, returnCode, method);
201+
validateResponse(rsp, returnCode, expectedMethod, includeBody);
125202
}
126203

127204
protected void testForResponseCodeUsingHttps(SdkHttpClient client, int returnCode) throws Exception {
128205
SdkHttpMethod sdkHttpMethod = SdkHttpMethod.POST;
129206
stubForMockRequest(returnCode);
130207

131-
SdkHttpFullRequest req = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), sdkHttpMethod);
208+
SdkHttpFullRequest req = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), sdkHttpMethod, true);
132209
HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder()
133210
.request(req)
134211
.contentStreamProvider(req.contentStreamProvider()
135212
.orElse(null))
136213
.build())
137214
.call();
138215

139-
validateResponse(rsp, returnCode, sdkHttpMethod);
216+
validateResponse(rsp, returnCode, sdkHttpMethod, true);
140217
}
141218

142219
private void stubForMockRequest(int returnCode) {
@@ -151,17 +228,24 @@ private void stubForMockRequest(int returnCode) {
151228
mockServer.stubFor(any(urlPathEqualTo("/")).willReturn(responseBuilder));
152229
}
153230

154-
private void validateResponse(HttpExecuteResponse response, int returnCode, SdkHttpMethod method) throws IOException {
231+
private void validateResponse(HttpExecuteResponse response,
232+
int returnCode,
233+
SdkHttpMethod method,
234+
boolean expectBody) throws IOException {
155235
RequestMethod requestMethod = RequestMethod.fromString(method.name());
156236

157237
RequestPatternBuilder patternBuilder = RequestPatternBuilder.newRequestPattern(requestMethod, urlMatching("/"))
158238
.withHeader("Host", containing("localhost"))
159239
.withHeader("User-Agent", equalTo("hello-world!"));
160240

161-
if (method == SdkHttpMethod.HEAD) {
162-
patternBuilder.withRequestBody(absent());
241+
if (expectBody) {
242+
patternBuilder.withRequestBody(equalTo("Body"))
243+
.withHeader("Content-Length", equalTo("4"));
163244
} else {
164-
patternBuilder.withRequestBody(equalTo("Body"));
245+
patternBuilder.withRequestBody(absent());
246+
if (method != SdkHttpMethod.PATCH && method != SdkHttpMethod.POST && method != SdkHttpMethod.PUT) {
247+
patternBuilder.withoutHeader("Content-Length");
248+
}
165249
}
166250

167251
mockServer.verify(1, patternBuilder);
@@ -177,14 +261,14 @@ private void validateResponse(HttpExecuteResponse response, int returnCode, SdkH
177261
mockServer.resetMappings();
178262
}
179263

180-
private SdkHttpFullRequest mockSdkRequest(String uriString, SdkHttpMethod method) {
264+
private SdkHttpFullRequest mockSdkRequest(String uriString, SdkHttpMethod method, boolean includeBody) {
181265
URI uri = URI.create(uriString);
182266
SdkHttpFullRequest.Builder requestBuilder = SdkHttpFullRequest.builder()
183267
.uri(uri)
184268
.method(method)
185269
.putHeader("Host", uri.getHost())
186270
.putHeader("User-Agent", "hello-world!");
187-
if (method != SdkHttpMethod.HEAD) {
271+
if (includeBody) {
188272
byte[] content = "Body".getBytes(StandardCharsets.UTF_8);
189273
requestBuilder.putHeader("Content-Length", Integer.toString(content.length));
190274
requestBuilder.contentStreamProvider(() -> new ByteArrayInputStream(content));

0 commit comments

Comments
 (0)