Add caching support for IdentityProviderStorageProvider.getForLogin operations#2
Conversation
Closes #32573 Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
WalkthroughCaching and invalidation for identity providers (IDPs) filtered by login availability and fetch mode are introduced. New methods and cache keys support efficient retrieval and coherence of login-related IDP queries. Predicate logic for login eligibility is refined, explicit enabled checks are added in filtering, and a comprehensive test validates cache population and invalidation scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant InfinispanIDPStorageProvider
participant Cache
participant DelegateProvider
Client->>InfinispanIDPStorageProvider: getForLogin(fetchMode, organizationId)
InfinispanIDPStorageProvider->>Cache: Check login cache (cacheKeyForLogin)
alt Cache hit and valid
Cache-->>InfinispanIDPStorageProvider: Return cached IDPs
InfinispanIDPStorageProvider-->>Client: Return IDPs
else Cache miss or invalid
InfinispanIDPStorageProvider->>DelegateProvider: Load IDPs for login
DelegateProvider-->>InfinispanIDPStorageProvider: Return IDPs
InfinispanIDPStorageProvider->>Cache: Store IDPs in cache
InfinispanIDPStorageProvider-->>Client: Return IDPs
end
sequenceDiagram
participant Admin
participant InfinispanIDPStorageProvider
participant Cache
Admin->>InfinispanIDPStorageProvider: create/update/remove IDP
InfinispanIDPStorageProvider->>Cache: Register invalidation (if login-eligible or org changes)
Cache-->>InfinispanIDPStorageProvider: Invalidate affected login caches
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java (1)
369-535: Comprehensive test coverage for login IDP caching!The test thoroughly validates caching behavior across multiple scenarios:
- Initial empty cache state
- Cache population with different fetch modes
- Four distinct invalidation scenarios
The test design effectively verifies that caches are selectively invalidated only when necessary, which is crucial for performance.
Consider these improvements for better maintainability:
- Extract magic numbers as constants:
+ private static final int TOTAL_IDPS = 20; + private static final int ORG_IDPS_START_INDEX = 10; + @Test public void testCacheIDPForLogin() { // create 20 providers, and associate 10 of them with an organization. - for (int i = 0; i < 20; i++) { + for (int i = 0; i < TOTAL_IDPS; i++) {
- Consider breaking this 166-line test into smaller, focused test methods:
testCacheIDPForLogin_InitialState()testCacheIDPForLogin_NoInvalidationOnNonLoginChanges()testCacheIDPForLogin_InvalidationOnLoginStatusChange()testCacheIDPForLogin_InvalidationOnOrgAssociation()model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java (1)
382-389: Consider adding debug logging for cache invalidations.Cache invalidation can be a source of performance issues if it happens too frequently. Adding debug logging would help with troubleshooting.
private void registerIDPLoginInvalidation(IdentityProviderModel idp) { // only invalidate login caches if the IDP qualifies as a login IDP. if (getLoginPredicate().test(idp)) { + if (logger.isDebugEnabled()) { + logger.debugf("Invalidating login caches for IDP: %s", idp.getAlias()); + } for (FetchMode mode : FetchMode.values()) { realmCache.registerInvalidation(cacheKeyForLogin(getRealm(), mode)); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java(6 hunks)server-spi/src/main/java/org/keycloak/models/IdentityProviderStorageProvider.java(1 hunks)services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareIdentityProviderBean.java(1 hunks)testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/cache/OrganizationCacheTest.java(3 hunks)
🔇 Additional comments (5)
services/src/main/java/org/keycloak/organization/forms/login/freemarker/model/OrganizationAwareIdentityProviderBean.java (1)
75-75: LGTM! Defensive re-check for wrapped IDPs.The explicit
isEnabled()re-checks are necessary and correctly placed. Since IDPs may be wrapped with organization-aware implementations that override theisEnabled()method, these checks ensure that only truly enabled IDPs are included in the results.Also applies to: 80-80
server-spi/src/main/java/org/keycloak/models/IdentityProviderStorageProvider.java (1)
254-254: Well-structured login predicate enhancement.The predicate correctly restricts login-eligible IDPs to those either not associated with any organization or explicitly marked as public brokers. I’ve verified that
OrganizationModel.BROKER_PUBLICis properly imported and defined inOrganizationModel.java. LGTM.model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java (3)
77-79: Clean cache key generation for login-specific caches.The cache key structure appropriately includes the realm ID and fetch mode, enabling efficient cache management per fetch mode.
213-255: Well-implemented caching for login IDPs.The
getForLoginmethod correctly follows the established caching pattern used ingetByOrganization. It properly handles:
- Cache miss scenarios
- Partial cache entries
- Cache invalidation when IDPs are not found
391-418: Excellent selective cache invalidation logic!The
registerIDPLoginInvalidationOnUpdatemethod implements smart invalidation that only clears caches when necessary. This optimization prevents unnecessary cache invalidations when:
- Non-login IDPs remain non-login after update
- Login IDPs remain login-eligible with unchanged organization association
This is a performance-conscious implementation that minimizes cache churn.
| String cacheKey = cacheKeyIdpAlias(getRealm(), alias); | ||
| IdentityProviderModel storedIdp = idpDelegate.getByAlias(alias); | ||
| if (isInvalid(cacheKey)) { | ||
| //lookup idp by alias in cache to be able to invalidate its internalId | ||
| registerIDPInvalidation(idpDelegate.getByAlias(alias)); | ||
| registerIDPInvalidation(storedIdp); | ||
| } else { | ||
| CachedIdentityProvider cached = realmCache.getCache().get(cacheKey, CachedIdentityProvider.class); | ||
| if (cached != null) { | ||
| registerIDPInvalidation(cached.getIdentityProvider()); | ||
| } | ||
| } | ||
| registerCountInvalidation(); | ||
| registerIDPLoginInvalidation(storedIdp); |
There was a problem hiding this comment.
Potential NPE when removing non-existent IDP.
The storedIdp retrieved at line 100 could be null if the alias doesn't exist, which would cause issues when passed to registerIDPInvalidation and registerIDPLoginInvalidation.
Add null check:
@Override
public boolean remove(String alias) {
String cacheKey = cacheKeyIdpAlias(getRealm(), alias);
IdentityProviderModel storedIdp = idpDelegate.getByAlias(alias);
+ if (storedIdp == null) {
+ return false;
+ }
if (isInvalid(cacheKey)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String cacheKey = cacheKeyIdpAlias(getRealm(), alias); | |
| IdentityProviderModel storedIdp = idpDelegate.getByAlias(alias); | |
| if (isInvalid(cacheKey)) { | |
| //lookup idp by alias in cache to be able to invalidate its internalId | |
| registerIDPInvalidation(idpDelegate.getByAlias(alias)); | |
| registerIDPInvalidation(storedIdp); | |
| } else { | |
| CachedIdentityProvider cached = realmCache.getCache().get(cacheKey, CachedIdentityProvider.class); | |
| if (cached != null) { | |
| registerIDPInvalidation(cached.getIdentityProvider()); | |
| } | |
| } | |
| registerCountInvalidation(); | |
| registerIDPLoginInvalidation(storedIdp); | |
| @Override | |
| public boolean remove(String alias) { | |
| String cacheKey = cacheKeyIdpAlias(getRealm(), alias); | |
| IdentityProviderModel storedIdp = idpDelegate.getByAlias(alias); | |
| if (storedIdp == null) { | |
| return false; | |
| } | |
| if (isInvalid(cacheKey)) { | |
| //lookup idp by alias in cache to be able to invalidate its internalId | |
| registerIDPInvalidation(storedIdp); | |
| } else { | |
| CachedIdentityProvider cached = realmCache.getCache().get(cacheKey, CachedIdentityProvider.class); | |
| if (cached != null) { | |
| registerIDPInvalidation(cached.getIdentityProvider()); | |
| } | |
| } | |
| registerCountInvalidation(); | |
| registerIDPLoginInvalidation(storedIdp); | |
| } |
🤖 Prompt for AI Agents
In
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java
around lines 99 to 111, the variable storedIdp can be null if the alias does not
exist, which leads to potential NullPointerExceptions when passed to
registerIDPInvalidation and registerIDPLoginInvalidation. Add null checks before
calling these methods to ensure storedIdp is not null before invoking them.
2 participants
Test 2
Summary by CodeRabbit