From 76cb620fc814083926e03bd981f7aefbf316f4ff Mon Sep 17 00:00:00 2001 From: Marco Ziccardi Date: Tue, 17 Nov 2015 15:30:39 +0100 Subject: [PATCH 1/3] Better document delete methods --- .../main/java/com/google/gcloud/storage/Blob.java | 6 +++--- .../main/java/com/google/gcloud/storage/Bucket.java | 2 +- .../main/java/com/google/gcloud/storage/Storage.java | 12 ++++++------ 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/gcloud-java-storage/src/main/java/com/google/gcloud/storage/Blob.java b/gcloud-java-storage/src/main/java/com/google/gcloud/storage/Blob.java index a8e315be0e45..503cad361e29 100644 --- a/gcloud-java-storage/src/main/java/com/google/gcloud/storage/Blob.java +++ b/gcloud-java-storage/src/main/java/com/google/gcloud/storage/Blob.java @@ -281,7 +281,7 @@ public CopyWriter copyTo(BlobId targetBlob, BlobSourceOption... options) { * Deletes this blob. * * @param options blob delete options - * @return true if blob was deleted + * @return {@code true} if blob was deleted, {@code false} if it was not found * @throws StorageException upon failure */ public boolean delete(BlobSourceOption... options) { @@ -422,8 +422,8 @@ public Blob apply(BlobInfo f) { * @param storage the storage service used to issue the request * @param blobs the blobs to delete * @return an immutable list of booleans. If a blob has been deleted the corresponding item in the - * list is {@code true}. If deletion failed or access to the resource was denied the item is - * {@code false}. + * list is {@code true}. If a blob was not found, deletion failed or access to the resource + * was denied the corresponding item is {@code false}. * @throws StorageException upon failure */ public static List delete(Storage storage, BlobId... blobs) { diff --git a/gcloud-java-storage/src/main/java/com/google/gcloud/storage/Bucket.java b/gcloud-java-storage/src/main/java/com/google/gcloud/storage/Bucket.java index 21aafd92b5d4..54e60cc006dc 100644 --- a/gcloud-java-storage/src/main/java/com/google/gcloud/storage/Bucket.java +++ b/gcloud-java-storage/src/main/java/com/google/gcloud/storage/Bucket.java @@ -271,7 +271,7 @@ public Bucket update(BucketInfo bucketInfo, BucketTargetOption... options) { * Deletes this bucket. * * @param options bucket delete options - * @return true if bucket was deleted + * @return {@code true} if bucket was deleted, {@code false} if it was not found * @throws StorageException upon failure */ public boolean delete(BucketSourceOption... options) { diff --git a/gcloud-java-storage/src/main/java/com/google/gcloud/storage/Storage.java b/gcloud-java-storage/src/main/java/com/google/gcloud/storage/Storage.java index b61cfb269c1a..1fe096524ab7 100644 --- a/gcloud-java-storage/src/main/java/com/google/gcloud/storage/Storage.java +++ b/gcloud-java-storage/src/main/java/com/google/gcloud/storage/Storage.java @@ -1316,7 +1316,7 @@ private static void checkContentType(BlobInfo blobInfo) throws IllegalArgumentEx /** * Delete the requested bucket. * - * @return true if bucket was deleted + * @return {@code true} if bucket was deleted, {@code false} if it was not found * @throws StorageException upon failure */ boolean delete(String bucket, BucketSourceOption... options); @@ -1324,7 +1324,7 @@ private static void checkContentType(BlobInfo blobInfo) throws IllegalArgumentEx /** * Delete the requested blob. * - * @return true if blob was deleted + * @return {@code true} if blob was deleted, {@code false} if it was not found * @throws StorageException upon failure */ boolean delete(String bucket, String blob, BlobSourceOption... options); @@ -1332,7 +1332,7 @@ private static void checkContentType(BlobInfo blobInfo) throws IllegalArgumentEx /** * Delete the requested blob. * - * @return true if blob was deleted + * @return {@code true} if blob was deleted, {@code false} if it was not found * @throws StorageException upon failure */ boolean delete(BlobId blob, BlobSourceOption... options); @@ -1340,7 +1340,7 @@ private static void checkContentType(BlobInfo blobInfo) throws IllegalArgumentEx /** * Delete the requested blob. * - * @return true if blob was deleted + * @return {@code true} if blob was deleted, {@code false} if it was not found * @throws StorageException upon failure */ boolean delete(BlobId blob); @@ -1478,8 +1478,8 @@ private static void checkContentType(BlobInfo blobInfo) throws IllegalArgumentEx * * @param blobIds blobs to delete * @return an immutable list of booleans. If a blob has been deleted the corresponding item in the - * list is {@code true}. If deletion failed or access to the resource was denied the item is - * {@code false}. + * list is {@code true}. If a blob was not found, deletion failed or access to the resource + * was denied the corresponding item is {@code false}. * @throws StorageException upon failure */ List delete(BlobId... blobIds); From e22e8a3e6d6052c831f9e9f4237f428a7bb7558a Mon Sep 17 00:00:00 2001 From: Marco Ziccardi Date: Tue, 17 Nov 2015 16:55:42 +0100 Subject: [PATCH 2/3] Make batch delete return false if not found --- .../main/java/com/google/gcloud/spi/DefaultStorageRpc.java | 6 +++++- .../test/java/com/google/gcloud/storage/ITStorageTest.java | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/gcloud-java-storage/src/main/java/com/google/gcloud/spi/DefaultStorageRpc.java b/gcloud-java-storage/src/main/java/com/google/gcloud/spi/DefaultStorageRpc.java index 55bca2319c2d..1021a58f64fe 100644 --- a/gcloud-java-storage/src/main/java/com/google/gcloud/spi/DefaultStorageRpc.java +++ b/gcloud-java-storage/src/main/java/com/google/gcloud/spi/DefaultStorageRpc.java @@ -368,7 +368,11 @@ public void onSuccess(Void ignore, HttpHeaders responseHeaders) { @Override public void onFailure(GoogleJsonError e, HttpHeaders responseHeaders) { - deletes.put(tuple.x(), Tuple.of(null, translate(e))); + if (e.getCode() == 404) { + deletes.put(tuple.x(), Tuple.of(Boolean.FALSE, null)); + } else { + deletes.put(tuple.x(), Tuple.of(null, translate(e))); + } } }); } diff --git a/gcloud-java-storage/src/test/java/com/google/gcloud/storage/ITStorageTest.java b/gcloud-java-storage/src/test/java/com/google/gcloud/storage/ITStorageTest.java index d22bf06c8b16..0d4e876271e3 100644 --- a/gcloud-java-storage/src/test/java/com/google/gcloud/storage/ITStorageTest.java +++ b/gcloud-java-storage/src/test/java/com/google/gcloud/storage/ITStorageTest.java @@ -644,7 +644,8 @@ public void testBatchRequestFail() { assertFalse(batchResponse.gets().get(1).failed()); assertNull(batchResponse.gets().get(1).get()); assertTrue(batchResponse.deletes().get(0).failed()); - assertTrue(batchResponse.deletes().get(1).failed()); + assertFalse(batchResponse.deletes().get(1).failed()); + assertFalse(batchResponse.deletes().get(1).get()); assertTrue(storage.delete(BUCKET, blobName)); } From 1e7fe54e8cef571608ab4992c2769b5cad5c615b Mon Sep 17 00:00:00 2001 From: Marco Ziccardi Date: Wed, 18 Nov 2015 10:16:37 +0100 Subject: [PATCH 3/3] Move all checks for HTTP_NOT_FOUND to DefaultStorageRpc --- .../google/gcloud/spi/DefaultStorageRpc.java | 31 ++++++++++---- .../com/google/gcloud/spi/StorageRpc.java | 10 +++++ .../google/gcloud/storage/StorageImpl.java | 40 +++++-------------- 3 files changed, 43 insertions(+), 38 deletions(-) diff --git a/gcloud-java-storage/src/main/java/com/google/gcloud/spi/DefaultStorageRpc.java b/gcloud-java-storage/src/main/java/com/google/gcloud/spi/DefaultStorageRpc.java index 1021a58f64fe..0bd80a9c11ce 100644 --- a/gcloud-java-storage/src/main/java/com/google/gcloud/spi/DefaultStorageRpc.java +++ b/gcloud-java-storage/src/main/java/com/google/gcloud/spi/DefaultStorageRpc.java @@ -30,6 +30,7 @@ import static com.google.gcloud.spi.StorageRpc.Option.PREDEFINED_DEFAULT_OBJECT_ACL; import static com.google.gcloud.spi.StorageRpc.Option.PREFIX; import static com.google.gcloud.spi.StorageRpc.Option.VERSIONS; +import static java.net.HttpURLConnection.HTTP_NOT_FOUND; import com.google.api.client.googleapis.batch.json.JsonBatchCallback; import com.google.api.client.googleapis.json.GoogleJsonError; @@ -94,7 +95,8 @@ public DefaultStorageRpc(StorageOptions options) { private static StorageException translate(IOException exception) { StorageException translated; - if (exception instanceof GoogleJsonResponseException) { + if (exception instanceof GoogleJsonResponseException + && ((GoogleJsonResponseException) exception).getDetails() != null) { translated = translate(((GoogleJsonResponseException) exception).getDetails()); } else { translated = new StorageException(0, exception.getMessage(), false); @@ -191,7 +193,11 @@ public Bucket get(Bucket bucket, Map options) { .setFields(FIELDS.getString(options)) .execute(); } catch (IOException ex) { - throw translate(ex); + StorageException serviceException = translate(ex); + if (serviceException.code() == HTTP_NOT_FOUND) { + return null; + } + throw serviceException; } } @@ -200,7 +206,11 @@ public StorageObject get(StorageObject object, Map options) { try { return getRequest(object, options).execute(); } catch (IOException ex) { - throw translate(ex); + StorageException serviceException = translate(ex); + if (serviceException.code() == HTTP_NOT_FOUND) { + return null; + } + throw serviceException; } } @@ -265,7 +275,7 @@ public boolean delete(Bucket bucket, Map options) { return true; } catch (IOException ex) { StorageException serviceException = translate(ex); - if (serviceException.code() == 404) { + if (serviceException.code() == HTTP_NOT_FOUND) { return false; } throw serviceException; @@ -279,7 +289,7 @@ public boolean delete(StorageObject blob, Map options) { return true; } catch (IOException ex) { StorageException serviceException = translate(ex); - if (serviceException.code() == 404) { + if (serviceException.code() == HTTP_NOT_FOUND) { return false; } throw serviceException; @@ -368,7 +378,7 @@ public void onSuccess(Void ignore, HttpHeaders responseHeaders) { @Override public void onFailure(GoogleJsonError e, HttpHeaders responseHeaders) { - if (e.getCode() == 404) { + if (e.getCode() == HTTP_NOT_FOUND) { deletes.put(tuple.x(), Tuple.of(Boolean.FALSE, null)); } else { deletes.put(tuple.x(), Tuple.of(null, translate(e))); @@ -401,8 +411,13 @@ public void onSuccess(StorageObject storageObject, HttpHeaders responseHeaders) @Override public void onFailure(GoogleJsonError e, HttpHeaders responseHeaders) { - gets.put(tuple.x(), - Tuple.of(null, translate(e))); + if (e.getCode() == HTTP_NOT_FOUND) { + gets.put(tuple.x(), + Tuple.of(null, null)); + } else { + gets.put(tuple.x(), + Tuple.of(null, translate(e))); + } } }); } diff --git a/gcloud-java-storage/src/main/java/com/google/gcloud/spi/StorageRpc.java b/gcloud-java-storage/src/main/java/com/google/gcloud/spi/StorageRpc.java index e4b1be785951..1f708c269ad0 100644 --- a/gcloud-java-storage/src/main/java/com/google/gcloud/spi/StorageRpc.java +++ b/gcloud-java-storage/src/main/java/com/google/gcloud/spi/StorageRpc.java @@ -227,8 +227,18 @@ StorageObject create(StorageObject object, InputStream content, Map o Tuple> list(String bucket, Map options) throws StorageException; + /** + * Returns the requested bucket or {@code null} if not found. + * + * @throws StorageException upon failure + */ Bucket get(Bucket bucket, Map options) throws StorageException; + /** + * Returns the requested storage object or {@code null} if not found. + * + * @throws StorageException upon failure + */ StorageObject get(StorageObject object, Map options) throws StorageException; diff --git a/gcloud-java-storage/src/main/java/com/google/gcloud/storage/StorageImpl.java b/gcloud-java-storage/src/main/java/com/google/gcloud/storage/StorageImpl.java index fa059254eddb..0f8d9124a4cf 100644 --- a/gcloud-java-storage/src/main/java/com/google/gcloud/storage/StorageImpl.java +++ b/gcloud-java-storage/src/main/java/com/google/gcloud/storage/StorageImpl.java @@ -28,7 +28,6 @@ import static com.google.gcloud.spi.StorageRpc.Option.IF_SOURCE_GENERATION_NOT_MATCH; import static com.google.gcloud.spi.StorageRpc.Option.IF_SOURCE_METAGENERATION_MATCH; import static com.google.gcloud.spi.StorageRpc.Option.IF_SOURCE_METAGENERATION_NOT_MATCH; -import static java.net.HttpURLConnection.HTTP_NOT_FOUND; import static java.nio.charset.StandardCharsets.UTF_8; import com.google.api.services.storage.model.StorageObject; @@ -175,14 +174,7 @@ public BucketInfo get(String bucket, BucketGetOption... options) { new Callable() { @Override public com.google.api.services.storage.model.Bucket call() { - try { - return storageRpc.get(bucketPb, optionsMap); - } catch (StorageException ex) { - if (ex.code() == HTTP_NOT_FOUND) { - return null; - } - throw ex; - } + return storageRpc.get(bucketPb, optionsMap); } }, options().retryParams(), EXCEPTION_HANDLER); return answer == null ? null : BucketInfo.fromPb(answer); @@ -204,14 +196,7 @@ public BlobInfo get(BlobId blob, BlobGetOption... options) { StorageObject storageObject = runWithRetries(new Callable() { @Override public StorageObject call() { - try { - return storageRpc.get(storedObject, optionsMap); - } catch (StorageException ex) { - if (ex.code() == HTTP_NOT_FOUND) { - return null; - } - throw ex; - } + return storageRpc.get(storedObject, optionsMap); } }, options().retryParams(), EXCEPTION_HANDLER); return storageObject == null ? null : BlobInfo.fromPb(storageObject); @@ -523,28 +508,23 @@ public BatchResponse apply(BatchRequest batchRequest) { List> updates = transformBatchResult( toUpdate, response.updates, BlobInfo.FROM_PB_FUNCTION); List> gets = transformBatchResult( - toGet, response.gets, BlobInfo.FROM_PB_FUNCTION, HTTP_NOT_FOUND); + toGet, response.gets, BlobInfo.FROM_PB_FUNCTION); return new BatchResponse(deletes, updates, gets); } private List> transformBatchResult( Iterable>> request, - Map> results, Function transform, - int... nullOnErrorCodes) { - Set nullOnErrorCodesSet = Sets.newHashSet(Ints.asList(nullOnErrorCodes)); + Map> results, Function transform) { List> response = Lists.newArrayListWithCapacity(results.size()); for (Tuple tuple : request) { Tuple result = results.get(tuple.x()); - if (result.x() != null) { - response.add(BatchResponse.Result.of(transform.apply(result.x()))); + I object = result.x(); + StorageException exception = result.y(); + if (exception != null) { + response.add(new BatchResponse.Result(exception)); } else { - StorageException exception = result.y(); - if (nullOnErrorCodesSet.contains(exception.code())) { - //noinspection unchecked - response.add(BatchResponse.Result.empty()); - } else { - response.add(new BatchResponse.Result(exception)); - } + response.add(object != null ? + BatchResponse.Result.of(transform.apply(object)) : BatchResponse.Result.empty()); } } return response;