feat(auth): add JSpecify Null annotations to Auth#13842
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces JSpecify nullness annotations (@NullMarked and @nullable) across several modules, including GAX and Google Cloud Bigtable, and updates tests to configure Mockito mocks to ignore annotations. The reviewer feedback highlights several improvement opportunities, such as correcting a potentially unsafe @nullable annotation on a builder constructor parameter to avoid NullPointerExceptions, and ensuring that the previousThrowable parameters in retry algorithms are explicitly marked as @nullable to handle successful attempts where no exception is thrown.
|
|
||
| /** Create a builder from a StubSettings object. */ | ||
| protected Builder(StubSettings.Builder stubSettings) { | ||
| protected Builder(StubSettings.@Nullable Builder stubSettings) { |
There was a problem hiding this comment.
Annotating stubSettings as @Nullable suggests that null is a valid argument. However, if stubSettings is null, calling methods like getQuotaProjectId() or getWatchdogProvider() on this builder will result in a NullPointerException because they dereference stubSettings without a null check. If stubSettings must not be null, it should not be annotated with @Nullable, and ideally, a null check like Preconditions.checkNotNull(stubSettings) should be added.
| * thrown instead | ||
| * @param previousSettings previous attempt settings | ||
| * @return next attempt settings, can be {@code null}, if there should be no new attempt | ||
| */ |
There was a problem hiding this comment.
| public boolean shouldRetry( | ||
| RetryingContext context, | ||
| @Nullable RetryingContext context, | ||
| Throwable previousThrowable, |
There was a problem hiding this comment.
In a @NullMarked class, previousThrowable should be annotated with @Nullable because on successful attempts (e.g., during polling or when checking if retries should continue after a success), previousThrowable can be null.
| Throwable previousThrowable, | |
| @Nullable Throwable previousThrowable, |
| public TimedAttemptSettings createNextAttempt( | ||
| RetryingContext context, | ||
| @Nullable RetryingContext context, | ||
| Throwable previousThrowable, |
There was a problem hiding this comment.
In a @NullMarked class, previousThrowable should be annotated with @Nullable because on successful attempts (e.g., during polling or when checking if retries should continue after a success), previousThrowable can be null.
| Throwable previousThrowable, | |
| @Nullable Throwable previousThrowable, |
| public boolean shouldRetry( | ||
| RetryingContext context, | ||
| @Nullable RetryingContext context, | ||
| Throwable previousThrowable, |
There was a problem hiding this comment.
In a @NullMarked class, previousThrowable should be annotated with @Nullable because on successful attempts (e.g., during polling or when checking if retries should continue after a success), previousThrowable can be null.
| Throwable previousThrowable, | |
| @Nullable Throwable previousThrowable, |
|
|
76c939a to
afdc8c6
Compare
afdc8c6 to
432c5d0
Compare


tba