Repository: googleapis/java-storage (java)

Issue:
Jackson usage is hardcoded in HttpStorageRpc
At https://github.com/googleapis/java-storage/blob/main/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java#L125

`new JacksonFactory()` is hardcoded, making it impossible to switch it out for `GsonFactory` and making it impossible to exclude Jackson as a dependency of `google-cloud-storage`.

Pull request:
feat: add constructor parameter for JsonFactory to HttpStorageRpc
Fixes #1584


Patch:
diff --git a/google-cloud-storage/pom.xml b/google-cloud-storage/pom.xml
index 5ef4cf8a..c27c7269 100644
--- a/google-cloud-storage/pom.xml
+++ b/google-cloud-storage/pom.xml
@@ -32,6 +32,11 @@
       <groupId>com.google.http-client</groupId>
       <artifactId>google-http-client-jackson2</artifactId>
     </dependency>
+    <dependency>
+      <groupId>com.google.http-client</groupId>
+      <artifactId>google-http-client-gson</artifactId>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <groupId>com.google.api-client</groupId>
       <artifactId>google-api-client</artifactId>
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
index 58cfc9cb..b83c6a50 100644
--- 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
@@ -29,6 +29,7 @@ import com.google.cloud.storage.Acl.Role;
 import com.google.cloud.storage.BlobInfo.CustomerEncryption;
 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.PublicAccessPrevention;
 import com.google.cloud.storage.Conversions.Codec;
 import com.google.cloud.storage.HmacKey.HmacKeyState;
@@ -589,6 +590,10 @@ final class GrpcConversions {
             BucketInfo.LifecycleRule.LifecycleAction.newSetStorageClassAction(
                 StorageClass.valueOf(action.getStorageClass()));
         break;
+      case AbortIncompleteMPUAction.TYPE:
+        lifecycleAction =
+            BucketInfo.LifecycleRule.LifecycleAction.newAbortIncompleteMPUploadAction();
+        break;
       default:
         BucketInfo.log.warning(
             "The lifecycle action "
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java
index 97d7d4c5..893c5a42 100644
--- a/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java
+++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java
@@ -62,6 +62,7 @@ import com.google.cloud.storage.UnifiedOpts.ObjectSourceOpt;
 import com.google.cloud.storage.UnifiedOpts.ObjectTargetOpt;
 import com.google.cloud.storage.UnifiedOpts.Opts;
 import com.google.cloud.storage.UnifiedOpts.ProjectId;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.io.BaseEncoding;
 import com.google.common.io.ByteStreams;
@@ -458,7 +459,7 @@ final class GrpcStorageImpl extends BaseService<StorageOptions> implements Stora
     UpdateBucketRequest req =
         opts.updateBucketsRequest()
             .apply(builder)
-            .setUpdateMask(fieldMaskGenerator(bucket))
+            .setUpdateMask(updateMaskGenerator(bucket))
             .build();
 
     return Retrying.run(
@@ -478,7 +479,7 @@ final class GrpcStorageImpl extends BaseService<StorageOptions> implements Stora
     UpdateObjectRequest req =
         opts.updateObjectsRequest()
             .apply(builder)
-            .setUpdateMask(fieldMaskGenerator(object))
+            .setUpdateMask(updateMaskGenerator(object))
             .build();
     return Retrying.run(
         getOptions(),
@@ -1294,12 +1295,14 @@ final class GrpcStorageImpl extends BaseService<StorageOptions> implements Stora
         .build();
   }
 
-  private FieldMask fieldMaskGenerator(Message message) {
+  private FieldMask updateMaskGenerator(Message message) {
+    // TODO: Filter based on API behavior and not a list
     return FieldMask.newBuilder()
         .addAllPaths(
             message.getAllFields().entrySet().stream()
                 .filter(x -> x.getValue() != null)
                 .map(e -> e.getKey().getName())
+                .filter(f -> updateFields().contains(f))
                 .collect(Collectors.toList()))
         .build();
   }
@@ -1357,4 +1360,22 @@ final class GrpcStorageImpl extends BaseService<StorageOptions> implements Stora
         throw new IllegalStateException("Unrecognized status code: " + code);
     }
   }
+
+  private static ImmutableList<String> updateFields() {
+    return ImmutableList.of(
+        "cors",
+        "default_event_based_hold",
+        "retention_policy",
+        "versioning",
+        "billing",
+        "iam_config",
+        "encryption",
+        "lifecycle",
+        "logging",
+        "website",
+        "acl",
+        "default_object_acl",
+        "storage_class",
+        "rpo");
+  }
 }
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java
index 5f52b76f..9112da05 100644
--- a/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java
+++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java
@@ -111,6 +111,10 @@ public class HttpStorageRpc implements StorageRpc {
   private static final FileNameMap FILE_NAME_MAP = URLConnection.getFileNameMap();
 
   public HttpStorageRpc(StorageOptions options) {
+    this(options, new JacksonFactory());
+  }
+
+  public HttpStorageRpc(StorageOptions options, JsonFactory jsonFactory) {
     HttpTransportOptions transportOptions = (HttpTransportOptions) options.getTransportOptions();
     HttpTransport transport = transportOptions.getHttpTransportFactory().create();
     HttpRequestInitializer initializer = transportOptions.getHttpRequestInitializer(options);
@@ -122,7 +126,7 @@ public class HttpStorageRpc implements StorageRpc {
     initializer = new InvocationIdInitializer(initializer);
     batchRequestInitializer = censusHttpModule.getHttpRequestInitializer(null);
     storage =
-        new Storage.Builder(transport, new JacksonFactory(), initializer)
+        new Storage.Builder(transport, jsonFactory, initializer)
             .setRootUrl(options.getHost())
             .setApplicationName(options.getApplicationName())
             .build();


Test patch:
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITBucketLifecycleTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITBucketLifecycleTest.java
index 00d886af..508db717 100644
--- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITBucketLifecycleTest.java
+++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITBucketLifecycleTest.java
@@ -22,6 +22,7 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
+import com.google.cloud.NoCredentials;
 import com.google.cloud.storage.Bucket;
 import com.google.cloud.storage.BucketInfo;
 import com.google.cloud.storage.BucketInfo.LifecycleRule;
@@ -32,18 +33,39 @@ import com.google.cloud.storage.Storage;
 import com.google.cloud.storage.Storage.BucketField;
 import com.google.cloud.storage.StorageClass;
 import com.google.cloud.storage.StorageFixture;
+import com.google.cloud.storage.StorageOptions;
+import com.google.cloud.storage.conformance.retry.ParallelParameterized;
+import com.google.cloud.storage.conformance.retry.TestBench;
 import com.google.cloud.storage.testing.RemoteStorageHelper;
 import com.google.common.collect.ImmutableList;
 import java.time.OffsetDateTime;
+import java.util.Arrays;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
-import org.junit.BeforeClass;
 import org.junit.ClassRule;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized.Parameters;
 
+@RunWith(ParallelParameterized.class)
 public class ITBucketLifecycleTest {
 
-  @ClassRule public static final StorageFixture storageFixture = StorageFixture.defaultHttp();
+  @ClassRule(order = 0)
+  public static final TestBench TEST_BENCH =
+      TestBench.newBuilder().setContainerName("it-grpc").build();
+
+  @ClassRule(order = 1)
+  public static final StorageFixture storageFixtureGrpc =
+      StorageFixture.from(
+          () ->
+              StorageOptions.grpc()
+                  .setHost(TEST_BENCH.getGRPCBaseUri())
+                  .setCredentials(NoCredentials.getInstance())
+                  .setProjectId("test-project-id")
+                  .build());
+
+  @ClassRule(order = 1)
+  public static final StorageFixture storageFixtureHttp = StorageFixture.defaultHttp();
 
   private static final LifecycleRule LIFECYCLE_RULE_1 =
       new LifecycleRule(
@@ -60,37 +82,49 @@ public class ITBucketLifecycleTest {
   private static final ImmutableList<LifecycleRule> LIFECYCLE_RULES =
       ImmutableList.of(LIFECYCLE_RULE_1, LIFECYCLE_RULE_2);
 
-  private static Storage storage;
+  private final StorageFixture storageFixture;
+  private final String clientName;
 
-  @BeforeClass
-  public static void setup() {
-    storage = storageFixture.getInstance();
+  public ITBucketLifecycleTest(String clientName, StorageFixture storageFixture) {
+    this.clientName = clientName;
+    this.storageFixture = storageFixture;
+  }
+
+  @Parameters(name = "{0}")
+  public static Iterable<Object[]> data() {
+    return Arrays.asList(
+        new Object[] {"JSON/Prod", storageFixtureHttp},
+        new Object[] {"GRPC/TestBench", storageFixtureGrpc});
   }
 
   @Test
   public void testGetBucketLifecycleRules() {
     String lifecycleTestBucketName = RemoteStorageHelper.generateBucketName();
-    storage.create(
-        BucketInfo.newBuilder(lifecycleTestBucketName)
-            .setLocation("us")
-            .setLifecycleRules(
-                ImmutableList.of(
-                    new LifecycleRule(
-                        LifecycleAction.newSetStorageClassAction(StorageClass.COLDLINE),
-                        LifecycleCondition.newBuilder()
-                            .setAge(1)
-                            .setNumberOfNewerVersions(3)
-                            .setIsLive(false)
-                            .setCreatedBeforeOffsetDateTime(OffsetDateTime.now())
-                            .setMatchesStorageClass(ImmutableList.of(StorageClass.COLDLINE))
-                            .setDaysSinceNoncurrentTime(30)
-                            .setNoncurrentTimeBeforeOffsetDateTime(OffsetDateTime.now())
-                            .setCustomTimeBeforeOffsetDateTime(OffsetDateTime.now())
-                            .setDaysSinceCustomTime(30)
-                            .build())))
-            .build());
+    storageFixture
+        .getInstance()
+        .create(
+            BucketInfo.newBuilder(lifecycleTestBucketName)
+                .setLocation("us")
+                .setLifecycleRules(
+                    ImmutableList.of(
+                        new LifecycleRule(
+                            LifecycleAction.newSetStorageClassAction(StorageClass.COLDLINE),
+                            LifecycleCondition.newBuilder()
+                                .setAge(1)
+                                .setNumberOfNewerVersions(3)
+                                .setIsLive(false)
+                                .setCreatedBeforeOffsetDateTime(OffsetDateTime.now())
+                                .setMatchesStorageClass(ImmutableList.of(StorageClass.COLDLINE))
+                                .setDaysSinceNoncurrentTime(30)
+                                .setNoncurrentTimeBeforeOffsetDateTime(OffsetDateTime.now())
+                                .setCustomTimeBeforeOffsetDateTime(OffsetDateTime.now())
+                                .setDaysSinceCustomTime(30)
+                                .build())))
+                .build());
     Bucket remoteBucket =
-        storage.get(lifecycleTestBucketName, Storage.BucketGetOption.fields(BucketField.LIFECYCLE));
+        storageFixture
+            .getInstance()
+            .get(lifecycleTestBucketName, Storage.BucketGetOption.fields(BucketField.LIFECYCLE));
     LifecycleRule lifecycleRule = remoteBucket.getLifecycleRules().get(0);
     try {
       assertTrue(
@@ -108,30 +142,34 @@ public class ITBucketLifecycleTest {
       assertEquals(30, lifecycleRule.getCondition().getDaysSinceCustomTime().intValue());
       assertNotNull(lifecycleRule.getCondition().getCustomTimeBeforeOffsetDateTime());
     } finally {
-      storage.delete(lifecycleTestBucketName);
+      storageFixture.getInstance().delete(lifecycleTestBucketName);
     }
   }
 
   @Test
   public void testGetBucketAbortMPULifecycle() {
     String lifecycleTestBucketName = RemoteStorageHelper.generateBucketName();
-    storage.create(
-        BucketInfo.newBuilder(lifecycleTestBucketName)
-            .setLocation("us")
-            .setLifecycleRules(
-                ImmutableList.of(
-                    new LifecycleRule(
-                        LifecycleAction.newAbortIncompleteMPUploadAction(),
-                        LifecycleCondition.newBuilder().setAge(1).build())))
-            .build());
+    storageFixture
+        .getInstance()
+        .create(
+            BucketInfo.newBuilder(lifecycleTestBucketName)
+                .setLocation("us")
+                .setLifecycleRules(
+                    ImmutableList.of(
+                        new LifecycleRule(
+                            LifecycleAction.newAbortIncompleteMPUploadAction(),
+                            LifecycleCondition.newBuilder().setAge(1).build())))
+                .build());
     Bucket remoteBucket =
-        storage.get(lifecycleTestBucketName, Storage.BucketGetOption.fields(BucketField.LIFECYCLE));
+        storageFixture
+            .getInstance()
+            .get(lifecycleTestBucketName, Storage.BucketGetOption.fields(BucketField.LIFECYCLE));
     LifecycleRule lifecycleRule = remoteBucket.getLifecycleRules().get(0);
     try {
       assertEquals(AbortIncompleteMPUAction.TYPE, lifecycleRule.getAction().getActionType());
       assertEquals(1, lifecycleRule.getCondition().getAge().intValue());
     } finally {
-      storage.delete(lifecycleTestBucketName);
+      storageFixture.getInstance().delete(lifecycleTestBucketName);
     }
   }
 
@@ -139,18 +177,22 @@ public class ITBucketLifecycleTest {
   public void testDeleteLifecycleRules() throws ExecutionException, InterruptedException {
     String bucketName = RemoteStorageHelper.generateBucketName();
     Bucket bucket =
-        storage.create(
-            BucketInfo.newBuilder(bucketName)
-                .setLocation("us")
-                .setLifecycleRules(LIFECYCLE_RULES)
-                .build());
+        storageFixture
+            .getInstance()
+            .create(
+                BucketInfo.newBuilder(bucketName)
+                    .setLocation("us")
+                    .setLifecycleRules(LIFECYCLE_RULES)
+                    .build());
     assertThat(bucket.getLifecycleRules()).isNotNull();
     assertThat(bucket.getLifecycleRules()).hasSize(2);
     try {
-      Bucket updatedBucket = bucket.toBuilder().deleteLifecycleRules().build().update();
+      Bucket updatedBucket = bucket.toBuilder().deleteLifecycleRules().build();
+      storageFixture.getInstance().update(updatedBucket);
       assertThat(updatedBucket.getLifecycleRules()).hasSize(0);
     } finally {
-      RemoteStorageHelper.forceDelete(storage, bucketName, 5, TimeUnit.SECONDS);
+      RemoteStorageHelper.forceDelete(
+          storageFixture.getInstance(), bucketName, 5, TimeUnit.SECONDS);
     }
   }
 }
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITCustomJsonFactoryTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITCustomJsonFactoryTest.java
new file mode 100644
index 00000000..d009a89c
--- /dev/null
+++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITCustomJsonFactoryTest.java
@@ -0,0 +1,76 @@
+/*
+ * Copyright 2022 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.cloud.storage.it;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.atLeastOnce;
+import static org.mockito.Mockito.verify;
+
+import com.google.api.client.json.gson.GsonFactory;
+import com.google.api.gax.paging.Page;
+import com.google.cloud.storage.Bucket;
+import com.google.cloud.storage.HttpStorageOptions;
+import com.google.cloud.storage.Storage;
+import com.google.cloud.storage.Storage.BucketListOption;
+import com.google.cloud.storage.StorageOptions;
+import com.google.cloud.storage.spi.v1.HttpStorageRpc;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+
+/**
+ * Explicitly configure an alternative {@link com.google.api.client.json.JsonFactory} for use by an
+ * instance of {@link com.google.cloud.storage.Storage}
+ */
+@RunWith(MockitoJUnitRunner.class)
+public final class ITCustomJsonFactoryTest {
+
+  @Spy GsonFactory gsonFactory = new GsonFactory();
+
+  @Test
+  public void customJsonFactoryConfigurableViaStorageOptions() {
+    Storage s =
+        StorageOptions.newBuilder()
+            .setServiceRpcFactory(
+                options -> {
+                  if (options instanceof HttpStorageOptions) {
+                    HttpStorageOptions httpStorageOptions = (HttpStorageOptions) options;
+                    return new HttpStorageRpc(httpStorageOptions, gsonFactory);
+                  } else {
+                    throw new IllegalArgumentException("Only HttpStorageOptions supported");
+                  }
+                })
+            .build()
+            .getService();
+
+    Page<Bucket> bucketPage = s.list(BucketListOption.pageSize(10));
+    List<Bucket> buckets =
+        StreamSupport.stream(bucketPage.iterateAll().spliterator(), false)
+            .collect(Collectors.toList());
+    // if we reach here, we're good the call didn't fail, and we parsed some json.
+
+    verify(gsonFactory, atLeastOnce())
+        .createJsonParser(any(InputStream.class), eq(StandardCharsets.UTF_8));
+  }
+}