Repository: googleapis/java-storage
Pull request #1798 (2022-12-05 23:58:51)

fix: update GrpcConversions to use Bucket.RetentionPolicy.retention_duration


Patch:
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 d5ca3b59..a8b87186 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
@@ -73,7 +73,6 @@ import com.google.cloud.storage.NotificationInfo.PayloadFormat;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Maps;
 import java.math.BigInteger;
-import java.time.Duration;
 import java.time.OffsetDateTime;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -375,19 +374,7 @@ final class ApiaryConversions {
         k -> new Encryption().setDefaultKmsKeyName(k),
         to::setEncryption);
     ifNonNull(from.getLabels(), to::setLabels);
-    Duration retentionPeriod = from.getRetentionPeriodDuration();
-    if (retentionPeriod == null) {
-      to.setRetentionPolicy(Data.nullOf(Bucket.RetentionPolicy.class));
-    } else {
-      Bucket.RetentionPolicy retentionPolicy = new Bucket.RetentionPolicy();
-      retentionPolicy.setRetentionPeriod(durationSecondsCodec.encode(retentionPeriod));
-      ifNonNull(
-          from.getRetentionEffectiveTimeOffsetDateTime(),
-          dateTimeCodec::encode,
-          retentionPolicy::setEffectiveTime);
-      ifNonNull(from.retentionPolicyIsLocked(), retentionPolicy::setIsLocked);
-      to.setRetentionPolicy(retentionPolicy);
-    }
+    maybeEncodeRetentionPolicy(from, to);
     ifNonNull(from.getIamConfiguration(), this::iamConfigEncode, to::setIamConfiguration);
     ifNonNull(from.getAutoclass(), this::autoclassEncode, to::setAutoclass);
     ifNonNull(from.getLogging(), this::loggingEncode, to::setLogging);
@@ -434,13 +421,7 @@ final class ApiaryConversions {
       to.setDefaultKmsKeyName(encryption.getDefaultKmsKeyName());
     }
 
-    RetentionPolicy retentionPolicy = from.getRetentionPolicy();
-    if (retentionPolicy != null && retentionPolicy.getEffectiveTime() != null) {
-      to.setRetentionEffectiveTimeOffsetDateTime(
-          dateTimeCodec.decode(retentionPolicy.getEffectiveTime()));
-    }
-    ifNonNull(retentionPolicy, RetentionPolicy::getIsLocked, to::setRetentionPolicyIsLocked);
-    ifNonNull(retentionPolicy, RetentionPolicy::getRetentionPeriod, to::setRetentionPeriod);
+    maybeDecodeRetentionPolicy(from, to);
     ifNonNull(from.getIamConfiguration(), this::iamConfigDecode, to::setIamConfiguration);
     ifNonNull(from.getAutoclass(), this::autoclassDecode, to::setAutoclass);
     ifNonNull(from.getLogging(), this::loggingDecode, to::setLogging);
@@ -901,4 +882,39 @@ final class ApiaryConversions {
   private CustomPlacementConfig customPlacementConfigDecode(Bucket.CustomPlacementConfig from) {
     return CustomPlacementConfig.newBuilder().setDataLocations(from.getDataLocations()).build();
   }
+
+  private static void maybeEncodeRetentionPolicy(BucketInfo from, Bucket to) {
+    if (from.getRetentionPeriodDuration() != null
+        || from.retentionPolicyIsLocked() != null
+        || from.getRetentionEffectiveTimeOffsetDateTime() != null) {
+      RetentionPolicy retentionPolicy = new RetentionPolicy();
+      ifNonNull(
+          from.getRetentionPeriodDuration(),
+          durationSecondsCodec::encode,
+          retentionPolicy::setRetentionPeriod);
+      ifNonNull(from.retentionPolicyIsLocked(), retentionPolicy::setIsLocked);
+      ifNonNull(
+          from.getRetentionEffectiveTimeOffsetDateTime(),
+          dateTimeCodec::encode,
+          retentionPolicy::setEffectiveTime);
+      to.setRetentionPolicy(retentionPolicy);
+    } else {
+      to.setRetentionPolicy(Data.nullOf(Bucket.RetentionPolicy.class));
+    }
+  }
+
+  private static void maybeDecodeRetentionPolicy(Bucket from, BucketInfo.Builder to) {
+    RetentionPolicy retentionPolicy = from.getRetentionPolicy();
+    if (retentionPolicy != null && retentionPolicy.getEffectiveTime() != null) {
+      to.setRetentionEffectiveTimeOffsetDateTime(
+          dateTimeCodec.decode(retentionPolicy.getEffectiveTime()));
+    }
+    if (retentionPolicy != null) {
+      ifNonNull(retentionPolicy.getIsLocked(), to::setRetentionPolicyIsLocked);
+      ifNonNull(
+          retentionPolicy.getRetentionPeriod(),
+          durationSecondsCodec::decode,
+          to::setRetentionPeriodDuration);
+    }
+  }
 }
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 3a29f16d..7e5e8c7e 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
@@ -17,7 +17,6 @@
 package com.google.cloud.storage;
 
 import static com.google.cloud.storage.Utils.bucketNameCodec;
-import static com.google.cloud.storage.Utils.durationSecondsCodec;
 import static com.google.cloud.storage.Utils.ifNonNull;
 import static com.google.cloud.storage.Utils.lift;
 import static com.google.cloud.storage.Utils.projectNameCodec;
@@ -56,6 +55,7 @@ import com.google.storage.v2.ObjectChecksums;
 import com.google.storage.v2.Owner;
 import com.google.type.Date;
 import com.google.type.Expr;
+import java.time.Duration;
 import java.time.Instant;
 import java.time.LocalDate;
 import java.time.OffsetDateTime;
@@ -118,6 +118,17 @@ final class GrpcConversions {
                   .plusNanos(t.getNanos())
                   .atOffset(ZoneOffset.UTC));
 
+  @VisibleForTesting
+  final Codec<Duration, com.google.protobuf.Duration> durationCodec =
+      Codec.of(
+          javaDuration ->
+              com.google.protobuf.Duration.newBuilder()
+                  .setSeconds(javaDuration.getSeconds())
+                  .setNanos(javaDuration.getNano())
+                  .build(),
+          protoDuration ->
+              Duration.ofSeconds(protoDuration.getSeconds()).plusNanos(protoDuration.getNanos()));
+
   @VisibleForTesting
   final Codec<OffsetDateTime, Date> odtDateCodec =
       Codec.of(
@@ -200,18 +211,7 @@ final class GrpcConversions {
     BucketInfo.Builder to = new BucketInfo.BuilderImpl(bucketNameCodec.decode(from.getName()));
     to.setProject(from.getProject());
     to.setGeneratedId(from.getBucketId());
-    if (from.hasRetentionPolicy()) {
-      Bucket.RetentionPolicy retentionPolicy = from.getRetentionPolicy();
-      to.setRetentionPolicyIsLocked(retentionPolicy.getIsLocked());
-      if (retentionPolicy.hasRetentionPeriod()) {
-        to.setRetentionPeriodDuration(
-            durationSecondsCodec.decode(retentionPolicy.getRetentionPeriod()));
-      }
-      if (retentionPolicy.hasEffectiveTime()) {
-        to.setRetentionEffectiveTimeOffsetDateTime(
-            timestampCodec.decode(retentionPolicy.getEffectiveTime()));
-      }
-    }
+    maybeDecodeRetentionPolicy(from, to);
     ifNonNull(from.getLocation(), to::setLocation);
     ifNonNull(from.getLocationType(), to::setLocationType);
     ifNonNull(from.getMetageneration(), to::setMetageneration);
@@ -303,21 +303,7 @@ final class GrpcConversions {
     Bucket.Builder to = Bucket.newBuilder();
     to.setName(bucketNameCodec.encode(from.getName()));
     ifNonNull(from.getGeneratedId(), to::setBucketId);
-    if (from.getRetentionPeriodDuration() != null) {
-      Bucket.RetentionPolicy.Builder retentionPolicyBuilder = to.getRetentionPolicyBuilder();
-      ifNonNull(
-          from.getRetentionPeriodDuration(),
-          durationSecondsCodec::encode,
-          retentionPolicyBuilder::setRetentionPeriod);
-      ifNonNull(from.retentionPolicyIsLocked(), retentionPolicyBuilder::setIsLocked);
-      if (from.retentionPolicyIsLocked() == Boolean.TRUE) {
-        ifNonNull(
-            from.getRetentionEffectiveTimeOffsetDateTime(),
-            timestampCodec::encode,
-            retentionPolicyBuilder::setEffectiveTime);
-      }
-      to.setRetentionPolicy(retentionPolicyBuilder.build());
-    }
+    maybeEncodeRetentionPolicy(from, to);
     ifNonNull(from.getLocation(), to::setLocation);
     ifNonNull(from.getLocationType(), to::setLocationType);
     ifNonNull(from.getMetageneration(), to::setMetageneration);
@@ -395,6 +381,38 @@ final class GrpcConversions {
     return to.build();
   }
 
+  private void maybeEncodeRetentionPolicy(BucketInfo from, Bucket.Builder to) {
+    if (from.getRetentionPeriodDuration() != null
+        || from.retentionPolicyIsLocked() != null
+        || from.getRetentionEffectiveTimeOffsetDateTime() != null) {
+      Bucket.RetentionPolicy.Builder retentionPolicyBuilder = to.getRetentionPolicyBuilder();
+      ifNonNull(
+          from.getRetentionPeriodDuration(),
+          durationCodec::encode,
+          retentionPolicyBuilder::setRetentionDuration);
+      ifNonNull(from.retentionPolicyIsLocked(), retentionPolicyBuilder::setIsLocked);
+      ifNonNull(
+          from.getRetentionEffectiveTimeOffsetDateTime(),
+          timestampCodec::encode,
+          retentionPolicyBuilder::setEffectiveTime);
+      to.setRetentionPolicy(retentionPolicyBuilder.build());
+    }
+  }
+
+  private void maybeDecodeRetentionPolicy(Bucket from, BucketInfo.Builder to) {
+    if (from.hasRetentionPolicy()) {
+      Bucket.RetentionPolicy retentionPolicy = from.getRetentionPolicy();
+      to.setRetentionPolicyIsLocked(retentionPolicy.getIsLocked());
+      if (retentionPolicy.hasRetentionDuration()) {
+        to.setRetentionPeriodDuration(durationCodec.decode(retentionPolicy.getRetentionDuration()));
+      }
+      if (retentionPolicy.hasEffectiveTime()) {
+        to.setRetentionEffectiveTimeOffsetDateTime(
+            timestampCodec.decode(retentionPolicy.getEffectiveTime()));
+      }
+    }
+  }
+
   private Bucket.Logging loggingEncode(BucketInfo.Logging from) {
     Bucket.Logging.Builder to = Bucket.Logging.newBuilder();
     if (from.getLogObjectPrefix() != null && !from.getLogObjectPrefix().isEmpty()) {
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java
index d53854cd..27572417 100644
--- a/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java
+++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java
@@ -41,10 +41,12 @@ import com.google.api.client.http.InputStreamContent;
 import com.google.api.client.http.json.JsonHttpContent;
 import com.google.api.client.json.JsonFactory;
 import com.google.api.client.json.jackson2.JacksonFactory;
+import com.google.api.client.util.Data;
 import com.google.api.services.storage.Storage;
 import com.google.api.services.storage.Storage.Objects.Get;
 import com.google.api.services.storage.Storage.Objects.Insert;
 import com.google.api.services.storage.model.Bucket;
+import com.google.api.services.storage.model.Bucket.RetentionPolicy;
 import com.google.api.services.storage.model.BucketAccessControl;
 import com.google.api.services.storage.model.Buckets;
 import com.google.api.services.storage.model.ComposeRequest;
@@ -534,6 +536,18 @@ public class HttpStorageRpc implements StorageRpc {
     Span span = startSpan(HttpStorageRpcSpans.SPAN_NAME_PATCH_BUCKET);
     Scope scope = tracer.withSpan(span);
     try {
+      RetentionPolicy retentionPolicy = bucket.getRetentionPolicy();
+      if (retentionPolicy != null) {
+        // according to https://cloud.google.com/storage/docs/json_api/v1/buckets both effectiveTime
+        // and isLocked are output_only. If retentionPeriod is null, null out the whole
+        // RetentionPolicy.
+        if (retentionPolicy.getRetentionPeriod() == null) {
+          // Using Data.nullOf here is important here so the null value is written into the request
+          // json. The explicit null values tells the backend to remove the policy.
+          bucket.setRetentionPolicy(Data.nullOf(RetentionPolicy.class));
+        }
+      }
+
       String projection = Option.PROJECTION.getString(options);
       if (bucket.getIamConfiguration() != null
           && bucket.getIamConfiguration().getBucketPolicyOnly() != null


Test patch:
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/DurationCodecPropertyTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/DurationCodecPropertyTest.java
new file mode 100644
index 00000000..f65cc966
--- /dev/null
+++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/DurationCodecPropertyTest.java
@@ -0,0 +1,46 @@
+/*
+ * 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;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.cloud.storage.Conversions.Codec;
+import com.google.cloud.storage.jqwik.StorageArbitraries;
+import com.google.protobuf.Duration;
+import net.jqwik.api.Arbitrary;
+import net.jqwik.api.ArbitrarySupplier;
+import net.jqwik.api.ForAll;
+import net.jqwik.api.Property;
+
+final class DurationCodecPropertyTest {
+
+  @Property
+  void timestampCodecShouldRoundTrip(@ForAll(supplier = Supp.class) Duration ts) {
+    Codec<java.time.Duration, Duration> codec = GrpcConversions.INSTANCE.durationCodec;
+    java.time.Duration decode = codec.decode(ts);
+    Duration encode = codec.encode(decode);
+
+    assertThat(encode).isEqualTo(ts);
+  }
+
+  private static final class Supp implements ArbitrarySupplier<Duration> {
+    @Override
+    public Arbitrary<Duration> get() {
+      return StorageArbitraries.duration();
+    }
+  }
+}
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/jqwik/StorageArbitraries.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/jqwik/StorageArbitraries.java
index d6ac7f00..83b2d434 100644
--- a/google-cloud-storage/src/test/java/com/google/cloud/storage/jqwik/StorageArbitraries.java
+++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/jqwik/StorageArbitraries.java
@@ -78,6 +78,12 @@ public final class StorageArbitraries {
                 Timestamp.newBuilder().setSeconds(odt.toEpochSecond()).setNanos(nanos).build());
   }
 
+  public static Arbitrary<com.google.protobuf.Duration> duration() {
+    return Arbitraries.longs()
+        .between(0, 315_576_000_000L)
+        .map(seconds -> com.google.protobuf.Duration.newBuilder().setSeconds(seconds).build());
+  }
+
   public static Arbitrary<Date> date() {
     return DateTimes.offsetDateTimes()
         .offsetBetween(ZoneOffset.UTC, ZoneOffset.UTC)
@@ -384,11 +390,11 @@ public final class StorageArbitraries {
     }
 
     public Arbitrary<Bucket.RetentionPolicy> retentionPolicy() {
-      return Combinators.combine(bool(), Arbitraries.longs().greaterOrEqual(0), timestamp())
+      return Combinators.combine(bool(), duration().injectNull(0.25), timestamp())
           .as(
-              (locked, period, effectiveTime) -> {
+              (locked, duration, effectiveTime) -> {
                 RetentionPolicy.Builder retentionBuilder = RetentionPolicy.newBuilder();
-                retentionBuilder.setRetentionPeriod(period);
+                ifNonNull(duration, retentionBuilder::setRetentionDuration);
                 retentionBuilder.setIsLocked(locked);
                 if (locked) {
                   retentionBuilder.setEffectiveTime(effectiveTime);