Skip to content

Commit c8bf3c7

Browse files
BenWhiteheadBenWhitehead
authored
fix: update GrpcStorageImpl#update to support fine-grained update of BucketInfo.labels and BlobInfo.metadata (#1843)
Successor to #1830, whereas this preserves existing behavior rather than introducing a new method. A new set of tests have been added to validate all permutations of modifying metadata/labels.
1 parent 3345ac9 commit c8bf3c7

File tree

13 files changed

+447
-122
lines changed

13 files changed

+447
-122
lines changed

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

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@
7878
import java.util.Collections;
7979
import java.util.List;
8080
import java.util.Map;
81+
import java.util.Map.Entry;
8182
import java.util.function.Function;
8283

8384
@InternalApi
@@ -373,7 +374,14 @@ private Bucket bucketInfoEncode(BucketInfo from) {
373374
from.getDefaultKmsKeyName(),
374375
k -> new Encryption().setDefaultKmsKeyName(k),
375376
to::setEncryption);
376-
ifNonNull(from.getLabels(), to::setLabels);
377+
Map<String, String> pbLabels = from.getLabels();
378+
if (pbLabels != null && !Data.isNull(pbLabels)) {
379+
pbLabels = Maps.newHashMapWithExpectedSize(from.getLabels().size());
380+
for (Map.Entry<String, String> entry : from.getLabels().entrySet()) {
381+
pbLabels.put(entry.getKey(), firstNonNull(entry.getValue(), Data.nullOf(String.class)));
382+
}
383+
}
384+
to.setLabels(pbLabels);
377385
maybeEncodeRetentionPolicy(from, to);
378386
ifNonNull(from.getIamConfiguration(), this::iamConfigEncode, to::setIamConfiguration);
379387
ifNonNull(from.getAutoclass(), this::autoclassEncode, to::setAutoclass);
@@ -412,7 +420,7 @@ private BucketInfo bucketInfoDecode(com.google.api.services.storage.model.Bucket
412420
lift(Lifecycle::getRule).andThen(toImmutableListOf(lifecycleRule()::decode)),
413421
to::setLifecycleRules);
414422
ifNonNull(from.getDefaultEventBasedHold(), to::setDefaultEventBasedHold);
415-
ifNonNull(from.getLabels(), to::setLabels);
423+
ifNonNull(from.getLabels(), ApiaryConversions::replaceDataNullValuesWithNull, to::setLabels);
416424
ifNonNull(from.getBilling(), Billing::getRequesterPays, to::setRequesterPays);
417425
Encryption encryption = from.getEncryption();
418426
if (encryption != null
@@ -917,4 +925,21 @@ private static void maybeDecodeRetentionPolicy(Bucket from, BucketInfo.Builder t
917925
to::setRetentionPeriodDuration);
918926
}
919927
}
928+
929+
private static Map<String, String> replaceDataNullValuesWithNull(Map<String, String> labels) {
930+
boolean anyDataNull = labels.values().stream().anyMatch(Data::isNull);
931+
if (anyDataNull) {
932+
// If we received any Data null values, replace them with null before setting.
933+
// Explicitly use a HashMap as it allows null values.
934+
Map<String, String> tmp = Maps.newHashMapWithExpectedSize(labels.size());
935+
for (Entry<String, String> e : labels.entrySet()) {
936+
String k = e.getKey();
937+
String v = e.getValue();
938+
tmp.put(k, Data.isNull(v) ? null : v);
939+
}
940+
return Collections.unmodifiableMap(tmp);
941+
} else {
942+
return labels;
943+
}
944+
}
920945
}

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

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,15 @@
1717
package com.google.cloud.storage;
1818

1919
import static com.google.cloud.storage.BackwardCompatibilityUtils.millisOffsetDateTimeCodec;
20+
import static com.google.cloud.storage.Utils.diffMaps;
2021
import static com.google.common.base.MoreObjects.firstNonNull;
2122
import static com.google.common.base.Preconditions.checkNotNull;
2223

2324
import com.google.api.client.util.Data;
2425
import com.google.api.core.BetaApi;
2526
import com.google.cloud.storage.Storage.BlobField;
2627
import com.google.cloud.storage.TransportCompatibility.Transport;
28+
import com.google.cloud.storage.UnifiedOpts.NamedField;
2729
import com.google.common.base.MoreObjects;
2830
import com.google.common.collect.ImmutableList;
2931
import com.google.common.collect.ImmutableSet;
@@ -38,6 +40,8 @@
3840
import java.util.Map;
3941
import java.util.Objects;
4042
import java.util.Set;
43+
import org.checkerframework.checker.nullness.qual.NonNull;
44+
import org.checkerframework.checker.nullness.qual.Nullable;
4145

4246
/**
4347
* Information about an object in Google Cloud Storage. A {@code BlobInfo} object includes the
@@ -100,7 +104,7 @@ public class BlobInfo implements Serializable {
100104
private final Boolean eventBasedHold;
101105
private final Boolean temporaryHold;
102106
private final OffsetDateTime retentionExpirationTime;
103-
private final transient ImmutableSet<Storage.BlobField> modifiedFields;
107+
private final transient ImmutableSet<NamedField> modifiedFields;
104108

105109
/** This class is meant for internal use only. Users are discouraged from using this class. */
106110
public static final class ImmutableEmptyMap<K, V> extends AbstractMap<K, V> {
@@ -331,7 +335,7 @@ public Builder setTimeStorageClassUpdatedOffsetDateTime(
331335
}
332336

333337
/** Sets the blob's user provided metadata. */
334-
public abstract Builder setMetadata(Map<String, String> metadata);
338+
public abstract Builder setMetadata(@Nullable Map<@NonNull String, @Nullable String> metadata);
335339

336340
abstract Builder setMetageneration(Long metageneration);
337341

@@ -502,7 +506,7 @@ static final class BuilderImpl extends Builder {
502506
private Boolean eventBasedHold;
503507
private Boolean temporaryHold;
504508
private OffsetDateTime retentionExpirationTime;
505-
private final ImmutableSet.Builder<Storage.BlobField> modifiedFields = ImmutableSet.builder();
509+
private final ImmutableSet.Builder<NamedField> modifiedFields = ImmutableSet.builder();
506510

507511
BuilderImpl(BlobId blobId) {
508512
this.blobId = blobId;
@@ -757,15 +761,18 @@ Builder setMediaLink(String mediaLink) {
757761
return this;
758762
}
759763

764+
@SuppressWarnings({"UnnecessaryLocalVariable"})
760765
@Override
761-
public Builder setMetadata(Map<String, String> metadata) {
762-
if (!Objects.equals(this.metadata, metadata)) {
763-
modifiedFields.add(BlobField.METADATA);
764-
}
765-
if (metadata != null) {
766-
this.metadata = new HashMap<>(metadata);
767-
} else {
768-
this.metadata = (Map<String, String>) Data.nullOf(ImmutableEmptyMap.class);
766+
public Builder setMetadata(@Nullable Map<@NonNull String, @Nullable String> metadata) {
767+
Map<String, String> left = this.metadata;
768+
Map<String, String> right = metadata;
769+
if (!Objects.equals(left, right)) {
770+
diffMaps(BlobField.METADATA, left, right, modifiedFields::add);
771+
if (right != null) {
772+
this.metadata = new HashMap<>(right);
773+
} else {
774+
this.metadata = (Map<String, String>) Data.nullOf(ImmutableEmptyMap.class);
775+
}
769776
}
770777
return this;
771778
}
@@ -1317,7 +1324,8 @@ public String getMediaLink() {
13171324
}
13181325

13191326
/** Returns blob's user provided metadata. */
1320-
public Map<String, String> getMetadata() {
1327+
@Nullable
1328+
public Map<@NonNull String, @Nullable String> getMetadata() {
13211329
return metadata == null || Data.isNull(metadata) ? null : Collections.unmodifiableMap(metadata);
13221330
}
13231331

@@ -1617,7 +1625,7 @@ public boolean equals(Object o) {
16171625
&& Objects.equals(retentionExpirationTime, blobInfo.retentionExpirationTime);
16181626
}
16191627

1620-
ImmutableSet<BlobField> getModifiedFields() {
1628+
ImmutableSet<NamedField> getModifiedFields() {
16211629
return modifiedFields;
16221630
}
16231631

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
import java.util.Map;
4646
import java.util.Objects;
4747
import org.checkerframework.checker.nullness.qual.NonNull;
48+
import org.checkerframework.checker.nullness.qual.Nullable;
4849

4950
/**
5051
* A Google cloud storage bucket.
@@ -546,7 +547,7 @@ public Builder setDefaultAcl(Iterable<Acl> acl) {
546547
}
547548

548549
@Override
549-
public Builder setLabels(Map<String, String> labels) {
550+
public Builder setLabels(@Nullable Map<@NonNull String, @Nullable String> labels) {
550551
infoBuilder.setLabels(labels);
551552
return this;
552553
}

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

Lines changed: 21 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import static com.google.cloud.storage.BackwardCompatibilityUtils.millisOffsetDateTimeCodec;
2020
import static com.google.cloud.storage.BackwardCompatibilityUtils.millisUtcCodec;
2121
import static com.google.cloud.storage.BackwardCompatibilityUtils.nullableDurationSecondsCodec;
22+
import static com.google.cloud.storage.Utils.diffMaps;
2223
import static com.google.common.base.MoreObjects.firstNonNull;
2324
import static com.google.common.base.Preconditions.checkNotNull;
2425
import static com.google.common.collect.Lists.newArrayList;
@@ -29,12 +30,13 @@
2930
import com.google.api.core.BetaApi;
3031
import com.google.api.services.storage.model.Bucket.Lifecycle.Rule;
3132
import com.google.cloud.storage.Acl.Entity;
33+
import com.google.cloud.storage.BlobInfo.ImmutableEmptyMap;
3234
import com.google.cloud.storage.Storage.BucketField;
3335
import com.google.cloud.storage.TransportCompatibility.Transport;
36+
import com.google.cloud.storage.UnifiedOpts.NamedField;
3437
import com.google.common.base.MoreObjects;
3538
import com.google.common.collect.ImmutableList;
3639
import com.google.common.collect.ImmutableSet;
37-
import com.google.common.collect.Maps;
3840
import com.google.common.collect.Streams;
3941
import java.io.IOException;
4042
import java.io.ObjectInputStream;
@@ -43,6 +45,7 @@
4345
import java.time.Duration;
4446
import java.time.OffsetDateTime;
4547
import java.util.ArrayList;
48+
import java.util.HashMap;
4649
import java.util.List;
4750
import java.util.Map;
4851
import java.util.Objects;
@@ -100,7 +103,7 @@ public class BucketInfo implements Serializable {
100103
private final String location;
101104
private final Rpo rpo;
102105
private final StorageClass storageClass;
103-
private final Map<String, String> labels;
106+
@Nullable private final Map<String, String> labels;
104107
private final String defaultKmsKeyName;
105108
private final Boolean defaultEventBasedHold;
106109
private final OffsetDateTime retentionEffectiveTime;
@@ -111,7 +114,7 @@ public class BucketInfo implements Serializable {
111114
private final String locationType;
112115
private final Logging logging;
113116
private final CustomPlacementConfig customPlacementConfig;
114-
private final transient ImmutableSet<Storage.BucketField> modifiedFields;
117+
private final transient ImmutableSet<NamedField> modifiedFields;
115118

116119
/**
117120
* non-private for backward compatibility on message class. log messages are now emitted from
@@ -1477,7 +1480,7 @@ Builder setUpdateTimeOffsetDateTime(OffsetDateTime updateTime) {
14771480
public abstract Builder setDefaultAcl(Iterable<Acl> acl);
14781481

14791482
/** Sets the label of this bucket. */
1480-
public abstract Builder setLabels(Map<String, String> labels);
1483+
public abstract Builder setLabels(@Nullable Map<@NonNull String, @Nullable String> labels);
14811484

14821485
/** Sets the default Cloud KMS key name for this bucket. */
14831486
public abstract Builder setDefaultKmsKeyName(String defaultKmsKeyName);
@@ -1630,7 +1633,7 @@ static final class BuilderImpl extends Builder {
16301633
private String locationType;
16311634
private Logging logging;
16321635
private CustomPlacementConfig customPlacementConfig;
1633-
private final ImmutableSet.Builder<Storage.BucketField> modifiedFields = ImmutableSet.builder();
1636+
private final ImmutableSet.Builder<NamedField> modifiedFields = ImmutableSet.builder();
16341637

16351638
BuilderImpl(String name) {
16361639
this.name = name;
@@ -1909,20 +1912,18 @@ public Builder setDefaultAcl(Iterable<Acl> acl) {
19091912
return this;
19101913
}
19111914

1915+
@SuppressWarnings("UnnecessaryLocalVariable")
19121916
@Override
1913-
public Builder setLabels(Map<String, String> labels) {
1914-
if (labels != null) {
1915-
Map<String, String> tmp =
1916-
Maps.transformValues(
1917-
labels,
1918-
input -> {
1919-
// replace null values with empty strings
1920-
return input == null ? Data.nullOf(String.class) : input;
1921-
});
1922-
if (!Objects.equals(this.labels, tmp)) {
1923-
modifiedFields.add(BucketField.LABELS);
1917+
public Builder setLabels(@Nullable Map<@NonNull String, @Nullable String> labels) {
1918+
Map<String, String> left = this.labels;
1919+
Map<String, String> right = labels;
1920+
if (!Objects.equals(left, right)) {
1921+
diffMaps(BucketField.LABELS, left, right, modifiedFields::add);
1922+
if (right != null) {
1923+
this.labels = new HashMap<>(right);
1924+
} else {
1925+
this.labels = (Map<String, String>) Data.nullOf(ImmutableEmptyMap.class);
19241926
}
1925-
this.labels = tmp;
19261927
}
19271928
return this;
19281929
}
@@ -2483,7 +2484,8 @@ public List<Acl> getDefaultAcl() {
24832484
}
24842485

24852486
/** Returns the labels for this bucket. */
2486-
public Map<String, String> getLabels() {
2487+
@Nullable
2488+
public Map<@NonNull String, @Nullable String> getLabels() {
24872489
return labels;
24882490
}
24892491

@@ -2692,7 +2694,7 @@ Bucket asBucket(Storage storage) {
26922694
return new Bucket(storage, new BucketInfo.BuilderImpl(this));
26932695
}
26942696

2695-
ImmutableSet<BucketField> getModifiedFields() {
2697+
ImmutableSet<NamedField> getModifiedFields() {
26962698
return modifiedFields;
26972699
}
26982700

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ private Bucket bucketInfoEncode(BucketInfo from) {
333333
to.setVersioning(versioningBuilder.build());
334334
}
335335
ifNonNull(from.getDefaultEventBasedHold(), to::setDefaultEventBasedHold);
336-
ifNonNull(from.getLabels(), to::putAllLabels);
336+
ifNonNull(from.getLabels(), this::removeNullValues, to::putAllLabels);
337337
// Do not use, #getLifecycleRules, it can not return null, which is important to our logic here
338338
List<? extends LifecycleRule> lifecycleRules = from.lifecycleRules;
339339
if (lifecycleRules != null) {

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@
6161
import com.google.cloud.storage.UnifiedOpts.HmacKeySourceOpt;
6262
import com.google.cloud.storage.UnifiedOpts.HmacKeyTargetOpt;
6363
import com.google.cloud.storage.UnifiedOpts.Mapper;
64+
import com.google.cloud.storage.UnifiedOpts.NamedField;
6465
import com.google.cloud.storage.UnifiedOpts.ObjectListOpt;
6566
import com.google.cloud.storage.UnifiedOpts.ObjectSourceOpt;
6667
import com.google.cloud.storage.UnifiedOpts.ObjectTargetOpt;
@@ -490,7 +491,7 @@ public Bucket update(BucketInfo bucketInfo, BucketTargetOption... options) {
490491
.getUpdateMaskBuilder()
491492
.addAllPaths(
492493
bucketInfo.getModifiedFields().stream()
493-
.map(BucketField::getGrpcName)
494+
.map(NamedField::getGrpcName)
494495
.collect(ImmutableList.toImmutableList()));
495496
UpdateBucketRequest req = builder.build();
496497
return Retrying.run(
@@ -512,7 +513,7 @@ public Blob update(BlobInfo blobInfo, BlobTargetOption... options) {
512513
.getUpdateMaskBuilder()
513514
.addAllPaths(
514515
blobInfo.getModifiedFields().stream()
515-
.map(BlobField::getGrpcName)
516+
.map(NamedField::getGrpcName)
516517
.collect(ImmutableList.toImmutableList()));
517518
UpdateObjectRequest req = builder.build();
518519
return Retrying.run(

0 commit comments

Comments
 (0)