fix(grpc): fix bucket logging conversion to allow clearing (#1822)

While re-enabling several gRPC excluded tests after backend fixes clearing a buckets logging config via gRPC wasn't working. * test: re-enable several gRPC excluded tests after backend fixes
googleapis · Dec 21, 2022 · 30e19dc · 30e19dc
1 parent 94cd288
commit 30e19dc
Show file tree

Hide file tree

Showing 4 changed files with 18 additions and 36 deletions.
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcConversions.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcConversions.java
@@ -33,6 +33,7 @@
 import com.google.cloud.storage.BucketInfo.CustomPlacementConfig;
 import com.google.cloud.storage.BucketInfo.LifecycleRule;
 import com.google.cloud.storage.BucketInfo.LifecycleRule.AbortIncompleteMPUAction;
+import com.google.cloud.storage.BucketInfo.Logging;
 import com.google.cloud.storage.BucketInfo.PublicAccessPrevention;
 import com.google.cloud.storage.Conversions.Codec;
 import com.google.cloud.storage.HmacKey.HmacKeyState;
@@ -261,7 +262,10 @@ private BucketInfo bucketInfoDecode(Bucket from) {
       to.setCors(toImmutableListOf(corsCodec::decode).apply(corsList));
     }
     if (from.hasLogging()) {
-      to.setLogging(loggingCodec.decode(from.getLogging()));
+      Bucket.Logging logging = from.getLogging();
+      if (!logging.getLogBucket().isEmpty() || !logging.getLogObjectPrefix().isEmpty()) {
+        to.setLogging(loggingCodec.decode(logging));
+      }
     }
     if (from.hasOwner()) {
       to.setOwner(entityCodec.decode(from.getOwner().getEntity()));
@@ -357,7 +361,16 @@ private Bucket bucketInfoEncode(BucketInfo from) {
       }
       to.setLifecycle(lifecycleBuilder.build());
     }
-    ifNonNull(from.getLogging(), loggingCodec::encode, to::setLogging);
+
+    Logging logging = from.getLogging();
+    if (logging != null) {
+      // an empty bucket name is invalid, don't even attempt to encode if neither name or prefix
+      // are both empty
+      if ((logging.getLogBucket() != null && !logging.getLogBucket().isEmpty())
+          || (logging.getLogObjectPrefix() != null && !logging.getLogObjectPrefix().isEmpty())) {
+        to.setLogging(loggingCodec.encode(logging));
+      }
+    }
     ifNonNull(from.getCors(), toImmutableListOf(corsCodec::encode), to::addAllCors);
     ifNonNull(
         from.getOwner(),

diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITBucketTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITBucketTest.java
@@ -107,8 +107,6 @@ public void testListBuckets() throws InterruptedException {
   }
 
   @Test
-  // FieldMask not supported in GRPC get currently.
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testGetBucketSelectedFields() {
     Bucket remoteBucket =
         storage.get(bucket.getName(), Storage.BucketGetOption.fields(BucketField.ID));
@@ -118,19 +116,14 @@ public void testGetBucketSelectedFields() {
   }
 
   @Test
-  // FieldMask not supported in GRPC currently.
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testGetBucketAllSelectedFields() {
     Bucket remoteBucket =
         storage.get(bucket.getName(), Storage.BucketGetOption.fields(BucketField.values()));
     assertEquals(bucket.getName(), remoteBucket.getName());
     assertNotNull(remoteBucket.getCreateTime());
-    assertNotNull(remoteBucket.getSelfLink());
   }
 
   @Test
-  // Cannot turn on for GRPC until b/246634709 is resolved, verified locally.
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testBucketLocationType() throws Exception {
     String bucketName = generator.randomBucketName();
     BucketInfo bucketInfo = BucketInfo.newBuilder(bucketName).setLocation("us").build();
@@ -143,8 +136,6 @@ public void testBucketLocationType() throws Exception {
   }
 
   @Test
-  // Cannot turn on for GRPC until creation bug b/246634709 is resolved, verified locally.
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testBucketCustomPlacmentConfigDualRegion() throws Exception {
     String bucketName = generator.randomBucketName();
     List<String> locations = new ArrayList<>();
@@ -167,8 +158,6 @@ public void testBucketCustomPlacmentConfigDualRegion() throws Exception {
   }
 
   @Test
-  // Cannot turn on until GRPC Update logic bug is fixed b/247133805
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testBucketLogging() throws Exception {
     String logsBucketName = generator.randomBucketName();
     String loggingBucketName = generator.randomBucketName();
@@ -210,8 +199,6 @@ public void testBucketLogging() throws Exception {
   }
 
   @Test
-  // GRPC Update logic bug b/247133805
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testRemoveBucketCORS() {
     String bucketName = generator.randomBucketName();
     List<Cors.Origin> origins = ImmutableList.of(Cors.Origin.of("http://cloud.google.com"));
@@ -260,8 +247,6 @@ public void testRemoveBucketCORS() {
   }
 
   @Test
-  // Cannot turn on for GRPC until b/246634709 is resolved, verified locally.
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testRpoConfig() {
     String rpoBucket = generator.randomBucketName();
     try {
@@ -383,7 +368,6 @@ public void testUpdateBucketRequesterPays() {
   }
 
   @Test
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testEnableDisableBucketDefaultEventBasedHold() {
     String bucketName = generator.randomBucketName();
     Bucket remoteBucket =

diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITKmsTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITKmsTest.java
@@ -430,9 +430,7 @@ public void testListBlobsKmsKeySelectedFields() throws InterruptedException {
   }
 
   @Test
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testRotateFromCustomerEncryptionToKmsKey() {
-    // Bucket attribute extration on allowlist bug b/246634709
     String sourceBlobName = "test-copy-blob-encryption-key-source";
     BlobId source = BlobId.of(bucket.getName(), sourceBlobName);
     ImmutableMap<String, String> metadata = ImmutableMap.of("k", "v");

diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITObjectTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITObjectTest.java
@@ -51,6 +51,7 @@
 import com.google.cloud.storage.TransportCompatibility.Transport;
 import com.google.cloud.storage.it.runner.StorageITRunner;
 import com.google.cloud.storage.it.runner.annotations.Backend;
+import com.google.cloud.storage.it.runner.annotations.BucketFixture;
 import com.google.cloud.storage.it.runner.annotations.BucketType;
 import com.google.cloud.storage.it.runner.annotations.CrossRun;
 import com.google.cloud.storage.it.runner.annotations.Inject;
@@ -109,11 +110,11 @@ public class ITObjectTest {
   @Rule public final TestName testName = new TestName();
 
   @Inject
-  @com.google.cloud.storage.it.runner.annotations.BucketFixture(BucketType.DEFAULT)
+  @BucketFixture(BucketType.DEFAULT)
   public BucketInfo bucket;
 
   @Inject
-  @com.google.cloud.storage.it.runner.annotations.BucketFixture(BucketType.REQUESTER_PAYS)
+  @BucketFixture(BucketType.REQUESTER_PAYS)
   public BucketInfo requesterPaysBucket;
 
   @Inject public Storage storage;
@@ -235,8 +236,6 @@ public void testCreateBlobFail() {
   }
 
   @Test
-  // FieldMask on get not supported by GRPC yet.
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testGetBlobEmptySelectedFields() {
 
     String blobName = "test-get-empty-selected-fields-blob";
@@ -248,8 +247,6 @@ public void testGetBlobEmptySelectedFields() {
   }
 
   @Test
-  // FieldMask on get not supported by GRPC yet.
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testGetBlobSelectedFields() {
 
     String blobName = "test-get-selected-fields-blob";
@@ -267,8 +264,6 @@ public void testGetBlobSelectedFields() {
   }
 
   @Test
-  // FieldMask on get not supported by GRPC yet.
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testGetBlobAllSelectedFields() {
 
     String blobName = "test-get-all-selected-fields-blob";
@@ -283,8 +278,6 @@ public void testGetBlobAllSelectedFields() {
     assertEquals(blob.getBucket(), remoteBlob.getBucket());
     assertEquals(blob.getName(), remoteBlob.getName());
     assertEquals(ImmutableMap.of("k", "v"), remoteBlob.getMetadata());
-    assertNotNull(remoteBlob.getGeneratedId());
-    assertNotNull(remoteBlob.getSelfLink());
   }
 
   @Test
@@ -318,8 +311,6 @@ public void testGetBlobFailNonExistingGeneration() {
   }
 
   @Test(timeout = 5000)
-  // FieldMask on get not supported by GRPC yet.
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testListBlobsSelectedFields() throws InterruptedException {
 
     String[] blobNames = {
@@ -367,8 +358,6 @@ public void testListBlobsSelectedFields() throws InterruptedException {
   }
 
   @Test(timeout = 5000)
-  // FieldMask on get not supported by GRPC yet.
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testListBlobsEmptySelectedFields() throws InterruptedException {
 
     String[] blobNames = {
@@ -1438,8 +1427,6 @@ public void testAutoContentTypeWriter() throws IOException {
   }
 
   @Test
-  // Bucket attribute extration on allowlist bug b/246634709
-  @CrossRun.Exclude(transports = Transport.GRPC)
   public void testBlobTimeStorageClassUpdated() {
 
     String blobName = "test-blob-with-storage-class";