Skip to content

fix(storage): do not apply retention or holds to parallel composite upload parts #14894

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sandeepvinayak wants to merge 1 commit into
base: main
Choose a base branch
from
+91 −0
Open

fix(storage): do not apply retention or holds to parallel composite upload parts #14894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...ge/src/main/java/com/google/cloud/storage/ParallelCompositeUploadWritableByteChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,11 @@ private BlobInfo definePart(BlobInfo ultimateObject, PartRange partRange, long o
// however, gcs will not accept that version resource identifier when creating a new object
// strip it out, so it can be included as a query string parameter instead
b.setKmsKeyName(null);
// parts are transient and must remain deletable. Retention and holds only belong on the
// ultimate object, which the final compose applies from ultimateObject.
b.setRetention(null);
b.setTemporaryHold(null);
b.setEventBasedHold(null);
b = partMetadataFieldDecorator.apply(b);
return b.build();
}
Expand Down
86 changes: 86 additions & 0 deletions ...rc/test/java/com/google/cloud/storage/ParallelCompositeUploadWritableByteChannelTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.api.gax.grpc.GrpcStatusCode;
import com.google.api.gax.rpc.ApiExceptionFactory;
import com.google.api.gax.rpc.ApiExceptions;
import com.google.cloud.storage.BlobInfo.Retention;
import com.google.cloud.storage.BufferHandlePool.PooledBuffer;
import com.google.cloud.storage.Crc32cValue.Crc32cLengthKnown;
import com.google.cloud.storage.MetadataField.PartRange;
Expand Down Expand Up @@ -846,6 +847,91 @@ public BlobInfo compose(ComposeRequest composeRequest) {
pcu.close();
}

@Test
public void partsDoNotInheritRetentionOrHolds() throws Exception {
Retention retention =
Retention.newBuilder()
.setMode(Retention.Mode.LOCKED)
.setRetainUntilTime(OffsetDateTime.parse("2100-01-01T00:00:00Z"))
.build();
BlobInfo ultimate =
info.toBuilder()
.setRetention(retention)
.setTemporaryHold(true)
.setEventBasedHold(true)
.build();

List<BlobInfo> partTargets = Collections.synchronizedList(new ArrayList<>());
AtomicReference<BlobInfo> finalTarget = new AtomicReference<>();
FakeStorageInternal storageInternal =
new FakeStorageInternal() {
@Override
public BlobInfo internalDirectUpload(
BlobInfo info, Opts<ObjectTargetOpt> opts, ByteBuffer buf) {
partTargets.add(info);
return super.internalDirectUpload(info, opts, buf);
}

@Override
public BlobInfo compose(ComposeRequest composeRequest) {
BlobInfo target = composeRequest.getTarget();
if (target.getBlobId().getName().startsWith(partNamingStrategy.prefix)) {
partTargets.add(target);
} else {
finalTarget.set(target);
}
return super.compose(composeRequest);
}

@Override
public Void internalObjectDelete(BlobId id, Opts<ObjectSourceOpt> opts) {
// mimic GCS: retained or held objects cannot be deleted
Data d = addedObjects.get(id);
if (d != null) {
BlobInfo i = d.getInfo();
if (i.getRetention() != null
|| Boolean.TRUE.equals(i.getTemporaryHold())
|| Boolean.TRUE.equals(i.getEventBasedHold())) {
deleteRequests.add(id);
throw ApiExceptionFactory.createException(
null, GrpcStatusCode.of(Code.PERMISSION_DENIED), false);
}
}
return super.internalObjectDelete(id, opts);
}
};
ParallelCompositeUploadWritableByteChannel pcu =
new ParallelCompositeUploadWritableByteChannel(
bufferHandlePool,
MoreExecutors.directExecutor(),
partNamingStrategy,
PartCleanupStrategy.always(),
2,
partMetadataFieldDecorator,
finalObject,
storageInternal,
ultimate,
opts);

byte[] bytes = DataGenerator.base64Characters().genBytes(bufferCapacity * 3 - 1);
pcu.write(ByteBuffer.wrap(bytes));
pcu.close();

BlobInfo result = ApiFutureUtils.await(finalObject);

assertAll(
() -> assertThat(result.getName()).isEqualTo(info.getName()),
// 3 direct uploads + 2 intermediate composes
() -> assertThat(partTargets).hasSize(5),
() -> assertThat(partTargets.stream().allMatch(p -> p.getRetention() == null)).isTrue(),
() -> assertThat(partTargets.stream().allMatch(p -> p.getTemporaryHold() == null)).isTrue(),
() ->
assertThat(partTargets.stream().allMatch(p -> p.getEventBasedHold() == null)).isTrue(),
Comment on lines +926 to +929

@gemini-code-assist gemini-code-assist Bot Nov 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using stream().allMatch(...).isTrue() in AssertJ assertions is an anti-pattern because it produces unhelpful error messages (e.g., Expected true but was false) if the assertion fails.

Using allSatisfy provides much more descriptive failure messages, showing exactly which element failed and what its actual values were.

Suggested change
() -> assertThat(partTargets.stream().allMatch(p -> p.getRetention() == null)).isTrue(),
() -> assertThat(partTargets.stream().allMatch(p -> p.getTemporaryHold() == null)).isTrue(),
() ->
assertThat(partTargets.stream().allMatch(p -> p.getEventBasedHold() == null)).isTrue(),
() -> assertThat(partTargets).allSatisfy(p -> {
assertThat(p.getRetention()).isNull();
assertThat(p.getTemporaryHold()).isNull();
assertThat(p.getEventBasedHold()).isNull();
}),

() -> assertThat(finalTarget.get().getRetention()).isEqualTo(retention),
() -> assertThat(finalTarget.get().getTemporaryHold()).isTrue(),
() -> assertThat(finalTarget.get().getEventBasedHold()).isTrue());
}

@NonNull
private ParallelCompositeUploadWritableByteChannel defaultPcu(int maxElementsPerCompact) {
return new ParallelCompositeUploadWritableByteChannel(
Expand Down
Loading