feat(auth): add JSpecify Null annotations to Auth#13842
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
76c939a to
afdc8c6
Compare
afdc8c6 to
432c5d0
Compare
|
/gemini review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request integrates JSpecify nullness annotations (@NullMarked and @Nullable) across multiple modules in the google-auth-library-java repository to enhance static analysis and null safety. The review feedback correctly identifies a critical issue in TokenVerifier.java where @Nullable is applied to type parameters of Guava's ImmutableMap and ImmutableMap.Builder. Because Guava's immutable collections do not allow null values, these changes could lead to runtime NullPointerExceptions. The reviewer advises reverting these map type parameters to PublicKey and adding explicit null checks before inserting elements into the builder.
| private final PublicKey publicKey; | ||
| private final Clock clock; | ||
| private final LoadingCache<String, Map<String, PublicKey>> publicKeyCache; | ||
| private final LoadingCache<String, Map<String, @Nullable PublicKey>> publicKeyCache; |
There was a problem hiding this comment.
Since Guava's ImmutableMap does not permit null values, the cached map should not contain @Nullable values. Please keep the type parameter as PublicKey.
| private final LoadingCache<String, Map<String, @Nullable PublicKey>> publicKeyCache; | |
| private final LoadingCache<String, Map<String, PublicKey>> publicKeyCache; |
|
|
||
| /** Custom CacheLoader for mapping certificate urls to the contained public keys. */ | ||
| static class PublicKeyLoader extends CacheLoader<String, Map<String, PublicKey>> { | ||
| static class PublicKeyLoader extends CacheLoader<String, Map<String, @Nullable PublicKey>> { |
There was a problem hiding this comment.
Since Guava's ImmutableMap does not permit null values, the map returned by the loader should not contain @Nullable values. Please keep the type parameter as PublicKey.
| static class PublicKeyLoader extends CacheLoader<String, Map<String, @Nullable PublicKey>> { | |
| static class PublicKeyLoader extends CacheLoader<String, Map<String, PublicKey>> { |
|
|
||
| @Override | ||
| public Map<String, PublicKey> load(String certificateUrl) throws Exception { | ||
| public Map<String, @Nullable PublicKey> load(String certificateUrl) throws Exception { |
There was a problem hiding this comment.
Since Guava's ImmutableMap does not permit null values, the map returned by load should not contain @Nullable values. Please keep the type parameter as PublicKey.
| public Map<String, @Nullable PublicKey> load(String certificateUrl) throws Exception { | |
| public Map<String, PublicKey> load(String certificateUrl) throws Exception { |
| jwks = response.parseAs(JsonWebKeySet.class); | ||
|
|
||
| ImmutableMap.Builder<String, PublicKey> keyCacheBuilder = new ImmutableMap.Builder<>(); | ||
| ImmutableMap.Builder<String, @Nullable PublicKey> keyCacheBuilder = new ImmutableMap.Builder<>(); |
There was a problem hiding this comment.
Guava's ImmutableMap and its builder do not permit null keys or values. Declaring the builder as ImmutableMap.Builder<String, @Nullable PublicKey> is problematic because if buildPublicKey(key) returns null and is added to the builder, it will throw a NullPointerException at runtime. If null values are filtered out before being added to the builder, then the map will never contain null values, and the type parameter should be PublicKey instead of @Nullable PublicKey to accurately reflect the map's contents and avoid type-safety issues. Please revert the map and builder type parameters to PublicKey (without @Nullable).
| ImmutableMap.Builder<String, @Nullable PublicKey> keyCacheBuilder = new ImmutableMap.Builder<>(); | |
| ImmutableMap.Builder<String, PublicKey> keyCacheBuilder = new ImmutableMap.Builder<>(); |
| } | ||
|
|
||
| ImmutableMap<String, PublicKey> keyCache = keyCacheBuilder.build(); | ||
| ImmutableMap<String, @Nullable PublicKey> keyCache = keyCacheBuilder.build(); |
There was a problem hiding this comment.
Since Guava's ImmutableMap does not permit null values, the built map should not contain @Nullable values. Please keep the type parameter as PublicKey.
| ImmutableMap<String, @Nullable PublicKey> keyCache = keyCacheBuilder.build(); | |
| ImmutableMap<String, PublicKey> keyCache = keyCacheBuilder.build(); |
| } | ||
|
|
||
| private PublicKey buildPublicKey(JsonWebKey key) | ||
| private @Nullable PublicKey buildPublicKey(JsonWebKey key) |
There was a problem hiding this comment.
Summary
This PR migrates the Google Auth Library for Java (
google-auth-library-java) to use JSpecify 1.0.0 nullability annotations (@NullMarkedand@Nullable), replacing the previousjavax.annotationreferences.Key Changes
JSpecify 1.0.0 Annotation Adoption in Auth:
@NullMarkedat the class and package levels across core auth classes (incredentials,oauth2_http,appengine, andcab-token-generatormodules) to define non-null by default semantics.javax.annotation.Nullablewithorg.jspecify.annotations.Nullable.pom.xmlfiles and BazelBUILD.bazelconfigurations to declare and manage theorg.jspecify:jspecifydependency.Verification Results