fix: a resumable session without a Range header should be interpreted as 0 length

BenWhitehead · BenWhitehead · commit 98521f2cc8ef · 2023-08-25T14:47:07.000-04:00
According to https://cloud.google.com/storage/docs/performing-resumable-uploads#status-check a 308 response that does not contain a Range header should interpret as GCS having received no data. Include x-goog-gcs-idempotency-token in Json Resumable upload debug context
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/JsonResumableSessionFailureScenario.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/JsonResumableSessionFailureScenario.java
@@ -65,8 +65,7 @@ enum JsonResumableSessionFailureScenario {
       BaseServiceException.UNKNOWN_CODE,
       "dataLoss",
       "Client side data loss detected. Bytes acked is more than client sent."),
-  SCENARIO_9(503, "backendNotConnected", "Ack less than bytes sent"),
-  QUERY_SCENARIO_1(503, "", "Missing Range header in response");
+  SCENARIO_9(503, "backendNotConnected", "Ack less than bytes sent");
 
   private static final String PREFIX_I = "\t|< ";
   private static final String PREFIX_O = "\t|> ";
@@ -79,6 +78,7 @@ enum JsonResumableSessionFailureScenario {
           .or(matches("Content-Type"))
           .or(matches("Range"))
           .or(startsWith("X-Goog-Stored-"))
+          .or(matches("X-Goog-GCS-Idempotency-Token"))
           .or(matches("X-GUploader-UploadID"));
 
   private static final Predicate<Map.Entry<String, ?>> includeHeader =
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/JsonResumableSessionQueryTask.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/JsonResumableSessionQueryTask.java
@@ -92,13 +92,14 @@ final class JsonResumableSessionQueryTask
         }
       } else if (JsonResumableSessionFailureScenario.isContinue(code)) {
         String range1 = response.getHeaders().getRange();
+        //
         if (range1 != null) {
           ByteRangeSpec range = ByteRangeSpec.parse(range1);
           long endOffset = range.endOffset();
           return ResumableOperationResult.incremental(endOffset);
         } else {
-          throw JsonResumableSessionFailureScenario.QUERY_SCENARIO_1.toStorageException(
-              uploadId, response);
+          // https://cloud.google.com/storage/docs/performing-resumable-uploads#status-check
+          return ResumableOperationResult.incremental(0);
         }
       } else {
         HttpResponseException cause = new HttpResponseException(response);
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/ITJsonResumableSessionQueryTaskTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/ITJsonResumableSessionQueryTaskTest.java
@@ -141,10 +141,6 @@ public void incompleteSession() throws Exception {
     }
   }
 
-  /**
-   * This is a hard failure from the perspective of GCS as a range header is a required header to be
-   * included in the response to a query upload request.
-   */
   @Test
   public void incompleteSession_missingRangeHeader() throws Exception {
     HttpRequestHandler handler =
@@ -156,9 +152,9 @@ public void incompleteSession_missingRangeHeader() throws Exception {
       JsonResumableSessionQueryTask task =
           new JsonResumableSessionQueryTask(httpClientContext, uploadUrl);
 
-      StorageException se = assertThrows(StorageException.class, task::call);
-      assertThat(se.getCode()).isEqualTo(503);
-      assertThat(se).hasMessageThat().contains("Range");
+      ResumableOperationResult<@Nullable StorageObject> result = task.call();
+      assertThat(result.getPersistedSize()).isEqualTo(0);
+      assertThat(result.getObject()).isNull();
     }
   }
 
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/JsonResumableSessionFailureScenarioTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/JsonResumableSessionFailureScenarioTest.java
@@ -139,6 +139,25 @@ public void xGoogStoredHeadersIncludedIfPresent() throws IOException {
     assertThat(storageException).hasMessageThat().contains("|< x-goog-stored-something: blah");
   }
 
+  @Test
+  public void xGoogGcsIdempotencyTokenHeadersIncludedIfPresent() throws IOException {
+    HttpRequest req =
+        new MockHttpTransport()
+            .createRequestFactory()
+            .buildPutRequest(new GenericUrl("http://localhost:80980"), new EmptyContent());
+    req.getHeaders().setContentLength(0L);
+
+    HttpResponse resp = req.execute();
+    resp.getHeaders().set("X-Goog-Gcs-Idempotency-Token", "5").setContentLength(0L);
+
+    StorageException storageException =
+        JsonResumableSessionFailureScenario.SCENARIO_0.toStorageException(
+            "uploadId", resp, null, () -> null);
+
+    assertThat(storageException.getCode()).isEqualTo(0);
+    assertThat(storageException).hasMessageThat().contains("|< x-goog-gcs-idempotency-token: 5");
+  }
+
   private static final class Cause extends RuntimeException {
 
     private Cause() {

Original file line number	Diff line number	Diff line change
`@@ -141,10 +141,6 @@ public void incompleteSession() throws Exception {`
`141`	`141`	`}`
`142`	`142`	`}`
`143`	`143`
`144`		`- /**`
`145`		`- * This is a hard failure from the perspective of GCS as a range header is a required header to be`
`146`		`- * included in the response to a query upload request.`
`147`		`- */`
`148`	`144`	`@Test`
`149`	`145`	`public void incompleteSession_missingRangeHeader() throws Exception {`
`150`	`146`	`HttpRequestHandler handler =`
`@@ -156,9 +152,9 @@ public void incompleteSession_missingRangeHeader() throws Exception {`
`156`	`152`	`JsonResumableSessionQueryTask task =`
`157`	`153`	`new JsonResumableSessionQueryTask(httpClientContext, uploadUrl);`
`158`	`154`
`159`		`- StorageException se = assertThrows(StorageException.class, task::call);`
`160`		`- assertThat(se.getCode()).isEqualTo(503);`
`161`		`- assertThat(se).hasMessageThat().contains("Range");`
	`155`	`+ ResumableOperationResult<@Nullable StorageObject> result = task.call();`
	`156`	`+ assertThat(result.getPersistedSize()).isEqualTo(0);`
	`157`	`+ assertThat(result.getObject()).isNull();`
`162`	`158`	`}`
`163`	`159`	`}`
`164`	`160`