From b471198e2cdd58591162b2bfa3dae1b0b785ebf4 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Wed, 28 Dec 2022 18:16:22 -0500 Subject: [PATCH] fix: fix object metadata and bucket label change tracking to be fine-grained ### Context Historically, when calling storage#update(BlobInfo) or storage#update(BucketInfo) the whole info would be packed and sent to the backend as a PATCH request. The backend would then perform some server side diffing against what it already had and make modifications. Of note here, for Object#metadata and Bucket#labels the PATCH behavior meant that any value provided in the request was always treated as an append. #### Code sample Specifically, the following call sequence will actually append `key2: value2` to the object metadata despite nothing in the method names communicating this. ```java try (Storage storage = StorageOptions.http().build().getService()) { BlobInfo info = BlobInfo.newBuilder("bucket", "object") .setMetadata(ImmutableMap.of("key1", "value1")) .build(); Blob gen1 = storage.create(info); BlobInfo modified = gen1.toBuilder().setMetadata(ImmutableMap.of("key2", "value2")).build(); Blob gen2 = storage.update(modified, BlobTargetOption.metagenerationMatch()); // gen2.metadata = {"key1": "value1", "key2": "value2"} } ``` Additionally, this now leaves the internal state of `modified` such that if you were to call `modified.getMetadata()` it would only contain the new `key2: value2`. However, after calling `storage.update` `gen2.getMetadata()` both keys are present. This is confusing, and can be remedied now that we have the capability of tracking field level modification. Another confusing thing about this: if you want to clear the metadata on an object, the intuitive `.setMetadata(Collections.emptyMap()` does not work. Instead, you would need to do the following: ```java try (Storage storage = StorageOptions.http().build().getService()) { Blob gen1 = storage.get("bucket", "object"); // Can't use ImmutableMap here, as we need null values HashMap clearedKeys = new HashMap<>(); for (String key : gen1.getMetadata().keySet()) { clearedKeys.put(key, null); } BlobInfo modified = gen1.toBuilder().setMetadata(clearedKeys).build(); Blob gen2 = storage.update(modified, BlobTargetOption.metagenerationMatch()); } ``` ### The Change Update the behavior of `BlobInfo#setMetadata` and `BucketInfo#setLabels` to always explicitly set to the specified value, whether adding, removing or updating a key. Add new methods `BlobInfo#addAllMetadata(Map)` and `BucketInfo#addAllLabels(Map)` to allow performing an upsert without the need to first query and reconcile. ##### Clearing all object metadata ```java try (Storage storage = StorageOptions.http().build().getService()) { Blob gen1 = storage.get("bucket", "object"); BlobInfo modified = gen1.toBuilder().setMetadata(Collections.emptyMap()).build(); Blob gen2 = storage.update(modified, BlobTargetOption.metagenerationMatch()); } ``` ##### Adding/modifying object metadata ```java try (Storage storage = StorageOptions.http().build().getService()) { Blob gen1 = storage.get("bucket", "object"); BlobInfo modified = gen1.toBuilder().addAllMetadata(ImmutableMap.of("key1", "value2", "key3", "value3").build(); Blob gen2 = storage.update(modified, BlobTargetOption.metagenerationMatch()); } ``` ##### Removing an individual metadata key ```java try (Storage storage = StorageOptions.http().build().getService()) { Blob gen1 = storage.get("bucket", "object"); HashMap map = new HashMap<>(); map.put("key1", null); BlobInfo modified = gen1.toBuilder().addAllMetadata(map).build(); Blob gen2 = storage.update(modified, BlobTargetOption.metagenerationMatch()); } ``` ### Implementation Details 1. Field modification now tracks nested metadata/label keys * New UnifiedOpts.NestedNamedField has been added to track nested keys * UpdateMaskTest has been updated to expect nested field names 2. `StorageImpl#update(BlobInfo)` and `StorageImpl#update(BucketInfo)` have both been updated to take field modification into consideration. 3. New test class `ITNestedUpdateMaskTest` has been added to validate many permutations of modifying metadata/labels 4. Each of the metadata/label related methods have had `@NonNull`/`@Nullable` annotations to their signatures to communicate acceptable arguments, and to set expectations. 5. All `Data.isNull`/`Data.nullOf` has been moved away from Blob metadata and Bucket labels into `ApiaryConversions` as they only apply to Apiary usage not gRPC/Protobuf. 6. Several easymock test in StorageImplTest have been wholly deleted as they weren't testing behavior, only locking implementation. 7. Update BucketInfoTest deep equals assertions to use `TestUtils.assertAll` ### Samples Update samples to use new `addAll*` methods for their upsert use cases rather than `set*`. --- .../clirr-ignored-differences.xml | 11 + .../cloud/storage/ApiaryConversions.java | 29 ++- .../java/com/google/cloud/storage/Blob.java | 11 +- .../com/google/cloud/storage/BlobInfo.java | 67 ++++-- .../java/com/google/cloud/storage/Bucket.java | 9 +- .../com/google/cloud/storage/BucketInfo.java | 70 +++++-- .../google/cloud/storage/GrpcConversions.java | 2 +- .../google/cloud/storage/GrpcStorageImpl.java | 5 +- .../com/google/cloud/storage/StorageImpl.java | 119 +++++++++-- .../com/google/cloud/storage/UnifiedOpts.java | 52 ++++- .../java/com/google/cloud/storage/Utils.java | 49 +++++ .../google/cloud/storage/BucketInfoTest.java | 146 ++++++------- .../google/cloud/storage/StorageImplTest.java | 56 ----- .../com/google/cloud/storage/TestUtils.java | 25 +++ .../google/cloud/storage/UpdateMaskTest.java | 17 +- .../storage/it/ITNestedUpdateMaskTest.java | 195 ++++++++++++++++++ .../google/cloud/storage/it/ITObjectTest.java | 6 +- .../storage/bucket/AddBucketLabel.java | 6 +- .../storage/bucket/RemoveBucketLabel.java | 9 +- .../object/BatchSetObjectMetadata.java | 2 +- .../storage/object/SetObjectMetadata.java | 2 +- 21 files changed, 678 insertions(+), 210 deletions(-) create mode 100644 google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITNestedUpdateMaskTest.java diff --git a/google-cloud-storage/clirr-ignored-differences.xml b/google-cloud-storage/clirr-ignored-differences.xml index 51e0258c4f..43a8980764 100644 --- a/google-cloud-storage/clirr-ignored-differences.xml +++ b/google-cloud-storage/clirr-ignored-differences.xml @@ -19,4 +19,15 @@ com/google/cloud/storage/Hasher$ConstantConcatValueHasher + + 7013 + com/google/cloud/storage/BlobInfo$Builder + * addAllMetadata(java.util.Map) + + + 7013 + com/google/cloud/storage/BucketInfo$Builder + * addAllLabels(java.util.Map) + + diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/ApiaryConversions.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/ApiaryConversions.java index d5ca3b594b..fa43c7e78a 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/ApiaryConversions.java +++ b/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 pbLabels = from.getLabels(); + if (pbLabels != null && !Data.isNull(pbLabels)) { + pbLabels = Maps.newHashMapWithExpectedSize(from.getLabels().size()); + for (Map.Entry 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 replaceDataNullValuesWithNull(Map 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 tmp = Maps.newHashMapWithExpectedSize(labels.size()); + for (Entry 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; + } + } } diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java index 15d3c489fc..348187ff37 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/Blob.java +++ b/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 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; } diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java index 95716f8947..b28192c2a4 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java +++ b/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 modifiedFields; + private final transient ImmutableSet modifiedFields; /** This class is meant for internal use only. Users are discouraged from using this class. */ public static final class ImmutableEmptyMap extends AbstractMap { @@ -330,8 +336,28 @@ public Builder setTimeStorageClassUpdatedOffsetDateTime( return setTimeStorageClassUpdated(millisOffsetDateTimeCodec.decode(timeStorageClassUpdated)); } - /** Sets the blob's user provided metadata. */ - public abstract Builder setMetadata(Map metadata); + /** + * Convenience method to add new metadata values to any existing present metadata. + * + *

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. + * + *

This method will set the metadata to exactly what is provided by {@code metadata}. + * + *

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 modifiedFields = ImmutableSet.builder(); + private final ImmutableSet.Builder modifiedFields = ImmutableSet.builder(); BuilderImpl(BlobId blobId) { this.blobId = blobId; @@ -758,14 +784,26 @@ Builder setMediaLink(String mediaLink) { } @Override - public Builder setMetadata(Map metadata) { - if (!Objects.equals(this.metadata, metadata)) { - modifiedFields.add(BlobField.METADATA); - } - if (metadata != null) { - this.metadata = new HashMap<>(metadata); - } else { - this.metadata = (Map) 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 left = this.metadata; + Map 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) Data.nullOf(ImmutableEmptyMap.class); + } } return this; } @@ -1317,7 +1355,8 @@ public String getMediaLink() { } /** Returns blob's user provided metadata. */ - public Map 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 getModifiedFields() { + ImmutableSet getModifiedFields() { return modifiedFields; } diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/Bucket.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/Bucket.java index 6147552c08..ac4db7a3d7 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/Bucket.java +++ b/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) { } @Override - public Builder setLabels(Map 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; } diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index 2a2c7256ae..7f8ccc290a 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -19,9 +19,11 @@ 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; @@ -29,12 +31,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; @@ -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 labels; + @Nullable private final Map 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 modifiedFields; + private final transient ImmutableSet 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); - /** Sets the label of this bucket. */ - public abstract Builder setLabels(Map labels); + /** + * Convenience method to add new labels values to any existing present labels. + * + *

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. + * + *

This method will set the labels to exactly what is provided by {@code labels}. + * + *

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 modifiedFields = ImmutableSet.builder(); + private final ImmutableSet.Builder modifiedFields = ImmutableSet.builder(); BuilderImpl(String name) { this.name = name; @@ -1910,19 +1933,25 @@ public Builder setDefaultAcl(Iterable acl) { } @Override - public Builder setLabels(Map labels) { - if (labels != null) { - Map 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 left = this.labels; + Map 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) Data.nullOf(ImmutableEmptyMap.class); } - this.labels = tmp; } return this; } @@ -2483,7 +2512,8 @@ public List getDefaultAcl() { } /** Returns the labels for this bucket. */ - public Map 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 getModifiedFields() { + ImmutableSet getModifiedFields() { return modifiedFields; } diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcConversions.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcConversions.java index 3a29f16de9..7733ab08c1 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcConversions.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcConversions.java @@ -347,7 +347,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 lifecycleRules = from.lifecycleRules; if (lifecycleRules != null) { 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 dc8e475e19..528cfd8133 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 @@ -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; @@ -494,7 +495,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( @@ -516,7 +517,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( 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 efbbd99826..8fe7aeeb4a 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 @@ -23,6 +23,8 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.concurrent.Executors.callable; +import com.google.api.client.json.GenericJson; +import com.google.api.client.util.Data; import com.google.api.gax.paging.Page; import com.google.api.gax.retrying.ResultRetryAlgorithm; import com.google.api.services.storage.model.BucketAccessControl; @@ -43,6 +45,8 @@ import com.google.cloud.storage.PostPolicyV4.PostConditionsV4; import com.google.cloud.storage.PostPolicyV4.PostFieldsV4; import com.google.cloud.storage.PostPolicyV4.PostPolicyV4Document; +import com.google.cloud.storage.UnifiedOpts.BucketTargetOpt; +import com.google.cloud.storage.UnifiedOpts.NamedField; import com.google.cloud.storage.UnifiedOpts.ObjectSourceOpt; import com.google.cloud.storage.UnifiedOpts.ObjectTargetOpt; import com.google.cloud.storage.UnifiedOpts.Opts; @@ -86,7 +90,9 @@ import java.util.TimeZone; import java.util.concurrent.Callable; import java.util.concurrent.TimeUnit; +import java.util.function.BiConsumer; import java.util.function.Function; +import java.util.function.Supplier; final class StorageImpl extends BaseService implements Storage { @@ -103,6 +109,22 @@ final class StorageImpl extends BaseService implements Storage { private static final int MIN_BUFFER_SIZE = 256 * 1024; private static final ApiaryConversions codecs = Conversions.apiary(); + private static final JsonTrimmer STORAGE_OBJECT_JSON_TRIMMER = + new JsonTrimmer<>( + ImmutableSet.copyOf(BlobField.REQUIRED_FIELDS), + BlobField.METADATA, + StorageObject::new, + StorageObject::getMetadata, + StorageObject::setMetadata); + + private static final JsonTrimmer + BUCKET_JSON_TRIMMER = + new JsonTrimmer<>( + ImmutableSet.copyOf(BucketField.REQUIRED_FIELDS), + BucketField.LABELS, + com.google.api.services.storage.model.Bucket::new, + com.google.api.services.storage.model.Bucket::getLabels, + com.google.api.services.storage.model.Bucket::setLabels); final HttpRetryAlgorithmManager retryAlgorithmManager; final StorageRpc storageRpc; @@ -438,15 +460,16 @@ 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 = - Opts.unwrap(options).resolveFrom(bucketInfo).getRpcOptions(); + Opts opts = Opts.unwrap(options).resolveFrom(bucketInfo); + Map optionsMap = opts.getRpcOptions(); + com.google.api.services.storage.model.Bucket pb = codecs.bucketInfo().encode(bucketInfo); + com.google.api.services.storage.model.Bucket trimPb = + BUCKET_JSON_TRIMMER.trim(pb, bucketInfo.getModifiedFields()); ResultRetryAlgorithm algorithm = - retryAlgorithmManager.getForBucketsUpdate(bucketPb, optionsMap); + retryAlgorithmManager.getForBucketsUpdate(trimPb, optionsMap); return run( algorithm, - () -> storageRpc.patch(bucketPb, optionsMap), + () -> storageRpc.patch(trimPb, optionsMap), (x) -> Conversions.apiary().bucketInfo().decode(x).asBucket(this)); } @@ -456,14 +479,13 @@ public Blob update(BlobInfo blobInfo, BlobTargetOption... options) { Map optionsMap = opts.getRpcOptions(); BlobInfo updated = opts.blobInfoMapper().apply(blobInfo.toBuilder()).build(); StorageObject pb = codecs.blobInfo().encode(updated); - ResultRetryAlgorithm algorithm = retryAlgorithmManager.getForObjectsUpdate(pb, optionsMap); + StorageObject trimPb = STORAGE_OBJECT_JSON_TRIMMER.trim(pb, blobInfo.getModifiedFields()); + ResultRetryAlgorithm algorithm = + retryAlgorithmManager.getForObjectsUpdate(trimPb, optionsMap); return run( algorithm, - () -> storageRpc.patch(pb, optionsMap), - (x) -> { - BlobInfo info = Conversions.apiary().blobInfo().decode(x); - return info.asBlob(this); - }); + () -> storageRpc.patch(trimPb, optionsMap), + (x) -> Conversions.apiary().blobInfo().decode(x).asBlob(this)); } @Override @@ -1528,4 +1550,77 @@ public boolean deleteNotification(final String bucket, final String notification public HttpStorageOptions getOptions() { return (HttpStorageOptions) super.getOptions(); } + + /** + * Our "Update" methods actually issue {@code PATCH} requests. Historically, all fields present in + * the respective {@link BlobInfo} or {@link BucketInfo} would all be sent to the backend at which + * point the backend would perform some diffing to determine what to change. With the addition of + * gRPC support, our models now track field level modification allowing for fine-grained requests. + * Unfortunately, this fine-grained tracking does not yet extend to the actual encoding/decoding + * of messages to models. As such, this class encapsulates the logic of performing a postprocess + * step of trimming down a json object to only those fields which are required or modified. + * + *

An additional consideration is fields which provide nested keys {@link + * BlobInfo#getMetadata()} and {@link BucketInfo#getLabels()}. As they have special logic in how + * {@code PATCH} is handled. + * + *

As part of defining a trimmer, an accessor/consumer pair is defined representing the get/set + * method pair for the nested keys handling. + * + *

One note: this class is not fully generic with respect to deeply nested definitions, only + * what is needed specifically for {@link BucketInfo#getLabels()} and {@link + * BlobInfo#getMetadata()}. + */ + private static final class JsonTrimmer { + private final ImmutableSet requiredFields; + private final Supplier constructor; + private final Function> metadataAccessor; + private final BiConsumer> metadataConsumer; + private final NamedField metadataNamedField; + + private JsonTrimmer( + ImmutableSet requiredFields, + NamedField metadataNamedField, + Supplier constructor, + Function> metadataAccessor, + BiConsumer> metadataConsumer) { + this.requiredFields = requiredFields; + this.constructor = constructor; + this.metadataAccessor = metadataAccessor; + this.metadataConsumer = metadataConsumer; + this.metadataNamedField = metadataNamedField; + } + + T trim(T base, ImmutableSet modifiedFields) { + if (modifiedFields.isEmpty()) { + return base; + } + + T trimPb = constructor.get(); + + Map metadata = new HashMap<>(); + Iterable concat = Iterables.concat(requiredFields, modifiedFields); + for (NamedField nf : concat) { + String apiaryName = nf.getApiaryName(); + + String metadataApiaryName = metadataNamedField.getApiaryName(); + if (apiaryName.startsWith(metadataApiaryName)) { + String key = apiaryName.substring(metadataApiaryName.length() + 1); + Map map = getOrEmpty(metadataAccessor.apply(base)); + String s = map.get(key); + metadata.put(key, firstNonNull(s, Data.nullOf(String.class))); + } else { + trimPb.put(apiaryName, base.get(apiaryName)); + } + } + if (!metadata.isEmpty()) { + metadataConsumer.accept(trimPb, metadata); + } + return trimPb; + } + + private static Map getOrEmpty(Map map) { + return map != null ? map : Collections.emptyMap(); + } + } } 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 6eecc74fd4..6e78001b24 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 @@ -2361,6 +2361,10 @@ static NamedField prefixed(String prefix, NamedField delegate) { static NamedField literal(String name) { return new LiteralNamedField(name); } + + static NamedField nested(NamedField parent, NamedField child) { + return new NestedNamedField(parent, child); + } } private static CommonObjectRequestParams.Builder customerSuppliedKey( @@ -2376,7 +2380,7 @@ private static final class PrefixedNamedField implements NamedField { private final String prefix; private final NamedField delegate; - public PrefixedNamedField(String prefix, NamedField delegate) { + private PrefixedNamedField(String prefix, NamedField delegate) { this.prefix = prefix; this.delegate = delegate; } @@ -2417,11 +2421,11 @@ public String toString() { } } - private static class LiteralNamedField implements NamedField { + private static final class LiteralNamedField implements NamedField { private final String name; - LiteralNamedField(String name) { + private LiteralNamedField(String name) { this.name = name; } @@ -2457,4 +2461,46 @@ public String toString() { return MoreObjects.toStringHelper(this).add("name", name).toString(); } } + + private static final class NestedNamedField implements NamedField { + private final NamedField parent; + private final NamedField child; + + private NestedNamedField(NamedField parent, NamedField child) { + this.parent = parent; + this.child = child; + } + + @Override + public String getApiaryName() { + return parent.getApiaryName() + "." + child.getApiaryName(); + } + + @Override + public String getGrpcName() { + return parent.getGrpcName() + "." + child.getGrpcName(); + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof NestedNamedField)) { + return false; + } + NestedNamedField that = (NestedNamedField) o; + return Objects.equals(parent, that.parent) && Objects.equals(child, that.child); + } + + @Override + public int hashCode() { + return Objects.hash(parent, child); + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this).add("parent", parent).add("child", child).toString(); + } + } } diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/Utils.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/Utils.java index 9a37855691..0c35f44391 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/Utils.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/Utils.java @@ -21,8 +21,11 @@ import com.google.api.client.util.DateTime; import com.google.api.core.InternalApi; import com.google.cloud.storage.Conversions.Codec; +import com.google.cloud.storage.UnifiedOpts.NamedField; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; +import com.google.common.collect.MapDifference; +import com.google.common.collect.Maps; import com.google.common.io.BaseEncoding; import com.google.common.primitives.Ints; import com.google.storage.v2.BucketName; @@ -33,11 +36,14 @@ import java.time.ZoneOffset; import java.time.format.DateTimeFormatter; import java.time.temporal.ChronoUnit; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Supplier; +import java.util.stream.Stream; import org.checkerframework.checker.nullness.qual.NonNull; import org.checkerframework.checker.nullness.qual.Nullable; @@ -234,6 +240,49 @@ static T firstNonNull(Supplier<@Nullable T>... ss) { throw new IllegalStateException("Unable to resolve non-null value"); } + /** + * Diff two maps, and append each differing key to {@code sink} with the parent of {{@code parent} + */ + @SuppressWarnings("ConstantValue") + static void diffMaps( + NamedField parent, + Map left, + Map right, + Consumer sink) { + final Stream keys; + if (left != null && right == null) { + keys = left.keySet().stream(); + } else if (left == null && right != null) { + keys = right.keySet().stream(); + } else if (left != null && right != null) { + MapDifference difference = Maps.difference(left, right); + keys = + Stream.of( + // keys with modified values + difference.entriesDiffering().keySet().stream(), + // removed keys + difference.entriesOnlyOnLeft().keySet().stream(), + // new keys + difference.entriesOnlyOnRight().keySet().stream()) + .flatMap(x -> x); + } else { + keys = Stream.empty(); + } + keys.map(NamedField::literal).map(k -> NamedField.nested(parent, k)).forEach(sink); + } + + /** + * Null friendly alternative to {@link Maps#newHashMap(Map)}. If {@code baseMap} is null, a new + * HashMap will still be returned. + */ + static HashMap newHashMap(@Nullable Map baseMap) { + HashMap map = new HashMap<>(); + if (baseMap != null) { + map.putAll(baseMap); + } + return map; + } + private static int crc32cDecode(String from) { byte[] decodeCrc32c = BaseEncoding.base64().decode(from); return Ints.fromByteArray(decodeCrc32c); diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java index 4cb1ac5457..a11d989bfc 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java @@ -17,6 +17,7 @@ package com.google.cloud.storage; import static com.google.cloud.storage.Acl.Project.ProjectRole.VIEWERS; +import static com.google.cloud.storage.TestUtils.assertAll; import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -45,12 +46,10 @@ import com.google.cloud.storage.BucketInfo.PublicAccessPrevention; import com.google.cloud.storage.Conversions.Codec; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import java.io.IOException; import java.io.StringWriter; import java.util.Arrays; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; import org.junit.Test; @@ -100,16 +99,9 @@ public class BucketInfoTest { private static final String DEFAULT_KMS_KEY_NAME = "projects/p/locations/kr-loc/keyRings/kr/cryptoKeys/key"; private static final Boolean VERSIONING_ENABLED = true; - private static final Map BUCKET_LABELS; + private static final Map BUCKET_LABELS = + TestUtils.hashMapOf("label1", "value1", "label2", null); - static { - BUCKET_LABELS = new HashMap<>(); - BUCKET_LABELS.put("label1", "value1"); - BUCKET_LABELS.put("label2", null); - } - - private static final Map BUCKET_LABELS_TARGET = - ImmutableMap.of("label1", "value1", "label2", ""); private static final Boolean REQUESTER_PAYS = true; private static final Boolean DEFAULT_EVENT_BASED_HOLD = true; private static final Long RETENTION_EFFECTIVE_TIME = 10L; @@ -186,7 +178,7 @@ public class BucketInfoTest { private static final Lifecycle EMPTY_LIFECYCLE = lifecycle(Collections.emptyList()); @Test - public void testToBuilder() { + public void testToBuilder() throws Exception { compareBuckets(BUCKET_INFO, BUCKET_INFO.toBuilder().build()); BucketInfo bucketInfo = BUCKET_INFO.toBuilder().setName("B").setGeneratedId("id").build(); assertEquals("B", bucketInfo.getName()); @@ -197,7 +189,7 @@ public void testToBuilder() { } @Test - public void testToBuilderIncomplete() { + public void testToBuilderIncomplete() throws Exception { BucketInfo incompleteBucketInfo = BucketInfo.newBuilder("b").build(); compareBuckets(incompleteBucketInfo, incompleteBucketInfo.toBuilder().build()); } @@ -210,39 +202,40 @@ public void testOf() { @Test @SuppressWarnings({"unchecked", "deprecation"}) - public void testBuilder() { - assertEquals("b", BUCKET_INFO.getName()); - assertEquals(ACL, BUCKET_INFO.getAcl()); - assertEquals(ETAG, BUCKET_INFO.getEtag()); - assertEquals(GENERATED_ID, BUCKET_INFO.getGeneratedId()); - assertEquals(META_GENERATION, BUCKET_INFO.getMetageneration()); - assertEquals(OWNER, BUCKET_INFO.getOwner()); - assertEquals(SELF_LINK, BUCKET_INFO.getSelfLink()); - assertEquals(CREATE_TIME, BUCKET_INFO.getCreateTime()); - assertEquals(UPDATE_TIME, BUCKET_INFO.getUpdateTime()); - assertEquals(CORS, BUCKET_INFO.getCors()); - assertEquals(DEFAULT_ACL, BUCKET_INFO.getDefaultAcl()); - assertEquals(DELETE_RULES, BUCKET_INFO.getDeleteRules()); - assertEquals(INDEX_PAGE, BUCKET_INFO.getIndexPage()); - assertEquals(IAM_CONFIGURATION, BUCKET_INFO.getIamConfiguration()); - assertEquals(NOT_FOUND_PAGE, BUCKET_INFO.getNotFoundPage()); - assertEquals(LOCATION, BUCKET_INFO.getLocation()); - assertEquals(STORAGE_CLASS, BUCKET_INFO.getStorageClass()); - assertEquals(DEFAULT_KMS_KEY_NAME, BUCKET_INFO.getDefaultKmsKeyName()); - assertEquals(VERSIONING_ENABLED, BUCKET_INFO.versioningEnabled()); - assertEquals(BUCKET_LABELS_TARGET, BUCKET_INFO.getLabels()); - assertEquals(REQUESTER_PAYS, BUCKET_INFO.requesterPays()); - assertEquals(DEFAULT_EVENT_BASED_HOLD, BUCKET_INFO.getDefaultEventBasedHold()); - assertEquals(RETENTION_EFFECTIVE_TIME, BUCKET_INFO.getRetentionEffectiveTime()); - assertEquals(RETENTION_PERIOD, BUCKET_INFO.getRetentionPeriod()); - assertEquals(RETENTION_POLICY_IS_LOCKED, BUCKET_INFO.retentionPolicyIsLocked()); - assertTrue(LOCATION_TYPES.contains(BUCKET_INFO.getLocationType())); - assertEquals(LOGGING, BUCKET_INFO.getLogging()); + public void testBuilder() throws Exception { + assertAll( + () -> assertEquals("b", BUCKET_INFO.getName()), + () -> assertEquals(ACL, BUCKET_INFO.getAcl()), + () -> assertEquals(ETAG, BUCKET_INFO.getEtag()), + () -> assertEquals(GENERATED_ID, BUCKET_INFO.getGeneratedId()), + () -> assertEquals(META_GENERATION, BUCKET_INFO.getMetageneration()), + () -> assertEquals(OWNER, BUCKET_INFO.getOwner()), + () -> assertEquals(SELF_LINK, BUCKET_INFO.getSelfLink()), + () -> assertEquals(CREATE_TIME, BUCKET_INFO.getCreateTime()), + () -> assertEquals(UPDATE_TIME, BUCKET_INFO.getUpdateTime()), + () -> assertEquals(CORS, BUCKET_INFO.getCors()), + () -> assertEquals(DEFAULT_ACL, BUCKET_INFO.getDefaultAcl()), + () -> assertEquals(DELETE_RULES, BUCKET_INFO.getDeleteRules()), + () -> assertEquals(INDEX_PAGE, BUCKET_INFO.getIndexPage()), + () -> assertEquals(IAM_CONFIGURATION, BUCKET_INFO.getIamConfiguration()), + () -> assertEquals(NOT_FOUND_PAGE, BUCKET_INFO.getNotFoundPage()), + () -> assertEquals(LOCATION, BUCKET_INFO.getLocation()), + () -> assertEquals(STORAGE_CLASS, BUCKET_INFO.getStorageClass()), + () -> assertEquals(DEFAULT_KMS_KEY_NAME, BUCKET_INFO.getDefaultKmsKeyName()), + () -> assertEquals(VERSIONING_ENABLED, BUCKET_INFO.versioningEnabled()), + () -> assertEquals(BUCKET_LABELS, BUCKET_INFO.getLabels()), + () -> assertEquals(REQUESTER_PAYS, BUCKET_INFO.requesterPays()), + () -> assertEquals(DEFAULT_EVENT_BASED_HOLD, BUCKET_INFO.getDefaultEventBasedHold()), + () -> assertEquals(RETENTION_EFFECTIVE_TIME, BUCKET_INFO.getRetentionEffectiveTime()), + () -> assertEquals(RETENTION_PERIOD, BUCKET_INFO.getRetentionPeriod()), + () -> assertEquals(RETENTION_POLICY_IS_LOCKED, BUCKET_INFO.retentionPolicyIsLocked()), + () -> assertTrue(LOCATION_TYPES.contains(BUCKET_INFO.getLocationType())), + () -> assertEquals(LOGGING, BUCKET_INFO.getLogging())); } @Test @SuppressWarnings({"unchecked", "deprecation"}) - public void testToPbAndFromPb() { + public void testToPbAndFromPb() throws Exception { Codec codec = Conversions.apiary().bucketInfo(); Bucket encode1 = codec.encode(BUCKET_INFO); @@ -260,37 +253,44 @@ public void testToPbAndFromPb() { compareBuckets(bucketInfo, decode2); } - private void compareBuckets(BucketInfo expected, BucketInfo value) { - assertEquals(expected.getName(), value.getName()); - assertEquals(expected.getAcl(), value.getAcl()); - assertEquals(expected.getEtag(), value.getEtag()); - assertEquals(expected.getGeneratedId(), value.getGeneratedId()); - assertEquals(expected.getMetageneration(), value.getMetageneration()); - assertEquals(expected.getOwner(), value.getOwner()); - assertEquals(expected.getSelfLink(), value.getSelfLink()); - assertEquals(expected.getCreateTimeOffsetDateTime(), value.getCreateTimeOffsetDateTime()); - assertEquals(expected.getUpdateTimeOffsetDateTime(), value.getUpdateTimeOffsetDateTime()); - assertEquals(expected.getCors(), value.getCors()); - assertEquals(expected.getDefaultAcl(), value.getDefaultAcl()); - assertEquals(expected.getDeleteRules(), value.getDeleteRules()); - assertEquals(expected.getLifecycleRules(), value.getLifecycleRules()); - assertEquals(expected.getIndexPage(), value.getIndexPage()); - assertEquals(expected.getIamConfiguration(), value.getIamConfiguration()); - assertEquals(expected.getNotFoundPage(), value.getNotFoundPage()); - assertEquals(expected.getLocation(), value.getLocation()); - assertEquals(expected.getStorageClass(), value.getStorageClass()); - assertEquals(expected.getDefaultKmsKeyName(), value.getDefaultKmsKeyName()); - assertEquals(expected.versioningEnabled(), value.versioningEnabled()); - assertEquals(expected.getLabels(), value.getLabels()); - assertEquals(expected.requesterPays(), value.requesterPays()); - assertEquals(expected.getDefaultEventBasedHold(), value.getDefaultEventBasedHold()); - assertEquals( - expected.getRetentionEffectiveTimeOffsetDateTime(), - value.getRetentionEffectiveTimeOffsetDateTime()); - assertEquals(expected.getRetentionPeriodDuration(), value.getRetentionPeriodDuration()); - assertEquals(expected.retentionPolicyIsLocked(), value.retentionPolicyIsLocked()); - assertEquals(expected.getLogging(), value.getLogging()); - assertEquals(expected, value); + private void compareBuckets(BucketInfo expected, BucketInfo value) throws Exception { + assertAll( + () -> assertEquals(expected.getName(), value.getName()), + () -> assertEquals(expected.getAcl(), value.getAcl()), + () -> assertEquals(expected.getEtag(), value.getEtag()), + () -> assertEquals(expected.getGeneratedId(), value.getGeneratedId()), + () -> assertEquals(expected.getMetageneration(), value.getMetageneration()), + () -> assertEquals(expected.getOwner(), value.getOwner()), + () -> assertEquals(expected.getSelfLink(), value.getSelfLink()), + () -> + assertEquals( + expected.getCreateTimeOffsetDateTime(), value.getCreateTimeOffsetDateTime()), + () -> + assertEquals( + expected.getUpdateTimeOffsetDateTime(), value.getUpdateTimeOffsetDateTime()), + () -> assertEquals(expected.getCors(), value.getCors()), + () -> assertEquals(expected.getDefaultAcl(), value.getDefaultAcl()), + () -> assertEquals(expected.getDeleteRules(), value.getDeleteRules()), + () -> assertEquals(expected.getLifecycleRules(), value.getLifecycleRules()), + () -> assertEquals(expected.getIndexPage(), value.getIndexPage()), + () -> assertEquals(expected.getIamConfiguration(), value.getIamConfiguration()), + () -> assertEquals(expected.getNotFoundPage(), value.getNotFoundPage()), + () -> assertEquals(expected.getLocation(), value.getLocation()), + () -> assertEquals(expected.getStorageClass(), value.getStorageClass()), + () -> assertEquals(expected.getDefaultKmsKeyName(), value.getDefaultKmsKeyName()), + () -> assertEquals(expected.versioningEnabled(), value.versioningEnabled()), + () -> assertEquals(expected.getLabels(), value.getLabels()), + () -> assertEquals(expected.requesterPays(), value.requesterPays()), + () -> assertEquals(expected.getDefaultEventBasedHold(), value.getDefaultEventBasedHold()), + () -> + assertEquals( + expected.getRetentionEffectiveTimeOffsetDateTime(), + value.getRetentionEffectiveTimeOffsetDateTime()), + () -> + assertEquals(expected.getRetentionPeriodDuration(), value.getRetentionPeriodDuration()), + () -> assertEquals(expected.retentionPolicyIsLocked(), value.retentionPolicyIsLocked()), + () -> assertEquals(expected.getLogging(), value.getLogging()), + () -> assertEquals(expected, value)); } @Test diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java index f4e497be0e..4316d5b0ac 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java @@ -349,62 +349,6 @@ private void initializeServiceDependentObjects() { expectedBucket3 = new Bucket(storage, new BucketInfo.BuilderImpl(BUCKET_INFO3)); } - @Test - public void testUpdateBucket() { - BucketInfo updatedBucketInfo = BUCKET_INFO1.toBuilder().setIndexPage("some-page").build(); - EasyMock.expect( - storageRpcMock.patch( - Conversions.apiary().bucketInfo().encode(updatedBucketInfo), EMPTY_RPC_OPTIONS)) - .andReturn(Conversions.apiary().bucketInfo().encode(updatedBucketInfo)); - EasyMock.replay(storageRpcMock); - initializeService(); - Bucket bucket = storage.update(updatedBucketInfo); - assertEquals(new Bucket(storage, new BucketInfo.BuilderImpl(updatedBucketInfo)), bucket); - } - - @Test - public void testUpdateBucketWithOptions() { - BucketInfo updatedBucketInfo = BUCKET_INFO1.toBuilder().setIndexPage("some-page").build(); - EasyMock.expect( - storageRpcMock.patch( - Conversions.apiary().bucketInfo().encode(updatedBucketInfo), BUCKET_TARGET_OPTIONS)) - .andReturn(Conversions.apiary().bucketInfo().encode(updatedBucketInfo)); - EasyMock.replay(storageRpcMock); - initializeService(); - Bucket bucket = - storage.update( - updatedBucketInfo, BUCKET_TARGET_METAGENERATION, BUCKET_TARGET_PREDEFINED_ACL); - assertEquals(new Bucket(storage, new BucketInfo.BuilderImpl(updatedBucketInfo)), bucket); - } - - @Test - public void testUpdateBlob() { - BlobInfo updatedBlobInfo = BLOB_INFO1.toBuilder().setContentType("some-content-type").build(); - EasyMock.expect( - storageRpcMock.patch( - Conversions.apiary().blobInfo().encode(updatedBlobInfo), EMPTY_RPC_OPTIONS)) - .andReturn(Conversions.apiary().blobInfo().encode(updatedBlobInfo)); - EasyMock.replay(storageRpcMock); - initializeService(); - Blob blob = storage.update(updatedBlobInfo); - assertEquals(new Blob(storage, new BlobInfo.BuilderImpl(updatedBlobInfo)), blob); - } - - @Test - public void testUpdateBlobWithOptions() { - BlobInfo updatedBlobInfo = BLOB_INFO1.toBuilder().setContentType("some-content-type").build(); - EasyMock.expect( - storageRpcMock.patch( - Conversions.apiary().blobInfo().encode(updatedBlobInfo), - BLOB_TARGET_OPTIONS_UPDATE)) - .andReturn(Conversions.apiary().blobInfo().encode(updatedBlobInfo)); - EasyMock.replay(storageRpcMock); - initializeService(); - Blob blob = - storage.update(updatedBlobInfo, BLOB_TARGET_METAGENERATION, BLOB_TARGET_PREDEFINED_ACL); - assertEquals(new Blob(storage, new BlobInfo.BuilderImpl(updatedBlobInfo)), blob); - } - @Test public void testDeleteBucket() { EasyMock.expect( diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/TestUtils.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/TestUtils.java index 03b287b36a..0ce0b4c31e 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/TestUtils.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/TestUtils.java @@ -16,6 +16,8 @@ package com.google.cloud.storage; +import static java.util.Objects.requireNonNull; + import com.google.api.core.ApiClock; import com.google.api.core.NanoClock; import com.google.api.gax.grpc.GrpcCallContext; @@ -48,7 +50,10 @@ import java.nio.Buffer; import java.nio.ByteBuffer; import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.concurrent.Callable; import java.util.function.Function; @@ -239,4 +244,24 @@ public static void assertAll(ThrowingRunnable... trs) throws Exception { .collect(ImmutableList.toImmutableList()); MultipleFailureException.assertEmpty(x); } + + /** ImmutableMap does not allow null values, this method does */ + public static Map<@NonNull String, @Nullable String> hashMapOf( + @NonNull String k1, @Nullable String v1) { + requireNonNull(k1, "k1 must be non null"); + HashMap map = new HashMap<>(); + map.put(k1, v1); + return Collections.unmodifiableMap(map); + } + + /** ImmutableMap does not allow null values, this method does */ + public static Map<@NonNull String, @Nullable String> hashMapOf( + @NonNull String k1, @Nullable String v1, @NonNull String k2, @Nullable String v2) { + requireNonNull(k1, "k1 must be non null"); + requireNonNull(k2, "k2 must be non null"); + HashMap map = new HashMap<>(); + map.put(k1, v1); + map.put(k2, v2); + return Collections.unmodifiableMap(map); + } } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/UpdateMaskTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/UpdateMaskTest.java index 891730466b..6d343f2ea2 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/UpdateMaskTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/UpdateMaskTest.java @@ -30,6 +30,7 @@ import com.google.cloud.storage.BucketInfo.Logging; import com.google.cloud.storage.Storage.BlobField; import com.google.cloud.storage.Storage.BucketField; +import com.google.cloud.storage.UnifiedOpts.NamedField; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -64,7 +65,7 @@ public void updateObjectRequest() throws Exception { UpdateObjectRequest expected = UpdateObjectRequest.newBuilder() .setObject(expectedObject) - .setUpdateMask(FieldMask.newBuilder().addPaths("metadata").build()) + .setUpdateMask(FieldMask.newBuilder().addPaths("metadata.x").build()) .build(); AtomicReference actualRequest = new AtomicReference<>(); @@ -95,7 +96,9 @@ public void updateObject(UpdateObjectRequest request, StreamObserver obs @Test public void blobInfo_field_metadata() { - testBlobField(b -> b.setMetadata(ImmutableMap.of("x", "X")), BlobField.METADATA); + testBlobField( + b -> b.setMetadata(ImmutableMap.of("x", "X")), + NamedField.nested(BlobField.METADATA, NamedField.literal("x"))); } @Test @@ -239,7 +242,7 @@ public void blobInfo_field_blobId_changeGeneration() { } private static void testBlobField( - UnaryOperator f, BlobField... expectedModified) { + UnaryOperator f, NamedField... expectedModified) { BlobInfo actual1 = f.apply(base().toBuilder()).build(); assertThat(actual1.getModifiedFields()).isEqualTo(ImmutableSet.copyOf(expectedModified)); // verify that nothing is carried through from a previous state, and that setting the same @@ -262,7 +265,7 @@ public void updateBucketRequest() throws Exception { UpdateBucketRequest expected = UpdateBucketRequest.newBuilder() .setBucket(expectedBucket) - .setUpdateMask(FieldMask.newBuilder().addPaths("labels").build()) + .setUpdateMask(FieldMask.newBuilder().addPaths("labels.x").build()) .build(); AtomicReference actualRequest = new AtomicReference<>(); @@ -381,7 +384,9 @@ public void bucketInfo_field_setDefaultAcl() { @Test public void bucketInfo_field_setLabels() { - testBucketField(b -> b.setLabels(ImmutableMap.of("x", "X")), BucketField.LABELS); + testBucketField( + b -> b.setLabels(ImmutableMap.of("x", "X")), + NamedField.nested(BucketField.LABELS, NamedField.literal("x"))); } @Test @@ -445,7 +450,7 @@ public void bucketInfo_field_setLocationType() { } private static void testBucketField( - UnaryOperator f, BucketField... expectedModified) { + UnaryOperator f, NamedField... expectedModified) { BucketInfo actual1 = f.apply(base().toBuilder()).build(); assertThat(actual1.getModifiedFields()).isEqualTo(ImmutableSet.copyOf(expectedModified)); // verify that nothing is carried through from a previous state, and that setting the same diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITNestedUpdateMaskTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITNestedUpdateMaskTest.java new file mode 100644 index 0000000000..74e67e5c07 --- /dev/null +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITNestedUpdateMaskTest.java @@ -0,0 +1,195 @@ +/* + * Copyright 2022 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.storage.it; + +import static com.google.cloud.storage.TestUtils.hashMapOf; +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; +import static java.util.Objects.requireNonNull; + +import com.google.cloud.storage.Blob; +import com.google.cloud.storage.BlobInfo; +import com.google.cloud.storage.Bucket; +import com.google.cloud.storage.BucketInfo; +import com.google.cloud.storage.Storage; +import com.google.cloud.storage.Storage.BlobTargetOption; +import com.google.cloud.storage.Storage.BucketTargetOption; +import com.google.cloud.storage.TransportCompatibility.Transport; +import com.google.cloud.storage.it.ITNestedUpdateMaskTest.NestedUpdateMaskParametersProvider; +import com.google.cloud.storage.it.runner.StorageITRunner; +import com.google.cloud.storage.it.runner.annotations.Backend; +import com.google.cloud.storage.it.runner.annotations.CrossRun; +import com.google.cloud.storage.it.runner.annotations.Inject; +import com.google.cloud.storage.it.runner.annotations.ParallelFriendly; +import com.google.cloud.storage.it.runner.annotations.Parameterized; +import com.google.cloud.storage.it.runner.annotations.Parameterized.Parameter; +import com.google.cloud.storage.it.runner.annotations.Parameterized.ParametersProvider; +import com.google.cloud.storage.it.runner.registry.Generator; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import java.util.Map; +import org.checkerframework.checker.nullness.qual.NonNull; +import org.checkerframework.checker.nullness.qual.Nullable; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** + * A set of tests to specifically test scenarios related to update handling of {@link + * BlobInfo#getMetadata()} and {@link BucketInfo#getLabels()} and the various permutations which can + * be used to add and remove keys. + */ +@RunWith(StorageITRunner.class) +@CrossRun( + backends = Backend.PROD, + transports = {Transport.HTTP, Transport.GRPC}) +@Parameterized(NestedUpdateMaskParametersProvider.class) +@ParallelFriendly +public final class ITNestedUpdateMaskTest { + + @Inject public Generator generator; + + @Inject public Storage storage; + + @Inject public BucketInfo bucket; + + @Parameter public Param param; + + public static final class NestedUpdateMaskParametersProvider implements ParametersProvider { + private static final Map empty = ImmutableMap.of(); + private static final Map k1a = ImmutableMap.of("k1", "a"); + private static final Map k2b = ImmutableMap.of("k2", "b"); + private static final Map k1z = ImmutableMap.of("k1", "z"); + private static final Map k1a_k2b = ImmutableMap.of("k1", "a", "k2", "b"); + private static final Map k1z_k2b = ImmutableMap.of("k1", "z", "k2", "b"); + private static final Map k1a_k2null = hashMapOf("k1", "a", "k2", null); + private static final Map k1null = hashMapOf("k1", null); + private static final Map k2null = hashMapOf("k2", null); + + @Override + public ImmutableList parameters() { + return ImmutableList.of( + new Param(UpdateMethod.SET, "null to 1", null, k1a, k1a), + new Param(UpdateMethod.ADD, "null to 1", null, k1a, k1a), + new Param(UpdateMethod.SET, "empty to 1", empty, k1a, k1a), + new Param(UpdateMethod.ADD, "empty to 1", empty, k1a, k1a), + new Param(UpdateMethod.SET, "1 to 2", k1a, k1a_k2b, k1a_k2b), + new Param(UpdateMethod.ADD, "1 to 2", k1a, k2b, k1a_k2b), + new Param(UpdateMethod.SET, "2 keys, modify 1 value", k1a_k2b, k1z_k2b, k1z_k2b), + new Param(UpdateMethod.ADD, "2 keys, modify 1 value", k1a_k2b, k1z, k1z_k2b), + new Param(UpdateMethod.SET, "2 keys, modify 1 null", k1a_k2b, k1a_k2null, k1a), + new Param(UpdateMethod.ADD, "2 keys, modify 1 null", k1a_k2b, k2null, k1a), + new Param(UpdateMethod.SET, "1 key, set empty", k1a, empty, null), + new Param(UpdateMethod.ADD, "1 key, null key", k1a, k1null, null), + new Param(UpdateMethod.SET, "2 keys, set null", k1a_k2b, null, null)); + } + } + + @Test + public void testBucketLabels() throws Exception { + BucketInfo bucket = newBucketInfo(param.initial); + try (TemporaryBucket tempB = + TemporaryBucket.newBuilder().setBucketInfo(bucket).setStorage(storage).build()) { + BucketInfo gen1 = tempB.getBucket(); + + BucketInfo.Builder b = gen1.toBuilder(); + switch (param.updateMethod) { + case SET: + b.setLabels(param.update); + break; + case ADD: + assertThat(param.update).isNotNull(); + b.addAllLabels(param.update); + break; + } + BucketInfo modified = b.build(); + Bucket gen2 = storage.update(modified, BucketTargetOption.metagenerationMatch()); + assertThat(gen2.getLabels()).isEqualTo(param.expected); + } + } + + @Test + public void testBlobMetadata() { + BlobInfo blob = newBlobInfo(param.initial); + Blob gen1 = storage.create(blob, BlobTargetOption.doesNotExist()); + BlobInfo.Builder b = gen1.toBuilder(); + switch (param.updateMethod) { + case SET: + b.setMetadata(param.update); + break; + case ADD: + assertThat(param.update).isNotNull(); + b.addAllMetadata(param.update); + break; + } + BlobInfo modified = b.build(); + Blob gen2 = storage.update(modified, BlobTargetOption.metagenerationMatch()); + assertThat(gen2.getMetadata()).isEqualTo(param.expected); + } + + private BlobInfo newBlobInfo(Map metadata) { + String blobName = generator.randomObjectName(); + BlobInfo.Builder builder = BlobInfo.newBuilder(bucket, blobName); + if (metadata != null) { + builder.setMetadata(metadata); + } + return builder.build(); + } + + private BucketInfo newBucketInfo(Map metadata) { + BucketInfo.Builder builder = BucketInfo.newBuilder(generator.randomBucketName()); + if (metadata != null) { + builder.setLabels(metadata); + } + return builder.build(); + } + + private enum UpdateMethod { + SET, + ADD + } + + private static final class Param { + private final UpdateMethod updateMethod; + private final String description; + @Nullable private final Map<@NonNull String, @Nullable String> initial; + @Nullable private final Map<@NonNull String, @Nullable String> update; + @Nullable private final Map<@NonNull String, @Nullable String> expected; + + private Param( + UpdateMethod updateMethod, + String description, + @Nullable Map<@NonNull String, @Nullable String> initial, + @Nullable Map<@NonNull String, @Nullable String> update, + @Nullable Map<@NonNull String, @Nullable String> expected) { + requireNonNull(description, "description must be non null"); + requireNonNull(updateMethod, "updateMethod must be non null"); + assertWithMessage("Specifying null update with ADD invalid") + .that(updateMethod == UpdateMethod.ADD && update == null) + .isFalse(); + this.description = description; + this.updateMethod = updateMethod; + this.initial = initial; + this.update = update; + this.expected = expected; + } + + @Override + public String toString() { + return String.format("%s via %s", description, updateMethod); + } + } +} 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 206382f24f..4968f7e13e 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 @@ -638,7 +638,7 @@ public void testUpdateBlobMergeMetadata() { .build(); Blob remoteBlob = storage.create(blob); assertNotNull(remoteBlob); - Blob updatedBlob = remoteBlob.toBuilder().setMetadata(newMetadata).build().update(); + Blob updatedBlob = remoteBlob.toBuilder().addAllMetadata(newMetadata).build().update(); assertNotNull(updatedBlob); assertEquals(blob.getName(), updatedBlob.getName()); assertEquals(blob.getBucket(), updatedBlob.getBucket()); @@ -646,8 +646,6 @@ public void testUpdateBlobMergeMetadata() { } @Test - // Metadata update bug b/230510191 - @Exclude(transports = Transport.GRPC) public void testUpdateBlobUnsetMetadata() { String blobName = "test-update-blob-unset-metadata"; @@ -663,7 +661,7 @@ public void testUpdateBlobUnsetMetadata() { .build(); Blob remoteBlob = storage.create(blob); assertNotNull(remoteBlob); - Blob updatedBlob = remoteBlob.toBuilder().setMetadata(newMetadata).build().update(); + Blob updatedBlob = remoteBlob.toBuilder().addAllMetadata(newMetadata).build().update(); assertNotNull(updatedBlob); assertEquals(blob.getName(), updatedBlob.getName()); assertEquals(blob.getBucket(), updatedBlob.getBucket()); diff --git a/samples/snippets/src/main/java/com/example/storage/bucket/AddBucketLabel.java b/samples/snippets/src/main/java/com/example/storage/bucket/AddBucketLabel.java index 473f270215..9684f01b4d 100644 --- a/samples/snippets/src/main/java/com/example/storage/bucket/AddBucketLabel.java +++ b/samples/snippets/src/main/java/com/example/storage/bucket/AddBucketLabel.java @@ -44,11 +44,7 @@ public static void addBucketLabel( Storage storage = StorageOptions.newBuilder().setProjectId(projectId).build().getService(); Bucket bucket = storage.get(bucketName); - Map labels = bucket.getLabels(); - if (labels != null) { - newLabels.putAll(labels); - } - bucket.toBuilder().setLabels(newLabels).build().update(); + bucket.toBuilder().addAllLabels(newLabels).build().update(); System.out.println( "Added label " + labelKey + " with value " + labelValue + " to bucket " + bucketName + "."); diff --git a/samples/snippets/src/main/java/com/example/storage/bucket/RemoveBucketLabel.java b/samples/snippets/src/main/java/com/example/storage/bucket/RemoveBucketLabel.java index 1e63ba76d3..64291f2b6f 100644 --- a/samples/snippets/src/main/java/com/example/storage/bucket/RemoveBucketLabel.java +++ b/samples/snippets/src/main/java/com/example/storage/bucket/RemoveBucketLabel.java @@ -41,14 +41,7 @@ public static void removeBucketLabel(String projectId, String bucketName, String labelsToRemove.put(labelKey, null); Bucket bucket = storage.get(bucketName); - Map labels; - if (bucket.getLabels() == null) { - labels = new HashMap<>(); - } else { - labels = new HashMap(bucket.getLabels()); - } - labels.putAll(labelsToRemove); - bucket.toBuilder().setLabels(labels).build().update(); + bucket.toBuilder().addAllLabels(labelsToRemove).build().update(); System.out.println("Removed label " + labelKey + " from bucket " + bucketName); } diff --git a/samples/snippets/src/main/java/com/example/storage/object/BatchSetObjectMetadata.java b/samples/snippets/src/main/java/com/example/storage/object/BatchSetObjectMetadata.java index d7db73597b..ce903da6cc 100644 --- a/samples/snippets/src/main/java/com/example/storage/object/BatchSetObjectMetadata.java +++ b/samples/snippets/src/main/java/com/example/storage/object/BatchSetObjectMetadata.java @@ -50,7 +50,7 @@ public static void batchSetObjectMetadata( // Add all blobs with the given prefix to the batch request for (Blob blob : blobs.iterateAll()) { - batchRequest.update(blob.toBuilder().setMetadata(newMetadata).build()); + batchRequest.update(blob.toBuilder().addAllMetadata(newMetadata).build()); } // Execute the batch request diff --git a/samples/snippets/src/main/java/com/example/storage/object/SetObjectMetadata.java b/samples/snippets/src/main/java/com/example/storage/object/SetObjectMetadata.java index 5fde73f384..4c61b897ec 100644 --- a/samples/snippets/src/main/java/com/example/storage/object/SetObjectMetadata.java +++ b/samples/snippets/src/main/java/com/example/storage/object/SetObjectMetadata.java @@ -53,7 +53,7 @@ public static void setObjectMetadata(String projectId, String bucketName, String // Does an upsert operation, if the key already exists it's replaced by the new value, otherwise // it's added. - blob.toBuilder().setMetadata(newMetadata).build().update(precondition); + blob.toBuilder().addAllMetadata(newMetadata).build().update(precondition); System.out.println( "Updated custom metadata for object " + objectName + " in bucket " + bucketName);