diff --git a/CHANGELOG.md b/CHANGELOG.md index c6e280c3b..d5ec856cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -165,7 +165,7 @@ 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. @@ -173,16 +173,17 @@ Version 4.x is JDK17 LTS bytecode compatible, with Docker and JUnit / direct Jav * 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. diff --git a/build-config/pom.xml b/build-config/pom.xml index 923b7045f..a3a5ab7b2 100644 --- a/build-config/pom.xml +++ b/build-config/pom.xml @@ -21,7 +21,7 @@ com.adobe.testing s3mock-parent - 4.7.1-SNAPSHOT + 4.8.0-SNAPSHOT s3mock-build-config diff --git a/docker/pom.xml b/docker/pom.xml index 18bb6c998..ff05ab970 100644 --- a/docker/pom.xml +++ b/docker/pom.xml @@ -22,7 +22,7 @@ com.adobe.testing s3mock-parent - 4.7.1-SNAPSHOT + 4.8.0-SNAPSHOT s3mock-docker diff --git a/integration-tests/pom.xml b/integration-tests/pom.xml index 944afe78f..a970c7db6 100644 --- a/integration-tests/pom.xml +++ b/integration-tests/pom.xml @@ -22,7 +22,7 @@ com.adobe.testing s3mock-parent - 4.7.1-SNAPSHOT + 4.8.0-SNAPSHOT s3mock-integration-tests diff --git a/integration-tests/src/test/kotlin/com/adobe/testing/s3mock/its/MultipartIT.kt b/integration-tests/src/test/kotlin/com/adobe/testing/s3mock/its/MultipartIT.kt index 0bbded963..4193a0a2b 100644 --- a/integration-tests/src/test/kotlin/com/adobe/testing/s3mock/its/MultipartIT.kt +++ b/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 diff --git a/pom.xml b/pom.xml index a527d821a..e73cf722e 100644 --- a/pom.xml +++ b/pom.xml @@ -21,7 +21,7 @@ com.adobe.testing s3mock-parent - 4.7.1-SNAPSHOT + 4.8.0-SNAPSHOT pom S3Mock - Parent diff --git a/server/pom.xml b/server/pom.xml index 929cd56c1..15faa0e88 100644 --- a/server/pom.xml +++ b/server/pom.xml @@ -22,7 +22,7 @@ com.adobe.testing s3mock-parent - 4.7.1-SNAPSHOT + 4.8.0-SNAPSHOT s3mock diff --git a/server/src/main/java/com/adobe/testing/s3mock/MultipartController.java b/server/src/main/java/com/adobe/testing/s3mock/MultipartController.java index 1e1bcc103..851df4f01 100644 --- a/server/src/main/java/com/adobe/testing/s3mock/MultipartController.java +++ b/server/src/main/java/com/adobe/testing/s3mock/MultipartController.java @@ -420,25 +420,44 @@ public ResponseEntity 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() diff --git a/server/src/main/java/com/adobe/testing/s3mock/service/MultipartService.java b/server/src/main/java/com/adobe/testing/s3mock/service/MultipartService.java index 795347f52..5ed305847 100644 --- a/server/src/main/java/com/adobe/testing/s3mock/service/MultipartService.java +++ b/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 requestedParts) throws S3Exception { + public void verifyMultipartParts( + String bucketName, + String key, + UUID uploadId, + List 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; } diff --git a/server/src/main/java/com/adobe/testing/s3mock/store/MultipartStore.java b/server/src/main/java/com/adobe/testing/s3mock/store/MultipartStore.java index 5c753ca3d..7e4b2e9dd 100644 --- a/server/src/main/java/com/adobe/testing/s3mock/store/MultipartStore.java +++ b/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 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 paths) { diff --git a/server/src/main/java/com/adobe/testing/s3mock/store/MultipartUploadInfo.java b/server/src/main/java/com/adobe/testing/s3mock/store/MultipartUploadInfo.java index d075305cd..836a83b02 100644 --- a/server/src/main/java/com/adobe/testing/s3mock/store/MultipartUploadInfo.java +++ b/server/src/main/java/com/adobe/testing/s3mock/store/MultipartUploadInfo.java @@ -39,5 +39,53 @@ public record MultipartUploadInfo( List tags, @Nullable String checksum, @Nullable ChecksumType checksumType, - @Nullable ChecksumAlgorithm checksumAlgorithm) { + @Nullable ChecksumAlgorithm checksumAlgorithm, + boolean completed +) { + + public MultipartUploadInfo( + MultipartUpload upload, + String contentType, + Map userMetadata, + Map storeHeaders, + Map encryptionHeaders, + String bucket, + StorageClass storageClass, + List tags, + String checksum, + ChecksumType checksumType, + ChecksumAlgorithm checksumAlgorithm + ) { + this( + upload, + contentType, + userMetadata, + storeHeaders, + encryptionHeaders, + bucket, + storageClass, + tags, + checksum, + checksumType, + checksumAlgorithm, + false + ); + } + + public MultipartUploadInfo complete() { + return new MultipartUploadInfo( + this.upload, + this.contentType, + this.userMetadata, + this.storeHeaders, + this.encryptionHeaders, + this.bucket, + this.storageClass, + this.tags, + this.checksum, + this.checksumType, + this.checksumAlgorithm, + true + ); + } } diff --git a/server/src/test/kotlin/com/adobe/testing/s3mock/MultipartControllerTest.kt b/server/src/test/kotlin/com/adobe/testing/s3mock/MultipartControllerTest.kt index ae21a6522..1a900e018 100644 --- a/server/src/test/kotlin/com/adobe/testing/s3mock/MultipartControllerTest.kt +++ b/server/src/test/kotlin/com/adobe/testing/s3mock/MultipartControllerTest.kt @@ -334,7 +334,8 @@ internal class MultipartControllerTest : BaseControllerTest() { emptyList(), null, ChecksumType.FULL_OBJECT, - null + null, + false ) val result = CompleteMultipartUploadResult.from( "http://localhost/${TEST_BUCKET_NAME}/$key", @@ -410,7 +411,8 @@ internal class MultipartControllerTest : BaseControllerTest() { emptyList(), null, ChecksumType.FULL_OBJECT, - null + null, + false ) val result = CompleteMultipartUploadResult.from( "http://localhost/${TEST_BUCKET_NAME}/$key", @@ -479,7 +481,8 @@ internal class MultipartControllerTest : BaseControllerTest() { emptyList(), null, ChecksumType.FULL_OBJECT, - null + null, + false ) val result = CompleteMultipartUploadResult.from( "http://localhost/${TEST_BUCKET_NAME}/$key", @@ -603,7 +606,7 @@ internal class MultipartControllerTest : BaseControllerTest() { doThrow(S3Exception.NO_SUCH_UPLOAD_MULTIPART) .whenever(multipartService) - .verifyMultipartUploadExists(TEST_BUCKET_NAME, uploadId) + .verifyMultipartUploadExists(TEST_BUCKET_NAME, uploadId, true) val uploadRequest = CompleteMultipartUpload(ArrayList()) uploadRequest.addPart(CompletedPart(null, null, null, null, null, "etag1", 1)) diff --git a/server/src/test/kotlin/com/adobe/testing/s3mock/dto/CompleteMultipartUploadResultTest.kt b/server/src/test/kotlin/com/adobe/testing/s3mock/dto/CompleteMultipartUploadResultTest.kt index 9519f2f3a..f4f21d944 100644 --- a/server/src/test/kotlin/com/adobe/testing/s3mock/dto/CompleteMultipartUploadResultTest.kt +++ b/server/src/test/kotlin/com/adobe/testing/s3mock/dto/CompleteMultipartUploadResultTest.kt @@ -51,7 +51,8 @@ internal class CompleteMultipartUploadResultTest { listOf(Tag("key", "value")), "checksumSHA256", ChecksumType.COMPOSITE, - ChecksumAlgorithm.SHA256 + ChecksumAlgorithm.SHA256, + false ), "checksumSHA256", ChecksumType.COMPOSITE, diff --git a/server/src/test/kotlin/com/adobe/testing/s3mock/service/MultipartServiceTest.kt b/server/src/test/kotlin/com/adobe/testing/s3mock/service/MultipartServiceTest.kt index a5565aec5..a8dd9ef26 100644 --- a/server/src/test/kotlin/com/adobe/testing/s3mock/service/MultipartServiceTest.kt +++ b/server/src/test/kotlin/com/adobe/testing/s3mock/service/MultipartServiceTest.kt @@ -24,7 +24,8 @@ import com.adobe.testing.s3mock.store.MultipartStore import com.adobe.testing.s3mock.store.ObjectStore import org.assertj.core.api.Assertions.assertThatThrownBy import org.junit.jupiter.api.Test -import org.mockito.ArgumentMatchers +import org.mockito.ArgumentMatchers.any +import org.mockito.ArgumentMatchers.eq import org.mockito.kotlin.whenever import org.springframework.beans.factory.annotation.Autowired import org.springframework.boot.test.context.SpringBootTest @@ -195,9 +196,9 @@ internal class MultipartServiceTest : ServiceTestBase() { ) whenever( multipartStore.getMultipartUpload( - ArgumentMatchers.any( - BucketMetadata::class.java - ), ArgumentMatchers.eq(uploadId) + any(BucketMetadata::class.java), + eq(uploadId), + eq(false) ) ) .thenThrow(IllegalArgumentException()) @@ -277,8 +278,9 @@ internal class MultipartServiceTest : ServiceTestBase() { // Simulate missing upload -> MultipartService should translate to NO_SUCH_UPLOAD_MULTIPART whenever( multipartStore.getMultipartUpload( - ArgumentMatchers.eq(bucketMetadata), - ArgumentMatchers.eq(uploadId) + eq(bucketMetadata), + eq(uploadId), + eq(false) ) ).thenThrow(IllegalArgumentException()) diff --git a/testsupport/common/pom.xml b/testsupport/common/pom.xml index d795a8b50..6f2d43af4 100644 --- a/testsupport/common/pom.xml +++ b/testsupport/common/pom.xml @@ -22,7 +22,7 @@ com.adobe.testing s3mock-testsupport-reactor - 4.7.1-SNAPSHOT + 4.8.0-SNAPSHOT s3mock-testsupport-common diff --git a/testsupport/junit4/pom.xml b/testsupport/junit4/pom.xml index 17ddbc7f4..120820d14 100644 --- a/testsupport/junit4/pom.xml +++ b/testsupport/junit4/pom.xml @@ -22,7 +22,7 @@ com.adobe.testing s3mock-testsupport-reactor - 4.7.1-SNAPSHOT + 4.8.0-SNAPSHOT s3mock-junit4 diff --git a/testsupport/junit5/pom.xml b/testsupport/junit5/pom.xml index e807e1c7e..8c633d73d 100644 --- a/testsupport/junit5/pom.xml +++ b/testsupport/junit5/pom.xml @@ -22,7 +22,7 @@ com.adobe.testing s3mock-testsupport-reactor - 4.7.1-SNAPSHOT + 4.8.0-SNAPSHOT s3mock-junit5 diff --git a/testsupport/pom.xml b/testsupport/pom.xml index 3a56c8a71..4771f5830 100644 --- a/testsupport/pom.xml +++ b/testsupport/pom.xml @@ -22,7 +22,7 @@ com.adobe.testing s3mock-parent - 4.7.1-SNAPSHOT + 4.8.0-SNAPSHOT s3mock-testsupport-reactor diff --git a/testsupport/testcontainers/pom.xml b/testsupport/testcontainers/pom.xml index 8ca64edbc..2af44e737 100644 --- a/testsupport/testcontainers/pom.xml +++ b/testsupport/testcontainers/pom.xml @@ -22,7 +22,7 @@ com.adobe.testing s3mock-testsupport-reactor - 4.7.1-SNAPSHOT + 4.8.0-SNAPSHOT s3mock-testcontainers diff --git a/testsupport/testng/pom.xml b/testsupport/testng/pom.xml index 8d7bf3434..323fe53df 100644 --- a/testsupport/testng/pom.xml +++ b/testsupport/testng/pom.xml @@ -22,7 +22,7 @@ com.adobe.testing s3mock-testsupport-reactor - 4.7.1-SNAPSHOT + 4.8.0-SNAPSHOT s3mock-testng