fix(core): now creating each sub-directory in the path recursively up…

…on creating a resource or directory for listing to work properly
kestra-io · Nov 8, 2023 · ae94ca0 · ae94ca0
1 parent 45cd541
commit ae94ca0
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 15 deletions.
diff --git a/src/main/java/io/kestra/storage/s3/S3Storage.java b/src/main/java/io/kestra/storage/s3/S3Storage.java
@@ -23,10 +23,7 @@
 import java.io.InputStream;
 import java.net.URI;
 import java.nio.file.Path;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Objects;
-import java.util.Optional;
+import java.util.*;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executors;
 
@@ -82,7 +79,9 @@ private InputStream get(String path) throws IOException {
     @Override
     public List<FileAttributes> list(String tenantId, URI uri) throws IOException {
         String path = getPath(tenantId, uri);
-        String prefix = (path.endsWith("/")) ? path : path + "/";
+        String prefix = path.isEmpty()
+            ? path
+            : (path.endsWith("/") ? path : path + "/");
         try {
             ListObjectsV2Request request = ListObjectsV2Request.builder()
                 .bucket(s3Config.getBucket())
@@ -161,7 +160,8 @@ private FileAttributes getFileAttributes(String path) throws IOException {
                 .key(path)
                 .build();
             S3FileAttributes.S3FileAttributesBuilder builder = S3FileAttributes.builder()
-                .fileName(Path.of(path).getFileName().toString())
+                .fileName(Optional.ofNullable(Path.of(path).getFileName()).map(Path::toString)
+                    .orElse("/"))
                 .head(s3Client.headObject(headObjectRequest));
             if (path.endsWith("/")) {
                 builder.isDirectory(true);
@@ -231,15 +231,21 @@ public URI createDirectory(String tenantId, URI uri) throws IOException {
 
     private void mkdirs(String path) throws IOException {
         try {
-            if (!path.endsWith("/") && path.lastIndexOf('/') > 0) {
-                path = path.substring(0, path.lastIndexOf('/') + 1);
+            if(!path.startsWith("/")) {
+                path = "/" + path;
             }
 
-            PutObjectRequest putRequest = PutObjectRequest.builder()
-                .bucket(s3Config.getBucket())
-                .key(path)
-                .build();
-            s3Client.putObject(putRequest, RequestBody.empty());
+            // perform 1 put request per parent directory in the path
+            String[] directories = path.split("/");
+            StringBuilder aggregatedPath = new StringBuilder();
+            for (int i = 0; i < directories.length - (path.endsWith("/") ? 1 : 2); i++) {
+                aggregatedPath.append(directories[i]).append("/");
+                PutObjectRequest putRequest = PutObjectRequest.builder()
+                    .bucket(s3Config.getBucket())
+                    .key(aggregatedPath.toString())
+                    .build();
+                s3Client.putObject(putRequest, RequestBody.empty());
+            }
         } catch (AwsServiceException | SdkClientException exception) {
             throw new IOException(exception);
         }
@@ -332,11 +338,16 @@ public List<URI> deleteByPrefix(String tenantId, URI storagePrefix) throws IOExc
     }
 
     private String getPath(String tenantId, URI uri) {
+        if(uri == null) {
+            return "";
+        }
+
         parentTraversalGuard(uri);
+        String path = uri.getPath();
         if (tenantId == null) {
-            return uri.getPath();
+            return path;
         }
-        return "/" + tenantId + uri.getPath();
+        return "/" + tenantId + (path.startsWith("/") ? "" : "/") + path;
     }
 
     // Traversal does not work with s3 but it just return empty objects so throwing is more explicit

diff --git a/src/test/java/io/kestra/storage/s3/S3StorageTest.java b/src/test/java/io/kestra/storage/s3/S3StorageTest.java
@@ -1,6 +1,7 @@
 package io.kestra.storage.s3;
 
 import io.kestra.core.storage.StorageTestSuite;
+import io.kestra.core.storages.FileAttributes;
 import io.kestra.core.storages.StorageInterface;
 import io.kestra.core.utils.IdUtils;
 import jakarta.inject.Inject;
@@ -14,6 +15,7 @@
 
 import java.io.*;
 import java.net.URI;
+import java.util.List;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.*;
@@ -61,4 +63,19 @@ void getEmptyFile() throws IOException {
         assertThat(inputStream, not(instanceOf(ResponseInputStream.class)));
         assertThat(new BufferedReader(new InputStreamReader(inputStream)).lines().count(), is(0L));
     }
+
+    @Test
+    void mkdirRecursive() throws IOException {
+        storageInterface.createDirectory(null, URI.create("/first/second/third"));
+
+        List<FileAttributes> list = storageInterface.list(null, null);
+        assertThat(list, contains(
+            hasProperty("fileName", is("/"))
+        ));
+
+        list = storageInterface.list(null, URI.create("/"));
+        assertThat(list, contains(
+            hasProperty("fileName", is("first"))
+        ));
+    }
 }