Refactor cache operations to use read-safe methods for concurrent environments#7871
Merged
ThaminduR merged 4 commits intowso2:masterfrom Mar 24, 2026
Merged
Refactor cache operations to use read-safe methods for concurrent environments#7871ThaminduR merged 4 commits intowso2:masterfrom
ThaminduR merged 4 commits intowso2:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces cache-on-read semantics throughout the codebase by adding Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
160
to
+161
| } | ||
| apiResourceCacheById.addToCache(cacheKey, new APIResourceCacheEntry(apiResource), tenantId); | ||
| apiResourceCacheById.addToCacheOnRead(cacheKey, new APIResourceCacheEntry(apiResource), tenantId); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| } | |
| apiResourceCacheById.addToCache(cacheKey, new APIResourceCacheEntry(apiResource), tenantId); | |
| apiResourceCacheById.addToCacheOnRead(cacheKey, new APIResourceCacheEntry(apiResource), tenantId); | |
| } | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("Adding API Resource to cache with ID: " + apiResource.getId() + " and identifier: " + apiResource.getIdentifier()); | |
| } | |
| apiResourceCacheById.addToCacheOnRead(cacheKey, new APIResourceCacheEntry(apiResource), tenantId); |
Comment on lines
200
to
+201
| } | ||
| apiResourceCacheByIdentifier.addToCache(cacheKey, new APIResourceCacheEntry(apiResource), tenantId); | ||
| apiResourceCacheByIdentifier.addToCacheOnRead(cacheKey, new APIResourceCacheEntry(apiResource), tenantId); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| } | |
| apiResourceCacheByIdentifier.addToCache(cacheKey, new APIResourceCacheEntry(apiResource), tenantId); | |
| apiResourceCacheByIdentifier.addToCacheOnRead(cacheKey, new APIResourceCacheEntry(apiResource), tenantId); | |
| } | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("Adding API Resource to cache with identifier: " + apiResource.getIdentifier() + " and ID: " + apiResource.getId()); | |
| } | |
| apiResourceCacheByIdentifier.addToCacheOnRead(cacheKey, new APIResourceCacheEntry(apiResource), tenantId); |
Comment on lines
+170
to
+174
| public void addToCacheOnRead(K key, V entry) { | ||
|
|
||
| if (!isEnabled()) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| public void addToCacheOnRead(K key, V entry) { | |
| if (!isEnabled()) { | |
| return; | |
| } | |
| public void addToCacheOnRead(K key, V entry) { | |
| if (!isEnabled()) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Cache is disabled. Skipping cache population for key."); | |
| } | |
| return; | |
| } |
Comment on lines
+183
to
+186
| Cache<K, V> cache = getBaseCache(); | ||
| if (cache != null) { | ||
| cache.putOnRead(key, entry); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| Cache<K, V> cache = getBaseCache(); | |
| if (cache != null) { | |
| cache.putOnRead(key, entry); | |
| } | |
| Cache<K, V> cache = getBaseCache(); | |
| if (cache != null) { | |
| cache.putOnRead(key, entry); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Cache entry added for key during read operation."); | |
| } | |
| } else { | |
| log.warn("Base cache is null. Unable to add cache entry during read operation."); | |
| } |
Comment on lines
+56
to
+59
| public void addToCacheOnRead(RandomPasswordContainerCacheKey key, RandomPasswordContainerCacheEntry entry) { | ||
|
|
||
| super.addToCacheOnRead(key, entry); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| public void addToCacheOnRead(RandomPasswordContainerCacheKey key, RandomPasswordContainerCacheEntry entry) { | |
| super.addToCacheOnRead(key, entry); | |
| } | |
| public void addToCacheOnRead(RandomPasswordContainerCacheKey key, RandomPasswordContainerCacheEntry entry) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Adding random password container to cache on read for key: " + key.getKey()); | |
| } | |
| super.addToCacheOnRead(key, entry); |
Comment on lines
97
to
99
| UserDefinedLocalAuthenticatorConfig authenticatorConfig = authenticatorMgtFacade | ||
| .getUserDefinedLocalAuthenticator(authenticatorConfigName, tenantId); | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| UserDefinedLocalAuthenticatorConfig authenticatorConfig = authenticatorMgtFacade | |
| .getUserDefinedLocalAuthenticator(authenticatorConfigName, tenantId); | |
| UserDefinedLocalAuthenticatorConfig authenticatorConfig = authenticatorMgtFacade | |
| .getUserDefinedLocalAuthenticator(authenticatorConfigName, tenantId); | |
| if (authenticatorConfig != null) { | |
| LOG.info("Successfully retrieved user-defined local authenticator: " + authenticatorConfigName); | |
| } else { | |
| LOG.warn("User-defined local authenticator not found: " + authenticatorConfigName); | |
| } |
Comment on lines
+100
to
101
| authenticatorCache.addToCacheOnRead(cacheKey, new AuthenticatorCacheEntry(authenticatorConfig), tenantId); | ||
| LOG.debug("Entry fetched from DB for authenticator " + authenticatorConfigName + ". Adding cache entry."); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| authenticatorCache.addToCacheOnRead(cacheKey, new AuthenticatorCacheEntry(authenticatorConfig), tenantId); | |
| LOG.debug("Entry fetched from DB for authenticator " + authenticatorConfigName + ". Adding cache entry."); | |
| authenticatorCache.addToCacheOnRead(cacheKey, new AuthenticatorCacheEntry(authenticatorConfig), tenantId); | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("Cache entry added for authenticator: " + authenticatorConfigName + | |
| " with cache method: addToCacheOnRead"); | |
| } |
Comment on lines
+1965
to
+1968
|
|
||
| // Add application template to cache | ||
| ServiceProviderTemplateCacheKey templateCacheKey = new ServiceProviderTemplateCacheKey(spTemplate.getName()); | ||
| ServiceProviderTemplateCache.getInstance().addToCache(templateCacheKey, spTemplate, tenantDomain); | ||
| ServiceProviderTemplateCache.getInstance().addToCacheOnRead(templateCacheKey, spTemplate, tenantDomain); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
Suggested change
| // Add application template to cache | |
| ServiceProviderTemplateCacheKey templateCacheKey = new ServiceProviderTemplateCacheKey(spTemplate.getName()); | |
| ServiceProviderTemplateCache.getInstance().addToCache(templateCacheKey, spTemplate, tenantDomain); | |
| ServiceProviderTemplateCache.getInstance().addToCacheOnRead(templateCacheKey, spTemplate, tenantDomain); | |
| // Add application template to cache | |
| ServiceProviderTemplateCacheKey templateCacheKey = new ServiceProviderTemplateCacheKey(spTemplate.getName()); | |
| log.debug("Adding application template to cache: " + spTemplate.getName()); | |
| ServiceProviderTemplateCache.getInstance().addToCacheOnRead(templateCacheKey, spTemplate, tenantDomain); |
Comment on lines
+326
to
+333
| private void addDefaultAuthSeqToCacheOnRead(DefaultAuthenticationSequence sequence, String tenantDomain) { | ||
|
|
||
| if (DefaultAuthSeqMgtCache.getInstance().isEnabled()) { | ||
| DefaultAuthSeqMgtCacheEntry entry = new DefaultAuthSeqMgtCacheEntry(sequence); | ||
| DefaultAuthSeqMgtCache.getInstance().addToCacheOnRead(sequence.getName(), entry, tenantDomain); | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("Default authentication sequence for tenant: " + tenantDomain + " is added to cache."); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 11
Suggested change
| private void addDefaultAuthSeqToCacheOnRead(DefaultAuthenticationSequence sequence, String tenantDomain) { | |
| if (DefaultAuthSeqMgtCache.getInstance().isEnabled()) { | |
| DefaultAuthSeqMgtCacheEntry entry = new DefaultAuthSeqMgtCacheEntry(sequence); | |
| DefaultAuthSeqMgtCache.getInstance().addToCacheOnRead(sequence.getName(), entry, tenantDomain); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Default authentication sequence for tenant: " + tenantDomain + " is added to cache."); | |
| } | |
| private void addDefaultAuthSeqToCacheOnRead(DefaultAuthenticationSequence sequence, String tenantDomain) { | |
| if (DefaultAuthSeqMgtCache.getInstance().isEnabled()) { | |
| DefaultAuthSeqMgtCacheEntry entry = new DefaultAuthSeqMgtCacheEntry(sequence); | |
| DefaultAuthSeqMgtCache.getInstance().addToCacheOnRead(sequence.getName(), entry, tenantDomain); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Default authentication sequence: " + sequence.getName() + " for tenant: " + tenantDomain + " is added to cache on read."); | |
| } |
Comment on lines
+83
to
+85
| public void addToCacheOnRead(K key, V entry) { | ||
|
|
||
| addToCacheOnRead(key, entry, getLoginTenantDomainFromContext()); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 12
Suggested change
| public void addToCacheOnRead(K key, V entry) { | |
| addToCacheOnRead(key, entry, getLoginTenantDomainFromContext()); | |
| public void addToCacheOnRead(K key, V entry) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Adding cache entry on read for key: " + key); | |
| } | |
| addToCacheOnRead(key, entry, getLoginTenantDomainFromContext()); |
Comment on lines
+158
to
+159
| */ | ||
| public void addToCacheOnRead(AuthenticationContextCacheKey key, AuthenticationContextCacheEntry entry) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 13
Suggested change
| */ | |
| public void addToCacheOnRead(AuthenticationContextCacheKey key, AuthenticationContextCacheEntry entry) { | |
| public void addToCacheOnRead(AuthenticationContextCacheKey key, AuthenticationContextCacheEntry entry) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Adding authentication context to cache on read for context id: " + key.getContextId()); | |
| } |
Comment on lines
+203
to
+204
| } | ||
| SessionDataStore.getInstance().storeSessionData(key.getContextId(), AUTHENTICATION_CONTEXT_CACHE_NAME, |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 14
Suggested change
| } | |
| SessionDataStore.getInstance().storeSessionData(key.getContextId(), AUTHENTICATION_CONTEXT_CACHE_NAME, | |
| SessionDataStore.getInstance().storeSessionData(key.getContextId(), AUTHENTICATION_CONTEXT_CACHE_NAME, | |
| entry, tenantId); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Session data stored successfully for context id: " + key.getContextId() + | |
| " in tenant: " + tenantDomain); | |
| } |
| * @param key Key which the cache entry is indexed by. | ||
| * @param entry Value to be stored in the cache. | ||
| */ | ||
| public void addToCacheOnRead(AuthenticationErrorCacheKey key, AuthenticationErrorCacheEntry entry) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 15
Suggested change
| public void addToCacheOnRead(AuthenticationErrorCacheKey key, AuthenticationErrorCacheEntry entry) { | |
| public void addToCacheOnRead(AuthenticationErrorCacheKey key, AuthenticationErrorCacheEntry entry) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Adding authentication error cache entry on read for key: " + key.getErrorKey()); | |
| } |
| tenantId = IdentityTenantUtil.getTenantId(tenantDomain); | ||
| } | ||
| } | ||
| SessionDataStore.getInstance().storeSessionData(key.getErrorKey(), CACHE_NAME, entry, tenantId); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 16
Suggested change
| SessionDataStore.getInstance().storeSessionData(key.getErrorKey(), CACHE_NAME, entry, tenantId); | |
| SessionDataStore.getInstance().storeSessionData(key.getErrorKey(), CACHE_NAME, entry, tenantId); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Stored authentication error in session data store for tenant: " + tenantId); | |
| } |
| * @param key Key which the cache entry is indexed by. | ||
| * @param entry Value to be stored in the cache. | ||
| */ | ||
| public void addToCacheOnRead(AuthenticationRequestCacheKey key, AuthenticationRequestCacheEntry entry) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 17
Suggested change
| public void addToCacheOnRead(AuthenticationRequestCacheKey key, AuthenticationRequestCacheEntry entry) { | |
| public void addToCacheOnRead(AuthenticationRequestCacheKey key, AuthenticationRequestCacheEntry entry) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Adding authentication request to cache on read for key: " + key.getResultId()); | |
| } |
Comment on lines
+106
to
+107
| SessionDataStore.getInstance().storeSessionData(key.getResultId(), AUTHENTICATION_REQUEST_CACHE_NAME, | ||
| entry, tenantId); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 18
Suggested change
| SessionDataStore.getInstance().storeSessionData(key.getResultId(), AUTHENTICATION_REQUEST_CACHE_NAME, | |
| entry, tenantId); | |
| SessionDataStore.getInstance().storeSessionData(key.getResultId(), AUTHENTICATION_REQUEST_CACHE_NAME, | |
| entry, tenantId); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Stored authentication request in session data store for tenant: " + tenantDomain); | |
| } |
Comment on lines
+102
to
+103
| public void addToCacheOnRead(AuthenticationResultCacheKey key, AuthenticationResultCacheEntry entry) { | ||
| super.addToCacheOnRead(key, entry); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 19
Suggested change
| public void addToCacheOnRead(AuthenticationResultCacheKey key, AuthenticationResultCacheEntry entry) { | |
| super.addToCacheOnRead(key, entry); | |
| public void addToCacheOnRead(AuthenticationResultCacheKey key, AuthenticationResultCacheEntry entry) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Adding authentication result to cache on read for key: " + key.getResultId()); | |
| } | |
| super.addToCacheOnRead(key, entry); |
Comment on lines
+111
to
+113
| } | ||
| SessionDataStore.getInstance().storeSessionData(key.getResultId(), CACHE_NAME, entry, tenantId); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 20
Suggested change
| } | |
| SessionDataStore.getInstance().storeSessionData(key.getResultId(), CACHE_NAME, entry, tenantId); | |
| } | |
| } | |
| SessionDataStore.getInstance().storeSessionData(key.getResultId(), CACHE_NAME, entry, tenantId); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Stored session data for key: " + key.getResultId() + " in tenant: " + tenantId); | |
| } | |
| } |
Comment on lines
+50
to
+54
| @Override | ||
| public void addToCacheOnRead(LongWaitResultCacheKey key, LongWaitResultCacheEntry entry) { | ||
|
|
||
| super.addToCacheOnRead(key, entry); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 21
Suggested change
| @Override | |
| public void addToCacheOnRead(LongWaitResultCacheKey key, LongWaitResultCacheEntry entry) { | |
| super.addToCacheOnRead(key, entry); | |
| } | |
| @Override | |
| public void addToCacheOnRead(LongWaitResultCacheKey key, LongWaitResultCacheEntry entry) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Adding entry to LongWaitResultCache on read for key: " + key.getKey()); | |
| } | |
| super.addToCacheOnRead(key, entry); | |
| } |
| * @param key Key which the cache entry is indexed by. | ||
| * @param entry Value to be stored in the cache. | ||
| */ | ||
| public void addToCacheOnRead(SessionContextCacheKey key, SessionContextCacheEntry entry, String loginTenantDomain) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 22
Suggested change
| public void addToCacheOnRead(SessionContextCacheKey key, SessionContextCacheEntry entry, String loginTenantDomain) { | |
| public void addToCacheOnRead(SessionContextCacheKey key, SessionContextCacheEntry entry, String loginTenantDomain) { | |
| log.info("Adding session context to cache on read for context id: " + key.getContextId()); |
Comment on lines
+131
to
+135
| } catch (SessionDataStorageOptimizationClientException e) { | ||
| if (log.isDebugEnabled()) { | ||
| log.debug(String.format("Client error occurred while optimizing the Session " + | ||
| "context with context id: %s", entry.getContext()), e); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 23
Suggested change
| } catch (SessionDataStorageOptimizationClientException e) { | |
| if (log.isDebugEnabled()) { | |
| log.debug(String.format("Client error occurred while optimizing the Session " + | |
| "context with context id: %s", entry.getContext()), e); | |
| } | |
| } catch (SessionDataStorageOptimizationClientException e) { | |
| if (log.isDebugEnabled()) { | |
| log.debug(String.format("Client error occurred while optimizing the Session " + | |
| "context with context id: %s", entry.getContext()), e); | |
| } | |
| log.warn("Failed to optimize session context for context id: " + key.getContextId() + | |
| " due to client error: " + e.getMessage()); |
Comment on lines
81
to
+82
| LongWaitResultCacheEntry cacheEntry = new LongWaitResultCacheEntry(status); | ||
| LongWaitResultCache.getInstance().addToCache(cacheKey, cacheEntry); | ||
| LongWaitResultCache.getInstance().addToCacheOnRead(cacheKey, cacheEntry); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 24
Suggested change
| LongWaitResultCacheEntry cacheEntry = new LongWaitResultCacheEntry(status); | |
| LongWaitResultCache.getInstance().addToCache(cacheKey, cacheEntry); | |
| LongWaitResultCache.getInstance().addToCacheOnRead(cacheKey, cacheEntry); | |
| LongWaitResultCacheKey cacheKey = new LongWaitResultCacheKey(waitKey); | |
| LongWaitResultCacheEntry cacheEntry = new LongWaitResultCacheEntry(status); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Adding long wait status to cache for waitKey: " + waitKey); | |
| } | |
| LongWaitResultCache.getInstance().addToCacheOnRead(cacheKey, cacheEntry); |
Comment on lines
+77
to
+78
| public void addToCacheOnRead(String key, IdentityMessageContext context) { | ||
| super.addToCacheOnRead(key, context); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 25
Suggested change
| public void addToCacheOnRead(String key, IdentityMessageContext context) { | |
| super.addToCacheOnRead(key, context); | |
| public void addToCacheOnRead(String key, IdentityMessageContext context) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Adding identity message context to cache on read for key: " + key); | |
| } | |
| super.addToCacheOnRead(key, context); |
| if (tenantDomain != null) { | ||
| tenantId = IdentityTenantUtil.getTenantId(tenantDomain); | ||
| } | ||
| SessionDataStore.getInstance().storeSessionData(key, INBOUND_CONTEXT_CACHE_NAME, context, tenantId); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 26
Suggested change
| SessionDataStore.getInstance().storeSessionData(key, INBOUND_CONTEXT_CACHE_NAME, context, tenantId); | |
| SessionDataStore.getInstance().storeSessionData(key, INBOUND_CONTEXT_CACHE_NAME, context, tenantId); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Stored session data in request scope cache for tenant: " + tenantDomain); | |
| } |
Comment on lines
1292
to
+1293
| cacheEntry.setContext(sessionContext); | ||
| SessionContextCache.getInstance().addToCache(cacheKey, cacheEntry); | ||
| SessionContextCache.getInstance().addToCache(cacheKey, cacheEntry, getLoginTenantDomainFromContext()); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 27
Suggested change
| cacheEntry.setContext(sessionContext); | |
| SessionContextCache.getInstance().addToCache(cacheKey, cacheEntry); | |
| SessionContextCache.getInstance().addToCache(cacheKey, cacheEntry, getLoginTenantDomainFromContext()); | |
| } | |
| cacheEntry.setContext(sessionContext); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Adding session context to cache for key: " + cacheKey.getKey()); | |
| } | |
| SessionContextCache.getInstance().addToCache(cacheKey, cacheEntry, getLoginTenantDomainFromContext()); |
Comment on lines
129
to
+130
| localClaimList = super.getLocalClaims(tenantId); | ||
| localClaimCache.addToCache(tenantId, new ArrayList<>(localClaimList), tenantId); | ||
| localClaimCache.addToCacheOnRead(tenantId, new ArrayList<>(localClaimList), tenantId); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 28
Suggested change
| localClaimList = super.getLocalClaims(tenantId); | |
| localClaimCache.addToCache(tenantId, new ArrayList<>(localClaimList), tenantId); | |
| localClaimCache.addToCacheOnRead(tenantId, new ArrayList<>(localClaimList), tenantId); | |
| localClaimList = super.getLocalClaims(tenantId); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Retrieved " + localClaimList.size() + " local claims from store for tenant: " + tenantId); | |
| } | |
| localClaimCache.addToCacheOnRead(tenantId, new ArrayList<>(localClaimList), tenantId); |
Comment on lines
213
to
+214
| externalClaimList = super.getExternalClaims(externalClaimDialectURI, tenantId); | ||
| externalClaimCache.addToCache(cacheKey, new ArrayList<>(externalClaimList), tenantId); | ||
| externalClaimCache.addToCacheOnRead(cacheKey, new ArrayList<>(externalClaimList), tenantId); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 29
Suggested change
| externalClaimList = super.getExternalClaims(externalClaimDialectURI, tenantId); | |
| externalClaimCache.addToCache(cacheKey, new ArrayList<>(externalClaimList), tenantId); | |
| externalClaimCache.addToCacheOnRead(cacheKey, new ArrayList<>(externalClaimList), tenantId); | |
| externalClaimList = super.getExternalClaims(externalClaimDialectURI, tenantId); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Retrieved " + externalClaimList.size() + " external claims from store for dialect: " + | |
| externalClaimDialectURI + " in tenant: " + tenantId); | |
| } | |
| externalClaimCache.addToCacheOnRead(cacheKey, new ArrayList<>(externalClaimList), tenantId); |
Comment on lines
+58
to
+61
| public void putClaimDialectsOnRead(int tenantId, List<ClaimDialect> claimDialectList) { | ||
|
|
||
| super.addToCacheOnRead(tenantId, (Serializable) claimDialectList, tenantId); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 30
Suggested change
| public void putClaimDialectsOnRead(int tenantId, List<ClaimDialect> claimDialectList) { | |
| super.addToCacheOnRead(tenantId, (Serializable) claimDialectList, tenantId); | |
| } | |
| public void putClaimDialectsOnRead(int tenantId, List<ClaimDialect> claimDialectList) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Adding claim dialects to cache on read for tenant: " + tenantId); | |
| } | |
| super.addToCacheOnRead(tenantId, (Serializable) claimDialectList, tenantId); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationErrorCache.java (1)
1-2:⚠️ Potential issue | 🟡 MinorUpdate copyright year to include current year.
The license header shows "Copyright (c) 2022" but per coding guidelines, it should include the current year (e.g., "2022-2026" or "2026").
Suggested fix
/* - * Copyright (c) 2022, WSO2 Inc. (http://www.wso2.org). + * Copyright (c) 2022-2026, WSO2 Inc. (http://www.wso2.org). *As per coding guidelines: "Ensure that all Java files contain the appropriate license header at the top with copyright year as the current year or a range ending in the current year (e.g., 2018-2026)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationErrorCache.java` around lines 1 - 2, Update the file header in AuthenticationErrorCache (class/file: AuthenticationErrorCache.java) to use the current copyright year or a year-range ending in the current year (e.g., "2022-2026" or "2026") instead of the standalone "2022"; modify the top-of-file comment block (license header) accordingly so the copyright span reflects the current year per project guidelines.components/system-config-mgt/org.wso2.carbon.identity.system.config.mgt/src/main/java/org/wso2/carbon/identity/system/config/mgt/advisory/DBBasedAdminBannerDAO.java (1)
2-2:⚠️ Potential issue | 🟠 MajorUpdate the license header year to include the current year.
Line 2 ends at
2025, but the current required range should end in2026.Proposed fix
- * Copyright (c) 2025, WSO2 LLC. (http://www.wso2.com). + * Copyright (c) 2025-2026, WSO2 LLC. (http://www.wso2.com).As per coding guidelines,
**/*.java: "Ensure that all Java files contain the appropriate license header at the top with copyright year as the current year or a range ending in the current year (e.g., 2018-2026)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/system-config-mgt/org.wso2.carbon.identity.system.config.mgt/src/main/java/org/wso2/carbon/identity/system/config/mgt/advisory/DBBasedAdminBannerDAO.java` at line 2, Update the license header year range in the top of DBBasedAdminBannerDAO.java so it ends in 2026 instead of 2025; locate the file by the class/file name DBBasedAdminBannerDAO and change the copyright line that currently reads "2025" (or "2018-2025") to the correct current year or range ending in "2026".components/cors-mgt/org.wso2.carbon.identity.cors.mgt.core/src/main/java/org/wso2/carbon/identity/cors/mgt/core/dao/impl/CacheBackedCORSConfigurationDAO.java (1)
2-2:⚠️ Potential issue | 🟡 MinorUpdate the license header year to end in 2026.
The header currently ends at
2021; this file should use the current year or a range ending in the current year.As per coding guidelines, "
**/*.java: Ensure that all Java files contain the appropriate license header at the top with copyright year as the current year or a range ending in the current year (e.g., 2018-2026)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/cors-mgt/org.wso2.carbon.identity.cors.mgt.core/src/main/java/org/wso2/carbon/identity/cors/mgt/core/dao/impl/CacheBackedCORSConfigurationDAO.java` at line 2, Update the license header year range in the top-of-file comment so it ends in 2026 (e.g., change "2021" to "2018-2026" or "2021-2026" as appropriate); edit the header in CacheBackedCORSConfigurationDAO (class CacheBackedCORSConfigurationDAO) to reflect the current year/range and keep the rest of the licence text unchanged.components/application-mgt/org.wso2.carbon.identity.application.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/defaultsequence/DefaultAuthSeqMgtServiceImpl.java (1)
2-2:⚠️ Potential issue | 🟡 MinorUpdate the Java license header year range to include 2026.
The header currently uses only
2018; please update it to a range ending in the current year.Proposed fix
- * Copyright (c) 2018, WSO2 Inc. (http://www.wso2.org) All Rights Reserved. + * Copyright (c) 2018-2026, WSO2 Inc. (http://www.wso2.org) All Rights Reserved.As per coding guidelines, "
**/*.java: Ensure that all Java files contain the appropriate license header at the top with copyright year as the current year or a range ending in the current year (e.g., 2018-2026)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/application-mgt/org.wso2.carbon.identity.application.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/defaultsequence/DefaultAuthSeqMgtServiceImpl.java` at line 2, The Java file DefaultAuthSeqMgtServiceImpl.java has an outdated copyright header year; update the header at the top of the file to use a range ending in the current year (change "2018" to "2018-2026"), ensuring the license header format remains identical otherwise and appears before the package/imports declarations.components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.java (1)
2-2:⚠️ Potential issue | 🟡 MinorUpdate the Java license header year range to end in 2026.
The current header uses only
2014; repository rules require current year or range ending in 2026.As per coding guidelines: "
**/*.java: Ensure that all Java files contain the appropriate license header ... with copyright year as the current year or a range ending in the current year."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.java` at line 2, Update the Java file header in InMemoryIdentityDataStore.java to use the required copyright year range ending in 2026 (e.g., "2014-2026" or just "2026"); locate the existing header comment at the top of the file and replace the solitary "2014" with the correct year or range so it matches repository licensing rules (ensure the rest of the header text remains unchanged).components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationResultCache.java (1)
1-17:⚠️ Potential issue | 🟡 MinorRefresh the license header.
This file still uses the old 2014 header template. Please align it with the current repository Apache 2.0 header and year range.
As per coding guidelines, "Ensure that all Java files contain the appropriate license header at the top with copyright year as the current year or a range ending in the current year (e.g., 2018-2026)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationResultCache.java` around lines 1 - 17, The file AuthenticationResultCache.java still has the old 2014 Apache header; update the top-of-file license block to the repository's current Apache 2.0 header and change the copyright year to a current-year or year range ending in 2026 (e.g., 2014-2026 or 2018-2026) so the header conforms to project guidelines; retain the same Apache 2.0 text and formatting used across the repo and replace only the header block at the very top of the file.components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationContextCache.java (1)
1-17:⚠️ Potential issue | 🟡 MinorRefresh the license header.
This file still ends its copyright range at 2023 even though it is modified in this PR.
As per coding guidelines, "Ensure that all Java files contain the appropriate license header at the top with copyright year as the current year or a range ending in the current year (e.g., 2018-2026)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationContextCache.java` around lines 1 - 17, Update the file-level license header to use the current copyright year range by changing the end year in the header (currently "2015-2023") to the current year (e.g., "2015-2026"); locate the top-of-file header in AuthenticationContextCache (class name AuthenticationContextCache) and replace the year range accordingly so the license block matches project guidelines.
♻️ Duplicate comments (6)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/inbound/IdentityContextCache.java (1)
66-75:⚠️ Potential issue | 🟠 MajorPreserve
addToCacheOnReadif-absent semantics for session-store writes.On Line 74,
storeSessionData(...)runs unconditionally, so this path can still overwrite existing request-scope data even thoughsuper.addToCacheOnRead(...)is an on-read/add-if-absent operation.💡 Suggested fix
public void addToCacheOnRead(String key, IdentityMessageContext context) { super.addToCacheOnRead(key, context); if (enableRequestScopeCache) { int tenantId = MultitenantConstants.INVALID_TENANT_ID; String tenantDomain = context.getRequest().getTenantDomain(); if (tenantDomain != null) { tenantId = IdentityTenantUtil.getTenantId(tenantDomain); } - SessionDataStore.getInstance().storeSessionData(key, INBOUND_CONTEXT_CACHE_NAME, context, tenantId); + if (SessionDataStore.getInstance().getSessionData(key, INBOUND_CONTEXT_CACHE_NAME) == null) { + SessionDataStore.getInstance().storeSessionData(key, INBOUND_CONTEXT_CACHE_NAME, context, tenantId); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/inbound/IdentityContextCache.java` around lines 66 - 75, The addToCacheOnRead path currently always calls SessionDataStore.getInstance().storeSessionData(...) which can overwrite existing request-scoped entries; modify addToCacheOnRead so it preserves the original "add-if-absent" semantics used by super.addToCacheOnRead by first checking whether session data already exists for the given key/INBOUND_CONTEXT_CACHE_NAME/tenantId (e.g. via SessionDataStore.getInstance().getSessionData(...) or a contains/get method) and only call storeSessionData(key, INBOUND_CONTEXT_CACHE_NAME, context, tenantId) when no existing entry is present.components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/UserStoreBasedIdentityDataStore.java (3)
2-2:⚠️ Potential issue | 🟡 MinorUpdate the Java license header year range to end in 2026.
The current header uses
2014only; update to a current-valid range (e.g.,2014-2026).As per coding guidelines: "
**/*.java: Ensure that all Java files contain the appropriate license header ... with copyright year as the current year or a range ending in the current year."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/UserStoreBasedIdentityDataStore.java` at line 2, Update the Java file header in UserStoreBasedIdentityDataStore (the top-of-file license comment) to use the current year range by changing the copyright year from "2014" to "2014-2026"; locate the header comment at the top of org.wso2.carbon.identity.mgt.store.UserStoreBasedIdentityDataStore.java and replace the single-year copyright with the range so it matches project guidelines.
122-136:⚠️ Potential issue | 🟠 MajorAdd a null guard for
userIdentityDTObefore dereference, and use WARN for null username.Line 122 dereferences
userIdentityDTObefore any null check (NPE risk). Also Line 135 logs a client/input validation condition as ERROR.Proposed fix
+ if (userIdentityDTO == null) { + log.warn("User identity data object is null. Skipping persistence on read."); + return; + } UserIdentityClaimsDO newIdentityClaimDO = new UserIdentityClaimsDO(userIdentityDTO.getUserName(), userIdentityDTO.getUserDataMap()); @@ - if (userIdentityDTO.getUserName() == null) { - log.error("Error while persisting user data. Null user name is provided."); + if (userIdentityDTO.getUserName() == null) { + log.warn("Skipping persistence on read. Null user name is provided."); return; }As per coding guidelines: "Do not log client errors as ERROR (use WARN or DEBUG instead)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/UserStoreBasedIdentityDataStore.java` around lines 122 - 136, The code dereferences userIdentityDTO when constructing UserIdentityClaimsDO and calling getUserName()/getUserDataMap(), risking NPE; add an early null guard that checks userIdentityDTO before any use and return (or throw) appropriately, moving the creation of UserIdentityClaimsDO until after that null check, and replace the log.error for a null username with log.warn to reflect client/input validation; update references in this method (userIdentityDTO, UserIdentityClaimsDO, userStoreManager, storeOnRead) accordingly so tenantId logic and storeOnRead only execute when userIdentityDTO is non-null.
154-155:⚠️ Potential issue | 🟠 MajorDefensive-null-check
e.getMessage()before callingstartsWith().Line 154 can throw
NullPointerExceptionif the exception message is null.Proposed fix
- if(!e.getMessage().startsWith(IdentityCoreConstants.USER_NOT_FOUND)){ + String errorMessage = e.getMessage(); + if (errorMessage == null || !errorMessage.startsWith(IdentityCoreConstants.USER_NOT_FOUND)) { throw IdentityException.error("Error while persisting identity user data in to user store", e); } else if (log.isDebugEnabled()){🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/UserStoreBasedIdentityDataStore.java` around lines 154 - 155, The code in UserStoreBasedIdentityDataStore currently calls e.getMessage().startsWith(...), which can NPE if getMessage() is null; update the conditional in the catch block that references e.getMessage() so it first checks for null (e.g., e.getMessage() != null && e.getMessage().startsWith(IdentityCoreConstants.USER_NOT_FOUND)) before deciding not to rethrow, or alternatively convert the message to a non-null string with Objects.toString(e.getMessage(), "") and then call startsWith; keep the existing throw IdentityException.error("Error while persisting identity user data in to user store", e) behavior unchanged.components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.java (2)
141-145:⚠️ Potential issue | 🔴 CriticalGuard the cast before using
org.wso2.carbon.user.core.UserStoreManager.Line 141 performs an unconditional cast from interface type
UserStoreManager; this can throwClassCastExceptionfor non-core implementations.Proposed fix
- org.wso2.carbon.user.core.UserStoreManager store = (org.wso2.carbon.user.core.UserStoreManager) userStoreManager; + if (!(userStoreManager instanceof org.wso2.carbon.user.core.UserStoreManager)) { + if (log.isDebugEnabled()) { + log.debug("Skipping cache write for unsupported user store manager type: " + + userStoreManager.getClass().getName()); + } + return; + } + org.wso2.carbon.user.core.UserStoreManager store = + (org.wso2.carbon.user.core.UserStoreManager) userStoreManager; String domainName = store.getRealmConfiguration().getUserStoreProperty(UserCoreConstants.RealmConfig.PROPERTY_DOMAIN_NAME);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.java` around lines 141 - 145, The code in InMemoryIdentityDataStore unconditionally casts userStoreManager to org.wso2.carbon.user.core.UserStoreManager to read the domain name and build the key, which can throw ClassCastException for non-core implementations; update the block around the cast (the use of userStoreManager and the temporary store variable) to first check "if (userStoreManager instanceof org.wso2.carbon.user.core.UserStoreManager)" and only then cast and call getRealmConfiguration().getUserStoreProperty(...); for the else branch derive a safe domainName (e.g., use userStoreManager.getDomainName() if available or a default/empty value) so key construction (domainName + userStoreManager.getTenantId() + userName) always uses a valid domainName without risking a ClassCastException.
127-139:⚠️ Potential issue | 🟠 MajorAvoid logging raw claim values in debug output.
Line 138 logs the full claims payload (
key=valuepairs), which can expose identity/PII data. Log only safe metadata (e.g., username + claim count or keys only).Proposed fix
- if (log.isDebugEnabled()) { - StringBuilder data = new StringBuilder("{"); - if (userIdentityDTO.getUserIdentityDataMap() != null) { - for (Map.Entry<String, String> entry : userIdentityDTO.getUserIdentityDataMap().entrySet()) { - data.append("[").append(entry.getKey()).append(" = ").append(entry.getValue()).append("], "); - } - } - if (data.indexOf(",") >= 0) { - data.deleteCharAt(data.lastIndexOf(",")); - } - data.append("}"); - log.debug("Storing UserIdentityClaimsDO to cache for user: " + userName + " with claims: " + data); - } + if (log.isDebugEnabled()) { + int claimCount = userIdentityDTO.getUserIdentityDataMap() == null + ? 0 : userIdentityDTO.getUserIdentityDataMap().size(); + log.debug("Storing UserIdentityClaimsDO to cache for user: " + userName + + " with claim count: " + claimCount); + }As per coding guidelines: "Do not log sensitive information including emails, passwords, tokens, secrets, or personally identifiable information; log only safe identifiers that are neither sensitive nor PII if necessary."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.java` around lines 127 - 139, The debug logging currently builds and emits full claim key=value pairs (in the block inside InMemoryIdentityDataStore where userIdentityDTO is inspected and 'data' is built), which can expose PII; change the log to avoid values by only emitting safe metadata such as userName plus either the number of claims or a comma-separated list of claim keys (use userIdentityDTO.getUserIdentityDataMap().keySet()) and do not append entry.getValue() anywhere; update the log.debug call to reference the new safe string (and remove the data StringBuilder usage that includes values).
🧹 Nitpick comments (10)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationErrorCache.java (2)
96-98: Remove redundant null check.The inner null check on
tenantDomainis redundant since it's assigned fromentry.getTenantDomain()which was already verified as non-null in the outer condition.Suggested fix
if (isTemporarySessionDataPersistEnabled) { int tenantId = MultitenantConstants.INVALID_TENANT_ID; - if (entry.getTenantDomain() != null) { - String tenantDomain = entry.getTenantDomain(); - if (tenantDomain != null) { - tenantId = IdentityTenantUtil.getTenantId(tenantDomain); - } + String tenantDomain = entry.getTenantDomain(); + if (tenantDomain != null) { + tenantId = IdentityTenantUtil.getTenantId(tenantDomain); } SessionDataStore.getInstance().storeSessionData(key.getErrorKey(), CACHE_NAME, entry, tenantId); }Note: The same pattern exists in
addToCache()(lines 71-75), which could be simplified consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationErrorCache.java` around lines 96 - 98, In AuthenticationErrorCache, remove the redundant inner null check on tenantDomain: in the method that reads entry.getTenantDomain() (the shown block and the similar pattern in addToCache()), replace the if (entry.getTenantDomain() != null) { String tenantDomain = entry.getTenantDomain(); if (tenantDomain != null) { ... } } with a single check that assigns once (e.g., String tenantDomain = entry.getTenantDomain(); if (tenantDomain != null) { ... }) so you don't call getTenantDomain() twice or repeat the null check; apply the same simplification inside addToCache().
91-91: Add@Overrideannotation.This method overrides
addToCacheOnReadfromAuthenticationBaseCache. Adding the annotation improves clarity and enables compile-time verification.Suggested fix
+ `@Override` public void addToCacheOnRead(AuthenticationErrorCacheKey key, AuthenticationErrorCacheEntry entry) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationErrorCache.java` at line 91, The method addToCacheOnRead in AuthenticationErrorCache overrides the same method from AuthenticationBaseCache but is missing the `@Override` annotation; add `@Override` directly above the public void addToCacheOnRead(AuthenticationErrorCacheKey key, AuthenticationErrorCacheEntry entry) method declaration in the AuthenticationErrorCache class to make the override explicit and enable compile-time checking.components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationRequestCache.java (1)
1-2: Update copyright year to include 2026.The copyright header should include the current year when modifying an existing file. As per coding guidelines, the copyright year should be the current year or a range ending in the current year.
Suggested fix
-* Copyright (c) 2014, WSO2 Inc. (http://www.wso2.org) All Rights Reserved. +* Copyright (c) 2014-2026, WSO2 Inc. (http://www.wso2.org) All Rights Reserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationRequestCache.java` around lines 1 - 2, Update the file header to include the current year 2026 in the copyright notice for the class AuthenticationRequestCache (the file org.wso2.carbon.identity.application.authentication.framework.cache.AuthenticationRequestCache.java); locate the top-of-file comment block and change the year or year range so it ends with 2026 (e.g., "2014-2026" or add ", 2026") to reflect the current copyright year.components/cors-mgt/org.wso2.carbon.identity.cors.mgt.core/src/main/java/org/wso2/carbon/identity/cors/mgt/core/dao/impl/CacheBackedCORSConfigurationDAO.java (1)
100-116: Consolidate duplicated cache-write helper logic.
addCORSConfigurationToCacheOnRead(...)duplicates key/entry/logging code already present inaddCORSConfigurationToCache(...). Consider a single helper to avoid future divergence.♻️ Proposed consolidation
-private void addCORSConfigurationToCache(CORSConfiguration corsConfiguration, String tenantDomain) { +private void addCORSConfigurationToCache(CORSConfiguration corsConfiguration, String tenantDomain, + boolean onRead) { CORSConfigurationCacheKey cacheKey = new CORSConfigurationCacheKey(tenantDomain); CORSConfigurationCacheEntry cacheEntry = new CORSConfigurationCacheEntry(corsConfiguration); if (log.isDebugEnabled()) { log.debug("Adding CORS configuration to Cache with Key: " + tenantDomain); } - CORSConfigurationCache.getInstance().addToCache(cacheKey, cacheEntry, tenantDomain); -} - -private void addCORSConfigurationToCacheOnRead(CORSConfiguration corsConfiguration, String tenantDomain) { - CORSConfigurationCacheKey cacheKey = new CORSConfigurationCacheKey(tenantDomain); - CORSConfigurationCacheEntry cacheEntry = new CORSConfigurationCacheEntry(corsConfiguration); - - if (log.isDebugEnabled()) { - log.debug("Adding CORS configuration to Cache with Key: " + tenantDomain); + if (onRead) { + CORSConfigurationCache.getInstance().addToCacheOnRead(cacheKey, cacheEntry, tenantDomain); + } else { + CORSConfigurationCache.getInstance().addToCache(cacheKey, cacheEntry, tenantDomain); } - - CORSConfigurationCache.getInstance().addToCacheOnRead(cacheKey, cacheEntry, tenantDomain); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/cors-mgt/org.wso2.carbon.identity.cors.mgt.core/src/main/java/org/wso2/carbon/identity/cors/mgt/core/dao/impl/CacheBackedCORSConfigurationDAO.java` around lines 100 - 116, Both addCORSConfigurationToCacheOnRead(...) and addCORSConfigurationToCache(...) duplicate the logic to build CORSConfigurationCacheKey/CORSConfigurationCacheEntry and logging; refactor by extracting a single helper (e.g., buildAndPutCORSConfigurationToCache or putCORSConfigurationToCache) that accepts the CORSConfiguration, tenantDomain and a flag or strategy for onRead vs normal add, reuse CORSConfigurationCacheKey and CORSConfigurationCacheEntry creation and the debug log there, and call CORSConfigurationCache.getInstance().addToCacheOnRead(...) or addToCache(...) from that helper to replace both existing methods.components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/inbound/IdentityContextCache.java (1)
66-66: Add@Overrideannotation to compile-time verify override relationship.The method
addToCacheOnRead(String, IdentityMessageContext)on line 66 overrides the parent method but is missing the@Overrideannotation. While not strictly required, this annotation provides compile-time safety and clarifies intent.💡 Suggested fix
+ `@Override` public void addToCacheOnRead(String key, IdentityMessageContext context) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/inbound/IdentityContextCache.java` at line 66, The method addToCacheOnRead(String, IdentityMessageContext) in class IdentityContextCache overrides a superclass method but is missing the `@Override` annotation; modify the method signature in IdentityContextCache to add the `@Override` annotation immediately above the addToCacheOnRead(String, IdentityMessageContext) declaration to ensure compile-time verification of the override relationship.components/application-mgt/org.wso2.carbon.identity.application.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/defaultsequence/DefaultAuthSeqMgtServiceImpl.java (1)
326-335: Consider consolidating duplicated cache-write helper logic.
addDefaultAuthSeqToCache(...)andaddDefaultAuthSeqToCacheOnRead(...)are nearly identical except the cache API call. A shared private helper would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/application-mgt/org.wso2.carbon.identity.application.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/defaultsequence/DefaultAuthSeqMgtServiceImpl.java` around lines 326 - 335, Consolidate the duplicated cache-write logic in addDefaultAuthSeqToCache(...) and addDefaultAuthSeqToCacheOnRead(...) by creating a single private helper (e.g., private void putDefaultAuthSeqInCache(DefaultAuthenticationSequence sequence, String tenantDomain, boolean onRead) or a helper that accepts a BiConsumer<String,DefaultAuthSeqMgtCacheEntry> to perform the actual cache call); inside the helper construct the DefaultAuthSeqMgtCacheEntry and call either DefaultAuthSeqMgtCache.getInstance().addToCache(...) or .addToCacheOnRead(...) based on the flag or passed-in consumer, keep the DefaultAuthSeqMgtCache.getInstance().isEnabled() check and the existing debug log message, and then have both addDefaultAuthSeqToCache and addDefaultAuthSeqToCacheOnRead delegate to this new helper.components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/dao/impl/CachedBackedConfigurationDAO.java (1)
352-352: Drop the unused checked exception fromaddResourceToCacheOnRead.
ConfigurationManagementExceptionis declared but not thrown by the method body.🧹 Suggested signature cleanup
- private void addResourceToCacheOnRead(Resource resource) throws ConfigurationManagementException { + private void addResourceToCacheOnRead(Resource resource) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/dao/impl/CachedBackedConfigurationDAO.java` at line 352, The method addResourceToCacheOnRead currently declares a checked exception (ConfigurationManagementException) that it never throws; remove the throws declaration from addResourceToCacheOnRead's signature, update any callers or references that expect a checked exception (remove redundant try/catch or propagated throws in callers such as methods invoking addResourceToCacheOnRead), and run compilation/tests to ensure no remaining references to the removed throws remain (adjust signatures of calling methods only if they were solely declaring the exception because of this call).components/secret-mgt/org.wso2.carbon.identity.secret.mgt.core/src/main/java/org/wso2/carbon/identity/secret/mgt/core/dao/impl/CachedBackedSecretDAO.java (1)
231-247: Consider deduplicating cache-entry construction between write/read helpers.
addSecretToCacheandaddSecretToCacheOnReadrepeat the same key/entry assembly and logging. Extracting a common helper would reduce drift and maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/secret-mgt/org.wso2.carbon.identity.secret.mgt.core/src/main/java/org/wso2/carbon/identity/secret/mgt/core/dao/impl/CachedBackedSecretDAO.java` around lines 231 - 247, The addSecretToCache and addSecretToCacheOnRead methods in CachedBackedSecretDAO duplicate key/entry creation and logging; extract a private helper (e.g., buildAndLogCacheEntry or createCacheEntryForSecret) that takes a Secret and returns a small struct/object (or tuple) containing SecretByIdCacheKey, SecretByNameCacheKey and SecretCacheEntry and performs the shared debug log using secret.getSecretName(), secret.getSecretId(), and secret.getTenantDomain(); then replace the duplicated code in both addSecretToCache and addSecretToCacheOnRead to call this helper and use its returned keys/entry when invoking secretByIdCache.addToCache... and secretByNameCache.addToCache....components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.java (1)
106-156: Extract shared write-path logic betweenstoreandstoreOnRead.
storeOnReadduplicates most ofstore(tenant flow, username normalization, debug preparation, key building). This duplication is already driving complexity and increases drift risk. Prefer a shared helper that accepts a cache write strategy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.java` around lines 106 - 156, Extract the duplicated write-path in store and storeOnRead into a private helper (e.g., writeToCache(UserIdentityClaimsDO userIdentityDTO, UserStoreManager userStoreManager, BiConsumer<Cache<String, UserIdentityClaimsDO>, MapEntry> writeStrategy) or a small functional interface) that encapsulates starting/ending tenant flow, username normalization (the block using IdentityUtil.isUserStoreCaseSensitive and userName.toLowerCase), debug string assembly, domainName/tenantId/userName key construction, and obtaining the Cache via getCache(); then call this helper from both store(...) and storeOnRead(...), passing a strategy that calls cache.put(...) for store and cache.putOnRead(...) for storeOnRead; preserve the existing UserStoreException catch and finally PrivilegedCarbonContext.endTenantFlow behavior inside the helper so both call sites remain thin and consistent.components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/UserStoreBasedIdentityDataStore.java (1)
116-172: RefactorstoreOnReadto reuse shared persistence logic fromstore.This method repeats nearly all of
store(...)with only one delegation difference (super.storeOnRead). Extract a common private helper to reduce complexity and keep behavior aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/UserStoreBasedIdentityDataStore.java` around lines 116 - 172, storeOnRead duplicates most logic from store(...) except it calls super.storeOnRead; extract a private helper (e.g., persistIdentityData(UserIdentityClaimsDO dto, UserStoreManager manager, boolean callSuperStoreOnRead)) that encapsulates tenantId lookup, new UserIdentityClaimsDO creation, setting tenantId, calling super.storeOnRead or super.store as appropriate, null username check, domain stripping, read-only check, and the setUserClaimsValuesInUserStore call and exception handling; then refactor both storeOnRead and store to call this helper (pass true for callSuperStoreOnRead in storeOnRead and false in store) so shared persistence logic is centralized and behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationContextCache.java`:
- Around line 151-195: super.addToCacheOnRead(key, entry) can be a no-op if
another thread won the race, so before calling
SessionDataStore.storeSessionData(...) verify that the cache actually contains
the same/most-recent entry you just attempted to insert; in
AuthenticationContextCache.addToCacheOnRead after the super call, read the
cached value for key (e.g., via the cache's get/getValueFromCache method or
equivalent) and compare its context identifier or object equality to entry
(null-checks included); only proceed to optimize and call
SessionDataStore.getInstance().storeSessionData(...) when the cached entry
matches the one we intended to store, otherwise return to avoid persisting a
stale context that lost the race.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationRequestCache.java`:
- Line 98: The method addToCacheOnRead in class AuthenticationRequestCache
overrides AuthenticationBaseCache.addToCacheOnRead but lacks the `@Override`
annotation; add the `@Override` annotation immediately above the
addToCacheOnRead(AuthenticationRequestCacheKey key,
AuthenticationRequestCacheEntry entry) method declaration in
AuthenticationRequestCache to match other subclasses (e.g., LongWaitResultCache)
and improve compile-time checking.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationResultCache.java`:
- Around line 102-113: The addToCacheOnRead method currently writes the entry
back to the backing store (SessionDataStore.storeSessionData) on every read when
isTemporarySessionDataPersistEnabled is true, causing unnecessary writes; change
addToCacheOnRead in AuthenticationResultCache to stop calling
SessionDataStore.getInstance().storeSessionData(...) during read-fill: remove or
guard the storeSessionData call so only mutation/initial-write paths invoke it
(keep the tenant-id extraction logic if needed elsewhere, but do not perform
storeSessionData in addToCacheOnRead), letting the existing read-from-backend
logic (the reload path) remain the single place that writes to the backing
store.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/SessionContextCache.java`:
- Around line 120-154: getSessionFromDB still repopulates the cache using
super.addToCache(...), which triggers cluster-wide invalidations and defeats the
read-safe insert intended by addToCacheOnRead; change the repopulation path in
getSessionFromDB to call addToCacheOnRead(SessionContextCacheKey key,
SessionContextCacheEntry entry, String loginTenantDomain) (passing the same key,
entry and resolved loginTenantDomain) instead of calling super.addToCache(...)
so the read-optimized insert (including entry.setAccessedTime(), optimization
and proper SessionDataStore writes) is used for cache-miss reloads.
In
`@components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/dao/impl/CachedBackedConfigurationDAO.java`:
- Around line 363-365: The repeated cache-log message in
CachedBackedConfigurationDAO should be extracted into a single constant: add a
private static final String (e.g. CACHE_LOG_TEMPLATE) in the
CachedBackedConfigurationDAO class containing the format string
"[AddToCacheOnRead]Following two cache entries created. 1. Resource by name
cache %s, 2. Resource by id cache %s. Tenant domain for all caches: %s" and
replace the inline literal in the method that builds 'message' (and any other
occurrences) to use String.format(CACHE_LOG_TEMPLATE,
resource.getResourceName(), resource.getResourceId(),
resource.getTenantDomain()).
In
`@components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/cache/BaseCache.java`:
- Around line 212-223: The Javadoc for addToCacheOnRead(K key, V entry, int
tenantId) incorrectly documents a parameter named tenantDomain; update the
docblock to reference tenantId instead and adjust its description to mention
tenant id (int) rather than tenant domain; ensure the `@param` tags match the
method signature (key, entry, tenantId) and the overall summary still explains
the behavior of addToCacheOnRead.
In
`@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.java`:
- Line 148: The code in InMemoryIdentityDataStore uses cache.putOnRead(key,
userIdentityDTO) on a variable declared as javax.cache.Cache<String,
UserIdentityClaimsDO>, but javax.cache.Cache does not define putOnRead and this
will cause a runtime error; replace the call with cache.put(key,
userIdentityDTO) (or, if you intentionally need a non-standard implementation,
cast the cache to that specific implementation type before calling its custom
putOnRead method) to resolve the issue.
In
`@components/secret-mgt/org.wso2.carbon.identity.secret.mgt.core/src/main/java/org/wso2/carbon/identity/secret/mgt/core/dao/impl/CachedBackedSecretDAO.java`:
- Around line 239-244: The debug log in CachedBackedSecretDAO currently prints
sensitive data (secret.getSecretName(), secret.getSecretId(),
secret.getTenantDomain()); remove those direct values and replace the message
with a non-sensitive statement such as "Created two cache entries for secret
(identifier omitted)" or log only safe context (e.g., cache names and count) —
if you must correlate, emit a non-reversible hash/fingerprint of
secret.getSecretId() instead of the raw id; update the log.debug call
accordingly and ensure any formatted message no longer includes
secret.getSecretName(), secret.getSecretId(), or tenant domain.
In
`@components/system-config-mgt/org.wso2.carbon.identity.system.config.mgt/src/main/java/org/wso2/carbon/identity/system/config/mgt/advisory/DBBasedAdminBannerDAO.java`:
- Around line 78-80: The code uses
advisoryBannerCache.addToCacheOnRead(CACHE_KEY, valueToCache, tenantDomain) in
DBBasedAdminBannerDAO which is a read-safe put-if-absent and can leave stale
values after a save; replace this call with a write-through cache update that
overwrites existing entries (use the cache API method that performs an
unconditional put/update rather than addToCacheOnRead) so CACHE_KEY is updated
with Pair.of(adminAdvisoryBannerDTO.getEnableBanner(),
adminAdvisoryBannerDTO.getBannerContent()) for the tenant when saving; ensure
you modify the call in DBBasedAdminBannerDAO (where addToCacheOnRead is invoked)
to use the cache method that guarantees overwrite.
---
Outside diff comments:
In
`@components/application-mgt/org.wso2.carbon.identity.application.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/defaultsequence/DefaultAuthSeqMgtServiceImpl.java`:
- Line 2: The Java file DefaultAuthSeqMgtServiceImpl.java has an outdated
copyright header year; update the header at the top of the file to use a range
ending in the current year (change "2018" to "2018-2026"), ensuring the license
header format remains identical otherwise and appears before the package/imports
declarations.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationContextCache.java`:
- Around line 1-17: Update the file-level license header to use the current
copyright year range by changing the end year in the header (currently
"2015-2023") to the current year (e.g., "2015-2026"); locate the top-of-file
header in AuthenticationContextCache (class name AuthenticationContextCache) and
replace the year range accordingly so the license block matches project
guidelines.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationErrorCache.java`:
- Around line 1-2: Update the file header in AuthenticationErrorCache
(class/file: AuthenticationErrorCache.java) to use the current copyright year or
a year-range ending in the current year (e.g., "2022-2026" or "2026") instead of
the standalone "2022"; modify the top-of-file comment block (license header)
accordingly so the copyright span reflects the current year per project
guidelines.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationResultCache.java`:
- Around line 1-17: The file AuthenticationResultCache.java still has the old
2014 Apache header; update the top-of-file license block to the repository's
current Apache 2.0 header and change the copyright year to a current-year or
year range ending in 2026 (e.g., 2014-2026 or 2018-2026) so the header conforms
to project guidelines; retain the same Apache 2.0 text and formatting used
across the repo and replace only the header block at the very top of the file.
In
`@components/cors-mgt/org.wso2.carbon.identity.cors.mgt.core/src/main/java/org/wso2/carbon/identity/cors/mgt/core/dao/impl/CacheBackedCORSConfigurationDAO.java`:
- Line 2: Update the license header year range in the top-of-file comment so it
ends in 2026 (e.g., change "2021" to "2018-2026" or "2021-2026" as appropriate);
edit the header in CacheBackedCORSConfigurationDAO (class
CacheBackedCORSConfigurationDAO) to reflect the current year/range and keep the
rest of the licence text unchanged.
In
`@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.java`:
- Line 2: Update the Java file header in InMemoryIdentityDataStore.java to use
the required copyright year range ending in 2026 (e.g., "2014-2026" or just
"2026"); locate the existing header comment at the top of the file and replace
the solitary "2014" with the correct year or range so it matches repository
licensing rules (ensure the rest of the header text remains unchanged).
In
`@components/system-config-mgt/org.wso2.carbon.identity.system.config.mgt/src/main/java/org/wso2/carbon/identity/system/config/mgt/advisory/DBBasedAdminBannerDAO.java`:
- Line 2: Update the license header year range in the top of
DBBasedAdminBannerDAO.java so it ends in 2026 instead of 2025; locate the file
by the class/file name DBBasedAdminBannerDAO and change the copyright line that
currently reads "2025" (or "2018-2025") to the correct current year or range
ending in "2026".
---
Duplicate comments:
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/inbound/IdentityContextCache.java`:
- Around line 66-75: The addToCacheOnRead path currently always calls
SessionDataStore.getInstance().storeSessionData(...) which can overwrite
existing request-scoped entries; modify addToCacheOnRead so it preserves the
original "add-if-absent" semantics used by super.addToCacheOnRead by first
checking whether session data already exists for the given
key/INBOUND_CONTEXT_CACHE_NAME/tenantId (e.g. via
SessionDataStore.getInstance().getSessionData(...) or a contains/get method) and
only call storeSessionData(key, INBOUND_CONTEXT_CACHE_NAME, context, tenantId)
when no existing entry is present.
In
`@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.java`:
- Around line 141-145: The code in InMemoryIdentityDataStore unconditionally
casts userStoreManager to org.wso2.carbon.user.core.UserStoreManager to read the
domain name and build the key, which can throw ClassCastException for non-core
implementations; update the block around the cast (the use of userStoreManager
and the temporary store variable) to first check "if (userStoreManager
instanceof org.wso2.carbon.user.core.UserStoreManager)" and only then cast and
call getRealmConfiguration().getUserStoreProperty(...); for the else branch
derive a safe domainName (e.g., use userStoreManager.getDomainName() if
available or a default/empty value) so key construction (domainName +
userStoreManager.getTenantId() + userName) always uses a valid domainName
without risking a ClassCastException.
- Around line 127-139: The debug logging currently builds and emits full claim
key=value pairs (in the block inside InMemoryIdentityDataStore where
userIdentityDTO is inspected and 'data' is built), which can expose PII; change
the log to avoid values by only emitting safe metadata such as userName plus
either the number of claims or a comma-separated list of claim keys (use
userIdentityDTO.getUserIdentityDataMap().keySet()) and do not append
entry.getValue() anywhere; update the log.debug call to reference the new safe
string (and remove the data StringBuilder usage that includes values).
In
`@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/UserStoreBasedIdentityDataStore.java`:
- Line 2: Update the Java file header in UserStoreBasedIdentityDataStore (the
top-of-file license comment) to use the current year range by changing the
copyright year from "2014" to "2014-2026"; locate the header comment at the top
of org.wso2.carbon.identity.mgt.store.UserStoreBasedIdentityDataStore.java and
replace the single-year copyright with the range so it matches project
guidelines.
- Around line 122-136: The code dereferences userIdentityDTO when constructing
UserIdentityClaimsDO and calling getUserName()/getUserDataMap(), risking NPE;
add an early null guard that checks userIdentityDTO before any use and return
(or throw) appropriately, moving the creation of UserIdentityClaimsDO until
after that null check, and replace the log.error for a null username with
log.warn to reflect client/input validation; update references in this method
(userIdentityDTO, UserIdentityClaimsDO, userStoreManager, storeOnRead)
accordingly so tenantId logic and storeOnRead only execute when userIdentityDTO
is non-null.
- Around line 154-155: The code in UserStoreBasedIdentityDataStore currently
calls e.getMessage().startsWith(...), which can NPE if getMessage() is null;
update the conditional in the catch block that references e.getMessage() so it
first checks for null (e.g., e.getMessage() != null &&
e.getMessage().startsWith(IdentityCoreConstants.USER_NOT_FOUND)) before deciding
not to rethrow, or alternatively convert the message to a non-null string with
Objects.toString(e.getMessage(), "") and then call startsWith; keep the existing
throw IdentityException.error("Error while persisting identity user data in to
user store", e) behavior unchanged.
---
Nitpick comments:
In
`@components/application-mgt/org.wso2.carbon.identity.application.mgt/src/main/java/org/wso2/carbon/identity/application/mgt/defaultsequence/DefaultAuthSeqMgtServiceImpl.java`:
- Around line 326-335: Consolidate the duplicated cache-write logic in
addDefaultAuthSeqToCache(...) and addDefaultAuthSeqToCacheOnRead(...) by
creating a single private helper (e.g., private void
putDefaultAuthSeqInCache(DefaultAuthenticationSequence sequence, String
tenantDomain, boolean onRead) or a helper that accepts a
BiConsumer<String,DefaultAuthSeqMgtCacheEntry> to perform the actual cache
call); inside the helper construct the DefaultAuthSeqMgtCacheEntry and call
either DefaultAuthSeqMgtCache.getInstance().addToCache(...) or
.addToCacheOnRead(...) based on the flag or passed-in consumer, keep the
DefaultAuthSeqMgtCache.getInstance().isEnabled() check and the existing debug
log message, and then have both addDefaultAuthSeqToCache and
addDefaultAuthSeqToCacheOnRead delegate to this new helper.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationErrorCache.java`:
- Around line 96-98: In AuthenticationErrorCache, remove the redundant inner
null check on tenantDomain: in the method that reads entry.getTenantDomain()
(the shown block and the similar pattern in addToCache()), replace the if
(entry.getTenantDomain() != null) { String tenantDomain =
entry.getTenantDomain(); if (tenantDomain != null) { ... } } with a single check
that assigns once (e.g., String tenantDomain = entry.getTenantDomain(); if
(tenantDomain != null) { ... }) so you don't call getTenantDomain() twice or
repeat the null check; apply the same simplification inside addToCache().
- Line 91: The method addToCacheOnRead in AuthenticationErrorCache overrides the
same method from AuthenticationBaseCache but is missing the `@Override`
annotation; add `@Override` directly above the public void
addToCacheOnRead(AuthenticationErrorCacheKey key, AuthenticationErrorCacheEntry
entry) method declaration in the AuthenticationErrorCache class to make the
override explicit and enable compile-time checking.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationRequestCache.java`:
- Around line 1-2: Update the file header to include the current year 2026 in
the copyright notice for the class AuthenticationRequestCache (the file
org.wso2.carbon.identity.application.authentication.framework.cache.AuthenticationRequestCache.java);
locate the top-of-file comment block and change the year or year range so it
ends with 2026 (e.g., "2014-2026" or add ", 2026") to reflect the current
copyright year.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/inbound/IdentityContextCache.java`:
- Line 66: The method addToCacheOnRead(String, IdentityMessageContext) in class
IdentityContextCache overrides a superclass method but is missing the `@Override`
annotation; modify the method signature in IdentityContextCache to add the
`@Override` annotation immediately above the addToCacheOnRead(String,
IdentityMessageContext) declaration to ensure compile-time verification of the
override relationship.
In
`@components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/dao/impl/CachedBackedConfigurationDAO.java`:
- Line 352: The method addResourceToCacheOnRead currently declares a checked
exception (ConfigurationManagementException) that it never throws; remove the
throws declaration from addResourceToCacheOnRead's signature, update any callers
or references that expect a checked exception (remove redundant try/catch or
propagated throws in callers such as methods invoking addResourceToCacheOnRead),
and run compilation/tests to ensure no remaining references to the removed
throws remain (adjust signatures of calling methods only if they were solely
declaring the exception because of this call).
In
`@components/cors-mgt/org.wso2.carbon.identity.cors.mgt.core/src/main/java/org/wso2/carbon/identity/cors/mgt/core/dao/impl/CacheBackedCORSConfigurationDAO.java`:
- Around line 100-116: Both addCORSConfigurationToCacheOnRead(...) and
addCORSConfigurationToCache(...) duplicate the logic to build
CORSConfigurationCacheKey/CORSConfigurationCacheEntry and logging; refactor by
extracting a single helper (e.g., buildAndPutCORSConfigurationToCache or
putCORSConfigurationToCache) that accepts the CORSConfiguration, tenantDomain
and a flag or strategy for onRead vs normal add, reuse CORSConfigurationCacheKey
and CORSConfigurationCacheEntry creation and the debug log there, and call
CORSConfigurationCache.getInstance().addToCacheOnRead(...) or addToCache(...)
from that helper to replace both existing methods.
In
`@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.java`:
- Around line 106-156: Extract the duplicated write-path in store and
storeOnRead into a private helper (e.g., writeToCache(UserIdentityClaimsDO
userIdentityDTO, UserStoreManager userStoreManager, BiConsumer<Cache<String,
UserIdentityClaimsDO>, MapEntry> writeStrategy) or a small functional interface)
that encapsulates starting/ending tenant flow, username normalization (the block
using IdentityUtil.isUserStoreCaseSensitive and userName.toLowerCase), debug
string assembly, domainName/tenantId/userName key construction, and obtaining
the Cache via getCache(); then call this helper from both store(...) and
storeOnRead(...), passing a strategy that calls cache.put(...) for store and
cache.putOnRead(...) for storeOnRead; preserve the existing UserStoreException
catch and finally PrivilegedCarbonContext.endTenantFlow behavior inside the
helper so both call sites remain thin and consistent.
In
`@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/UserStoreBasedIdentityDataStore.java`:
- Around line 116-172: storeOnRead duplicates most logic from store(...) except
it calls super.storeOnRead; extract a private helper (e.g.,
persistIdentityData(UserIdentityClaimsDO dto, UserStoreManager manager, boolean
callSuperStoreOnRead)) that encapsulates tenantId lookup, new
UserIdentityClaimsDO creation, setting tenantId, calling super.storeOnRead or
super.store as appropriate, null username check, domain stripping, read-only
check, and the setUserClaimsValuesInUserStore call and exception handling; then
refactor both storeOnRead and store to call this helper (pass true for
callSuperStoreOnRead in storeOnRead and false in store) so shared persistence
logic is centralized and behavior remains identical.
In
`@components/secret-mgt/org.wso2.carbon.identity.secret.mgt.core/src/main/java/org/wso2/carbon/identity/secret/mgt/core/dao/impl/CachedBackedSecretDAO.java`:
- Around line 231-247: The addSecretToCache and addSecretToCacheOnRead methods
in CachedBackedSecretDAO duplicate key/entry creation and logging; extract a
private helper (e.g., buildAndLogCacheEntry or createCacheEntryForSecret) that
takes a Secret and returns a small struct/object (or tuple) containing
SecretByIdCacheKey, SecretByNameCacheKey and SecretCacheEntry and performs the
shared debug log using secret.getSecretName(), secret.getSecretId(), and
secret.getTenantDomain(); then replace the duplicated code in both
addSecretToCache and addSecretToCacheOnRead to call this helper and use its
returned keys/entry when invoking secretByIdCache.addToCache... and
secretByNameCache.addToCache....
Comment on lines
+151
to
+195
| super.addToCacheOnRead(key, entry); | ||
| if (isTemporarySessionDataPersistEnabled) { | ||
| int tenantId = MultitenantConstants.INVALID_TENANT_ID; | ||
| String tenantDomain = entry.getContext().getTenantDomain(); | ||
| if (tenantDomain != null) { | ||
| tenantId = IdentityTenantUtil.getTenantId(tenantDomain); | ||
| } | ||
|
|
||
| if (entry.getContext() != null && entry.getContext().getProperties() != null) { | ||
| Iterator it = entry.getContext().getProperties().entrySet().iterator(); | ||
| while (it.hasNext()) { | ||
| Map.Entry<String, Object> item = (Map.Entry<String, Object>) it.next(); | ||
| if (!(item.getValue() instanceof Serializable)) { | ||
| it.remove(); | ||
| } | ||
| } | ||
| if (log.isDebugEnabled()) { | ||
| String message = "[AddToCacheOnRead][ Context Id : " + key.getContextId() + | ||
| ", Cache type : " + AUTHENTICATION_CONTEXT_CACHE_NAME + | ||
| ", Operation : STORE ]"; | ||
| log.debug("[AddToCacheOnRead]Authentication context is stored with details " + message); | ||
| } | ||
| if (entry.getContext() != null) { | ||
| try { | ||
| AuthenticationContextLoader.getInstance().optimizeAuthenticationContext(entry.getContext()); | ||
| } catch (SessionDataStorageOptimizationClientException e) { | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("[AddToCacheOnRead]Client error occurred while optimizing the Authentication " + | ||
| "context with context id: " + entry.getContext().getContextIdentifier(), e); | ||
| } | ||
| return; | ||
| } catch (SessionDataStorageOptimizationServerException e) { | ||
| log.error("Server error occurred while optimizing the Authentication context with " + | ||
| "context id: " + entry.getContext().getContextIdentifier(), e); | ||
| return; | ||
| } catch (SessionDataStorageOptimizationException e) { | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("[AddToCacheOnRead]Error occurred while optimizing the Authentication " + | ||
| "context with context id: " + entry.getContext().getContextIdentifier(), e); | ||
| } | ||
| return; | ||
| } | ||
| } | ||
| SessionDataStore.getInstance().storeSessionData(key.getContextId(), AUTHENTICATION_CONTEXT_CACHE_NAME, | ||
| entry, tenantId); |
There was a problem hiding this comment.
Don't persist after a skipped on-read insert.
super.addToCacheOnRead(...) is allowed to no-op when another thread has already populated the key. This method still continues into SessionDataStore.storeSessionData(...), so the older entry that lost the race can overwrite a fresher authentication context in the backing store and be served on the next cache miss.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 160-160: Provide the parametrized type for this generic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationContextCache.java`
around lines 151 - 195, super.addToCacheOnRead(key, entry) can be a no-op if
another thread won the race, so before calling
SessionDataStore.storeSessionData(...) verify that the cache actually contains
the same/most-recent entry you just attempted to insert; in
AuthenticationContextCache.addToCacheOnRead after the super call, read the
cached value for key (e.g., via the cache's get/getValueFromCache method or
equivalent) and compare its context identifier or object equality to entry
(null-checks included); only proceed to optimize and call
SessionDataStore.getInstance().storeSessionData(...) when the cached entry
matches the one we intended to store, otherwise return to avoid persisting a
stale context that lost the race.
| * @param key Key which the cache entry is indexed by. | ||
| * @param entry Value to be stored in the cache. | ||
| */ | ||
| public void addToCacheOnRead(AuthenticationRequestCacheKey key, AuthenticationRequestCacheEntry entry) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add @Override annotation.
This method overrides addToCacheOnRead from AuthenticationBaseCache. Adding @Override improves compile-time safety and aligns with other subclasses (e.g., LongWaitResultCache).
Suggested fix
* `@param` key Key which the cache entry is indexed by.
* `@param` entry Value to be stored in the cache.
*/
+ `@Override`
public void addToCacheOnRead(AuthenticationRequestCacheKey key, AuthenticationRequestCacheEntry entry) {📝 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.
Suggested change
| public void addToCacheOnRead(AuthenticationRequestCacheKey key, AuthenticationRequestCacheEntry entry) { | |
| /** | |
| * `@param` key Key which the cache entry is indexed by. | |
| * `@param` entry Value to be stored in the cache. | |
| */ | |
| `@Override` | |
| public void addToCacheOnRead(AuthenticationRequestCacheKey key, AuthenticationRequestCacheEntry entry) { |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 98-98: Add the "@OverRide" annotation above this method signature
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationRequestCache.java`
at line 98, The method addToCacheOnRead in class AuthenticationRequestCache
overrides AuthenticationBaseCache.addToCacheOnRead but lacks the `@Override`
annotation; add the `@Override` annotation immediately above the
addToCacheOnRead(AuthenticationRequestCacheKey key,
AuthenticationRequestCacheEntry entry) method declaration in
AuthenticationRequestCache to match other subclasses (e.g., LongWaitResultCache)
and improve compile-time checking.
Comment on lines
+102
to
+113
| public void addToCacheOnRead(AuthenticationResultCacheKey key, AuthenticationResultCacheEntry entry) { | ||
| super.addToCacheOnRead(key, entry); | ||
| if (isTemporarySessionDataPersistEnabled) { | ||
| int tenantId = MultitenantConstants.INVALID_TENANT_ID; | ||
| if (entry.getResult() != null && entry.getResult().getSubject() != null) { | ||
| String tenantDomain = entry.getResult().getSubject().getTenantDomain(); | ||
| if (tenantDomain != null) { | ||
| tenantId = IdentityTenantUtil.getTenantId(tenantDomain); | ||
| } | ||
| } | ||
| SessionDataStore.getInstance().storeSessionData(key.getResultId(), CACHE_NAME, entry, tenantId); | ||
| } |
There was a problem hiding this comment.
Avoid rewriting the backing store from an on-read fill.
This cache already reloads misses from SessionDataStore on Lines 125-127. Writing the same entry back again here turns each read-side refill into an extra store write even though the entry is not mutated first, which adds avoidable DB contention in the concurrent path.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 102-102: Add the "@OverRide" annotation above this method signature
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/AuthenticationResultCache.java`
around lines 102 - 113, The addToCacheOnRead method currently writes the entry
back to the backing store (SessionDataStore.storeSessionData) on every read when
isTemporarySessionDataPersistEnabled is true, causing unnecessary writes; change
addToCacheOnRead in AuthenticationResultCache to stop calling
SessionDataStore.getInstance().storeSessionData(...) during read-fill: remove or
guard the storeSessionData call so only mutation/initial-write paths invoke it
(keep the tenant-id extraction logic if needed elsewhere, but do not perform
storeSessionData in addToCacheOnRead), letting the existing read-from-backend
logic (the reload path) remain the single place that writes to the backing
store.
Comment on lines
+120
to
+154
| public void addToCacheOnRead(SessionContextCacheKey key, SessionContextCacheEntry entry, String loginTenantDomain) { | ||
|
|
||
| if (log.isDebugEnabled()) { | ||
| log.debug("Adding session context corresponding to the key : " + key.getContextId() + | ||
| " with accessed time " + entry.getAccessedTime() + " and validity time " + entry.getValidityPeriod()); | ||
| } | ||
| entry.setAccessedTime(); | ||
| super.addToCacheOnRead(key, entry, resolveLoginTenantDomain(loginTenantDomain)); | ||
| Object authUser = entry.getContext().getProperty(FrameworkConstants.AUTHENTICATED_USER); | ||
| try { | ||
| entry = SessionContextLoader.getInstance().optimizeSessionContextCacheEntry(entry); | ||
| } catch (SessionDataStorageOptimizationClientException e) { | ||
| if (log.isDebugEnabled()) { | ||
| log.debug(String.format("Client error occurred while optimizing the Session " + | ||
| "context with context id: %s", entry.getContext()), e); | ||
| } | ||
| return; | ||
| } catch (SessionDataStorageOptimizationServerException e) { | ||
| log.error("Server error occurred while optimizing the Session context with " + | ||
| "context id: " + entry.getContext(), e); | ||
| return; | ||
| } catch (SessionDataStorageOptimizationException e) { | ||
| log.debug("Error occurred while optimizing the Session context with " + | ||
| "context id: " + entry.getContext(), e); | ||
| return; | ||
| } | ||
| if (authUser != null && authUser instanceof AuthenticatedUser) { | ||
| String tenantDomain = ((AuthenticatedUser) authUser).getTenantDomain(); | ||
| int tenantId = IdentityTenantUtil.getTenantId(tenantDomain); | ||
| SessionDataStore.getInstance() | ||
| .storeSessionData(key.getContextId(), SESSION_CONTEXT_CACHE_NAME, entry, tenantId); | ||
| } else { | ||
| SessionDataStore.getInstance().storeSessionData(key.getContextId(), SESSION_CONTEXT_CACHE_NAME, entry); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use the read-safe insert in the actual reload path.
addToCacheOnRead(...) is added here, but getSessionFromDB() still repopulates with super.addToCache(...) on Line 236. So the normal session cache-miss flow in this class keeps broadcasting invalidations across the cluster, which defeats the concurrency-safe change for session reloads.
Suggested fix
- // Cache the session immediately after fetching it from the database, although it will be updated later.
- // This ensures that older cached values in other nodes of the cluster are invalidated via a
- // cacheEntryUpdate event.
- super.addToCache(key, cacheEntry, resolveLoginTenantDomain(getLoginTenantDomainFromContext()));
+ addToCacheOnRead(key, cacheEntry, getLoginTenantDomainFromContext());🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 146-146: Remove this unnecessary null check; "instanceof" returns false for nulls.
[warning] 146-146: Replace this instanceof check and cast with 'instanceof AuthenticatedUser authenticateduser'
[warning] 120-120: Add the "@OverRide" annotation above this method signature
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/cache/SessionContextCache.java`
around lines 120 - 154, getSessionFromDB still repopulates the cache using
super.addToCache(...), which triggers cluster-wide invalidations and defeats the
read-safe insert intended by addToCacheOnRead; change the repopulation path in
getSessionFromDB to call addToCacheOnRead(SessionContextCacheKey key,
SessionContextCacheEntry entry, String loginTenantDomain) (passing the same key,
entry and resolved loginTenantDomain) instead of calling super.addToCache(...)
so the read-optimized insert (including entry.setAccessedTime(), optimization
and proper SessionDataStore writes) is used for cache-miss reloads.
Comment on lines
+363
to
+365
| String message = String.format("[AddToCacheOnRead]Following two cache entries created. 1. Resource by name cache %s, 2." + | ||
| " Resource by id cache %s. Tenant domain for all caches: %s", resource.getResourceName(), | ||
| resource.getResourceId(), resource.getTenantDomain()); |
There was a problem hiding this comment.
Extract the repeated cache-log template into a constant.
This literal is duplicated and is already reported as a quality failure; centralizing it avoids drift.
♻️ Suggested constant extraction
+ private static final String RESOURCE_CACHE_ENTRY_LOG_TEMPLATE =
+ "Following two cache entries created. 1. Resource by name cache %s, 2. Resource by id cache %s. " +
+ "Tenant domain for all caches: %s";
...
- String message = String.format("[AddToCacheOnRead]Following two cache entries created. 1. Resource by name cache %s, 2." +
- " Resource by id cache %s. Tenant domain for all caches: %s", resource.getResourceName(),
- resource.getResourceId(), resource.getTenantDomain());
+ String message = String.format(RESOURCE_CACHE_ENTRY_LOG_TEMPLATE, resource.getResourceName(),
+ resource.getResourceId(), resource.getTenantDomain());🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 364-364: Define a constant instead of duplicating this literal " Resource by id cache %s. Tenant domain for all caches: %s" 3 times.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/configuration-mgt/org.wso2.carbon.identity.configuration.mgt.core/src/main/java/org/wso2/carbon/identity/configuration/mgt/core/dao/impl/CachedBackedConfigurationDAO.java`
around lines 363 - 365, The repeated cache-log message in
CachedBackedConfigurationDAO should be extracted into a single constant: add a
private static final String (e.g. CACHE_LOG_TEMPLATE) in the
CachedBackedConfigurationDAO class containing the format string
"[AddToCacheOnRead]Following two cache entries created. 1. Resource by name
cache %s, 2. Resource by id cache %s. Tenant domain for all caches: %s" and
replace the inline literal in the method that builds 'message' (and any other
occurrences) to use String.format(CACHE_LOG_TEMPLATE,
resource.getResourceName(), resource.getResourceId(),
resource.getTenantDomain()).
Comment on lines
+212
to
+223
| /** | ||
| * Add a cache entry. | ||
| * <p> | ||
| * This populates the cache only if the key does not already have a value. | ||
| * If a value already exists, the cache is left unchanged, which avoids | ||
| * unnecessary cache invalidation broadcasts in clustered environments. | ||
| * | ||
| * @param key Key which cache entry is indexed. | ||
| * @param entry Actual object where cache entry is placed. | ||
| * @param tenantDomain The tenant domain where the cache is maintained. | ||
| */ | ||
| public void addToCacheOnRead(K key, V entry, int tenantId) { |
There was a problem hiding this comment.
Fix Javadoc parameter mismatch in tenant-id overload.
The Javadoc still references tenantDomain, but the method takes tenantId. Please align the doc block with the actual signature.
✏️ Suggested doc fix
- /**
- * Add a cache entry.
+ /**
+ * Add a cache entry during a READ operation.
* <p>
* This populates the cache only if the key does not already have a value.
* If a value already exists, the cache is left unchanged, which avoids
* unnecessary cache invalidation broadcasts in clustered environments.
*
* `@param` key Key which cache entry is indexed.
* `@param` entry Actual object where cache entry is placed.
- * `@param` tenantDomain The tenant domain where the cache is maintained.
+ * `@param` tenantId The tenant Id where the cache is maintained.
*/As per coding guidelines, all public methods should have a docstring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/cache/BaseCache.java`
around lines 212 - 223, The Javadoc for addToCacheOnRead(K key, V entry, int
tenantId) incorrectly documents a parameter named tenantDomain; update the
docblock to reference tenantId instead and adjust its description to mention
tenant id (int) rather than tenant domain; ensure the `@param` tags match the
method signature (key, entry, tenantId) and the overall summary still explains
the behavior of addToCacheOnRead.
|
|
||
| Cache<String, UserIdentityClaimsDO> cache = getCache(); | ||
| if (cache != null) { | ||
| cache.putOnRead(key, userIdentityDTO); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Call sites using putOnRead =="
rg -n --type java '\.putOnRead\s*\('
echo
echo "== Candidate javax.cache.Cache declarations in repo =="
fd "Cache.java" --hidden --exclude .git --exec sh -c '
for f in "$@"; do
if rg -n "^\s*package\s+javax\.cache\s*;" "$f" >/dev/null; then
echo "-- $f"
rg -n "interface\s+Cache|putOnRead\s*\(" "$f"
fi
done
' sh {}
echo
echo "== Imports of javax.cache.Cache in identity-mgt store classes =="
rg -n --type java '^import\s+javax\.cache\.Cache;' \
components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/storeRepository: wso2/carbon-identity-framework
Length of output: 1123
🏁 Script executed:
# Read the file around line 148 to understand the context
cat -n components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.java | head -160 | tail -40Repository: wso2/carbon-identity-framework
Length of output: 2215
🏁 Script executed:
# Search for cache variable declaration in InMemoryIdentityDataStore.java
rg -A 5 -B 5 "cache\s*=" components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.javaRepository: wso2/carbon-identity-framework
Length of output: 3239
🏁 Script executed:
# Check if there's a custom Cache interface or wrapper in the codebase
rg -n "interface\s+Cache\s*<|class\s+.*Cache\s*extends.*Cache" --type javaRepository: wso2/carbon-identity-framework
Length of output: 10230
🏁 Script executed:
# Look at the full imports in InMemoryIdentityDataStore.java
head -50 components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.javaRepository: wso2/carbon-identity-framework
Length of output: 2169
🏁 Script executed:
# Look at BaseCache class in identity-core
rg -n "class BaseCache|void putOnRead|putOnRead\s*\(" \
components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/cache/BaseCache.java | head -30Repository: wso2/carbon-identity-framework
Length of output: 256
🏁 Script executed:
# Get the full BaseCache class definition and imports
head -100 components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/cache/BaseCache.javaRepository: wso2/carbon-identity-framework
Length of output: 3854
🏁 Script executed:
# Search for putOnRead method definition in BaseCache
ast-grep --pattern 'void putOnRead($_, $_) { $$$ }' components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/cache/BaseCache.javaRepository: wso2/carbon-identity-framework
Length of output: 56
🏁 Script executed:
# Get the full BaseCache.java file and search for putOnRead definition
wc -l components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/cache/BaseCache.javaRepository: wso2/carbon-identity-framework
Length of output: 197
🏁 Script executed:
# Read the file to find putOnRead method
cat components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/cache/BaseCache.java | grep -n "putOnRead"Repository: wso2/carbon-identity-framework
Length of output: 171
🏁 Script executed:
# Look at the context around line 181 where putOnRead is used
sed -n '170,195p' components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/cache/BaseCache.javaRepository: wso2/carbon-identity-framework
Length of output: 792
🏁 Script executed:
# Check CacheImpl to see if it has putOnRead method
fd "CacheImpl.java" --type f | xargs grep -l "putOnRead" 2>/dev/nullRepository: wso2/carbon-identity-framework
Length of output: 58
🏁 Script executed:
# Search for putOnRead in the entire codebase to understand its origin
rg -n "putOnRead\s*\(" --type java | head -20Repository: wso2/carbon-identity-framework
Length of output: 806
🏁 Script executed:
# Check the wso2.carbon.caching package to see if putOnRead is defined there
fd "caching" --type d | head -5Repository: wso2/carbon-identity-framework
Length of output: 56
🏁 Script executed:
# Look at what getBaseCache returns - check if it's CacheImpl or something else
sed -n '100,130p' components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/cache/BaseCache.javaRepository: wso2/carbon-identity-framework
Length of output: 1811
🏁 Script executed:
# Search for CacheImpl class definition
fd "CacheImpl.java" --type fRepository: wso2/carbon-identity-framework
Length of output: 56
🏁 Script executed:
# Check if putOnRead is defined in CacheImpl
rg -n "class CacheImpl|putOnRead" components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/cache/BaseCache.java | grep -A 5 "CacheImpl"Repository: wso2/carbon-identity-framework
Length of output: 56
🏁 Script executed:
# Search in the codebase for where CacheImpl might be defined
rg -l "class CacheImpl" --type javaRepository: wso2/carbon-identity-framework
Length of output: 56
🏁 Script executed:
# Find where Cache objects are actually built and returned
sed -n '90,150p' components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/cache/BaseCache.javaRepository: wso2/carbon-identity-framework
Length of output: 2650
🏁 Script executed:
# Check what getBaseCache() returns more completely
sed -n '100,160p' components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/cache/BaseCache.javaRepository: wso2/carbon-identity-framework
Length of output: 2603
🏁 Script executed:
# Search for web/external information about putOnRead in Cache implementations
web_search
Search for "javax.cache.Cache putOnRead method" to understand if this is a standard or custom extensionRepository: wso2/carbon-identity-framework
Length of output: 169
🌐 Web query:
"putOnRead" "wso2.carbon.caching" cache method
💡 Result:
No evidence found of a "putOnRead" method in wso2.carbon.caching or related WSO2 Carbon caching components. WSO2 Carbon implements JSR 107 (JCache) standard APIs like Cache.put(key, value), Cache.get(key), Cache.remove(key). The package org.wso2.carbon.caching contains CarbonCachingService for obtaining JCache CachingProvider and CacheManager. No custom "putOnRead" method appears in source code, documentation, or Javadoc examined across multiple searches of GitHub repositories (carbon-caching, carbon-kernel), official docs, and Maven artifacts. Searches for exact phrase "putOnRead" with WSO2 Carbon caching returned no matches. It may be a misremembered method name, custom extension, or from unrelated context.
Citations:
- 1: https://github.com/wso2/carbon-caching
- 2: https://github.com/wso2/carbon-caching/blob/master/component/src/main/java/org/wso2/carbon/caching/internal/CarbonCacheManager.java
- 3: https://github.com/wso2/carbon-kernel/blob/3c6f96d1e69db3a9a5abe3248b7b069fb726808c/core/javax.cache/src/main/java/org/wso2/carbon/caching/impl/clustering/ClusterCacheInvalidationRequest.java
- 4: https://github.com/wso2/carbon-mediation/blob/master/components/mediation-ui/mediators-ui/org.wso2.carbon.mediator.cache.ui/src/main/java/org/wso2/carbon/mediator/cache/ui/CacheMediator.java
- 5: https://javadoc.io/doc/org.wso2.carbon/javax.cache.wso2
- 6: https://wso2docs.atlassian.net/wiki/spaces/ADMIN44x/pages/6684722/Configuring+registry.xml
🏁 Script executed:
# Check the pom.xml or dependencies to see what caching library is used
fd "pom.xml" --type f | xargs grep -l "carbon-caching" 2>/dev/null | head -3Repository: wso2/carbon-identity-framework
Length of output: 56
🏁 Script executed:
# Check what version of carbon-caching is used
rg -n "carbon-caching" --type xml -A 2 | head -20Repository: wso2/carbon-identity-framework
Length of output: 56
🏁 Script executed:
# Search for putOnRead definition anywhere in the repository
rg -n "def putOnRead|void putOnRead|public.*putOnRead" --type javaRepository: wso2/carbon-identity-framework
Length of output: 56
🏁 Script executed:
# Check if there's any custom Cache interface or extension that might define putOnRead
rg -n "interface.*Cache|extends.*Cache" --type java | grep -v "BaseCache\|extends BaseCache" | head -20Repository: wso2/carbon-identity-framework
Length of output: 4496
🏁 Script executed:
# Look at the actual imports in InMemoryIdentityDataStore to see what Cache type is being used
sed -n '1,40p' components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.javaRepository: wso2/carbon-identity-framework
Length of output: 1492
🏁 Script executed:
# Double-check what the actual return type of getCache() is
rg -B 5 -A 10 "protected Cache.*getCache\(\)" components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.javaRepository: wso2/carbon-identity-framework
Length of output: 870
🏁 Script executed:
# Check the pom.xml for the exact version of org.wso2.carbon.caching being used
fd "pom.xml" -x grep -l "org.wso2.carbon.*caching" {} \; | head -3 | xargs cat | grep -A 3 "caching"Repository: wso2/carbon-identity-framework
Length of output: 1017
🏁 Script executed:
# Check if there are any build dependencies that might provide custom Cache implementations
rg -n "org.wso2.carbon.caching\|javax.cache" --type xml | head -20Repository: wso2/carbon-identity-framework
Length of output: 56
putOnRead() is not a valid method on javax.cache.Cache—this will cause a runtime error.
The standard JCache API and WSO2 Carbon caching implementations (including CacheImpl) do not provide a putOnRead() method. The variable at line 148 is declared as javax.cache.Cache<String, UserIdentityClaimsDO>, which only supports standard methods like put(), get(), and remove(). Replace cache.putOnRead(key, userIdentityDTO) with cache.put(key, userIdentityDTO) unless a custom extension method exists in a different Cache implementation that should be explicitly cast to.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/identity-mgt/org.wso2.carbon.identity.mgt/src/main/java/org/wso2/carbon/identity/mgt/store/InMemoryIdentityDataStore.java`
at line 148, The code in InMemoryIdentityDataStore uses cache.putOnRead(key,
userIdentityDTO) on a variable declared as javax.cache.Cache<String,
UserIdentityClaimsDO>, but javax.cache.Cache does not define putOnRead and this
will cause a runtime error; replace the call with cache.put(key,
userIdentityDTO) (or, if you intentionally need a non-standard implementation,
cast the cache to that specific implementation type before calling its custom
putOnRead method) to resolve the issue.
Comment on lines
+239
to
+244
| if (log.isDebugEnabled()) { | ||
| String message = String.format("Following two cache entries created. 1. Secret by name cache %s, 2." + | ||
| " Secret by id cache %s. Tenant domain for all caches: %s", secret.getSecretName(), | ||
| secret.getSecretId(), secret.getTenantDomain()); | ||
| log.debug(message); | ||
| } |
There was a problem hiding this comment.
Avoid logging secret identifiers in debug statements.
This debug log includes secret name/id and tenant domain; that increases sensitive-data exposure risk in logs.
🔐 Suggested safer logging
- if (log.isDebugEnabled()) {
- String message = String.format("Following two cache entries created. 1. Secret by name cache %s, 2." +
- " Secret by id cache %s. Tenant domain for all caches: %s", secret.getSecretName(),
- secret.getSecretId(), secret.getTenantDomain());
- log.debug(message);
- }
+ if (log.isDebugEnabled()) {
+ log.debug("Secret cache entries created on read path.");
+ }As per coding guidelines, do not log sensitive information including emails, passwords, tokens, secrets, or personally identifiable information.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/secret-mgt/org.wso2.carbon.identity.secret.mgt.core/src/main/java/org/wso2/carbon/identity/secret/mgt/core/dao/impl/CachedBackedSecretDAO.java`
around lines 239 - 244, The debug log in CachedBackedSecretDAO currently prints
sensitive data (secret.getSecretName(), secret.getSecretId(),
secret.getTenantDomain()); remove those direct values and replace the message
with a non-sensitive statement such as "Created two cache entries for secret
(identifier omitted)" or log only safe context (e.g., cache names and count) —
if you must correlate, emit a non-reversible hash/fingerprint of
secret.getSecretId() instead of the raw id; update the log.debug call
accordingly and ensure any formatted message no longer includes
secret.getSecretName(), secret.getSecretId(), or tenant domain.
Comment on lines
78
to
+80
| Pair<Boolean, String> valueToCache = | ||
| Pair.of(adminAdvisoryBannerDTO.getEnableBanner(), adminAdvisoryBannerDTO.getBannerContent()); | ||
| advisoryBannerCache.addToCache(CACHE_KEY, valueToCache, tenantDomain); | ||
| advisoryBannerCache.addToCacheOnRead(CACHE_KEY, valueToCache, tenantDomain); |
There was a problem hiding this comment.
Use write-through cache update on save, not read-safe put-if-absent.
At Line 80, addToCacheOnRead(...) is unsafe for this write path. Based on BaseCache.addToCacheOnRead (components/application-mgt/.../BaseCache.java:160-190), existing keys are not overwritten, so updated banner values can remain stale in cache after save.
Proposed fix
- advisoryBannerCache.addToCacheOnRead(CACHE_KEY, valueToCache, tenantDomain);
+ advisoryBannerCache.addToCache(CACHE_KEY, valueToCache, tenantDomain);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/system-config-mgt/org.wso2.carbon.identity.system.config.mgt/src/main/java/org/wso2/carbon/identity/system/config/mgt/advisory/DBBasedAdminBannerDAO.java`
around lines 78 - 80, The code uses
advisoryBannerCache.addToCacheOnRead(CACHE_KEY, valueToCache, tenantDomain) in
DBBasedAdminBannerDAO which is a read-safe put-if-absent and can leave stale
values after a save; replace this call with a write-through cache update that
overwrites existing entries (use the cache API method that performs an
unconditional put/update rather than addToCacheOnRead) so CACHE_KEY is updated
with Pair.of(adminAdvisoryBannerDTO.getEnableBanner(),
adminAdvisoryBannerDTO.getBannerContent()) for the tenant when saving; ensure
you modify the call in DBBasedAdminBannerDAO (where addToCacheOnRead is invoked)
to use the cache method that guarantees overwrite.
20 tasks
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/claim-mgt/org.wso2.carbon.identity.claim.metadata.mgt/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/dao/CacheBackedLocalClaimDAO.java (1)
1-2: Consider updating the copyright year.The copyright header shows 2016, but per coding guidelines, it should be the current year or a range ending in 2026 (e.g.,
2016-2026). This is a minor update that could be included with this change.Suggested change
-* Copyright (c) 2016, WSO2 Inc. (http://www.wso2.org) All Rights Reserved. +* Copyright (c) 2016-2026, WSO2 Inc. (http://www.wso2.org) All Rights Reserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/claim-mgt/org.wso2.carbon.identity.claim.metadata.mgt/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/dao/CacheBackedLocalClaimDAO.java` around lines 1 - 2, Update the file header copyright year in CacheBackedLocalClaimDAO (class CacheBackedLocalClaimDAO) to reflect the current year or a range ending in 2026 (for example change "2016" to "2016-2026" or "2026") by editing the top-of-file comment block accordingly; no code logic changes required—just replace the year token in the header comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@components/claim-mgt/org.wso2.carbon.identity.claim.metadata.mgt/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/dao/CacheBackedLocalClaimDAO.java`:
- Around line 1-2: Update the file header copyright year in
CacheBackedLocalClaimDAO (class CacheBackedLocalClaimDAO) to reflect the current
year or a range ending in 2026 (for example change "2016" to "2016-2026" or
"2026") by editing the top-of-file comment block accordingly; no code logic
changes required—just replace the year token in the header comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ad92fcdf-f411-44a7-a80f-07d52c8b9aca
📒 Files selected for processing (2)
components/claim-mgt/org.wso2.carbon.identity.claim.metadata.mgt/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/dao/CacheBackedLocalClaimDAO.javacomponents/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/CacheBackedRoleDAO.java
🚧 Files skipped from review as they are similar to previous changes (1)
- components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/CacheBackedRoleDAO.java
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
|
PR builder completed |
jenkins-is-staging
approved these changes
Mar 23, 2026
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/23455301473
ThaminduR
reviewed
Mar 24, 2026
| .getThreadLocalCarbonContext(); | ||
| carbonContext.setTenantId(MultitenantConstants.SUPER_TENANT_ID); | ||
| carbonContext.setTenantDomain(MultitenantConstants.SUPER_TENANT_DOMAIN_NAME); | ||
| // Element already in the cache. Remove it first |
Contributor
There was a problem hiding this comment.
Let's remove this comment
|
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
approved these changes
Mar 24, 2026
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/23480070253
ThaminduR
approved these changes
Mar 24, 2026
ThaminduR
added a commit
that referenced
this pull request
Mar 26, 2026
[Sync][master -> next][#7871]: Refactor cache operations to use read-safe methods for concurrent environments
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
3 participants
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.
Proposed changes in this pull request
When should this PR be merged
[Please describe any preconditions that need to be addressed before we
can merge this pull request.]
Follow up actions
[List any possible follow-up actions here; for instance, testing data
migrations, software that we need to install on staging and production
environments.]
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Checklist (for reviewing)
General
Functionality
Code
Tests
Security
Documentation