Skip to content
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

Jump to bottom

fix: update GrpcStorageImpl#update to support fine-grained update of BucketInfo.labels and BlobInfo.metadata #1843

Merged
merged 1 commit into from
Jan 12, 2023
Merged

fix: update GrpcStorageImpl#update to support fine-grained update of BucketInfo.labels and BlobInfo.metadata #1843

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 27 additions & 2 deletions google-cloud-storage/src/main/java/com/google/cloud/storage/ApiaryConversions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.function.Function;

@InternalApi
Expand Down Expand Up @@ -373,7 +374,14 @@ private Bucket bucketInfoEncode(BucketInfo from) {
from.getDefaultKmsKeyName(),
k -> new Encryption().setDefaultKmsKeyName(k),
to::setEncryption);
ifNonNull(from.getLabels(), to::setLabels);
Map<String, String> pbLabels = from.getLabels();
if (pbLabels != null && !Data.isNull(pbLabels)) {
pbLabels = Maps.newHashMapWithExpectedSize(from.getLabels().size());
for (Map.Entry<String, String> entry : from.getLabels().entrySet()) {
pbLabels.put(entry.getKey(), firstNonNull(entry.getValue(), Data.nullOf(String.class)));
}
}
to.setLabels(pbLabels);
maybeEncodeRetentionPolicy(from, to);
ifNonNull(from.getIamConfiguration(), this::iamConfigEncode, to::setIamConfiguration);
ifNonNull(from.getAutoclass(), this::autoclassEncode, to::setAutoclass);
Expand Down Expand Up @@ -412,7 +420,7 @@ private BucketInfo bucketInfoDecode(com.google.api.services.storage.model.Bucket
lift(Lifecycle::getRule).andThen(toImmutableListOf(lifecycleRule()::decode)),
to::setLifecycleRules);
ifNonNull(from.getDefaultEventBasedHold(), to::setDefaultEventBasedHold);
ifNonNull(from.getLabels(), to::setLabels);
ifNonNull(from.getLabels(), ApiaryConversions::replaceDataNullValuesWithNull, to::setLabels);
ifNonNull(from.getBilling(), Billing::getRequesterPays, to::setRequesterPays);
Encryption encryption = from.getEncryption();
if (encryption != null
Expand Down Expand Up @@ -917,4 +925,21 @@ private static void maybeDecodeRetentionPolicy(Bucket from, BucketInfo.Builder t
to::setRetentionPeriodDuration);
}
}

private static Map<String, String> replaceDataNullValuesWithNull(Map<String, String> labels) {
boolean anyDataNull = labels.values().stream().anyMatch(Data::isNull);
if (anyDataNull) {
// If we received any Data null values, replace them with null before setting.
// Explicitly use a HashMap as it allows null values.
Map<String, String> tmp = Maps.newHashMapWithExpectedSize(labels.size());
for (Entry<String, String> e : labels.entrySet()) {
String k = e.getKey();
String v = e.getValue();
tmp.put(k, Data.isNull(v) ? null : v);
}
return Collections.unmodifiableMap(tmp);
} else {
return labels;
}
}
}
34 changes: 21 additions & 13 deletions google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@
package com.google.cloud.storage;

import static com.google.cloud.storage.BackwardCompatibilityUtils.millisOffsetDateTimeCodec;
import static com.google.cloud.storage.Utils.diffMaps;
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkNotNull;

import com.google.api.client.util.Data;
import com.google.api.core.BetaApi;
import com.google.cloud.storage.Storage.BlobField;
import com.google.cloud.storage.TransportCompatibility.Transport;
import com.google.cloud.storage.UnifiedOpts.NamedField;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
Expand All @@ -38,6 +40,8 @@
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;

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

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

/** Sets the blob's user provided metadata. */
public abstract Builder setMetadata(Map<String, String> metadata);
public abstract Builder setMetadata(@Nullable Map<@NonNull String, @Nullable String> metadata);
Copy link
Member

Choose a reason for hiding this comment

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

If a dev is extended BlobInfo and they overload; they'll need to add @nullable as well IIUC?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The annotations are optional from the actual runtime standpoint, this is putting a label to the existing behavior that wasn't explicitly expressed anywhere.


abstract Builder setMetageneration(Long metageneration);

Expand Down Expand Up @@ -502,7 +506,7 @@ static final class BuilderImpl extends Builder {
private Boolean eventBasedHold;
private Boolean temporaryHold;
private OffsetDateTime retentionExpirationTime;
private final ImmutableSet.Builder<Storage.BlobField> modifiedFields = ImmutableSet.builder();
private final ImmutableSet.Builder<NamedField> modifiedFields = ImmutableSet.builder();

BuilderImpl(BlobId blobId) {
this.blobId = blobId;
Expand Down Expand Up @@ -757,15 +761,18 @@ Builder setMediaLink(String mediaLink) {
return this;
}

@SuppressWarnings({"UnnecessaryLocalVariable"})
@Override
public Builder setMetadata(Map<String, String> metadata) {
if (!Objects.equals(this.metadata, metadata)) {
modifiedFields.add(BlobField.METADATA);
}
if (metadata != null) {
this.metadata = new HashMap<>(metadata);
} else {
this.metadata = (Map<String, String>) Data.nullOf(ImmutableEmptyMap.class);
public Builder setMetadata(@Nullable Map<@NonNull String, @Nullable String> metadata) {
Map<String, String> left = this.metadata;
Map<String, String> right = metadata;
if (!Objects.equals(left, right)) {
diffMaps(BlobField.METADATA, left, right, modifiedFields::add);
if (right != null) {
this.metadata = new HashMap<>(right);
} else {
this.metadata = (Map<String, String>) Data.nullOf(ImmutableEmptyMap.class);
}
}
return this;
}
Expand Down Expand Up @@ -1317,7 +1324,8 @@ public String getMediaLink() {
}

/** Returns blob's user provided metadata. */
public Map<String, String> getMetadata() {
@Nullable
public Map<@NonNull String, @Nullable String> getMetadata() {
return metadata == null || Data.isNull(metadata) ? null : Collections.unmodifiableMap(metadata);
}

Expand Down Expand Up @@ -1617,7 +1625,7 @@ public boolean equals(Object o) {
&& Objects.equals(retentionExpirationTime, blobInfo.retentionExpirationTime);
}

ImmutableSet<BlobField> getModifiedFields() {
ImmutableSet<NamedField> getModifiedFields() {
return modifiedFields;
}

Expand Down
3 changes: 2 additions & 1 deletion google-cloud-storage/src/main/java/com/google/cloud/storage/Bucket.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import java.util.Map;
import java.util.Objects;
import org.checkerframework.checker.nullness.qual.NonNull;
import org.checkerframework.checker.nullness.qual.Nullable;

/**
* A Google cloud storage bucket.
Expand Down Expand Up @@ -546,7 +547,7 @@ public Builder setDefaultAcl(Iterable<Acl> acl) {
}

@Override
public Builder setLabels(Map<String, String> labels) {
public Builder setLabels(@Nullable Map<@NonNull String, @Nullable String> labels) {
infoBuilder.setLabels(labels);
return this;
}
Expand Down
40 changes: 21 additions & 19 deletions google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.cloud.storage.BackwardCompatibilityUtils.millisOffsetDateTimeCodec;
import static com.google.cloud.storage.BackwardCompatibilityUtils.millisUtcCodec;
import static com.google.cloud.storage.BackwardCompatibilityUtils.nullableDurationSecondsCodec;
import static com.google.cloud.storage.Utils.diffMaps;
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.collect.Lists.newArrayList;
Expand All @@ -29,12 +30,13 @@
import com.google.api.core.BetaApi;
import com.google.api.services.storage.model.Bucket.Lifecycle.Rule;
import com.google.cloud.storage.Acl.Entity;
import com.google.cloud.storage.BlobInfo.ImmutableEmptyMap;
import com.google.cloud.storage.Storage.BucketField;
import com.google.cloud.storage.TransportCompatibility.Transport;
import com.google.cloud.storage.UnifiedOpts.NamedField;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Maps;
import com.google.common.collect.Streams;
import java.io.IOException;
import java.io.ObjectInputStream;
Expand All @@ -43,6 +45,7 @@
import java.time.Duration;
import java.time.OffsetDateTime;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
Expand Down Expand Up @@ -100,7 +103,7 @@ public class BucketInfo implements Serializable {
private final String location;
private final Rpo rpo;
private final StorageClass storageClass;
private final Map<String, String> labels;
@Nullable private final Map<String, String> labels;
private final String defaultKmsKeyName;
private final Boolean defaultEventBasedHold;
private final OffsetDateTime retentionEffectiveTime;
Expand All @@ -111,7 +114,7 @@ public class BucketInfo implements Serializable {
private final String locationType;
private final Logging logging;
private final CustomPlacementConfig customPlacementConfig;
private final transient ImmutableSet<Storage.BucketField> modifiedFields;
private final transient ImmutableSet<NamedField> modifiedFields;

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

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

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

BuilderImpl(String name) {
this.name = name;
Expand Down Expand Up @@ -1909,20 +1912,18 @@ public Builder setDefaultAcl(Iterable<Acl> acl) {
return this;
}

@SuppressWarnings("UnnecessaryLocalVariable")
@Override
public Builder setLabels(Map<String, String> labels) {
if (labels != null) {
Map<String, String> tmp =
Maps.transformValues(
labels,
input -> {
// replace null values with empty strings
return input == null ? Data.nullOf(String.class) : input;
});
if (!Objects.equals(this.labels, tmp)) {
modifiedFields.add(BucketField.LABELS);
public Builder setLabels(@Nullable Map<@NonNull String, @Nullable String> labels) {
Map<String, String> left = this.labels;
Map<String, String> right = labels;
if (!Objects.equals(left, right)) {
diffMaps(BucketField.LABELS, left, right, modifiedFields::add);
if (right != null) {
this.labels = new HashMap<>(right);
} else {
this.labels = (Map<String, String>) Data.nullOf(ImmutableEmptyMap.class);
}
this.labels = tmp;
}
return this;
}
Expand Down Expand Up @@ -2483,7 +2484,8 @@ public List<Acl> getDefaultAcl() {
}

/** Returns the labels for this bucket. */
public Map<String, String> getLabels() {
@Nullable
public Map<@NonNull String, @Nullable String> getLabels() {
return labels;
}

Expand Down Expand Up @@ -2692,7 +2694,7 @@ Bucket asBucket(Storage storage) {
return new Bucket(storage, new BucketInfo.BuilderImpl(this));
}

ImmutableSet<BucketField> getModifiedFields() {
ImmutableSet<NamedField> getModifiedFields() {
return modifiedFields;
}

Expand Down
2 changes: 1 addition & 1 deletion google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcConversions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ private Bucket bucketInfoEncode(BucketInfo from) {
to.setVersioning(versioningBuilder.build());
}
ifNonNull(from.getDefaultEventBasedHold(), to::setDefaultEventBasedHold);
ifNonNull(from.getLabels(), to::putAllLabels);
ifNonNull(from.getLabels(), this::removeNullValues, to::putAllLabels);
// Do not use, #getLifecycleRules, it can not return null, which is important to our logic here
List<? extends LifecycleRule> lifecycleRules = from.lifecycleRules;
if (lifecycleRules != null) {
Expand Down
5 changes: 3 additions & 2 deletions google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
import com.google.cloud.storage.UnifiedOpts.HmacKeySourceOpt;
import com.google.cloud.storage.UnifiedOpts.HmacKeyTargetOpt;
import com.google.cloud.storage.UnifiedOpts.Mapper;
import com.google.cloud.storage.UnifiedOpts.NamedField;
import com.google.cloud.storage.UnifiedOpts.ObjectListOpt;
import com.google.cloud.storage.UnifiedOpts.ObjectSourceOpt;
import com.google.cloud.storage.UnifiedOpts.ObjectTargetOpt;
Expand Down Expand Up @@ -490,7 +491,7 @@ public Bucket update(BucketInfo bucketInfo, BucketTargetOption... options) {
.getUpdateMaskBuilder()
.addAllPaths(
bucketInfo.getModifiedFields().stream()
.map(BucketField::getGrpcName)
.map(NamedField::getGrpcName)
.collect(ImmutableList.toImmutableList()));
UpdateBucketRequest req = builder.build();
return Retrying.run(
Expand All @@ -512,7 +513,7 @@ public Blob update(BlobInfo blobInfo, BlobTargetOption... options) {
.getUpdateMaskBuilder()
.addAllPaths(
blobInfo.getModifiedFields().stream()
.map(BlobField::getGrpcName)
.map(NamedField::getGrpcName)
.collect(ImmutableList.toImmutableList()));
UpdateObjectRequest req = builder.build();
return Retrying.run(
Expand Down