Merge pull request #2593 from adobe/2586-mulitpart-upload-fixes

CompleteMultipartUpload is idempotent

Author: afranken

CHANGELOG.md

@@ -165,24 +165,25 @@ Version 4.x is JDK17 LTS bytecode compatible, with Docker and JUnit / direct Jav
 Version 4.x is JDK17 LTS bytecode compatible, with Docker and JUnit / direct Java integration.
 
 * Features and fixes
-  * TBD
+  * CompleteMultipartUpload is idempotent (fixes #2586)
 * Refactorings
   * UploadId is always a UUID. Use UUID type in S3Mock instead of String.
   * Validate that partNumbers to be positive integers.
   * Force convergence on the newest available transitive dependency versions.
   * Optimize file storage for large objects by using buffered streams.
 * Version updates (deliverable dependencies)
   * Bump spring-boot.version from 3.5.4 to 3.5.5
-  * Bump aws-v2.version from 2.32.7 to 2.32.23
+  * Bump aws-v2.version from 2.32.7 to 2.32.31
   * Bump org.apache.commons:commons-compress from 1.27.1 to 1.28.0
 * Version updates (build dependencies)
   * Bump kotlin.version from 2.2.0 to 2.2.10
-  * Bump aws.sdk.kotlin:s3-jvm from 1.4.125 to 1.5.19
+  * Bump aws.sdk.kotlin:s3-jvm from 1.4.125 to 1.5.26
   * Bump digital.pragmatech.testing:spring-test-profiler from 0.0.5 to 0.0.11
   * Bump com.puppycrawl.tools:checkstyle from 10.26.1 to 11.0.0
   * Bump github/codeql-action from 3.29.4 to 3.29.11
   * Bump actions/checkout from 4.2.2 to 5.0.0
   * Bump actions/setup-java from 4.7.1 to 5.0.0
+  * Bump actions/dependency-review-action from 4.7.2 to 4.7.3
 
 ## 4.7.0
 Version 4.x is JDK17 LTS bytecode compatible, with Docker and JUnit / direct Java integration.

build-config/pom.xml

@@ -21,7 +21,7 @@
   <parent>
     <groupId>com.adobe.testing</groupId>
     <artifactId>s3mock-parent</artifactId>
-    <version>4.7.1-SNAPSHOT</version>
+    <version>4.8.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>s3mock-build-config</artifactId>

docker/pom.xml

@@ -22,7 +22,7 @@
   <parent>
     <groupId>com.adobe.testing</groupId>
     <artifactId>s3mock-parent</artifactId>
-    <version>4.7.1-SNAPSHOT</version>
+    <version>4.8.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>s3mock-docker</artifactId>

integration-tests/pom.xml

@@ -22,7 +22,7 @@
   <parent>
     <groupId>com.adobe.testing</groupId>
     <artifactId>s3mock-parent</artifactId>
-    <version>4.7.1-SNAPSHOT</version>
+    <version>4.8.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>s3mock-integration-tests</artifactId>

integration-tests/src/test/kotlin/com/adobe/testing/s3mock/its/MultipartIT.kt

@@ -343,6 +343,36 @@ internal class MultipartIT : S3TestBase() {
 
     assertThat(completeMultipartUpload.location())
       .isEqualTo("${serviceEndpoint}/$bucketName/${UriUtils.encode(TEST_IMAGE_TIFF, StandardCharsets.UTF_8)}")
+
+    //verify completeMultipartUpload is idempotent
+    val completeMultipartUpload1 = s3Client.completeMultipartUpload {
+      it.bucket(initiateMultipartUploadResult.bucket())
+      it.key(initiateMultipartUploadResult.key())
+      it.uploadId(initiateMultipartUploadResult.uploadId())
+      it.multipartUpload {
+        it.parts(
+          {
+            it.eTag(etag1)
+            it.partNumber(1)
+            it.checksumCRC32(checksum1)
+          },
+          {
+            it.eTag(etag2)
+            it.partNumber(2)
+            it.checksumCRC32(checksum2)
+          }
+        )
+      }
+    }
+
+    //for unknown reasons, a simple equals call fails on both objects.
+    //assertThat(completeMultipartUpload).isEqualTo(completeMultipartUpload1)
+    assertThat(completeMultipartUpload.location()).isEqualTo(completeMultipartUpload1.location())
+    assertThat(completeMultipartUpload.bucket()).isEqualTo(completeMultipartUpload1.bucket())
+    assertThat(completeMultipartUpload.key()).isEqualTo(completeMultipartUpload1.key())
+    assertThat(completeMultipartUpload.eTag()).isEqualTo(completeMultipartUpload1.eTag())
+    assertThat(completeMultipartUpload.checksumCRC32()).isEqualTo(completeMultipartUpload1.checksumCRC32())
+    assertThat(completeMultipartUpload.checksumType()).isEqualTo(completeMultipartUpload1.checksumType())
   }
 
   @Test

pom.xml

@@ -21,7 +21,7 @@
 
   <groupId>com.adobe.testing</groupId>
   <artifactId>s3mock-parent</artifactId>
-  <version>4.7.1-SNAPSHOT</version>
+  <version>4.8.0-SNAPSHOT</version>
   <packaging>pom</packaging>
 
   <name>S3Mock - Parent</name>

server/pom.xml

@@ -22,7 +22,7 @@
   <parent>
     <groupId>com.adobe.testing</groupId>
     <artifactId>s3mock-parent</artifactId>
-    <version>4.7.1-SNAPSHOT</version>
+    <version>4.8.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>s3mock</artifactId>

server/src/main/java/com/adobe/testing/s3mock/MultipartController.java

@@ -420,25 +420,44 @@ public ResponseEntity<CompleteMultipartUploadResult> completeMultipartUpload(
       HttpServletRequest request,
       @RequestHeader HttpHeaders httpHeaders) {
     var bucket = bucketService.verifyBucketExists(bucketName);
-    multipartService.verifyMultipartUploadExists(bucketName, uploadId);
-    multipartService.verifyMultipartParts(bucketName, key.key(), uploadId, upload.parts());
+    var multipartUploadInfo = multipartService.verifyMultipartUploadExists(bucketName, uploadId, true);
+    var objectName = key.key();
+    boolean isCompleted = multipartUploadInfo != null && multipartUploadInfo.completed();
+    if (!isCompleted) {
+      multipartService.verifyMultipartParts(bucketName, objectName, uploadId, upload.parts());
+    }
     var s3ObjectMetadata = objectService.getObject(bucketName, key.key(), null);
     objectService.verifyObjectMatching(match, noneMatch, null, null, s3ObjectMetadata);
-    var objectName = key.key();
     var locationWithEncodedKey = request
         .getRequestURL()
         .toString()
         .replace(objectName, SdkHttpUtils.urlEncode(objectName));
 
-    var result = multipartService.completeMultipartUpload(bucketName,
-        key.key(),
-        uploadId,
-        upload.parts(),
-        encryptionHeadersFrom(httpHeaders),
-        locationWithEncodedKey,
-        checksumFrom(httpHeaders),
-        checksumAlgorithmFromHeader(httpHeaders)
-    );
+    CompleteMultipartUploadResult result;
+    if (!isCompleted) {
+      result = multipartService.completeMultipartUpload(
+          bucketName,
+          objectName,
+          uploadId,
+          upload.parts(),
+          encryptionHeadersFrom(httpHeaders),
+          locationWithEncodedKey,
+          checksumFrom(httpHeaders),
+          checksumAlgorithmFromHeader(httpHeaders)
+      );
+    } else {
+      result = CompleteMultipartUploadResult.from(
+          locationWithEncodedKey,
+          bucketName,
+          objectName,
+          s3ObjectMetadata.etag(),
+          multipartUploadInfo,
+          s3ObjectMetadata.checksum(),
+          s3ObjectMetadata.checksumType(),
+          s3ObjectMetadata.checksumAlgorithm(),
+          s3ObjectMetadata.versionId()
+      );
+    }
 
     return ResponseEntity
         .ok()

server/src/main/java/com/adobe/testing/s3mock/service/MultipartService.java

@@ -40,6 +40,7 @@
 import com.adobe.testing.s3mock.dto.Tag;
 import com.adobe.testing.s3mock.store.BucketStore;
 import com.adobe.testing.s3mock.store.MultipartStore;
+import com.adobe.testing.s3mock.store.MultipartUploadInfo;
 import java.nio.file.Path;
 import java.util.Comparator;
 import java.util.Date;
@@ -130,7 +131,7 @@ public ListPartsResult getMultipartUploadParts(
     if (id == null) {
       return null;
     }
-    var multipartUpload = multipartStore.getMultipartUpload(bucketMetadata, uploadId);
+    var multipartUpload = multipartStore.getMultipartUpload(bucketMetadata, uploadId, false);
     var parts = multipartStore.getMultipartUploadParts(bucketMetadata, id, uploadId)
         .stream()
         .toList();
@@ -321,8 +322,12 @@ public void verifyPartNumberLimits(String partNumberString) {
     }
   }
 
-  public void verifyMultipartParts(String bucketName, String key,
-      UUID uploadId, List<CompletedPart> requestedParts) throws S3Exception {
+  public void verifyMultipartParts(
+      String bucketName,
+      String key,
+      UUID uploadId,
+      List<CompletedPart> requestedParts
+  ) throws S3Exception {
     var bucketMetadata = bucketStore.getBucketMetadata(bucketName);
     var id = bucketMetadata.getID(key);
     if (id == null) {
@@ -353,7 +358,11 @@ public void verifyMultipartParts(String bucketName, String key,
     }
   }
 
-  public void verifyMultipartParts(String bucketName, UUID id, UUID uploadId) throws S3Exception {
+  public void verifyMultipartParts(
+      String bucketName,
+      UUID id,
+      UUID uploadId
+  ) throws S3Exception {
     verifyMultipartUploadExists(bucketName, uploadId);
     var bucketMetadata = bucketStore.getBucketMetadata(bucketName);
     var uploadedParts = multipartStore.getMultipartUploadParts(bucketMetadata, id, uploadId);
@@ -371,9 +380,18 @@ public void verifyMultipartParts(String bucketName, UUID id, UUID uploadId) thro
   }
 
   public void verifyMultipartUploadExists(String bucketName, UUID uploadId) throws S3Exception {
+    verifyMultipartUploadExists(bucketName, uploadId, false);
+  }
+
+  public @Nullable MultipartUploadInfo verifyMultipartUploadExists(
+      String bucketName,
+      UUID uploadId,
+      boolean includeCompleted
+  ) throws S3Exception {
     try {
       var bucketMetadata = bucketStore.getBucketMetadata(bucketName);
-      multipartStore.getMultipartUpload(bucketMetadata, uploadId);
+      multipartStore.getMultipartUpload(bucketMetadata, uploadId, includeCompleted);
+      return multipartStore.getMultipartUploadInfo(bucketMetadata, uploadId);
     } catch (IllegalArgumentException e) {
       throw NO_SUCH_UPLOAD_MULTIPART;
     }

server/src/main/java/com/adobe/testing/s3mock/store/MultipartStore.java

@@ -120,7 +120,9 @@ public MultipartUpload createMultipartUpload(
         tags,
         null,
         checksumType,
-        checksumAlgorithm);
+        checksumAlgorithm,
+        false
+    );
     lockStore.putIfAbsent(uploadId, new Object());
     writeMetafile(bucket, multipartUploadInfo);
 
@@ -139,7 +141,7 @@ public List<MultipartUpload> listMultipartUploads(BucketMetadata bucketMetadata,
               path -> {
                 var fileName = path.getFileName().toString();
                 var uploadMetadata = getUploadMetadata(bucketMetadata, UUID.fromString(fileName));
-                if (uploadMetadata != null) {
+                if (uploadMetadata != null && !uploadMetadata.completed()) {
                   return uploadMetadata.upload();
                 } else  {
                   return null;
@@ -161,10 +163,18 @@ public MultipartUploadInfo getMultipartUploadInfo(
     return getUploadMetadata(bucketMetadata, uploadId);
   }
 
-  public MultipartUpload getMultipartUpload(BucketMetadata bucketMetadata, UUID uploadId) {
+  public MultipartUpload getMultipartUpload(BucketMetadata bucketMetadata, UUID uploadId, boolean includeCompleted) {
     var uploadMetadata = getUploadMetadata(bucketMetadata, uploadId);
     if (uploadMetadata != null) {
-      return uploadMetadata.upload();
+      if (includeCompleted) {
+        return uploadMetadata.upload();
+      } else {
+        if (uploadMetadata.completed()) {
+          throw new IllegalArgumentException("No active MultipartUpload found with uploadId: " + uploadId);
+        } else {
+          return uploadMetadata.upload();
+        }
+      }
     } else {
       throw new IllegalArgumentException("No MultipartUpload found with uploadId: " + uploadId);
     }
@@ -243,12 +253,16 @@ public CompleteMultipartUploadResult completeMultipartUpload(
             uploadInfo.storageClass(),
             ChecksumType.COMPOSITE
         );
-        FileUtils.deleteDirectory(partFolder.toFile());
+        //delete parts and update MultipartInfo
+        partsPaths.forEach(partPath -> FileUtils.deleteQuietly(partPath.toFile()));
+        var completedUploadInfo = uploadInfo.complete();
+        writeMetafile(bucket, completedUploadInfo);
+
         return CompleteMultipartUploadResult.from(location,
-            uploadInfo.bucket(),
+            completedUploadInfo.bucket(),
             key,
             etag,
-            uploadInfo,
+            completedUploadInfo,
             checksumFor,
             s3ObjectMetadata.checksumType(),
             checksumAlgorithm,
@@ -343,8 +357,13 @@ public String copyPart(
 
     verifyMultipartUploadPreparation(destinationBucket, destinationId, uploadId);
 
-    return copyPartToFile(bucket, id, copyRange,
-        createPartFile(destinationBucket, destinationId, uploadId, partNumber), versionId);
+    return copyPartToFile(
+        bucket,
+        id,
+        copyRange,
+        createPartFile(destinationBucket, destinationId, uploadId, partNumber),
+        versionId
+    );
   }
 
   private static InputStream toInputStream(List<Path> paths) {