Fix file name mapping in downloadDirectory (#3386)

Author: zoewangg

.changes/next-release/bugfix-S3TransferManager-90ca843.json

@@ -0,0 +1,6 @@
+{
+    "type": "bugfix",
+    "category": "S3 Transfer Manager",
+    "contributor": "",
+    "description": "Fixed the file name mapping issue in downloadDirectory. See [#3366](https://github.com/aws/aws-sdk-java-v2/issues/3366)"
+}

services-custom/s3-transfer-manager/src/it/java/software/amazon/awssdk/transfer/s3/S3TransferManagerDownloadDirectoryIntegrationTest.java

@@ -35,6 +35,8 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.ComparisonFailure;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import software.amazon.awssdk.testutils.FileUtils;
 import software.amazon.awssdk.utils.Logger;
 
@@ -131,7 +133,7 @@ public void downloadDirectory() throws Exception {
     }
 
     /**
-     * The destination directory structure should be the following with prefix "notes"
+     * With prefix = "notes", the destination directory structure should be the following:
      * <pre>
      *   {@code
      *      - destination
@@ -144,9 +146,9 @@ public void downloadDirectory() throws Exception {
      *   }
      * </pre>
      */
-    @Test
-    public void downloadDirectory_withPrefix() throws Exception {
-        String prefix = "notes";
+    @ParameterizedTest
+    @ValueSource(strings = {"notes/2021", "notes/2021/", "notes"})
+    public void downloadDirectory_withPrefix(String prefix) throws Exception {
         DirectoryDownload downloadDirectory = tm.downloadDirectory(u -> u.destinationDirectory(destinationDirectory)
                                                                          .prefix(prefix)
                                                                          .bucket(TEST_BUCKET));

services-custom/s3-transfer-manager/src/main/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelper.java

@@ -94,6 +94,7 @@ private void doDownloadDirectory(CompletableFuture<CompletedDirectoryDownload> r
                                      DownloadDirectoryRequest downloadDirectoryRequest) {
         validateDirectoryIfExists(downloadDirectoryRequest.destinationDirectory());
         String bucket = downloadDirectoryRequest.bucket();
+        // Delimiter is null by default. See https://github.com/aws/aws-sdk-java/issues/1215
         String delimiter = downloadDirectoryRequest.delimiter().orElse(null);
         String prefix = downloadDirectoryRequest.prefix().orElse(DEFAULT_PREFIX);
 
@@ -146,7 +147,7 @@ private Function<DownloadFileContext, CompletableFuture<?>> downloadSingleFile(
 
     private DownloadFileContext determineDestinationPath(DownloadDirectoryRequest downloadDirectoryRequest, S3Object s3Object) {
         FileSystem fileSystem = downloadDirectoryRequest.destinationDirectory().getFileSystem();
-        String delimiter = downloadDirectoryRequest.delimiter().orElse(null);
+        String delimiter = downloadDirectoryRequest.delimiter().orElse(DEFAULT_DELIMITER);
         String key = normalizeKey(downloadDirectoryRequest, s3Object, delimiter);
         String relativePath = getRelativePath(fileSystem, delimiter, key);
         Path destinationPath = downloadDirectoryRequest.destinationDirectory().resolve(relativePath);
@@ -194,17 +195,28 @@ private CompletableFuture<CompletedFileDownload> doDownloadSingleFile(DownloadDi
     }
 
     /**
-     * Normalizing the key by stripping the prefix from the s3 object key if the prefix is not empty. For example: given a request
-     * with prefix = "notes/2021", delimiter = "/" and key = "notes/2021/1.txt", the returned string should be "1.txt". If a
-     * delimiter is null (not provided by user), use "/" by default.
+     * Normalizing the key by stripping the prefix from the s3 object key if the prefix is not empty.
+     *
+     * If a delimiter is null (not provided by user), use "/" by default.
+     *
+     * For example: given a request with prefix = "notes/2021"  or "notes/2021/", delimiter = "/" and key = "notes/2021/1.txt",
+     * the normalized key should be "1.txt".
      */
     private static String normalizeKey(DownloadDirectoryRequest downloadDirectoryRequest,
                                        S3Object s3Object,
                                        String delimiter) {
-        int delimiterLength = delimiter == null ? DEFAULT_DELIMITER.length() : delimiter.length();
+        int delimiterLength = delimiter.length();
         return downloadDirectoryRequest.prefix()
                                        .filter(prefix -> !prefix.isEmpty())
-                                       .map(prefix -> s3Object.key().substring(prefix.length() + delimiterLength))
+                                       .map(prefix -> {
+                                           String normalizedKey;
+                                           if (prefix.endsWith(delimiter)) {
+                                               normalizedKey = s3Object.key().substring(prefix.length());
+                                           } else {
+                                               normalizedKey = s3Object.key().substring(prefix.length() + delimiterLength);
+                                           }
+                                           return normalizedKey;
+                                       })
                                        .orElseGet(s3Object::key);
     }
 

services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/DownloadDirectoryHelperParameterizedTest.java

@@ -103,6 +103,53 @@ void downloadDirectory_shouldRecursivelyDownload(FileSystem jimfs) {
         verifyDestinationPathForSingleDownload(jimfs, "/", keys, actualRequests);
     }
 
+    /**
+     * The S3 bucket has the following keys:
+     * abc/def/image.jpg
+     * abc/def/title.jpg
+     * abc/def/ghi/xyz.txt
+     *
+     * if the prefix is "abc/def/", the structure should like this:
+     * image.jpg
+     * title.jpg
+     * ghi
+     *  - xyz.txt
+     */
+    @ParameterizedTest
+    @MethodSource("fileSystems")
+    void downloadDirectory_withPrefix_shouldStripPrefixInDestinationPath(FileSystem jimfs) {
+        directory = jimfs.getPath("test");
+        String[] keys = {"abc/def/image.jpg", "abc/def/title.jpg", "abc/def/ghi/xyz.txt"};
+        stubSuccessfulListObjects(listObjectsHelper, keys);
+        ArgumentCaptor<DownloadFileRequest> requestArgumentCaptor = ArgumentCaptor.forClass(DownloadFileRequest.class);
+
+        when(singleDownloadFunction.apply(requestArgumentCaptor.capture()))
+            .thenReturn(completedDownload());
+        DirectoryDownload downloadDirectory =
+            downloadDirectoryHelper.downloadDirectory(DownloadDirectoryRequest.builder()
+                                                                              .destinationDirectory(directory)
+                                                                              .bucket("bucket")
+                                                                              .prefix("abc/def/")
+                                                                              .build());
+        CompletedDirectoryDownload completedDirectoryDownload = downloadDirectory.completionFuture().join();
+        assertThat(completedDirectoryDownload.failedTransfers()).isEmpty();
+
+        List<DownloadFileRequest> actualRequests = requestArgumentCaptor.getAllValues();
+
+        assertThat(actualRequests.size()).isEqualTo(keys.length);
+
+        List<String> destinations =
+            actualRequests.stream().map(u -> u.destination().toString())
+                          .collect(Collectors.toList());
+
+        String jimfsSeparator = jimfs.getSeparator();
+
+        List<String> expectedPaths =
+            Arrays.asList("image.jpg", "title.jpg", "ghi/xyz.txt").stream()
+                  .map(k -> DIRECTORY_NAME + jimfsSeparator + k.replace("/",jimfsSeparator)).collect(Collectors.toList());
+        assertThat(destinations).isEqualTo(expectedPaths);
+    }
+
     @ParameterizedTest
     @MethodSource("fileSystems")
     void downloadDirectory_withDelimiter_shouldHonor(FileSystem jimfs) {