From bf335262ba5e52209ae1541c69b1a453d0326248 Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Thu, 23 May 2024 09:54:54 -0400 Subject: [PATCH 1/4] add WRITE_TO_POSITION option for FileAsyncResponseTransformer --- .../core/FileTransformerConfiguration.java | 77 +++++++++++-- .../async/FileAsyncResponseTransformer.java | 24 +++-- .../FileTransformerConfigurationTest.java | 19 ++++ .../FileAsyncResponseTransformerTest.java | 102 +++++++++++++----- 4 files changed, 182 insertions(+), 40 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java index 902815f96c49..54a29481d4f8 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java @@ -23,6 +23,7 @@ import java.util.concurrent.ExecutorService; import software.amazon.awssdk.annotations.SdkPublicApi; import software.amazon.awssdk.core.async.AsyncResponseTransformer; +import software.amazon.awssdk.utils.ToString; import software.amazon.awssdk.utils.Validate; import software.amazon.awssdk.utils.builder.CopyableBuilder; import software.amazon.awssdk.utils.builder.ToCopyableBuilder; @@ -41,11 +42,19 @@ public final class FileTransformerConfiguration implements ToCopyableBuilder executorService() { return Optional.ofNullable(executorService); } + /** + * Exclusively used with {@link FileWriteOption#WRITE_TO_POSITION}. Configures the position, where to start writing to the existing + * file. The location correspond to the first byte where new data will be written. For example, if {@code 128} is configured, + * bytes 0-127 of the existing file will remain untouched and data will be appended starting at byte 128. If not specified, + * defaults to 0. + * + * @return The offset at which to start overwriting data in the file. + */ + public Long position() { + return position; + } + /** * Create a {@link Builder}, used to create a {@link FileTransformerConfiguration}. */ @@ -82,8 +103,8 @@ public static Builder builder() { /** * Returns the default {@link FileTransformerConfiguration} for {@link FileWriteOption#CREATE_NEW} *

- * Always create a new file. If the file already exists, {@link FileAlreadyExistsException} will be thrown. - * In the event of an error, the SDK will attempt to delete the file (whatever has been written to it so far). + * Always create a new file. If the file already exists, {@link FileAlreadyExistsException} will be thrown. In the event of an + * error, the SDK will attempt to delete the file (whatever has been written to it so far). */ public static FileTransformerConfiguration defaultCreateNew() { return builder().fileWriteOption(FileWriteOption.CREATE_NEW) @@ -94,8 +115,8 @@ public static FileTransformerConfiguration defaultCreateNew() { /** * Returns the default {@link FileTransformerConfiguration} for {@link FileWriteOption#CREATE_OR_REPLACE_EXISTING} *

- * Create a new file if it doesn't exist, otherwise replace the existing file. - * In the event of an error, the SDK will NOT attempt to delete the file, leaving it as-is + * Create a new file if it doesn't exist, otherwise replace the existing file. In the event of an error, the SDK will NOT + * attempt to delete the file, leaving it as-is */ public static FileTransformerConfiguration defaultCreateOrReplaceExisting() { return builder().fileWriteOption(FileWriteOption.CREATE_OR_REPLACE_EXISTING) @@ -106,8 +127,8 @@ public static FileTransformerConfiguration defaultCreateOrReplaceExisting() { /** * Returns the default {@link FileTransformerConfiguration} for {@link FileWriteOption#CREATE_OR_APPEND_TO_EXISTING} *

- * Create a new file if it doesn't exist, otherwise append to the existing file. - * In the event of an error, the SDK will NOT attempt to delete the file, leaving it as-is + * Create a new file if it doesn't exist, otherwise append to the existing file. In the event of an error, the SDK will NOT + * attempt to delete the file, leaving it as-is */ public static FileTransformerConfiguration defaultCreateOrAppend() { return builder().fileWriteOption(FileWriteOption.CREATE_OR_APPEND_TO_EXISTING) @@ -137,6 +158,9 @@ public boolean equals(Object o) { if (failureBehavior != that.failureBehavior) { return false; } + if (!Objects.equals(position, that.position)) { + return false; + } return Objects.equals(executorService, that.executorService); } @@ -145,6 +169,7 @@ public int hashCode() { int result = fileWriteOption != null ? fileWriteOption.hashCode() : 0; result = 31 * result + (failureBehavior != null ? failureBehavior.hashCode() : 0); result = 31 * result + (executorService != null ? executorService.hashCode() : 0); + result = 31 * result + (position != null ? position.hashCode() : 0); return result; } @@ -165,7 +190,15 @@ public enum FileWriteOption { /** * Create a new file if it doesn't exist, otherwise append to the existing file. */ - CREATE_OR_APPEND_TO_EXISTING + CREATE_OR_APPEND_TO_EXISTING, + + /** + * Write to an existing file at the specified position, defined by the + * {@link FileTransformerConfiguration#position()}. If the file does not exist, a + * {@link java.nio.file.NoSuchFileException} will be thrown. If {@link FileTransformerConfiguration#position()} is + * not configured, start overriding data at the beginning of the file (byte 0). + */ + WRITE_TO_POSITION } /** @@ -209,12 +242,24 @@ public interface Builder extends CopyableBuilder implements AsyncRespo private final FileTransformerConfiguration configuration; public FileAsyncResponseTransformer(Path path) { - this.path = path; - this.configuration = FileTransformerConfiguration.defaultCreateNew(); - this.position = 0L; + this(path, FileTransformerConfiguration.defaultCreateNew()); } public FileAsyncResponseTransformer(Path path, FileTransformerConfiguration fileConfiguration) { + this(path, fileConfiguration, determineFilePositionToWrite(path, fileConfiguration)); + } + + private FileAsyncResponseTransformer(Path path, FileTransformerConfiguration fileTransformerConfiguration, long position) { this.path = path; - this.configuration = fileConfiguration; - this.position = determineFilePositionToWrite(path); + this.configuration = fileTransformerConfiguration; + this.position = position; } - private long determineFilePositionToWrite(Path path) { - if (configuration.fileWriteOption() == CREATE_OR_APPEND_TO_EXISTING) { + private static long determineFilePositionToWrite(Path path, FileTransformerConfiguration fileConfiguration) { + if (fileConfiguration.fileWriteOption() == CREATE_OR_APPEND_TO_EXISTING) { try { return Files.size(path); } catch (NoSuchFileException e) { @@ -79,6 +83,9 @@ private long determineFilePositionToWrite(Path path) { throw SdkClientException.create("Cannot determine the current file size " + path, exception); } } + if (fileConfiguration.fileWriteOption() == WRITE_TO_POSITION) { + return Validate.getOrDefault(fileConfiguration.position(), () -> 0L); + } return 0L; } @@ -95,6 +102,9 @@ private AsynchronousFileChannel createChannel(Path path) throws IOException { case CREATE_NEW: Collections.addAll(options, StandardOpenOption.WRITE, StandardOpenOption.CREATE_NEW); break; + case WRITE_TO_POSITION: + Collections.addAll(options, StandardOpenOption.WRITE); + break; default: throw new IllegalArgumentException("Unsupported file write option: " + configuration.fileWriteOption()); } diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/FileTransformerConfigurationTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/FileTransformerConfigurationTest.java index 9eb426ee3ac6..a0e533a9f1a1 100644 --- a/core/sdk-core/src/test/java/software/amazon/awssdk/core/FileTransformerConfigurationTest.java +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/FileTransformerConfigurationTest.java @@ -16,14 +16,33 @@ package software.amazon.awssdk.core; import static org.assertj.core.api.AssertionsForClassTypes.assertThat; +import static org.assertj.core.api.AssertionsForClassTypes.assertThatCode; +import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy; import static software.amazon.awssdk.core.FileTransformerConfiguration.FailureBehavior.DELETE; import static software.amazon.awssdk.core.FileTransformerConfiguration.FileWriteOption.CREATE_NEW; import nl.jqno.equalsverifier.EqualsVerifier; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; class FileTransformerConfigurationTest { + @ParameterizedTest + @EnumSource( + value = FileTransformerConfiguration.FileWriteOption.class, + names = {"CREATE_NEW", "CREATE_OR_REPLACE_EXISTING", "CREATE_OR_APPEND_TO_EXISTING"}) + void position_whenUsedWithNotWriteToPosition_shouldThrowIllegalArgumentException( + FileTransformerConfiguration.FileWriteOption fileWriteOption) { + FileTransformerConfiguration.Builder builder = FileTransformerConfiguration.builder() + .position(123L) + .failureBehavior(DELETE) + .fileWriteOption(fileWriteOption); + assertThatThrownBy(builder::build) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining(fileWriteOption.name()); + } + @Test void equalsHashcode() { EqualsVerifier.forClass(FileTransformerConfiguration.class) 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 bcab30e49675..95953eef4877 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 @@ -28,9 +28,9 @@ import java.nio.file.FileAlreadyExistsException; import java.nio.file.FileSystem; import java.nio.file.Files; +import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.List; import java.util.concurrent.Callable; @@ -50,6 +50,7 @@ import org.reactivestreams.Subscription; import software.amazon.awssdk.core.FileTransformerConfiguration; import software.amazon.awssdk.core.FileTransformerConfiguration.FileWriteOption; +import software.amazon.awssdk.core.FileTransformerConfiguration.FailureBehavior; import software.amazon.awssdk.core.async.SdkPublisher; /** @@ -185,8 +186,11 @@ void createOrAppendExisting_fileExists_shouldAppend() throws Exception { @MethodSource("configurations") void exceptionOccurred_deleteFileBehavior(FileTransformerConfiguration configuration) throws Exception { Path testPath = testFs.getPath("test_file.txt"); - FileAsyncResponseTransformer transformer = new FileAsyncResponseTransformer<>(testPath, - configuration); + if (configuration.fileWriteOption() == FileWriteOption.WRITE_TO_POSITION) { + // file must exist for WRITE_TO_POSITION + Files.write(testPath, "foobar".getBytes(StandardCharsets.UTF_8)); + } + FileAsyncResponseTransformer transformer = new FileAsyncResponseTransformer<>(testPath, configuration); stubException(RandomStringUtils.random(200), transformer); if (configuration.failureBehavior() == LEAVE) { assertThat(testPath).exists(); @@ -196,28 +200,19 @@ void exceptionOccurred_deleteFileBehavior(FileTransformerConfiguration configura } private static List configurations() { - return Arrays.asList( - FileTransformerConfiguration.defaultCreateNew(), - FileTransformerConfiguration.defaultCreateOrAppend(), - FileTransformerConfiguration.defaultCreateOrReplaceExisting(), - FileTransformerConfiguration.builder() - .fileWriteOption(FileWriteOption.CREATE_NEW) - .failureBehavior(LEAVE).build(), - FileTransformerConfiguration.builder() - .fileWriteOption(FileWriteOption.CREATE_NEW) - .failureBehavior(DELETE).build(), - FileTransformerConfiguration.builder() - .fileWriteOption(FileWriteOption.CREATE_OR_APPEND_TO_EXISTING) - .failureBehavior(DELETE).build(), - FileTransformerConfiguration.builder() - .fileWriteOption(FileWriteOption.CREATE_OR_APPEND_TO_EXISTING) - .failureBehavior(LEAVE).build(), - FileTransformerConfiguration.builder() - .fileWriteOption(FileWriteOption.CREATE_OR_REPLACE_EXISTING) - .failureBehavior(DELETE).build(), - FileTransformerConfiguration.builder() - .fileWriteOption(FileWriteOption.CREATE_OR_REPLACE_EXISTING) - .failureBehavior(LEAVE).build()); + List conf = new ArrayList<>(); + conf.add(FileTransformerConfiguration.defaultCreateNew()); + conf.add(FileTransformerConfiguration.defaultCreateOrAppend()); + conf.add(FileTransformerConfiguration.defaultCreateOrReplaceExisting()); + for (FailureBehavior failureBehavior : FailureBehavior.values()) { + for (FileWriteOption fileWriteOption : FileWriteOption.values()) { + conf.add(FileTransformerConfiguration.builder() + .fileWriteOption(fileWriteOption) + .failureBehavior(failureBehavior) + .build()); + } + } + return conf; } @Test @@ -246,6 +241,63 @@ void explicitExecutor_shouldUseExecutor() throws Exception { } } + @Test + void writeToPosition_fileExists_shouldAppendFromPosition() throws Exception { + int totalSize = 100; + long prefixSize = 80L; + int newContentLength = 20; + + Path testPath = testFs.getPath("test_file.txt"); + String contentBeforeRewrite = RandomStringUtils.randomAlphanumeric(totalSize); + byte[] existingBytes = contentBeforeRewrite.getBytes(StandardCharsets.UTF_8); + Files.write(testPath, existingBytes); + String newContent = RandomStringUtils.randomAlphanumeric(newContentLength); + FileAsyncResponseTransformer transformer = new FileAsyncResponseTransformer<>( + testPath, + FileTransformerConfiguration.builder() + .position(prefixSize) + .failureBehavior(DELETE) + .fileWriteOption(FileWriteOption.WRITE_TO_POSITION) + .build()); + + stubSuccessfulStreaming(newContent, transformer); + + String expectedContent = contentBeforeRewrite.substring(0, 80) + newContent; + assertThat(testPath).hasContent(expectedContent); + } + + @Test + void writeToPosition_fileDoesNotExists_shouldThrowException() throws Exception { + Path path = testFs.getPath("this/file/does/not/exists"); + FileAsyncResponseTransformer transformer = new FileAsyncResponseTransformer<>(path); + transformer.prepare(); + transformer.onResponse("foobar"); + assertThatThrownBy(() -> transformer.onStream(testPublisher("foo-bar-content"))) + .hasRootCauseInstanceOf(NoSuchFileException.class); + + } + + @Test + void writeToPosition_fileExists_positionNotDefined_shouldRewriteFromStart() throws Exception { + int totalSize = 100; + Path testPath = testFs.getPath("test_file.txt"); + String contentBeforeRewrite = RandomStringUtils.randomAlphanumeric(totalSize); + byte[] existingBytes = contentBeforeRewrite.getBytes(StandardCharsets.UTF_8); + Files.write(testPath, existingBytes); + String newContent = RandomStringUtils.randomAlphanumeric(totalSize); + FileAsyncResponseTransformer transformer = new FileAsyncResponseTransformer<>( + testPath, + FileTransformerConfiguration.builder() + .failureBehavior(DELETE) + .fileWriteOption(FileWriteOption.WRITE_TO_POSITION) + .build()); + + stubSuccessfulStreaming(newContent, transformer); + + assertThat(testPath).hasContent(newContent); + + } + private static void stubSuccessfulStreaming(String newContent, FileAsyncResponseTransformer transformer) throws Exception { CompletableFuture future = transformer.prepare(); transformer.onResponse("foobar"); From ecc8337f01268c121cafe519612c163141d2f1e3 Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Thu, 23 May 2024 12:21:39 -0400 Subject: [PATCH 2/4] manually set position to 0 for path constructor --- .../core/internal/async/FileAsyncResponseTransformer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 00e3926aa36c..0528b1e88f19 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 @@ -60,7 +60,7 @@ public final class FileAsyncResponseTransformer implements AsyncRespo private final FileTransformerConfiguration configuration; public FileAsyncResponseTransformer(Path path) { - this(path, FileTransformerConfiguration.defaultCreateNew()); + this(path, FileTransformerConfiguration.defaultCreateNew(), 0L); } public FileAsyncResponseTransformer(Path path, FileTransformerConfiguration fileConfiguration) { From aba92f4e0a8cc75364c527037dc1dae4263ece2b Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Thu, 23 May 2024 12:26:41 -0400 Subject: [PATCH 3/4] javadoc comments cleanup --- .../core/FileTransformerConfiguration.java | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java index 54a29481d4f8..61f33927b0d7 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java @@ -53,7 +53,7 @@ private FileTransformerConfiguration(DefaultBuilder builder) { throw new IllegalArgumentException(String.format( "'position' can only be used with 'WRITE_TO_POSITION' file write option, but was used with '%s'", fileWriteOption - )); + )); } } @@ -82,10 +82,10 @@ public Optional executorService() { } /** - * Exclusively used with {@link FileWriteOption#WRITE_TO_POSITION}. Configures the position, where to start writing to the existing - * file. The location correspond to the first byte where new data will be written. For example, if {@code 128} is configured, - * bytes 0-127 of the existing file will remain untouched and data will be appended starting at byte 128. If not specified, - * defaults to 0. + * Exclusively used with {@link FileWriteOption#WRITE_TO_POSITION}. Configures the position, where to start writing to the + * existing file. The location correspond to the first byte where new data will be written. For example, if {@code 128} is + * configured, bytes 0-127 of the existing file will remain untouched and data will be appended starting at byte 128. If not + * specified, defaults to 0. * * @return The offset at which to start overwriting data in the file. */ @@ -103,8 +103,8 @@ public static Builder builder() { /** * Returns the default {@link FileTransformerConfiguration} for {@link FileWriteOption#CREATE_NEW} *

- * Always create a new file. If the file already exists, {@link FileAlreadyExistsException} will be thrown. In the event of an - * error, the SDK will attempt to delete the file (whatever has been written to it so far). + * Always create a new file. If the file already exists, {@link FileAlreadyExistsException} will be thrown. + * In the event of an error, the SDK will attempt to delete the file (whatever has been written to it so far). */ public static FileTransformerConfiguration defaultCreateNew() { return builder().fileWriteOption(FileWriteOption.CREATE_NEW) @@ -115,8 +115,8 @@ public static FileTransformerConfiguration defaultCreateNew() { /** * Returns the default {@link FileTransformerConfiguration} for {@link FileWriteOption#CREATE_OR_REPLACE_EXISTING} *

- * Create a new file if it doesn't exist, otherwise replace the existing file. In the event of an error, the SDK will NOT - * attempt to delete the file, leaving it as-is + * Create a new file if it doesn't exist, otherwise replace the existing file. + * In the event of an error, the SDK will NOT attempt to delete the file, leaving it as-is */ public static FileTransformerConfiguration defaultCreateOrReplaceExisting() { return builder().fileWriteOption(FileWriteOption.CREATE_OR_REPLACE_EXISTING) @@ -127,8 +127,8 @@ public static FileTransformerConfiguration defaultCreateOrReplaceExisting() { /** * Returns the default {@link FileTransformerConfiguration} for {@link FileWriteOption#CREATE_OR_APPEND_TO_EXISTING} *

- * Create a new file if it doesn't exist, otherwise append to the existing file. In the event of an error, the SDK will NOT - * attempt to delete the file, leaving it as-is + * Create a new file if it doesn't exist, otherwise append to the existing file. + * In the event of an error, the SDK will NOT attempt to delete the file, leaving it as-is */ public static FileTransformerConfiguration defaultCreateOrAppend() { return builder().fileWriteOption(FileWriteOption.CREATE_OR_APPEND_TO_EXISTING) @@ -193,10 +193,10 @@ public enum FileWriteOption { CREATE_OR_APPEND_TO_EXISTING, /** - * Write to an existing file at the specified position, defined by the - * {@link FileTransformerConfiguration#position()}. If the file does not exist, a - * {@link java.nio.file.NoSuchFileException} will be thrown. If {@link FileTransformerConfiguration#position()} is - * not configured, start overriding data at the beginning of the file (byte 0). + * Write to an existing file at the specified position, defined by the {@link FileTransformerConfiguration#position()}. If + * the file does not exist, a {@link java.nio.file.NoSuchFileException} will be thrown. If + * {@link FileTransformerConfiguration#position()} is not configured, start overriding data at the beginning of the file + * (byte 0). */ WRITE_TO_POSITION } @@ -244,8 +244,8 @@ public interface Builder extends CopyableBuilder Date: Thu, 23 May 2024 14:03:24 -0400 Subject: [PATCH 4/4] javadoc comments --- .../amazon/awssdk/core/FileTransformerConfiguration.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java index 61f33927b0d7..3d8b74fd50c4 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java @@ -84,7 +84,7 @@ public Optional executorService() { /** * Exclusively used with {@link FileWriteOption#WRITE_TO_POSITION}. Configures the position, where to start writing to the * existing file. The location correspond to the first byte where new data will be written. For example, if {@code 128} is - * configured, bytes 0-127 of the existing file will remain untouched and data will be appended starting at byte 128. If not + * configured, bytes 0-127 of the existing file will remain untouched and data will be written starting at byte 128. If not * specified, defaults to 0. * * @return The offset at which to start overwriting data in the file. @@ -195,7 +195,7 @@ public enum FileWriteOption { /** * Write to an existing file at the specified position, defined by the {@link FileTransformerConfiguration#position()}. If * the file does not exist, a {@link java.nio.file.NoSuchFileException} will be thrown. If - * {@link FileTransformerConfiguration#position()} is not configured, start overriding data at the beginning of the file + * {@link FileTransformerConfiguration#position()} is not configured, start overwriting data at the beginning of the file * (byte 0). */ WRITE_TO_POSITION @@ -246,7 +246,7 @@ public interface Builder extends CopyableBuilder