This is an automated email from the ASF dual-hosted git repository.
yuqi4733 pushed a commit to branch fix-10545-batch-load-securable-objects
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to
refs/heads/fix-10545-batch-load-securable-objects by this push:
new 81098981cb [#10545] Address Copilot review comments on batch
securable-object loading
81098981cb is described below
commit 81098981cb19f6c61f134fcf56e8e2df39729a8a
Author: yuqi <[email protected]>
AuthorDate: Fri Apr 10 15:00:39 2026 +0800
[#10545] Address Copilot review comments on batch securable-object loading
- Add try/catch around batchListSecurableObjectsForRoles with actionable
context (metalake, userId, roleIds)
- Remove unused executor field and thread pool from JcasbinAuthorizer (no
longer used after N+1 fix)
- Fix fully-qualified ImmutableMap.Builder reference in
TestJcasbinAuthorizer → use import
- Add testBatchListSecurableObjectsForRoles to TestRoleMetaService covering
multiple roles and missing role ID
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
---
.../relational/service/TestRoleMetaService.java | 61 ++++++++++++++++++++++
.../authorization/jcasbin/JcasbinAuthorizer.java | 39 +++++---------
.../jcasbin/TestJcasbinAuthorizer.java | 23 +-------
3 files changed, 77 insertions(+), 46 deletions(-)
diff --git
a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestRoleMetaService.java
b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestRoleMetaService.java
index fbd17a31d7..d9ee5de0ab 100644
---
a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestRoleMetaService.java
+++
b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestRoleMetaService.java
@@ -286,6 +286,67 @@ class TestRoleMetaService extends TestJDBCBackend {
}
}
+ @TestTemplate
+ void testBatchListSecurableObjectsForRoles() throws IOException {
+ createAndInsertMakeLake(METALAKE_NAME);
+ String catalogName1 = "catalog1";
+ String catalogName2 = "catalog2";
+ createAndInsertCatalog(METALAKE_NAME, catalogName1);
+ createAndInsertCatalog(METALAKE_NAME, catalogName2);
+
+ RoleMetaService roleMetaService = RoleMetaService.getInstance();
+
+ // Create role1 with a securable object on catalog1
+ RoleEntity role1 =
+ createRoleEntity(
+ RandomIdGenerator.INSTANCE.nextId(),
+ AuthorizationUtils.ofRoleNamespace(METALAKE_NAME),
+ "role1",
+ AUDIT_INFO,
+ SecurableObjects.ofCatalog(
+ catalogName1,
Lists.newArrayList(Privileges.UseCatalog.allow())),
+ ImmutableMap.of());
+ roleMetaService.insertRole(role1, false);
+
+ // Create role2 with a securable object on catalog2
+ RoleEntity role2 =
+ createRoleEntity(
+ RandomIdGenerator.INSTANCE.nextId(),
+ AuthorizationUtils.ofRoleNamespace(METALAKE_NAME),
+ "role2",
+ AUDIT_INFO,
+ SecurableObjects.ofCatalog(
+ catalogName2,
Lists.newArrayList(Privileges.UseCatalog.allow())),
+ ImmutableMap.of());
+ roleMetaService.insertRole(role2, false);
+
+ // Batch-load securable objects for both roles
+ Map<Long, List<SecurableObject>> result =
+ RoleMetaService.batchListSecurableObjectsForRoles(
+ Lists.newArrayList(role1.id(), role2.id()));
+
+ assertEquals(2, result.size());
+
+ List<SecurableObject> role1SecObjs = result.get(role1.id());
+ Assertions.assertNotNull(role1SecObjs);
+ assertEquals(1, role1SecObjs.size());
+ assertEquals(catalogName1, role1SecObjs.get(0).name());
+
+ List<SecurableObject> role2SecObjs = result.get(role2.id());
+ Assertions.assertNotNull(role2SecObjs);
+ assertEquals(1, role2SecObjs.size());
+ assertEquals(catalogName2, role2SecObjs.get(0).name());
+
+ // A role ID that does not exist should return an empty list
+ long nonExistentRoleId = -9999L;
+ Map<Long, List<SecurableObject>> resultWithMissing =
+ RoleMetaService.batchListSecurableObjectsForRoles(
+ Lists.newArrayList(role1.id(), nonExistentRoleId));
+ assertEquals(2, resultWithMissing.size());
+ assertEquals(1, resultWithMissing.get(role1.id()).size());
+ assertEquals(0, resultWithMissing.get(nonExistentRoleId).size());
+ }
+
@TestTemplate
void testInsertRole() throws IOException {
createAndInsertMakeLake(METALAKE_NAME);
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
index 62220b1205..31eb41e383 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java
@@ -32,9 +32,6 @@ import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
-import java.util.concurrent.Executor;
-import java.util.concurrent.Executors;
-import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import org.apache.commons.io.IOUtils;
@@ -92,8 +89,6 @@ public class JcasbinAuthorizer implements GravitinoAuthorizer
{
private Cache<Long, Optional<Long>> ownerRel;
- private Executor executor = null;
-
@Override
public void initialize() {
long cacheExpirationSecs =
@@ -129,16 +124,6 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
.expireAfterAccess(cacheExpirationSecs, TimeUnit.SECONDS)
.maximumSize(ownerCacheSize)
.build();
- executor =
- Executors.newFixedThreadPool(
- GravitinoEnv.getInstance()
- .config()
- .get(Configs.GRAVITINO_AUTHORIZATION_THREAD_POOL_SIZE),
- runnable -> {
- Thread thread = new Thread(runnable);
- thread.setName("GravitinoAuthorizer-ThreadPool-" +
thread.getId());
- return thread;
- });
}
private Model getModel(String modelFilePath) {
@@ -416,14 +401,7 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
}
@Override
- public void close() throws IOException {
- if (executor != null) {
- if (executor instanceof ThreadPoolExecutor) {
- ThreadPoolExecutor threadPoolExecutor = (ThreadPoolExecutor) executor;
- threadPoolExecutor.shutdown();
- }
- }
- }
+ public void close() throws IOException {}
private class InternalAuthorizer {
@@ -525,8 +503,19 @@ public class JcasbinAuthorizer implements
GravitinoAuthorizer {
// eliminating the N+1 pattern of per-role entityStore.get() calls.
List<Long> unloadedRoleIds =
unloadedRoleStubs.stream().map(RoleEntity::id).collect(Collectors.toList());
- Map<Long, List<SecurableObject>> secObjsByRoleId =
-
RoleMetaService.batchListSecurableObjectsForRoles(unloadedRoleIds);
+ Map<Long, List<SecurableObject>> secObjsByRoleId;
+ try {
+ secObjsByRoleId =
RoleMetaService.batchListSecurableObjectsForRoles(unloadedRoleIds);
+ } catch (RuntimeException e) {
+ throw new RuntimeException(
+ "Failed to batch-load securable objects for roles "
+ + unloadedRoleIds
+ + " of metalake "
+ + metalake
+ + ", userId "
+ + userId,
+ e);
+ }
for (RoleEntity stub : unloadedRoleStubs) {
List<SecurableObject> securableObjects =
diff --git
a/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
b/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
index dbca29ad0c..9a46a0a7c4 100644
---
a/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
+++
b/server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java
@@ -34,6 +34,7 @@ import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.github.benmanes.caffeine.cache.Cache;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
import java.io.IOException;
import java.lang.reflect.Field;
import java.security.Principal;
@@ -42,9 +43,7 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
-import java.util.concurrent.Executor;
import java.util.stream.Collectors;
-import org.apache.commons.lang3.reflect.FieldUtils;
import org.apache.gravitino.Entity;
import org.apache.gravitino.EntityStore;
import org.apache.gravitino.GravitinoEnv;
@@ -134,8 +133,7 @@ public class TestJcasbinAuthorizer {
.thenAnswer(
inv -> {
List<Long> ids = inv.getArgument(0);
- com.google.common.collect.ImmutableMap.Builder<Long,
List<SecurableObject>> result =
- com.google.common.collect.ImmutableMap.builder();
+ ImmutableMap.Builder<Long, List<SecurableObject>> result =
ImmutableMap.builder();
for (Long id : ids) {
result.put(id, roleSecurableObjectsMap.getOrDefault(id,
ImmutableList.of()));
}
@@ -201,7 +199,6 @@ public class TestJcasbinAuthorizer {
@Test
public void testAuthorize() throws Exception {
- makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
Principal currentPrincipal = PrincipalUtils.getCurrentPrincipal();
assertFalse(doAuthorize(currentPrincipal));
RoleEntity allowRole =
@@ -380,21 +377,8 @@ public class TestJcasbinAuthorizer {
"testCatalog2", getDenySecurableObjectPO(),
MetadataObject.Type.CATALOG);
}
- private static void makeCompletableFutureUseCurrentThread(JcasbinAuthorizer
jcasbinAuthorizer) {
- try {
- Executor currentThread = Runnable::run;
- Class<JcasbinAuthorizer> jcasbinAuthorizerClass =
JcasbinAuthorizer.class;
- Field field = jcasbinAuthorizerClass.getDeclaredField("executor");
- field.setAccessible(true);
- FieldUtils.writeField(field, jcasbinAuthorizer, currentThread);
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
- }
-
@Test
public void testRoleCacheInvalidation() throws Exception {
- makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
// Get the loadedRoles cache via reflection
Cache<Long, Boolean> loadedRoles = getLoadedRolesCache(jcasbinAuthorizer);
@@ -437,7 +421,6 @@ public class TestJcasbinAuthorizer {
@Test
public void testRoleCacheSynchronousRemovalListenerDeletesPolicy() throws
Exception {
- makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
// Get the enforcers via reflection
Enforcer allowEnforcer = getAllowEnforcer(jcasbinAuthorizer);
@@ -462,7 +445,6 @@ public class TestJcasbinAuthorizer {
assertTrue(denyEnforcer.hasPolicy(roleIdStr, "CATALOG", "999",
"USE_CATALOG", "allow"));
// Invalidate the cache entry - this triggers the synchronous removal
listener
- // (using executor(Runnable::run) to ensure synchronous execution)
loadedRoles.invalidate(testRoleId);
// Verify the role's policies have been deleted from enforcers
(synchronous, no need to wait)
@@ -483,7 +465,6 @@ public class TestJcasbinAuthorizer {
/** Tests {@link JcasbinAuthorizer#hasMetadataPrivilegePermission} hierarchy
walk */
@Test
public void testHasMetadataPrivilegePermission() throws Exception {
- makeCompletableFutureUseCurrentThread(jcasbinAuthorizer);
NameIdentifier userNameIdentifier = NameIdentifierUtil.ofUser(METALAKE,
USERNAME);
// --- Case 1: no MANAGE_GRANTS anywhere → false ---
