Repository: googleapis/java-storage
Pull request #679 (2021-01-12 19:20:38)

fix: unnecessary options in resumable upload URL
Fix unnecessary options in resumable upload URL by explicitly selecting them instead of iterating though all


Patch:
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 2e9c0a38..ca87933c 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
@@ -120,6 +120,14 @@ class BlobWriteChannel extends BaseWriteChannel<StorageOptions, BlobInfo> {
                     // For completeness, this case is not possible because it would require retrying
                     // a 400 status code which is not allowed.
                     //
+                    // Case 7: remoteNextByteOffset==-1 && last == true
+                    // Upload is complete and retry occurred in the "last" chunk. Data sent was
+                    // received by the service.
+                    //
+                    // Case 8: remoteNextByteOffset==-1 && last == false
+                    // Upload was completed by another client because this retry did not occur
+                    // during the last chunk.
+                    //
                     // Get remote offset from API
                     long remoteNextByteOffset =
                         getOptions().getStorageRpcV1().getCurrentUploadOffset(getUploadId());
@@ -154,7 +162,8 @@ class BlobWriteChannel extends BaseWriteChannel<StorageOptions, BlobInfo> {
                       // Continue to next chunk
                       retrying = false;
                       return;
-                    } else {
+                    } else if (localNextByteOffset < remoteNextByteOffset
+                        && driftOffset > getChunkSize()) {
                       // Case 5
                       StringBuilder sb = new StringBuilder();
                       sb.append(
@@ -167,6 +176,13 @@ class BlobWriteChannel extends BaseWriteChannel<StorageOptions, BlobInfo> {
                       sb.append("remoteNextByteOffset: ").append(remoteNextByteOffset).append('\n');
                       sb.append("driftOffset: ").append(driftOffset).append("\n\n");
                       throw new StorageException(0, sb.toString());
+                    } else if (remoteNextByteOffset == -1 && last) {
+                      // Case 7
+                      retrying = false;
+                      return;
+                    } else if (remoteNextByteOffset == -1 && !last) {
+                      // Case 8
+                      throw new StorageException(0, "Resumable upload is already complete.");
                     }
                   }
                   // Request was successful and retrying state is now disabled.
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 abdfa33c..74977dc6 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
@@ -766,7 +766,7 @@ public class HttpStorageRpc implements StorageRpc {
         response = httpRequest.execute();
         int code = response.getStatusCode();
         if (code == 201 || code == 200) {
-          throw new StorageException(0, "Resumable upload is already complete.");
+          return -1;
         }
         StringBuilder sb = new StringBuilder();
         sb.append("Not sure what occurred. Here's debugging information:\n");
@@ -888,38 +888,30 @@ public class HttpStorageRpc implements StorageRpc {
       if (kmsKeyName != null && kmsKeyName.contains("cryptoKeyVersions")) {
         object.setKmsKeyName("");
       }
-      Insert req = storage.objects().insert(object.getBucket(), object);
-      GenericUrl url = req.buildHttpRequest().getUrl();
-      String scheme = url.getScheme();
-      String host = url.getHost();
-      int port = url.getPort();
-      port = port > 0 ? port : url.toURL().getDefaultPort();
-      String path = "/upload" + url.getRawPath();
-      url = new GenericUrl(scheme + "://" + host + ":" + port + path);
+      Insert req =
+          storage
+              .objects()
+              .insert(object.getBucket(), object)
+              .setName(object.getName())
+              .setProjection(Option.PROJECTION.getString(options))
+              .setPredefinedAcl(Option.PREDEFINED_ACL.getString(options))
+              .setIfMetagenerationMatch(Option.IF_METAGENERATION_MATCH.getLong(options))
+              .setIfMetagenerationNotMatch(Option.IF_METAGENERATION_NOT_MATCH.getLong(options))
+              .setIfGenerationMatch(Option.IF_GENERATION_MATCH.getLong(options))
+              .setIfGenerationNotMatch(Option.IF_GENERATION_NOT_MATCH.getLong(options))
+              .setUserProject(Option.USER_PROJECT.getString(options))
+              .setKmsKeyName(Option.KMS_KEY_NAME.getString(options));
+      GenericUrl url = req.buildHttpRequestUrl();
+      url.setRawPath("/upload" + url.getRawPath());
       url.set("uploadType", "resumable");
-      url.set("name", object.getName());
-      for (Option option : options.keySet()) {
-        Object content = option.get(options);
-        if (content != null) {
-          url.set(option.value(), content.toString());
-        }
-      }
+
       JsonFactory jsonFactory = storage.getJsonFactory();
       HttpRequestFactory requestFactory = storage.getRequestFactory();
       HttpRequest httpRequest =
           requestFactory.buildPostRequest(url, new JsonHttpContent(jsonFactory, object));
       HttpHeaders requestHeaders = httpRequest.getHeaders();
       requestHeaders.set("X-Upload-Content-Type", detectContentType(object, options));
-      String key = Option.CUSTOMER_SUPPLIED_KEY.getString(options);
-      if (key != null) {
-        BaseEncoding base64 = BaseEncoding.base64();
-        HashFunction hashFunction = Hashing.sha256();
-        requestHeaders.set("x-goog-encryption-algorithm", "AES256");
-        requestHeaders.set("x-goog-encryption-key", key);
-        requestHeaders.set(
-            "x-goog-encryption-key-sha256",
-            base64.encode(hashFunction.hashBytes(base64.decode(key)).asBytes()));
-      }
+      setEncryptionHeaders(requestHeaders, "x-goog-encryption-", options);
       HttpResponse response = httpRequest.execute();
       if (response.getStatusCode() != 200) {
         GoogleJsonError error = new GoogleJsonError();
diff --git a/pom.xml b/pom.xml
index f9ac84c0..51864ccf 100644
--- a/pom.xml
+++ b/pom.xml
@@ -63,7 +63,7 @@
     <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
     <github.global.server>github</github.global.server>
     <site.installationModule>google-cloud-storage-parent</site.installationModule>
-    <google.cloud.shared-dependencies.version>0.17.0</google.cloud.shared-dependencies.version>
+    <google.cloud.shared-dependencies.version>0.17.1</google.cloud.shared-dependencies.version>
   </properties>
 
   <dependencyManagement>


Test patch:
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobWriteChannelTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobWriteChannelTest.java
index b6d7bbf3..eefee772 100644
--- a/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobWriteChannelTest.java
+++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/BlobWriteChannelTest.java
@@ -202,6 +202,64 @@ public class BlobWriteChannelTest {
     assertArrayEquals(buffer.array(), capturedBuffer.getValue());
   }
 
+  @Test
+  public void testWriteWithLastFlushRetryChunkButCompleted() throws IOException {
+    StorageException exception = new StorageException(new SocketException("Socket closed"));
+    ByteBuffer buffer = randomBuffer(MIN_CHUNK_SIZE);
+    Capture<byte[]> capturedBuffer = Capture.newInstance();
+    expect(storageRpcMock.open(BLOB_INFO.toPb(), EMPTY_RPC_OPTIONS)).andReturn(UPLOAD_ID);
+    expect(
+            storageRpcMock.writeWithResponse(
+                eq(UPLOAD_ID),
+                capture(capturedBuffer),
+                eq(0),
+                eq(0L),
+                eq(MIN_CHUNK_SIZE),
+                eq(true)))
+        .andThrow(exception);
+    expect(storageRpcMock.getCurrentUploadOffset(eq(UPLOAD_ID))).andReturn(-1L);
+    replay(storageRpcMock);
+    writer = new BlobWriteChannel(options, BLOB_INFO, EMPTY_RPC_OPTIONS);
+    assertEquals(MIN_CHUNK_SIZE, writer.write(buffer));
+    writer.close();
+    assertFalse(writer.isRetrying());
+    assertFalse(writer.isOpen());
+    // Capture captures entire buffer of a chunk even when not completely used.
+    // Making assert selective up to the size of MIN_CHUNK_SIZE
+    assertArrayEquals(Arrays.copyOf(capturedBuffer.getValue(), MIN_CHUNK_SIZE), buffer.array());
+  }
+
+  @Test
+  public void testWriteWithFlushRetryChunkButCompletedByAnotherClient() throws IOException {
+    StorageException exception = new StorageException(new SocketException("Socket closed"));
+    StorageException completedException =
+        new StorageException(0, "Resumable upload is already complete.");
+    ByteBuffer buffer = randomBuffer(MIN_CHUNK_SIZE);
+    Capture<byte[]> capturedBuffer = Capture.newInstance();
+    expect(storageRpcMock.open(BLOB_INFO.toPb(), EMPTY_RPC_OPTIONS)).andReturn(UPLOAD_ID);
+    expect(
+            storageRpcMock.writeWithResponse(
+                eq(UPLOAD_ID),
+                capture(capturedBuffer),
+                eq(0),
+                eq(0L),
+                eq(MIN_CHUNK_SIZE),
+                eq(false)))
+        .andThrow(exception);
+    expect(storageRpcMock.getCurrentUploadOffset(eq(UPLOAD_ID))).andReturn(-1L);
+    replay(storageRpcMock);
+    writer = new BlobWriteChannel(options, BLOB_INFO, EMPTY_RPC_OPTIONS);
+    writer.setChunkSize(MIN_CHUNK_SIZE);
+    try {
+      writer.write(buffer);
+      fail("Expected completed exception.");
+    } catch (StorageException ex) {
+      assertEquals(ex, completedException);
+    }
+    assertTrue(writer.isRetrying());
+    assertTrue(writer.isOpen());
+  }
+
   @Test
   public void testWriteWithFlush() throws IOException {
     expect(storageRpcMock.open(BLOB_INFO.toPb(), EMPTY_RPC_OPTIONS)).andReturn(UPLOAD_ID);