Skip to content

Commit 0adeb14

Browse files
BenWhiteheadBenWhitehead
authored
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
1 parent 6652ad8 commit 0adeb14

File tree

7 files changed

+198
-76
lines changed

7 files changed

+198
-76
lines changed

‎google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java‎

Lines changed: 54 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@
146146
import java.util.stream.Collectors;
147147
import java.util.stream.Stream;
148148
import java.util.stream.StreamSupport;
149+
import org.checkerframework.checker.nullness.qual.Nullable;
149150

150151
@BetaApi
151152
final class GrpcStorageImpl extends BaseService<StorageOptions> implements Storage {
@@ -381,17 +382,8 @@ public Blob createFrom(
381382

382383
@Override
383384
public Bucket get(String bucket, BucketGetOption... options) {
384-
Opts<BucketSourceOpt> opts = Opts.unwrap(options).prepend(defaultOpts);
385-
GrpcCallContext grpcCallContext =
386-
opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault());
387-
GetBucketRequest.Builder builder =
388-
GetBucketRequest.newBuilder().setName(bucketNameCodec.encode(bucket));
389-
GetBucketRequest req = opts.getBucketsRequest().apply(builder).build();
390-
return Retrying.run(
391-
getOptions(),
392-
retryAlgorithmManager.getFor(req),
393-
() -> storageClient.getBucketCallable().call(req, grpcCallContext),
394-
syntaxDecoders.bucket.andThen(opts.clearBucketFields()));
385+
Opts<BucketSourceOpt> unwrap = Opts.unwrap(options);
386+
return internalBucketGet(bucket, unwrap);
395387
}
396388

397389
@Override
@@ -418,26 +410,8 @@ public Blob get(String bucket, String blob, BlobGetOption... options) {
418410

419411
@Override
420412
public Blob get(BlobId blob, BlobGetOption... options) {
421-
Opts<ObjectSourceOpt> opts = Opts.unwrap(options).resolveFrom(blob).prepend(defaultOpts);
422-
GrpcCallContext grpcCallContext =
423-
opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault());
424-
GetObjectRequest.Builder builder =
425-
GetObjectRequest.newBuilder()
426-
.setBucket(bucketNameCodec.encode(blob.getBucket()))
427-
.setObject(blob.getName());
428-
ifNonNull(blob.getGeneration(), builder::setGeneration);
429-
GetObjectRequest req = opts.getObjectsRequest().apply(builder).build();
430-
return Retrying.run(
431-
getOptions(),
432-
retryAlgorithmManager.getFor(req),
433-
() -> {
434-
try {
435-
return storageClient.getObjectCallable().call(req, grpcCallContext);
436-
} catch (NotFoundException ignore) {
437-
return null;
438-
}
439-
},
440-
syntaxDecoders.blob.andThen(opts.clearBlobFields()));
413+
Opts<ObjectSourceOpt> unwrap = Opts.unwrap(options);
414+
return internalBlobGet(blob, unwrap);
441415
}
442416

443417
@Override
@@ -493,7 +467,11 @@ public Page<Blob> list(String bucket, BlobListOption... options) {
493467

494468
@Override
495469
public Bucket update(BucketInfo bucketInfo, BucketTargetOption... options) {
496-
Opts<BucketTargetOpt> opts = Opts.unwrap(options).resolveFrom(bucketInfo).prepend(defaultOpts);
470+
Opts<BucketTargetOpt> unwrap = Opts.unwrap(options);
471+
if (bucketInfo.getModifiedFields().isEmpty()) {
472+
return internalBucketGet(bucketInfo.getName(), unwrap.constrainTo(BucketSourceOpt.class));
473+
}
474+
Opts<BucketTargetOpt> opts = unwrap.resolveFrom(bucketInfo).prepend(defaultOpts);
497475
GrpcCallContext grpcCallContext =
498476
opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault());
499477
com.google.storage.v2.Bucket bucket = codecs.bucketInfo().encode(bucketInfo);
@@ -515,7 +493,11 @@ public Bucket update(BucketInfo bucketInfo, BucketTargetOption... options) {
515493

516494
@Override
517495
public Blob update(BlobInfo blobInfo, BlobTargetOption... options) {
518-
Opts<ObjectTargetOpt> opts = Opts.unwrap(options).resolveFrom(blobInfo).prepend(defaultOpts);
496+
Opts<ObjectTargetOpt> unwrap = Opts.unwrap(options);
497+
if (blobInfo.getModifiedFields().isEmpty()) {
498+
return internalBlobGet(blobInfo.getBlobId(), unwrap.constrainTo(ObjectSourceOpt.class));
499+
}
500+
Opts<ObjectTargetOpt> opts = unwrap.resolveFrom(blobInfo).prepend(defaultOpts);
519501
GrpcCallContext grpcCallContext =
520502
opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault());
521503
Object object = codecs.blobInfo().encode(blobInfo);
@@ -1963,4 +1945,43 @@ private static Hasher getHasherForRequest(WriteObjectRequest req, Hasher default
19631945
}
19641946
}
19651947
}
1948+
1949+
@Nullable
1950+
private Blob internalBlobGet(BlobId blob, Opts<ObjectSourceOpt> unwrap) {
1951+
Opts<ObjectSourceOpt> opts = unwrap.resolveFrom(blob).prepend(defaultOpts);
1952+
GrpcCallContext grpcCallContext =
1953+
opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault());
1954+
GetObjectRequest.Builder builder =
1955+
GetObjectRequest.newBuilder()
1956+
.setBucket(bucketNameCodec.encode(blob.getBucket()))
1957+
.setObject(blob.getName());
1958+
ifNonNull(blob.getGeneration(), builder::setGeneration);
1959+
GetObjectRequest req = opts.getObjectsRequest().apply(builder).build();
1960+
return Retrying.run(
1961+
getOptions(),
1962+
retryAlgorithmManager.getFor(req),
1963+
() -> {
1964+
try {
1965+
return storageClient.getObjectCallable().call(req, grpcCallContext);
1966+
} catch (NotFoundException ignore) {
1967+
return null;
1968+
}
1969+
},
1970+
syntaxDecoders.blob.andThen(opts.clearBlobFields()));
1971+
}
1972+
1973+
@Nullable
1974+
private Bucket internalBucketGet(String bucket, Opts<BucketSourceOpt> unwrap) {
1975+
Opts<BucketSourceOpt> opts = unwrap.prepend(defaultOpts);
1976+
GrpcCallContext grpcCallContext =
1977+
opts.grpcMetadataMapper().apply(GrpcCallContext.createDefault());
1978+
GetBucketRequest.Builder builder =
1979+
GetBucketRequest.newBuilder().setName(bucketNameCodec.encode(bucket));
1980+
GetBucketRequest req = opts.getBucketsRequest().apply(builder).build();
1981+
return Retrying.run(
1982+
getOptions(),
1983+
retryAlgorithmManager.getFor(req),
1984+
() -> storageClient.getBucketCallable().call(req, grpcCallContext),
1985+
syntaxDecoders.bucket.andThen(opts.clearBucketFields()));
1986+
}
19661987
}

‎google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java‎

Lines changed: 54 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -270,15 +270,8 @@ private static void uploadHelper(ReadableByteChannel reader, WriteChannel writer
270270

271271
@Override
272272
public Bucket get(String bucket, BucketGetOption... options) {
273-
final com.google.api.services.storage.model.Bucket bucketPb =
274-
codecs.bucketInfo().encode(BucketInfo.of(bucket));
275273
ImmutableMap<StorageRpc.Option, ?> optionsMap = Opts.unwrap(options).getRpcOptions();
276-
ResultRetryAlgorithm<?> algorithm =
277-
retryAlgorithmManager.getForBucketsGet(bucketPb, optionsMap);
278-
return run(
279-
algorithm,
280-
() -> storageRpc.get(bucketPb, optionsMap),
281-
(b) -> Conversions.apiary().bucketInfo().decode(b).asBucket(this));
274+
return internalBucketGet(bucket, optionsMap);
282275
}
283276

284277
@Override
@@ -288,18 +281,9 @@ public Blob get(String bucket, String blob, BlobGetOption... options) {
288281

289282
@Override
290283
public Blob get(BlobId blob, BlobGetOption... options) {
291-
final StorageObject storedObject = codecs.blobId().encode(blob);
292284
ImmutableMap<StorageRpc.Option, ?> optionsMap =
293285
Opts.unwrap(options).resolveFrom(blob).getRpcOptions();
294-
ResultRetryAlgorithm<?> algorithm =
295-
retryAlgorithmManager.getForObjectsGet(storedObject, optionsMap);
296-
return run(
297-
algorithm,
298-
() -> storageRpc.get(storedObject, optionsMap),
299-
(x) -> {
300-
BlobInfo info = Conversions.apiary().blobInfo().decode(x);
301-
return info.asBlob(this);
302-
});
286+
return internalGetBlob(blob, optionsMap);
303287
}
304288

305289
@Override
@@ -437,32 +421,42 @@ private static Page<Blob> listBlobs(
437421

438422
@Override
439423
public Bucket update(BucketInfo bucketInfo, BucketTargetOption... options) {
440-
final com.google.api.services.storage.model.Bucket bucketPb =
441-
codecs.bucketInfo().encode(bucketInfo);
442-
final Map<StorageRpc.Option, ?> optionsMap =
424+
Map<StorageRpc.Option, ?> optionsMap =
443425
Opts.unwrap(options).resolveFrom(bucketInfo).getRpcOptions();
444-
ResultRetryAlgorithm<?> algorithm =
445-
retryAlgorithmManager.getForBucketsUpdate(bucketPb, optionsMap);
446-
return run(
447-
algorithm,
448-
() -> storageRpc.patch(bucketPb, optionsMap),
449-
(x) -> Conversions.apiary().bucketInfo().decode(x).asBucket(this));
426+
if (bucketInfo.getModifiedFields().isEmpty()) {
427+
return internalBucketGet(bucketInfo.getName(), optionsMap);
428+
} else {
429+
com.google.api.services.storage.model.Bucket bucketPb =
430+
codecs.bucketInfo().encode(bucketInfo);
431+
ResultRetryAlgorithm<?> algorithm =
432+
retryAlgorithmManager.getForBucketsUpdate(bucketPb, optionsMap);
433+
return run(
434+
algorithm,
435+
() -> storageRpc.patch(bucketPb, optionsMap),
436+
(x) -> Conversions.apiary().bucketInfo().decode(x).asBucket(this));
437+
}
450438
}
451439

452440
@Override
453441
public Blob update(BlobInfo blobInfo, BlobTargetOption... options) {
454442
Opts<ObjectTargetOpt> opts = Opts.unwrap(options).resolveFrom(blobInfo);
455443
Map<StorageRpc.Option, ?> optionsMap = opts.getRpcOptions();
444+
boolean unmodifiedBeforeOpts = blobInfo.getModifiedFields().isEmpty();
456445
BlobInfo updated = opts.blobInfoMapper().apply(blobInfo.toBuilder()).build();
457-
StorageObject pb = codecs.blobInfo().encode(updated);
458-
ResultRetryAlgorithm<?> algorithm = retryAlgorithmManager.getForObjectsUpdate(pb, optionsMap);
459-
return run(
460-
algorithm,
461-
() -> storageRpc.patch(pb, optionsMap),
462-
(x) -> {
463-
BlobInfo info = Conversions.apiary().blobInfo().decode(x);
464-
return info.asBlob(this);
465-
});
446+
boolean unmodifiedAfterOpts = updated.getModifiedFields().isEmpty();
447+
if (unmodifiedBeforeOpts && unmodifiedAfterOpts) {
448+
return internalGetBlob(blobInfo.getBlobId(), optionsMap);
449+
} else {
450+
StorageObject pb = codecs.blobInfo().encode(updated);
451+
ResultRetryAlgorithm<?> algorithm = retryAlgorithmManager.getForObjectsUpdate(pb, optionsMap);
452+
return run(
453+
algorithm,
454+
() -> storageRpc.patch(pb, optionsMap),
455+
(x) -> {
456+
BlobInfo info = Conversions.apiary().blobInfo().decode(x);
457+
return info.asBlob(this);
458+
});
459+
}
466460
}
467461

468462
@Override
@@ -1527,4 +1521,28 @@ public boolean deleteNotification(final String bucket, final String notification
15271521
public HttpStorageOptions getOptions() {
15281522
return (HttpStorageOptions) super.getOptions();
15291523
}
1524+
1525+
private Blob internalGetBlob(BlobId blob, Map<StorageRpc.Option, ?> optionsMap) {
1526+
StorageObject storedObject = codecs.blobId().encode(blob);
1527+
ResultRetryAlgorithm<?> algorithm =
1528+
retryAlgorithmManager.getForObjectsGet(storedObject, optionsMap);
1529+
return run(
1530+
algorithm,
1531+
() -> storageRpc.get(storedObject, optionsMap),
1532+
(x) -> {
1533+
BlobInfo info = Conversions.apiary().blobInfo().decode(x);
1534+
return info.asBlob(this);
1535+
});
1536+
}
1537+
1538+
private Bucket internalBucketGet(String bucket, Map<StorageRpc.Option, ?> optionsMap) {
1539+
com.google.api.services.storage.model.Bucket bucketPb =
1540+
codecs.bucketInfo().encode(BucketInfo.of(bucket));
1541+
ResultRetryAlgorithm<?> algorithm =
1542+
retryAlgorithmManager.getForBucketsGet(bucketPb, optionsMap);
1543+
return run(
1544+
algorithm,
1545+
() -> storageRpc.get(bucketPb, optionsMap),
1546+
(b) -> Conversions.apiary().bucketInfo().decode(b).asBucket(this));
1547+
}
15301548
}

‎google-cloud-storage/src/main/java/com/google/cloud/storage/UnifiedOpts.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2328,6 +2328,16 @@ Opts<T> prepend(Opts<? extends T> toPrepend) {
23282328
return new Opts<>(list);
23292329
}
23302330

2331+
/**
2332+
* Create a new instance of {@code Opts<R>} consisting of those {@code Opt}s which are also an
2333+
* {@code R}.
2334+
*
2335+
* <p>i.e. Given {@code Opts<ObjectTargetOpt>} produce {@code Opts<ObjectSourceOpt>}
2336+
*/
2337+
<R extends Opt> Opts<R> constrainTo(Class<R> c) {
2338+
return new Opts<>(filterTo(c).collect(ImmutableList.toImmutableList()));
2339+
}
2340+
23312341
private Mapper<ImmutableMap.Builder<StorageRpc.Option, Object>> rpcOptionMapper() {
23322342
return fuseMappers(RpcOptVal.class, RpcOptVal::mapper);
23332343
}

‎google-cloud-storage/src/test/java/com/google/cloud/storage/conformance/retry/RpcMethodMappings.java‎

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ final class RpcMethodMappings {
123123
methodGroupIs("storage.resumable.upload");
124124

125125
static final int _2MiB = 2 * 1024 * 1024;
126+
private static final ImmutableMap<String, String> MODIFY = ImmutableMap.of("a", "b");
126127
final Multimap<RpcMethod, RpcMethodMapping> funcMap;
127128

128129
RpcMethodMappings() {
@@ -552,7 +553,16 @@ private static void patch(ArrayList<RpcMethodMapping> a) {
552553
.withApplicable(not(TestRetryConformance::isPreconditionsProvided))
553554
.withTest(
554555
(ctx, c) ->
555-
ctx.map(state -> state.with(ctx.getStorage().update(state.getBucket()))))
556+
ctx.map(
557+
state ->
558+
state.with(
559+
ctx.getStorage()
560+
.update(
561+
state
562+
.getBucket()
563+
.toBuilder()
564+
.setLabels(MODIFY)
565+
.build()))))
556566
.build());
557567
a.add(
558568
RpcMethodMapping.newBuilder(122, buckets.patch)
@@ -564,7 +574,7 @@ private static void patch(ArrayList<RpcMethodMapping> a) {
564574
state.with(
565575
ctx.getStorage()
566576
.update(
567-
state.getBucket(),
577+
state.getBucket().toBuilder().setLabels(MODIFY).build(),
568578
BucketTargetOption.metagenerationMatch()))))
569579
.build());
570580
a.add(
@@ -577,12 +587,25 @@ private static void patch(ArrayList<RpcMethodMapping> a) {
577587
state.with(
578588
state
579589
.getBucket()
590+
.toBuilder()
591+
.setLabels(MODIFY)
592+
.build()
580593
.update(BucketTargetOption.metagenerationMatch()))))
581594
.build());
582595
a.add(
583596
RpcMethodMapping.newBuilder(243, buckets.patch)
584597
.withApplicable(not(TestRetryConformance::isPreconditionsProvided))
585-
.withTest((ctx, c) -> ctx.map(state -> state.with(state.getBucket().update())))
598+
.withTest(
599+
(ctx, c) ->
600+
ctx.map(
601+
state ->
602+
state.with(
603+
state
604+
.getBucket()
605+
.toBuilder()
606+
.setLabels(MODIFY)
607+
.build()
608+
.update())))
586609
.build());
587610
}
588611

@@ -1860,7 +1883,15 @@ private static void patch(ArrayList<RpcMethodMapping> a) {
18601883
.withTest(
18611884
(ctx, c) ->
18621885
ctx.map(
1863-
state -> state.with(ctx.getStorage().update(ctx.getState().getBlob()))))
1886+
state ->
1887+
state.with(
1888+
ctx.getStorage()
1889+
.update(
1890+
ctx.getState()
1891+
.getBlob()
1892+
.toBuilder()
1893+
.setMetadata(MODIFY)
1894+
.build()))))
18641895
.build());
18651896
a.add(
18661897
RpcMethodMapping.newBuilder(57, objects.patch)
@@ -1872,13 +1903,21 @@ private static void patch(ArrayList<RpcMethodMapping> a) {
18721903
state.with(
18731904
ctx.getStorage()
18741905
.update(
1875-
ctx.getState().getBlob(),
1906+
ctx.getState()
1907+
.getBlob()
1908+
.toBuilder()
1909+
.setMetadata(MODIFY)
1910+
.build(),
18761911
BlobTargetOption.metagenerationMatch()))))
18771912
.build());
18781913
a.add(
18791914
RpcMethodMapping.newBuilder(79, objects.patch)
18801915
.withApplicable(not(TestRetryConformance::isPreconditionsProvided))
1881-
.withTest((ctx, c) -> ctx.peek(state -> state.getBlob().update()))
1916+
.withTest(
1917+
(ctx, c) ->
1918+
ctx.peek(
1919+
state ->
1920+
state.getBlob().toBuilder().setMetadata(MODIFY).build().update()))
18821921
.build());
18831922
a.add(
18841923
RpcMethodMapping.newBuilder(80, objects.patch)
@@ -1890,6 +1929,9 @@ private static void patch(ArrayList<RpcMethodMapping> a) {
18901929
state.with(
18911930
state
18921931
.getBlob()
1932+
.toBuilder()
1933+
.setMetadata(MODIFY)
1934+
.build()
18931935
.update(BlobTargetOption.metagenerationMatch()))))
18941936
.build());
18951937
}

0 commit comments

Comments
 (0)