Repository: googleapis/java-storage
Pull request #241 (2020-04-13 23:21:24)

fix: surface storage interface expectations correctly.
cc: @dmitry-fa , @athakor based on pending PRs.

We will continue to update the Storage interface and mark it as InternalExtensionOnly.




codecov[bot]: # [Codecov](https://codecov.io/gh/googleapis/java-storage/pull/241?src=pr&el=h1) Report
> Merging [#241](https://codecov.io/gh/googleapis/java-storage/pull/241?src=pr&el=desc) into [master](https://codecov.io/gh/googleapis/java-storage/commit/0f06c9b7a4f2763cf4440114421bc2913e5ec8c8&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/googleapis/java-storage/pull/241/graphs/tree.svg?width=650&height=150&src=pr&token=ssm139YOdp)](https://codecov.io/gh/googleapis/java-storage/pull/241?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master     #241   +/-   ##
=========================================
  Coverage     63.50%   63.50%           
  Complexity      540      540           
=========================================
  Files            30       30           
  Lines          4762     4762           
  Branches        427      427           
=========================================
  Hits           3024     3024           
  Misses         1578     1578           
  Partials        160      160           
```


| [Impacted Files](https://codecov.io/gh/googleapis/java-storage/pull/241?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rc/main/java/com/google/cloud/storage/Storage.java](https://codecov.io/gh/googleapis/java-storage/pull/241/diff?src=pr&el=tree#diff-Z29vZ2xlLWNsb3VkLXN0b3JhZ2Uvc3JjL21haW4vamF2YS9jb20vZ29vZ2xlL2Nsb3VkL3N0b3JhZ2UvU3RvcmFnZS5qYXZh) | `80.57% <ø> (ø)` | `0.00 <0.00> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/googleapis/java-storage/pull/241?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/googleapis/java-storage/pull/241?src=pr&el=footer). Last update [15cb267...423a4f4](https://codecov.io/gh/googleapis/java-storage/pull/241?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

dmitry-fa: This fix is a good solution for the situation with extending the Storage interface. My question, is this change allowed?
Quoting: [InternalExtensionOnly](https://googleapis.github.io/api-common-java/1.6.0/apidocs/com/google/api/core/InternalExtensionOnly.html)
> Adding this annotation to an API is considered API-breaking.

frankyn: Hi @dmitry-fa, +1 to being allowed. I went through internal review/discussion with folks. The annotation is only documentation and doesn't change how the interface can be used. 

Also acknowledging that this library doesn't follow Semantic versioning strictly but looking for input from @JesseLovelace and @crwilcox on this point. I may end up removing it until later. 

Patch:
diff --git a/README.md b/README.md
index bcdb231c..368cbf10 100644
--- a/README.md
+++ b/README.md
@@ -216,7 +216,8 @@ See [TESTING] to read more about testing.
 Versioning
 ----------
 
-This library follows [Semantic Versioning](http://semver.org/).
+This library follows [Semantic Versioning](http://semver.org/), but does update [Storage interface](src/main/java/com.google.cloud.storage/Storage.java)
+to introduce new methods which can break your implementations if you implement this interface for testing purposes.
 
 It is currently in major version one (``1.y.z``), which means that the public API should be considered stable.
 
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 7efc24c9..3cb99e61 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
@@ -52,12 +52,26 @@ import java.util.Objects;
 import java.util.concurrent.TimeUnit;
 
 /**
- * A Google cloud storage object.
+ * An object in Google Cloud Storage. A {@code Blob} object includes the {@code BlobId} instance,
+ * the set of properties inherited from the {@link BlobInfo} class and the {@code Storage} instance.
+ * The class provides methods to perform operations on the object. Reading a property value does not
+ * issue any RPC calls. The object content is not stored within the {@code Blob} instance.
+ * Operations that access the content issue one or multiple RPC calls, depending on the content
+ * size.
  *
  * <p>Objects of this class are immutable. Operations that modify the blob like {@link #update} and
- * {@link #copyTo} return a new object. To get a {@code Blob} object with the most recent
- * information use {@link #reload}. {@code Blob} adds a layer of service-related functionality over
- * {@link BlobInfo}.
+ * {@link #copyTo} return a new object. Any changes to the object in Google Cloud Storage made after
+ * creation of the {@code Blob} are not visible in the {@code Blob}. To get a {@code Blob} object
+ * with the most recent information use {@link #reload}.
+ *
+ * <p>Example of getting the content of the object in Google Cloud Storage:
+ *
+ * <pre>{@code
+ * BlobId blobId = BlobId.of(bucketName, blobName);
+ * Blob blob = storage.get(blobId);
+ * long size = blob.getSize(); // no RPC call is required
+ * byte[] content = blob.getContent(); // one or multiple RPC calls will be issued
+ * }</pre>
  */
 public class Blob extends BlobInfo {
 
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 f21e0215..ef38ba03 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
@@ -34,8 +34,8 @@ import com.google.common.collect.Maps;
 import com.google.common.io.BaseEncoding;
 import java.io.Serializable;
 import java.math.BigInteger;
+import java.nio.ByteBuffer;
 import java.util.AbstractMap;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -44,7 +44,21 @@ import java.util.Objects;
 import java.util.Set;
 
 /**
- * Google Storage object metadata.
+ * Information about an object in Google Cloud Storage. A {@code BlobInfo} object includes the
+ * {@code BlobId} instance and the set of properties, such as the blob's access control
+ * configuration, user provided metadata, the CRC32C checksum, etc. Instances of this class are used
+ * to create a new object in Google Cloud Storage or update the properties of an existing object. To
+ * deal with existing Storage objects the API includes the {@link Blob} class which extends {@code
+ * BlobInfo} and declares methods to perform operations on the object. Neither {@code BlobInfo} nor
+ * {@code Blob} instances keep the object content, just the object properties.
+ *
+ * <p>Example of usage {@code BlobInfo} to create an object in Google Cloud Storage:
+ *
+ * <pre>{@code
+ * BlobId blobId = BlobId.of(bucketName, blobName);
+ * BlobInfo blobInfo = BlobInfo.newBuilder(blobId).setContentType("text/plain").build();
+ * Blob blob = storage.create(blobInfo, "Hello, world".getBytes(StandardCharsets.UTF_8));
+ * }</pre>
  *
  * @see <a href="https://cloud.google.com/storage/docs/concepts-techniques#concepts">Concepts and
  *     Terminology</a>
@@ -232,6 +246,7 @@ public class BlobInfo implements Serializable {
      *
      * @see <a href="https://cloud.google.com/storage/docs/hashes-etags#_JSONAPI">Hashes and ETags:
      *     Best Practices</a>
+     * @throws IllegalArgumentException when given an invalid hexadecimal value.
      */
     public abstract Builder setMd5FromHexString(String md5HexString);
 
@@ -252,6 +267,7 @@ public class BlobInfo implements Serializable {
      *
      * @see <a href="https://cloud.google.com/storage/docs/hashes-etags#_JSONAPI">Hashes and ETags:
      *     Best Practices</a>
+     * @throws IllegalArgumentException when given an invalid hexadecimal value.
      */
     public abstract Builder setCrc32cFromHexString(String crc32cHexString);
 
@@ -293,7 +309,7 @@ public class BlobInfo implements Serializable {
   }
 
   static final class BuilderImpl extends Builder {
-
+    private final String hexDecimalValues = "0123456789abcdef";
     private BlobId blobId;
     private String generatedId;
     private String contentType;
@@ -442,16 +458,27 @@ public class BlobInfo implements Serializable {
       return this;
     }
 
+    @Override
     public Builder setMd5FromHexString(String md5HexString) {
       if (md5HexString == null) {
         return this;
       }
-      byte[] bytes = new BigInteger(md5HexString, 16).toByteArray();
-      int leadingEmptyBytes = bytes.length - md5HexString.length() / 2;
-      if (leadingEmptyBytes > 0) {
-        bytes = Arrays.copyOfRange(bytes, leadingEmptyBytes, bytes.length);
+      if (md5HexString.length() % 2 != 0) {
+        throw new IllegalArgumentException(
+            "each byte must be represented by 2 valid hexadecimal characters");
       }
-      this.md5 = BaseEncoding.base64().encode(bytes);
+      String md5HexStringLower = md5HexString.toLowerCase();
+      ByteBuffer md5ByteBuffer = ByteBuffer.allocate(md5HexStringLower.length() / 2);
+      for (int charIndex = 0; charIndex < md5HexStringLower.length(); charIndex += 2) {
+        int higherOrderBits = this.hexDecimalValues.indexOf(md5HexStringLower.charAt(charIndex));
+        int lowerOrderBits = this.hexDecimalValues.indexOf(md5HexStringLower.charAt(charIndex + 1));
+        if (higherOrderBits == -1 || lowerOrderBits == -1) {
+          throw new IllegalArgumentException(
+              "each byte must be represented by 2 valid hexadecimal characters");
+        }
+        md5ByteBuffer.put((byte) (higherOrderBits << 4 | lowerOrderBits));
+      }
+      this.md5 = BaseEncoding.base64().encode(md5ByteBuffer.array());
       return this;
     }
 
@@ -466,12 +493,23 @@ public class BlobInfo implements Serializable {
       if (crc32cHexString == null) {
         return this;
       }
-      byte[] bytes = new BigInteger(crc32cHexString, 16).toByteArray();
-      int leadingEmptyBytes = bytes.length - crc32cHexString.length() / 2;
-      if (leadingEmptyBytes > 0) {
-        bytes = Arrays.copyOfRange(bytes, leadingEmptyBytes, bytes.length);
+      if (crc32cHexString.length() % 2 != 0) {
+        throw new IllegalArgumentException(
+            "each byte must be represented by 2 valid hexadecimal characters");
+      }
+      String crc32cHexStringLower = crc32cHexString.toLowerCase();
+      ByteBuffer crc32cByteBuffer = ByteBuffer.allocate(crc32cHexStringLower.length() / 2);
+      for (int charIndex = 0; charIndex < crc32cHexStringLower.length(); charIndex += 2) {
+        int higherOrderBits = this.hexDecimalValues.indexOf(crc32cHexStringLower.charAt(charIndex));
+        int lowerOrderBits =
+            this.hexDecimalValues.indexOf(crc32cHexStringLower.charAt(charIndex + 1));
+        if (higherOrderBits == -1 || lowerOrderBits == -1) {
+          throw new IllegalArgumentException(
+              "each byte must be represented by 2 valid hexadecimal characters");
+        }
+        crc32cByteBuffer.put((byte) (higherOrderBits << 4 | lowerOrderBits));
       }
-      this.crc32c = BaseEncoding.base64().encode(bytes);
+      this.crc32c = BaseEncoding.base64().encode(crc32cByteBuffer.array());
       return this;
     }
 
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java
index c95213d8..a9bb589a 100644
--- a/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java
+++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java
@@ -19,6 +19,7 @@ package com.google.cloud.storage;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 
+import com.google.api.core.InternalExtensionOnly;
 import com.google.api.gax.paging.Page;
 import com.google.auth.ServiceAccountSigner;
 import com.google.auth.ServiceAccountSigner.SigningException;
@@ -55,6 +56,7 @@ import java.util.concurrent.TimeUnit;
  *
  * @see <a href="https://cloud.google.com/storage/docs">Google Cloud Storage</a>
  */
+@InternalExtensionOnly
 public interface Storage extends Service<StorageOptions> {
 
   enum PredefinedAcl {
diff --git a/pom.xml b/pom.xml
index 7de0dee7..2df8c308 100644
--- a/pom.xml
+++ b/pom.xml
@@ -77,7 +77,7 @@
       <dependency>
         <groupId>com.google.guava</groupId>
         <artifactId>guava</artifactId>
-        <version>28.2-android</version>
+        <version>29.0-android</version>
       </dependency>
       <dependency>
         <groupId>com.google.http-client</groupId>
diff --git a/samples/snippets/pom.xml b/samples/snippets/pom.xml
index 2d979c7e..bd7d378c 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>4.4.1</version>
+        <version>5.1.0</version>
         <type>pom</type>
         <scope>import</scope>
       </dependency>


Test patch:
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobInfoTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobInfoTest.java
index a57be6df..ea4f6dbd 100644
--- a/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobInfoTest.java
+++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobInfoTest.java
@@ -49,12 +49,16 @@ public class BlobInfoTest {
   private static final String CONTENT_LANGUAGE = "En";
   private static final String CRC32 = "FF00";
   private static final String CRC32_HEX_STRING = "145d34";
+  private static final String CRC32_HEX_STRING_LEADING_ZEROS = "005d34";
+  private static final String CRC32_BASE64_LEADING_ZEROS = "AF00";
   private static final Long DELETE_TIME = System.currentTimeMillis();
   private static final String ETAG = "0xFF00";
   private static final Long GENERATION = 1L;
   private static final String GENERATED_ID = "B/N:1";
   private static final String MD5 = "FF00";
   private static final String MD5_HEX_STRING = "145d34";
+  private static final String MD5_HEX_STRING_LEADING_ZEROS = "0006a7de52b4e0b82602ce09809523ca";
+  private static final String MD5_BASE64_LEADING_ZEROS = "AAan3lK04LgmAs4JgJUjyg==";
   private static final String MEDIA_LINK = "http://media/b/n";
   private static final Map<String, String> METADATA = ImmutableMap.of("n1", "v1", "n2", "v2");
   private static final Long META_GENERATION = 10L;
@@ -132,6 +136,15 @@ public class BlobInfoTest {
     assertEquals(MD5, blobInfo.getMd5());
   }
 
+  @Test
+  public void testToBuilderSetMd5FromHexStringLeadingZeros() {
+    BlobInfo blobInfo =
+        BlobInfo.newBuilder(BlobId.of("b2", "n2"))
+            .setMd5FromHexString(MD5_HEX_STRING_LEADING_ZEROS)
+            .build();
+    assertEquals(MD5_BASE64_LEADING_ZEROS, blobInfo.getMd5());
+  }
+
   @Test
   public void testToBuilderSetCrc32cFromHexString() {
     BlobInfo blobInfo =
@@ -139,6 +152,15 @@ public class BlobInfoTest {
     assertEquals(CRC32, blobInfo.getCrc32c());
   }
 
+  @Test
+  public void testToBuilderSetCrc32cFromHexStringLeadingZeros() {
+    BlobInfo blobInfo =
+        BlobInfo.newBuilder(BlobId.of("b2", "n2"))
+            .setCrc32cFromHexString(CRC32_HEX_STRING_LEADING_ZEROS)
+            .build();
+    assertEquals(CRC32_BASE64_LEADING_ZEROS, blobInfo.getCrc32c());
+  }
+
   @Test
   public void testToBuilderIncomplete() {
     BlobInfo incompleteBlobInfo = BlobInfo.newBuilder(BlobId.of("b2", "n2")).build();