cleanup(gax-java): remove javax.annotation.Nonnull usage - #13959
cleanup(gax-java): remove javax.annotation.Nonnull usage#13959nnicolee wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the @Nonnull annotation and its corresponding javax.annotation.Nonnull import across numerous files in the gax-java library, likely as part of a migration to JSpecify annotations. A review of the changes identified a redundant null check in ServerStreamingCallSettings.java where Preconditions.checkNotNull(resumptionStrategy) is called twice consecutively, which should be simplified.
| Preconditions.checkNotNull(resumptionStrategy); | ||
| this.resumptionStrategy = Preconditions.checkNotNull(resumptionStrategy); |
There was a problem hiding this comment.
The null check Preconditions.checkNotNull(resumptionStrategy) is performed twice in a row. We can simplify this by directly assigning the result of Preconditions.checkNotNull(resumptionStrategy) to this.resumptionStrategy.
| Preconditions.checkNotNull(resumptionStrategy); | |
| this.resumptionStrategy = Preconditions.checkNotNull(resumptionStrategy); | |
| this.resumptionStrategy = Preconditions.checkNotNull(resumptionStrategy); |
References
- Ensure that null checks and validation steps are not redundant with checks already performed by upstream callers or preceding logic in the same method.
| <dependency> | ||
| <groupId>com.google.code.findbugs</groupId> | ||
| <artifactId>jsr305</artifactId> | ||
| <scope>provided</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Since we migrated the codebase to JSpecify, we removed all references to the javax.annotation packages (which are provided by the JSR-305 / jsr305 dependency).
Leaving this dependency declared would trigger an "unused declared dependency" warning!
There was a problem hiding this comment.
Ok, got it. Can you add a note to the PR description why this isn't a breaking change and why we removed it? I think this should be fine since this is provided scope and won't be part of the customer's dependency tree from this jar
There was a problem hiding this comment.
Yes, updated the PR message!
|
|
|
@nnicolee I think there are a few straggling instances I've seen with Can you take a look? I think we can also raise a new PR to do additional cleanup |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the codebase from JSR-305 nullness annotations (@nonnull, @nullable) to JSpecify annotations (@nullable, @NullMarked). It removes the jsr305 dependency from the Maven POM files across multiple modules (gax-grpc, gax-httpjson, and gax) and updates the Java source files by removing @nonnull annotations (relying on class-level @NullMarked instead) and shifting @nullable annotations to type-use positions. There are no review comments to address, and I have no additional feedback to provide.
2 participants
This PR removes all occurrences of
javax.annotation.Nonnull(both imports and annotations) across thegax-javacodebase, and removes the now unusedjsr305dependency.Why this change is needed:
As part of the repository-wide migration to JSpecify annotations, classes are annotated with
@NullMarked, making all unannotated types non-nullable by default. The legacyjavax.annotation.Nonnullannotations are redundant and can be safely removed without changing null-safety semantics.JSR-305 (
jsr305) Dependency Removal:We removed the
com.google.code.findbugs:jsr305dependency from thepom.xmlfiles. This is not a breaking change for the following reasons:javax.annotationpackage in the codebase, the Maven dependency analyzer (mvn dependency:analyze) flaggedjsr305as an unused declared dependency. Since our verification fails on any warning (-DfailOnWarning=true), the dependency had to be removed to pass presubmit checks.providedscope, meaning it was only used during compilation and was never distributed transitively to downstream users. Removing it has no runtime impact on library consumers.Changes:
import javax.annotation.Nonnull;and@Nonnullfrom 30 files acrossgax,gax-grpc, andgax-httpjson(including test files).javax.annotation.Nullableusages in tests to JSpecify's@Nullabletype-use annotation.jsr305dependency from thegax-javamodule POMs.fmt-maven-plugin.