Skip to content

Commit 6f9634a

Browse files
afrankenafranken
committed
CompleteMultipartUpload is idempotent
CompleteMultipartUpload will not delete MultipartUpload data, only the parts. This is only part of a fix, we really shouldn't delete parts at all and preserve all multipart data, streaming a full binary on getObject requests. Fixes #2586
1 parent 35ecdee commit 6f9634a

File tree

9 files changed

+152
-40
lines changed

9 files changed

+152
-40
lines changed

‎CHANGELOG.md‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -165,24 +165,25 @@ Version 4.x is JDK17 LTS bytecode compatible, with Docker and JUnit / direct Jav
165165
Version 4.x is JDK17 LTS bytecode compatible, with Docker and JUnit / direct Java integration.
166166

167167
* Features and fixes
168-
* TBD
168+
* CompleteMultipartUpload is idempotent (fixes #2586)
169169
* Refactorings
170170
* UploadId is always a UUID. Use UUID type in S3Mock instead of String.
171171
* Validate that partNumbers to be positive integers.
172172
* Force convergence on the newest available transitive dependency versions.
173173
* Optimize file storage for large objects by using buffered streams.
174174
* Version updates (deliverable dependencies)
175175
* Bump spring-boot.version from 3.5.4 to 3.5.5
176-
* Bump aws-v2.version from 2.32.7 to 2.32.23
176+
* Bump aws-v2.version from 2.32.7 to 2.32.31
177177
* Bump org.apache.commons:commons-compress from 1.27.1 to 1.28.0
178178
* Version updates (build dependencies)
179179
* Bump kotlin.version from 2.2.0 to 2.2.10
180-
* Bump aws.sdk.kotlin:s3-jvm from 1.4.125 to 1.5.19
180+
* Bump aws.sdk.kotlin:s3-jvm from 1.4.125 to 1.5.26
181181
* Bump digital.pragmatech.testing:spring-test-profiler from 0.0.5 to 0.0.11
182182
* Bump com.puppycrawl.tools:checkstyle from 10.26.1 to 11.0.0
183183
* Bump github/codeql-action from 3.29.4 to 3.29.11
184184
* Bump actions/checkout from 4.2.2 to 5.0.0
185185
* Bump actions/setup-java from 4.7.1 to 5.0.0
186+
* Bump actions/dependency-review-action from 4.7.2 to 4.7.3
186187

187188
## 4.7.0
188189
Version 4.x is JDK17 LTS bytecode compatible, with Docker and JUnit / direct Java integration.

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,36 @@ internal class MultipartIT : S3TestBase() {
343343

344344
assertThat(completeMultipartUpload.location())
345345
.isEqualTo("${serviceEndpoint}/$bucketName/${UriUtils.encode(TEST_IMAGE_TIFF, StandardCharsets.UTF_8)}")
346+
347+
//verify completeMultipartUpload is idempotent
348+
val completeMultipartUpload1 = s3Client.completeMultipartUpload {
349+
it.bucket(initiateMultipartUploadResult.bucket())
350+
it.key(initiateMultipartUploadResult.key())
351+
it.uploadId(initiateMultipartUploadResult.uploadId())
352+
it.multipartUpload {
353+
it.parts(
354+
{
355+
it.eTag(etag1)
356+
it.partNumber(1)
357+
it.checksumCRC32(checksum1)
358+
},
359+
{
360+
it.eTag(etag2)
361+
it.partNumber(2)
362+
it.checksumCRC32(checksum2)
363+
}
364+
)
365+
}
366+
}
367+
368+
//for unknown reasons, a simple equals call fails on both objects.
369+
//assertThat(completeMultipartUpload).isEqualTo(completeMultipartUpload1)
370+
assertThat(completeMultipartUpload.location()).isEqualTo(completeMultipartUpload1.location())
371+
assertThat(completeMultipartUpload.bucket()).isEqualTo(completeMultipartUpload1.bucket())
372+
assertThat(completeMultipartUpload.key()).isEqualTo(completeMultipartUpload1.key())
373+
assertThat(completeMultipartUpload.eTag()).isEqualTo(completeMultipartUpload1.eTag())
374+
assertThat(completeMultipartUpload.checksumCRC32()).isEqualTo(completeMultipartUpload1.checksumCRC32())
375+
assertThat(completeMultipartUpload.checksumType()).isEqualTo(completeMultipartUpload1.checksumType())
346376
}
347377

348378
@Test

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

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -420,8 +420,11 @@ public ResponseEntity<CompleteMultipartUploadResult> completeMultipartUpload(
420420
HttpServletRequest request,
421421
@RequestHeader HttpHeaders httpHeaders) {
422422
var bucket = bucketService.verifyBucketExists(bucketName);
423-
multipartService.verifyMultipartUploadExists(bucketName, uploadId);
424-
multipartService.verifyMultipartParts(bucketName, key.key(), uploadId, upload.parts());
423+
var multipartUploadInfo = multipartService.verifyMultipartUploadExists(bucketName, uploadId, true);
424+
boolean isCompleted = multipartUploadInfo != null && Boolean.TRUE.equals(multipartUploadInfo.completed());
425+
if (!isCompleted) {
426+
multipartService.verifyMultipartParts(bucketName, key.key(), uploadId, upload.parts());
427+
}
425428
var s3ObjectMetadata = objectService.getObject(bucketName, key.key(), null);
426429
objectService.verifyObjectMatching(match, noneMatch, null, null, s3ObjectMetadata);
427430
var objectName = key.key();
@@ -430,15 +433,31 @@ public ResponseEntity<CompleteMultipartUploadResult> completeMultipartUpload(
430433
.toString()
431434
.replace(objectName, SdkHttpUtils.urlEncode(objectName));
432435

433-
var result = multipartService.completeMultipartUpload(bucketName,
434-
key.key(),
435-
uploadId,
436-
upload.parts(),
437-
encryptionHeadersFrom(httpHeaders),
438-
locationWithEncodedKey,
439-
checksumFrom(httpHeaders),
440-
checksumAlgorithmFromHeader(httpHeaders)
441-
);
436+
CompleteMultipartUploadResult result;
437+
if (!isCompleted) {
438+
result = multipartService.completeMultipartUpload(
439+
bucketName,
440+
key.key(),
441+
uploadId,
442+
upload.parts(),
443+
encryptionHeadersFrom(httpHeaders),
444+
locationWithEncodedKey,
445+
checksumFrom(httpHeaders),
446+
checksumAlgorithmFromHeader(httpHeaders)
447+
);
448+
} else {
449+
result = CompleteMultipartUploadResult.from(
450+
locationWithEncodedKey,
451+
bucketName,
452+
key.key(),
453+
s3ObjectMetadata.etag(),
454+
multipartUploadInfo,
455+
s3ObjectMetadata.checksum(),
456+
s3ObjectMetadata.checksumType(),
457+
s3ObjectMetadata.checksumAlgorithm(),
458+
s3ObjectMetadata.versionId()
459+
);
460+
}
442461

443462
return ResponseEntity
444463
.ok()

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

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import com.adobe.testing.s3mock.dto.Tag;
4141
import com.adobe.testing.s3mock.store.BucketStore;
4242
import com.adobe.testing.s3mock.store.MultipartStore;
43+
import com.adobe.testing.s3mock.store.MultipartUploadInfo;
4344
import java.nio.file.Path;
4445
import java.util.Comparator;
4546
import java.util.Date;
@@ -130,7 +131,7 @@ public ListPartsResult getMultipartUploadParts(
130131
if (id == null) {
131132
return null;
132133
}
133-
var multipartUpload = multipartStore.getMultipartUpload(bucketMetadata, uploadId);
134+
var multipartUpload = multipartStore.getMultipartUpload(bucketMetadata, uploadId, false);
134135
var parts = multipartStore.getMultipartUploadParts(bucketMetadata, id, uploadId)
135136
.stream()
136137
.toList();
@@ -321,8 +322,12 @@ public void verifyPartNumberLimits(String partNumberString) {
321322
}
322323
}
323324

324-
public void verifyMultipartParts(String bucketName, String key,
325-
UUID uploadId, List<CompletedPart> requestedParts) throws S3Exception {
325+
public void verifyMultipartParts(
326+
String bucketName,
327+
String key,
328+
UUID uploadId,
329+
List<CompletedPart> requestedParts
330+
) throws S3Exception {
326331
var bucketMetadata = bucketStore.getBucketMetadata(bucketName);
327332
var id = bucketMetadata.getID(key);
328333
if (id == null) {
@@ -353,7 +358,11 @@ public void verifyMultipartParts(String bucketName, String key,
353358
}
354359
}
355360

356-
public void verifyMultipartParts(String bucketName, UUID id, UUID uploadId) throws S3Exception {
361+
public void verifyMultipartParts(
362+
String bucketName,
363+
UUID id,
364+
UUID uploadId
365+
) throws S3Exception {
357366
verifyMultipartUploadExists(bucketName, uploadId);
358367
var bucketMetadata = bucketStore.getBucketMetadata(bucketName);
359368
var uploadedParts = multipartStore.getMultipartUploadParts(bucketMetadata, id, uploadId);
@@ -371,9 +380,18 @@ public void verifyMultipartParts(String bucketName, UUID id, UUID uploadId) thro
371380
}
372381

373382
public void verifyMultipartUploadExists(String bucketName, UUID uploadId) throws S3Exception {
383+
verifyMultipartUploadExists(bucketName, uploadId, false);
384+
}
385+
386+
public @Nullable MultipartUploadInfo verifyMultipartUploadExists(
387+
String bucketName,
388+
UUID uploadId,
389+
boolean includeCompleted
390+
) throws S3Exception {
374391
try {
375392
var bucketMetadata = bucketStore.getBucketMetadata(bucketName);
376-
multipartStore.getMultipartUpload(bucketMetadata, uploadId);
393+
multipartStore.getMultipartUpload(bucketMetadata, uploadId, includeCompleted);
394+
return multipartStore.getMultipartUploadInfo(bucketMetadata, uploadId);
377395
} catch (IllegalArgumentException e) {
378396
throw NO_SUCH_UPLOAD_MULTIPART;
379397
}

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

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,9 @@ public MultipartUpload createMultipartUpload(
120120
tags,
121121
null,
122122
checksumType,
123-
checksumAlgorithm);
123+
checksumAlgorithm,
124+
false
125+
);
124126
lockStore.putIfAbsent(uploadId, new Object());
125127
writeMetafile(bucket, multipartUploadInfo);
126128

@@ -139,7 +141,7 @@ public List<MultipartUpload> listMultipartUploads(BucketMetadata bucketMetadata,
139141
path -> {
140142
var fileName = path.getFileName().toString();
141143
var uploadMetadata = getUploadMetadata(bucketMetadata, UUID.fromString(fileName));
142-
if (uploadMetadata != null) {
144+
if (uploadMetadata != null && !uploadMetadata.completed()) {
143145
return uploadMetadata.upload();
144146
} else {
145147
return null;
@@ -161,10 +163,18 @@ public MultipartUploadInfo getMultipartUploadInfo(
161163
return getUploadMetadata(bucketMetadata, uploadId);
162164
}
163165

164-
public MultipartUpload getMultipartUpload(BucketMetadata bucketMetadata, UUID uploadId) {
166+
public MultipartUpload getMultipartUpload(BucketMetadata bucketMetadata, UUID uploadId, boolean includeCompleted) {
165167
var uploadMetadata = getUploadMetadata(bucketMetadata, uploadId);
166168
if (uploadMetadata != null) {
167-
return uploadMetadata.upload();
169+
if (includeCompleted) {
170+
return uploadMetadata.upload();
171+
} else {
172+
if (uploadMetadata.completed() != null && !uploadMetadata.completed()) {
173+
return uploadMetadata.upload();
174+
} else {
175+
throw new IllegalArgumentException("No active MultipartUpload found with uploadId: " + uploadId);
176+
}
177+
}
168178
} else {
169179
throw new IllegalArgumentException("No MultipartUpload found with uploadId: " + uploadId);
170180
}
@@ -243,12 +253,16 @@ public CompleteMultipartUploadResult completeMultipartUpload(
243253
uploadInfo.storageClass(),
244254
ChecksumType.COMPOSITE
245255
);
246-
FileUtils.deleteDirectory(partFolder.toFile());
256+
//delete parts and update MultipartInfo
257+
partsPaths.forEach(partPath -> FileUtils.deleteQuietly(partPath.toFile()));
258+
var completedUploadInfo = uploadInfo.complete();
259+
writeMetafile(bucket, completedUploadInfo);
260+
247261
return CompleteMultipartUploadResult.from(location,
248-
uploadInfo.bucket(),
262+
completedUploadInfo.bucket(),
249263
key,
250264
etag,
251-
uploadInfo,
265+
completedUploadInfo,
252266
checksumFor,
253267
s3ObjectMetadata.checksumType(),
254268
checksumAlgorithm,
@@ -343,8 +357,13 @@ public String copyPart(
343357

344358
verifyMultipartUploadPreparation(destinationBucket, destinationId, uploadId);
345359

346-
return copyPartToFile(bucket, id, copyRange,
347-
createPartFile(destinationBucket, destinationId, uploadId, partNumber), versionId);
360+
return copyPartToFile(
361+
bucket,
362+
id,
363+
copyRange,
364+
createPartFile(destinationBucket, destinationId, uploadId, partNumber),
365+
versionId
366+
);
348367
}
349368

350369
private static InputStream toInputStream(List<Path> paths) {

‎server/src/main/java/com/adobe/testing/s3mock/store/MultipartUploadInfo.java‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,5 +39,24 @@ public record MultipartUploadInfo(
3939
List<Tag> tags,
4040
@Nullable String checksum,
4141
@Nullable ChecksumType checksumType,
42-
@Nullable ChecksumAlgorithm checksumAlgorithm) {
42+
@Nullable ChecksumAlgorithm checksumAlgorithm,
43+
@Nullable Boolean completed
44+
) {
45+
46+
public MultipartUploadInfo complete() {
47+
return new MultipartUploadInfo(
48+
this.upload,
49+
this.contentType,
50+
this.userMetadata,
51+
this.storeHeaders,
52+
this.encryptionHeaders,
53+
this.bucket,
54+
this.storageClass,
55+
this.tags,
56+
this.checksum,
57+
this.checksumType,
58+
this.checksumAlgorithm,
59+
true
60+
);
61+
}
4362
}

‎server/src/test/kotlin/com/adobe/testing/s3mock/MultipartControllerTest.kt‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,8 @@ internal class MultipartControllerTest : BaseControllerTest() {
334334
emptyList(),
335335
null,
336336
ChecksumType.FULL_OBJECT,
337-
null
337+
null,
338+
false
338339
)
339340
val result = CompleteMultipartUploadResult.from(
340341
"http://localhost/${TEST_BUCKET_NAME}/$key",
@@ -410,7 +411,8 @@ internal class MultipartControllerTest : BaseControllerTest() {
410411
emptyList(),
411412
null,
412413
ChecksumType.FULL_OBJECT,
413-
null
414+
null,
415+
false
414416
)
415417
val result = CompleteMultipartUploadResult.from(
416418
"http://localhost/${TEST_BUCKET_NAME}/$key",
@@ -479,7 +481,8 @@ internal class MultipartControllerTest : BaseControllerTest() {
479481
emptyList(),
480482
null,
481483
ChecksumType.FULL_OBJECT,
482-
null
484+
null,
485+
false
483486
)
484487
val result = CompleteMultipartUploadResult.from(
485488
"http://localhost/${TEST_BUCKET_NAME}/$key",
@@ -603,7 +606,7 @@ internal class MultipartControllerTest : BaseControllerTest() {
603606

604607
doThrow(S3Exception.NO_SUCH_UPLOAD_MULTIPART)
605608
.whenever(multipartService)
606-
.verifyMultipartUploadExists(TEST_BUCKET_NAME, uploadId)
609+
.verifyMultipartUploadExists(TEST_BUCKET_NAME, uploadId, true)
607610

608611
val uploadRequest = CompleteMultipartUpload(ArrayList())
609612
uploadRequest.addPart(CompletedPart(null, null, null, null, null, "etag1", 1))

‎server/src/test/kotlin/com/adobe/testing/s3mock/dto/CompleteMultipartUploadResultTest.kt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ internal class CompleteMultipartUploadResultTest {
5151
listOf(Tag("key", "value")),
5252
"checksumSHA256",
5353
ChecksumType.COMPOSITE,
54-
ChecksumAlgorithm.SHA256
54+
ChecksumAlgorithm.SHA256,
55+
false
5556
),
5657
"checksumSHA256",
5758
ChecksumType.COMPOSITE,

‎server/src/test/kotlin/com/adobe/testing/s3mock/service/MultipartServiceTest.kt‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@ import com.adobe.testing.s3mock.store.MultipartStore
2424
import com.adobe.testing.s3mock.store.ObjectStore
2525
import org.assertj.core.api.Assertions.assertThatThrownBy
2626
import org.junit.jupiter.api.Test
27-
import org.mockito.ArgumentMatchers
27+
import org.mockito.ArgumentMatchers.any
28+
import org.mockito.ArgumentMatchers.eq
2829
import org.mockito.kotlin.whenever
2930
import org.springframework.beans.factory.annotation.Autowired
3031
import org.springframework.boot.test.context.SpringBootTest
@@ -195,9 +196,9 @@ internal class MultipartServiceTest : ServiceTestBase() {
195196
)
196197
whenever(
197198
multipartStore.getMultipartUpload(
198-
ArgumentMatchers.any(
199-
BucketMetadata::class.java
200-
), ArgumentMatchers.eq(uploadId)
199+
any(BucketMetadata::class.java),
200+
eq(uploadId),
201+
eq(false)
201202
)
202203
)
203204
.thenThrow(IllegalArgumentException())
@@ -277,8 +278,9 @@ internal class MultipartServiceTest : ServiceTestBase() {
277278
// Simulate missing upload -> MultipartService should translate to NO_SUCH_UPLOAD_MULTIPART
278279
whenever(
279280
multipartStore.getMultipartUpload(
280-
ArgumentMatchers.eq(bucketMetadata),
281-
ArgumentMatchers.eq(uploadId)
281+
eq(bucketMetadata),
282+
eq(uploadId),
283+
eq(false)
282284
)
283285
).thenThrow(IllegalArgumentException())
284286

0 commit comments

Comments
 (0)