Skip to content

Commit f3890aa

Browse files
lqiu96cloud-java-bot
lqiu96
and
cloud-java-bot
authored
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 <[email protected]> Co-authored-by: cloud-java-bot <[email protected]>
1 parent 8fe3a2d commit f3890aa

File tree

25 files changed

+77
-220
lines changed

25 files changed

+77
-220
lines changed

‎gapic-generator-java/src/main/java/com/google/api/generator/gapic/composer/common/AbstractServiceStubSettingsClassComposer.java

+2-47
Original file line numberDiff line numberDiff line change
@@ -65,13 +65,11 @@
6565
import com.google.api.generator.engine.ast.PrimitiveValue;
6666
import com.google.api.generator.engine.ast.Reference;
6767
import com.google.api.generator.engine.ast.ReferenceConstructorExpr;
68-
import com.google.api.generator.engine.ast.RelationalOperationExpr;
6968
import com.google.api.generator.engine.ast.ReturnExpr;
7069
import com.google.api.generator.engine.ast.ScopeNode;
7170
import com.google.api.generator.engine.ast.Statement;
7271
import com.google.api.generator.engine.ast.StringObjectValue;
7372
import com.google.api.generator.engine.ast.SuperObjectValue;
74-
import com.google.api.generator.engine.ast.TernaryExpr;
7573
import com.google.api.generator.engine.ast.ThisObjectValue;
7674
import com.google.api.generator.engine.ast.ThrowExpr;
7775
import com.google.api.generator.engine.ast.TypeNode;
@@ -788,27 +786,9 @@ private static Expr createPagedListDescriptorAssignExpr(
788786
.setGenerics(Arrays.asList(repeatedResponseType.reference()))
789787
.build());
790788

791-
Expr getResponsesExpr;
792-
Expr elseExpr;
793-
Expr thenExpr;
794789
if (repeatedResponseType.reference() != null
795790
&& "java.util.Map.Entry".equals(repeatedResponseType.reference().fullName())) {
796-
getResponsesExpr =
797-
MethodInvocationExpr.builder()
798-
.setExprReferenceExpr(payloadVarExpr)
799-
.setMethodName(
800-
String.format("get%sMap", JavaStyle.toUpperCamelCase(repeatedFieldName)))
801-
.setReturnType(returnType)
802-
.build();
803-
thenExpr =
804-
MethodInvocationExpr.builder()
805-
.setStaticReferenceType(
806-
TypeNode.withReference(ConcreteReference.withClazz(Collections.class)))
807-
.setGenerics(Arrays.asList(repeatedResponseType.reference()))
808-
.setMethodName("emptySet")
809-
.setReturnType(returnType)
810-
.build();
811-
elseExpr =
791+
returnExpr =
812792
MethodInvocationExpr.builder()
813793
.setMethodName("entrySet")
814794
.setExprReferenceExpr(
@@ -820,39 +800,14 @@ private static Expr createPagedListDescriptorAssignExpr(
820800
.setReturnType(returnType)
821801
.build();
822802
} else {
823-
getResponsesExpr =
803+
returnExpr =
824804
MethodInvocationExpr.builder()
825805
.setExprReferenceExpr(payloadVarExpr)
826806
.setMethodName(
827807
String.format("get%sList", JavaStyle.toUpperCamelCase(repeatedFieldName)))
828808
.setReturnType(returnType)
829809
.build();
830-
thenExpr =
831-
MethodInvocationExpr.builder()
832-
.setStaticReferenceType(
833-
TypeNode.withReference(ConcreteReference.withClazz(ImmutableList.class)))
834-
.setGenerics(Arrays.asList(repeatedResponseType.reference()))
835-
.setMethodName("of")
836-
.setReturnType(returnType)
837-
.build();
838-
elseExpr = getResponsesExpr;
839810
}
840-
// While protobufs should not be null, this null-check is needed to protect against NPEs
841-
// in paged iteration on clients that use legacy HTTP/JSON types, as these clients can
842-
// actually return null instead of an empty list.
843-
// Context:
844-
// Original issue: https://github.com/googleapis/google-cloud-java/issues/3736
845-
// Relevant discussion where this check was first added:
846-
// https://github.com/googleapis/google-cloud-java/pull/4499#discussion_r257057409
847-
Expr conditionExpr =
848-
RelationalOperationExpr.equalToWithExprs(getResponsesExpr, ValueExpr.createNullExpr());
849-
850-
returnExpr =
851-
TernaryExpr.builder()
852-
.setConditionExpr(conditionExpr)
853-
.setThenExpr(thenExpr)
854-
.setElseExpr(elseExpr)
855-
.build();
856811
anonClassMethods.add(
857812
methodStarterBuilder
858813
.setReturnType(returnType)

‎gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/EchoStubSettings.golden

+2-6
Original file line numberDiff line numberDiff line change
@@ -180,9 +180,7 @@ public class EchoStubSettings extends StubSettings<EchoStubSettings> {
180180

181181
@Override
182182
public Iterable<EchoResponse> extractResources(PagedExpandResponse payload) {
183-
return payload.getResponsesList() == null
184-
? ImmutableList.<EchoResponse>of()
185-
: payload.getResponsesList();
183+
return payload.getResponsesList();
186184
}
187185
};
188186

@@ -216,9 +214,7 @@ public class EchoStubSettings extends StubSettings<EchoStubSettings> {
216214

217215
@Override
218216
public Iterable<EchoResponse> extractResources(PagedExpandResponse payload) {
219-
return payload.getResponsesList() == null
220-
? ImmutableList.<EchoResponse>of()
221-
: payload.getResponsesList();
217+
return payload.getResponsesList();
222218
}
223219
};
224220

‎gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/LoggingServiceV2StubSettings.golden

+3-9
Original file line numberDiff line numberDiff line change
@@ -168,9 +168,7 @@ public class LoggingServiceV2StubSettings extends StubSettings<LoggingServiceV2S
168168

169169
@Override
170170
public Iterable<LogEntry> extractResources(ListLogEntriesResponse payload) {
171-
return payload.getEntriesList() == null
172-
? ImmutableList.<LogEntry>of()
173-
: payload.getEntriesList();
171+
return payload.getEntriesList();
174172
}
175173
};
176174

@@ -217,9 +215,7 @@ public class LoggingServiceV2StubSettings extends StubSettings<LoggingServiceV2S
217215
@Override
218216
public Iterable<MonitoredResourceDescriptor> extractResources(
219217
ListMonitoredResourceDescriptorsResponse payload) {
220-
return payload.getResourceDescriptorsList() == null
221-
? ImmutableList.<MonitoredResourceDescriptor>of()
222-
: payload.getResourceDescriptorsList();
218+
return payload.getResourceDescriptorsList();
223219
}
224220
};
225221

@@ -253,9 +249,7 @@ public class LoggingServiceV2StubSettings extends StubSettings<LoggingServiceV2S
253249

254250
@Override
255251
public Iterable<String> extractResources(ListLogsResponse payload) {
256-
return payload.getLogNamesList() == null
257-
? ImmutableList.<String>of()
258-
: payload.getLogNamesList();
252+
return payload.getLogNamesList();
259253
}
260254
};
261255

‎gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/PublisherStubSettings.golden

+3-9
Original file line numberDiff line numberDiff line change
@@ -166,9 +166,7 @@ public class PublisherStubSettings extends StubSettings<PublisherStubSettings> {
166166

167167
@Override
168168
public Iterable<Topic> extractResources(ListTopicsResponse payload) {
169-
return payload.getTopicsList() == null
170-
? ImmutableList.<Topic>of()
171-
: payload.getTopicsList();
169+
return payload.getTopicsList();
172170
}
173171
};
174172

@@ -208,9 +206,7 @@ public class PublisherStubSettings extends StubSettings<PublisherStubSettings> {
208206

209207
@Override
210208
public Iterable<String> extractResources(ListTopicSubscriptionsResponse payload) {
211-
return payload.getSubscriptionsList() == null
212-
? ImmutableList.<String>of()
213-
: payload.getSubscriptionsList();
209+
return payload.getSubscriptionsList();
214210
}
215211
};
216212

@@ -247,9 +243,7 @@ public class PublisherStubSettings extends StubSettings<PublisherStubSettings> {
247243

248244
@Override
249245
public Iterable<String> extractResources(ListTopicSnapshotsResponse payload) {
250-
return payload.getSnapshotsList() == null
251-
? ImmutableList.<String>of()
252-
: payload.getSnapshotsList();
246+
return payload.getSnapshotsList();
253247
}
254248
};
255249

‎gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpcrest/goldens/EchoStubSettings.golden

+2-6
Original file line numberDiff line numberDiff line change
@@ -186,9 +186,7 @@ public class EchoStubSettings extends StubSettings<EchoStubSettings> {
186186

187187
@Override
188188
public Iterable<EchoResponse> extractResources(PagedExpandResponse payload) {
189-
return payload.getResponsesList() == null
190-
? ImmutableList.<EchoResponse>of()
191-
: payload.getResponsesList();
189+
return payload.getResponsesList();
192190
}
193191
};
194192

@@ -222,9 +220,7 @@ public class EchoStubSettings extends StubSettings<EchoStubSettings> {
222220

223221
@Override
224222
public Iterable<EchoResponse> extractResources(PagedExpandResponse payload) {
225-
return payload.getResponsesList() == null
226-
? ImmutableList.<EchoResponse>of()
227-
: payload.getResponsesList();
223+
return payload.getResponsesList();
228224
}
229225
};
230226

‎showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/ComplianceStubSettings.java

+1-3
Original file line numberDiff line numberDiff line change
@@ -171,9 +171,7 @@ public String extractNextToken(ListLocationsResponse payload) {
171171

172172
@Override
173173
public Iterable<Location> extractResources(ListLocationsResponse payload) {
174-
return payload.getLocationsList() == null
175-
? ImmutableList.<Location>of()
176-
: payload.getLocationsList();
174+
return payload.getLocationsList();
177175
}
178176
};
179177

‎showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/EchoStubSettings.java

+3-10
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,6 @@
8282
import com.google.showcase.v1beta1.WaitRequest;
8383
import com.google.showcase.v1beta1.WaitResponse;
8484
import java.io.IOException;
85-
import java.util.Collections;
8685
import java.util.List;
8786
import java.util.Map;
8887
import javax.annotation.Generated;
@@ -225,9 +224,7 @@ public String extractNextToken(PagedExpandResponse payload) {
225224

226225
@Override
227226
public Iterable<EchoResponse> extractResources(PagedExpandResponse payload) {
228-
return payload.getResponsesList() == null
229-
? ImmutableList.<EchoResponse>of()
230-
: payload.getResponsesList();
227+
return payload.getResponsesList();
231228
}
232229
};
233230

@@ -268,9 +265,7 @@ public String extractNextToken(PagedExpandLegacyMappedResponse payload) {
268265
@Override
269266
public Iterable<Map.Entry<String, PagedExpandResponseList>> extractResources(
270267
PagedExpandLegacyMappedResponse payload) {
271-
return payload.getAlphabetizedMap() == null
272-
? Collections.<Map.Entry<String, PagedExpandResponseList>>emptySet()
273-
: payload.getAlphabetizedMap().entrySet();
268+
return payload.getAlphabetizedMap().entrySet();
274269
}
275270
};
276271

@@ -304,9 +299,7 @@ public String extractNextToken(ListLocationsResponse payload) {
304299

305300
@Override
306301
public Iterable<Location> extractResources(ListLocationsResponse payload) {
307-
return payload.getLocationsList() == null
308-
? ImmutableList.<Location>of()
309-
: payload.getLocationsList();
302+
return payload.getLocationsList();
310303
}
311304
};
312305

‎showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/IdentityStubSettings.java

+2-6
Original file line numberDiff line numberDiff line change
@@ -172,9 +172,7 @@ public String extractNextToken(ListUsersResponse payload) {
172172

173173
@Override
174174
public Iterable<User> extractResources(ListUsersResponse payload) {
175-
return payload.getUsersList() == null
176-
? ImmutableList.<User>of()
177-
: payload.getUsersList();
175+
return payload.getUsersList();
178176
}
179177
};
180178

@@ -208,9 +206,7 @@ public String extractNextToken(ListLocationsResponse payload) {
208206

209207
@Override
210208
public Iterable<Location> extractResources(ListLocationsResponse payload) {
211-
return payload.getLocationsList() == null
212-
? ImmutableList.<Location>of()
213-
: payload.getLocationsList();
209+
return payload.getLocationsList();
214210
}
215211
};
216212

‎showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/MessagingStubSettings.java

+3-9
Original file line numberDiff line numberDiff line change
@@ -233,9 +233,7 @@ public String extractNextToken(ListRoomsResponse payload) {
233233

234234
@Override
235235
public Iterable<Room> extractResources(ListRoomsResponse payload) {
236-
return payload.getRoomsList() == null
237-
? ImmutableList.<Room>of()
238-
: payload.getRoomsList();
236+
return payload.getRoomsList();
239237
}
240238
};
241239

@@ -269,9 +267,7 @@ public String extractNextToken(ListBlurbsResponse payload) {
269267

270268
@Override
271269
public Iterable<Blurb> extractResources(ListBlurbsResponse payload) {
272-
return payload.getBlurbsList() == null
273-
? ImmutableList.<Blurb>of()
274-
: payload.getBlurbsList();
270+
return payload.getBlurbsList();
275271
}
276272
};
277273

@@ -305,9 +301,7 @@ public String extractNextToken(ListLocationsResponse payload) {
305301

306302
@Override
307303
public Iterable<Location> extractResources(ListLocationsResponse payload) {
308-
return payload.getLocationsList() == null
309-
? ImmutableList.<Location>of()
310-
: payload.getLocationsList();
304+
return payload.getLocationsList();
311305
}
312306
};
313307

‎showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/SequenceServiceStubSettings.java

+1-3
Original file line numberDiff line numberDiff line change
@@ -182,9 +182,7 @@ public String extractNextToken(ListLocationsResponse payload) {
182182

183183
@Override
184184
public Iterable<Location> extractResources(ListLocationsResponse payload) {
185-
return payload.getLocationsList() == null
186-
? ImmutableList.<Location>of()
187-
: payload.getLocationsList();
185+
return payload.getLocationsList();
188186
}
189187
};
190188

‎showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/stub/TestingStubSettings.java

+3-9
Original file line numberDiff line numberDiff line change
@@ -185,9 +185,7 @@ public String extractNextToken(ListSessionsResponse payload) {
185185

186186
@Override
187187
public Iterable<Session> extractResources(ListSessionsResponse payload) {
188-
return payload.getSessionsList() == null
189-
? ImmutableList.<Session>of()
190-
: payload.getSessionsList();
188+
return payload.getSessionsList();
191189
}
192190
};
193191

@@ -221,9 +219,7 @@ public String extractNextToken(ListTestsResponse payload) {
221219

222220
@Override
223221
public Iterable<Test> extractResources(ListTestsResponse payload) {
224-
return payload.getTestsList() == null
225-
? ImmutableList.<Test>of()
226-
: payload.getTestsList();
222+
return payload.getTestsList();
227223
}
228224
};
229225

@@ -257,9 +253,7 @@ public String extractNextToken(ListLocationsResponse payload) {
257253

258254
@Override
259255
public Iterable<Location> extractResources(ListLocationsResponse payload) {
260-
return payload.getLocationsList() == null
261-
? ImmutableList.<Location>of()
262-
: payload.getLocationsList();
256+
return payload.getLocationsList();
263257
}
264258
};
265259

‎showcase/gapic-showcase/src/main/resources/META-INF/native-image/com.google.showcase.v1beta1/reflect-config.json

+18
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,24 @@
332332
"allDeclaredClasses": true,
333333
"allPublicClasses": true
334334
},
335+
{
336+
"name": "com.google.api.PythonSettings$ExperimentalFeatures",
337+
"queryAllDeclaredConstructors": true,
338+
"queryAllPublicConstructors": true,
339+
"queryAllDeclaredMethods": true,
340+
"allPublicMethods": true,
341+
"allDeclaredClasses": true,
342+
"allPublicClasses": true
343+
},
344+
{
345+
"name": "com.google.api.PythonSettings$ExperimentalFeatures$Builder",
346+
"queryAllDeclaredConstructors": true,
347+
"queryAllPublicConstructors": true,
348+
"queryAllDeclaredMethods": true,
349+
"allPublicMethods": true,
350+
"allDeclaredClasses": true,
351+
"allPublicClasses": true
352+
},
335353
{
336354
"name": "com.google.api.ResourceDescriptor",
337355
"queryAllDeclaredConstructors": true,

‎test/integration/goldens/apigeeconnect/src/com/google/cloud/apigeeconnect/v1/stub/ConnectionServiceStubSettings.java

+1-3
Original file line numberDiff line numberDiff line change
@@ -150,9 +150,7 @@ public String extractNextToken(ListConnectionsResponse payload) {
150150

151151
@Override
152152
public Iterable<Connection> extractResources(ListConnectionsResponse payload) {
153-
return payload.getConnectionsList() == null
154-
? ImmutableList.<Connection>of()
155-
: payload.getConnectionsList();
153+
return payload.getConnectionsList();
156154
}
157155
};
158156

0 commit comments

Comments
 (0)