mchades commented on code in PR #12798:
URL: https://github.com/apache/gravitino/pull/12798#discussion_r3921422701
##########
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:
`buildSecretBindingUrns(...)` did not perform secret writes or deletes, but
discarding its result obscured that property. Commit 0d2abdbaeb 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.
##########
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:
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.
##########
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:
`"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.
##########
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));
Review Comment:
Implemented in 0d2abdbaeb. `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.
--
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]
