[#10772] feat(authorization): refactor JcasbinAuthorizer with version-validated cache#10996
Open
yuqi1129 wants to merge 2 commits into
Open
[#10772] feat(authorization): refactor JcasbinAuthorizer with version-validated cache#10996yuqi1129 wants to merge 2 commits into
yuqi1129 wants to merge 2 commits into
Conversation
Code Coverage Report
Files
|
yuqi1129
force-pushed
the
feat/jcasbin-cache-refactor
branch
from
bd8bc8c to
a6a9bfc
Compare
jerryshao
pushed a commit
that referenced
this pull request
May 9, 2026
### What changes were proposed in this pull request? This PR splits the group-related authorization cache mapper work from #10996. It adds: - `group_meta.updated_at` to H2/MySQL/PostgreSQL 1.3.0 schemas and 1.2.0-to-1.3.0 upgrade scripts. - Group updated-at mapper APIs and SQL providers. - `GroupUpdatedAt` PO. - `GroupMetaService.updateGroup` updates `group_meta.updated_at` when group role assignments change. - Mapper and service tests for group updated-at behavior. ### Why are the changes needed? Group role cache validation needs a DB-side version sentinel, matching the existing user/role cache pattern. Fix: #10770 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? ```bash ./gradlew :core:spotlessApply ./gradlew :core:test --tests org.apache.gravitino.storage.relational.mapper.provider.base.TestAuthMappers --tests org.apache.gravitino.storage.relational.service.TestGroupMetaService ```
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the server authorization path to introduce request-scoped deduplication (AuthorizationRequestContext) and a new cache abstraction (GravitinoCache) while reworking JcasbinAuthorizer to use version-validated caches plus an HA-oriented change poller for targeted invalidation.
Changes:
- Thread
AuthorizationRequestContextthrough request interception + authorization executors to enable per-request dedup and shared logging context. - Rewrite
JcasbinAuthorizercaching: version-validated user/group/role caches, metadata-id + owner caches, and a scheduled poller draining change logs for invalidation. - Add new cache abstraction + implementations/tests, and migrate change-log/owner polling queries to id-based cursors with supporting mapper + test updates.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/test/java/org/apache/gravitino/server/web/filter/TestGravitinoInterceptionService.java | Updates mocks for new checkCurrentUser signature and authorizer interface changes. |
| server/src/main/java/org/apache/gravitino/server/web/filter/GravitinoInterceptionService.java | Creates and threads a per-request AuthorizationRequestContext through user validation and authorization execution. |
| server/src/main/java/org/apache/gravitino/server/web/filter/authorization/RunJobAuthorizationExecutor.java | Updates executor interface to accept a request context and stores original expression into it. |
| server/src/main/java/org/apache/gravitino/server/web/filter/authorization/CommonAuthorizerExecutor.java | Passes a shared request context into expression evaluation (instead of allocating a new one). |
| server/src/main/java/org/apache/gravitino/server/web/filter/authorization/AuthorizationExecutor.java | Changes executor contract to require AuthorizationRequestContext. |
| server/src/main/java/org/apache/gravitino/server/web/filter/authorization/AssociateTagAuthorizationExecutor.java | Adapts to the new executor signature and reuses caller-provided request context. |
| server/src/main/java/org/apache/gravitino/server/web/filter/authorization/AssociatePolicyAuthorizationExecutor.java | Adapts to the new executor signature and reuses caller-provided request context. |
| server-common/src/test/java/org/apache/gravitino/server/authorization/TestPassThroughAuthorizer.java | Updates tests for new isMetalakeUser(..., AuthorizationRequestContext) signature. |
| server-common/src/test/java/org/apache/gravitino/server/authorization/MockGravitinoAuthorizer.java | Updates mock authorizer to implement new isMetalakeUser signature. |
| server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizerCacheHelpers.java | Adds unit tests for cache-key building helpers and cached role snapshot DTOs. |
| server-common/src/test/java/org/apache/gravitino/server/authorization/jcasbin/TestJcasbinAuthorizer.java | Refactors tests to align with new mapper-based/version-validated cache behavior. |
| server-common/src/main/java/org/apache/gravitino/server/authorization/PassThroughAuthorizer.java | Updates authorizer interface implementation to accept request context. |
| server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java | Major cache refactor: version-validated role caches, metadata-id cache, owner cache, and change poller-driven invalidation. |
| server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/CachedUserRoles.java | Adds cached snapshot class for user direct role IDs keyed by user_meta.updated_at. |
| server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/CachedGroupRoles.java | Adds cached snapshot class for group role IDs keyed by group_meta.updated_at. |
| server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConverter.java | Updates OGNL expression generation to pass authorizationContext into isMetalakeUser. |
| server-common/src/main/java/org/apache/gravitino/server/authorization/expression/AuthorizationExpressionConstants.java | Updates Javadoc reference for new isMetalakeUser signature. |
| server-common/build.gradle.kts | Adds Lombok processor/compileOnly deps for server-common sources/tests. |
| core/src/test/java/org/apache/gravitino/storage/relational/service/TestTableMetaService.java | Migrates entity change log polling tests from timestamp cursor to id cursor. |
| core/src/test/java/org/apache/gravitino/storage/relational/service/TestEntityChangeLogService.java | Migrates change-log service tests from timestamp cursor to id cursor. |
| core/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/base/TestEntityChangeLogMapper.java | Updates mapper test for new id-based selectEntityChanges signature. |
| core/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/base/TestAuthMappers.java | Updates owner-change polling test for new id-based cursor behavior. |
| core/src/test/java/org/apache/gravitino/cache/TestGravitinoCache.java | Adds unit tests for GravitinoCache implementations (TTL/eviction/prefix invalidation). |
| core/src/test/java/org/apache/gravitino/authorization/TestAuthorizationRequestContext.java | Adds tests for request-scoped dedup helpers and absent-result caching. |
| core/src/main/java/org/apache/gravitino/storage/relational/po/auth/ChangedOwnerInfo.java | Adds id field to support id-based owner change polling cursor. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/OwnerMetaBaseSQLProvider.java | Switches owner-change polling query to id-based cursor and adds selectLatestChangeId. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/EntityChangeLogBaseSQLProvider.java | Switches change-log polling query to id-based cursor and adds selectLatestChangeId. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/OwnerMetaSQLProviderFactory.java | Exposes updated SQL provider methods for owner change polling and latest id. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/OwnerMetaMapper.java | Updates mapper API for id-based owner polling and latest id. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/EntityChangeLogSQLProviderFactory.java | Exposes updated SQL provider methods for change-log polling and latest id. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/EntityChangeLogMapper.java | Updates mapper API for id-based polling and latest id. |
| core/src/main/java/org/apache/gravitino/Configs.java | Adds configuration for metadataId cache size and change poll interval. |
| core/src/main/java/org/apache/gravitino/cache/NoOpsGravitinoCache.java | Adds no-op cache implementation for disabling caching. |
| core/src/main/java/org/apache/gravitino/cache/GravitinoCache.java | Introduces cache abstraction used by authorization subsystem. |
| core/src/main/java/org/apache/gravitino/cache/CaffeineGravitinoCache.java | Adds Caffeine-backed GravitinoCache implementation. |
| core/src/main/java/org/apache/gravitino/authorization/GravitinoAuthorizer.java | Updates authorizer interface: isMetalakeUser now takes request context; adds structural-change hook. |
| core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java | Threads request context into checkCurrentUser and keeps backward-compatible overload. |
| core/src/main/java/org/apache/gravitino/authorization/AuthorizationRequestContext.java | Adds per-request caches for user/group/metadataId/owner and makes AuthorizationKey immutable via Lombok. |
Comment on lines
+656
to
+673
| /** | ||
| * 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; |
Comment on lines
347
to
351
| entityStore | ||
| .relationOperations() | ||
| .listEntitiesByRelation( | ||
| SupportsRelationOperations.Type.ROLE_USER_REL, | ||
| org.apache.gravitino.SupportsRelationOperations.Type.ROLE_USER_REL, | ||
| userNameIdentifier, |
Comment on lines
360
to
362
| // 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()); |
Comment on lines
+1011
to
+1019
| 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); |
Contributor
|
Is it possible to split this PR, it is too big to review. |
jerryshao
reviewed
May 11, 2026
| + " FROM " | ||
| + ENTITY_CHANGE_LOG_TABLE_NAME | ||
| + " WHERE created_at >= #{createdAtFrom} ORDER BY created_at, id LIMIT #{maxRows}"; | ||
| + " WHERE id > #{lastId} ORDER BY id LIMIT #{maxRows}"; |
Contributor
There was a problem hiding this comment.
What is the reason to change this?
This was referenced May 11, 2026
Contributor
Author
|
Let's merge #11033 first, as this one is quite large to review |
jerryshao
pushed a commit
that referenced
this pull request
May 14, 2026
### What changes were proposed in this pull request? This PR extracts the infrastructure part from #10996 to support the follow-up JcasbinAuthorizer cache refactor. It includes: - Add a generic `GravitinoCache` abstraction with Caffeine and no-op implementations. - Add per-request lookup caches in `AuthorizationRequestContext` for user, group, metadata id, and owner lookup deduplication. - Thread `AuthorizationRequestContext` through authorization filter/executor paths. - Update `isMetalakeUser` to accept `AuthorizationRequestContext`. - Switch entity/owner change polling mapper APIs to id-based cursors and expose latest id lookup helpers. This PR intentionally does not rewrite `JcasbinAuthorizer` cache behavior. The JcasbinAuthorizer version-validated cache refactor will stay in the follow-up PR. ### Why are the changes needed? This reduces the size of #10996 and provides the reusable support layer needed by the JcasbinAuthorizer cache refactor. Fix: #11032 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - `./gradlew :core:spotlessApply :server-common:spotlessApply :server:spotlessApply` - `./gradlew :core:test --tests org.apache.gravitino.authorization.TestAuthorizationRequestContext --tests org.apache.gravitino.cache.TestGravitinoCache --tests org.apache.gravitino.storage.relational.mapper.provider.base.TestAuthMappers --tests org.apache.gravitino.storage.relational.mapper.provider.base.TestEntityChangeLogMapper --tests org.apache.gravitino.storage.relational.service.TestEntityChangeLogService --tests org.apache.gravitino.storage.relational.service.TestTableMetaService :server-common:test --tests org.apache.gravitino.server.authorization.TestPassThroughAuthorizer :server:test --tests org.apache.gravitino.server.web.filter.TestGravitinoInterceptionService`
yuqi1129
added a commit
to yuqi1129/gravitino
that referenced
this pull request
May 14, 2026
…st isolation Address review polish on PR apache#10996: - Add a class-level doc on JcasbinAuthorizer summarising the three cache families (per-request dedup, version-validated, eventual-consistency) so reviewers do not have to read the entire 1100-line file to get the consistency model. - Document the id-based high-water cursor's "in-flight commit" trade-off next to the cursor init, and the entity_change_log writer contract (pre-mutation full_name) next to pollEntityChanges. - Note in Configs.GRAVITINO_AUTHORIZATION_ROLE_CACHE_SIZE that the same value drives the user-role, group-role, and loaded-role caches so operators can size accordingly. - TestJcasbinAuthorizer now builds a fresh JcasbinAuthorizer per test in @beforeeach and closes it in @AfterEach so enforcer g-rows and version-validated cache state never bleed across cases regardless of JUnit execution order. Follow-up tracked in apache#11088 (route isSelf(ROLE) through the version-validated cache). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jerryshao
reviewed
May 15, 2026
| .doc("The interval in seconds for polling entity and owner changes") | ||
| .version(ConfigConstants.VERSION_1_3_0) | ||
| .longConf() | ||
| .createWithDefault(DEFAULT_GRAVITINO_AUTHORIZATION_CHANGE_POLL_INTERVAL_SECS); |
Contributor
There was a problem hiding this comment.
Can you also update the doc about newly added configurations.
Comment on lines
+902
to
+922
| // 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); |
Comment on lines
+778
to
+787
| Optional<CachedUserRoles> cachedOpt = userRoleCache.getIfPresent(userCacheKey); | ||
|
|
||
| if (cachedOpt.isPresent() && cachedOpt.get().getUpdatedAt() >= userInfo.getUpdatedAt()) { | ||
| // Cache is still valid | ||
| CachedUserRoles cached = cachedOpt.get(); | ||
| bindUserRoles(userId, cached.getRoleIds()); | ||
| return cached.getRoleIds(); | ||
| } | ||
|
|
||
| // Cache miss or stale — reload from DB |
Comment on lines
+830
to
+833
| if (cachedOpt.isPresent() && cachedOpt.get().getUpdatedAt() >= groupInfo.getUpdatedAt()) { | ||
| CachedGroupRoles cached = cachedOpt.get(); | ||
| bindUserRoles(userId, cached.getRoleIds()); | ||
| return cached.getRoleIds(); |
Comment on lines
+897
to
+899
| if (cachedUpdatedAt.isPresent() && cachedUpdatedAt.get() >= dbUpdatedAt) { | ||
| // Role policies are still current | ||
| continue; |
Comment on lines
+28
to
+29
| annotationProcessor(libs.lombok) | ||
| compileOnly(libs.lombok) |
Comment on lines
+61
to
+62
| testAnnotationProcessor(libs.lombok) | ||
| testCompileOnly(libs.lombok) |
Comment on lines
+338
to
+352
| public static final ConfigEntry<Long> GRAVITINO_AUTHORIZATION_METADATA_ID_CACHE_SIZE = | ||
| new ConfigBuilder("gravitino.authorization.jcasbin.metadataIdCacheSize") | ||
| .doc("The maximum size of the metadata-id cache for authorization") | ||
| .version(ConfigConstants.VERSION_1_3_0) | ||
| .longConf() | ||
| .createWithDefault(DEFAULT_GRAVITINO_AUTHORIZATION_METADATA_ID_CACHE_SIZE); | ||
|
|
||
| public static final long DEFAULT_GRAVITINO_AUTHORIZATION_CHANGE_POLL_INTERVAL_SECS = 3L; | ||
|
|
||
| public static final ConfigEntry<Long> GRAVITINO_AUTHORIZATION_CHANGE_POLL_INTERVAL_SECS = | ||
| new ConfigBuilder("gravitino.authorization.jcasbin.changePollIntervalSecs") | ||
| .doc("The interval in seconds for polling entity and owner changes") | ||
| .version(ConfigConstants.VERSION_1_3_0) | ||
| .longConf() | ||
| .createWithDefault(DEFAULT_GRAVITINO_AUTHORIZATION_CHANGE_POLL_INTERVAL_SECS); |
| new ConfigBuilder("gravitino.authorization.jcasbin.changePollIntervalSecs") | ||
| .doc("The interval in seconds for polling entity and owner changes") | ||
| .version(ConfigConstants.VERSION_1_3_0) | ||
| .longConf() |
Comment on lines
+902
to
+905
| // Stale or missing — evict old policies and reload | ||
| if (cachedUpdatedAt.isPresent()) { | ||
| allowEnforcer.deleteRole(String.valueOf(roleId)); | ||
| denyEnforcer.deleteRole(String.valueOf(roleId)); |
Comment on lines
+213
to
+230
| // Initialize id cursors to the current DB tail so startup does not scan historical changes. | ||
| // | ||
| // Known trade-off: an id-based high-water mark can miss rows whose id is allocated before | ||
| // the cursor snapshot but whose commit lands after it. Concretely, if writer A holds id=N-1 | ||
| // uncommitted while writer B commits id=N, selectMaxChangeId() returns N and the next poll | ||
| // queries `id > N` — A's row is never consumed. In that case the affected cache entry stays | ||
| // stale until either (a) the version-validated path catches it on the next request, or | ||
| // (b) TTL eviction. Acceptable for the eventual-consistency caches (metadataIdCache / | ||
| // ownerRelCache) targeted by these pollers; revisit if we ever route strong-consistency | ||
| // data through here. | ||
| ownerPollHighWaterId = | ||
| nullToZero( | ||
| SessionUtils.getWithoutCommit( | ||
| OwnerMetaMapper.class, OwnerMetaMapper::selectMaxChangeId)); | ||
| entityPollHighWaterId = | ||
| nullToZero( | ||
| SessionUtils.getWithoutCommit( | ||
| EntityChangeLogMapper.class, EntityChangeLogMapper::selectMaxChangeId)); |
roryqi
reviewed
May 15, 2026
Contributor
roryqi
left a comment
roryqi
left a comment
There was a problem hiding this comment.
Could u extract a dedicated class to handle the function to handle polling the expired data?
| * @param nameIdentifier the entity name identifier | ||
| * @param type the entity type | ||
| */ | ||
| default void handleEntityStructuralChange( |
Contributor
There was a problem hiding this comment.
How about handleEntityNameIdMappingChange?
| * Evicted by entity change poller. | ||
| */ | ||
| private Cache<Long, Boolean> loadedRoles; | ||
| private GravitinoCache<String, Long> metadataIdCache; |
Contributor
There was a problem hiding this comment.
I have some concern about this point.
- You add a concept a hierarchical key. It will make developers confused. What's the relationship with hierarchical schema.
- You can use a object as the key. It will better than using a flat string.
jerryshao
reviewed
May 15, 2026
| cache.cleanUp(); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This class is so big, I suggest that we can split into several small classes.
Contributor
Author
There was a problem hiding this comment.
I see. let me split it again.
… JcasbinAuthorizer caches Introduces the HA invalidation infrastructure that will later carry the version-validated role caching (tracked in apache#10772). This change leaves the existing role-loading path on its current TTL-only behavior. Changes ------- * `JcasbinAuthorizationLookups`: two-tier metadata-id and owner lookup facade (per-request dedup via AuthorizationRequestContext, shared Caffeine-backed GravitinoCache fallback, DB on miss). Owners are now fetched directly from `owner_meta` via OwnerMetaMapper instead of the OWNER_REL relation query, so we have a metadataId-keyed cache that matches the change-log key space. * `JcasbinChangePoller`: scheduled thread that drains `entity_change_log` and `owner_meta` change rows since a high-water cursor and invalidates the affected `metadataIdCache` / `ownerRelCache` keys. Documents the id-cursor in-flight-commit trade-off and the writer-side pre-mutation-name contract. * `JcasbinAuthorizer`: - wires the two GravitinoCaches, the lookups facade, and the poller in `initialize()` / `close()`; - routes `isOwner`, `authorizeByJcasbin` (OWNER path) and `handleMetadataOwnerChange` through the lookups; - drops the old in-class `OwnerInfo` (replaced by `org.apache.gravitino.storage.relational.po.auth.OwnerInfo`) and the `loadOwnerPolicy` / `checkOwnership` pair, collapsed into a single `ownerMatchesUserOrGroups` helper that consumes the resolved `Optional<OwnerInfo>`; - implements `handleEntityStructuralChange` to invalidate (or prefix invalidate) `metadataIdCache` on rename / drop. * `GravitinoAuthorizer`: adds the `handleEntityStructuralChange` default method so callers can wire the new hook without breaking existing implementations. * `Configs`: adds `metadataIdCacheSize` and `changePollIntervalSecs`. The version-validated role caches and the JcasbinRoleLoader rewrite ride on top of this change in a follow-up PR. Test plan --------- `./gradlew :server-common:test --tests 'org.apache.gravitino.server.authorization.*' -PskipITs` `./gradlew :server-common:javadoc :core:javadoc` Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…nAuthorizer Builds on top of apache#11117 (eventual-consistency invalidation): adds the strong-consistency role-loading layer. With this PR landed, every cache in JcasbinAuthorizer has an explicit consistency story. What lands here --------------- - `CachedUserRoles` / `CachedGroupRoles`: small immutable snapshots carrying the {role ids, source updated_at} pair used as the staleness sentinel. - `JcasbinLoadedRolesCache`: extracts the previously-inline `LoadedRolesCache` to its own file. Wraps a Caffeine cache with a synchronous removal listener so that `loadedRoles` eviction always flushes the role's JCasbin policies from both enforcers. - `JcasbinAuthorizer` now: - keeps `userRoleCache` and `groupRoleCache` version-validated against `user_meta.updated_at` / `group_meta.updated_at` (probes per request via `loadUserInfo` / `loadGroupInfo`, with per-request dedup through `AuthorizationRequestContext`); - changes `loadedRoles` from `<Long, Boolean>` to `<Long, Long>` (storing `role_meta.updated_at`) so role-policy reloads happen on DB-version change, not TTL expiry; - replaces the `Executor` + `CompletableFuture` async role-load path with a synchronous 4-step `loadRolePrivilege`: 1. version-validated user-direct roles 2. version-validated group-inherited roles for every group in the current `UserPrincipal` 3. prune stale g-rows (IdP group removal / role unassignment) 4. batch `role_meta` version check + reload of stale policies - reuses `loadUserInfo`'s per-request cache from `isMetalakeUser` / `isOwner` so a single HTTP request never re-queries `user_meta`. - `Configs.GRAVITINO_AUTHORIZATION_ROLE_CACHE_SIZE` doc now notes that the same value sizes user-role, group-role and loaded-role caches. - Lombok wired in `server-common/build.gradle.kts` for the cached snapshot POJOs. Test plan --------- `./gradlew :server-common:test --tests 'org.apache.gravitino.server.authorization.*' -PskipITs` `./gradlew :server-common:javadoc :core:javadoc` Tests cover: version-validated user-role cache, group-inherited role, stale group skipped, group-role revocation, multi-group partial revocation, IdP group removal pruning g-rows, deny-wins over group-inherited allow, role-shared-by-user-and-group survives single-side revocation, plus the existing owner, isSelf, and hasMetadataPrivilegePermission flows. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
yuqi1129
force-pushed
the
feat/jcasbin-cache-refactor
branch
from
40a5dfb to
62ae012
Compare
Contributor
Author
|
We need to merge #11117 first before moving this PR forward, so I will convert this one to a draft. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This is the final piece of the #10812 split. It rewrites
JcasbinAuthorizer's strong-consistency caching layer on top of the lookups + change-poller infrastructure introduced in #11117:AuthorizationRequestContext: user identity, group identity and the role-load step are deduplicated within a single HTTP request.userRoleCache,groupRoleCache,loadedRoles): cache hits revalidate againstuser_meta.updated_at/group_meta.updated_at/role_meta.updated_atbefore being trusted, replacing TTL-only invalidation.JcasbinRoleLoader(new): owns the 4-step role load — version-checked user-direct roles, version-checked group-inherited roles, prune stale g-rows, then batch version-check + reload ofrole_meta.JcasbinLoadedRolesCache(new): wraps aGravitinoCachewith a removal listener that synchronously deletes the role's JCasbin policies, so eviction and policy state stay in sync.Executor+CompletableFutureasync role-load path; role loading is now synchronous viaAuthorizationRequestContext.loadRole(Runnable).isMetalakeUsernow takesAuthorizationRequestContextso the per-requestUserUpdatedAtcache populated byauthorize/isOwneris reused, removing a redundant DB round-trip.Why are the changes needed?
Fix: #10772
Does this PR introduce any user-facing change?
No. New configs (
gravitino.authorization.jcasbin.*) have safe defaults.How was this patch tested?
TestGravitinoCache(TTL, max-size eviction, prefix edge cases) andTestAuthorizationRequestContext(request-scoped dedup for user/owner/metadata-id, including absent-result caching).TestJcasbinAuthorizergroup-role / version-validated cases: group-inherited role, IdP group removal, multi-group partial revocation, deny-wins over group-inherited allow, role-shared-by-user-and-group survives single-side revocation.TestJcasbinAuthorizer,TestJcasbinAuthorizerCacheHelpersandTestPassThroughAuthorizersuites still pass.