Skip to content

Commit aa51b12

Browse files
dagnirdagnir
authored
Fix ChecksumIntegrationTest (#4798)
* Fix ChecksumIntegrationTest - Some tests specificy a part number, but CRT may do a range get under the hood. S3 will throw an error if both a range and part number are specified. This is an issue that needs to be fixed in CRT, but part number is not required in this test, so removing it. - Rename test file to CrtCheckIntegrationTest so it gets added to CRT test suite * Revert "Revert "Wait until response body or error body received to process request (#4786)"" This reverts commit 045bcc4.
1 parent 3568ec6 commit aa51b12

File tree

6 files changed

+237
-42
lines changed

6 files changed

+237
-42
lines changed

‎pom.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@
119119
<rxjava.version>2.2.21</rxjava.version>
120120
<commons-codec.verion>1.15</commons-codec.verion>
121121
<jmh.version>1.29</jmh.version>
122-
<awscrt.version>0.29.1</awscrt.version>
122+
<awscrt.version>0.29.2</awscrt.version>
123123

124124
<!--Test dependencies -->
125125
<junit5.version>5.10.0</junit5.version>

‎services/s3/src/it/java/software/amazon/awssdk/services/s3/crt/ChecksumIntegrationTest.java renamed to ‎services/s3/src/it/java/software/amazon/awssdk/services/s3/crt/CrtChecksumIntegrationTest.java

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,11 @@
2222
import java.nio.file.Files;
2323
import org.junit.jupiter.api.AfterAll;
2424
import org.junit.jupiter.api.BeforeAll;
25-
import org.junit.jupiter.api.Disabled;
2625
import org.junit.jupiter.api.Test;
2726
import software.amazon.awssdk.core.ResponseBytes;
2827
import software.amazon.awssdk.core.async.AsyncRequestBody;
2928
import software.amazon.awssdk.core.async.AsyncResponseTransformer;
30-
import software.amazon.awssdk.crt.CrtResource;
29+
import software.amazon.awssdk.core.checksums.Algorithm;
3130
import software.amazon.awssdk.services.s3.S3AsyncClient;
3231
import software.amazon.awssdk.services.s3.S3IntegrationTestBase;
3332
import software.amazon.awssdk.services.s3.internal.crt.S3CrtAsyncClient;
@@ -37,15 +36,20 @@
3736
import software.amazon.awssdk.services.s3.model.PutObjectTaggingResponse;
3837
import software.amazon.awssdk.services.s3.model.Tag;
3938
import software.amazon.awssdk.services.s3.model.Tagging;
39+
import software.amazon.awssdk.services.s3.utils.ChecksumUtils;
4040
import software.amazon.awssdk.testutils.RandomTempFile;
4141
import software.amazon.awssdk.testutils.service.AwsTestBase;
4242

43-
public class ChecksumIntegrationTest extends S3IntegrationTestBase {
44-
private static final String TEST_BUCKET = temporaryBucketName(ChecksumIntegrationTest.class);
43+
public class CrtChecksumIntegrationTest extends S3IntegrationTestBase {
44+
private static final String TEST_BUCKET = temporaryBucketName(CrtChecksumIntegrationTest.class);
4545
private static final String TEST_KEY = "10mib_file.dat";
4646
private static final int OBJ_SIZE = 10 * 1024 * 1024;
4747

4848
private static RandomTempFile testFile;
49+
50+
private static String testFileSha1;
51+
private static String testFileCrc32;
52+
4953
private static S3AsyncClient s3Crt;
5054

5155
@BeforeAll
@@ -54,10 +58,15 @@ public static void setup() throws Exception {
5458
S3IntegrationTestBase.createBucket(TEST_BUCKET);
5559

5660
testFile = new RandomTempFile(TEST_KEY, OBJ_SIZE);
61+
testFileSha1 = ChecksumUtils.calculatedChecksum(testFile.toPath(), Algorithm.SHA1);
62+
testFileCrc32 = ChecksumUtils.calculatedChecksum(testFile.toPath(), Algorithm.CRC32);
5763

5864
s3Crt = S3CrtAsyncClient.builder()
5965
.credentialsProvider(AwsTestBase.CREDENTIALS_PROVIDER_CHAIN)
6066
.region(S3IntegrationTestBase.DEFAULT_REGION)
67+
// make sure we don't do a multipart upload, it will mess with validation against the precomputed
68+
// checksums above
69+
.thresholdInBytes(2L * OBJ_SIZE)
6170
.build();
6271
}
6372

@@ -72,37 +81,33 @@ public static void teardown() throws IOException {
7281
@Test
7382
void noChecksumCustomization_crc32ShouldBeUsed() {
7483
AsyncRequestBody body = AsyncRequestBody.fromFile(testFile.toPath());
75-
PutObjectResponse putObjectResponse =
76-
s3Crt.putObject(r -> r.bucket(TEST_BUCKET).key(TEST_KEY), body).join();
77-
assertThat(putObjectResponse).isNotNull();
84+
s3Crt.putObject(r -> r.bucket(TEST_BUCKET).key(TEST_KEY), body).join();
7885

7986
ResponseBytes<GetObjectResponse> getObjectResponseResponseBytes =
80-
s3Crt.getObject(r -> r.bucket(TEST_BUCKET).key(TEST_KEY).partNumber(1), AsyncResponseTransformer.toBytes()).join();
87+
s3Crt.getObject(r -> r.bucket(TEST_BUCKET).key(TEST_KEY), AsyncResponseTransformer.toBytes()).join();
8188
String getObjectChecksum = getObjectResponseResponseBytes.response().checksumCRC32();
82-
assertThat(getObjectChecksum).isNotNull();
89+
assertThat(getObjectChecksum).isEqualTo(testFileCrc32);
8390
}
8491

8592
@Test
8693
void putObject_checksumProvidedInRequest_shouldTakePrecendence() {
8794
AsyncRequestBody body = AsyncRequestBody.fromFile(testFile.toPath());
88-
PutObjectResponse putObjectResponse =
89-
s3Crt.putObject(r -> r.bucket(TEST_BUCKET).key(TEST_KEY).checksumAlgorithm(ChecksumAlgorithm.SHA1), body).join();
90-
assertThat(putObjectResponse).isNotNull();
95+
s3Crt.putObject(r -> r.bucket(TEST_BUCKET).key(TEST_KEY).checksumAlgorithm(ChecksumAlgorithm.SHA1), body).join();
9196

9297
ResponseBytes<GetObjectResponse> getObjectResponseResponseBytes =
93-
s3Crt.getObject(r -> r.bucket(TEST_BUCKET).key(TEST_KEY).partNumber(1), AsyncResponseTransformer.toBytes()).join();
98+
s3Crt.getObject(r -> r.bucket(TEST_BUCKET).key(TEST_KEY), AsyncResponseTransformer.toBytes()).join();
9499
String getObjectChecksum = getObjectResponseResponseBytes.response().checksumSHA1();
95-
assertThat(getObjectChecksum).isNotNull();
100+
assertThat(getObjectChecksum).isEqualTo(testFileSha1);
96101
}
97102

98103
@Test
99104
void checksumDisabled_shouldNotPerformChecksumValidationByDefault() {
100105

101106
try (S3AsyncClient s3Crt = S3CrtAsyncClient.builder()
102-
.credentialsProvider(AwsTestBase.CREDENTIALS_PROVIDER_CHAIN)
103-
.region(S3IntegrationTestBase.DEFAULT_REGION)
104-
.checksumValidationEnabled(Boolean.FALSE)
105-
.build()) {
107+
.credentialsProvider(AwsTestBase.CREDENTIALS_PROVIDER_CHAIN)
108+
.region(S3IntegrationTestBase.DEFAULT_REGION)
109+
.checksumValidationEnabled(Boolean.FALSE)
110+
.build()) {
106111
AsyncRequestBody body = AsyncRequestBody.fromFile(testFile.toPath());
107112
PutObjectResponse putObjectResponse =
108113
s3Crt.putObject(r -> r.bucket(TEST_BUCKET).key(TEST_KEY), body).join();

‎services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapter.java

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,13 @@ public final class S3CrtResponseHandlerAdapter implements S3MetaRequestResponseH
4545

4646
private final SimplePublisher<ByteBuffer> responsePublisher = new SimplePublisher<>();
4747

48-
private final SdkHttpResponse.Builder respBuilder = SdkHttpResponse.builder();
48+
private final SdkHttpResponse.Builder initialHeadersResponse = SdkHttpResponse.builder();
4949
private volatile S3MetaRequest metaRequest;
5050

5151
private final PublisherListener<S3MetaRequestProgress> progressListener;
5252

53+
private volatile boolean responseHandlingInitiated;
54+
5355
public S3CrtResponseHandlerAdapter(CompletableFuture<Void> executeFuture,
5456
SdkAsyncHttpResponseHandler responseHandler,
5557
PublisherListener<S3MetaRequestProgress> progressListener) {
@@ -60,17 +62,17 @@ public S3CrtResponseHandlerAdapter(CompletableFuture<Void> executeFuture,
6062

6163
@Override
6264
public void onResponseHeaders(int statusCode, HttpHeader[] headers) {
63-
for (HttpHeader h : headers) {
64-
respBuilder.appendHeader(h.getName(), h.getValue());
65-
}
66-
67-
respBuilder.statusCode(statusCode);
68-
responseHandler.onHeaders(respBuilder.build());
69-
responseHandler.onStream(responsePublisher);
65+
// Note, we cannot call responseHandler.onHeaders() here because the response status code and headers may not represent
66+
// whether the request has succeeded or not (e.g. if this is for a HeadObject call that CRT calls under the hood). We
67+
// need to rely on onResponseBody/onFinished being called to determine this.
68+
populateSdkHttpResponse(initialHeadersResponse, statusCode, headers);
7069
}
7170

7271
@Override
7372
public int onResponseBody(ByteBuffer bodyBytesIn, long objectRangeStart, long objectRangeEnd) {
73+
// See reasoning in onResponseHeaders for why we call this here and not there.
74+
initiateResponseHandling(initialHeadersResponse.build());
75+
7476
if (bodyBytesIn == null) {
7577
failResponseHandlerAndFuture(new IllegalStateException("ByteBuffer delivered is null"));
7678
return 0;
@@ -98,6 +100,10 @@ public void onFinished(S3FinishedResponseContext context) {
98100
if (crtCode != CRT.AWS_CRT_SUCCESS) {
99101
handleError(context);
100102
} else {
103+
// onResponseBody() is not invoked for responses with no content, so we may not have invoked
104+
// SdkAsyncHttpResponseHandler#onHeaders yet.
105+
// See also reasoning in onResponseHeaders for why we call this here and not there.
106+
initiateResponseHandling(initialHeadersResponse.build());
101107
onSuccessfulResponseComplete();
102108
}
103109
}
@@ -127,10 +133,14 @@ public void cancelRequest() {
127133

128134
private void handleError(S3FinishedResponseContext context) {
129135
int crtCode = context.getErrorCode();
136+
HttpHeader[] headers = context.getErrorHeaders();
130137
int responseStatus = context.getResponseStatus();
131138
byte[] errorPayload = context.getErrorPayload();
132139

133140
if (isErrorResponse(responseStatus) && errorPayload != null) {
141+
SdkHttpResponse.Builder errorResponse = populateSdkHttpResponse(SdkHttpResponse.builder(),
142+
responseStatus, headers);
143+
initiateResponseHandling(errorResponse.build());
134144
onErrorResponseComplete(errorPayload);
135145
} else {
136146
Throwable cause = context.getCause();
@@ -142,6 +152,14 @@ private void handleError(S3FinishedResponseContext context) {
142152
}
143153
}
144154

155+
private void initiateResponseHandling(SdkHttpResponse response) {
156+
if (!responseHandlingInitiated) {
157+
responseHandlingInitiated = true;
158+
responseHandler.onHeaders(response);
159+
responseHandler.onStream(responsePublisher);
160+
}
161+
}
162+
145163
private void onErrorResponseComplete(byte[] errorPayload) {
146164
responsePublisher.send(ByteBuffer.wrap(errorPayload))
147165
.thenRun(responsePublisher::complete)
@@ -176,6 +194,17 @@ public void onProgress(S3MetaRequestProgress progress) {
176194
this.progressListener.subscriberOnNext(progress);
177195
}
178196

197+
private static SdkHttpResponse.Builder populateSdkHttpResponse(SdkHttpResponse.Builder respBuilder,
198+
int statusCode, HttpHeader[] headers) {
199+
if (headers != null) {
200+
for (HttpHeader h : headers) {
201+
respBuilder.appendHeader(h.getName(), h.getValue());
202+
}
203+
}
204+
respBuilder.statusCode(statusCode);
205+
return respBuilder;
206+
}
207+
179208
private static class NoOpPublisherListener implements PublisherListener<S3MetaRequestProgress> {
180209
}
181210
}

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/crt/CrtDownloadErrorTest.java

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
/*
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
16+
package software.amazon.awssdk.services.s3.crt;
17+
18+
import static org.assertj.core.api.Assertions.assertThat;
19+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
20+
21+
import com.github.tomakehurst.wiremock.WireMockServer;
22+
import com.github.tomakehurst.wiremock.client.WireMock;
23+
import com.github.tomakehurst.wiremock.core.WireMockConfiguration;
24+
import java.net.URI;
25+
import java.nio.charset.StandardCharsets;
26+
import org.junit.jupiter.api.AfterAll;
27+
import org.junit.jupiter.api.AfterEach;
28+
import org.junit.jupiter.api.BeforeAll;
29+
import org.junit.jupiter.api.Test;
30+
import software.amazon.awssdk.core.async.AsyncResponseTransformer;
31+
import software.amazon.awssdk.crt.Log;
32+
import software.amazon.awssdk.regions.Region;
33+
import software.amazon.awssdk.services.s3.S3AsyncClient;
34+
import software.amazon.awssdk.services.s3.model.S3Exception;
35+
36+
public class CrtDownloadErrorTest {
37+
private static final String BUCKET = "my-bucket";
38+
private static final String KEY = "my-key";
39+
private static final WireMockServer WM = new WireMockServer(WireMockConfiguration.wireMockConfig().dynamicPort());
40+
private S3AsyncClient s3;
41+
42+
@BeforeAll
43+
public static void setup() {
44+
WM.start();
45+
// Execute this statement before constructing the SDK service client.
46+
Log.initLoggingToStdout(Log.LogLevel.Trace);
47+
}
48+
49+
@AfterAll
50+
public static void teardown() {
51+
WM.stop();
52+
}
53+
54+
@AfterEach
55+
public void methodTeardown() {
56+
if (s3 != null) {
57+
s3.close();
58+
}
59+
s3 = null;
60+
}
61+
62+
@Test
63+
public void getObject_headObjectOk_getObjectThrows_operationThrows() {
64+
s3 = S3AsyncClient.crtBuilder()
65+
.endpointOverride(URI.create("http://localhost:" + WM.port()))
66+
.forcePathStyle(true)
67+
.region(Region.US_EAST_1)
68+
.build();
69+
70+
String path = String.format("/%s/%s", BUCKET, KEY);
71+
72+
WM.stubFor(WireMock.head(WireMock.urlPathEqualTo(path))
73+
.willReturn(WireMock.aResponse()
74+
.withStatus(200)
75+
.withHeader("ETag", "etag")
76+
.withHeader("Content-Length", "5")));
77+
78+
String errorContent = ""
79+
+ "<Error>\n"
80+
+ " <Code>AccessDenied</Code>\n"
81+
+ " <Message>User does not have permission</Message>\n"
82+
+ " <RequestId>request-id</RequestId>\n"
83+
+ " <HostId>host-id</HostId>\n"
84+
+ "</Error>";
85+
WM.stubFor(WireMock.get(WireMock.urlPathEqualTo(path))
86+
.willReturn(WireMock.aResponse()
87+
.withStatus(403)
88+
.withBody(errorContent)));
89+
90+
assertThatThrownBy(s3.getObject(r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes())::join)
91+
.hasCauseInstanceOf(S3Exception.class)
92+
.hasMessageContaining("User does not have permission")
93+
.hasMessageContaining("Status Code: 403");
94+
}
95+
96+
@Test
97+
public void getObject_headObjectOk_getObjectOk_operationSucceeds() {
98+
s3 = S3AsyncClient.crtBuilder()
99+
.endpointOverride(URI.create("http://localhost:" + WM.port()))
100+
.forcePathStyle(true)
101+
.region(Region.US_EAST_1)
102+
.build();
103+
104+
String path = String.format("/%s/%s", BUCKET, KEY);
105+
106+
byte[] content = "hello".getBytes(StandardCharsets.UTF_8);
107+
108+
WM.stubFor(WireMock.head(WireMock.urlPathEqualTo(path))
109+
.willReturn(WireMock.aResponse()
110+
.withStatus(200)
111+
.withHeader("ETag", "etag")
112+
.withHeader("Content-Length", Integer.toString(content.length))));
113+
WM.stubFor(WireMock.get(WireMock.urlPathEqualTo(path))
114+
.willReturn(WireMock.aResponse()
115+
.withStatus(200)
116+
.withHeader("Content-Type", "text/plain")
117+
.withBody(content)));
118+
119+
String objectContent = s3.getObject(r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes())
120+
.join()
121+
.asUtf8String();
122+
123+
assertThat(objectContent.getBytes(StandardCharsets.UTF_8)).isEqualTo(content);
124+
}
125+
126+
@Test
127+
public void getObject_headObjectThrows_operationThrows() {
128+
s3 = S3AsyncClient.crtBuilder()
129+
.endpointOverride(URI.create("http://localhost:" + WM.port()))
130+
.forcePathStyle(true)
131+
.region(Region.US_EAST_1)
132+
.build();
133+
134+
String path = String.format("/%s/%s", BUCKET, KEY);
135+
136+
137+
WM.stubFor(WireMock.head(WireMock.urlPathEqualTo(path))
138+
.willReturn(WireMock.aResponse()
139+
.withStatus(403)));
140+
141+
assertThatThrownBy(s3.getObject(r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes())::join)
142+
.hasCauseInstanceOf(S3Exception.class)
143+
.hasMessageContaining("Status Code: 403");
144+
}
145+
}

0 commit comments

Comments
 (0)