fix: fix object metadata and bucket label change tracking to be fine-grained

google-cloud-storage/clirr-ignored-differences.xml

@@ -19,4 +19,15 @@
     <className>com/google/cloud/storage/Hasher$ConstantConcatValueHasher</className>
   </difference>
 
+  <difference>
+    <differenceType>7013</differenceType>
+    <className>com/google/cloud/storage/BlobInfo$Builder</className>
+    <method>* addAllMetadata(java.util.Map)</method>
+  </difference>
+  <difference>
+    <differenceType>7013</differenceType>
+    <className>com/google/cloud/storage/BucketInfo$Builder</className>
+    <method>* addAllLabels(java.util.Map)</method>
+  </difference>
+
 </differences>

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

@@ -79,6 +79,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.function.Function;
 
 @InternalApi
@@ -374,7 +375,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);
     Duration retentionPeriod = from.getRetentionPeriodDuration();
     if (retentionPeriod == null) {
       to.setRetentionPolicy(Data.nullOf(Bucket.RetentionPolicy.class));
@@ -425,7 +433,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
@@ -901,4 +909,21 @@ private Bucket.CustomPlacementConfig customPlacementConfigEncode(CustomPlacement
   private CustomPlacementConfig customPlacementConfigDecode(Bucket.CustomPlacementConfig from) {
     return CustomPlacementConfig.newBuilder().setDataLocations(from.getDataLocations()).build();
   }
+
+  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;
+    }
+  }
 }

google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java

@@ -44,7 +44,9 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.concurrent.TimeUnit;
+import javax.annotation.Nonnull;
 import org.checkerframework.checker.nullness.qual.NonNull;
+import org.checkerframework.checker.nullness.qual.Nullable;
 
 /**
  * An object in Google Cloud Storage. A {@code Blob} object includes the {@code BlobId} instance,
@@ -360,7 +362,14 @@ Builder setMediaLink(String mediaLink) {
     }
 
     @Override
-    public Builder setMetadata(Map<String, String> metadata) {
+    @BetaApi
+    public Builder addAllMetadata(@Nonnull Map<@NonNull String, @Nullable String> newMetadata) {
+      infoBuilder.addAllMetadata(newMetadata);
+      return this;
+    }
+
+    @Override
+    public Builder setMetadata(@Nullable Map<@NonNull String, @Nullable String> metadata) {
       infoBuilder.setMetadata(metadata);
       return this;
     }

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

@@ -17,13 +17,16 @@
 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 static java.util.Objects.requireNonNull;
 
 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;
@@ -38,6 +41,9 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import javax.annotation.Nonnull;
+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
@@ -100,7 +106,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> {
@@ -330,8 +336,28 @@ public Builder setTimeStorageClassUpdatedOffsetDateTime(
       return setTimeStorageClassUpdated(millisOffsetDateTimeCodec.decode(timeStorageClassUpdated));
     }
 
-    /** Sets the blob's user provided metadata. */
-    public abstract Builder setMetadata(Map<String, String> metadata);
+    /**
+     * Convenience method to add new metadata values to any existing present metadata.
+     *
+     * <p>If a key from {@code newMetadata} overlaps with any existing key in {@link
+     * #getMetadata()}, the new value will overwrite the old value.
+     *
+     * @see #setMetadata(Map)
+     */
+    @BetaApi
+    public abstract Builder addAllMetadata(
+        @Nonnull Map<@NonNull String, @Nullable String> newMetadata);
+
+    /**
+     * Sets the blob's user provided metadata.
+     *
+     * <p>This method will set the metadata to exactly what is provided by {@code metadata}.
+     *
+     * <p>If you instead want to add or modify existing values use {@link #addAllMetadata(Map)}.
+     *
+     * @see #addAllMetadata(Map)
+     */
+    public abstract Builder setMetadata(@Nullable Map<@NonNull String, @Nullable String> metadata);
 
     abstract Builder setMetageneration(Long metageneration);
 
@@ -502,7 +528,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;
@@ -758,14 +784,26 @@ Builder setMediaLink(String mediaLink) {
     }
 
     @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);
+    @BetaApi
+    public Builder addAllMetadata(@Nonnull Map<@NonNull String, @Nullable String> newMetadata) {
+      requireNonNull(newMetadata, "newMetadata must be non null");
+      HashMap<@NonNull String, @Nullable String> map = Utils.newHashMap(this.metadata);
+      map.putAll(newMetadata);
+      return setMetadata(map);
+    }
+
+    @SuppressWarnings({"UnnecessaryLocalVariable"})
+    @Override
+    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;
     }
@@ -1317,7 +1355,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);
   }
 
@@ -1617,7 +1656,7 @@ public boolean equals(Object o) {
         && Objects.equals(retentionExpirationTime, blobInfo.retentionExpirationTime);
   }
 
-  ImmutableSet<BlobField> getModifiedFields() {
+  ImmutableSet<NamedField> getModifiedFields() {
     return modifiedFields;
   }
 

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

@@ -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.
@@ -546,7 +547,13 @@ public Builder setDefaultAcl(Iterable<Acl> acl) {
     }
 
     @Override
-    public Builder setLabels(Map<String, String> labels) {
+    public Builder addAllLabels(@NonNull Map<@NonNull String, @Nullable String> newLabels) {
+      infoBuilder.addAllLabels(newLabels);
+      return this;
+    }
+
+    @Override
+    public Builder setLabels(@Nullable Map<@NonNull String, @Nullable String> labels) {
       infoBuilder.setLabels(labels);
       return this;
     }

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

@@ -19,22 +19,25 @@
 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;
+import static java.util.Objects.requireNonNull;
 
 import com.google.api.client.json.jackson2.JacksonFactory;
 import com.google.api.client.util.Data;
 import com.google.api.client.util.DateTime;
 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;
@@ -43,6 +46,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;
@@ -100,7 +104,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;
@@ -111,7 +115,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
@@ -1476,8 +1480,27 @@ 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);
+    /**
+     * Convenience method to add new labels values to any existing present labels.
+     *
+     * <p>If a key from {@code newLabels} overlaps with any existing key in {@link #getLabels()},
+     * the new value will overwrite the old value.
+     *
+     * @see #setLabels(Map)
+     */
+    @BetaApi
+    public abstract Builder addAllLabels(@NonNull Map<@NonNull String, @Nullable String> newLabels);
+
+    /**
+     * Sets the bucket's user provided labels.
+     *
+     * <p>This method will set the labels to exactly what is provided by {@code labels}.
+     *
+     * <p>If you instead want to add or modify existing values use {@link #addAllLabels(Map)}.
+     *
+     * @see #addAllLabels(Map)
+     */
+    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);
@@ -1630,7 +1653,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;
@@ -1910,19 +1933,25 @@ public Builder setDefaultAcl(Iterable<Acl> acl) {
     }
 
     @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 addAllLabels(@NonNull Map<@NonNull String, @Nullable String> newLabels) {
+      requireNonNull(newLabels, "newLabels must be non null");
+      HashMap<@NonNull String, @Nullable String> map = Utils.newHashMap(this.labels);
+      map.putAll(newLabels);
+      return setLabels(map);
+    }
+
+    @SuppressWarnings("UnnecessaryLocalVariable")
+    @Override
+    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;
     }
@@ -2483,7 +2512,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;
   }
 
@@ -2692,7 +2722,7 @@ Bucket asBucket(Storage storage) {
     return new Bucket(storage, new BucketInfo.BuilderImpl(this));
   }
 
-  ImmutableSet<BucketField> getModifiedFields() {
+  ImmutableSet<NamedField> getModifiedFields() {
     return modifiedFields;
   }