yuqi1129 commented on code in PR #10908:
URL: https://github.com/apache/gravitino/pull/10908#discussion_r3166314383
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java:
##########
@@ -103,6 +113,14 @@ public void setOriginalAuthorizationExpression(String
originalAuthorizationExpre
this.originalAuthorizationExpression = originalAuthorizationExpression;
}
+ public Set<Long> getUserRoleIds() {
+ return userRoleIds;
+ }
+
+ public void setUserRoleIds(@Nullable Set<Long> userRoleIds) {
+ this.userRoleIds = userRoleIds == null ? Collections.emptySet() :
userRoleIds;
+ }
Review Comment:
Addressed. setUserRoleIds now defensively copies the provided set and stores
an unmodifiable set, so caller-side mutation cannot affect the request context
and downstream code cannot mutate the returned set. Added a unit test for both
behaviors.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -482,36 +528,37 @@ private void loadRolePrivilege(
SupportsRelationOperations.Type.ROLE_USER_REL,
userNameIdentifier,
Entity.EntityType.USER);
+ Set<Long> roleIds = new HashSet<>(entities.size());
List<CompletableFuture<Void>> loadRoleFutures = new ArrayList<>();
for (RoleEntity role : entities) {
Long roleId = role.id();
+ roleIds.add(roleId);
allowEnforcer.addRoleForUser(String.valueOf(userId),
String.valueOf(roleId));
denyEnforcer.addRoleForUser(String.valueOf(userId),
String.valueOf(roleId));
- if (loadedRoles.getIfPresent(roleId) != null) {
- continue;
- }
CompletableFuture<Void> loadRoleFuture =
- CompletableFuture.supplyAsync(
- () -> {
- try {
- return entityStore.get(
- NameIdentifierUtil.ofRole(metalake,
role.name()),
- Entity.EntityType.ROLE,
- RoleEntity.class);
- } catch (Exception e) {
- throw new RuntimeException("Failed to load role:
" + role.name(), e);
- }
- },
- executor)
- .thenAcceptAsync(
- roleEntity -> {
- loadPolicyByRoleEntity(roleEntity);
- loadedRoles.put(roleId, true);
- },
- executor);
+ CompletableFuture.runAsync(
+ () -> {
+ loadedRoles.get(
+ roleId,
+ unused -> {
+ try {
+ RoleEntity roleEntity =
+ entityStore.get(
+ NameIdentifierUtil.ofRole(metalake,
role.name()),
+ Entity.EntityType.ROLE,
+ RoleEntity.class);
+ return loadPolicyByRoleEntity(roleEntity);
+ } catch (Exception e) {
+ throw new RuntimeException(
+ "Failed to load role: " + role.name(), e);
+ }
+ });
+ },
+ executor);
loadRoleFutures.add(loadRoleFuture);
}
Review Comment:
Addressed. I added a loadedRoles.getIfPresent(roleId) fast path before
creating the async load task, so cached roles no longer pay the
CompletableFuture scheduling cost on every request. I also added
testCachedRolePolicyIndexSkipsRoleEntityLoad to verify cached roles do not
reload RoleEntity.
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java:
##########
@@ -103,6 +113,14 @@ public void setOriginalAuthorizationExpression(String
originalAuthorizationExpre
this.originalAuthorizationExpression = originalAuthorizationExpression;
}
+ public Set<Long> getUserRoleIds() {
+ return userRoleIds;
+ }
+
Review Comment:
Addressed. I added JavaDoc for getUserRoleIds and setUserRoleIds describing
the request-scoped lifecycle, immutable returned set, and null handling.
--
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]
