Repository: googleapis/java-storage (java)

Issue:
Storage#update always removes all lifecycle rules if they are not specified
Issue with `com.google.cloud.storage.Storage#update(com.google.cloud.storage.BucketInfo, com.google.cloud.storage.Storage.BucketTargetOption...)`

#### Expected behaviour:
update only fields that are specified in `BucketInfo` parameter.

#### Actual behaviour:
along the updated fields, removes lifecycle rules if they are not specified.

Example code:
```scala
    val bucketId = "[...]"

    val client = StorageOptions.getDefaultInstance.getService
    val before = client.get(bucketId)
    println(s"Before: ${before.getLifecycleRules.size()}")

    val updateRequest = BucketInfo
      .newBuilder(bucketId)
      .setLabels(singletonMap("test", "test"))
      .build
    client.update(updateRequest)

    val after = client.get(bucketId)
    println(s"After: ${after.getLifecycleRules.size()}")
```

Actual output (for bucket with 1 lifecycle rule):
```txt
Before: 1
After: 0
```

I suspect that issue is located here:
https://github.com/googleapis/java-storage/blob/accf5021e99a083aa4b3e8935d8ccd0e3668ae6f/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java#L1841-L1869


Pull request:
fix: update BucketInfo translation code to properly handle lifecycle rules
Fixes #850



Patch:
diff --git a/.github/readme/synth.metadata/synth.metadata b/.github/readme/synth.metadata/synth.metadata
index 03740b44..26c40706 100644
--- a/.github/readme/synth.metadata/synth.metadata
+++ b/.github/readme/synth.metadata/synth.metadata
@@ -4,14 +4,14 @@
       "git": {
         "name": ".",
         "remote": "https://github.com/googleapis/java-storage.git",
-        "sha": "c71ccc4f5704902b48e458b8225b2ad74a405290"
+        "sha": "513f3bb556150094d2d26aca1a4e5279cf414a53"
       }
     },
     {
       "git": {
         "name": "synthtool",
         "remote": "https://github.com/googleapis/synthtool.git",
-        "sha": "1778de119522ea9c9b18763c2a4577e22a60433c"
+        "sha": "c3e41da0fa256ad7f6b4bc76b9d069dedecdfef4"
       }
     }
   ]
diff --git a/.kokoro/build.sh b/.kokoro/build.sh
index 9e0dd51f..b43a9f96 100755
--- a/.kokoro/build.sh
+++ b/.kokoro/build.sh
@@ -23,8 +23,8 @@ cd ${scriptDir}/..
 # include common functions
 source ${scriptDir}/common.sh
 
-# Print out Java version
-java -version
+# Print out Maven & Java version
+mvn -version
 echo ${JOB_TYPE}
 
 # attempt to install 3 times with exponential backoff (starting with 10 seconds)
diff --git a/README.md b/README.md
index d5e613be..086565d7 100644
--- a/README.md
+++ b/README.md
@@ -17,7 +17,7 @@ If you are using Maven with [BOM][libraries-bom], add this to your pom.xml file
     <dependency>
       <groupId>com.google.cloud</groupId>
       <artifactId>libraries-bom</artifactId>
-      <version>20.4.0</version>
+      <version>20.5.0</version>
       <type>pom</type>
       <scope>import</scope>
     </dependency>
diff --git a/google-cloud-storage/pom.xml b/google-cloud-storage/pom.xml
index 3f63333a..30596eff 100644
--- a/google-cloud-storage/pom.xml
+++ b/google-cloud-storage/pom.xml
@@ -16,7 +16,7 @@
   </parent>
   <properties>
     <site.installationModule>google-cloud-storage</site.installationModule>
-    <kms.version>0.88.4</kms.version>
+    <kms.version>0.89.0</kms.version>
   </properties>
   <dependencies>
     <dependency>
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BlobWriteChannel.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BlobWriteChannel.java
index 09bc1708..2d7a8520 100644
--- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BlobWriteChannel.java
+++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BlobWriteChannel.java
@@ -26,6 +26,7 @@ import com.google.cloud.RetryHelper;
 import com.google.cloud.WriteChannel;
 import com.google.cloud.storage.spi.v1.StorageRpc;
 import com.google.common.collect.Maps;
+import java.math.BigInteger;
 import java.net.URL;
 import java.util.Map;
 import java.util.concurrent.Callable;
@@ -83,14 +84,52 @@ class BlobWriteChannel extends BaseWriteChannel<StorageOptions, BlobInfo> {
         .get(getEntity().toPb(), Maps.newEnumMap(StorageRpc.Option.class));
   }
 
-  private StorageException unrecoverableState(
-      int chunkOffset, int chunkLength, long localPosition, long remotePosition, boolean last) {
+  private static StorageException unrecoverableState(
+      String uploadId,
+      int chunkOffset,
+      int chunkLength,
+      long localPosition,
+      long remotePosition,
+      boolean last) {
+    return unrecoverableState(
+        uploadId,
+        chunkOffset,
+        chunkLength,
+        localPosition,
+        remotePosition,
+        last,
+        "Unable to recover in upload.\nThis may be a symptom of multiple clients uploading to the same upload session.");
+  }
+
+  private static StorageException errorResolvingMetadataLastChunk(
+      String uploadId,
+      int chunkOffset,
+      int chunkLength,
+      long localPosition,
+      long remotePosition,
+      boolean last) {
+    return unrecoverableState(
+        uploadId,
+        chunkOffset,
+        chunkLength,
+        localPosition,
+        remotePosition,
+        last,
+        "Unable to load object metadata to determine if last chunk was successfully written");
+  }
+
+  private static StorageException unrecoverableState(
+      String uploadId,
+      int chunkOffset,
+      int chunkLength,
+      long localPosition,
+      long remotePosition,
+      boolean last,
+      String message) {
     StringBuilder sb = new StringBuilder();
-    sb.append("Unable to recover in upload.\n");
-    sb.append(
-        "This may be a symptom of multiple clients uploading to the same upload session.\n\n");
+    sb.append(message).append("\n\n");
     sb.append("For debugging purposes:\n");
-    sb.append("uploadId: ").append(getUploadId()).append('\n');
+    sb.append("uploadId: ").append(uploadId).append('\n');
     sb.append("chunkOffset: ").append(chunkOffset).append('\n');
     sb.append("chunkLength: ").append(chunkLength).append('\n');
     sb.append("localOffset: ").append(localPosition).append('\n');
@@ -162,7 +201,7 @@ class BlobWriteChannel extends BaseWriteChannel<StorageOptions, BlobInfo> {
                   // Get remote offset from API
                   final long localPosition = getPosition();
                   // For each request it should be possible to retry from its location in this code
-                  final long remotePosition = isRetrying() ? getRemotePosition() : getPosition();
+                  final long remotePosition = isRetrying() ? getRemotePosition() : localPosition;
                   final int chunkOffset = (int) (remotePosition - localPosition);
                   final int chunkLength = length - chunkOffset;
                   final boolean uploadAlreadyComplete = remotePosition == -1;
@@ -176,10 +215,38 @@ class BlobWriteChannel extends BaseWriteChannel<StorageOptions, BlobInfo> {
                     if (storageObject == null) {
                       storageObject = getRemoteStorageObject();
                     }
+                    // the following checks are defined here explicitly to provide a more
+                    // informative if either storageObject is unable to be resolved or it's size is
+                    // unable to be determined. This scenario is a very rare case of failure that
+                    // can arise when packets are lost.
+                    if (storageObject == null) {
+                      throw errorResolvingMetadataLastChunk(
+                          getUploadId(),
+                          chunkOffset,
+                          chunkLength,
+                          localPosition,
+                          remotePosition,
+                          lastChunk);
+                    }
                     // Verify that with the final chunk we match the blob length
-                    if (storageObject.getSize().longValue() != getPosition() + length) {
+                    BigInteger size = storageObject.getSize();
+                    if (size == null) {
+                      throw errorResolvingMetadataLastChunk(
+                          getUploadId(),
+                          chunkOffset,
+                          chunkLength,
+                          localPosition,
+                          remotePosition,
+                          lastChunk);
+                    }
+                    if (size.longValue() != getPosition() + length) {
                       throw unrecoverableState(
-                          chunkOffset, chunkLength, localPosition, remotePosition, lastChunk);
+                          getUploadId(),
+                          chunkOffset,
+                          chunkLength,
+                          localPosition,
+                          remotePosition,
+                          lastChunk);
                     }
                     retrying = false;
                   } else if (uploadAlreadyComplete && !lastChunk && !checkingForLastChunk) {
@@ -201,7 +268,12 @@ class BlobWriteChannel extends BaseWriteChannel<StorageOptions, BlobInfo> {
                   } else {
                     // Case 4 && Case 8 && Case 9
                     throw unrecoverableState(
-                        chunkOffset, chunkLength, localPosition, remotePosition, lastChunk);
+                        getUploadId(),
+                        chunkOffset,
+                        chunkLength,
+                        localPosition,
+                        remotePosition,
+                        lastChunk);
                   }
                 }
               }),
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 5b69814e..c1339523 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
@@ -43,6 +43,7 @@ import java.io.IOException;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
 import java.io.Serializable;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -1838,33 +1839,52 @@ public class BucketInfo implements Serializable {
       website.setNotFoundPage(notFoundPage);
       bucketPb.setWebsite(website);
     }
-    Set<Rule> rules = new HashSet<>();
-    if (deleteRules != null) {
-      rules.addAll(
-          transform(
-              deleteRules,
-              new Function<DeleteRule, Rule>() {
-                @Override
-                public Rule apply(DeleteRule deleteRule) {
-                  return deleteRule.toPb();
-                }
-              }));
-    }
-    if (lifecycleRules != null) {
-      rules.addAll(
-          transform(
-              lifecycleRules,
-              new Function<LifecycleRule, Rule>() {
-                @Override
-                public Rule apply(LifecycleRule lifecycleRule) {
-                  return lifecycleRule.toPb();
-                }
-              }));
-    }
 
-    if (rules != null) {
+    if (deleteRules != null || lifecycleRules != null) {
       Lifecycle lifecycle = new Lifecycle();
-      lifecycle.setRule(ImmutableList.copyOf(rules));
+
+      // Here we determine if we need to "clear" any defined Lifecycle rules by explicitly setting
+      // the Rule list of lifecycle to the empty list.
+      // In order for us to clear the rules, one of the three following must be true:
+      //   1. deleteRules is null while lifecycleRules is non-null and empty
+      //   2. lifecycleRules is null while deleteRules is non-null and empty
+      //   3. lifecycleRules is non-null and empty while deleteRules is non-null and empty
+      // If none of the above three is true, we will interpret as the Lifecycle rules being
+      // updated to the defined set of DeleteRule and LifecycleRule.
+      if ((deleteRules == null && lifecycleRules.isEmpty())
+          || (lifecycleRules == null && deleteRules.isEmpty())
+          || (deleteRules != null && deleteRules.isEmpty() && lifecycleRules.isEmpty())) {
+        lifecycle.setRule(Collections.<Rule>emptyList());
+      } else {
+        Set<Rule> rules = new HashSet<>();
+        if (deleteRules != null) {
+          rules.addAll(
+              transform(
+                  deleteRules,
+                  new Function<DeleteRule, Rule>() {
+                    @Override
+                    public Rule apply(DeleteRule deleteRule) {
+                      return deleteRule.toPb();
+                    }
+                  }));
+        }
+        if (lifecycleRules != null) {
+          rules.addAll(
+              transform(
+                  lifecycleRules,
+                  new Function<LifecycleRule, Rule>() {
+                    @Override
+                    public Rule apply(LifecycleRule lifecycleRule) {
+                      return lifecycleRule.toPb();
+                    }
+                  }));
+        }
+
+        if (!rules.isEmpty()) {
+          lifecycle.setRule(ImmutableList.copyOf(rules));
+        }
+      }
+
       bucketPb.setLifecycle(lifecycle);
     }
 
diff --git a/pom.xml b/pom.xml
index 3fd7589e..a4f28287 100644
--- a/pom.xml
+++ b/pom.xml
@@ -187,7 +187,7 @@
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-javadoc-plugin</artifactId>
-        <version>3.2.0</version>
+        <version>3.3.0</version>
         <reportSets>
           <reportSet>
             <id>html</id>
diff --git a/samples/snippets/pom.xml b/samples/snippets/pom.xml
index d1db78c1..7f13eaac 100644
--- a/samples/snippets/pom.xml
+++ b/samples/snippets/pom.xml
@@ -30,7 +30,7 @@
       <dependency>
         <groupId>com.google.cloud</groupId>
         <artifactId>libraries-bom</artifactId>
-        <version>20.4.0</version>
+        <version>20.5.0</version>
         <type>pom</type>
         <scope>import</scope>
       </dependency>
diff --git a/synth.metadata b/synth.metadata
index 782ce883..69d7a143 100644
--- a/synth.metadata
+++ b/synth.metadata
@@ -4,14 +4,14 @@
       "git": {
         "name": ".",
         "remote": "https://github.com/googleapis/java-storage.git",
-        "sha": "c1eb941585d83814da26f9bf1cdcd1e1d1c80069"
+        "sha": "340f393f81f5b17b8835166ac7a3da9e8ffea2f4"
       }
     },
     {
       "git": {
         "name": "synthtool",
         "remote": "https://github.com/googleapis/synthtool.git",
-        "sha": "7332178a11ddddc91188dc0f25bca1ccadcaa6c6"
+        "sha": "8eae0234a16b26c2ff616d305dbd9786c8b10a47"
       }
     }
   ],


Test patch:
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 8def7c30..d72901c1 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
@@ -19,10 +19,12 @@ package com.google.cloud.storage;
 import static com.google.cloud.storage.Acl.Project.ProjectRole.VIEWERS;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
 import com.google.api.client.util.DateTime;
 import com.google.api.services.storage.model.Bucket;
+import com.google.api.services.storage.model.Bucket.Lifecycle;
 import com.google.api.services.storage.model.Bucket.Lifecycle.Rule;
 import com.google.cloud.storage.Acl.Project;
 import com.google.cloud.storage.Acl.Role;
@@ -39,6 +41,7 @@ import com.google.cloud.storage.BucketInfo.NumNewerVersionsDeleteRule;
 import com.google.cloud.storage.BucketInfo.RawDeleteRule;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -172,6 +175,8 @@ public class BucketInfoTest {
           .setLogging(LOGGING)
           .build();
 
+  private static final Lifecycle EMPTY_LIFECYCLE = lifecycle(Collections.<Rule>emptyList());
+
   @Test
   public void testToBuilder() {
     compareBuckets(BUCKET_INFO, BUCKET_INFO.toBuilder().build());
@@ -376,4 +381,70 @@ public class BucketInfoTest {
     assertEquals("test-bucket", logging.getLogBucket());
     assertEquals("test-", logging.getLogObjectPrefix());
   }
+
+  @Test
+  public void testRuleMappingIsCorrect_noMutations() {
+    Bucket bucket = bi().build().toPb();
+    assertNull(bucket.getLifecycle());
+  }
+
+  @Test
+  public void testRuleMappingIsCorrect_deleteLifecycleRules() {
+    Bucket bucket = bi().deleteLifecycleRules().build().toPb();
+    assertEquals(EMPTY_LIFECYCLE, bucket.getLifecycle());
+  }
+
+  @Test
+  @SuppressWarnings({"deprecation"})
+  public void testRuleMappingIsCorrect_setDeleteRules_null() {
+    Bucket bucket = bi().setDeleteRules(null).build().toPb();
+    assertNull(bucket.getLifecycle());
+  }
+
+  @Test
+  @SuppressWarnings({"deprecation"})
+  public void testRuleMappingIsCorrect_setDeleteRules_empty() {
+    Bucket bucket = bi().setDeleteRules(Collections.<DeleteRule>emptyList()).build().toPb();
+    assertEquals(EMPTY_LIFECYCLE, bucket.getLifecycle());
+  }
+
+  @Test
+  public void testRuleMappingIsCorrect_setLifecycleRules_empty() {
+    Bucket bucket = bi().setLifecycleRules(Collections.<LifecycleRule>emptyList()).build().toPb();
+    assertEquals(EMPTY_LIFECYCLE, bucket.getLifecycle());
+  }
+
+  @Test
+  public void testRuleMappingIsCorrect_setLifeCycleRules_nonEmpty() {
+    LifecycleRule lifecycleRule =
+        new LifecycleRule(
+            LifecycleAction.newDeleteAction(), LifecycleCondition.newBuilder().setAge(10).build());
+    Rule lifecycleDeleteAfter10 = lifecycleRule.toPb();
+    Bucket bucket = bi().setLifecycleRules(ImmutableList.of(lifecycleRule)).build().toPb();
+    assertEquals(lifecycle(lifecycleDeleteAfter10), bucket.getLifecycle());
+  }
+
+  @Test
+  @SuppressWarnings({"deprecation"})
+  public void testRuleMappingIsCorrect_setDeleteRules_nonEmpty() {
+    DeleteRule deleteRule = DELETE_RULES.get(0);
+    Rule deleteRuleAge5 = deleteRule.toPb();
+    Bucket bucket = bi().setDeleteRules(ImmutableList.of(deleteRule)).build().toPb();
+    assertEquals(lifecycle(deleteRuleAge5), bucket.getLifecycle());
+  }
+
+  private static Lifecycle lifecycle(Rule... rules) {
+    return lifecycle(Arrays.asList(rules));
+  }
+
+  private static Lifecycle lifecycle(List<Rule> rules) {
+    Lifecycle emptyLifecycle = new Lifecycle();
+    emptyLifecycle.setRule(rules);
+    return emptyLifecycle;
+  }
+
+  private static BucketInfo.Builder bi() {
+    String bucketId = "bucketId";
+    return BucketInfo.newBuilder(bucketId);
+  }
 }