From 0adeb14479fc9aef35e32d286bcd9ae414eda25a Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Fri, 28 Apr 2023 16:57:09 -0400 Subject: [PATCH] fix: make Blob and Bucket update diff aware (#1994) If a blob or bucket update does not actually contain a diff, instead of sending the empty update refresh the metadata. Add integration tests to verify behavior of updating object or bucket with no modification --- .../google/cloud/storage/GrpcStorageImpl.java | 87 +++++++++++------- .../com/google/cloud/storage/StorageImpl.java | 90 +++++++++++-------- .../com/google/cloud/storage/UnifiedOpts.java | 10 +++ .../conformance/retry/RpcMethodMappings.java | 54 +++++++++-- .../google/cloud/storage/it/ITBucketTest.java | 19 ++++ .../google/cloud/storage/it/ITObjectTest.java | 11 +++ .../storage/it/ITOptionRegressionTest.java | 3 +- 7 files changed, 198 insertions(+), 76 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java index ce5ebf0433..e8faef4d2f 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java @@ -146,6 +146,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; import java.util.stream.StreamSupport; +import org.checkerframework.checker.nullness.qual.Nullable; @BetaApi final class GrpcStorageImpl extends BaseService implements Storage { @@ -381,17 +382,8 @@ public Blob createFrom( @Override public Bucket get(String bucket, BucketGetOption... options) { - Opts opts = Opts.unwrap(options).prepend(defaultOpts); - GrpcCallContext grpcCallContext = - opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault()); - GetBucketRequest.Builder builder = - GetBucketRequest.newBuilder().setName(bucketNameCodec.encode(bucket)); - GetBucketRequest req = opts.getBucketsRequest().apply(builder).build(); - return Retrying.run( - getOptions(), - retryAlgorithmManager.getFor(req), - () -> storageClient.getBucketCallable().call(req, grpcCallContext), - syntaxDecoders.bucket.andThen(opts.clearBucketFields())); + Opts unwrap = Opts.unwrap(options); + return internalBucketGet(bucket, unwrap); } @Override @@ -418,26 +410,8 @@ public Blob get(String bucket, String blob, BlobGetOption... options) { @Override public Blob get(BlobId blob, BlobGetOption... options) { - Opts opts = Opts.unwrap(options).resolveFrom(blob).prepend(defaultOpts); - GrpcCallContext grpcCallContext = - opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault()); - GetObjectRequest.Builder builder = - GetObjectRequest.newBuilder() - .setBucket(bucketNameCodec.encode(blob.getBucket())) - .setObject(blob.getName()); - ifNonNull(blob.getGeneration(), builder::setGeneration); - GetObjectRequest req = opts.getObjectsRequest().apply(builder).build(); - return Retrying.run( - getOptions(), - retryAlgorithmManager.getFor(req), - () -> { - try { - return storageClient.getObjectCallable().call(req, grpcCallContext); - } catch (NotFoundException ignore) { - return null; - } - }, - syntaxDecoders.blob.andThen(opts.clearBlobFields())); + Opts unwrap = Opts.unwrap(options); + return internalBlobGet(blob, unwrap); } @Override @@ -493,7 +467,11 @@ public Page list(String bucket, BlobListOption... options) { @Override public Bucket update(BucketInfo bucketInfo, BucketTargetOption... options) { - Opts opts = Opts.unwrap(options).resolveFrom(bucketInfo).prepend(defaultOpts); + Opts unwrap = Opts.unwrap(options); + if (bucketInfo.getModifiedFields().isEmpty()) { + return internalBucketGet(bucketInfo.getName(), unwrap.constrainTo(BucketSourceOpt.class)); + } + Opts opts = unwrap.resolveFrom(bucketInfo).prepend(defaultOpts); GrpcCallContext grpcCallContext = opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault()); com.google.storage.v2.Bucket bucket = codecs.bucketInfo().encode(bucketInfo); @@ -515,7 +493,11 @@ public Bucket update(BucketInfo bucketInfo, BucketTargetOption... options) { @Override public Blob update(BlobInfo blobInfo, BlobTargetOption... options) { - Opts opts = Opts.unwrap(options).resolveFrom(blobInfo).prepend(defaultOpts); + Opts unwrap = Opts.unwrap(options); + if (blobInfo.getModifiedFields().isEmpty()) { + return internalBlobGet(blobInfo.getBlobId(), unwrap.constrainTo(ObjectSourceOpt.class)); + } + Opts opts = unwrap.resolveFrom(blobInfo).prepend(defaultOpts); GrpcCallContext grpcCallContext = opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault()); Object object = codecs.blobInfo().encode(blobInfo); @@ -1963,4 +1945,43 @@ private static Hasher getHasherForRequest(WriteObjectRequest req, Hasher default } } } + + @Nullable + private Blob internalBlobGet(BlobId blob, Opts unwrap) { + Opts opts = unwrap.resolveFrom(blob).prepend(defaultOpts); + GrpcCallContext grpcCallContext = + opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault()); + GetObjectRequest.Builder builder = + GetObjectRequest.newBuilder() + .setBucket(bucketNameCodec.encode(blob.getBucket())) + .setObject(blob.getName()); + ifNonNull(blob.getGeneration(), builder::setGeneration); + GetObjectRequest req = opts.getObjectsRequest().apply(builder).build(); + return Retrying.run( + getOptions(), + retryAlgorithmManager.getFor(req), + () -> { + try { + return storageClient.getObjectCallable().call(req, grpcCallContext); + } catch (NotFoundException ignore) { + return null; + } + }, + syntaxDecoders.blob.andThen(opts.clearBlobFields())); + } + + @Nullable + private Bucket internalBucketGet(String bucket, Opts unwrap) { + Opts opts = unwrap.prepend(defaultOpts); + GrpcCallContext grpcCallContext = + opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault()); + GetBucketRequest.Builder builder = + GetBucketRequest.newBuilder().setName(bucketNameCodec.encode(bucket)); + GetBucketRequest req = opts.getBucketsRequest().apply(builder).build(); + return Retrying.run( + getOptions(), + retryAlgorithmManager.getFor(req), + () -> storageClient.getBucketCallable().call(req, grpcCallContext), + syntaxDecoders.bucket.andThen(opts.clearBucketFields())); + } } diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java index cd6816e266..8c0768b9de 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java @@ -270,15 +270,8 @@ private static void uploadHelper(ReadableByteChannel reader, WriteChannel writer @Override public Bucket get(String bucket, BucketGetOption... options) { - final com.google.api.services.storage.model.Bucket bucketPb = - codecs.bucketInfo().encode(BucketInfo.of(bucket)); ImmutableMap optionsMap = Opts.unwrap(options).getRpcOptions(); - ResultRetryAlgorithm algorithm = - retryAlgorithmManager.getForBucketsGet(bucketPb, optionsMap); - return run( - algorithm, - () -> storageRpc.get(bucketPb, optionsMap), - (b) -> Conversions.apiary().bucketInfo().decode(b).asBucket(this)); + return internalBucketGet(bucket, optionsMap); } @Override @@ -288,18 +281,9 @@ public Blob get(String bucket, String blob, BlobGetOption... options) { @Override public Blob get(BlobId blob, BlobGetOption... options) { - final StorageObject storedObject = codecs.blobId().encode(blob); ImmutableMap optionsMap = Opts.unwrap(options).resolveFrom(blob).getRpcOptions(); - ResultRetryAlgorithm algorithm = - retryAlgorithmManager.getForObjectsGet(storedObject, optionsMap); - return run( - algorithm, - () -> storageRpc.get(storedObject, optionsMap), - (x) -> { - BlobInfo info = Conversions.apiary().blobInfo().decode(x); - return info.asBlob(this); - }); + return internalGetBlob(blob, optionsMap); } @Override @@ -437,32 +421,42 @@ private static Page listBlobs( @Override public Bucket update(BucketInfo bucketInfo, BucketTargetOption... options) { - final com.google.api.services.storage.model.Bucket bucketPb = - codecs.bucketInfo().encode(bucketInfo); - final Map optionsMap = + Map optionsMap = Opts.unwrap(options).resolveFrom(bucketInfo).getRpcOptions(); - ResultRetryAlgorithm algorithm = - retryAlgorithmManager.getForBucketsUpdate(bucketPb, optionsMap); - return run( - algorithm, - () -> storageRpc.patch(bucketPb, optionsMap), - (x) -> Conversions.apiary().bucketInfo().decode(x).asBucket(this)); + if (bucketInfo.getModifiedFields().isEmpty()) { + return internalBucketGet(bucketInfo.getName(), optionsMap); + } else { + com.google.api.services.storage.model.Bucket bucketPb = + codecs.bucketInfo().encode(bucketInfo); + ResultRetryAlgorithm algorithm = + retryAlgorithmManager.getForBucketsUpdate(bucketPb, optionsMap); + return run( + algorithm, + () -> storageRpc.patch(bucketPb, optionsMap), + (x) -> Conversions.apiary().bucketInfo().decode(x).asBucket(this)); + } } @Override public Blob update(BlobInfo blobInfo, BlobTargetOption... options) { Opts opts = Opts.unwrap(options).resolveFrom(blobInfo); Map optionsMap = opts.getRpcOptions(); + boolean unmodifiedBeforeOpts = blobInfo.getModifiedFields().isEmpty(); BlobInfo updated = opts.blobInfoMapper().apply(blobInfo.toBuilder()).build(); - StorageObject pb = codecs.blobInfo().encode(updated); - ResultRetryAlgorithm algorithm = retryAlgorithmManager.getForObjectsUpdate(pb, optionsMap); - return run( - algorithm, - () -> storageRpc.patch(pb, optionsMap), - (x) -> { - BlobInfo info = Conversions.apiary().blobInfo().decode(x); - return info.asBlob(this); - }); + boolean unmodifiedAfterOpts = updated.getModifiedFields().isEmpty(); + if (unmodifiedBeforeOpts && unmodifiedAfterOpts) { + return internalGetBlob(blobInfo.getBlobId(), optionsMap); + } else { + StorageObject pb = codecs.blobInfo().encode(updated); + ResultRetryAlgorithm algorithm = retryAlgorithmManager.getForObjectsUpdate(pb, optionsMap); + return run( + algorithm, + () -> storageRpc.patch(pb, optionsMap), + (x) -> { + BlobInfo info = Conversions.apiary().blobInfo().decode(x); + return info.asBlob(this); + }); + } } @Override @@ -1527,4 +1521,28 @@ public boolean deleteNotification(final String bucket, final String notification public HttpStorageOptions getOptions() { return (HttpStorageOptions) super.getOptions(); } + + private Blob internalGetBlob(BlobId blob, Map optionsMap) { + StorageObject storedObject = codecs.blobId().encode(blob); + ResultRetryAlgorithm algorithm = + retryAlgorithmManager.getForObjectsGet(storedObject, optionsMap); + return run( + algorithm, + () -> storageRpc.get(storedObject, optionsMap), + (x) -> { + BlobInfo info = Conversions.apiary().blobInfo().decode(x); + return info.asBlob(this); + }); + } + + private Bucket internalBucketGet(String bucket, Map optionsMap) { + com.google.api.services.storage.model.Bucket bucketPb = + codecs.bucketInfo().encode(BucketInfo.of(bucket)); + ResultRetryAlgorithm algorithm = + retryAlgorithmManager.getForBucketsGet(bucketPb, optionsMap); + return run( + algorithm, + () -> storageRpc.get(bucketPb, optionsMap), + (b) -> Conversions.apiary().bucketInfo().decode(b).asBucket(this)); + } } diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/UnifiedOpts.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/UnifiedOpts.java index 295455f4ef..7aca392106 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/UnifiedOpts.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/UnifiedOpts.java @@ -2328,6 +2328,16 @@ Opts prepend(Opts toPrepend) { return new Opts<>(list); } + /** + * Create a new instance of {@code Opts} consisting of those {@code Opt}s which are also an + * {@code R}. + * + *

i.e. Given {@code Opts} produce {@code Opts} + */ + Opts constrainTo(Class c) { + return new Opts<>(filterTo(c).collect(ImmutableList.toImmutableList())); + } + private Mapper> rpcOptionMapper() { return fuseMappers(RpcOptVal.class, RpcOptVal::mapper); } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java index b2401f326e..67b8159b44 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java @@ -123,6 +123,7 @@ final class RpcMethodMappings { methodGroupIs("storage.resumable.upload"); static final int _2MiB = 2 * 1024 * 1024; + private static final ImmutableMap MODIFY = ImmutableMap.of("a", "b"); final Multimap funcMap; RpcMethodMappings() { @@ -552,7 +553,16 @@ private static void patch(ArrayList a) { .withApplicable(not(TestRetryConformance::isPreconditionsProvided)) .withTest( (ctx, c) -> - ctx.map(state -> state.with(ctx.getStorage().update(state.getBucket())))) + ctx.map( + state -> + state.with( + ctx.getStorage() + .update( + state + .getBucket() + .toBuilder() + .setLabels(MODIFY) + .build())))) .build()); a.add( RpcMethodMapping.newBuilder(122, buckets.patch) @@ -564,7 +574,7 @@ private static void patch(ArrayList a) { state.with( ctx.getStorage() .update( - state.getBucket(), + state.getBucket().toBuilder().setLabels(MODIFY).build(), BucketTargetOption.metagenerationMatch())))) .build()); a.add( @@ -577,12 +587,25 @@ private static void patch(ArrayList a) { state.with( state .getBucket() + .toBuilder() + .setLabels(MODIFY) + .build() .update(BucketTargetOption.metagenerationMatch())))) .build()); a.add( RpcMethodMapping.newBuilder(243, buckets.patch) .withApplicable(not(TestRetryConformance::isPreconditionsProvided)) - .withTest((ctx, c) -> ctx.map(state -> state.with(state.getBucket().update()))) + .withTest( + (ctx, c) -> + ctx.map( + state -> + state.with( + state + .getBucket() + .toBuilder() + .setLabels(MODIFY) + .build() + .update()))) .build()); } @@ -1860,7 +1883,15 @@ private static void patch(ArrayList a) { .withTest( (ctx, c) -> ctx.map( - state -> state.with(ctx.getStorage().update(ctx.getState().getBlob())))) + state -> + state.with( + ctx.getStorage() + .update( + ctx.getState() + .getBlob() + .toBuilder() + .setMetadata(MODIFY) + .build())))) .build()); a.add( RpcMethodMapping.newBuilder(57, objects.patch) @@ -1872,13 +1903,21 @@ private static void patch(ArrayList a) { state.with( ctx.getStorage() .update( - ctx.getState().getBlob(), + ctx.getState() + .getBlob() + .toBuilder() + .setMetadata(MODIFY) + .build(), BlobTargetOption.metagenerationMatch())))) .build()); a.add( RpcMethodMapping.newBuilder(79, objects.patch) .withApplicable(not(TestRetryConformance::isPreconditionsProvided)) - .withTest((ctx, c) -> ctx.peek(state -> state.getBlob().update())) + .withTest( + (ctx, c) -> + ctx.peek( + state -> + state.getBlob().toBuilder().setMetadata(MODIFY).build().update())) .build()); a.add( RpcMethodMapping.newBuilder(80, objects.patch) @@ -1890,6 +1929,9 @@ private static void patch(ArrayList a) { state.with( state .getBlob() + .toBuilder() + .setMetadata(MODIFY) + .build() .update(BlobTargetOption.metagenerationMatch())))) .build()); } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITBucketTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITBucketTest.java index 616aee62a3..240f5d6ca1 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITBucketTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITBucketTest.java @@ -39,6 +39,7 @@ import com.google.cloud.storage.Storage; import com.google.cloud.storage.Storage.BlobField; import com.google.cloud.storage.Storage.BucketField; +import com.google.cloud.storage.Storage.BucketGetOption; import com.google.cloud.storage.Storage.BucketListOption; import com.google.cloud.storage.Storage.BucketTargetOption; import com.google.cloud.storage.TransportCompatibility.Transport; @@ -416,6 +417,24 @@ public void testCreateBucketWithAutoclass() { } } + @Test + public void testUpdateBucket_noModification() throws Exception { + String bucketName = generator.randomBucketName(); + BucketInfo bucketInfo = BucketInfo.newBuilder(bucketName).build(); + try (TemporaryBucket tempB = + TemporaryBucket.newBuilder().setBucketInfo(bucketInfo).setStorage(storage).build()) { + // in grpc, create will return acls but update does not. re-get the metadata with default + // fields + BucketInfo bucket = tempB.getBucket(); + Bucket gen1 = + storage.get( + bucket.getName(), BucketGetOption.metagenerationMatch(bucket.getMetageneration())); + + Bucket gen2 = storage.update(gen1); + assertThat(gen2).isEqualTo(gen1); + } + } + private void unsetRequesterPays() { Bucket remoteBucket = storage.get( diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITObjectTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITObjectTest.java index 1da2d7686f..e6f0607d7e 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITObjectTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITObjectTest.java @@ -1497,4 +1497,15 @@ public void testBlobTimeStorageClassUpdated() { .isEqualTo(updatedBlob1.getTimeStorageClassUpdated()); assertThat(updatedBlob2.delete()).isTrue(); } + + @Test + public void testUpdateBlob_noModification() { + BlobInfo info = BlobInfo.newBuilder(bucket, generator.randomObjectName()).build(); + + // in grpc, create will return acls but update does not. re-get the metadata with default fields + Blob gen1 = storage.create(info); + gen1 = storage.get(gen1.getBlobId()); + Blob gen2 = storage.update(gen1); + assertThat(gen2).isEqualTo(gen1); + } } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITOptionRegressionTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITOptionRegressionTest.java index 0ca27c52a1..dc99678c6d 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITOptionRegressionTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITOptionRegressionTest.java @@ -159,7 +159,8 @@ public void storage_BucketTargetOption_userProject_String() { public void storage_BucketTargetOption_projection_String() { Bucket bucket = s.create(BucketInfo.of(bucketName())); requestAuditing.clear(); - s.update(bucket, BucketTargetOption.projection("noAcl")); + Bucket update = bucket.toBuilder().setLabels(ImmutableMap.of("a", "b")).build(); + s.update(update, BucketTargetOption.projection("noAcl")); requestAuditing.assertQueryParam("projection", "noAcl"); }