Copilot commented on code in PR #10895:
URL: https://github.com/apache/gravitino/pull/10895#discussion_r3213152051
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/rest/LanceTableOperations.java:
##########
@@ -90,16 +96,37 @@ public Response describeTable(
DescribeTableRequest request) {
try {
validateDescribeTableRequest(request);
+ boolean vendCredentials =
+ request.getVendCredentials() == null ||
Boolean.TRUE.equals(request.getVendCredentials());
+ CredentialPrivilege privilege =
+ vendCredentials ? getCredentialPrivilege(tableId, delimiter) : null;
Review Comment:
`vendCredentials` is treated as `true` when the request field is omitted
(`null`). This makes credential vending effectively enabled-by-default for
clients that don’t send the new flag, which conflicts with the PR description’s
“opt-in” behavior and changes responses for catalogs that have credential
providers configured. Consider defaulting to `false` when `vendCredentials` is
`null` (and only vending when it is explicitly `true`).
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java:
##########
@@ -131,10 +148,61 @@ public DescribeTableResponse describeTable(
Optional.ofNullable(table.properties().get(LANCE_TABLE_VERSION))
.map(Long::valueOf)
.orElse(null));
-
response.setStorageOptions(LancePropertiesUtils.getLanceStorageOptions(table.properties()));
+
+ if (credentialPrivilege != null) {
+ response.setStorageOptions(
+ buildVendedStorageOptions(catalogName, catalog, table,
credentialPrivilege));
+ } else {
+
response.setStorageOptions(LancePropertiesUtils.getLanceStorageOptions(table.properties()));
+ }
+
return response;
}
+ private Map<String, String> buildVendedStorageOptions(
+ String catalogName, Catalog catalog, Table table, CredentialPrivilege
credentialPrivilege) {
+ String tableLocation = table.properties().get(LANCE_LOCATION);
+ Preconditions.checkArgument(
+ tableLocation != null && !tableLocation.isEmpty(),
+ "Table location is required for credential vending");
+
+ ImmutableSet<String> paths = ImmutableSet.of(tableLocation);
+ String userName = PrincipalUtils.getCurrentUserName();
+
+ PathBasedCredentialContext context =
+ credentialPrivilege == CredentialPrivilege.WRITE
+ ? new PathBasedCredentialContext(userName, paths,
ImmutableSet.of())
+ : new PathBasedCredentialContext(userName, ImmutableSet.of(),
paths);
+
+ CatalogCredentialManager credManager =
+ credentialManagers.computeIfAbsent(
+ catalogName, name -> new CatalogCredentialManager(name,
catalog.properties()));
+
Review Comment:
`credentialManagers.computeIfAbsent(catalogName, …)` caches a
`CatalogCredentialManager` indefinitely using the catalog properties from the
first request. This can (1) leak resources because `CatalogCredentialManager`
is `Closeable` and is never closed/evicted here, and (2) make
credential-provider config changes (altered catalog properties) not take effect
until process restart. Prefer using an existing per-catalog manager with proper
lifecycle (e.g., if the loaded `Catalog` is a `BaseCatalog`, use its
`catalogCredentialManager()`), or introduce a cache with invalidation/close
semantics when catalog properties change or catalogs are dropped.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -960,6 +962,63 @@ void testDeclareTable() {
Assertions.assertFalse(new File(anotherLocation).exists());
}
+ @Test
+ void testDescribeTableWithCredentialVending() {
+ // Skip in deploy mode: the dummy credential provider is only available in
test classpath
+ org.junit.Assume.assumeTrue(
+ "Credential vending IT only runs in embedded mode",
+ ITUtils.EMBEDDED_TEST_MODE.equals(testMode));
Review Comment:
This test uses `org.junit.Assume.assumeTrue(...)` (JUnit 4). The
lance-rest-server module is configured for JUnit 5 (`useJUnitPlatform()` and
Jupiter deps), and doesn’t declare a JUnit 4 dependency, so this is likely to
fail compilation or won’t be handled as a skipped test. Use
`org.junit.jupiter.api.Assumptions.assumeTrue(...)` instead.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/integration/test/LanceRESTServiceIT.java:
##########
@@ -960,6 +962,63 @@ void testDeclareTable() {
Assertions.assertFalse(new File(anotherLocation).exists());
}
+ @Test
+ void testDescribeTableWithCredentialVending() {
+ // Skip in deploy mode: the dummy credential provider is only available in
test classpath
+ org.junit.Assume.assumeTrue(
+ "Credential vending IT only runs in embedded mode",
+ ITUtils.EMBEDDED_TEST_MODE.equals(testMode));
+
+ // Create a catalog with credential-providers configured to use the dummy
provider
+ String credCatalogName =
GravitinoITUtils.genRandomName("lance_cred_catalog");
+ Map<String, String> credCatalogProps =
+ new HashMap<>() {
+ {
+ put("credential-providers",
LanceDummyCredentialProvider.CREDENTIAL_TYPE);
+ }
+ };
Review Comment:
Avoid double-brace initialization (`new HashMap<>() {{ ... }}`) in tests: it
creates an anonymous class and can retain a reference to the outer instance,
making tests harder to reason about. Prefer `Map.of(...)` for a single entry or
create a plain `HashMap` and `put` into it.
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestLanceNamespaceOperations.java:
##########
@@ -136,6 +136,10 @@ protected void configure() {
public static void setup() {
when(namespaceWrapper.asNamespaceOps()).thenReturn(namespaceOps);
when(namespaceWrapper.asTableOps()).thenReturn(tableOps);
+ org.apache.gravitino.lance.common.config.LanceConfig lanceConfig =
+ mock(org.apache.gravitino.lance.common.config.LanceConfig.class);
+ when(namespaceWrapper.config()).thenReturn(lanceConfig);
+ when(lanceConfig.getGravitinoMetalake()).thenReturn("test-metalake");
Review Comment:
Avoid using fully-qualified class names inside methods for types without
name collisions (e.g., `org.apache.gravitino.lance.common.config.LanceConfig`).
Import the class and use `LanceConfig` directly to match the repo’s Java import
hygiene guideline and keep the test readable.
##########
lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java:
##########
@@ -68,13 +79,16 @@ public class GravitinoLanceTableOperations implements
LanceTableOperations {
public static final Logger LOG =
LoggerFactory.getLogger(GravitinoLanceTableOperations.class);
+ private final GravitinoLanceNamespaceWrapper namespaceWrapper;
+
+ private final ConcurrentHashMap<String, CatalogCredentialManager>
credentialManagers =
+ new ConcurrentHashMap<>();
+
private static final Map<Class<?>, GravitinoLanceTableAlterHandler<?, ?>>
ALTER_HANDLERS =
Map.of(
AlterTableDropColumnsRequest.class, new DropColumns(),
AlterTableAlterColumnsRequest.class, new
AlterColumnsGravitinoLance());
Review Comment:
Class member ordering: `ALTER_HANDLERS` is a `static final` field but is
declared after instance fields (`namespaceWrapper`, `credentialManagers`). The
codebase guideline expects static constants/fields before instance fields;
consider moving `ALTER_HANDLERS` above the instance fields to keep a consistent
member layout.
--
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]
