Copilot commented on code in PR #10996:
URL: https://github.com/apache/gravitino/pull/10996#discussion_r3213226241
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -490,86 +586,239 @@ private boolean authorizeByJcasbin(
}
}
- private static UserEntity getUserEntity(String username, String metalake)
throws IOException {
+ //
---------------------------------------------------------------------------
+ // Metadata ID / user info / owner resolution — two-tier (request +
Caffeine)
+ //
---------------------------------------------------------------------------
+
+ /**
+ * Two-tier lookup: the per-request map in {@code requestContext} dedups
calls within the same
+ * HTTP request; on a miss, we consult the long-lived Caffeine cache, and
finally fall back to a
+ * DB query via {@link MetadataIdConverter}.
+ */
+ private Long resolveMetadataId(
+ MetadataObject metadataObject, String metalake,
AuthorizationRequestContext requestContext) {
+ String cacheKey = buildCacheKey(metalake, metadataObject);
+ return requestContext.computeMetadataIdIfAbsent(
+ cacheKey,
+ k -> {
+ Optional<Long> cached = metadataIdCache.getIfPresent(k);
+ if (cached.isPresent()) {
+ return cached.get();
+ }
+ Long id = MetadataIdConverter.getID(metadataObject, metalake);
+ metadataIdCache.put(k, id);
+ return id;
+ });
+ }
+
+ /**
+ * Per-request {@link UserUpdatedAt} lookup. The underlying {@code
user_meta} query is issued at
+ * most once per (metalake, username) within a single request.
+ */
+ private Optional<UserUpdatedAt> loadUserInfo(
+ String metalake, String username, AuthorizationRequestContext
requestContext) {
+ String cacheKey = metalake + KEY_SEP + username;
+ return requestContext.computeUserInfoIfAbsent(
+ cacheKey,
+ k ->
+ Optional.ofNullable(
+ SessionUtils.getWithoutCommit(
+ UserMetaMapper.class, m -> m.getUserUpdatedAt(metalake,
username))));
+ }
+
+ /**
+ * Two-tier owner lookup: request-level dedup first, then the shared {@code
ownerRelCache}, and
+ * finally a single {@code owner_meta} query. A successful DB fetch
populates both tiers so
+ * subsequent {@code isOwner} calls — in this request and later ones — hit
the cache.
+ */
+ private Optional<Long> resolveOwnerId(
+ Long metadataId,
+ MetadataObject.Type metadataType,
+ AuthorizationRequestContext requestContext) {
+ return requestContext.computeOwnerIfAbsent(
+ metadataId,
+ id -> {
+ Optional<Optional<Long>> cached = ownerRelCache.getIfPresent(id);
+ if (cached.isPresent()) {
+ return cached.get();
+ }
+ OwnerInfo ownerInfo =
+ SessionUtils.getWithoutCommit(
+ OwnerMetaMapper.class,
+ m -> m.selectOwnerByMetadataObjectIdAndType(id,
metadataType.name()));
+ Optional<Long> owner =
+ ownerInfo == null ? Optional.empty() :
Optional.of(ownerInfo.getOwnerId());
+ ownerRelCache.put(id, owner);
+ return owner;
+ });
+ }
+
+ /**
+ * Returns true if the owner ID equals the user's ID, or matches any of the
user's group IDs.
+ * Entity IDs are unique across users and groups, so a single {@code Long}
comparison is
+ * unambiguous. Group IDs are resolved from the principal via {@link
+ * #resolveCurrentUserGroups(String, EntityStore)}.
+ */
+ private boolean ownerMatchesUserOrGroups(Optional<Long> owner, long userId,
String metalake) {
+ if (!owner.isPresent()) {
+ return false;
+ }
+ long ownerId = owner.get();
+ if (ownerId == userId) {
+ return true;
+ }
EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- UserEntity userEntity =
- entityStore.get(
- NameIdentifierUtil.ofUser(metalake, username),
- Entity.EntityType.USER,
- UserEntity.class);
- return userEntity;
+ for (GroupEntity groupEntity : resolveCurrentUserGroups(metalake,
entityStore)) {
+ if (Objects.equals(groupEntity.id(), ownerId)) {
+ return true;
Review Comment:
This Javadoc/logic assumes “Entity IDs are unique across users and groups”,
but the DB schema/models keep `owner_type` precisely to disambiguate. Relying
on ID-only comparisons can lead to false positives in ownership checks. Use the
cached/queried `ownerType` to decide whether to compare against `userId` or the
user’s group IDs.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -490,86 +586,239 @@ private boolean authorizeByJcasbin(
}
}
- private static UserEntity getUserEntity(String username, String metalake)
throws IOException {
+ //
---------------------------------------------------------------------------
+ // Metadata ID / user info / owner resolution — two-tier (request +
Caffeine)
+ //
---------------------------------------------------------------------------
+
+ /**
+ * Two-tier lookup: the per-request map in {@code requestContext} dedups
calls within the same
+ * HTTP request; on a miss, we consult the long-lived Caffeine cache, and
finally fall back to a
+ * DB query via {@link MetadataIdConverter}.
+ */
+ private Long resolveMetadataId(
+ MetadataObject metadataObject, String metalake,
AuthorizationRequestContext requestContext) {
+ String cacheKey = buildCacheKey(metalake, metadataObject);
+ return requestContext.computeMetadataIdIfAbsent(
+ cacheKey,
+ k -> {
+ Optional<Long> cached = metadataIdCache.getIfPresent(k);
+ if (cached.isPresent()) {
+ return cached.get();
+ }
+ Long id = MetadataIdConverter.getID(metadataObject, metalake);
+ metadataIdCache.put(k, id);
+ return id;
+ });
+ }
+
+ /**
+ * Per-request {@link UserUpdatedAt} lookup. The underlying {@code
user_meta} query is issued at
+ * most once per (metalake, username) within a single request.
+ */
+ private Optional<UserUpdatedAt> loadUserInfo(
+ String metalake, String username, AuthorizationRequestContext
requestContext) {
+ String cacheKey = metalake + KEY_SEP + username;
+ return requestContext.computeUserInfoIfAbsent(
+ cacheKey,
+ k ->
+ Optional.ofNullable(
+ SessionUtils.getWithoutCommit(
+ UserMetaMapper.class, m -> m.getUserUpdatedAt(metalake,
username))));
+ }
+
+ /**
+ * Two-tier owner lookup: request-level dedup first, then the shared {@code
ownerRelCache}, and
+ * finally a single {@code owner_meta} query. A successful DB fetch
populates both tiers so
+ * subsequent {@code isOwner} calls — in this request and later ones — hit
the cache.
+ */
+ private Optional<Long> resolveOwnerId(
+ Long metadataId,
+ MetadataObject.Type metadataType,
+ AuthorizationRequestContext requestContext) {
+ return requestContext.computeOwnerIfAbsent(
+ metadataId,
+ id -> {
+ Optional<Optional<Long>> cached = ownerRelCache.getIfPresent(id);
+ if (cached.isPresent()) {
+ return cached.get();
+ }
+ OwnerInfo ownerInfo =
+ SessionUtils.getWithoutCommit(
+ OwnerMetaMapper.class,
+ m -> m.selectOwnerByMetadataObjectIdAndType(id,
metadataType.name()));
+ Optional<Long> owner =
+ ownerInfo == null ? Optional.empty() :
Optional.of(ownerInfo.getOwnerId());
+ ownerRelCache.put(id, owner);
+ return owner;
Review Comment:
`resolveOwnerId` drops `owner_type` from `owner_meta` and caches only
`ownerId`. Because `owner_meta` explicitly stores both ID and type, treating
the owner as a bare `Long` can misclassify ownership if a USER id and GROUP id
ever overlap, potentially granting OWNER privileges incorrectly. Cache a
composite (e.g., ownerType + ownerId) or keep `OwnerInfo` in the cache and
enforce based on `ownerType`.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -285,7 +347,7 @@ public boolean isSelf(Entity.EntityType type,
NameIdentifier nameIdentifier) {
entityStore
.relationOperations()
.listEntitiesByRelation(
- SupportsRelationOperations.Type.ROLE_USER_REL,
+
org.apache.gravitino.SupportsRelationOperations.Type.ROLE_USER_REL,
userNameIdentifier,
Review Comment:
Avoid using a fully-qualified class name in method bodies here; it makes the
code noisier and conflicts with the project’s import hygiene. Add an import for
`org.apache.gravitino.SupportsRelationOperations` and use
`SupportsRelationOperations.Type.ROLE_USER_REL` instead.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -587,178 +836,283 @@ private List<GroupEntity>
resolveCurrentUserGroups(String metalake, EntityStore
return entityStore.batchGet(groupIdents, Entity.EntityType.GROUP,
GroupEntity.class);
}
- /**
- * Adds a role mapping for the given user in both enforcers and
asynchronously loads the role's
- * policies if they are not already cached. When a role needs loading, the
resulting {@link
- * CompletableFuture} is appended to {@code loadRoleFutures} so the caller
can join all futures
- * after processing both direct and group-inherited roles.
- */
- private void addRoleForUserAndLoadPolicies(
- Long userId,
- String metalake,
- Long roleId,
- String roleName,
- List<CompletableFuture<Void>> loadRoleFutures,
- EntityStore entityStore) {
- allowEnforcer.addRoleForUser(String.valueOf(userId),
String.valueOf(roleId));
- denyEnforcer.addRoleForUser(String.valueOf(userId),
String.valueOf(roleId));
- if (loadedRoles.getIfPresent(roleId) != null) {
- return;
- }
- CompletableFuture<Void> loadRoleFuture =
- CompletableFuture.supplyAsync(
- () -> {
- try {
- return entityStore.get(
- NameIdentifierUtil.ofRole(metalake, roleName),
- Entity.EntityType.ROLE,
- RoleEntity.class);
- } catch (Exception e) {
- throw new RuntimeException("Failed to load role: " +
roleName, e);
- }
- },
- executor)
- .thenAcceptAsync(
- roleEntity -> {
- loadPolicyByRoleEntity(roleEntity);
- loadedRoles.put(roleId, true);
- },
- executor);
- loadRoleFutures.add(loadRoleFuture);
- }
-
- private void loadOwnerPolicy(String metalake, MetadataObject metadataObject,
Long metadataId) {
- if (ownerRel.getIfPresent(metadataId) != null) {
- LOG.debug("Metadata {} OWNER has been loaded.", metadataId);
- return;
- }
- try {
- NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
- EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
- List<? extends Entity> owners =
- entityStore
- .relationOperations()
- .listEntitiesByRelation(
- SupportsRelationOperations.Type.OWNER_REL,
- entityIdent,
- Entity.EntityType.valueOf(metadataObject.type().name()));
- if (owners.isEmpty()) {
- ownerRel.put(metadataId, Optional.empty());
- } else {
- for (Entity ownerEntity : owners) {
- if (ownerEntity instanceof UserEntity) {
- UserEntity user = (UserEntity) ownerEntity;
- ownerRel.put(
- metadataId,
- Optional.of(new OwnerInfo(user.id(), Entity.EntityType.USER,
user.name())));
- } else if (ownerEntity instanceof GroupEntity) {
- GroupEntity group = (GroupEntity) ownerEntity;
- ownerRel.put(
- metadataId,
- Optional.of(new OwnerInfo(group.id(), Entity.EntityType.GROUP,
group.name())));
- }
- }
+ private void versionCheckAndLoadRoles(
+ String metalake, List<Long> roleIds, AuthorizationRequestContext
requestContext) {
+ // Step 3: batch fetch (roleId, roleName, updated_at) for all role IDs — 1
query
+ List<Long> uniqueRoleIds =
roleIds.stream().distinct().collect(Collectors.toList());
+ List<RoleUpdatedAt> roleVersions =
+ SessionUtils.getWithoutCommit(
+ RoleMetaMapper.class, m -> m.batchGetRoleUpdatedAt(uniqueRoleIds));
+
+ for (RoleUpdatedAt rv : roleVersions) {
+ long roleId = rv.getRoleId();
+ long dbUpdatedAt = rv.getUpdatedAt();
+ Optional<Long> cachedUpdatedAt = loadedRoles.getIfPresent(roleId);
+
+ if (cachedUpdatedAt.isPresent() && cachedUpdatedAt.get() >= dbUpdatedAt)
{
+ // Role policies are still current
+ continue;
}
- } catch (IOException e) {
- LOG.warn("Can not load metadata owner", e);
+
+ // Stale or missing — evict old policies and reload
+ if (cachedUpdatedAt.isPresent()) {
+ allowEnforcer.deleteRole(String.valueOf(roleId));
+ denyEnforcer.deleteRole(String.valueOf(roleId));
+ }
+
+ // Load full role entity using roleName from the batch query (no extra
DB scan)
+ try {
+ EntityStore entityStore = GravitinoEnv.getInstance().entityStore();
+ RoleEntity roleEntity =
+ entityStore.get(
+ NameIdentifierUtil.ofRole(metalake, rv.getRoleName()),
+ Entity.EntityType.ROLE,
+ RoleEntity.class);
+ loadPolicyByRoleEntity(roleEntity, requestContext);
+ } catch (Exception e) {
+ LOG.warn("Failed to load role policies for roleId {}", roleId, e);
+ continue;
+ }
+
+ loadedRoles.put(roleId, dbUpdatedAt);
}
}
- private void loadPolicyByRoleEntity(RoleEntity roleEntity) {
+ private void bindUserRoles(long userId, List<Long> roleIds) {
+ for (Long roleId : roleIds) {
+ allowEnforcer.addRoleForUser(String.valueOf(userId),
String.valueOf(roleId));
+ denyEnforcer.addRoleForUser(String.valueOf(userId),
String.valueOf(roleId));
+ }
+ }
+
+ //
---------------------------------------------------------------------------
+ // Policy loading from role entity
+ //
---------------------------------------------------------------------------
+
+ private void loadPolicyByRoleEntity(
+ RoleEntity roleEntity, AuthorizationRequestContext requestContext) {
String metalake =
NameIdentifierUtil.getMetalake(roleEntity.nameIdentifier());
List<SecurableObject> securableObjects = roleEntity.securableObjects();
for (SecurableObject securableObject : securableObjects) {
+ Long securableId = resolveMetadataId(securableObject, metalake,
requestContext);
for (Privilege privilege : securableObject.privileges()) {
Privilege.Condition condition = privilege.condition();
if (AuthConstants.DENY.equalsIgnoreCase(condition.name())) {
denyEnforcer.addPolicy(
String.valueOf(roleEntity.id()),
securableObject.type().name(),
- String.valueOf(MetadataIdConverter.getID(securableObject,
metalake)),
+ String.valueOf(securableId),
AuthorizationUtils.replaceLegacyPrivilegeName(privilege.name())
.name()
- .toUpperCase(java.util.Locale.ROOT),
+ .toUpperCase(Locale.ROOT),
AuthConstants.ALLOW);
}
- // Since different roles of a user may simultaneously hold both
"allow" and "deny"
- // permissions
- // for the same privilege on a given MetadataObject, the allowEnforcer
must also incorporate
- // the "deny" privilege to ensure that the authorize method correctly
returns false in such
- // cases. For example, if role1 has an "allow" privilege for
SELECT_TABLE on table1, while
- // role2 has a "deny" privilege for the same action on table1, then a
user assigned both
- // roles should receive a false result when calling the authorize
method.
allowEnforcer.addPolicy(
String.valueOf(roleEntity.id()),
securableObject.type().name(),
- String.valueOf(MetadataIdConverter.getID(securableObject,
metalake)),
+ String.valueOf(securableId),
AuthorizationUtils.replaceLegacyPrivilegeName(privilege.name())
.name()
- .toUpperCase(java.util.Locale.ROOT),
- condition.name().toLowerCase(java.util.Locale.ROOT));
+ .toUpperCase(Locale.ROOT),
+ condition.name().toLowerCase(Locale.ROOT));
}
}
}
- /**
- * Checks whether the given principal is the owner of the metadata object
identified by
- * metadataId. Supports both user and group ownership.
- */
- private boolean checkOwnership(Principal principal, String metalake, Long
metadataId) {
- Optional<OwnerInfo> ownerOpt = ownerRel.getIfPresent(metadataId);
- if (ownerOpt == null || !ownerOpt.isPresent()) {
- return false;
+ //
---------------------------------------------------------------------------
+ // Change poller (eventual consistency for HA)
+ //
---------------------------------------------------------------------------
+
+ @VisibleForTesting
+ void pollChanges() {
+ try {
+ LOG.debug("Polling for owner changes after id {}", ownerPollHighWaterId);
+ pollOwnerChanges();
+ } catch (Exception e) {
+ LOG.warn("Owner change poll failed", e);
}
- OwnerInfo owner = ownerOpt.get();
- // We compare by entity ID rather than name to guard against stale cache
entries.
- // If a user/group is deleted and recreated with the same name, the cached
OwnerInfo
- // still holds the old ID. A name-only comparison would incorrectly grant
ownership
- // to the new entity. The extra IO to fetch the current entity ensures
correctness.
- if (owner.type == Entity.EntityType.USER) {
- try {
- UserEntity userEntity = getUserEntity(principal.getName(), metalake);
- return Objects.equals(userEntity.id(), owner.id);
- } catch (Exception e) {
- LOG.debug("Can not get user entity for ownership check", e);
- return false;
+
+ try {
+ LOG.debug("Polling for entity changes after id {}",
entityPollHighWaterId);
+ pollEntityChanges();
+ } catch (Exception e) {
+ LOG.warn("Entity change poll failed", e);
+ }
+ }
+
+ private void pollOwnerChanges() {
+ List<ChangedOwnerInfo> changes =
+ SessionUtils.getWithoutCommit(
+ OwnerMetaMapper.class, m ->
m.selectChangedOwners(ownerPollHighWaterId));
+
+ long maxSeenId = ownerPollHighWaterId;
+ for (ChangedOwnerInfo change : changes) {
+ ownerRelCache.invalidate(change.getMetadataObjectId());
+ if (change.getId() > maxSeenId) {
+ maxSeenId = change.getId();
}
- } else if (owner.type == Entity.EntityType.GROUP) {
- if (principal instanceof UserPrincipal) {
- List<UserGroup> groups = ((UserPrincipal) principal).getGroups();
- if (groups.isEmpty()) {
- return false;
- }
- try {
- List<NameIdentifier> groupIdents =
- groups.stream()
- .map(g -> NameIdentifierUtil.ofGroup(metalake,
g.getGroupname()))
- .collect(Collectors.toList());
- List<GroupEntity> groupEntities =
- GravitinoEnv.getInstance()
- .entityStore()
- .batchGet(groupIdents, Entity.EntityType.GROUP,
GroupEntity.class);
- return groupEntities.stream().anyMatch(ge -> Objects.equals(ge.id(),
owner.id));
- } catch (Exception e) {
- LOG.debug("Can not get group entities for ownership check", e);
- return false;
+ }
+ ownerPollHighWaterId = maxSeenId;
+ }
+
+ private void pollEntityChanges() {
+ List<EntityChangeRecord> changes =
+ SessionUtils.getWithoutCommit(
+ EntityChangeLogMapper.class,
+ m -> m.selectEntityChanges(entityPollHighWaterId,
POLLER_MAX_ROWS));
+
+ long maxSeenId = entityPollHighWaterId;
+ for (EntityChangeRecord change : changes) {
+ String metalake = change.getMetalakeName();
+ String entityType = change.getEntityType();
+ String fullName = change.getFullName();
+
+ MetadataObject.Type mdType;
+ try {
+ mdType =
MetadataObject.Type.valueOf(entityType.toUpperCase(Locale.ROOT));
+ } catch (IllegalArgumentException e) {
+ LOG.warn("Unknown entity type in change log: {}", entityType);
+ if (change.getId() > maxSeenId) {
+ maxSeenId = change.getId();
}
+ continue;
+ }
+
+ MetadataObject mdObj = metadataObjectFromChangeLog(metalake, fullName,
mdType);
+ String cacheKey = buildCacheKey(metalake, mdObj);
+
+ if (isNonLeaf(mdType)) {
+ metadataIdCache.invalidateByPrefix(cacheKey);
+ } else {
+ metadataIdCache.invalidate(cacheKey);
+ }
+
+ if (change.getId() > maxSeenId) {
+ maxSeenId = change.getId();
}
- return false;
}
- return false;
+ entityPollHighWaterId = maxSeenId;
+ }
+
+ //
---------------------------------------------------------------------------
+ // Helpers
+ //
---------------------------------------------------------------------------
+
+ /**
+ * Builds a hierarchical cache key for the metadataIdCache. Non-leaf objects
end with "::" to
+ * enable prefix-based cascade invalidation.
+ *
+ * <p>Examples: metalake::catalog:: , metalake::catalog::schema:: ,
+ * metalake::catalog::schema::table::TABLE
+ */
+ @VisibleForTesting
+ static String buildCacheKey(String metalake, MetadataObject metadataObject) {
+ StringBuilder sb = new StringBuilder(metalake);
+ sb.append(KEY_SEP);
+ // fullName uses '.' as separator, e.g. "catalog1.schema1.table1"
+ String[] parts = metadataObject.fullName().split("\\.");
+ sb.append(String.join(KEY_SEP, parts));
+ if (isNonLeaf(metadataObject.type())) {
+ // Trailing separator enables prefix-based cascade invalidation
+ sb.append(KEY_SEP);
Review Comment:
`buildCacheKey` currently makes the METALAKE key `metalake::metalake::`
(because `metadataObject.fullName()` for a METALAKE is the metalake name). That
breaks cascade invalidation for metalake-level changes: child keys like
`metalake::catalog::` do not start with `metalake::metalake::`, so
`invalidateByPrefix(cacheKey)` on METALAKE change-log records won’t evict any
descendant entries. Consider special-casing METALAKE so its prefix key is just
`metalake::`, ensuring it prefixes all keys in that metalake.
##########
core/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/base/TestAuthMappers.java:
##########
@@ -351,14 +351,14 @@ void testOwnerMetaSelectChangedOwners() {
throw new RuntimeException("Update failed", e);
}
- List<ChangedOwnerInfo> changed = ownerMetaMapper.selectChangedOwners(50L);
+ List<ChangedOwnerInfo> changed = ownerMetaMapper.selectChangedOwners(0L);
Assertions.assertEquals(1, changed.size());
Assertions.assertEquals(200L, changed.get(0).getMetadataObjectId());
Assertions.assertEquals("SCHEMA", changed.get(0).getMetadataObjectType());
Assertions.assertEquals(100L, changed.get(0).getUpdatedAt());
// With the same timestamp, the row is returned again for timestamp-only
polling.
- List<ChangedOwnerInfo> sameTimestamp =
ownerMetaMapper.selectChangedOwners(100L);
+ List<ChangedOwnerInfo> sameTimestamp =
ownerMetaMapper.selectChangedOwners(0L);
Assertions.assertEquals(1, sameTimestamp.size());
Review Comment:
The test comment and assertion no longer match the new id-based cursor
semantics. Calling `selectChangedOwners(0L)` twice will always return the same
row; to validate monotonic id polling, the second call should pass the last
seen `ChangedOwnerInfo.id` and assert the result is empty (and update the
comment accordingly).
--
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]
