Skip to content

Commit 0a7ec87

Browse files
dagnirdagnir
committed
Review comments
 - Rename to computeAndMoveContentLength - Rename variable to chunkedPayload - Fix javadocs - Sign payload only if non-null
1 parent 7c7941c commit 0a7ec87

File tree

7 files changed

+33
-25
lines changed

7 files changed

+33
-25
lines changed

‎core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/crt/internal/signer/AwsChunkedV4aPayloadSigner.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
import static software.amazon.awssdk.http.auth.aws.internal.signer.util.SignerConstant.STREAMING_ECDSA_SIGNED_PAYLOAD_TRAILER;
2222
import static software.amazon.awssdk.http.auth.aws.internal.signer.util.SignerConstant.STREAMING_UNSIGNED_PAYLOAD_TRAILER;
2323
import static software.amazon.awssdk.http.auth.aws.internal.signer.util.SignerConstant.X_AMZ_TRAILER;
24-
import static software.amazon.awssdk.http.auth.aws.internal.signer.util.SignerUtils.moveContentLength;
24+
import static software.amazon.awssdk.http.auth.aws.internal.signer.util.SignerUtils.computeAndMoveContentLength;
2525

2626
import java.io.InputStream;
2727
import java.nio.charset.StandardCharsets;
@@ -40,6 +40,7 @@
4040
import software.amazon.awssdk.http.auth.aws.internal.signer.chunkedencoding.TrailerProvider;
4141
import software.amazon.awssdk.http.auth.aws.internal.signer.io.ChecksumInputStream;
4242
import software.amazon.awssdk.http.auth.aws.internal.signer.io.ResettableContentStreamProvider;
43+
import software.amazon.awssdk.http.auth.aws.internal.signer.util.SignerUtils;
4344
import software.amazon.awssdk.utils.BinaryUtils;
4445
import software.amazon.awssdk.utils.Pair;
4546
import software.amazon.awssdk.utils.StringInputStream;
@@ -108,7 +109,7 @@ public ContentStreamProvider sign(ContentStreamProvider payload, V4aRequestSigni
108109
@Override
109110
public void beforeSigning(SdkHttpRequest.Builder request, ContentStreamProvider payload, String checksum) {
110111
long encodedContentLength = 0;
111-
long contentLength = moveContentLength(request, payload);
112+
long contentLength = SignerUtils.computeAndMoveContentLength(request, payload);
112113
setupPreExistingTrailers(request);
113114

114115
// pre-existing trailers

‎core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/AwsChunkedV4PayloadSigner.java

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
import static software.amazon.awssdk.http.auth.aws.internal.signer.util.SignerConstant.X_AMZ_CONTENT_SHA256;
2626
import static software.amazon.awssdk.http.auth.aws.internal.signer.util.SignerConstant.X_AMZ_DECODED_CONTENT_LENGTH;
2727
import static software.amazon.awssdk.http.auth.aws.internal.signer.util.SignerConstant.X_AMZ_TRAILER;
28-
import static software.amazon.awssdk.http.auth.aws.internal.signer.util.SignerUtils.moveContentLength;
28+
import static software.amazon.awssdk.http.auth.aws.internal.signer.util.SignerUtils.computeAndMoveContentLength;
2929

3030
import java.nio.ByteBuffer;
3131
import java.nio.charset.StandardCharsets;
@@ -50,6 +50,7 @@
5050
import software.amazon.awssdk.http.auth.aws.internal.signer.chunkedencoding.SyncChunkEncodedPayload;
5151
import software.amazon.awssdk.http.auth.aws.internal.signer.chunkedencoding.TrailerProvider;
5252
import software.amazon.awssdk.http.auth.aws.internal.signer.io.ResettableContentStreamProvider;
53+
import software.amazon.awssdk.http.auth.aws.internal.signer.util.SignerUtils;
5354
import software.amazon.awssdk.utils.BinaryUtils;
5455
import software.amazon.awssdk.utils.Pair;
5556
import software.amazon.awssdk.utils.Validate;
@@ -97,8 +98,8 @@ public Publisher<ByteBuffer> signAsync(Publisher<ByteBuffer> payload, V4RequestS
9798
.chunkSize(chunkSize)
9899
.addEmptyTrailingChunk(true);
99100

100-
AsyncChunkEncodedPayload checksumPayload = new AsyncChunkEncodedPayload(chunkedStreamBuilder);
101-
signCommon(checksumPayload, requestSigningResult);
101+
AsyncChunkEncodedPayload chunkedPayload = new AsyncChunkEncodedPayload(chunkedStreamBuilder);
102+
signCommon(chunkedPayload, requestSigningResult);
102103

103104
return chunkedStreamBuilder.build();
104105
}
@@ -144,7 +145,7 @@ private void signCommon(ChunkedEncodedPayload payload, V4RequestSigningResult re
144145
@Override
145146
public void beforeSigning(SdkHttpRequest.Builder request, ContentStreamProvider payload) {
146147
long encodedContentLength = 0;
147-
long contentLength = moveContentLength(request, payload);
148+
long contentLength = SignerUtils.computeAndMoveContentLength(request, payload);
148149
setupPreExistingTrailers(request);
149150

150151
// pre-existing trailers
@@ -161,7 +162,7 @@ public void beforeSigning(SdkHttpRequest.Builder request, ContentStreamProvider
161162
@Override
162163
public CompletableFuture<Pair<SdkHttpRequest.Builder, Optional<Publisher<ByteBuffer>>>> beforeSigningAsync(
163164
SdkHttpRequest.Builder request, Publisher<ByteBuffer> payload) {
164-
return moveContentLength(request, payload)
165+
return computeAndMoveContentLength(request, payload)
165166
.thenApply(p -> {
166167
SdkHttpRequest.Builder requestBuilder = p.left();
167168
setupPreExistingTrailers(requestBuilder);

‎core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultAwsV4HttpSigner.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,9 +231,14 @@ private static CompletableFuture<AsyncSignedRequest> doSignAsync(AsyncSignReques
231231
Publisher<ByteBuffer> payloadToSign = p.right().orElse(null);
232232

233233
V4RequestSigningResult requestSigningResult = requestSigner.sign(requestToSign);
234+
235+
Publisher<ByteBuffer> signedPayload = null;
236+
if (payloadToSign != null) {
237+
signedPayload = payloadSigner.signAsync(payloadToSign, requestSigningResult);
238+
}
234239
return AsyncSignedRequest.builder()
235240
.request(requestSigningResult.getSignedRequest().build())
236-
.payload(payloadSigner.signAsync(payloadToSign, requestSigningResult))
241+
.payload(signedPayload)
237242
.build();
238243
});
239244
}

‎core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/chunkedencoding/ChunkedEncodedPayload.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@
2121
import software.amazon.awssdk.http.auth.aws.internal.signer.AwsChunkedV4PayloadSigner;
2222

2323
/**
24-
* Abstraction interface to simplify payload signing in {@link AwsChunkedV4PayloadSigner}.
24+
* Abstraction interface to simplify payload signing in {@link AwsChunkedV4PayloadSigner} by allowing us to have a uniform
25+
* interface for signing both sync and async payloads. See the {@code signCommon} method in {@link AwsChunkedV4PayloadSigner}.
2526
*/
2627
@SdkInternalApi
2728
public interface ChunkedEncodedPayload {

‎core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/io/UnbufferedChecksumSubscriber.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@
2424
import software.amazon.awssdk.checksums.SdkChecksum;
2525

2626
/**
27-
* A decorating {@code Subscriber} that updates a list of {@code SdkChecksum}s with the data of each buffer given to {@code
28-
* onNext}.
27+
* A decorating {@code Subscriber} that updates a list of {@code SdkChecksum}s with the data of each buffer given to
28+
* {@code onNext}.
2929
* <p>
3030
* This is "unbuffered", as opposed to {@link ChecksumSubscriber} which <i>does</i> buffer the data.
3131
*/

‎core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/util/SignerUtils.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ public static void addDateHeader(SdkHttpRequest.Builder requestBuilder, String d
201201
* Move `Content-Length` to `x-amz-decoded-content-length` if not already present. If `Content-Length` is not present, then
202202
* the payload is read in its entirety to calculate the length.
203203
*/
204-
public static long moveContentLength(SdkHttpRequest.Builder request, ContentStreamProvider contentStreamProvider) {
204+
public static long computeAndMoveContentLength(SdkHttpRequest.Builder request, ContentStreamProvider contentStreamProvider) {
205205
Optional<String> decodedContentLength = request.firstMatchingHeader(X_AMZ_DECODED_CONTENT_LENGTH);
206206

207207
if (decodedContentLength.isPresent()) {
@@ -228,7 +228,7 @@ public static long moveContentLength(SdkHttpRequest.Builder request, ContentStre
228228
* Move `Content-Length` to `x-amz-decoded-content-length` if not already present. If `Content-Length` is not present, then
229229
* the payload is read in its entirety to calculate the length.
230230
*/
231-
public static CompletableFuture<Pair<SdkHttpRequest.Builder, Optional<Publisher<ByteBuffer>>>> moveContentLength(
231+
public static CompletableFuture<Pair<SdkHttpRequest.Builder, Optional<Publisher<ByteBuffer>>>> computeAndMoveContentLength(
232232
SdkHttpRequest.Builder request, Publisher<ByteBuffer> contentPublisher) {
233233
Optional<String> decodedContentLength = request.firstMatchingHeader(X_AMZ_DECODED_CONTENT_LENGTH);
234234

‎core/http-auth-aws/src/test/java/software/amazon/awssdk/http/auth/aws/internal/signer/util/SignerUtilsTest.java

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -38,26 +38,26 @@
3838
public class SignerUtilsTest {
3939

4040
@Test
41-
void moveContentLength_decodedContentLengthPresent_shouldNotInvokeNewStream() {
41+
void computeAndMoveContentLength_decodedContentLengthPresent_shouldNotInvokeNewStream() {
4242
SdkHttpRequest.Builder request = SdkHttpRequest.builder()
4343
.appendHeader(X_AMZ_DECODED_CONTENT_LENGTH, "10")
4444
.appendHeader(CONTENT_LENGTH, "10");
4545

4646
ContentStreamProvider streamProvider = Mockito.mock(ContentStreamProvider.class);
47-
long contentLength = SignerUtils.moveContentLength(request, streamProvider);
47+
long contentLength = SignerUtils.computeAndMoveContentLength(request, streamProvider);
4848
Mockito.verify(streamProvider, Mockito.never()).newStream();
4949
assertThat(contentLength).isEqualTo(10L);
5050
assertThat(request.firstMatchingHeader(CONTENT_LENGTH)).isEmpty();
5151
assertThat(request.firstMatchingHeader(X_AMZ_DECODED_CONTENT_LENGTH)).contains("10");
5252
}
5353

5454
@Test
55-
void moveContentLength_contentLengthPresent_shouldNotInvokeNewStream() {
55+
void computeAndMoveContentLength_contentLengthPresent_shouldNotInvokeNewStream() {
5656
SdkHttpRequest.Builder request = SdkHttpRequest.builder()
5757
.appendHeader(CONTENT_LENGTH, "10");
5858

5959
ContentStreamProvider streamProvider = Mockito.mock(ContentStreamProvider.class);
60-
long contentLength = SignerUtils.moveContentLength(request, streamProvider);
60+
long contentLength = SignerUtils.computeAndMoveContentLength(request, streamProvider);
6161
Mockito.verify(streamProvider, Mockito.never()).newStream();
6262
assertThat(contentLength).isEqualTo(10L);
6363
assertThat(request.firstMatchingHeader(CONTENT_LENGTH)).isEmpty();
@@ -66,43 +66,43 @@ void moveContentLength_contentLengthPresent_shouldNotInvokeNewStream() {
6666

6767
@ParameterizedTest
6868
@MethodSource("streams")
69-
void moveContentLength_contentLengthNotPresent_shouldInvokeNewStream(InputStream inputStream, long expectedLength) {
69+
void computeAndMoveContentLength_contentLengthNotPresent_shouldInvokeNewStream(InputStream inputStream, long expectedLength) {
7070
SdkHttpRequest.Builder request = SdkHttpRequest.builder();
7171

7272
ContentStreamProvider streamProvider = Mockito.mock(ContentStreamProvider.class);
7373
Mockito.when(streamProvider.newStream()).thenReturn(inputStream);
7474

75-
long contentLength = SignerUtils.moveContentLength(request, streamProvider);
75+
long contentLength = SignerUtils.computeAndMoveContentLength(request, streamProvider);
7676
Mockito.verify(streamProvider, Mockito.times(1)).newStream();
7777
assertThat(contentLength).isEqualTo(expectedLength);
7878
assertThat(request.firstMatchingHeader(CONTENT_LENGTH)).isEmpty();
7979
assertThat(request.firstMatchingHeader(X_AMZ_DECODED_CONTENT_LENGTH)).contains(String.valueOf(expectedLength));
8080
}
8181

8282
@Test
83-
void moveContentLength_async_decodedContentLengthPresent_shouldNotSubscribeToPublisher() {
83+
void computeAndMoveContentLength_async_decodedContentLengthPresent_shouldNotSubscribeToPublisher() {
8484

8585
SdkHttpRequest.Builder request = SdkHttpRequest.builder()
8686
.appendHeader(X_AMZ_DECODED_CONTENT_LENGTH, "10")
8787
.appendHeader(CONTENT_LENGTH, "10");
8888

8989
Publisher<ByteBuffer> contentPublisher = Mockito.spy(Flowable.empty());
9090

91-
SignerUtils.moveContentLength(request, contentPublisher).join();
91+
SignerUtils.computeAndMoveContentLength(request, contentPublisher).join();
9292
Mockito.verify(contentPublisher, Mockito.never()).subscribe(Mockito.any(Subscriber.class));
9393

9494
assertThat(request.firstMatchingHeader(CONTENT_LENGTH)).isEmpty();
9595
assertThat(request.firstMatchingHeader(X_AMZ_DECODED_CONTENT_LENGTH)).contains("10");
9696
}
9797

9898
@Test
99-
void moveContentLength_async_contentLengthPresent_shouldNotSubscribeToPublisher() {
99+
void computeAndMoveContentLength_async_contentLengthPresent_shouldNotSubscribeToPublisher() {
100100
SdkHttpRequest.Builder request = SdkHttpRequest.builder()
101101
.appendHeader(CONTENT_LENGTH, "10");
102102

103103
Publisher<ByteBuffer> contentPublisher = Mockito.spy(Flowable.empty());
104104

105-
SignerUtils.moveContentLength(request, contentPublisher).join();
105+
SignerUtils.computeAndMoveContentLength(request, contentPublisher).join();
106106
Mockito.verify(contentPublisher, Mockito.never()).subscribe(Mockito.any(Subscriber.class));
107107

108108
assertThat(request.firstMatchingHeader(CONTENT_LENGTH)).isEmpty();
@@ -111,14 +111,14 @@ void moveContentLength_async_contentLengthPresent_shouldNotSubscribeToPublisher(
111111

112112
@ParameterizedTest
113113
@MethodSource("publishers")
114-
void moveContentLength_contentLengthNotPresent_shouldInvokeSubscribe(Flowable<ByteBuffer> publisher, long expectedLength) {
114+
void computeAndMoveContentLength_contentLengthNotPresent_shouldInvokeSubscribe(Flowable<ByteBuffer> publisher, long expectedLength) {
115115
SdkHttpRequest.Builder request = SdkHttpRequest.builder();
116116

117117
if (publisher != null) {
118118
publisher = Mockito.spy(publisher);
119119
}
120120

121-
SignerUtils.moveContentLength(request, publisher).join();
121+
SignerUtils.computeAndMoveContentLength(request, publisher).join();
122122

123123
if (publisher != null) {
124124
Mockito.verify(publisher, Mockito.times(1)).subscribe(Mockito.any(Subscriber.class));

0 commit comments

Comments
 (0)