feat(gapic-generator): Add Nullable annotation to generated classes#13558
feat(gapic-generator): Add Nullable annotation to generated classes#13558nnicolee wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces @Nullable annotations to various generated classes and generator code in the Java GAPIC generator. Specifically, it updates the composer classes to annotate settings fields, getter methods, non-required resource name helper arguments, parse methods, toStringList generic types, and equals method arguments, with corresponding updates to the golden files. A review comment highlights a fragile workaround in ResourceNameHelperClassComposer.java where the @Nullable annotation is hacked into the VaporReference name string, warning that this could lead to issues with type comparisons, fully qualified names, AST manipulation, and missing imports, and suggests extending the AST classes to properly support type-use annotations.
| .setGenerics( | ||
| Arrays.asList( | ||
| VaporReference.builder() | ||
| .setName("@Nullable " + thisClassType.reference().name()) | ||
| .setPakkage(thisClassType.reference().pakkage()) | ||
| .build())) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
|
|
||
| private ListLocationsFixedSizeCollection(List<ListLocationsPage> pages, int collectionSize) { | ||
| private ListLocationsFixedSizeCollection( | ||
| @Nullable List<ListLocationsPage> pages, int collectionSize) { |
There was a problem hiding this comment.
Can you double check this one? Do we need to add an annotation here for paging?
There was a problem hiding this comment.
Hi! Yes, we need to add an annotation here because two places call this function (createCollection and createEmptyCollection (on line 1590-1) which calls the function with null in the first parameter (which is pages). Verified this annotation with Nullaway!
| public final Room getRoom(RoomName name) { | ||
| GetRoomRequest request = | ||
| GetRoomRequest.newBuilder().setName(name == null ? null : name.toString()).build(); | ||
| GetRoomRequest request = GetRoomRequest.newBuilder().setName(name.toString()).build(); |
There was a problem hiding this comment.
qq, I don't think we should change this. Perhaps better to mark the param as nullable?
There was a problem hiding this comment.
+1 the new logic may affect this, which is a good way to catch additional cases where @Nullable is needed.
There was a problem hiding this comment.
Discussed with @nnicolee, looks like setName is a proto class, which we don't have control over, and is being marked by NullAway with:
[ERROR] /usr/local/google/home/nicolejlee/google-cloud-java/java-showcase/gapic-showcase/src/main/java/com/google/showcase/v1beta1/MessagingClient.java:[1822,64] error: [NullAway] passing @Nullable parameter 'parent == null ? null : parent.toString()' where @NonNull is required
If it was possible to configure ErrorProne to ignore proto classes in these cases that would be great. That could be a good first step.
On the other hand, looks like Proto classes actually can take nulls (since the code's been like this), which can be worked around by adding a warning suppression as Nicole suggested.
| .toString(); | ||
| } | ||
|
|
||
| @Nullable |
There was a problem hiding this comment.
same here, should be type-use annotation here.



This PR updates the gapic-generator-java to add the JSpecify @nullable annotation to all generated class declarations. This is the second phase in onboarding the generated client libraries to compile-time safety validation, see design doc for more details: go/sdk:java-jspecify-null-annotations-gapic
Classes Annotated:
Implementation Changes:
Verification/Testing: