diff --git a/.changes/next-release/bugfix-AWSSDKforJavav2-7d3f9c1.json b/.changes/next-release/bugfix-AWSSDKforJavav2-7d3f9c1.json new file mode 100644 index 000000000000..c1828f2ebbb7 --- /dev/null +++ b/.changes/next-release/bugfix-AWSSDKforJavav2-7d3f9c1.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "AWS SDK for Java v2", + "contributor": "", + "description": "`getObject(request, path)` and other streaming-to-file downloads now validate the destination file client-side, on both sync and async clients, before sending a request. A destination that can never be written - an existing file with the default `CREATE_NEW`, or a missing file with `WRITE_TO_POSITION` - now fails immediately with an `SdkClientException` (caused by `FileAlreadyExistsException` or `NoSuchFileException`) instead of after a wasted request." +} diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java index 70ff1e6aef50..fce889e21524 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java @@ -186,8 +186,10 @@ default String name() { /** * Creates an {@link AsyncResponseTransformer} that writes all the content to the given file. In the event of an error, the - * SDK will attempt to delete the file (whatever has been written to it so far). If the file already exists, an exception will - * be thrown. + * SDK will attempt to delete the file (whatever has been written to it so far). If the file already exists, the + * operation's returned future completes exceptionally with an + * {@link software.amazon.awssdk.core.exception.SdkClientException} caused by a + * {@link java.nio.file.FileAlreadyExistsException}, and no request is sent. * *

The file's parent directories must already exist. The SDK will not auto-create directories, and a * {@link java.nio.file.NoSuchFileException} will be thrown if they are missing. @@ -205,6 +207,13 @@ static AsyncResponseTransformer toFile(Path pa * Creates an {@link AsyncResponseTransformer} that writes all the content to the given file with the specified * {@link FileTransformerConfiguration}. * + *

Before any request is sent, the destination is checked against the configured + * {@link FileTransformerConfiguration.FileWriteOption}: {@code CREATE_NEW} requires that the file does not yet + * exist, and {@code WRITE_TO_POSITION} requires that it does. If the check fails, the operation's returned future + * completes exceptionally with an {@link software.amazon.awssdk.core.exception.SdkClientException} - caused by a + * {@link java.nio.file.FileAlreadyExistsException} or {@link java.nio.file.NoSuchFileException} - and no request is + * sent. + * *

The file's parent directories must already exist. The SDK will not auto-create directories, and a * {@link java.nio.file.NoSuchFileException} will be thrown if they are missing. * diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java index 752af9ea4c28..3b8825423c54 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java @@ -15,6 +15,7 @@ package software.amazon.awssdk.core.internal.async; +import static software.amazon.awssdk.core.FileTransformerConfiguration.FileWriteOption.CREATE_NEW; import static software.amazon.awssdk.core.FileTransformerConfiguration.FileWriteOption.CREATE_OR_APPEND_TO_EXISTING; import static software.amazon.awssdk.core.FileTransformerConfiguration.FileWriteOption.WRITE_TO_POSITION; import static software.amazon.awssdk.utils.FunctionalUtils.invokeSafely; @@ -24,6 +25,7 @@ import java.nio.ByteBuffer; import java.nio.channels.AsynchronousFileChannel; import java.nio.channels.CompletionHandler; +import java.nio.file.FileAlreadyExistsException; import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.OpenOption; @@ -139,6 +141,9 @@ private AsynchronousFileChannel createChannel(Path path) throws IOException { @Override public CompletableFuture prepare() { + // Throw synchronously, don't fail the future: the pipeline dispatches regardless of the future's state, so a + // failed future would still send the request. + validateDestination(); fileChannel = null; cf = new CompletableFuture<>(); cf.whenComplete((r, t) -> { @@ -151,6 +156,23 @@ public CompletableFuture prepare() { return cf.thenApply(ignored -> response); } + /** + * Rejects a destination the {@link #createChannel(Path)} open could never accept, before it costs a request; keep + * this in sync with that open's {@link OpenOption}s. The open stays authoritative, so a file that appears or + * disappears after this check still fails there. ({@code notExists} is not {@code !exists}: both are false when the + * filesystem is uncertain, which defers the verdict to the open.) + */ + private void validateDestination() { + if (configuration.fileWriteOption() == CREATE_NEW && Files.exists(path)) { + throw SdkClientException.create("Cannot write to the existing file " + path + " with file write option " + + CREATE_NEW, new FileAlreadyExistsException(path.toString())); + } + if (configuration.fileWriteOption() == WRITE_TO_POSITION && Files.notExists(path)) { + throw SdkClientException.create("Cannot write to the missing file " + path + " with file write option " + + WRITE_TO_POSITION, new NoSuchFileException(path.toString())); + } + } + @Override public void onResponse(ResponseT response) { this.response = response; diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisher.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisher.java index ba811809c536..388eb92638ab 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisher.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisher.java @@ -143,7 +143,15 @@ public void onResponse(T response) { } this.delegate = getDelegateTransformer(contentRangePair.get().left()); - CompletableFuture delegateFuture = delegate.prepare(); + CompletableFuture delegateFuture; + try { + delegateFuture = delegate.prepare(); + } catch (RuntimeException e) { + // prepare() validates the destination and can throw. Complete the part future here instead of letting + // the throw escape this callback, which would leave the part hanging. + handleError(e); + return; + } CompletableFutureUtils.forwardResultTo(delegateFuture, future); CompletableFutureUtils.forwardExceptionTo(future, delegateFuture); transformerCount.incrementAndGet(); diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/handler/BaseSyncClientHandler.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/handler/BaseSyncClientHandler.java index 2a4d20f88ed3..1a070614dc84 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/handler/BaseSyncClientHandler.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/handler/BaseSyncClientHandler.java @@ -35,6 +35,7 @@ import software.amazon.awssdk.core.internal.http.AmazonSyncHttpClient; import software.amazon.awssdk.core.internal.http.CombinedResponseHandler; import software.amazon.awssdk.core.internal.http.InterruptMonitor; +import software.amazon.awssdk.core.internal.sync.ValidatingResponseTransformer; import software.amazon.awssdk.core.metrics.CoreMetric; import software.amazon.awssdk.core.sync.RequestBody; import software.amazon.awssdk.core.sync.ResponseTransformer; @@ -60,6 +61,11 @@ public ReturnT ResponseTransformer responseTransformer) { return measureApiCall(executionParams, () -> { + // Let the transformer reject the call before it costs a request. Async does this in prepare(). + if (responseTransformer instanceof ValidatingResponseTransformer) { + ((ValidatingResponseTransformer) responseTransformer).validate(); + } + // Running beforeExecution interceptors and modifyRequest interceptors. ExecutionContext executionContext = invokeInterceptorsAndCreateExecutionContext(executionParams); diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/sync/ValidatingResponseTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/sync/ValidatingResponseTransformer.java new file mode 100644 index 000000000000..ca87b92d1534 --- /dev/null +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/sync/ValidatingResponseTransformer.java @@ -0,0 +1,36 @@ +/* + * 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.core.internal.sync; + +import software.amazon.awssdk.annotations.SdkInternalApi; +import software.amazon.awssdk.core.sync.ResponseTransformer; + +/** + * A {@link ResponseTransformer} that can reject a call before the SDK dispatches the request. This is the synchronous + * counterpart to {@link software.amazon.awssdk.core.async.AsyncResponseTransformer#prepare()}, kept internal so no + * public API is added. + * + * @param Type of unmarshalled response POJO. + */ +@SdkInternalApi +public interface ValidatingResponseTransformer extends ResponseTransformer { + + /** + * Called by the SDK immediately before the request is dispatched. Throwing fails the call without sending a + * request. + */ + void validate(); +} diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/sync/ResponseTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/sync/ResponseTransformer.java index 279c29c29c3c..c3cfc92a6ed5 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/sync/ResponseTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/sync/ResponseTransformer.java @@ -36,6 +36,7 @@ import software.amazon.awssdk.core.exception.SdkClientException; import software.amazon.awssdk.core.exception.SdkException; import software.amazon.awssdk.core.internal.http.InterruptMonitor; +import software.amazon.awssdk.core.internal.sync.ValidatingResponseTransformer; import software.amazon.awssdk.core.retry.RetryPolicy; import software.amazon.awssdk.http.AbortableInputStream; import software.amazon.awssdk.utils.IoUtils; @@ -109,7 +110,8 @@ default String name() { /** * Creates a response transformer that writes all response content to the specified file. If the file already exists - * then a {@link FileAlreadyExistsException} will be thrown. + * then a {@link FileAlreadyExistsException} will be thrown, as the cause of an {@link SdkClientException}, without + * sending a request. * *

The file's parent directories must already exist. The SDK will not auto-create directories, and a * {@link NoSuchFileException} will be thrown if they are missing. @@ -119,7 +121,17 @@ default String name() { * @return ResponseTransformer instance. */ static ResponseTransformer toFile(Path path) { - return new ResponseTransformer() { + return new ValidatingResponseTransformer() { + @Override + public void validate() { + // Reject a destination the Files.copy below could never write, before the request is sent. That copy + // uses CREATE_NEW semantics and stays authoritative, so a file appearing later still fails there. + if (Files.exists(path)) { + throw SdkClientException.create("Cannot write to the existing file " + path, + new FileAlreadyExistsException(path.toString())); + } + } + @Override public ResponseT transform(ResponseT response, AbortableInputStream inputStream) throws Exception { try { diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/client/handler/FileTransformerFailFastTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/client/handler/FileTransformerFailFastTest.java new file mode 100644 index 000000000000..4e788b817545 --- /dev/null +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/client/handler/FileTransformerFailFastTest.java @@ -0,0 +1,159 @@ +/* + * 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.core.client.handler; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import java.io.ByteArrayInputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.FileAlreadyExistsException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Optional; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import software.amazon.awssdk.core.SdkRequest; +import software.amazon.awssdk.core.SdkResponse; +import software.amazon.awssdk.core.async.AsyncResponseTransformer; +import software.amazon.awssdk.core.client.config.SdkClientConfiguration; +import software.amazon.awssdk.core.client.config.SdkClientOption; +import software.amazon.awssdk.core.exception.SdkClientException; +import software.amazon.awssdk.core.exception.SdkServiceException; +import software.amazon.awssdk.core.http.HttpResponseHandler; +import software.amazon.awssdk.core.protocol.VoidSdkResponse; +import software.amazon.awssdk.core.retry.RetryPolicy; +import software.amazon.awssdk.core.runtime.transform.Marshaller; +import software.amazon.awssdk.core.sync.ResponseTransformer; +import software.amazon.awssdk.http.AbortableInputStream; +import software.amazon.awssdk.http.ExecutableHttpRequest; +import software.amazon.awssdk.http.HttpExecuteResponse; +import software.amazon.awssdk.http.SdkHttpClient; +import software.amazon.awssdk.http.SdkHttpResponse; +import software.amazon.awssdk.http.async.SdkAsyncHttpClient; +import software.amazon.awssdk.retries.DefaultRetryStrategy; +import utils.HttpTestUtils; +import utils.ValidSdkObjects; + +/** + * Verifies that downloading to a destination that violates the file write option precondition fails client-side, before + * any request is dispatched, on both the sync and async client handlers. + */ +class FileTransformerFailFastTest { + + @TempDir + Path tempDir; + + private final SdkRequest request = mock(SdkRequest.class); + private final Marshaller marshaller = mock(Marshaller.class); + private final HttpResponseHandler responseHandler = mock(HttpResponseHandler.class); + private final HttpResponseHandler errorResponseHandler = mock(HttpResponseHandler.class); + + /** + * Mirrors the generated {@code getObject(request, path)} overload, which evaluates + * {@code ResponseTransformer.toFile(path)} inside the call, so the failure surfaces from the call and no request is + * dispatched. + */ + @Test + void syncExecute_toFileDestinationExists_failsFastWithoutDispatchingRequest() throws Exception { + SdkHttpClient httpClient = mock(SdkHttpClient.class); + SdkSyncClientHandler handler = new SdkSyncClientHandler(syncClientConfiguration(httpClient)); + + Path existingFile = Files.createFile(tempDir.resolve("sync-existing-dest.bin")); + + assertThatThrownBy(() -> handler.execute(clientExecutionParams(), ResponseTransformer.toFile(existingFile))) + .isInstanceOf(SdkClientException.class) + .hasRootCauseInstanceOf(FileAlreadyExistsException.class); + + verifyNoInteractions(httpClient); + } + + /** + * Guards against over-eager rejection: a destination that does not exist yet must still be requested and written. + */ + @Test + void syncExecute_toFileDestinationDoesNotExist_dispatchesRequestAndWritesFile() throws Exception { + SdkHttpClient httpClient = mock(SdkHttpClient.class); + ExecutableHttpRequest httpClientCall = mock(ExecutableHttpRequest.class); + when(httpClient.prepareRequest(any())).thenReturn(httpClientCall); + when(httpClientCall.call()).thenReturn( + HttpExecuteResponse.builder() + .response(SdkHttpResponse.builder().statusCode(200).build()) + .responseBody(AbortableInputStream.create( + new ByteArrayInputStream("hello".getBytes(StandardCharsets.UTF_8)))) + .build()); + when(marshaller.marshall(request)).thenReturn(ValidSdkObjects.sdkHttpFullRequest().build()); + when(responseHandler.handle(any(), any())).thenReturn(VoidSdkResponse.builder().build()); + + SdkSyncClientHandler handler = new SdkSyncClientHandler(syncClientConfiguration(httpClient)); + Path freshPath = tempDir.resolve("fresh-dest.bin"); + + handler.execute(clientExecutionParams(), ResponseTransformer.toFile(freshPath)); + + verify(httpClient).prepareRequest(any()); + assertThat(freshPath).hasContent("hello"); + } + + @Test + void asyncExecute_toFileDestinationExists_failsFastWithoutDispatchingRequest() throws Exception { + SdkAsyncHttpClient httpClient = mock(SdkAsyncHttpClient.class); + SdkAsyncClientHandler handler = new SdkAsyncClientHandler(asyncClientConfiguration(httpClient)); + + Path existingFile = Files.createFile(tempDir.resolve("async-existing-dest.bin")); + + CompletableFuture responseFuture = + handler.execute(clientExecutionParams(), AsyncResponseTransformer.toFile(existingFile)); + + assertThatThrownBy(() -> responseFuture.get(5, TimeUnit.SECONDS)) + .hasRootCauseInstanceOf(FileAlreadyExistsException.class); + verify(httpClient, never()).execute(any()); + } + + private ClientExecutionParams clientExecutionParams() { + when(request.overrideConfiguration()).thenReturn(Optional.empty()); + return new ClientExecutionParams() + .withInput(request) + .withMarshaller(marshaller) + .withResponseHandler(responseHandler) + .withErrorResponseHandler(errorResponseHandler); + } + + private SdkClientConfiguration syncClientConfiguration(SdkHttpClient httpClient) { + return HttpTestUtils.testClientConfiguration().toBuilder() + .option(SdkClientOption.SYNC_HTTP_CLIENT, httpClient) + .option(SdkClientOption.RETRY_STRATEGY, DefaultRetryStrategy.doNotRetry()) + .build(); + } + + private SdkClientConfiguration asyncClientConfiguration(SdkAsyncHttpClient httpClient) { + ScheduledExecutorService scheduledExecutor = mock(ScheduledExecutorService.class); + return HttpTestUtils.testClientConfiguration().toBuilder() + .option(SdkClientOption.ASYNC_HTTP_CLIENT, httpClient) + .option(SdkClientOption.RETRY_POLICY, RetryPolicy.none()) + .option(SdkClientOption.RETRY_STRATEGY, DefaultRetryStrategy.doNotRetry()) + .option(SdkClientOption.SCHEDULED_EXECUTOR_SERVICE, scheduledExecutor) + .build(); + } +} diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisherTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisherTest.java index 683325f5be31..933d9516eb86 100644 --- a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisherTest.java +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerPublisherTest.java @@ -24,6 +24,7 @@ import com.google.common.jimfs.Jimfs; import io.reactivex.Flowable; import java.nio.ByteBuffer; +import java.nio.file.FileAlreadyExistsException; import java.nio.file.FileSystem; import java.nio.file.Files; import java.nio.file.Path; @@ -382,4 +383,46 @@ public void onComplete() { assertThat(exception.get()).hasMessageContaining("Content range header is missing"); } + @Test + void createNewDestinationExists_partFailsFastAndDoesNotHang() throws Exception { + Files.write(testFile, "already here".getBytes()); + + AsyncResponseTransformer initialTransformer = AsyncResponseTransformer.toFile(testFile); + FileAsyncResponseTransformerPublisher publisher = + new FileAsyncResponseTransformerPublisher<>((FileAsyncResponseTransformer) initialTransformer); + + CompletableFuture future = new CompletableFuture<>(); + + publisher.subscribe(new Subscriber>() { + @Override + public void onSubscribe(Subscription s) { + s.request(1); + } + + @Override + public void onNext(AsyncResponseTransformer transformer) { + CompletableFuture prepareFuture = transformer.prepare(); + CompletableFutureUtils.forwardResultTo(prepareFuture, future); + // onResponse triggers the delegate prepare(), which fails fast because the CREATE_NEW destination + // already exists. The part future must complete exceptionally rather than the throw escaping the + // reactive callback and hanging the part. + transformer.onResponse(createMockResponseWithRange("bytes 0-9/10")); + } + + @Override + public void onError(Throwable t) { + future.completeExceptionally(t); + } + + @Override + public void onComplete() { + // unused for test + } + }); + + assertThatThrownBy(() -> future.get(5, TimeUnit.SECONDS)) + .hasRootCauseInstanceOf(FileAlreadyExistsException.class); + assertThat(testFile).hasContent("already here"); + } + } \ No newline at end of file diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerTest.java index 81a8fa87de9e..fb4130c7f391 100644 --- a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerTest.java +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformerTest.java @@ -54,6 +54,7 @@ import software.amazon.awssdk.core.FileTransformerConfiguration.FileWriteOption; import software.amazon.awssdk.core.FileTransformerConfiguration.FailureBehavior; import software.amazon.awssdk.core.async.SdkPublisher; +import software.amazon.awssdk.core.exception.SdkClientException; import software.amazon.awssdk.core.internal.util.NoopSubscription; /** @@ -119,19 +120,19 @@ public void synchronousPublisher_shouldNotHang() throws Exception { } @Test - void noConfiguration_fileAlreadyExists_shouldThrowException() throws Exception { + void prepare_createNew_fileAlreadyExists_shouldThrowSynchronously() throws Exception { Path testPath = testFs.getPath("test_file.txt"); String existingContent = RandomStringUtils.randomAlphanumeric(1000); Files.write(testPath, existingContent.getBytes(StandardCharsets.UTF_8)); assertThat(testPath).exists(); - String content = RandomStringUtils.randomAlphanumeric(30000); FileAsyncResponseTransformer transformer = new FileAsyncResponseTransformer<>(testPath); - CompletableFuture future = transformer.prepare(); - transformer.onResponse("foobar"); - transformer.onStream(testPublisher(content)); - assertThatThrownBy(() -> future.join()).hasRootCauseInstanceOf(FileAlreadyExistsException.class); + // CREATE_NEW cannot succeed against an existing destination, so prepare() fails fast, synchronously, + // before any request is dispatched. The pre-existing file is left untouched. + assertThatThrownBy(transformer::prepare) + .isInstanceOf(SdkClientException.class) + .hasRootCauseInstanceOf(FileAlreadyExistsException.class); assertThat(testPath).hasContent(existingContent); } @@ -188,8 +189,10 @@ void createOrAppendExisting_fileExists_shouldAppend() throws Exception { assertThat(testPath).hasContent(existingString + content); } + // CREATE_NEW is intentionally excluded: an existing destination now fails fast in prepare() before onStream, + // covered by prepare_createNew_fileAlreadyExists_shouldThrowSynchronously. @ParameterizedTest - @MethodSource("deleteConfigurations") + @MethodSource("deleteConfigurationsExcludingCreateNew") void exceptionOccurred_beforeFileOpened_shouldPreserveExistingFile(FileTransformerConfiguration configuration) throws Exception { Path testPath = testFs.getPath("test_file.txt"); @@ -208,7 +211,7 @@ void exceptionOccurred_beforeFileOpened_shouldPreserveExistingFile(FileTransform } @Test - void exceptionOccurred_beforeFileOpenedOnRetry_shouldPreserveExistingFile() throws Exception { + void prepare_createNewRetryAfterDestinationAppeared_shouldThrowAndPreserveExistingFile() throws Exception { Path testPath = testFs.getPath("test_file.txt"); FileAsyncResponseTransformer transformer = new FileAsyncResponseTransformer<>(testPath); @@ -217,13 +220,12 @@ void exceptionOccurred_beforeFileOpenedOnRetry_shouldPreserveExistingFile() thro String existingContent = RandomStringUtils.randomAlphanumeric(1000); Files.write(testPath, existingContent.getBytes(StandardCharsets.UTF_8)); - CompletableFuture future = transformer.prepare(); - RuntimeException exception = new RuntimeException("oops"); - transformer.exceptionOccurred(exception); - assertThat(future).failsWithin(1, TimeUnit.SECONDS) - .withThrowableOfType(ExecutionException.class) - .withCause(exception); + // prepare() runs on every attempt. If the destination appears between attempts, the CREATE_NEW retry + // now fails fast in prepare() before re-sending the request, leaving the existing file untouched. + assertThatThrownBy(transformer::prepare) + .isInstanceOf(SdkClientException.class) + .hasRootCauseInstanceOf(FileAlreadyExistsException.class); assertThat(testPath).hasContent(existingContent); } @@ -272,9 +274,12 @@ void exceptionOccurred_deleteFileBehavior(FileTransformerConfiguration configura } } - private static List deleteConfigurations() { + private static List deleteConfigurationsExcludingCreateNew() { List conf = new ArrayList<>(); for (FileWriteOption fileWriteOption : FileWriteOption.values()) { + if (fileWriteOption == FileWriteOption.CREATE_NEW) { + continue; + } conf.add(FileTransformerConfiguration.builder() .fileWriteOption(fileWriteOption) .failureBehavior(DELETE) @@ -351,7 +356,7 @@ void writeToPosition_fileExists_shouldAppendFromPosition() throws Exception { } @Test - void writeToPosition_fileDoesNotExists_shouldThrowException() throws Exception { + void prepare_writeToPosition_fileDoesNotExist_shouldThrowSynchronously() throws Exception { Path path = testFs.getPath("this/file/does/not/exists"); FileAsyncResponseTransformer transformer = new FileAsyncResponseTransformer<>( path, @@ -360,12 +365,12 @@ void writeToPosition_fileDoesNotExists_shouldThrowException() throws Exception { .failureBehavior(DELETE) .fileWriteOption(FileWriteOption.WRITE_TO_POSITION) .build()); - CompletableFuture future = transformer.prepare(); - transformer.onResponse("foobar"); - assertThatThrownBy(() -> { - transformer.onStream(testPublisher("foo-bar-content")); - future.get(10, TimeUnit.SECONDS); - }).hasRootCauseInstanceOf(NoSuchFileException.class); + + // WRITE_TO_POSITION requires the destination to already exist, so prepare() fails fast, synchronously, + // before any request is dispatched. + assertThatThrownBy(transformer::prepare) + .isInstanceOf(SdkClientException.class) + .hasRootCauseInstanceOf(NoSuchFileException.class); } @Test @@ -424,7 +429,7 @@ void parentDirectoryDoesNotExist_throwsWithHelpfulMessage(FileTransformerConfigu } @Test - void writeToPosition_fileDoesNotExist_throwsWithHelpfulMessage() { + void prepare_writeToPosition_fileDoesNotExistInExistingDirectory_shouldThrowSynchronously() { Path testPath = testFs.getPath("nonexistent_file.txt"); FileAsyncResponseTransformer transformer = new FileAsyncResponseTransformer<>(testPath, FileTransformerConfiguration.builder() @@ -432,13 +437,9 @@ void writeToPosition_fileDoesNotExist_throwsWithHelpfulMessage() { .fileWriteOption(FileWriteOption.WRITE_TO_POSITION) .build()); - CompletableFuture future = transformer.prepare(); - transformer.onResponse("foobar"); - transformer.onStream(testPublisher("content")); - - assertThat(future).failsWithin(1, TimeUnit.SECONDS) - .withThrowableOfType(ExecutionException.class) - .withCauseInstanceOf(NoSuchFileException.class); + assertThatThrownBy(transformer::prepare) + .isInstanceOf(SdkClientException.class) + .hasRootCauseInstanceOf(NoSuchFileException.class); } private static void stubSuccessfulStreaming(String newContent, FileAsyncResponseTransformer transformer) throws Exception { diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriberWiremockTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriberWiremockTest.java index ad05af389452..bb238d7b41d7 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriberWiremockTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/PresignedUrlMultipartDownloaderSubscriberWiremockTest.java @@ -33,6 +33,7 @@ import java.net.URL; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; +import java.nio.file.FileAlreadyExistsException; import java.nio.file.Files; import java.nio.file.Path; import java.util.Arrays; @@ -325,8 +326,15 @@ void presignedUrlDownload_416OnSecondRequest_shouldFailWithError(String transfor PresignedUrlDownloadRequest request = PresignedUrlDownloadRequest.builder() .presignedUrl(presignedUrl) .build(); - assertThatThrownBy(() -> executeDownload(request, transformerType).join()) - .hasRootCauseInstanceOf(S3Exception.class); + if ("toFile".equals(transformerType)) { + // Part 1 created the file. The 416 triggers a retry with the same CREATE_NEW file transformer, which now + // fails before sending a request because the destination already exists. + assertThatThrownBy(() -> executeDownload(request, transformerType).join()) + .hasRootCauseInstanceOf(FileAlreadyExistsException.class); + } else { + assertThatThrownBy(() -> executeDownload(request, transformerType).join()) + .hasRootCauseInstanceOf(S3Exception.class); + } } @ParameterizedTest(name = "presignedUrlDownload_withRangeHeader_emptyObject_shouldThrow416 [{0}]") diff --git a/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/useragent/StreamingBodyAndTransformerImplTrackingTest.java b/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/useragent/StreamingBodyAndTransformerImplTrackingTest.java index 6f93bff6a7d9..993e182a23b2 100644 --- a/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/useragent/StreamingBodyAndTransformerImplTrackingTest.java +++ b/test/codegen-generated-classes-test/src/test/java/software/amazon/awssdk/services/useragent/StreamingBodyAndTransformerImplTrackingTest.java @@ -19,8 +19,10 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import java.io.ByteArrayInputStream; +import java.io.File; import java.io.IOException; import java.io.OutputStream; +import java.nio.file.Files; import java.util.concurrent.Executors; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -94,7 +96,7 @@ public void streamingOutputOperation_syncClient_bytes_recordsMetadata() { @Test public void streamingOutputOperation_syncClient_file_recordsMetadata() throws IOException { - callStreamingOutputOperation(syncClient(), ResponseTransformer.toFile(new RandomTempFile(0))); + callStreamingOutputOperation(syncClient(), ResponseTransformer.toFile(nonExistentTempFile())); assertThat(interceptor.userAgent()).contains("md/rt#f"); } @@ -117,10 +119,20 @@ public void streamingOutputOperation_asyncClient_bytes_recordsMetadata() { @Test public void streamingOutputOperation_asyncClient_file_recordsMetadata() throws IOException { - callStreamingOutputOperation(asyncClient(), AsyncResponseTransformer.toFile(new RandomTempFile(0))); + callStreamingOutputOperation(asyncClient(), AsyncResponseTransformer.toFile(nonExistentTempFile())); assertThat(interceptor.userAgent()).contains("md/rt#f"); } + /** + * The default CREATE_NEW write option rejects a destination that already exists before sending the request, so + * these tests must name a file that does not exist yet. + */ + private static File nonExistentTempFile() throws IOException { + File file = new RandomTempFile(0); + Files.delete(file.toPath()); + return file; + } + @Test public void streamingOutputOperation_asyncClient_publisher_recordsMetadata() { callStreamingOutputOperation(asyncClient(), AsyncResponseTransformer.toPublisher());