Repository navigation
Conversation
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
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.
Addresses six reviewer issues on the batch composite-role expansion PR (#50370) that introduced a level-wise
INquery to reduce N+1getChildRolescalls during composite-role tree expansion.Changes
RoleUtils.expandCompositeRoles— null-session guardKeycloakSessionUtil.getKeycloakSession()returnsnulloutside 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 supportlocalStorage(), silently dropping composite children of federated/external roles. Now splitsparentRoleIdsbyStorageId.isLocalStorage: local IDs go to the batched JPA query; external IDs fall back togetRoleById + getCompositesStreamper role. Results are de-duplicated with.distinct().RealmCacheSession.getCompositeRolesStream— cache key collision + NPErealm.getId() + "::"and usesString.joininstead ofList.toString()for an unambiguous format.cache.get(parentId, CachedRole.class)can returnnullon 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 namecountQueriesleft HibernateStatisticspermanently enabled on the sharedSessionFactory, potentially affecting other tests. Now saves and restoresisStatisticsEnabled()around the measurement.droppingIsCompositePreQueryHalvesGetChildRolesOnCompositeNodes→chainWalkIssuesOneQueryPerBreadthFirstLevelto match what the assertion actually validates.