[#12794] feat(catalog): Support connection tests with proposed changes by mchades · Pull Request #12798 · apache/gravitino

mchades · 2026-09-01T15:37:11Z

What changes were proposed in this pull request?

Extend existing catalog connection testing to accept proposed CatalogChange values.

The changes are applied to a temporary effective catalog configuration before running the existing connection probe. The temporary catalog is always closed, and no catalog configuration or secret material is persisted.

This also adds REST, OpenAPI, Java client, Python client, and regression test coverage.

Why are the changes needed?

The existing API can only test the stored catalog configuration. Users need to validate proposed catalog changes before altering the catalog.

Fix: #12794

Does this PR introduce any user-facing change?

Yes.

Adds testConnection(String, CatalogChange...) for existing catalogs.
Allows an optional CatalogUpdatesRequest body on the existing-catalog connection-test endpoint. Omitting the body retains the existing stored-configuration behavior.
Adds corresponding Java and Python client support.
No property keys are added or removed.

How was this patch tested?

Focused Core, REST, and Java client unit tests.
Hive Docker integration test covering temporary configuration and non-persistence.
Python client unit tests, Black, and Ruff.
Spotless checks.
./gradlew :docs:build :api:javadoc -PskipITs.
git diff --check.

…changes

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Adds support for testing an existing catalog’s connection using proposed (non-persisted) CatalogChange updates, and wires it through REST + OpenAPI and Java/Python clients with regression coverage.

Changes:

Extend server REST endpoint to accept optional CatalogUpdatesRequest and forward translated CatalogChange[] into the dispatcher/manager.
Implement core support for testing with temporary effective entity/config, including secret-handling adjustments for non-persistence.
Add/update OpenAPI plus Java/Python client methods and corresponding tests.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java	Accepts optional updates body and maps update DTOs to `CatalogChange[]` for connection testing.
server/src/test/java/org/apache/gravitino/server/web/rest/TestCatalogOperations.java	Adds REST test coverage for connection test with proposed changes and invalid-change behavior.
docs/open-api/catalogs.yaml	Documents optional request body for existing-catalog connection test endpoint.
core/src/main/java/org/apache/gravitino/catalog/SupportsCatalogs.java	Adds new `testConnection(ident, CatalogChange...)` API to core supports interface.
core/src/main/java/org/apache/gravitino/catalog/CatalogNormalizeDispatcher.java	Validates catalog name (and rename target) for connection tests with proposed changes.
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java	Implements temp effective entity/config application for connection test with proposed changes.
core/src/main/java/org/apache/gravitino/secret/SecretAlterChanges.java	Introduces secret-change preparation for connection tests without secret persistence.
core/src/main/java/org/apache/gravitino/listener/CatalogEventDispatcher.java	Adds new overload passthrough for connection tests with proposed changes.
core/src/main/java/org/apache/gravitino/hook/CatalogHookDispatcher.java	Adds new overload passthrough for connection tests with proposed changes.
core/src/test/java/org/apache/gravitino/catalog/TestCatalogNormalizeDispatcher.java	Adds validation test for invalid rename during connection-test changes.
core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java	Adds tests for temporary config behavior and “no secret mutation” guarantee.
api/src/main/java/org/apache/gravitino/SupportsCatalogs.java	Adds default API method for testing connection with proposed changes.
clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java	Adds Java client method to call connection test with proposed changes.
clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java	Adds Java client forwarding method for proposed-change connection tests.
clients/client-java/src/test/java/org/apache/gravitino/client/TestGravitinoAdminClient.java	Adds unit test coverage for Java client request body on connection test.
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/CatalogIT.java	Adds IT coverage ensuring proposed changes don’t persist and can fail the probe.
clients/client-python/gravitino/client/gravitino_metalake.py	Extends Python client `test_connection` to accept optional changes and send updates request.
clients/client-python/gravitino/client/gravitino_client.py	Extends Python top-level client to forward optional changes.
clients/client-python/tests/unittests/test_metalake.py	Adds unit test for Python client sending `CatalogUpdatesRequest` on changes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

mchades · 2026-09-03T05:39:22Z

+      if (change instanceof CatalogChange.SetSecretBinding) {
+        CatalogChange.SetSecretBinding c = (CatalogChange.SetSecretBinding) change;
+        String property = c.getProperty();
+        SecretBinding binding = c.getBinding();
+        Preconditions.checkArgument(StringUtils.isNotBlank(property), "property must not be blank");
+        Preconditions.checkArgument(binding != null, "binding must not be null");
+        SecretPropertyUtils.validateAlterSecretBindingPlaintext(binding.plaintext());
+        secretManager.buildSecretBindingUrns("catalog", entityId, Map.of(property, binding));
+        out.add(CatalogChange.setProperty(property, binding.plaintext()));


buildSecretBindingUrns(...) did not write or delete secret material; it only validated bindings and providers and constructed URNs. To make the dry-run intent explicit, 0d2abdb adds validateSecretBindingUrns(...), makes buildSecretBindingUrns(...) reuse it, and calls the validation-only method here. The test now also verifies that newly proposed secret material is not created.

mchades · 2026-09-03T05:39:25Z

+  public void testConnection(String catalogName, CatalogChange... changes) throws Exception {
+    List<CatalogUpdateRequest> requests =
+        Arrays.stream(changes)
+            .map(DTOConverters::toCatalogUpdateRequest)
+            .collect(Collectors.toList());
+    CatalogUpdatesRequest updatesRequest = new CatalogUpdatesRequest(requests);
+    updatesRequest.validate();


An empty updates list already passed request validation, but sending a body was unnecessary. In 0d2abdb, an empty changes array delegates to testConnection(catalogName), preserving the exact no-body behavior. A null array is rejected with a clear IllegalArgumentException, consistent with CatalogManager. Both cases are covered by tests.

mchades · 2026-09-03T05:14:31Z

+  public void testConnection(NameIdentifier ident, CatalogChange... changes) throws Exception {
+    validateCatalogName(ident.name());
+    Arrays.stream(changes)
+        .forEach(
+            change -> {


A null changes array cannot reach this dispatcher through the current server call path. When the request body is absent, the REST layer calls testConnection(ident); otherwise, stream().toArray(...) always produces a non-null array, including for an empty updates list. The hook and event dispatchers only forward that array.

There are no other current callers that pass null here, so an additional check in the normalization dispatcher is unnecessary.

mchades · 2026-09-03T05:39:29Z

+            if (request == null) {
+              catalogDispatcher.testConnection(ident);
+            } else {
+              request.validate();
+              CatalogChange[] changes =
+                  request.getUpdates().stream()
+                      .map(CatalogUpdateRequest::catalogChange)
+                      .toArray(CatalogChange[]::new);
+              catalogDispatcher.testConnection(ident, changes);


The cases are already distinct: an omitted body uses testConnection(ident), while {"updates":[]} passes validation and CatalogManager delegates the empty change array to the existing no-change path. {} and {"updates":null} are intentionally invalid because updates is required by the OpenAPI schema. An optional body does not make required fields within a present body optional, so no fallback is needed.

github-actions · 2026-09-01T17:40:02Z

Code Coverage Report

Overall Project	69.0% `+0.22%`	🟢
Files changed	78.45%	🟢

Module	Coverage
aliyun	19.74%	🔴
api	51.57% `-0.17%`	🟢
authorization-common	85.96%	🟢
authorization-ranger	4.38%	🔴
aws	53.54%	🟢
azure	32.1%	🔴
catalog-common	20.87%	🔴
catalog-fileset	82.17%	🟢
catalog-glue	69.24%	🟢
catalog-hive	82.96%	🟢
catalog-jdbc-common	45.69%	🟢
catalog-jdbc-doris	82.69%	🟢
catalog-jdbc-mysql	79.33%	🟢
catalog-jdbc-postgresql	83.39%	🟢
catalog-jdbc-starrocks	79.16%	🟢
catalog-kafka	76.99%	🟢
catalog-lakehouse-generic	60.55%	🟢
catalog-lakehouse-hudi	79.1%	🟢
catalog-lakehouse-iceberg	85.86%	🟢
catalog-lakehouse-paimon	84.29%	🟢
catalog-model	77.99%	🟢
cli	44.51%	🟢
client-java	77.5% `+2.66%`	🟢
common	56.34%	🟢
core	83.88% `-0.59%`	🟢
filesystem-hadoop3	76.48%	🟢
flink	0.0%	🔴
flink-common	52.1%	🟢
flink-runtime	0.0%	🔴
gcp	32.2%	🔴
hadoop-auth	68.0%	🟢
hadoop-common	17.84%	🔴
hive-metastore-common	53.4%	🟢
iceberg-aliyun-bundle	0.0%	🔴
iceberg-common	64.75%	🟢
iceberg-rest-server	75.96%	🟢
idp-basic	86.42%	🟢
integration-test-common	0.0%	🔴
jobs	62.92%	🟢
lance-common	32.52%	🔴
lance-rest-server	65.69%	🟢
lineage	53.02%	🟢
optimizer	83.24%	🟢
optimizer-api	21.95%	🔴
server	89.35% `+0.4%`	🟢
server-common	80.67%	🟢
spark	28.57%	🔴
spark-common	52.04%	🟢
tencent	81.78%	🟢
trino-connector	51.26%	🟢

Files

Module	File	Coverage
api	SupportsCatalogs.java	0.0%	🔴
client-java	GravitinoMetalake.java	91.94%	🟢
	GravitinoClient.java	69.32%	🟢
core	CatalogEventDispatcher.java	93.55%	🟢
	SecretManager.java	79.2%	🟢
	CatalogManager.java	72.41%	🟢
	SecretAlterChanges.java	68.99%	🟢
	CatalogNormalizeDispatcher.java	38.64%	🔴
	CatalogHookDispatcher.java	36.67%	🔴
	SupportsCatalogs.java	12.5%	🔴
server	CatalogOperations.java	100.0%	🟢

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.

mchades · 2026-09-03T05:39:32Z

+  @Override
+  public void testConnection(String catalogName, CatalogChange... changes) throws Exception {
+    List<CatalogUpdateRequest> requests =
+        Arrays.stream(changes)
+            .map(DTOConverters::toCatalogUpdateRequest)
+            .collect(Collectors.toList());
+    CatalogUpdatesRequest updatesRequest = new CatalogUpdatesRequest(requests);
+    updatesRequest.validate();


An empty updates list did not fail validation, but the client should still preserve the exact no-body behavior. This is handled in 0d2abdb: empty arrays delegate to testConnection(catalogName), while null arrays fail with a clear IllegalArgumentException. Tests cover both cases.

mchades · 2026-09-03T05:39:35Z

+   * @throws Exception if the test failed.
+   */
+  @Override
+  public void testConnection(String catalogName, CatalogChange... changes) throws Exception {


GravitinoClient is a forwarding facade, and GravitinoMetalake owns construction of the HTTP request. With the empty-array handling in 0d2abdb, this path already reaches the no-body overload. Adding the same branch here would duplicate the behavior without changing the request.

mchades · 2026-09-03T05:39:38Z

+  @Override
+  public void testConnection(NameIdentifier ident, CatalogChange... changes) throws Exception {
+    validateCatalogName(ident.name());
+    Arrays.stream(changes)
+        .forEach(
+            change -> {
+              if (change instanceof CatalogChange.RenameCatalog) {
+                validateCatalogName(((CatalogChange.RenameCatalog) change).getNewName());
+              }
+            });
+    dispatcher.testConnection(ident, changes);
+  }


This is the same concern as #12798 (comment). A null array cannot reach this dispatcher through the current server call path: an absent body uses the non-varargs overload, while a present body always produces a non-null array. There are no other current callers that pass null here, so no additional guard is needed.

mchades · 2026-09-03T05:39:41Z

+  public static CatalogChange[] prepareCatalogChangesForTest(
+      SecretManager secretManager, long entityId, CatalogChange... changes) {
+    Preconditions.checkArgument(secretManager != null, "secretManager must not be null");
+    Preconditions.checkArgument(changes != null, "changes must not be null");
+
+    List<CatalogChange> out = new ArrayList<>(changes.length);
+    for (CatalogChange change : changes) {
+      if (change instanceof CatalogChange.SetSecretBinding) {
+        CatalogChange.SetSecretBinding c = (CatalogChange.SetSecretBinding) change;
+        String property = c.getProperty();
+        SecretBinding binding = c.getBinding();


buildSecretBindingUrns(...) did not perform secret writes or deletes, but discarding its result obscured that property. Commit 0d2abdb adds a validation-only validateSecretBindingUrns(...) method and uses it in this dry-run path. The test also verifies that newly proposed secret material is not created.

mchades · 2026-09-03T05:39:44Z

+            if (request == null) {
+              catalogDispatcher.testConnection(ident);
+            } else {
+              request.validate();
+              CatalogChange[] changes =
+                  request.getUpdates().stream()
+                      .map(CatalogUpdateRequest::catalogChange)
+                      .toArray(CatalogChange[]::new);
+              catalogDispatcher.testConnection(ident, changes);
+            }


An omitted body is handled by request == null. {"updates":[]} is also valid: it passes validation and CatalogManager delegates the empty change array to the no-change path. {} and {"updates":null} are intentionally rejected because updates is required by the OpenAPI schema, so no additional fallback is needed.

mchades · 2026-09-03T05:39:47Z

+        SecretPropertyUtils.validateAlterSecretBindingPlaintext(binding.plaintext());
+        secretManager.buildSecretBindingUrns("catalog", entityId, Map.of(property, binding));
+        out.add(CatalogChange.setProperty(property, binding.plaintext()));


"catalog" is the stable lowercase entity-type segment used by persisted secret URNs, rather than the Java enum value. Deriving it from EntityType.CATALOG.name() would couple the persisted format to the enum identifier. Existing catalog, schema, and fileset secret paths consistently use these explicit lowercase values; migrating them to shared constants should be done together rather than changing only this call.

jerryshao · 2026-09-02T07:45:09Z

+      }
+    }
+    return out.toArray(new CatalogChange[0]);
+  }


@lasdf1234, can you please review this part?

Got. I'll review this part.

lasdf1234 · 2026-09-02T08:55:19Z

+        Preconditions.checkArgument(StringUtils.isNotBlank(property), "property must not be blank");
+        Preconditions.checkArgument(binding != null, "binding must not be null");
+        SecretPropertyUtils.validateAlterSecretBindingPlaintext(binding.plaintext());
+        secretManager.buildSecretBindingUrns("catalog", entityId, Map.of(property, binding));


secretManager.buildSecretBindingUrns("catalog", entityId, Map.of(property, binding)); This line of code discards the obtained urn. The fundamental reason for this is to verify the legality of the entityType, entityId, property, and binding. Therefore, the buildSecretBindingUrns method was used.
This verification is necessary, but the code's readability is poor.
So, could the explicit validate method be extracted from SecretManager? Extract a validateSecretBindingUrns method.
And buildSecretBindingUrns, will call the validateSecretBindingUrns method for verification.
This part can also be replaced with the validateSecretBindingUrns method.

Implemented in 0d2abdb. SecretManager now exposes validateSecretBindingUrns(...), buildSecretBindingUrns(...) reuses it, and the connection-test path calls the validation method directly. The test also verifies that a newly proposed secret is not written.

lasdf1234 · 2026-09-02T08:57:04Z

@mchades I reviewed the secret part and found a little issue. Could you resolve it?

mchades · 2026-09-03T05:43:58Z

@lasdf1234 @jerryshao all comments resolved; please help review again. Thanks!

lasdf1234 · 2026-09-03T06:05:54Z

@lasdf1234 @jerryshao all comments resolved; please help review again. Thanks!

I have no more comments. The 'secret' related part is good for me.

lasdf1234

LGTM

#12798) Extend existing catalog connection testing to accept proposed `CatalogChange` values. The changes are applied to a temporary effective catalog configuration before running the existing connection probe. The temporary catalog is always closed, and no catalog configuration or secret material is persisted. This also adds REST, OpenAPI, Java client, Python client, and regression test coverage. The existing API can only test the stored catalog configuration. Users need to validate proposed catalog changes before altering the catalog. Fix: #12794 Yes. - Adds `testConnection(String, CatalogChange...)` for existing catalogs. - Allows an optional `CatalogUpdatesRequest` body on the existing-catalog connection-test endpoint. Omitting the body retains the existing stored-configuration behavior. - Adds corresponding Java and Python client support. - No property keys are added or removed. - Focused Core, REST, and Java client unit tests. - Hive Docker integration test covering temporary configuration and non-persistence. - Python client unit tests, Black, and Ruff. - Spotless checks. - `./gradlew :docs:build :api:javadoc -PskipITs`. - `git diff --check`.

…n tests with proposed changes (#12798) (#12884) **Cherry-pick Information:** - Original commit: 2325b41 - Original PR: #12798 - Target branch: `branch-1.3` - Status: ✅ Conflicts resolved manually **Resolution:** - Rebuilt the #12798 change instead of retaining the bot-generated conflict markers. - Preserved the no-body behavior for stored-configuration probes and added temporary rename, comment, set-property, and remove-property changes without persistence. - Kept property validation, catalog read locking, temporary-wrapper cleanup, authorization, REST error handling, and Java/Python client behavior. - Omitted the main-only `SecretAlterChanges` and `SecretManager` changes and secret-specific tests because `branch-1.3` does not contain the Secret API or subsystem. **Validation:** - Compiled the affected API, Core, Server, and Java client production and test sources. - Ran Core, Server, and Java client unit tests. - Ran the targeted Hive Docker integration test covering temporary failure, non-persistence, and subsequent stored-configuration success. - Python client: 1018 unit tests passed; Black 26.3.1 and Pylint 4.0.5 passed. - `./gradlew spotlessApply` - `./gradlew :docs:build :api:javadoc -PskipITs -PskipDockerTests=true` - Conflict-marker scan and `git diff --check` Co-authored-by: mchades <liminghuang@datastrato.com>

[apache#12794] feat(catalog): Support connection tests with proposed …

174cda4

…changes

mchades added the branch-1.3 Automatically cherry-pick commit to branch-1.3 label Sep 1, 2026

mchades requested review from jerryshao and roryqi and a lite review from Copilot September 1, 2026 15:44

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Copilot started reviewing on behalf of mchades September 1, 2026 16:04 View session

jerryshao requested a lite review from Copilot September 2, 2026 07:16

Copilot started reviewing on behalf of jerryshao September 2, 2026 07:32 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

jerryshao reviewed Sep 2, 2026

View reviewed changes

lasdf1234 reviewed Sep 2, 2026

View reviewed changes

fix(catalog): Address connection test review comments

0d2abdb

mchades requested review from jerryshao and lasdf1234 September 3, 2026 05:43

lasdf1234 approved these changes Sep 3, 2026

View reviewed changes

jerryshao approved these changes Sep 3, 2026

View reviewed changes

jerryshao assigned mchades Sep 3, 2026

jerryshao merged commit 2325b41 into apache:main Sep 3, 2026
38 checks passed

mchades mentioned this pull request Sep 3, 2026

[Cherry-pick to branch-1.3] [#12794] feat(catalog): Support connection tests with proposed changes (#12798) #12884

Merged

mchades deleted the codex/12794-test-connection-with-changes branch September 4, 2026 07:42

Uh oh!

Conversation

mchades commented Sep 1, 2026

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 1, 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lasdf1234 commented Sep 2, 2026

Uh oh!

mchades commented Sep 3, 2026

Uh oh!

lasdf1234 commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lasdf1234 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Sep 1, 2026 •

edited

Loading

lasdf1234 commented Sep 3, 2026 •

edited

Loading