Repository: googleapis/java-storage
Pull request #246 (2020-04-14 18:45:43)

fix: implementations of FromHexString() for md5 and crc32c
FromHexString() helpers don't handle leading zeros correctly. Added tests and provided a new implementation.

codecov[bot]: # [Codecov](https://codecov.io/gh/googleapis/java-storage/pull/246?src=pr&el=h1) Report
> Merging [#246](https://codecov.io/gh/googleapis/java-storage/pull/246?src=pr&el=desc) into [master](https://codecov.io/gh/googleapis/java-storage/commit/0f06c9b7a4f2763cf4440114421bc2913e5ec8c8&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `66.66%`.

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

```diff
@@             Coverage Diff              @@
##             master     #246      +/-   ##
============================================
+ Coverage     63.50%   63.52%   +0.02%     
  Complexity      540      540              
============================================
  Files            30       30              
  Lines          4762     4776      +14     
  Branches        427      431       +4     
============================================
+ Hits           3024     3034      +10     
- Misses         1578     1580       +2     
- Partials        160      162       +2     
```


| [Impacted Files](https://codecov.io/gh/googleapis/java-storage/pull/246?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...c/main/java/com/google/cloud/storage/BlobInfo.java](https://codecov.io/gh/googleapis/java-storage/pull/246/diff?src=pr&el=tree#diff-Z29vZ2xlLWNsb3VkLXN0b3JhZ2Uvc3JjL21haW4vamF2YS9jb20vZ29vZ2xlL2Nsb3VkL3N0b3JhZ2UvQmxvYkluZm8uamF2YQ==) | `88.25% <66.66%> (-0.71%)` | `88.00 <0.00> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/googleapis/java-storage/pull/246?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/246?src=pr&el=footer). Last update [15cb267...b11e66c](https://codecov.io/gh/googleapis/java-storage/pull/246?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
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..463f34f9 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;
@@ -232,6 +232,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 +253,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 +295,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 +444,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");
+      }
+      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(bytes);
+      this.md5 = BaseEncoding.base64().encode(md5ByteBuffer.array());
       return this;
     }
 
@@ -466,12 +479,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;
     }
 


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();