Add caching support for IdentityProviderStorageProvider.getForLogin operations - #9
Conversation
Closes #32573 Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if (i >= 10) | ||
| idpRep.getConfig().put(OrganizationModel.BROKER_PUBLIC, Boolean.TRUE.toString()); | ||
| testRealm().identityProviders().create(idpRep).close(); | ||
| getCleanup().addCleanup(testRealm().identityProviders().get("alias")::remove); |
There was a problem hiding this comment.
Test cleanup uses wrong alias string literal
Medium Severity
The cleanup handler references the literal string "alias" instead of the actual IDP alias. At line 381, IDPs are created with aliases "idp-alias-0" through "idp-alias-19", but cleanup tries to remove an IDP named "alias" which doesn't exist. Same issue at line 425 for "idp-alias-20". Since TestCleanup.addCleanup silently swallows exceptions, these cleanups fail silently, leaving all created IDPs behind and causing test pollution across subsequent test runs.
Additional Locations (1)
| @Override | ||
| public boolean remove(String alias) { | ||
| String cacheKey = cacheKeyIdpAlias(getRealm(), alias); | ||
| IdentityProviderModel storedIdp = idpDelegate.getByAlias(alias); |
There was a problem hiding this comment.
Remove method unconditionally queries database for IDP
Low Severity
The idpDelegate.getByAlias(alias) call was moved outside the isInvalid(cacheKey) branch, causing it to execute unconditionally on every remove call. Previously, the delegate was only consulted when the cache was invalid. When the cache is valid, cached.getIdentityProvider() at line 107 already provides the IDP model, which could also be used for registerIDPLoginInvalidation — avoiding the unnecessary database round-trip.
2 participants
Test 2nnn## Summary by CodeRabbitnn* New Featuresn * Implemented caching mechanism for identity provider login retrieval to improve performance.nn* Bug Fixesn * Enhanced identity provider filtering to enforce enabled status and organization visibility requirements.n * Improved handling of organization linkage and broker public accessibility in login scenarios.nn* Testsn * Added comprehensive test coverage for identity provider login caching behavior.nn✏️ Tip: You can customize this high-level summary in your review settings.nnn---nReplicated from ai-code-review-evaluation/keycloak-coderabbit#2
Note
Medium Risk
Touches login-path identity provider selection and caching/invalidation logic; correctness depends on subtle eligibility and org-link state transitions, though changes are localized and covered by new tests.
Overview
Adds Infinispan-backed caching for
IdentityProviderStorageProvider.getForLoginby storing per-realm/per-FetchModequery results keyed by organization id (or empty key), similar to existing org-IDP list caching.Introduces targeted invalidation for these new login caches on IDP create/update/delete, avoiding invalidation when an update doesn’t change an IDP’s login-eligibility or org linkage; login eligibility now also requires org-linked IDPs to be marked
BROKER_PUBLIC.Updates login-page IDP listing to re-check
isEnabled()after models may be wrapped, and adds an integration test (testCacheIDPForLogin) covering cache population and invalidation scenarios.Written by Cursor Bugbot for commit 3e59772. Configure here.