Copilot commented on code in PR #10908:
URL: https://github.com/apache/gravitino/pull/10908#discussion_r3166109858
##########
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:
`loadRolePrivilege` now schedules a `CompletableFuture.runAsync` task for
every role on every request, even when the role is already present in
`loadedRoles`. This adds thread-pool scheduling/coordination overhead
proportional to roles-per-user and can partially negate the intended hot-path
optimization. Consider checking `loadedRoles.getIfPresent(roleId)` first (which
still counts as an access for `expireAfterAccess`) and only creating async load
futures for cache misses; alternatively batch-load misses and avoid creating
futures for cached roles.
##########
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:
The new `getUserRoleIds` / `setUserRoleIds` methods are public but lack
JavaDoc, while the rest of this class’s public API is documented. Please add
JavaDoc describing the expected lifecycle (request-scoped),
thread-safety/visibility expectations, and whether the returned set is safe to
iterate and/or modify.
```suggestion
/**
* Returns the user role IDs associated with the current authorization
request.
*
* <p>This context is request-scoped and is expected to be used only for
the lifetime of a
* single authorization request.
*
* <p>The returned set is the live backing set stored in this context, not
a defensive copy or
* unmodifiable view. Iteration is safe only when the set is not being
modified concurrently.
* Changes made through the returned set are reflected in this context.
*
* <p>This class is not intended for concurrent mutation across threads.
Callers should not rely
* on additional thread-safety or cross-thread visibility guarantees
beyond normal request-scoped
* usage.
*
* @return the live set of user role IDs for the current request, or an
empty set if none have
* been set
*/
public Set<Long> getUserRoleIds() {
return userRoleIds;
}
/**
* Sets the user role IDs associated with the current authorization
request.
*
* <p>This context is request-scoped and is expected to be used only for
the lifetime of a
* single authorization request.
*
* <p>The provided set reference is stored directly. The caller must
ensure the set is not
* modified concurrently while being read or iterated through this
context. Subsequent
* modifications to the provided set are visible through {@link
#getUserRoleIds()}.
*
* <p>This class is not intended for concurrent mutation across threads.
Callers should not rely
* on additional thread-safety or cross-thread visibility guarantees
beyond normal request-scoped
* usage.
*
* @param userRoleIds the user role IDs for the current request; if {@code
null}, an empty set is
* stored
*/
```
##########
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:
`setUserRoleIds` stores the provided `Set` reference directly. Because this
is request context state that may be read concurrently during authorization,
storing an externally mutable set can lead to hard-to-debug races if the caller
mutates it after setting. Consider defensively copying (e.g., to an
immutable/unmodifiable set) before assigning, and optionally returning an
unmodifiable view from `getUserRoleIds` to prevent accidental mutation by
downstream code.
--
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]
