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 a8b871867..9249e5853 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 @@ -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 @@ -373,7 +374,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); maybeEncodeRetentionPolicy(from, to); ifNonNull(from.getIamConfiguration(), this::iamConfigEncode, to::setIamConfiguration); ifNonNull(from.getAutoclass(), this::autoclassEncode, to::setAutoclass); @@ -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 @@ -917,4 +925,21 @@ private static void maybeDecodeRetentionPolicy(Bucket from, BucketInfo.Builder t to::setRetentionPeriodDuration); } } + + 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/BlobInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BlobInfo.java index 95716f894..d33f3140d 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,6 +17,7 @@ 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; @@ -24,6 +25,7 @@ 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 +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 @@ -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 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 { @@ -331,7 +335,7 @@ public Builder setTimeStorageClassUpdatedOffsetDateTime( } /** Sets the blob's user provided metadata. */ - public abstract Builder setMetadata(Map metadata); + public abstract Builder setMetadata(@Nullable Map<@NonNull String, @Nullable String> metadata); abstract Builder setMetageneration(Long metageneration); @@ -502,7 +506,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; @@ -757,15 +761,18 @@ Builder setMediaLink(String mediaLink) { return this; } + @SuppressWarnings({"UnnecessaryLocalVariable"}) @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); + 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 +1324,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 +1625,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 6147552c0..af5c2cc25 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,7 @@ public Builder setDefaultAcl(Iterable acl) { } @Override - public Builder setLabels(Map labels) { + 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 2a2c7256a..d328fa9f2 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,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; @@ -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; @@ -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; @@ -100,7 +103,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 +114,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 @@ -1477,7 +1480,7 @@ Builder setUpdateTimeOffsetDateTime(OffsetDateTime updateTime) { public abstract Builder setDefaultAcl(Iterable acl); /** Sets the label of this bucket. */ - public abstract Builder setLabels(Map 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); @@ -1630,7 +1633,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; @@ -1909,20 +1912,18 @@ public Builder setDefaultAcl(Iterable acl) { return this; } + @SuppressWarnings("UnnecessaryLocalVariable") @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 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 +2484,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 +2694,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 f2923a064..57f2edf64 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 @@ -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 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 97389f926..b15f62bd1 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; @@ -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( @@ -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( 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 6eecc74fd..6e78001b2 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 9a3785569..b067e303e 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; @@ -34,10 +37,12 @@ import java.time.format.DateTimeFormatter; import java.time.temporal.ChronoUnit; 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 +239,39 @@ 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(), + // Only include keys to remove if ALL keys were removed + right.isEmpty() + ? difference.entriesOnlyOnLeft().keySet().stream() + : Stream.empty(), + // 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); + } + 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 4cb1ac545..a11d989bf 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/TestUtils.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/TestUtils.java index 03b287b36..0ce0b4c31 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 891730466..6d343f2ea 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 000000000..8e3b944d9 --- /dev/null +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITNestedUpdateMaskTest.java @@ -0,0 +1,176 @@ +/* + * 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 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.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) +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); + + /** + * + * + *
+     * | base                | update               | expected            |
+     * |---------------------|----------------------|---------------------|
+     * | null                | {"k1":"a"}           | {"k1":"a"}          |
+     * | {}                  | {"k1":"a"}           | {"k1":"a"}          |
+     * | {"k1":"a"}          | {"k1":"a","k2":"b"}  | {"k1":"a","k2":"b"} |
+     * | {"k1":"a"}          | {"k2":"b"}           | {"k1":"a","k2":"b"} |
+     * | {"k1":"a","k2":"b"} | {"k1":"z","k2":"b"}  | {"k1":"z","k2":"b"} |
+     * | {"k1":"a","k2":"b"} | {"k1":"z"}           | {"k1":"z","k2":"b"} |
+     * | {"k1":"a","k2":"b"} | {"k1":"a","k2":null} | {"k1":"a"}          |
+     * | {"k1":"a","k2":"b"} | {"k2":null}          | {"k1":"a"}          |
+     * | {"k1":"a"}          | {}                   | null                |
+     * | {"k1":"a"}          | {"k1":null}          | null                |
+     * | {"k1":"a","k2":"b"} | null                 | null                |
+     * 
+ */ + @Override + public ImmutableList parameters() { + return ImmutableList.of( + new Param("null to 1", null, k1a, k1a), + new Param("empty to 1", empty, k1a, k1a), + new Param("1 to 2 set", k1a, k1a_k2b, k1a_k2b), + new Param("1 to 2 add", k1a, k2b, k1a_k2b), + new Param("2 keys, modify 1 value (full)", k1a_k2b, k1z_k2b, k1z_k2b), + new Param("2 keys, modify 1 value (fine)", k1a_k2b, k1z, k1z_k2b), + new Param("2 keys, modify 1 null (full)", k1a_k2b, k1a_k2null, k1a), + new Param("2 keys, modify 1 null (fine)", k1a_k2b, k2null, k1a), + new Param("1 key, set empty", k1a, empty, null), + new Param("1 key, null key", k1a, k1null, null), + new Param("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 modified = gen1.toBuilder().setLabels(param.update).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 modified = gen1.toBuilder().setMetadata(param.update).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 static final class Param { + 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( + 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"); + this.description = description; + this.initial = initial; + this.update = update; + this.expected = expected; + } + + @Override + public String toString() { + return description; + } + } +} 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 5a0fa76c8..f2afbb2db 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 @@ -625,8 +625,6 @@ public void testUpdateBlobMergeMetadata() { } @Test - // Metadata update bug b/230510191 - @Exclude(transports = Transport.GRPC) public void testUpdateBlobUnsetMetadata() { String blobName = "test-update-blob-unset-metadata";