Skip to content

Fix presigned url multipart download for empty objects #6952

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jencymaryjoseph wants to merge 1 commit into
base: feature/master/pre-signed-url-getobject
Choose a base branch
from
Open

Fix presigned url multipart download for empty objects #6952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions ...n/awssdk/services/s3/presignedurl/AsyncPresignedUrlExtensionMultipartIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/
package software.amazon.awssdk.services.s3.presignedurl;

import org.junit.jupiter.api.BeforeAll;
import software.amazon.awssdk.services.s3.S3AsyncClient;


public class AsyncPresignedUrlExtensionMultipartIntegrationTest extends AsyncPresignedUrlExtensionTestSuite {

@BeforeAll
static void setUpIntegrationTest() {
S3AsyncClient s3AsyncClient = s3AsyncClientBuilder()
.multipartEnabled(true)
.build();
presignedUrlExtension = s3AsyncClient.presignedUrlExtension();
}

@Override
protected S3AsyncClient createS3AsyncClient() {
return s3AsyncClientBuilder().build();
}
}
40 changes: 40 additions & 0 deletions .../software/amazon/awssdk/services/s3/presignedurl/AsyncPresignedUrlExtensionTestSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import static org.apache.commons.lang3.RandomStringUtils.randomAscii;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;

import java.io.ByteArrayInputStream;
import java.net.URL;
Expand Down Expand Up @@ -47,6 +48,7 @@
import software.amazon.awssdk.services.s3.model.DeleteObjectRequest;
import software.amazon.awssdk.services.s3.model.GetObjectResponse;
import software.amazon.awssdk.services.s3.model.PutObjectRequest;
import software.amazon.awssdk.services.s3.model.S3Exception;
import software.amazon.awssdk.services.s3.presigner.S3Presigner;
import software.amazon.awssdk.services.s3.presigner.model.PresignedGetObjectRequest;
import software.amazon.awssdk.services.s3.presignedurl.model.PresignedUrlDownloadRequest;
Expand Down Expand Up @@ -240,6 +242,44 @@ public void close() {}
}
}

@Test
void getObject_emptyObject_toBytes_shouldSucceed() throws Exception {
String emptyKey = uploadTestObject("empty-object-bytes", "");

PresignedUrlDownloadRequest request = createRequestForKey(emptyKey);
ResponseBytes<GetObjectResponse> response =
presignedUrlExtension.getObject(request, AsyncResponseTransformer.toBytes())
.get(30, TimeUnit.SECONDS);

assertThat(response.asByteArray()).isEmpty();
assertThat(response.response().contentLength()).isEqualTo(0L);
}

@Test
void getObject_emptyObject_toFile_shouldSucceed() throws Exception {
String emptyKey = uploadTestObject("empty-object-file", "");

PresignedUrlDownloadRequest request = createRequestForKey(emptyKey);
Path downloadFile = temporaryFolder.resolve("empty-download-" + UUID.randomUUID() + ".bin");
GetObjectResponse response =
presignedUrlExtension.getObject(request, downloadFile)
.get(30, TimeUnit.SECONDS);

assertThat(downloadFile).exists();
assertThat(downloadFile.toFile().length()).isEqualTo(0L);
}

@Test
void getObject_emptyObject_withRange_shouldThrow416() throws Exception {
String emptyKey = uploadTestObject("empty-object-range", "");

PresignedUrlDownloadRequest request = createRequestForKey(emptyKey, "bytes=0-1024");

assertThatThrownBy(() -> presignedUrlExtension.getObject(request, AsyncResponseTransformer.toBytes())
.get(30, TimeUnit.SECONDS))
.hasCauseInstanceOf(S3Exception.class);
}

static Stream<Arguments> basicFunctionalityTestData() {
return Stream.of(
Arguments.of("getObject_withValidUrl_returnsContent",
Expand Down
13 changes: 10 additions & 3 deletions ...sdk/services/s3/internal/multipart/ParallelPresignedUrlMultipartDownloaderSubscriber.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,18 @@ private void sendFirstRequest(AsyncResponseTransformer<GetObjectResponse, GetObj

inFlightRequests.put(0, response);
inFlightRequestsNum.incrementAndGet();
CompletableFutureUtils.forwardExceptionTo(resultFuture, response);

response.whenComplete((res, error) -> {
if (error != null || isCompletedExceptionally.get()) {
handlePartError(error, 0);
if (error != null) {
if (PresignedUrlDownloadHelper.isRangeNotSatisfiable(error)) {
resultFuture.completeExceptionally(
new PresignedUrlDownloadHelper.EmptyObjectRangeNotSatisfiableException(error));
synchronized (subscriptionLock) {
subscription.cancel();
}
} else {
handlePartError(error, 0);
}
return;
}

Expand Down
50 changes: 49 additions & 1 deletion ...ava/software/amazon/awssdk/services/s3/internal/multipart/PresignedUrlDownloadHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@
package software.amazon.awssdk.services.s3.internal.multipart;

import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.core.SplittingTransformerConfiguration;
import software.amazon.awssdk.core.async.AsyncResponseTransformer;
import software.amazon.awssdk.core.exception.SdkClientException;
import software.amazon.awssdk.services.s3.S3AsyncClient;
import software.amazon.awssdk.services.s3.model.GetObjectResponse;
import software.amazon.awssdk.services.s3.model.S3Exception;
import software.amazon.awssdk.services.s3.presignedurl.AsyncPresignedUrlExtension;
import software.amazon.awssdk.services.s3.presignedurl.model.PresignedUrlDownloadRequest;
import software.amazon.awssdk.utils.Logger;
Expand Down Expand Up @@ -60,6 +62,33 @@ public <T> CompletableFuture<T> downloadObject(
return asyncPresignedUrlExtension.getObject(presignedRequest, asyncResponseTransformer);
}

CompletableFuture<T> resultFuture = new CompletableFuture<>();
doMultipartDownload(presignedRequest, asyncResponseTransformer)
.whenComplete((result, error) -> {
Throwable cause = error instanceof CompletionException ? error.getCause() : error;
if (cause instanceof EmptyObjectRangeNotSatisfiableException) {
log.debug(() -> "Received 416 on first request, falling back to non-range GET for empty object");
asyncPresignedUrlExtension.getObject(presignedRequest, asyncResponseTransformer)
.whenComplete((r, e) -> {
if (e != null) {
resultFuture.completeExceptionally(e);
} else {
resultFuture.complete(r);
}
});
} else if (error != null) {
resultFuture.completeExceptionally(error);
} else {
resultFuture.complete(result);
}
});
return resultFuture;
}

private <T> CompletableFuture<T> doMultipartDownload(
PresignedUrlDownloadRequest presignedRequest,
AsyncResponseTransformer<GetObjectResponse, T> asyncResponseTransformer) {

SplittingTransformerConfiguration splittingConfig = SplittingTransformerConfiguration.builder()
.bufferSizeInBytes(bufferSizeInBytes)
.build();
Expand Down Expand Up @@ -112,4 +141,23 @@ static SdkClientException missingContentRangeHeader() {
static SdkClientException invalidContentLength() {
return SdkClientException.create("Invalid or missing Content-Length in response");
}
}

/**
* Returns true if the error is a 416 Range Not Satisfiable response from S3.
* Used by subscribers to detect empty object responses on the first range request.
*/
static boolean isRangeNotSatisfiable(Throwable error) {
Throwable cause = error instanceof CompletionException ? error.getCause() : error;
return cause instanceof S3Exception && ((S3Exception) cause).statusCode() == 416;
}

/**
* Marker exception wrapping a 416 on the first range request, signaling an empty object.
* Used to distinguish from 416 errors on subsequent requests which should propagate as failures.
*/
static class EmptyObjectRangeNotSatisfiableException extends RuntimeException {
EmptyObjectRangeNotSatisfiableException(Throwable cause) {
super("Object is empty (416 on first range request)", cause);
}
}
}
11 changes: 10 additions & 1 deletion ...azon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriber.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,16 @@ private void makeRangeRequest(int partIndex,
.getObject(partRequest, asyncResponseTransformer)
.whenComplete((response, error) -> {
if (error != null) {
handleError(error);
if (partIndex == 0 && PresignedUrlDownloadHelper.isRangeNotSatisfiable(error)) {
log.debug(() -> "Received 416 on first range request, object is empty");
resultFuture.completeExceptionally(
new PresignedUrlDownloadHelper.EmptyObjectRangeNotSatisfiableException(error));
synchronized (lock) {
subscription.cancel();
}
} else {
handleError(error);
}
return;
}
if (validatePart(response, partIndex, asyncResponseTransformer)) {
Expand Down
63 changes: 63 additions & 0 deletions ...services/s3/internal/multipart/ParallelPresignedUrlMultipartDownloaderSubscriberTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package software.amazon.awssdk.services.s3.internal.multipart;

import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
import static com.github.tomakehurst.wiremock.client.WireMock.absent;
import static com.github.tomakehurst.wiremock.client.WireMock.get;
import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor;
import static com.github.tomakehurst.wiremock.client.WireMock.matching;
Expand Down Expand Up @@ -234,6 +235,68 @@ void onNext_withNullTransformer_shouldThrowNPE() {
.isInstanceOf(NullPointerException.class);
}

@Test
void emptyObject_416OnFirstRequest_shouldFallbackToNonRangeGet() {
stubFor(get(urlEqualTo(PRESIGNED_URL_PATH))
.withHeader("Range", matching("bytes=.*"))
.willReturn(aResponse()
.withStatus(416)
.withBody("<Error><Code>InvalidRange</Code>"
+ "<Message>The requested range is not satisfiable</Message></Error>")));
stubFor(get(urlEqualTo(PRESIGNED_URL_PATH))
.withHeader("Range", absent())
.willReturn(aResponse()
.withStatus(200)
.withHeader("Content-Length", "0")
.withHeader("ETag", "\"empty-etag\"")
.withBody(new byte[0])));

tempFile = createTempFileUnchecked();
PresignedUrlDownloadRequest request = PresignedUrlDownloadRequest.builder()
.presignedUrl(presignedUrl)
.build();

s3AsyncClient.presignedUrlExtension()
.getObject(request, AsyncResponseTransformer.toFile(tempFile))
.join();

assertThat(tempFile.toFile()).exists();
assertThat(tempFile.toFile().length()).isEqualTo(0L);
}

@Test
void multipartObject_416OnSecondRequest_shouldFailWithError() {
stubFor(get(urlEqualTo(PRESIGNED_URL_PATH))
.inScenario("416-on-second")
.whenScenarioStateIs("Started")
.withHeader("Range", matching("bytes=0-15"))
.willReturn(aResponse()
.withStatus(206)
.withHeader("Content-Length", "16")
.withHeader("Content-Range", "bytes 0-15/32")
.withHeader("ETag", "\"test-etag\"")
.withBody(Arrays.copyOfRange(TEST_DATA, 0, 16)))
.willSetStateTo("first-done"));

stubFor(get(urlEqualTo(PRESIGNED_URL_PATH))
.inScenario("416-on-second")
.whenScenarioStateIs("first-done")
.willReturn(aResponse()
.withStatus(416)
.withBody("<Error><Code>InvalidRange</Code>"
+ "<Message>The requested range is not satisfiable</Message></Error>")));

tempFile = createTempFileUnchecked();
PresignedUrlDownloadRequest request = PresignedUrlDownloadRequest.builder()
.presignedUrl(presignedUrl)
.build();

assertThatThrownBy(() -> s3AsyncClient.presignedUrlExtension()
.getObject(request, AsyncResponseTransformer.toFile(tempFile))
.join())
.isInstanceOf(CompletionException.class);
}

private static Path createTempFile() throws IOException {
Path path = Files.createTempFile("parallel-test-" + UUID.randomUUID(), ".tmp");
Files.deleteIfExists(path);
Expand Down
79 changes: 79 additions & 0 deletions ...services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriberWiremockTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package software.amazon.awssdk.services.s3.internal.multipart;

import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
import static com.github.tomakehurst.wiremock.client.WireMock.absent;
import static com.github.tomakehurst.wiremock.client.WireMock.get;
import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
Expand Down Expand Up @@ -260,6 +261,84 @@ void onNext_withNullTransformer_shouldThrowException() {
.hasMessageContaining("onNext must not be called with null asyncResponseTransformer");
}

@ParameterizedTest(name = "presignedUrlDownload_emptyObject_shouldFallbackToNonRangeGet [{0}]")
@MethodSource("transformerTypes")
@SuppressWarnings("unchecked")
void presignedUrlDownload_emptyObject_shouldFallbackToNonRangeGet(String transformerType) throws IOException {
stubFor(get(urlEqualTo(PRESIGNED_URL_PATH))
.withHeader("Range", matching("bytes=.*"))
.willReturn(aResponse()
.withStatus(416)
.withBody("<Error><Code>InvalidRange</Code>"
+ "<Message>The requested range is not satisfiable</Message></Error>")));
stubFor(get(urlEqualTo(PRESIGNED_URL_PATH))
.withHeader("Range", absent())
.willReturn(aResponse()
.withStatus(200)
.withHeader("Content-Length", "0")
.withHeader("ETag", "\"empty-etag\"")
.withBody(new byte[0])));

PresignedUrlDownloadRequest request = PresignedUrlDownloadRequest.builder()
.presignedUrl(presignedUrl)
.build();
Object result = executeDownload(request, transformerType).join();
if ("toBytes".equals(transformerType)) {
ResponseBytes<GetObjectResponse> bytes = (ResponseBytes<GetObjectResponse>) result;
assertThat(bytes.asByteArray()).isEmpty();
} else {
assertThat(tempFile.toFile()).exists();
assertThat(Files.size(tempFile)).isEqualTo(0L);
}
}

@ParameterizedTest(name = "presignedUrlDownload_416OnSecondRequest_shouldFailWithError [{0}]")
@MethodSource("transformerTypes")
void presignedUrlDownload_416OnSecondRequest_shouldFailWithError(String transformerType) {
stubFor(get(urlEqualTo(PRESIGNED_URL_PATH))
.inScenario("416-on-second")
.whenScenarioStateIs("Started")
.withHeader("Range", matching("bytes=0-15"))
.willReturn(aResponse()
.withStatus(206)
.withHeader("Content-Length", "16")
.withHeader("Content-Range", "bytes 0-15/32")
.withHeader("ETag", "\"test-etag\"")
.withBody(Arrays.copyOfRange(TEST_DATA, 0, 16)))
.willSetStateTo("first-done"));

stubFor(get(urlEqualTo(PRESIGNED_URL_PATH))
.inScenario("416-on-second")
.whenScenarioStateIs("first-done")
.willReturn(aResponse()
.withStatus(416)
.withBody("<Error><Code>InvalidRange</Code>"
+ "<Message>The requested range is not satisfiable</Message></Error>")));

PresignedUrlDownloadRequest request = PresignedUrlDownloadRequest.builder()
.presignedUrl(presignedUrl)
.build();
assertThatThrownBy(() -> executeDownload(request, transformerType).join())
.hasRootCauseInstanceOf(S3Exception.class);
}

@ParameterizedTest(name = "presignedUrlDownload_withRangeHeader_emptyObject_shouldThrow416 [{0}]")
@MethodSource("transformerTypes")
void presignedUrlDownload_withRangeHeader_emptyObject_shouldThrow416(String transformerType) {
stubFor(get(urlEqualTo(PRESIGNED_URL_PATH))
.willReturn(aResponse()
.withStatus(416)
.withBody("<Error><Code>InvalidRange</Code>"
+ "<Message>The requested range is not satisfiable</Message></Error>")));

PresignedUrlDownloadRequest request = PresignedUrlDownloadRequest.builder()
.presignedUrl(presignedUrl)
.range("bytes=0-1024")
.build();
assertThatThrownBy(() -> executeDownload(request, transformerType).join())
.hasRootCauseInstanceOf(S3Exception.class);
}

@AfterEach
void cleanup() {
if (tempFile != null && Files.exists(tempFile)) {
Expand Down
Loading