Copilot commented on code in PR #12798:
URL: https://github.com/apache/gravitino/pull/12798#discussion_r3905969072
##########
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:
The method’s contract states it prepares changes for a connection test
*without writing or deleting secret material*, but it calls
`secretManager.buildSecretBindingUrns(...)`. If that method writes secrets (as
the name strongly suggests in a write-through flow), this would violate the
non-persistence guarantee and could leak secret material. Prefer a
validation-only path here (e.g., validate provider existence/config + plaintext
constraints) that does not create/write secrets; if such an API doesn’t exist,
consider adding a dedicated `validateSecretBinding(...)` method on
`SecretManager` and using it here. Also consider strengthening the tests to
assert that no *new* secret material is created, not just that the old one
remains readable.
##########
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 -> {
Review Comment:
`Arrays.stream(changes)` will throw a `NullPointerException` if a caller
passes `null` for the varargs array. Add an explicit argument check (e.g.,
`Preconditions.checkArgument(changes != null, ...)`) to fail with a clearer
error, consistent with `CatalogManager.testConnection(... )` which already
validates `changes != null`.
##########
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:
This overload doesn’t preserve the documented/expected behavior for an empty
`changes` varargs: it always constructs a `CatalogUpdatesRequest` and validates
it, which will typically fail when `changes.length == 0`. Align with the
server/default API behavior by short-circuiting: when `changes == null` or
`changes.length == 0`, delegate to `testConnection(String catalogName)` and
avoid sending a body.
##########
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 advertises an optional request body; in practice,
clients/proxies sometimes send `{}` or an empty updates list. As written, any
non-null body will go through `request.validate()` and then
`request.getUpdates().stream()`, which likely turns an 'empty/omitted changes'
payload into an application error instead of behaving like 'no changes'.
Consider treating `null` *or empty updates* as 'no proposed changes' (delegate
to `testConnection(ident)`), or explicitly document in OpenAPI that the body—if
provided—must include a non-empty `updates` list.
--
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]
