fix: Remove null check for paged Protobuf messages (#3164)

## Background This was caught from ErrorProne flagging an [ImpossibleNullComparison](https://errorprone.info/bugpattern/ImpossibleNullComparison) when upgrading to Protobuf 4.27.4 runtime. Protobuf messages should be non-null (with type specific default values) when they are serialized. Removing the null check that is being flagged from ErrorProne. ## Testing We have some issues with testing this inside Showcase. Protobuf serializes messages with default values and does not allow for null (will throw a NPE). Since showcase is configured to receive and send Protobuf Messages, we cannot configure the response to be return null values (at best we can return something like an empty string or an empty map). Instead, we have opted to create a sample Spring server to return a JSON payload with certain fields set to null. Create a GAPIC client and manually set the endpoint to point to the local server (i.e. `localhost:8080`). The Spring application will have a endpoint that matches the path of the RPC (i.e. `@GetMapping("/compute/v1/projects/{project}/zones/{zone}/instances")`) and returns the JSON. This proves that a null fields sent from the server is serialized properly into a Protobuf message and the message's fields are non-null. ``` @RestController public class ComputeController { @GetMapping("/compute/v1/projects/{project}/zones/{zone}/instances") String listInstances(@PathVariable String project, String zone) { return "{\"items\": null, \"id\": 5}"; } } ``` In the example above, the `items` field is a list and the printing it out in the client library will result in `[]`. Additionally, we have added some small, local tests against two repeated Protobuf messages: 1. BackendBucketList's items is a non-null List ``` JsonFormat.Parser parser = JsonFormat.parser().ignoringUnknownFields() .usingTypeRegistry(TypeRegistry.newBuilder().add(BackendBucketList.getDescriptor()).build()); BackendBucketList.Builder builder = BackendBucketList.newBuilder(); parser.merge("{\"items\": null}", builder); BackendBucketList list = builder.build(); System.out.println(list.getItemsList()); ``` 2. PredictResponse's metadata is a non-null Map ``` JsonFormat.Parser parser = JsonFormat.parser().ignoringUnknownFields() .usingTypeRegistry(TypeRegistry.newBuilder().add(PredictResponse.getDescriptor()).build());; PredictResponse.Builder builder = PredictResponse.newBuilder(); parser.merge("{\"recommendation_token\": \"343\"}", builder); System.out.println(builder.build().getMetadataMap()); ``` --------- Co-authored-by: cloud-java-bot <cloud-java-bot@google.com> Co-authored-by: cloud-java-bot <122572305+cloud-java-bot@users.noreply.github.com>
googleapis · Sep 5, 2024 · f3890aa · f3890aa
1 parent 8fe3a2d
commit f3890aa
Show file tree

Hide file tree

Showing 25 changed files with 77 additions and 220 deletions.
diff --git a/.../google/api/generator/gapic/composer/common/AbstractServiceStubSettingsClassComposer.java b/.../google/api/generator/gapic/composer/common/AbstractServiceStubSettingsClassComposer.java
@@ -65,13 +65,11 @@
 import com.google.api.generator.engine.ast.PrimitiveValue;
 import com.google.api.generator.engine.ast.Reference;
 import com.google.api.generator.engine.ast.ReferenceConstructorExpr;
-import com.google.api.generator.engine.ast.RelationalOperationExpr;
 import com.google.api.generator.engine.ast.ReturnExpr;
 import com.google.api.generator.engine.ast.ScopeNode;
 import com.google.api.generator.engine.ast.Statement;
 import com.google.api.generator.engine.ast.StringObjectValue;
 import com.google.api.generator.engine.ast.SuperObjectValue;
-import com.google.api.generator.engine.ast.TernaryExpr;
 import com.google.api.generator.engine.ast.ThisObjectValue;
 import com.google.api.generator.engine.ast.ThrowExpr;
 import com.google.api.generator.engine.ast.TypeNode;
@@ -788,27 +786,9 @@ private static Expr createPagedListDescriptorAssignExpr(
                 .setGenerics(Arrays.asList(repeatedResponseType.reference()))
                 .build());
 
-    Expr getResponsesExpr;
-    Expr elseExpr;
-    Expr thenExpr;
     if (repeatedResponseType.reference() != null
         && "java.util.Map.Entry".equals(repeatedResponseType.reference().fullName())) {
-      getResponsesExpr =
-          MethodInvocationExpr.builder()
-              .setExprReferenceExpr(payloadVarExpr)
-              .setMethodName(
-                  String.format("get%sMap", JavaStyle.toUpperCamelCase(repeatedFieldName)))
-              .setReturnType(returnType)
-              .build();
-      thenExpr =
-          MethodInvocationExpr.builder()
-              .setStaticReferenceType(
-                  TypeNode.withReference(ConcreteReference.withClazz(Collections.class)))
-              .setGenerics(Arrays.asList(repeatedResponseType.reference()))
-              .setMethodName("emptySet")
-              .setReturnType(returnType)
-              .build();
-      elseExpr =
+      returnExpr =
           MethodInvocationExpr.builder()
               .setMethodName("entrySet")
               .setExprReferenceExpr(
@@ -820,39 +800,14 @@ private static Expr createPagedListDescriptorAssignExpr(
               .setReturnType(returnType)
               .build();
     } else {
-      getResponsesExpr =
+      returnExpr =
           MethodInvocationExpr.builder()
               .setExprReferenceExpr(payloadVarExpr)
               .setMethodName(
                   String.format("get%sList", JavaStyle.toUpperCamelCase(repeatedFieldName)))
               .setReturnType(returnType)
               .build();
-      thenExpr =
-          MethodInvocationExpr.builder()
-              .setStaticReferenceType(
-                  TypeNode.withReference(ConcreteReference.withClazz(ImmutableList.class)))
-              .setGenerics(Arrays.asList(repeatedResponseType.reference()))
-              .setMethodName("of")
-              .setReturnType(returnType)
-              .build();
-      elseExpr = getResponsesExpr;
     }
-    // While protobufs should not be null, this null-check is needed to protect against NPEs
-    // in paged iteration on clients that use legacy HTTP/JSON types, as these clients can
-    // actually return null instead of an empty list.
-    // Context:
-    //   Original issue: /~https://github.com/googleapis/google-cloud-java/issues/3736
-    //   Relevant discussion where this check was first added:
-    //        /~https://github.com/googleapis/google-cloud-java/pull/4499#discussion_r257057409
-    Expr conditionExpr =
-        RelationalOperationExpr.equalToWithExprs(getResponsesExpr, ValueExpr.createNullExpr());
-
-    returnExpr =
-        TernaryExpr.builder()
-            .setConditionExpr(conditionExpr)
-            .setThenExpr(thenExpr)
-            .setElseExpr(elseExpr)
-            .build();
     anonClassMethods.add(
         methodStarterBuilder
             .setReturnType(returnType)

diff --git a/...rc/test/java/com/google/api/generator/gapic/composer/grpc/goldens/EchoStubSettings.golden b/...rc/test/java/com/google/api/generator/gapic/composer/grpc/goldens/EchoStubSettings.golden
@@ -180,9 +180,7 @@ public class EchoStubSettings extends StubSettings<EchoStubSettings> {
 
             @Override
             public Iterable<EchoResponse> extractResources(PagedExpandResponse payload) {
-              return payload.getResponsesList() == null
-                  ? ImmutableList.<EchoResponse>of()
-                  : payload.getResponsesList();
+              return payload.getResponsesList();
             }
           };
 
@@ -216,9 +214,7 @@ public class EchoStubSettings extends StubSettings<EchoStubSettings> {
 
             @Override
             public Iterable<EchoResponse> extractResources(PagedExpandResponse payload) {
-              return payload.getResponsesList() == null
-                  ? ImmutableList.<EchoResponse>of()
-                  : payload.getResponsesList();
+              return payload.getResponsesList();
             }
           };
 

diff --git a/.../com/google/api/generator/gapic/composer/grpc/goldens/LoggingServiceV2StubSettings.golden b/.../com/google/api/generator/gapic/composer/grpc/goldens/LoggingServiceV2StubSettings.golden
@@ -168,9 +168,7 @@ public class LoggingServiceV2StubSettings extends StubSettings<LoggingServiceV2S
 
             @Override
             public Iterable<LogEntry> extractResources(ListLogEntriesResponse payload) {
-              return payload.getEntriesList() == null
-                  ? ImmutableList.<LogEntry>of()
-                  : payload.getEntriesList();
+              return payload.getEntriesList();
             }
           };
 
@@ -217,9 +215,7 @@ public class LoggingServiceV2StubSettings extends StubSettings<LoggingServiceV2S
             @Override
             public Iterable<MonitoredResourceDescriptor> extractResources(
                 ListMonitoredResourceDescriptorsResponse payload) {
-              return payload.getResourceDescriptorsList() == null
-                  ? ImmutableList.<MonitoredResourceDescriptor>of()
-                  : payload.getResourceDescriptorsList();
+              return payload.getResourceDescriptorsList();
             }
           };
 
@@ -253,9 +249,7 @@ public class LoggingServiceV2StubSettings extends StubSettings<LoggingServiceV2S
 
             @Override
             public Iterable<String> extractResources(ListLogsResponse payload) {
-              return payload.getLogNamesList() == null
-                  ? ImmutableList.<String>of()
-                  : payload.getLogNamesList();
+              return payload.getLogNamesList();
             }
           };
 

diff --git a/...st/java/com/google/api/generator/gapic/composer/grpc/goldens/PublisherStubSettings.golden b/...st/java/com/google/api/generator/gapic/composer/grpc/goldens/PublisherStubSettings.golden
@@ -166,9 +166,7 @@ public class PublisherStubSettings extends StubSettings<PublisherStubSettings> {
 
             @Override
             public Iterable<Topic> extractResources(ListTopicsResponse payload) {
-              return payload.getTopicsList() == null
-                  ? ImmutableList.<Topic>of()
-                  : payload.getTopicsList();
+              return payload.getTopicsList();
             }
           };
 
@@ -208,9 +206,7 @@ public class PublisherStubSettings extends StubSettings<PublisherStubSettings> {
 
             @Override
             public Iterable<String> extractResources(ListTopicSubscriptionsResponse payload) {
-              return payload.getSubscriptionsList() == null
-                  ? ImmutableList.<String>of()
-                  : payload.getSubscriptionsList();
+              return payload.getSubscriptionsList();
             }
           };
 
@@ -247,9 +243,7 @@ public class PublisherStubSettings extends StubSettings<PublisherStubSettings> {
 
             @Override
             public Iterable<String> extractResources(ListTopicSnapshotsResponse payload) {
-              return payload.getSnapshotsList() == null
-                  ? ImmutableList.<String>of()
-                  : payload.getSnapshotsList();
+              return payload.getSnapshotsList();
             }
           };
 

diff --git a/...est/java/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoStubSettings.golden b/...est/java/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoStubSettings.golden
@@ -186,9 +186,7 @@ public class EchoStubSettings extends StubSettings<EchoStubSettings> {
 
             @Override
             public Iterable<EchoResponse> extractResources(PagedExpandResponse payload) {
-              return payload.getResponsesList() == null
-                  ? ImmutableList.<EchoResponse>of()
-                  : payload.getResponsesList();
+              return payload.getResponsesList();
             }
           };
 
@@ -222,9 +220,7 @@ public class EchoStubSettings extends StubSettings<EchoStubSettings> {
 
             @Override
             public Iterable<EchoResponse> extractResources(PagedExpandResponse payload) {
-              return payload.getResponsesList() == null
-                  ? ImmutableList.<EchoResponse>of()
-                  : payload.getResponsesList();
+              return payload.getResponsesList();
             }
           };
 

diff --git a/...gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/ComplianceStubSettings.java b/...gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/ComplianceStubSettings.java
@@ -171,9 +171,7 @@ public String extractNextToken(ListLocationsResponse payload) {
 
             @Override
             public Iterable<Location> extractResources(ListLocationsResponse payload) {
-              return payload.getLocationsList() == null
-                  ? ImmutableList.<Location>of()
-                  : payload.getLocationsList();
+              return payload.getLocationsList();
             }
           };
 

diff --git a/showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/EchoStubSettings.java b/showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/EchoStubSettings.java
@@ -82,7 +82,6 @@
 import com.google.showcase.v1beta1.WaitRequest;
 import com.google.showcase.v1beta1.WaitResponse;
 import java.io.IOException;
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import javax.annotation.Generated;
@@ -225,9 +224,7 @@ public String extractNextToken(PagedExpandResponse payload) {
 
             @Override
             public Iterable<EchoResponse> extractResources(PagedExpandResponse payload) {
-              return payload.getResponsesList() == null
-                  ? ImmutableList.<EchoResponse>of()
-                  : payload.getResponsesList();
+              return payload.getResponsesList();
             }
           };
 
@@ -268,9 +265,7 @@ public String extractNextToken(PagedExpandLegacyMappedResponse payload) {
             @Override
             public Iterable<Map.Entry<String, PagedExpandResponseList>> extractResources(
                 PagedExpandLegacyMappedResponse payload) {
-              return payload.getAlphabetizedMap() == null
-                  ? Collections.<Map.Entry<String, PagedExpandResponseList>>emptySet()
-                  : payload.getAlphabetizedMap().entrySet();
+              return payload.getAlphabetizedMap().entrySet();
             }
           };
 
@@ -304,9 +299,7 @@ public String extractNextToken(ListLocationsResponse payload) {
 
             @Override
             public Iterable<Location> extractResources(ListLocationsResponse payload) {
-              return payload.getLocationsList() == null
-                  ? ImmutableList.<Location>of()
-                  : payload.getLocationsList();
+              return payload.getLocationsList();
             }
           };
 

diff --git a/...e/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/IdentityStubSettings.java b/...e/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/IdentityStubSettings.java
@@ -172,9 +172,7 @@ public String extractNextToken(ListUsersResponse payload) {
 
             @Override
             public Iterable<User> extractResources(ListUsersResponse payload) {
-              return payload.getUsersList() == null
-                  ? ImmutableList.<User>of()
-                  : payload.getUsersList();
+              return payload.getUsersList();
             }
           };
 
@@ -208,9 +206,7 @@ public String extractNextToken(ListLocationsResponse payload) {
 
             @Override
             public Iterable<Location> extractResources(ListLocationsResponse payload) {
-              return payload.getLocationsList() == null
-                  ? ImmutableList.<Location>of()
-                  : payload.getLocationsList();
+              return payload.getLocationsList();
             }
           };
 

diff --git a/.../gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/MessagingStubSettings.java b/.../gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/MessagingStubSettings.java
@@ -233,9 +233,7 @@ public String extractNextToken(ListRoomsResponse payload) {
 
             @Override
             public Iterable<Room> extractResources(ListRoomsResponse payload) {
-              return payload.getRoomsList() == null
-                  ? ImmutableList.<Room>of()
-                  : payload.getRoomsList();
+              return payload.getRoomsList();
             }
           };
 
@@ -269,9 +267,7 @@ public String extractNextToken(ListBlurbsResponse payload) {
 
             @Override
             public Iterable<Blurb> extractResources(ListBlurbsResponse payload) {
-              return payload.getBlurbsList() == null
-                  ? ImmutableList.<Blurb>of()
-                  : payload.getBlurbsList();
+              return payload.getBlurbsList();
             }
           };
 
@@ -305,9 +301,7 @@ public String extractNextToken(ListLocationsResponse payload) {
 
             @Override
             public Iterable<Location> extractResources(ListLocationsResponse payload) {
-              return payload.getLocationsList() == null
-                  ? ImmutableList.<Location>of()
-                  : payload.getLocationsList();
+              return payload.getLocationsList();
             }
           };
 

diff --git a/...-showcase/src/main/java/com/google/showcase/v1beta1/stub/SequenceServiceStubSettings.java b/...-showcase/src/main/java/com/google/showcase/v1beta1/stub/SequenceServiceStubSettings.java
@@ -182,9 +182,7 @@ public String extractNextToken(ListLocationsResponse payload) {
 
             @Override
             public Iterable<Location> extractResources(ListLocationsResponse payload) {
-              return payload.getLocationsList() == null
-                  ? ImmutableList.<Location>of()
-                  : payload.getLocationsList();
+              return payload.getLocationsList();
             }
           };
 

diff --git a/...se/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/TestingStubSettings.java b/...se/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/TestingStubSettings.java
@@ -185,9 +185,7 @@ public String extractNextToken(ListSessionsResponse payload) {
 
             @Override
             public Iterable<Session> extractResources(ListSessionsResponse payload) {
-              return payload.getSessionsList() == null
-                  ? ImmutableList.<Session>of()
-                  : payload.getSessionsList();
+              return payload.getSessionsList();
             }
           };
 
@@ -221,9 +219,7 @@ public String extractNextToken(ListTestsResponse payload) {
 
             @Override
             public Iterable<Test> extractResources(ListTestsResponse payload) {
-              return payload.getTestsList() == null
-                  ? ImmutableList.<Test>of()
-                  : payload.getTestsList();
+              return payload.getTestsList();
             }
           };
 
@@ -257,9 +253,7 @@ public String extractNextToken(ListLocationsResponse payload) {
 
             @Override
             public Iterable<Location> extractResources(ListLocationsResponse payload) {
-              return payload.getLocationsList() == null
-                  ? ImmutableList.<Location>of()
-                  : payload.getLocationsList();
+              return payload.getLocationsList();
             }
           };
 

diff --git a/.../src/main/resources/META-INF/native-image/com.google.showcase.v1beta1/reflect-config.json b/.../src/main/resources/META-INF/native-image/com.google.showcase.v1beta1/reflect-config.json
@@ -332,6 +332,24 @@
     "allDeclaredClasses": true,
     "allPublicClasses": true
   },
+  {
+    "name": "com.google.api.PythonSettings$ExperimentalFeatures",
+    "queryAllDeclaredConstructors": true,
+    "queryAllPublicConstructors": true,
+    "queryAllDeclaredMethods": true,
+    "allPublicMethods": true,
+    "allDeclaredClasses": true,
+    "allPublicClasses": true
+  },
+  {
+    "name": "com.google.api.PythonSettings$ExperimentalFeatures$Builder",
+    "queryAllDeclaredConstructors": true,
+    "queryAllPublicConstructors": true,
+    "queryAllDeclaredMethods": true,
+    "allPublicMethods": true,
+    "allDeclaredClasses": true,
+    "allPublicClasses": true
+  },
   {
     "name": "com.google.api.ResourceDescriptor",
     "queryAllDeclaredConstructors": true,

diff --git a/...igeeconnect/src/com/google/cloud/apigeeconnect/v1/stub/ConnectionServiceStubSettings.java b/...igeeconnect/src/com/google/cloud/apigeeconnect/v1/stub/ConnectionServiceStubSettings.java
@@ -150,9 +150,7 @@ public String extractNextToken(ListConnectionsResponse payload) {
 
             @Override
             public Iterable<Connection> extractResources(ListConnectionsResponse payload) {
-              return payload.getConnectionsList() == null
-                  ? ImmutableList.<Connection>of()
-                  : payload.getConnectionsList();
+              return payload.getConnectionsList();
             }
           };