Copilot commented on code in PR #11155:
URL: https://github.com/apache/gravitino/pull/11155#discussion_r3271638111
##########
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:
`credentialCache.get(cacheKey, mappingFunction)` (Caffeine) does not allow
the mapping function to return `null`. Here
`credentialSupplier.apply(cacheKey).orElse(null)` will return `null` when the
supplier returns `Optional.empty()`, which will throw a `NullPointerException`
inside Caffeine and reintroduce the original failure mode.
To keep `Cache#get` atomic while supporting “no credential available”, store
a non-null wrapper in the cache (e.g., `Cache<T, Optional<Credential>>` or a
dedicated non-null sentinel value) and update the `Expiry` logic to handle the
empty/sentinel case without calling `expireTimeInMs()`.
##########
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:
PR description says there is no user-facing change, but changing
`CredentialProvider#getCredential` from returning `Credential` (nullable) to
returning `Optional<Credential>` is a binary- and source-incompatible change
for any external `CredentialProvider` implementations discovered via
`ServiceLoader`.
If this SPI is intended for third-party implementations, consider preserving
backward compatibility (e.g., keep the old method signature and treat `null` as
“no credential” at call sites, or introduce a new optional-returning method
while keeping the old one deprecated) and/or update the PR description to
reflect the breaking change.
--
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]
