Repository: googleapis/java-storage
Pull request #2043 (2023-06-02 18:36:48)

feat: Error handling for Downloads
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
- [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/java-storage/issues/new/choose) before writing your code!  That way we can discuss the change, evaluate designs, and agree on the general idea
- [ ] Ensure the tests and linter pass
- [ ] Code coverage does not decrease (if any source code was changed)
- [ ] Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the [samples format](
https://github.com/GoogleCloudPlatform/java-docs-samples/blob/main/SAMPLE_FORMAT.md).



Patch:
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/PackagePrivateMethodWorkarounds.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/PackagePrivateMethodWorkarounds.java
index 738bbd10..6fa31d71 100644
--- a/google-cloud-storage/src/main/java/com/google/cloud/storage/PackagePrivateMethodWorkarounds.java
+++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/PackagePrivateMethodWorkarounds.java
@@ -86,17 +86,6 @@ public final class PackagePrivateMethodWorkarounds {
     };
   }
 
-  public static Function<WriteChannel, Optional<BlobInfo>> maybeGetBlobInfoFunction() {
-    return writeChannel -> {
-      Optional<StorageObject> so = maybeGetStorageObjectFunction().apply(writeChannel);
-      if (so.isPresent()) {
-        return Optional.of(Conversions.apiary().blobInfo().decode(so.get()));
-      } else {
-        return Optional.empty();
-      }
-    };
-  }
-
   public static ApiFuture<BlobInfo> getBlobInfoFromReadChannelFunction(ReadChannel c) {
     if (c instanceof StorageReadChannel) {
       StorageReadChannel src = (StorageReadChannel) c;
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DirectDownloadCallable.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DirectDownloadCallable.java
index cb008c05..56a1e74d 100644
--- a/google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DirectDownloadCallable.java
+++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DirectDownloadCallable.java
@@ -17,12 +17,11 @@
 package com.google.cloud.storage.transfermanager;
 
 import com.google.cloud.ReadChannel;
+import com.google.cloud.storage.BlobId;
 import com.google.cloud.storage.BlobInfo;
 import com.google.cloud.storage.Storage;
 import com.google.cloud.storage.Storage.BlobSourceOption;
-import com.google.cloud.storage.StorageException;
 import com.google.common.io.ByteStreams;
-import java.io.IOException;
 import java.nio.channels.FileChannel;
 import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
@@ -50,16 +49,26 @@ final class DirectDownloadCallable implements Callable<DownloadResult> {
   @Override
   public DownloadResult call() {
     Path path = TransferManagerUtils.createDestPath(parallelDownloadConfig, originalBlob);
-    try (ReadChannel rc = storage.reader(originalBlob.getBlobId(), opts)) {
+    long bytesCopied = -1L;
+    try (ReadChannel rc =
+        storage.reader(
+            BlobId.of(parallelDownloadConfig.getBucketName(), originalBlob.getName()), opts)) {
       FileChannel wc =
           FileChannel.open(
               path,
               StandardOpenOption.WRITE,
               StandardOpenOption.CREATE,
               StandardOpenOption.TRUNCATE_EXISTING);
-      ByteStreams.copy(rc, wc);
-    } catch (IOException e) {
-      throw new StorageException(e);
+      bytesCopied = ByteStreams.copy(rc, wc);
+    } catch (Exception e) {
+      if (bytesCopied == -1) {
+        return DownloadResult.newBuilder(originalBlob, TransferStatus.FAILED_TO_START)
+            .setException(e)
+            .build();
+      }
+      return DownloadResult.newBuilder(originalBlob, TransferStatus.FAILED_TO_FINISH)
+          .setException(e)
+          .build();
     }
     DownloadResult result =
         DownloadResult.newBuilder(originalBlob, TransferStatus.SUCCESS)
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DownloadResult.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DownloadResult.java
index 143154d7..12feafce 100644
--- a/google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DownloadResult.java
+++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/DownloadResult.java
@@ -20,7 +20,6 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 
 import com.google.cloud.storage.BlobInfo;
-import com.google.cloud.storage.StorageException;
 import com.google.common.base.MoreObjects;
 import java.nio.file.Path;
 import java.util.Comparator;
@@ -35,13 +34,13 @@ public final class DownloadResult {
   @NonNull private final BlobInfo input;
   @MonotonicNonNull private final Path outputDestination;
   @NonNull private final TransferStatus status;
-  @MonotonicNonNull private final StorageException exception;
+  @MonotonicNonNull private final Exception exception;
 
   private DownloadResult(
       @NonNull BlobInfo input,
       Path outputDestination,
       @NonNull TransferStatus status,
-      StorageException exception) {
+      Exception exception) {
     this.input = input;
     this.outputDestination = outputDestination;
     this.status = status;
@@ -64,7 +63,7 @@ public final class DownloadResult {
     return status;
   }
 
-  public @NonNull StorageException getException() {
+  public @NonNull Exception getException() {
     checkState(
         status == TransferStatus.FAILED_TO_FINISH || status == TransferStatus.FAILED_TO_START,
         "getException() is only valid when an unexpected error has occurred but status was %s",
@@ -111,7 +110,7 @@ public final class DownloadResult {
     private @NonNull BlobInfo input;
     private @MonotonicNonNull Path outputDestination;
     private @NonNull TransferStatus status;
-    private @MonotonicNonNull StorageException exception;
+    private @MonotonicNonNull Exception exception;
 
     private Builder(@NonNull BlobInfo input, @NonNull TransferStatus status) {
       this.input = input;
@@ -133,7 +132,7 @@ public final class DownloadResult {
       return this;
     }
 
-    public Builder setException(@NonNull StorageException exception) {
+    public Builder setException(@NonNull Exception exception) {
       this.exception = exception;
       return this;
     }
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/UploadCallable.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/UploadCallable.java
index deebc5b9..7aca6034 100644
--- a/google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/UploadCallable.java
+++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/transfermanager/UploadCallable.java
@@ -61,7 +61,7 @@ final class UploadCallable implements Callable<UploadResult> {
   }
 
   private UploadResult uploadWithoutChunking() {
-    long bytesCopied = 0L;
+    long bytesCopied = -1L;
     try {
       Optional<BlobInfo> newBlob;
       try (FileChannel r = FileChannel.open(sourceFile, StandardOpenOption.READ);
@@ -74,7 +74,7 @@ final class UploadCallable implements Callable<UploadResult> {
           .setUploadedBlob(newBlob.get())
           .build();
     } catch (Exception e) {
-      if (bytesCopied == 0) {
+      if (bytesCopied == -1) {
         return UploadResult.newBuilder(originalBlob, TransferStatus.FAILED_TO_START)
             .setException(e)
             .build();


Test patch:
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITTransferManagerTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITTransferManagerTest.java
index ab3f2f49..21ed72ed 100644
--- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITTransferManagerTest.java
+++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITTransferManagerTest.java
@@ -253,6 +253,53 @@ public class ITTransferManagerTest {
     }
   }
 
+  @Test
+  public void downloadNonexistentBucket() throws Exception {
+    TransferManagerConfig config =
+        TransferManagerConfigTestingInstances.defaults(storage.getOptions());
+    try (TransferManager transferManager = config.getService()) {
+      String bucketName = bucket.getName() + "-does-not-exist";
+      ParallelDownloadConfig parallelDownloadConfig =
+          ParallelDownloadConfig.newBuilder()
+              .setBucketName(bucketName)
+              .setDownloadDirectory(baseDir)
+              .build();
+      DownloadJob job = transferManager.downloadBlobs(blobs, parallelDownloadConfig);
+      List<DownloadResult> downloadResults = ApiFutures.allAsList(job.getDownloadResults()).get();
+      List<DownloadResult> failedToStart =
+          downloadResults.stream()
+              .filter(x -> x.getStatus() == TransferStatus.FAILED_TO_START)
+              .collect(Collectors.toList());
+      assertThat(failedToStart).hasSize(3);
+    }
+  }
+
+  @Test
+  public void downloadBlobsChunkedFail() throws Exception {
+    TransferManagerConfig config =
+        TransferManagerConfigTestingInstances.defaults(storage.getOptions())
+            .toBuilder()
+            .setAllowDivideAndConquer(true)
+            .setPerWorkerBufferSize(128 * 1024)
+            .build();
+    try (TransferManager transferManager = config.getService()) {
+      String bucketName = bucket.getName() + "-does-not-exist";
+      ParallelDownloadConfig parallelDownloadConfig =
+          ParallelDownloadConfig.newBuilder()
+              .setBucketName(bucketName)
+              .setDownloadDirectory(baseDir)
+              .build();
+      DownloadJob job = transferManager.downloadBlobs(blobs, parallelDownloadConfig);
+      List<DownloadResult> downloadResults = ApiFutures.allAsList(job.getDownloadResults()).get();
+      assertThat(downloadResults).hasSize(3);
+      List<DownloadResult> failedToStart =
+          downloadResults.stream()
+              .filter(x -> x.getStatus() == TransferStatus.FAILED_TO_START)
+              .collect(Collectors.toList());
+      assertThat(failedToStart).hasSize(3);
+    }
+  }
+
   private void cleanUpFiles(List<DownloadResult> results) throws IOException {
     // Cleanup downloaded blobs and the parent directory
     for (DownloadResult res : results) {