Copilot commented on code in PR #11155:
URL: https://github.com/apache/gravitino/pull/11155#discussion_r3270915847
##########
core/src/main/java/org/apache/gravitino/credential/CredentialCache.java:
##########
@@ -87,8 +88,11 @@ public void initialize(Map<String, String>
catalogProperties) {
.build();
}
- public Credential getCredential(T cacheKey, Function<T, Credential>
credentialSupplier) {
- return credentialCache.get(cacheKey, key ->
credentialSupplier.apply(cacheKey));
+ public Optional<Credential> getCredential(
+ T cacheKey, Function<T, Optional<Credential>> credentialSupplier) {
+ Credential credential =
+ credentialCache.get(cacheKey, key ->
credentialSupplier.apply(cacheKey).orElse(null));
+ return Optional.ofNullable(credential);
Review Comment:
`Cache#get(key, mappingFunction)` from Caffeine does not permit the mapping
function to return `null`. Here,
`credentialSupplier.apply(cacheKey).orElse(null)` will return `null` when no
credential is available, which will throw a `NullPointerException` inside
Caffeine and reintroduce the original failure mode.
Consider avoiding caching misses (compute supplier result first and only
`put` when present), or change the cache value type to `Optional<Credential>`
(and update the `Expiry` accordingly) so the mapping function always returns
non-null.
##########
common/src/main/java/org/apache/gravitino/credential/CredentialProvider.java:
##########
@@ -66,9 +66,9 @@ default boolean supportsScheme(String scheme) {
* Gets a credential based on the provided context information.
*
* @param context A context object providing necessary information for
retrieving credentials.
- * @return A Credential object containing the authentication information
needed to access a system
- * or resource. Null will be returned if no credential is available.
+ * @return An {@link Optional} containing the {@link Credential} with the
authentication
+ * information needed to access a system or resource, or {@link
Optional#empty()} if no
+ * credential is available.
*/
- @Nullable
- Credential getCredential(CredentialContext context);
+ Optional<Credential> getCredential(CredentialContext context);
}
Review Comment:
Changing `CredentialProvider#getCredential` from returning a nullable
`Credential` to returning `Optional<Credential>` is a breaking API change for
any external credential provider implementations. If this interface is intended
to be implemented outside this repo, consider a compatibility path (e.g.,
introduce a new method name for the Optional-returning variant and deprecate
the old one) or document the breaking change explicitly.
##########
core/src/test/java/org/apache/gravitino/credential/TestJdbcCredentialProvider.java:
##########
@@ -154,9 +155,9 @@ void testEmptyPasswordReturnsNull() {
CredentialProviderFactory.create(JdbcCredential.JDBC_CREDENTIAL_TYPE,
catalogProperties);
CatalogCredentialContext context = new
CatalogCredentialContext("test-user");
- Credential credential = credentialProvider.getCredential(context);
+ Optional<Credential> credential =
credentialProvider.getCredential(context);
- Assertions.assertNull(credential);
+ Assertions.assertFalse(credential.isPresent());
Review Comment:
This test name no longer matches the behavior after switching to
`Optional<Credential>` (it now asserts `Optional.empty()` rather than returning
`null`). Renaming it to something like `testEmptyPasswordReturnsEmptyOptional`
would keep the intent clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
