mchades commented on code in PR #12798:
URL: https://github.com/apache/gravitino/pull/12798#discussion_r3921420427
##########
core/src/main/java/org/apache/gravitino/secret/SecretAlterChanges.java:
##########
@@ -90,6 +92,58 @@ public static Pair<CatalogChange[], List<SecretMaterial>>
prepareCatalogChanges(
}
}
+ /**
+ * Prepares catalog changes for a connection test without writing or
deleting secret material.
+ *
+ * <p>Write-through bindings are validated but represented by their
plaintext only in the
+ * temporary catalog configuration. External references are converted to
reference URNs so the
+ * temporary catalog resolves them through the configured provider.
+ *
+ * @param secretManager secret manager
+ * @param entityId catalog entity id
+ * @param changes proposed catalog changes
+ * @return effective changes for a temporary catalog entity
+ */
+ 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();
+ 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()));
Review Comment:
`buildSecretBindingUrns(...)` did not write or delete secret material; it
only validated bindings and providers and constructed URNs. To make the dry-run
intent explicit, 0d2abdbaeb 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.
##########
clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java:
##########
@@ -497,6 +497,40 @@ public void testConnection(String catalogName) throws
Exception {
ErrorHandlers.catalogErrorHandler().accept(resp);
}
+ /**
+ * Test the connection of an existing catalog with proposed changes without
persisting them.
+ *
+ * @param catalogName the name of the existing catalog.
+ * @param changes the proposed changes to apply temporarily.
+ * @throws Exception if the test failed.
+ */
+ @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();
Review Comment:
An empty updates list already passed request validation, but sending a body
was unnecessary. In 0d2abdbaeb, 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.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/CatalogOperations.java:
##########
@@ -221,14 +221,24 @@ public Response testExistingConnection(
@PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
String metalake,
@PathParam("catalog") @AuthorizationMetadata(type =
Entity.EntityType.CATALOG)
- String catalogName) {
+ String catalogName,
+ CatalogUpdatesRequest request) {
LOG.info("Received test connection request for existing catalog: {}.{}",
metalake, catalogName);
try {
return Utils.doAs(
httpRequest,
() -> {
NameIdentifier ident = NameIdentifierUtil.ofCatalog(metalake,
catalogName);
- catalogDispatcher.testConnection(ident);
+ if (request == null) {
+ catalogDispatcher.testConnection(ident);
+ } else {
+ request.validate();
+ CatalogChange[] changes =
+ request.getUpdates().stream()
+ .map(CatalogUpdateRequest::catalogChange)
+ .toArray(CatalogChange[]::new);
+ catalogDispatcher.testConnection(ident, changes);
Review Comment:
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.
##########
clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java:
##########
@@ -497,6 +497,40 @@ public void testConnection(String catalogName) throws
Exception {
ErrorHandlers.catalogErrorHandler().accept(resp);
}
+ /**
+ * Test the connection of an existing catalog with proposed changes without
persisting them.
+ *
+ * @param catalogName the name of the existing catalog.
+ * @param changes the proposed changes to apply temporarily.
+ * @throws Exception if the test failed.
+ */
+ @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();
Review Comment:
An empty updates list did not fail validation, but the client should still
preserve the exact no-body behavior. This is handled in 0d2abdbaeb: empty
arrays delegate to `testConnection(catalogName)`, while null arrays fail with a
clear `IllegalArgumentException`. Tests cover both cases.
##########
clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java:
##########
@@ -605,6 +605,18 @@ public void testConnection(String catalogName) throws
Exception {
getMetalake().testConnection(catalogName);
}
+ /**
+ * Test the connection of an existing catalog with proposed changes without
persisting them.
+ *
+ * @param catalogName the name of the existing catalog.
+ * @param changes the proposed changes to apply temporarily.
+ * @throws Exception if the test failed.
+ */
+ @Override
+ public void testConnection(String catalogName, CatalogChange... changes)
throws Exception {
Review Comment:
`GravitinoClient` is a forwarding facade, and `GravitinoMetalake` owns
construction of the HTTP request. With the empty-array handling in 0d2abdbaeb,
this path already reaches the no-body overload. Adding the same branch here
would duplicate the behavior without changing the request.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogNormalizeDispatcher.java:
##########
@@ -148,6 +148,19 @@ public void testConnection(NameIdentifier ident) throws
Exception {
dispatcher.testConnection(ident);
}
+ @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);
+ }
Review Comment:
This is the same concern as
https://github.com/apache/gravitino/pull/12798#discussion_r3905969163. 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.
--
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]
