Skip to content

Commit 7ce501e

Browse files
joviegasjoviegas
committed
Handled Review commments
1 parent 32853df commit 7ce501e

File tree

3 files changed

+96
-79
lines changed

3 files changed

+96
-79
lines changed

‎services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/handlers/StreamingRequestInterceptor.java‎

Lines changed: 20 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,11 @@
1515

1616
package software.amazon.awssdk.services.s3.internal.handlers;
1717

18+
import java.util.Optional;
1819
import software.amazon.awssdk.annotations.SdkInternalApi;
1920
import software.amazon.awssdk.core.interceptor.Context;
2021
import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
2122
import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
22-
import software.amazon.awssdk.core.sync.RequestBody;
2323
import software.amazon.awssdk.http.SdkHttpRequest;
2424
import software.amazon.awssdk.services.s3.model.PutObjectRequest;
2525
import software.amazon.awssdk.services.s3.model.UploadPartRequest;
@@ -30,6 +30,10 @@
3030
@SdkInternalApi
3131
//TODO: This should be generalized for all streaming requests
3232
public final class StreamingRequestInterceptor implements ExecutionInterceptor {
33+
34+
private static final String DECODED_CONTENT_LENGTH_HEADER = "x-amz-decoded-content-length";
35+
private static final String CONTENT_LENGTH_HEADER = "Content-Length";
36+
3337
@Override
3438
public SdkHttpRequest modifyHttpRequest(Context.ModifyHttpRequest context,
3539
ExecutionAttributes executionAttributes) {
@@ -39,32 +43,23 @@ public SdkHttpRequest modifyHttpRequest(Context.ModifyHttpRequest context,
3943
return context.httpRequest();
4044
}
4145

42-
/**
43-
* Determines whether to add 'Expect: 100-continue' header to streaming requests.
44-
*
45-
* Per RFC 9110 Section 10.1.1, clients MUST NOT send 100-continue for requests without content.
46-
*
47-
* Note: Empty Content length check currently applies to sync clients only. Sync HTTP clients (e.g., Apache HttpClient) may
48-
* reuse connections, and sending empty content with Expect header can cause issues if the server has already closed the
49-
* connection.
50-
*
51-
* @param context the HTTP request modification context
52-
* @return true if Expect header should be added, false otherwise
53-
*/
5446
private boolean shouldAddExpectContinueHeader(Context.ModifyHttpRequest context) {
55-
// Must be a streaming request type
56-
if (context.request() instanceof PutObjectRequest
57-
|| context.request() instanceof UploadPartRequest) {
58-
// Zero Content length check
59-
return context.requestBody()
60-
.flatMap(RequestBody::optionalContentLength)
61-
.map(length -> length != 0L)
62-
.orElse(true);
47+
// Only applies to streaming operations
48+
if (!(context.request() instanceof PutObjectRequest
49+
|| context.request() instanceof UploadPartRequest)) {
50+
return false;
6351
}
64-
return false;
52+
return getContentLengthHeader(context.httpRequest())
53+
.map(Long::parseLong)
54+
.map(length -> length != 0L)
55+
.orElse(true);
6556
}
6657

67-
68-
69-
58+
// Check x-amz-decoded-content-length first, fall back to Content-Length
59+
private Optional<String> getContentLengthHeader(SdkHttpRequest httpRequest) {
60+
Optional<String> decodedLength = httpRequest.firstMatchingHeader(DECODED_CONTENT_LENGTH_HEADER);
61+
return decodedLength.isPresent()
62+
? decodedLength
63+
: httpRequest.firstMatchingHeader(CONTENT_LENGTH_HEADER);
64+
}
7065
}

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/handlers/StreamingRequestInterceptorTest.java‎

Lines changed: 72 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,18 @@
1717

1818
import static org.assertj.core.api.Assertions.assertThat;
1919
import static software.amazon.awssdk.services.s3.utils.InterceptorTestUtils.modifyHttpRequestContext;
20+
import static software.amazon.awssdk.services.s3.utils.InterceptorTestUtils.modifyHttpRequestContextWithHttpRequest;
2021

22+
import java.net.URI;
23+
import java.util.stream.Stream;
2124
import org.junit.jupiter.api.Test;
25+
import org.junit.jupiter.params.ParameterizedTest;
26+
import org.junit.jupiter.params.provider.Arguments;
27+
import org.junit.jupiter.params.provider.MethodSource;
28+
import software.amazon.awssdk.core.SdkRequest;
2229
import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
30+
import software.amazon.awssdk.http.SdkHttpFullRequest;
31+
import software.amazon.awssdk.http.SdkHttpMethod;
2332
import software.amazon.awssdk.http.SdkHttpRequest;
2433
import software.amazon.awssdk.services.s3.model.GetObjectRequest;
2534
import software.amazon.awssdk.services.s3.model.PutObjectRequest;
@@ -50,49 +59,93 @@ public void modifyHttpRequest_setsExpect100Continue_whenSdkRequestIsUploadPart()
5059
@Test
5160
public void modifyHttpRequest_doesNotSetExpect_whenSdkRequestIsNotPutObject() {
5261

53-
final SdkHttpRequest modifiedRequest = interceptor.modifyHttpRequest(modifyHttpRequestContext(GetObjectRequest.builder().build()),
62+
SdkHttpRequest modifiedRequest = interceptor.modifyHttpRequest(modifyHttpRequestContext(GetObjectRequest.builder().build()),
5463
new ExecutionAttributes());
5564

5665
assertThat(modifiedRequest.firstMatchingHeader("Expect")).isNotPresent();
5766
}
5867

59-
@Test
60-
public void modifyHttpRequest_doesNotSetExpect_whenPutObjectHasZeroContentLength() {
61-
SdkHttpRequest modifiedRequest = interceptor.modifyHttpRequest(
62-
modifyHttpRequestContext(PutObjectRequest.builder().build(), 0L),
63-
new ExecutionAttributes());
68+
@ParameterizedTest(name = "{0} with {1}={2} should not set Expect header")
69+
@MethodSource("zeroContentLengthProvider")
70+
public void modifyHttpRequest_doesNotSetExpect_whenContentLengthIsZero(
71+
String requestType, String headerName, String headerValue, SdkRequest sdkRequest) {
6472

65-
assertThat(modifiedRequest.firstMatchingHeader("Expect"))
66-
.as("Expect header should not be present for zero-length content per RFC 9110")
67-
.isNotPresent();
68-
}
73+
SdkHttpRequest httpRequest = buildHttpRequest(headerName, headerValue);
6974

70-
@Test
71-
public void modifyHttpRequest_doesNotSetExpect_whenUploadPartHasZeroContentLength() {
7275
SdkHttpRequest modifiedRequest = interceptor.modifyHttpRequest(
73-
modifyHttpRequestContext(UploadPartRequest.builder().build(), 0L),
76+
modifyHttpRequestContextWithHttpRequest(sdkRequest, httpRequest),
7477
new ExecutionAttributes());
7578

7679
assertThat(modifiedRequest.firstMatchingHeader("Expect"))
7780
.as("Expect header should not be present for zero-length content per RFC 9110")
7881
.isNotPresent();
7982
}
8083

81-
@Test
82-
public void modifyHttpRequest_setsExpect_whenPutObjectHasNonZeroContentLength() {
84+
@ParameterizedTest(name = "{0} with {1}={2} should set Expect header")
85+
@MethodSource("nonZeroContentLengthProvider")
86+
public void modifyHttpRequest_setsExpect_whenContentLengthIsNonZero(
87+
String requestType, String headerName, String headerValue, SdkRequest sdkRequest) {
88+
89+
SdkHttpRequest httpRequest = buildHttpRequest(headerName, headerValue);
90+
8391
SdkHttpRequest modifiedRequest = interceptor.modifyHttpRequest(
84-
modifyHttpRequestContext(PutObjectRequest.builder().build(), 1024L),
92+
modifyHttpRequestContextWithHttpRequest(sdkRequest, httpRequest),
8593
new ExecutionAttributes());
8694

8795
assertThat(modifiedRequest.firstMatchingHeader("Expect")).hasValue("100-continue");
8896
}
8997

9098
@Test
91-
public void modifyHttpRequest_setsExpect_whenUploadPartHasNonZeroContentLength() {
99+
public void modifyHttpRequest_prioritizesDecodedContentLength_overContentLength() {
100+
SdkHttpRequest httpRequest = SdkHttpFullRequest.builder()
101+
.uri(URI.create("http://localhost:8080"))
102+
.method(SdkHttpMethod.PUT)
103+
.putHeader("x-amz-decoded-content-length", "0")
104+
.putHeader("Content-Length", "1024")
105+
.build();
106+
92107
SdkHttpRequest modifiedRequest = interceptor.modifyHttpRequest(
93-
modifyHttpRequestContext(UploadPartRequest.builder().build(), 5242880L),
108+
modifyHttpRequestContextWithHttpRequest(PutObjectRequest.builder().build(), httpRequest),
94109
new ExecutionAttributes());
95110

96-
assertThat(modifiedRequest.firstMatchingHeader("Expect")).hasValue("100-continue");
111+
assertThat(modifiedRequest.firstMatchingHeader("Expect"))
112+
.as("x-amz-decoded-content-length should take priority over Content-Length")
113+
.isNotPresent();
114+
}
115+
116+
// Helper method to build HTTP request with specific header
117+
private SdkHttpRequest buildHttpRequest(String headerName, String headerValue) {
118+
return SdkHttpFullRequest.builder()
119+
.uri(URI.create("http://localhost:8080"))
120+
.method(SdkHttpMethod.PUT)
121+
.putHeader(headerName, headerValue)
122+
.build();
123+
}
124+
125+
// Test data providers
126+
private static Stream<Arguments> zeroContentLengthProvider() {
127+
return Stream.of(
128+
Arguments.of("PutObject", "Content-Length", "0",
129+
PutObjectRequest.builder().build()),
130+
Arguments.of("PutObject", "x-amz-decoded-content-length", "0",
131+
PutObjectRequest.builder().build()),
132+
Arguments.of("UploadPart", "Content-Length", "0",
133+
UploadPartRequest.builder().build()),
134+
Arguments.of("UploadPart", "x-amz-decoded-content-length", "0",
135+
UploadPartRequest.builder().build())
136+
);
137+
}
138+
139+
private static Stream<Arguments> nonZeroContentLengthProvider() {
140+
return Stream.of(
141+
Arguments.of("PutObject", "Content-Length", "1024",
142+
PutObjectRequest.builder().build()),
143+
Arguments.of("PutObject", "x-amz-decoded-content-length", "1024",
144+
PutObjectRequest.builder().build()),
145+
Arguments.of("UploadPart", "Content-Length", "1024",
146+
UploadPartRequest.builder().build()),
147+
Arguments.of("UploadPart", "x-amz-decoded-content-length", "1024",
148+
UploadPartRequest.builder().build())
149+
);
97150
}
98151
}

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/InterceptorTestUtils.java‎

Lines changed: 4 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,10 @@ public static Context.ModifyHttpRequest modifyHttpRequestContext(SdkRequest requ
106106
return modifyHttpRequestContext(request, sdkHttpFullRequest());
107107
}
108108

109+
public static Context.ModifyHttpRequest modifyHttpRequestContextWithHttpRequest(SdkRequest request, SdkHttpRequest httpRequest) {
110+
return modifyHttpRequestContext(request, httpRequest);
111+
}
112+
109113
public static Context.ModifyHttpRequest modifyHttpRequestContext(SdkRequest request, SdkHttpRequest sdkHttpRequest) {
110114
Optional<RequestBody> requestBody = Optional.of(RequestBody.fromString("helloworld"));
111115
Optional<AsyncRequestBody> asyncRequestBody = Optional.of(AsyncRequestBody.fromString("helloworld"));
@@ -133,41 +137,6 @@ public SdkRequest request() {
133137
};
134138
}
135139

136-
/**
137-
* Creates a ModifyHttpRequest context with a specific content length.
138-
* Useful for testing scenarios where content length matters (e.g., RFC 9110 compliance).
139-
*
140-
* @param request the SDK request
141-
* @param contentLength the content length in bytes
142-
* @return a ModifyHttpRequest context with the specified content length
143-
*/
144-
public static Context.ModifyHttpRequest modifyHttpRequestContext(SdkRequest request, long contentLength) {
145-
Optional<RequestBody> requestBody = Optional.of(RequestBody.fromBytes(new byte[(int) contentLength]));
146-
Optional<AsyncRequestBody> asyncRequestBody = Optional.of(AsyncRequestBody.fromBytes(new byte[(int) contentLength]));
147-
148-
return new Context.ModifyHttpRequest() {
149-
@Override
150-
public SdkHttpRequest httpRequest() {
151-
return sdkHttpFullRequest();
152-
}
153-
154-
@Override
155-
public Optional<RequestBody> requestBody() {
156-
return requestBody;
157-
}
158-
159-
@Override
160-
public Optional<AsyncRequestBody> asyncRequestBody() {
161-
return asyncRequestBody;
162-
}
163-
164-
@Override
165-
public SdkRequest request() {
166-
return request;
167-
}
168-
};
169-
}
170-
171140
public static Context.ModifyResponse modifyResponseContext(SdkRequest request, SdkResponse response, SdkHttpResponse sdkHttpResponse) {
172141
return new Context.ModifyResponse() {
173142
@Override

0 commit comments

Comments
 (0)