From 2581f3cfff88ee6a1688ddb881baa30d9967b0c3 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Mon, 16 Mar 2020 18:15:29 -0700 Subject: [PATCH] fix: rely on google core for SSLException's (#188) * fix: rely on google core for SSLException's * update requester pays tests to not rely on Project Owner role * format * make sure requester pays is cleaned up --- .../cloud/storage/StorageException.java | 8 +-- .../storage/testing/RemoteStorageHelper.java | 11 ++-- .../cloud/storage/StorageExceptionTest.java | 16 ++++- .../cloud/storage/it/ITStorageTest.java | 66 ++++++++++++++++--- pom.xml | 2 +- 5 files changed, 80 insertions(+), 23 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java index 1f7401947..f4b8d0c42 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageException.java @@ -35,7 +35,6 @@ public final class StorageException extends BaseHttpServiceException { private static final String INTERNAL_ERROR = "internalError"; private static final String CONNECTION_CLOSED_PREMATURELY = "connectionClosedPrematurely"; - private static final String CONNECTION_RESET = "connectionReset"; // see: https://cloud.google.com/storage/docs/resumable-uploads-xml#practices private static final Set RETRYABLE_ERRORS = @@ -47,8 +46,7 @@ public final class StorageException extends BaseHttpServiceException { new Error(429, null), new Error(408, null), new Error(null, INTERNAL_ERROR), - new Error(null, CONNECTION_CLOSED_PREMATURELY), - new Error(null, CONNECTION_RESET)); + new Error(null, CONNECTION_CLOSED_PREMATURELY)); private static final long serialVersionUID = -4168430271327813063L; @@ -86,7 +84,7 @@ public static StorageException translateAndThrow(RetryHelperException ex) { /** * Translate IOException to a StorageException representing the cause of the error. This method * defaults to idempotent always being {@code true}. Additionally, this method translates - * transient issues Connection Closed Prematurely and Connection Reset as retryable errors. + * transient issues Connection Closed Prematurely as a retryable error. * * @returns {@code StorageException} */ @@ -94,8 +92,6 @@ public static StorageException translate(IOException exception) { if (exception.getMessage().contains("Connection closed prematurely")) { return new StorageException( 0, exception.getMessage(), CONNECTION_CLOSED_PREMATURELY, exception); - } else if (exception.getMessage().contains("Connection reset")) { - return new StorageException(0, exception.getMessage(), CONNECTION_RESET, exception); } else { // default return new StorageException(exception); diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/RemoteStorageHelper.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/RemoteStorageHelper.java index 299ac0395..eb18e4a7c 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/RemoteStorageHelper.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/testing/RemoteStorageHelper.java @@ -75,7 +75,9 @@ public static void cleanBuckets(final Storage storage, final long olderThan, lon @Override public void run() { Page buckets = - storage.list(Storage.BucketListOption.prefix(BUCKET_NAME_PREFIX)); + storage.list( + Storage.BucketListOption.prefix(BUCKET_NAME_PREFIX), + Storage.BucketListOption.userProject(storage.getOptions().getProjectId())); for (Bucket bucket : buckets.iterateAll()) { if (bucket.getCreateTime() < olderThan) { try { @@ -88,10 +90,9 @@ public void run() { .iterateAll()) { if (blob.getEventBasedHold() == true || blob.getTemporaryHold() == true) { storage.update( - blob.toBuilder() - .setTemporaryHold(false) - .setEventBasedHold(false) - .build()); + blob.toBuilder().setTemporaryHold(false).setEventBasedHold(false).build(), + Storage.BlobTargetOption.userProject( + storage.getOptions().getProjectId())); } } forceDelete(storage, bucket.getName()); diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageExceptionTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageExceptionTest.java index e065f4016..0051cac49 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageExceptionTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageExceptionTest.java @@ -141,11 +141,23 @@ public void testStorageException() { public void testTranslateConnectionReset() { StorageException exception = StorageException.translate( - new IOException( + new SSLException( "Connection has been shutdown: " + new SSLException(new SocketException("Connection reset")))); assertEquals(0, exception.getCode()); - assertEquals("connectionReset", exception.getReason()); + assertTrue(exception.isRetryable()); + } + + @Test + public void testTranslateConnectionShutdown() { + StorageException exception = + StorageException.translate( + new SSLException( + "Connection has been shutdown: " + + new SSLException(new SocketException("Socket closed")))); + String test = exception.getMessage(); + + assertEquals(0, exception.getCode()); assertTrue(exception.isRetryable()); } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java index 31dc630f0..dd47e7443 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java @@ -126,6 +126,7 @@ import org.apache.http.impl.client.HttpClients; import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; import org.junit.AfterClass; +import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; @@ -192,6 +193,23 @@ public static void beforeClass() throws IOException { prepareKmsKeys(); } + @Before + public void beforeEach() { + Bucket remoteBucket = + storage.get( + BUCKET, + Storage.BucketGetOption.fields(BucketField.ID, BucketField.BILLING), + Storage.BucketGetOption.userProject(storage.getOptions().getProjectId())); + // Disable requester pays in case a test fails to clean up. + if (remoteBucket.requesterPays() != null && remoteBucket.requesterPays() == true) { + remoteBucket + .toBuilder() + .setRequesterPays(false) + .build() + .update(Storage.BucketTargetOption.userProject(storage.getOptions().getProjectId())); + } + } + @AfterClass public static void afterClass() throws ExecutionException, InterruptedException { if (storage != null) { @@ -867,8 +885,9 @@ public void testListBlobRequesterPays() throws InterruptedException { assertNotNull(storage.create(blob1)); // Test listing a Requester Pays bucket. - Bucket remoteBucket = storage.get(BUCKET, Storage.BucketGetOption.fields(BucketField.ID)); - assertNull(remoteBucket.requesterPays()); + Bucket remoteBucket = + storage.get(BUCKET, Storage.BucketGetOption.fields(BucketField.ID, BucketField.BILLING)); + assertFalse(remoteBucket.requesterPays()); remoteBucket = remoteBucket.toBuilder().setRequesterPays(true).build(); Bucket updatedBucket = storage.update(remoteBucket); assertTrue(updatedBucket.requesterPays()); @@ -2138,7 +2157,8 @@ public void testBucketAcl() { private void testBucketAclRequesterPays(boolean requesterPays) { if (requesterPays) { - Bucket remoteBucket = storage.get(BUCKET, Storage.BucketGetOption.fields(BucketField.ID)); + Bucket remoteBucket = + storage.get(BUCKET, Storage.BucketGetOption.fields(BucketField.ID, BucketField.BILLING)); assertNull(remoteBucket.requesterPays()); remoteBucket = remoteBucket.toBuilder().setRequesterPays(true).build(); Bucket updatedBucket = storage.update(remoteBucket); @@ -2164,6 +2184,18 @@ private void testBucketAclRequesterPays(boolean requesterPays) { assertTrue(acls.contains(updatedAcl)); assertTrue(storage.deleteAcl(BUCKET, User.ofAllAuthenticatedUsers(), bucketOptions)); assertNull(storage.getAcl(BUCKET, User.ofAllAuthenticatedUsers(), bucketOptions)); + if (requesterPays) { + Bucket remoteBucket = + storage.get( + BUCKET, + Storage.BucketGetOption.fields(BucketField.ID, BucketField.BILLING), + Storage.BucketGetOption.userProject(projectId)); + assertTrue(remoteBucket.requesterPays()); + remoteBucket = remoteBucket.toBuilder().setRequesterPays(false).build(); + Bucket updatedBucket = + storage.update(remoteBucket, Storage.BucketTargetOption.userProject(projectId)); + assertFalse(updatedBucket.requesterPays()); + } } @Test @@ -2347,8 +2379,9 @@ public void testReadCompressedBlob() throws IOException { @Test public void testBucketPolicyV1RequesterPays() { - Bucket remoteBucket = storage.get(BUCKET, Storage.BucketGetOption.fields(BucketField.ID)); - assertNull(remoteBucket.requesterPays()); + Bucket remoteBucket = + storage.get(BUCKET, Storage.BucketGetOption.fields(BucketField.ID, BucketField.BILLING)); + assertFalse(remoteBucket.requesterPays()); remoteBucket = remoteBucket.toBuilder().setRequesterPays(true).build(); Bucket updatedBucket = storage.update(remoteBucket); assertTrue(updatedBucket.requesterPays()); @@ -2407,6 +2440,9 @@ public void testBucketPolicyV1RequesterPays() { BUCKET, ImmutableList.of("storage.buckets.getIamPolicy", "storage.buckets.setIamPolicy"), bucketOptions)); + remoteBucket = remoteBucket.toBuilder().setRequesterPays(false).build(); + updatedBucket = storage.update(remoteBucket, Storage.BucketTargetOption.userProject(projectId)); + assertFalse(updatedBucket.requesterPays()); } @Test @@ -2624,7 +2660,8 @@ public void testBucketPolicyV3() { @Test public void testUpdateBucketLabel() { - Bucket remoteBucket = storage.get(BUCKET, Storage.BucketGetOption.fields(BucketField.ID)); + Bucket remoteBucket = + storage.get(BUCKET, Storage.BucketGetOption.fields(BucketField.ID, BucketField.BILLING)); assertNull(remoteBucket.getLabels()); remoteBucket = remoteBucket.toBuilder().setLabels(BUCKET_LABELS).build(); Bucket updatedBucket = storage.update(remoteBucket); @@ -2635,8 +2672,9 @@ public void testUpdateBucketLabel() { @Test public void testUpdateBucketRequesterPays() { - Bucket remoteBucket = storage.get(BUCKET, Storage.BucketGetOption.fields(BucketField.ID)); - assertNull(remoteBucket.requesterPays()); + Bucket remoteBucket = + storage.get(BUCKET, Storage.BucketGetOption.fields(BucketField.ID, BucketField.BILLING)); + assertFalse(remoteBucket.requesterPays()); remoteBucket = remoteBucket.toBuilder().setRequesterPays(true).build(); Bucket updatedBucket = storage.update(remoteBucket); assertTrue(updatedBucket.requesterPays()); @@ -2646,8 +2684,12 @@ public void testUpdateBucketRequesterPays() { String blobName = "test-create-empty-blob-requester-pays"; Blob remoteBlob = updatedBucket.create(blobName, BLOB_BYTE_CONTENT, option); assertNotNull(remoteBlob); - byte[] readBytes = storage.readAllBytes(BUCKET, blobName); + byte[] readBytes = + storage.readAllBytes(BUCKET, blobName, Storage.BlobSourceOption.userProject(projectId)); assertArrayEquals(BLOB_BYTE_CONTENT, readBytes); + remoteBucket = remoteBucket.toBuilder().setRequesterPays(false).build(); + updatedBucket = storage.update(remoteBucket, Storage.BucketTargetOption.userProject(projectId)); + assertFalse(updatedBucket.requesterPays()); } @Test @@ -2786,6 +2828,12 @@ private void retentionPolicyLockRequesterPays(boolean requesterPays) assertTrue(remoteBucket.retentionPolicyIsLocked()); assertNotNull(remoteBucket.getRetentionEffectiveTime()); } finally { + if (requesterPays) { + bucketInfo = bucketInfo.toBuilder().setRequesterPays(false).build(); + Bucket updateBucket = + storage.update(bucketInfo, Storage.BucketTargetOption.userProject(projectId)); + assertFalse(updateBucket.requesterPays()); + } RemoteStorageHelper.forceDelete(storage, bucketName, 5, TimeUnit.SECONDS); } } diff --git a/pom.xml b/pom.xml index fe80a8a9a..cd36c389e 100644 --- a/pom.xml +++ b/pom.xml @@ -63,7 +63,7 @@ UTF-8 github google-cloud-storage-parent - 1.93.2 + 1.93.3 1.8.1 4.13 1.4.1