Skip to content

Fix review comments on batch composite-role expansion (N+1 reduction) - #50407

Closed
ahus1 with Copilot wants to merge 10 commits into
mainfrom
copilot/fix-code-review-comments
Closed

ahus1 with Copilot wants to merge 10 commits into
mainfrom
copilot/fix-code-review-comments

Conversation

Copilot AI commented Jun 29, 2026 •

Copy link
Copy Markdown
Contributor

Addresses six reviewer issues on the batch composite-role expansion PR (#50370) that introduced a level-wise IN query to reduce N+1 getChildRoles calls during composite-role tree expansion.

Changes

RoleUtils.expandCompositeRoles — null-session guard

  • KeycloakSessionUtil.getKeycloakSession() returns null outside a Resteasy/session-bound thread; the previous code dereferenced it unconditionally. Added a null check that falls back to the original per-role stack-based expansion, preserving pre-PR behaviour for callers without a session context.

RoleStorageManager.getCompositeRolesStream — federated role support

  • Previously delegated entirely to localStorage(), silently dropping composite children of federated/external roles. Now splits parentRoleIds by StorageId.isLocalStorage: local IDs go to the batched JPA query; external IDs fall back to getRoleById + getCompositesStream per role. Results are de-duplicated with .distinct().

RealmCacheSession.getCompositeRolesStream — cache key collision + NPE

  • Cache key was built from sorted parent IDs only, allowing different realms with identical parent-ID sets to collide. Key is now prefixed with realm.getId() + "::" and uses String.join instead of List.toString() for an unambiguous format.
  • cache.get(parentId, CachedRole.class) can return null on eviction; dereferencing it caused an NPE. Added a null check that treats a missing or evicted parent entry as a cache miss.

CompositeRoleExpansionQueryCountTest — statistics state leak + misleading test name

  • countQueries left Hibernate Statistics permanently enabled on the shared SessionFactory, potentially affecting other tests. Now saves and restores isStatisticsEnabled() around the measurement.
  • Renamed droppingIsCompositePreQueryHalvesGetChildRolesOnCompositeNodes → chainWalkIssuesOneQueryPerBreadthFirstLevel to match what the assertion actually validates.

Hüseyin Akdoğan and others added 9 commits June 29, 2026 01:08
RoleUtils.expandCompositeRolesStream gated each node on isComposite()
before calling getCompositesStream(). On the JPA (cache-miss) path both
run the getChildRoles named query -- and a FlushMode.AUTO flush before
each -- so every composite node issued two getChildRoles round-trips
while expanding the tree. getCompositesStream() already yields an empty
stream for non-composite roles, making the pre-check redundant.

Fetch the composites directly instead. Behaviour is unchanged (verified
by RoleUtilsCompositeExpansionTest: identical expansion, isComposite()
no longer called, cycles still terminate). On a 10-node composite chain
the deterministic query-count model test (CompositeRoleExpansionQueryCountTest,
jpa profile) shows getChildRoles executions drop from 19 to 10 -- the
saving equals the number of composite nodes (C) per expansion.

Signed-off-by: Hüseyin Akdoğan <huseyin@keymate.io>
The composite-role walk in RoleUtils issues one getChildRoles query per
role node. On a cache miss this is an N+1 over the whole role tree, each
query preceded by a FlushMode.AUTO flush.

Add a bulk primitive RoleProvider.getCompositeRolesStream(realm, parentIds)
-- default resolves each parent individually, JpaRealmProvider overrides it
with a single getChildRolesFromParentIds named query (IN clause) that
hydrates the child RoleEntity rows so the follow-up getRoleById calls are
served from the persistence context. RoleStorageManager delegates to local
storage.

RoleUtils.expandCompositeRoles(session, realm, roles) uses it to expand the
tree one breadth-first level at a time: one query per level instead of one
per node, while preserving the visited-set cycle protection and dedup.

CompositeRoleExpansionQueryCountTest (jpa profile) proves it on a 31-node
wide tree (root -> 5 -> 25): the per-node walk issues 31 getChildRoles
queries, level-wise batching issues 3 (one per depth), and both produce the
same expanded role set.

Signed-off-by: Hüseyin Akdoğan <huseyin@keymate.io>
Route the composite-role expansion on the token and permission paths through
the level-wise batched RoleUtils variants so the N+1 reduction reaches the
flows the load test exercises.

Add session-aware RoleUtils.getDeepUserRoleMappings(session, realm, user) and
expandCompositeRolesStream(session, realm, roles), and switch:
 - TokenManager.getAccess: add a session-aware overload; the existing
   (UserModel, ClientModel, Stream) signature is kept and delegates to the
   per-node expansion, so the public API is unchanged,
 - DefaultClientSessionContext client-scope role expansion and user roles,
 - RolePolicyProvider (admin fine-grained permissions) deep user role mappings.

The non-session methods are kept for callers without a session. Behaviour is
unchanged: the batched expansion returns the same role set as the per-node
walk (CompositeRoleExpansionQueryCountTest); a custom RoleProvider that does
not override getCompositeRolesStream still resolves composites via the
interface default (RoleProviderCompositeDefaultTest); RoleModelTest and
CompositeRolesModelTest pass.

Signed-off-by: Hüseyin Akdoğan <huseyin@keymate.io>
- CompositeRoleExpansionQueryCountTest: require JpaConnectionProvider so the
  test is skipped rather than failing with an NPE on non-JPA backends (it
  reads Hibernate statistics via JpaConnectionProvider).
- RoleProviderCompositeDefaultTest: pass a non-null RealmModel (honoring the
  getCompositeRolesStream contract) instead of null.

Signed-off-by: Hüseyin Akdoğan <huseyin@keymate.io>
Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
…er Keycloak strategy

Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>
- RoleUtils.expandCompositeRoles: guard against null KeycloakSession by
  falling back to per-role stack expansion when no session context is
  available (fixes r3488823474)
- RoleStorageManager.getCompositeRolesStream: split parent IDs into local
  and external; external (federated) IDs fall back to per-role
  getRoleById + getCompositesStream instead of being silently dropped
  (fixes r3488823485)
- RealmCacheSession.getCompositeRolesStream: prefix cache key with
  realm ID to prevent cross-realm key collisions; use String.join for
  a more predictable key format (fixes r3488823491)
- RealmCacheSession.getCompositeRolesStream: null-check the result of
  cache.get(parentId, CachedRole.class) before dereferencing it to
  prevent NPE on evicted entries (fixes r3488823493)
- CompositeRoleExpansionQueryCountTest.countQueries: save and restore
  Hibernate statistics enabled state to avoid affecting other tests
  (fixes r3488823497)
- CompositeRoleExpansionQueryCountTest: rename
  droppingIsCompositePreQueryHalvesGetChildRolesOnCompositeNodes to
  chainWalkIssuesOneQueryPerBreadthFirstLevel (fixes r3488823503)
Copilot AI changed the title [WIP] Fix code based on review comments Fix review comments on batch composite-role expansion (N+1 reduction) Jun 29, 2026
Copilot AI requested a review from ahus1 June 29, 2026 01:14
Copilot finished work on behalf of ahus1 June 29, 2026 01:14
Copilot finished work on behalf of ahus1 June 29, 2026 01:17
@ahus1 ahus1 closed this Jun 29, 2026
@stianst
stianst deleted the copilot/fix-code-review-comments branch September 30, 2026 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants