[#11154] fix(authz): Fix the NullPointerException when filesets request the credential by roryqi · Pull Request #11155 · apache/gravitino

roryqi · 2026-05-19T13:58:12Z

What changes were proposed in this pull request?

Fix the NullPointerException when filesets request the credential

Why are the changes needed?

Fix: #11154

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add UTs.

… request the credential

Change CredentialProvider.getCredential and downstream CatalogCredentialManager / CredentialCache methods to return Optional<Credential> instead of @nullable Credential, and update CredentialOperationDispatcher and CatalogWrapperForREST callers accordingly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…urn Optional Apply the Optional<Credential> return type change to JdbcCredentialProvider and its tests, which were added to upstream/main after the previous commit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

github-actions · 2026-05-19T19:35:29Z

Code Coverage Report

Overall Project	66.15% `-0.01%`	🟢
Files changed	64.63%	🟢

Module	Coverage
aliyun	1.72%	🔴
api	46.83%	🟢
authorization-common	85.96%	🟢
aws	1.08%	🔴
azure	2.47%	🔴
catalog-common	10.2%	🔴
catalog-fileset	80.02%	🟢
catalog-glue	64.03%	🟢
catalog-hive	79.59%	🟢
catalog-jdbc-clickhouse	80.02%	🟢
catalog-jdbc-common	44.46%	🟢
catalog-jdbc-doris	80.28%	🟢
catalog-jdbc-hologres	54.03%	🟢
catalog-jdbc-mysql	79.23%	🟢
catalog-jdbc-oceanbase	78.38%	🟢
catalog-jdbc-postgresql	82.17%	🟢
catalog-jdbc-starrocks	78.27%	🟢
catalog-kafka	77.01%	🟢
catalog-lakehouse-generic	44.89%	🟢
catalog-lakehouse-hudi	79.1%	🟢
catalog-lakehouse-iceberg	85.87%	🟢
catalog-lakehouse-paimon	77.25%	🟢
catalog-model	77.72%	🟢
cli	44.51%	🟢
client-java	77.92%	🟢
common	49.99% `-0.19%`	🟢
core	82.47% `-0.21%`	🟢
filesystem-hadoop3	76.97%	🟢
flink	0.0%	🔴
flink-common	43.17%	🟢
flink-runtime	0.0%	🔴
gcp	14.12%	🔴
hadoop-common	10.39%	🔴
hive-metastore-common	53.26%	🟢
iceberg-common	55.46%	🟢
iceberg-rest-server	69.77% `+0.44%`	🟢
idp-basic	94.17%	🟢
integration-test-common	0.0%	🔴
jobs	66.17%	🟢
lance-common	20.9%	🔴
lance-rest-server	62.78%	🟢
lineage	53.02%	🟢
optimizer	82.87%	🟢
optimizer-api	21.95%	🔴
server	85.75%	🟢
server-common	71.17%	🟢
spark	32.79%	🔴
spark-common	39.09%	🔴
trino-connector	34.82%	🔴

Files

Module	File	Coverage
common	CredentialProvider.java	0.0%	🔴
	CredentialProviderDelegator.java	0.0%	🔴
core	CredentialCache.java	93.55%	🟢
	JdbcCredentialProvider.java	90.91%	🟢
	CredentialOperationDispatcher.java	75.0%	🟢
	CatalogCredentialManager.java	1.64%	🔴
iceberg-rest-server	CatalogWrapperForREST.java	72.77%	🟢

Copilot

Pull request overview

This PR addresses a NullPointerException when filesets request credentials by making credential retrieval explicitly optional and ensuring missing credentials are skipped/handled safely across the credential vending flow.

Changes:

Updated CredentialProvider and related implementations to return Optional<Credential> instead of nullable Credential.
Updated credential vending call sites (dispatcher and Iceberg REST wrapper) to handle missing credentials via Optional (ifPresent, orElseThrow).
Updated/added unit tests to validate behavior when a provider returns no credential.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/extension/DummyCredentialProvider.java	Updates test provider to return `Optional<Credential>`.
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java	Switches to `Optional` flow and throws when no credential is available.
core/src/test/java/org/apache/gravitino/credential/TestJdbcCredentialProvider.java	Updates JDBC provider tests for `Optional` return values.
core/src/test/java/org/apache/gravitino/credential/TestCredentialProvider.java	Adapts dummy provider test to `Optional` API.
core/src/test/java/org/apache/gravitino/credential/TestCredentialOperationDispatcher.java	Adds coverage ensuring missing credentials are skipped.
core/src/test/java/org/apache/gravitino/credential/DummyCredentialProvider.java	Updates dummy provider to return `Optional`.
core/src/test/java/org/apache/gravitino/credential/Dummy2CredentialProvider.java	Updates dummy provider to return `Optional`.
core/src/main/java/org/apache/gravitino/credential/JdbcCredentialProvider.java	Returns `Optional.empty()` when JDBC properties are missing/blank.
core/src/main/java/org/apache/gravitino/credential/CredentialOperationDispatcher.java	Skips absent credentials via `Optional#ifPresent`.
core/src/main/java/org/apache/gravitino/credential/CredentialCache.java	Changes cache API to `Optional`, but currently maps empty to `null` (see review comments).
core/src/main/java/org/apache/gravitino/credential/CatalogCredentialManager.java	Propagates `Optional` through manager APIs.
common/src/main/java/org/apache/gravitino/credential/CredentialProviderDelegator.java	Wraps generator output with `Optional.ofNullable`.
common/src/main/java/org/apache/gravitino/credential/CredentialProvider.java	Changes interface contract to `Optional<Credential>`.
bundles/azure/src/main/java/org/apache/gravitino/abs/credential/AzureAccountKeyProvider.java	Updates provider to return `Optional`.
bundles/aws/src/main/java/org/apache/gravitino/s3/credential/S3SecretKeyProvider.java	Updates provider to return `Optional`.
bundles/aliyun/src/main/java/org/apache/gravitino/oss/credential/OSSSecretKeyProvider.java	Updates provider to return `Optional`.

roryqi · 2026-05-20T02:41:31Z

+    Credential credential =
+        credentialCache.get(cacheKey, key -> credentialSupplier.apply(cacheKey).orElse(null));
+    return Optional.ofNullable(credential);


unacceptable. It will breaks the atomic.

   * 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);
 }


roryqi · 2026-05-20T08:33:35Z

    CredentialProvider credentialProvider =
        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());


Rename testEmptyPasswordReturnsNull to testEmptyPasswordReturnsEmptyOptional to match the behavior after switching getCredential to return Optional. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

roryqi · 2026-05-20T08:33:48Z

+  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);


roryqi · 2026-05-20T08:33:58Z

  /**
   * 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);
 }


…al credential Fix the Caffeine cache mapping function returning null and keep the CredentialProvider SPI backward compatible. - CredentialCache stores a non-null Optional<Credential> so Cache#get stays atomic; the Expiry calculator handles the empty case without calling expireTimeInMs(). - Restore the legacy @nullable Credential getCredential(CredentialContext) as a deprecated default and add a new Optional-returning getCredentialOptional(CredentialContext), preserving binary/source compatibility for ServiceLoader-discovered implementations. - Update first-party providers, callers, and tests to the new method. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

roryqi · 2026-05-20T08:43:11Z

@diqiu50 Could u take a look again?

diqiu50 · 2026-05-20T09:15:45Z

+    Method method =
+        CredentialOperationDispatcher.class.getDeclaredMethod(
+            "getCredentials", BaseCatalog.class, NameIdentifier.class, CredentialPrivilege.class);
+    method.setAccessible(true);


Why don’t we call dispatcher.getCredentials() directly?

diqiu50 · 2026-05-20T09:29:31Z

+      // Do not cache the absence of a credential, expire it immediately.
+      if (!credential.isPresent()) {
+        return 0;
+      }


Add a test to cover this case.

- Call CredentialOperationDispatcher.getCredentials directly in the test instead of via reflection (made the overload package-private). - Add TestCredentialCache covering that an absent credential is not cached while a present one is. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…edential Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…l abstract Make getCredential the abstract SPI method that providers implement, with getCredentialOptional kept as a default wrapper. Update all in-repo providers (delegator, JDBC, Azure, OSS, S3 secret-key) and test providers to implement getCredential. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

diqiu50

LGTM

[apache#11154] fix(authz): Fix the NullPointerException when filesets…

abf4b46

… request the credential

roryqi force-pushed the minor_fix123 branch from 61303a8 to abf4b46 Compare May 19, 2026 14:13

roryqi requested a review from diqiu50 May 19, 2026 14:14

roryqi self-assigned this May 19, 2026

roryqi and others added 3 commits May 19, 2026 15:34

Merge remote-tracking branch 'upstream/main' into minor_fix123

2fc42c6

diqiu50 requested a review from Copilot May 20, 2026 02:34

Copilot started reviewing on behalf of diqiu50 May 20, 2026 02:34 View session

Copilot AI reviewed May 20, 2026

View reviewed changes

[MINOR] improvement(credential): Rename test to reflect Optional return

8488dfe

Rename testEmptyPasswordReturnsNull to testEmptyPasswordReturnsEmptyOptional to match the behavior after switching getCredential to return Optional. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

jerryshao requested a review from Copilot May 20, 2026 06:18

Copilot started reviewing on behalf of jerryshao May 20, 2026 06:18 View session

Copilot AI reviewed May 20, 2026

View reviewed changes

diqiu50 reviewed May 20, 2026

View reviewed changes

roryqi and others added 3 commits May 20, 2026 09:45

[MINOR] improvement(credential): Undeprecate CredentialProvider.getCr…

c344835

…edential Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

diqiu50 approved these changes May 20, 2026

View reviewed changes

diqiu50 merged commit f06c6d8 into apache:main May 20, 2026
31 checks passed

roryqi deleted the minor_fix123 branch May 21, 2026 02:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#11154] fix(authz): Fix the NullPointerException when filesets request the credential#11155

[#11154] fix(authz): Fix the NullPointerException when filesets request the credential#11155
diqiu50 merged 9 commits into
apache:mainfrom
qqqttt123:minor_fix123

roryqi commented May 19, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 19, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

roryqi May 20, 2026

Uh oh!

roryqi May 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

roryqi May 20, 2026

Uh oh!

roryqi May 20, 2026

Uh oh!

roryqi commented May 20, 2026

Uh oh!

diqiu50 May 20, 2026

Uh oh!

diqiu50 May 20, 2026

Uh oh!

roryqi May 20, 2026

Uh oh!

diqiu50 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

roryqi commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

github-actions Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

roryqi May 20, 2026

Choose a reason for hiding this comment

Uh oh!

roryqi May 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

roryqi May 20, 2026

Choose a reason for hiding this comment

Uh oh!

roryqi May 20, 2026

Choose a reason for hiding this comment

Uh oh!

roryqi commented May 20, 2026

Uh oh!

diqiu50 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

diqiu50 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

roryqi May 20, 2026

Choose a reason for hiding this comment

Uh oh!

diqiu50 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

roryqi commented May 19, 2026 •

edited

Loading

github-actions Bot commented May 19, 2026 •

edited

Loading