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.
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.
| */ | |
| @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, |
| 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
| <dependency> | ||
| <groupId>org.jspecify</groupId> | ||
| <artifactId>jspecify</artifactId> | ||
| <version>1.0.0</version> |
There was a problem hiding this comment.
nit: Can you declare this as a property, similar to above?
e.g. ${project.jspecify.version}
| LoggerFactoryProvider mockLoggerFactoryProvider = mock(LoggerFactoryProvider.class); | ||
| ILoggerFactory mockLoggerFactory = mock(ILoggerFactory.class); | ||
| LoggerFactoryProvider mockLoggerFactoryProvider = | ||
| mock(LoggerFactoryProvider.class, org.mockito.Mockito.withSettings().withoutAnnotations()); |
There was a problem hiding this comment.
nit: Just a few occurrences of the fully qualified names. See if you can import Mockito
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates JSpecify nullness annotations (@NullMarked and @nullable) across several modules, including appengine, cab-token-generator, credentials, and oauth2_http, along with updating the build configurations to include the jspecify dependency. The feedback highlights several nullability mismatches introduced by these changes: changing getRequestMetadata in the abstract Credentials class to accept @nullable URI requires updating all subclass implementations to avoid Liskov Substitution Principle violations; returning @nullable PublicKey from buildPublicKey could result in inserting nulls into a map expecting non-null values; and passing a @nullable URI from createSelfSignedJwtCredentials to getUriForSelfSignedJWT (which expects a non-null parameter) will trigger static analysis warnings.
| public abstract Map<String, List<String>> getRequestMetadata(@Nullable URI uri) | ||
| throws IOException; |
There was a problem hiding this comment.
Since Credentials is the base class and getRequestMetadata is an abstract method, changing its parameter uri to @nullable requires all concrete subclasses overriding this method to also be updated to accept @nullable URI. Since classes like OAuth2Credentials, ApiKeyCredentials, JwtCredentials, and ServiceAccountJwtAccessCredentials are annotated with @NullMarked in this PR, their implementations of getRequestMetadata will default to expecting a non-null URI parameter. This mismatch will cause static analysis or compilation errors due to Liskov Substitution Principle violations. Please ensure that all subclass implementations of getRequestMetadata are also updated to use @nullable URI.
| } | ||
|
|
||
| private PublicKey buildPublicKey(JsonWebKey key) | ||
| private @Nullable PublicKey buildPublicKey(JsonWebKey key) |
There was a problem hiding this comment.
The method buildPublicKey is updated to return @nullable PublicKey. However, the caller load returns a Map<String, PublicKey> (which under @NullMarked implies non-null keys and values). If buildPublicKey returns null and this value is put into keyCache, it will violate the non-null value constraint of the map, leading to static analysis errors or potential NullPointerExceptions down the line. Please ensure that null public keys are filtered out and not put into keyCache, or update the map signature to Map<String, @nullable PublicKey> if null values are expected.
| JwtCredentials createSelfSignedJwtCredentials(final @Nullable URI uri) { | ||
| return createSelfSignedJwtCredentials(uri, scopes.isEmpty() ? defaultScopes : scopes); | ||
| } |
There was a problem hiding this comment.
The parameter uri in createSelfSignedJwtCredentials is now annotated with @nullable. However, the helper method getUriForSelfSignedJWT(URI uri) (defined on line 1011) is not updated to accept @nullable URI. Under @NullMarked, passing a @nullable URI to getUriForSelfSignedJWT will trigger static analysis warnings/errors. Please update getUriForSelfSignedJWT to also accept @nullable URI or handle the null case before calling it.
tba