Copilot commented on code in PR #12798:
URL: https://github.com/apache/gravitino/pull/12798#discussion_r3911784250
##########
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:
`testConnection(String, CatalogChange...)` does not preserve the documented
“no changes == existing behavior” semantics. If a caller passes an empty
varargs array (e.g., `testConnection(name, changesArray)` where
`changesArray.length == 0`), this will build an empty `CatalogUpdatesRequest`
and `validate()` is likely to fail, even though the API/server behavior treats
“no changes” as valid. Fix by explicitly short-circuiting on `changes == null
|| changes.length == 0` and delegating to `testConnection(catalogName)` (no
request body).
##########
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:
This method forwards an empty `changes` array to
`GravitinoMetalake.testConnection(String, CatalogChange...)`. With the current
Metalake implementation, `changes.length == 0` can incorrectly fail (see
related issue). After fixing Metalake, this becomes harmless, but it’s still
safer/clearer to mirror the default API behavior here too by delegating to
`testConnection(catalogName)` when `changes` is empty.
##########
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:
`Arrays.stream(changes)` will throw a `NullPointerException` if a caller
passes `null` for the varargs array (e.g., `testConnection(ident,
(CatalogChange[]) null)`). Other implementations (e.g., `CatalogManager`)
explicitly guard against null. Add a precondition/check here (and potentially
treat null as empty) to fail with a clear `IllegalArgumentException` and
maintain consistent behavior across dispatch layers.
##########
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();
Review Comment:
This method’s contract says it prepares changes “without writing or deleting
secret material”, but `prepareCatalogChangesForTest` currently calls into
`SecretManager` methods (e.g., `buildSecretBindingUrns(...)`) that may have
side effects depending on implementation/provider. To keep the “non-persistent”
guarantee robust, prefer using validation-only helpers (or explicit “dry-run”
APIs) and avoid invoking SecretManager code paths that might write/delete
secrets as part of URN creation.
##########
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:
Avoid hard-coding the entity type string `"catalog"` here. This is a “magic
string” that can drift from the rest of the system (constants/enums) and makes
refactors error-prone. Prefer a shared constant (e.g.,
`EntityType.CATALOG`-derived value) or a dedicated constant in the secrets
layer.
##########
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 endpoint is documented as having an *optional* request body, but an
“empty body that deserializes to a non-null `CatalogUpdatesRequest` with
empty/null `updates`” will likely be rejected by `validate()` instead of
behaving like “no body”. To better preserve backward-compatible behavior across
HTTP clients/proxies that may send `{}` or an empty JSON body, consider
treating `request == null` *or* `request.getUpdates()` being null/empty as the
no-changes path.
--
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]
