Refactor cache operations to use read-safe methods for concurrent environments#4551
Merged
ThaminduR merged 2 commits intowso2:4.12.xfrom Mar 24, 2026
Merged
Refactor cache operations to use read-safe methods for concurrent environments#4551ThaminduR merged 2 commits intowso2:4.12.xfrom
ThaminduR merged 2 commits intowso2:4.12.xfrom
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds "OnRead" cache-population APIs and read-optimized lookup methods that use conditional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.41.1)core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadOnlyLDAPUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManager.javaThanks 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
core/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCResourceDAO.java (1)
2205-2222: Consider completing the Javadoc for this new public API method.The implementation is correct and consistent with the existing
getPathFromIdmethod. However, as a new public API addition, the Javadoc could be enhanced with standard annotations for better documentation.📝 Suggested Javadoc enhancement
/** * 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` pathId the path ID to look up + * `@return` the path string corresponding to the ID, or null if not found + * `@throws` RegistryException if a database error occurs */ public String getPathFromIdOnRead(int pathId) throws RegistryException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCResourceDAO.java` around lines 2205 - 2222, The new public method getPathFromIdOnRead(int pathId) lacks complete Javadoc; update its JavaDoc to include a one-line description of behavior (that it populates the path cache on read without overwriting existing entries), add `@param` pathId describing the input, add `@return` describing the returned path string (or null if not found), add `@throws` RegistryException documenting when SQL errors occur, and include `@since/`@see tags consistent with the existing getPathFromId method to keep the public API documentation complete and consistent.core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDManger.java (3)
144-175: Significant code duplication withgetUsermethod.The
getUserOnReadmethod is nearly identical togetUser(lines 87-118), differing only in the cache helper calls on lines 166-167. This violates DRY and increases maintenance burden—any bug fix or logic change must be applied in two places.Consider extracting the shared logic into a private helper method that accepts a flag or strategy for cache population.
♻️ Suggested refactor to eliminate duplication
+ private User getUserInternal(String uniqueId, AbstractUserStoreManager userStoreManager, + String userStoreDomain, boolean useReadSafeCache) + throws UserStoreException { + + String userName = getFromUserNameCache(uniqueId); + if (StringUtils.isEmpty(userName)) { + String[] usernames; + if (StringUtils.isNotEmpty(userStoreDomain)) { + usernames = userStoreManager.getUserList(UserCoreClaimConstants.USER_ID_CLAIM_URI, + UserCoreUtil.addDomainToName(uniqueId, userStoreDomain), null); + } else { + usernames = userStoreManager.getUserList(UserCoreClaimConstants.USER_ID_CLAIM_URI, + uniqueId, null); + } + + if (usernames.length > 1) { + throw new UserStoreException("More than one user presents with the same user unique id."); + } + + if (usernames.length == 0) { + return null; + } + userName = usernames[0]; + if (useReadSafeCache) { + addToUserNameCacheOnRead(uniqueId, userName); + addToUserIDCacheOnRead(uniqueId, userName); + } else { + addToUserNameCache(uniqueId, userName); + addToUserIDCache(uniqueId, userName); + } + } + + User user = new User(); + user.setUserID(uniqueId); + user.setUsername(UserCoreUtil.removeDomainFromName(userName)); + user.setUserStoreDomain(UserCoreUtil.extractDomainFromName(userName)); + return user; + } public User getUser(String uniqueId, AbstractUserStoreManager userStoreManager, String userStoreDomain) throws UserStoreException { - // ... current implementation + return getUserInternal(uniqueId, userStoreManager, userStoreDomain, false); } public User getUserOnRead(String uniqueId, AbstractUserStoreManager userStoreManager, String userStoreDomain) throws UserStoreException { - // ... current implementation + return getUserInternal(uniqueId, userStoreManager, userStoreDomain, true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDManger.java` around lines 144 - 175, getUserOnRead duplicates most of getUser; extract the shared lookup logic into a private helper (e.g., resolveUserByUniqueId) that performs cache lookup, userList retrieval via AbstractUserStoreManager.getUserList, validation of usernames length, and creation of the User object, and have both getUser and getUserOnRead call that helper; for the differing cache writes (addToUserNameCacheOnRead, addToUserIDCacheOnRead) add a boolean flag or a small callback/strategy parameter to the helper so getUserOnRead triggers the on-read cache population while getUser uses the existing behavior.
184-199: Incomplete Javadoc - empty@paramdescriptions.The
@paramtags on lines 191-192 are missing their descriptions. This pattern is repeated inaddToUserNameCacheOnReadas well.📝 Add parameter descriptions
* `@param` userID - * `@param` userName + * `@param` userID The unique identifier of the user. + * `@param` userName The username associated with the user ID. */ private void addToUserIDCacheOnRead(String userID, String userName) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDManger.java` around lines 184 - 199, The Javadoc for addToUserIDCacheOnRead (and the similar addToUserNameCacheOnRead) is missing descriptions for the `@param` tags; update the Javadoc in class UserUniqueIDManger to provide brief descriptions for each parameter (e.g., describe that userID is the unique identifier and userName is the username to be cached) so both methods' `@param` tags are populated and self-documenting.
120-130: Javadoc is malformed and inconsistent.The Javadoc mixes two different descriptions: it starts with cache behavior explanation (lines 121-126) and then abruptly switches to method documentation (lines 127-129). This appears to be copy-paste from the cache method documentation.
📝 Suggested Javadoc fix
/** - * 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. - * <p> * Get user from unique id. + * <p> + * This method uses read-safe cache population that only populates the cache + * if the key does not already have a value, avoiding unnecessary cache + * invalidation broadcasts in clustered environments. + * * `@param` uniqueId User's unique id. + * `@param` userStoreManager User store manager instance. * `@return` User object if user presents for the unique id. Null otherwise. + * `@throws` UserStoreException if more than one user exists with the same unique id. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDManger.java` around lines 120 - 130, The Javadoc above the getUserFromUniqueId method in UserUniqueIDManger is wrong: remove the cache-related paragraphs and replace them with a concise method description that documents behavior and parameters; ensure the first sentence describes "Get user from unique id", include a proper `@param` uniqueId description and a proper `@return` stating it returns the User object or null if not found, and keep formatting consistent (one description block followed by `@param` and `@return`) so the Javadoc matches the getUserFromUniqueId method signature.core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java (2)
14056-14068: Consider renaming for consistency with the new pattern.These private methods now use
addToCacheOnReadinternally (conditional/putIfAbsent semantics), but their names (addToUserIDCache,addToUserNameCache) don't reflect this behavioral change. The new protected methods follow an explicit naming convention withOnReadsuffix.This inconsistency could confuse maintainers about whether these methods perform unconditional or conditional cache population.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java` around lines 14056 - 14068, Rename the private helpers addToUserIDCache and addToUserNameCache to names that reflect the conditional/put-if-absent semantics (e.g., addToUserIDCacheOnRead and addToUserNameCacheOnRead) to match the existing protected methods that use the OnRead suffix; update all call sites accordingly and keep the internal logic that calls UserIdResolverCache.getInstance().addToCacheOnRead unchanged (referencing addToCacheOnRead and UserCoreUtil.addDomainToName to locate the implementations).
9502-9517: Consider adding null-safety checks for parameters.The method checks for
userRolesCache != nullbut doesn't validateuserNameorroleList. If callers pass null values,UserCoreUtil.addDomainToNameoraddDomainToNamescould behave unexpectedly.🛡️ Optional: Add defensive null checks
protected void addToUserRolesCacheOnRead(int tenantID, String userName, String[] roleList) { - if (userRolesCache != null) { + if (userRolesCache != null && userName != null && roleList != null) { String usernameWithDomain = UserCoreUtil.addDomainToName(userName, getMyDomainName()); String[] rolesWithDomain = UserCoreUtil.addDomainToNames(roleList, getMyDomainName()); userRolesCache.addToCacheOnRead(cacheIdentifier, tenantID, usernameWithDomain, rolesWithDomain); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java` around lines 9502 - 9517, The addToUserRolesCacheOnRead method currently only checks userRolesCache but can NPE when userName or roleList are null; update addToUserRolesCacheOnRead to defensively validate parameters (ensure userName != null/empty and roleList != null) before calling UserCoreUtil.addDomainToName and UserCoreUtil.addDomainToNames, and bail out early if invalid; keep the existing userRolesCache != null and use cacheIdentifier and tenantID only when inputs are sane; optionally treat an empty roleList as an empty array so the cache call remains safe.core/javax.cache/src/main/java/org/wso2/carbon/caching/impl/CacheImpl.java (1)
415-417: Avoid logging cached values in debug pathLine 416 logs
oldValue; cache entries can contain sensitive tenant/user data. Log key/context only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/javax.cache/src/main/java/org/wso2/carbon/caching/impl/CacheImpl.java` around lines 415 - 417, In CacheImpl, remove the sensitive value from the debug log: replace the log.debug that currently concatenates oldValue with a call that only logs the key/context (e.g., "Cache already populated on read. Key: " + key) and/or mask the value; update the log statement referencing variables key and oldValue so oldValue is not included in the message (keep class CacheImpl and the surrounding read/populate logic unchanged).core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDDomainResolver.java (1)
167-223: Consider consolidating duplicated domain-resolution logic into a shared internal method.This new method duplicates nearly all of
getDomainForUserId(...). A shared private method with a write-strategy flag/callback would reduce drift risk and keep bug fixes in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDDomainResolver.java` around lines 167 - 223, The getDomainForUserIdOnRead method largely duplicates getDomainForUserId; refactor by extracting the common domain-resolution flow into a single private helper (e.g., resolveDomainForUserIdInternal) that accepts a strategy flag or callback to handle write-side actions (cache putOnRead, DB writes/deletes) so both getDomainForUserIdOnRead and getDomainForUserId delegate to it; ensure the helper preserves existing behavior around PrivilegedCarbonContext, cache lookup (uniqueIdDomainCache), calling getDomainFromDB, validating via RealmService/AbstractUserStoreManager.getSecondaryUserStoreManager, and invoking deleteDomainFromDB/clearUserIDResolverCache only when necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/javax.cache/src/main/java/org/wso2/carbon/caching/impl/CacheImpl.java`:
- Around line 409-413: The local-only check in putOnRead (using localCache.get,
then internalPut and notifyCacheEntryCreated) can overwrite an existing key in
clustered/distributed mode; change putOnRead to perform a cluster-aware
put-if-absent instead of a blind local put: replace the local-only path in the
putOnRead implementation so it queries the distributed backing (e.g., via the
cluster/remote cache API or an existing internalPutIfAbsent/putIfAbsent
primitive) to determine whether the key truly is absent across the cluster, call
the cluster-aware insert only when absent, and only call notifyCacheEntryCreated
when that cluster-aware insert actually succeeded (i.e., the key was inserted).
Ensure you update references to internalPut/notifyCacheEntryCreated in
CacheImpl.putOnRead to use the put-if-absent primitive and check its result
before notifying.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/GroupIdResolverCache.java`:
- Around line 97-99: The code performs a check-then-act using
cache.containsKey(key) before calling cache.putOnRead(key, entry), which is
racy; remove the containsKey check and call cache.putOnRead(key, entry) directly
(inside the existing null check for cache) so the cache's own conditional-insert
logic handles concurrency; keep the existing debug logging
(log.isDebugEnabled()) and any subsequent behavior unchanged, only eliminate the
containsKey(...) pre-check.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadOnlyLDAPUserStoreManager.java`:
- Around line 4677-4692: The subclass UniqueIDReadOnlyLDAPUserStoreManager
currently overrides putToUserCache(...) but not the new
putToUserCacheOnRead(...), so when the parent ReadOnlyLDAPUserStoreManager calls
putToUserCacheOnRead(...) the subclass logic is bypassed; fix this by adding an
override of putToUserCacheOnRead(String name, LdapName value) in
UniqueIDReadOnlyLDAPUserStoreManager that implements the same custom cache
handling as its putToUserCache(...) override (or factor the shared behavior into
a private/shared helper and call it from both putToUserCache(...) and
putToUserCacheOnRead(...) in both classes), ensuring the subclass’s cache
semantics are preserved whenever the parent invokes putToUserCacheOnRead.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/JDBCTenantManager.java`:
- Around line 753-754: The reverse mapping cache update currently calls
tenantDomainCache.addToCache(new TenantIdKey(tenantId), new
TenantDomainEntry(tenantDomain)) in a read path; change this to use the
read-safe cache update used for the forward mapping (i.e., call the read-safe
variant such as tenantDomainCache.addToCacheOnRead or equivalent) so reads do
not perform write-style side effects, passing the same TenantIdKey(tenantId) and
TenantDomainEntry(tenantDomain) as arguments to match the forward call to
tenantIdCache.addToCacheOnRead(tenantDomainKey, new TenantIdEntry(tenantId)).
- Around line 651-652: The second cache write in the read path should use the
read-safe API: replace the write-style call tenantIdCache.addToCache(new
TenantDomainKey(tenantDomain), new TenantIdEntry(tenantId)) with the read-safe
method tenantIdCache.addToCacheOnRead(new TenantDomainKey(tenantDomain), new
TenantIdEntry(tenantId)) so both mappings use on-read semantics alongside
tenantDomainCache.addToCacheOnRead(tenantIdKey, new
TenantDomainEntry(tenantDomain)).
---
Nitpick comments:
In `@core/javax.cache/src/main/java/org/wso2/carbon/caching/impl/CacheImpl.java`:
- Around line 415-417: In CacheImpl, remove the sensitive value from the debug
log: replace the log.debug that currently concatenates oldValue with a call that
only logs the key/context (e.g., "Cache already populated on read. Key: " + key)
and/or mask the value; update the log statement referencing variables key and
oldValue so oldValue is not included in the message (keep class CacheImpl and
the surrounding read/populate logic unchanged).
In
`@core/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCResourceDAO.java`:
- Around line 2205-2222: The new public method getPathFromIdOnRead(int pathId)
lacks complete Javadoc; update its JavaDoc to include a one-line description of
behavior (that it populates the path cache on read without overwriting existing
entries), add `@param` pathId describing the input, add `@return` describing the
returned path string (or null if not found), add `@throws` RegistryException
documenting when SQL errors occur, and include `@since/`@see tags consistent with
the existing getPathFromId method to keep the public API documentation complete
and consistent.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java`:
- Around line 14056-14068: Rename the private helpers addToUserIDCache and
addToUserNameCache to names that reflect the conditional/put-if-absent semantics
(e.g., addToUserIDCacheOnRead and addToUserNameCacheOnRead) to match the
existing protected methods that use the OnRead suffix; update all call sites
accordingly and keep the internal logic that calls
UserIdResolverCache.getInstance().addToCacheOnRead unchanged (referencing
addToCacheOnRead and UserCoreUtil.addDomainToName to locate the
implementations).
- Around line 9502-9517: The addToUserRolesCacheOnRead method currently only
checks userRolesCache but can NPE when userName or roleList are null; update
addToUserRolesCacheOnRead to defensively validate parameters (ensure userName !=
null/empty and roleList != null) before calling UserCoreUtil.addDomainToName and
UserCoreUtil.addDomainToNames, and bail out early if invalid; keep the existing
userRolesCache != null and use cacheIdentifier and tenantID only when inputs are
sane; optionally treat an empty roleList as an empty array so the cache call
remains safe.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDDomainResolver.java`:
- Around line 167-223: The getDomainForUserIdOnRead method largely duplicates
getDomainForUserId; refactor by extracting the common domain-resolution flow
into a single private helper (e.g., resolveDomainForUserIdInternal) that accepts
a strategy flag or callback to handle write-side actions (cache putOnRead, DB
writes/deletes) so both getDomainForUserIdOnRead and getDomainForUserId delegate
to it; ensure the helper preserves existing behavior around
PrivilegedCarbonContext, cache lookup (uniqueIdDomainCache), calling
getDomainFromDB, validating via
RealmService/AbstractUserStoreManager.getSecondaryUserStoreManager, and invoking
deleteDomainFromDB/clearUserIDResolverCache only when necessary.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDManger.java`:
- Around line 144-175: getUserOnRead duplicates most of getUser; extract the
shared lookup logic into a private helper (e.g., resolveUserByUniqueId) that
performs cache lookup, userList retrieval via
AbstractUserStoreManager.getUserList, validation of usernames length, and
creation of the User object, and have both getUser and getUserOnRead call that
helper; for the differing cache writes (addToUserNameCacheOnRead,
addToUserIDCacheOnRead) add a boolean flag or a small callback/strategy
parameter to the helper so getUserOnRead triggers the on-read cache population
while getUser uses the existing behavior.
- Around line 184-199: The Javadoc for addToUserIDCacheOnRead (and the similar
addToUserNameCacheOnRead) is missing descriptions for the `@param` tags; update
the Javadoc in class UserUniqueIDManger to provide brief descriptions for each
parameter (e.g., describe that userID is the unique identifier and userName is
the username to be cached) so both methods' `@param` tags are populated and
self-documenting.
- Around line 120-130: The Javadoc above the getUserFromUniqueId method in
UserUniqueIDManger is wrong: remove the cache-related paragraphs and replace
them with a concise method description that documents behavior and parameters;
ensure the first sentence describes "Get user from unique id", include a proper
`@param` uniqueId description and a proper `@return` stating it returns the User
object or null if not found, and keep formatting consistent (one description
block followed by `@param` and `@return`) so the Javadoc matches the
getUserFromUniqueId method signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6734ceb9-5aaf-42eb-8359-45425ee08282
📒 Files selected for processing (23)
core/javax.cache/src/main/java/javax/cache/Cache.javacore/javax.cache/src/main/java/org/wso2/carbon/caching/impl/CacheImpl.javacore/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCPathCache.javacore/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCResourceDAO.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/authorization/AuthorizationCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/authorization/JDBCAuthorizationManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/DefaultRealmService.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/GroupIdResolverCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/RealmCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserIdResolverCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserRolesCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDDomainResolver.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDManger.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/jdbc/UniqueIDJDBCUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadOnlyLDAPUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/FileSystemTenantManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/JDBCTenantManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/TenantCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/TenantDomainCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/TenantIdCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/TenantUniqueIdCache.java
Comment on lines
+409
to
+413
| CacheEntry entry = localCache.get(key); | ||
| V oldValue = entry != null ? (V) entry.getValue() : null; | ||
| if (oldValue == null) { | ||
| internalPut(key, value); | ||
| notifyCacheEntryCreated(key, value); |
There was a problem hiding this comment.
putOnRead checks only local state, so it can violate “only if absent” in distributed mode
With Line 409-413, a local miss triggers internalPut, but in distributed mode the key may already exist remotely. That can overwrite existing clustered data during read-population.
Suggested fix
- CacheEntry entry = localCache.get(key);
- V oldValue = entry != null ? (V) entry.getValue() : null;
- if (oldValue == null) {
+ CacheEntry<K, V> entry = localCache.get(key);
+ if (entry == null && !isLocalCache) {
+ entry = distributedCache.get(key);
+ if (entry != null) {
+ localCache.putIfAbsent(key, entry);
+ }
+ }
+ if (entry == null) {
internalPut(key, value);
notifyCacheEntryCreated(key, value);
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/javax.cache/src/main/java/org/wso2/carbon/caching/impl/CacheImpl.java`
around lines 409 - 413, The local-only check in putOnRead (using localCache.get,
then internalPut and notifyCacheEntryCreated) can overwrite an existing key in
clustered/distributed mode; change putOnRead to perform a cluster-aware
put-if-absent instead of a blind local put: replace the local-only path in the
putOnRead implementation so it queries the distributed backing (e.g., via the
cluster/remote cache API or an existing internalPutIfAbsent/putIfAbsent
primitive) to determine whether the key truly is absent across the cluster, call
the cluster-aware insert only when absent, and only call notifyCacheEntryCreated
when that cluster-aware insert actually succeeded (i.e., the key was inserted).
Ensure you update references to internalPut/notifyCacheEntryCreated in
CacheImpl.putOnRead to use the put-if-absent primitive and check its result
before notifying.
...o2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/GroupIdResolverCache.java
Show resolved
Hide resolved
Comment on lines
+4677
to
+4692
| protected void putToUserCacheOnRead(String name, LdapName value) { | ||
| try { | ||
| startTenantFlow(); | ||
| Cache<String, LdapName> userDnCache = createOrGetUserDnCache(); | ||
| if (userDnCache == null) { | ||
| // User cache may be null while initializing. | ||
| return; | ||
| } | ||
| userDnCache.putOnRead(name, value); | ||
| } catch (IllegalStateException e) { | ||
| // There is no harm ignoring the put, as the cache(local) is already is of no use. Mis-penalty is low. | ||
| log.error("Error occurred while putting User DN to the cache having search base : " + userSearchBase, e); | ||
| } finally { | ||
| PrivilegedCarbonContext.endTenantFlow(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Subclasses of ReadOnlyLDAPUserStoreManager =="
rg -n --type=java -C2 'class\s+\w+\s+extends\s+ReadOnlyLDAPUserStoreManager' core
echo
echo "== Overrides of putToUserCache / putToUserCacheOnRead =="
rg -n --type=java -C2 'protected\s+void\s+putToUserCache\s*\(|protected\s+void\s+putToUserCacheOnRead\s*\(' \
core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldapRepository: wso2/carbon-kernel
Length of output: 4044
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Call sites of putToUserCacheOnRead =="
rg -n --type=java 'putToUserCacheOnRead\s*\(' core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ -B2 -A1
echo
echo "== UniqueIDReadOnlyLDAPUserStoreManager.putToUserCache() implementation (lines 3085-3105) =="
sed -n '3085,3105p' core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManager.java
echo
echo "== Check if ReadWriteLDAPUserStoreManager overrides putToUserCache =="
grep -n 'putToUserCache' core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadWriteLDAPUserStoreManager.java || echo "No overrides found"Repository: wso2/carbon-kernel
Length of output: 3008
Add putToUserCacheOnRead() override to subclasses with custom cache handling.
UniqueIDReadOnlyLDAPUserStoreManager overrides putToUserCache(...) but not the new putToUserCacheOnRead(...) method. When the parent class calls putToUserCacheOnRead() (lines 911, 2528), the subclass override is bypassed. Either override putToUserCacheOnRead(...) in UniqueIDReadOnlyLDAPUserStoreManager to maintain consistent behavior, or refactor both methods to a shared internal hook to avoid this inheritance gap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadOnlyLDAPUserStoreManager.java`
around lines 4677 - 4692, The subclass UniqueIDReadOnlyLDAPUserStoreManager
currently overrides putToUserCache(...) but not the new
putToUserCacheOnRead(...), so when the parent ReadOnlyLDAPUserStoreManager calls
putToUserCacheOnRead(...) the subclass logic is bypassed; fix this by adding an
override of putToUserCacheOnRead(String name, LdapName value) in
UniqueIDReadOnlyLDAPUserStoreManager that implements the same custom cache
handling as its putToUserCache(...) override (or factor the shared behavior into
a private/shared helper and call it from both putToUserCache(...) and
putToUserCacheOnRead(...) in both classes), ensuring the subclass’s cache
semantics are preserved whenever the parent invokes putToUserCacheOnRead.
....wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/JDBCTenantManager.java
Outdated
Show resolved
Hide resolved
....wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/JDBCTenantManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
core/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCPathCache.java (1)
452-518: Consider reducing duplication between*OnReadmethods and their counterparts.The
getPathOnReadandgetPathIDOnReadmethods are nearly identical togetPathandgetPathIDrespectively—only the cache write call differs (putOnReadvsput). This leads to duplicated JDBC query logic, error handling, and resource cleanup across ~120 lines.A possible approach is to extract a common helper or use a functional parameter for the cache-write strategy:
// Example refactor concept private String getPathInternal(AbstractConnection conn, int id, BiConsumer<RegistryCacheKey, RegistryCacheEntry> cacheWriter) throws SQLException { // ... shared logic ... cacheWriter.accept(key, result); // ... } public String getPath(AbstractConnection conn, int id) throws SQLException { return getPathInternal(conn, id, (k, v) -> getCache().put(k, v)); } public String getPathOnRead(AbstractConnection conn, int id) throws SQLException { return getPathInternal(conn, id, (k, v) -> getCache().putOnRead(k, v)); }Also applies to: 610-704
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCPathCache.java` around lines 452 - 518, getPathOnRead and getPathIDOnRead duplicate almost all JDBC, error-handling and cleanup logic present in getPath and getPathID; extract a shared helper (e.g., getPathInternal / getPathIDInternal) that takes AbstractConnection, id and a cache-write strategy (a BiConsumer<RegistryCacheKey, RegistryCacheEntry> or a functional interface) to perform the shared query, result mapping and resource cleanup, and call getCache().put(...) or getCache().putOnRead(...) from the callers; apply the same refactor for the getPathID/getPathIDOnRead pair (also for the similar block around lines 610-704) so only the cache write behavior differs while all JDBC and exception handling is centralized.core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDDomainResolver.java (1)
167-223: Consider extracting shared resolver flow to avoid divergence between read/write cache variants.
getDomainForUserId(...)andgetDomainForUserIdOnRead(...)now duplicate almost all logic. A shared private method with a cache-write strategy flag/lambda would reduce maintenance risk.♻️ Suggested refactor sketch
+private enum CacheWriteMode { WRITE, ON_READ } + +private String resolveDomainForUserId(String userId, int tenantId, CacheWriteMode mode) throws UserStoreException { + // existing common logic... + if (StringUtils.isNotBlank(domainName)) { + if (mode == CacheWriteMode.ON_READ) { + uniqueIdDomainCache.putOnRead(userId, domainName); + } else { + uniqueIdDomainCache.put(userId, domainName); + } + } + // existing common logic... +} + public String getDomainForUserId(String userId, int tenantId) throws UserStoreException { - // duplicated flow + return resolveDomainForUserId(userId, tenantId, CacheWriteMode.WRITE); } public String getDomainForUserIdOnRead(String userId, int tenantId) throws UserStoreException { - // duplicated flow + return resolveDomainForUserId(userId, tenantId, CacheWriteMode.ON_READ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDDomainResolver.java` around lines 167 - 223, getDomainForUserIdOnRead and getDomainForUserId duplicate almost all logic; extract the shared flow into a private helper (e.g., resolveDomainForUserId) that accepts the userId, tenantId and a cache-write strategy flag or functional lambda to control whether to update cache/DB on miss (read vs write behavior). Move common steps (start/end tenant flow, cache lookup, DB lookup via getDomainFromDB, cache update, validation of domain against RealmService and secondary store lookup) into that helper, and have getDomainForUserId and getDomainForUserIdOnRead call it with the appropriate strategy (read-only vs read-write), preserving calls to deleteDomainFromDB and clearUserIDResolverCache where the validation determines an outdated entry. Ensure exception handling around RealmService/UserStoreException remains the same.core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java (1)
14056-14068: Private cache methods use conditional writes but names don't reflect this.The private methods
addToUserIDCacheandaddToUserNameCache(lines 14056 and 14063) internally useaddToCacheOnRead, which conditionally populates the cache only if the key doesn't already exist. However, the method names suggest unconditional cache writes, potentially confusing future maintainers.All current callers are safely within read-only contexts (
getUserIDFromUserNameandgetUserNameFromCurrentUserStore), so the behavior change is safe. However, consider renaming these private methods toaddToUserIDCacheOnReadandaddToUserNameCacheOnReadto make the conditional caching semantics explicit, or document why the simpler naming is acceptable despite the semantic shift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java` around lines 14056 - 14068, Rename the two private helpers to make their conditional semantics explicit: change addToUserIDCache to addToUserIDCacheOnRead and addToUserNameCache to addToUserNameCacheOnRead, update all internal call sites (notably getUserIDFromUserName and getUserNameFromCurrentUserStore) to use the new names, and keep the internal implementation calling UserIdResolverCache.getInstance().addToCacheOnRead(...) unchanged; alternatively, if you prefer to keep names, add a short javadoc to both methods stating they only add the entry if absent via addToCacheOnRead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/authorization/JDBCAuthorizationManager.java`:
- Line 290: The call to authorizationCache.addToCacheOnRead in
JDBCAuthorizationManager can persist transient "deny" results
(userAllowed==false) produced when role checks partially failed or threw
UserStoreException; to fix, only add-on-read when the decision is definitive:
change the callers around addToCacheOnRead (the sites that pass cacheIdentifier,
tenantId, userName, resourceId, action, userAllowed) to skip caching when
userAllowed==false if the false was produced after catching an exception or if
role-check paths indicate partial failure, and only call addToCacheOnRead for
successful/definitive allows (or for definitive denies determined without
exceptions); alternatively, make addToCacheOnRead/putOnRead accept a
"definitive" flag and ensure put-if-absent is only used for definitive outcomes
so later successful evaluations can overwrite transient false entries.
---
Nitpick comments:
In
`@core/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCPathCache.java`:
- Around line 452-518: getPathOnRead and getPathIDOnRead duplicate almost all
JDBC, error-handling and cleanup logic present in getPath and getPathID; extract
a shared helper (e.g., getPathInternal / getPathIDInternal) that takes
AbstractConnection, id and a cache-write strategy (a
BiConsumer<RegistryCacheKey, RegistryCacheEntry> or a functional interface) to
perform the shared query, result mapping and resource cleanup, and call
getCache().put(...) or getCache().putOnRead(...) from the callers; apply the
same refactor for the getPathID/getPathIDOnRead pair (also for the similar block
around lines 610-704) so only the cache write behavior differs while all JDBC
and exception handling is centralized.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java`:
- Around line 14056-14068: Rename the two private helpers to make their
conditional semantics explicit: change addToUserIDCache to
addToUserIDCacheOnRead and addToUserNameCache to addToUserNameCacheOnRead,
update all internal call sites (notably getUserIDFromUserName and
getUserNameFromCurrentUserStore) to use the new names, and keep the internal
implementation calling UserIdResolverCache.getInstance().addToCacheOnRead(...)
unchanged; alternatively, if you prefer to keep names, add a short javadoc to
both methods stating they only add the entry if absent via addToCacheOnRead.
In
`@core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDDomainResolver.java`:
- Around line 167-223: getDomainForUserIdOnRead and getDomainForUserId duplicate
almost all logic; extract the shared flow into a private helper (e.g.,
resolveDomainForUserId) that accepts the userId, tenantId and a cache-write
strategy flag or functional lambda to control whether to update cache/DB on miss
(read vs write behavior). Move common steps (start/end tenant flow, cache
lookup, DB lookup via getDomainFromDB, cache update, validation of domain
against RealmService and secondary store lookup) into that helper, and have
getDomainForUserId and getDomainForUserIdOnRead call it with the appropriate
strategy (read-only vs read-write), preserving calls to deleteDomainFromDB and
clearUserIDResolverCache where the validation determines an outdated entry.
Ensure exception handling around RealmService/UserStoreException remains the
same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 9b38ae6c-da8b-4527-bb06-4334b74e5adb
📒 Files selected for processing (21)
core/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCPathCache.javacore/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCResourceDAO.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/authorization/AuthorizationCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/authorization/JDBCAuthorizationManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/DefaultRealmService.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/GroupIdResolverCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/RealmCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserIdResolverCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserRolesCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDDomainResolver.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDManger.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/jdbc/UniqueIDJDBCUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadOnlyLDAPUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/FileSystemTenantManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/JDBCTenantManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/TenantCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/TenantDomainCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/TenantIdCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/TenantUniqueIdCache.java
🚧 Files skipped from review as they are similar to previous changes (11)
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserIdResolverCache.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/GroupIdResolverCache.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserRolesCache.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/authorization/AuthorizationCache.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/TenantCache.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/RealmCache.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/JDBCTenantManager.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/jdbc/UniqueIDJDBCUserStoreManager.java
- core/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCResourceDAO.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadOnlyLDAPUserStoreManager.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDManger.java
...ser.core/src/main/java/org/wso2/carbon/user/core/authorization/JDBCAuthorizationManager.java
Show resolved
Hide resolved
20 tasks
ThaminduR
reviewed
Mar 20, 2026
| tenantDomainNameValidation(tenantDomain); | ||
| tenantDomainCache.addToCache(tenantIdKey, new TenantDomainEntry(tenantDomain)); | ||
| tenantDomainCache.addToCacheOnRead(tenantIdKey, new TenantDomainEntry(tenantDomain)); | ||
| tenantIdCache.addToCache(new TenantDomainKey(tenantDomain), new TenantIdEntry(tenantId)); |
ThaminduR
reviewed
Mar 20, 2026
| tenantDomainNameValidation(tenantDomain); | ||
| tenantIdCache.addToCache(tenantDomainKey, new TenantIdEntry(tenantId)); | ||
| tenantIdCache.addToCacheOnRead(tenantDomainKey, new TenantIdEntry(tenantId)); | ||
| tenantDomainCache.addToCache(new TenantIdKey(tenantId), new TenantDomainEntry(tenantDomain)); |
20 tasks
Malith-19
reviewed
Mar 21, 2026
...rbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCResourceDAO.java
Show resolved
Hide resolved
| @@ -14067,7 +14084,7 @@ private void addUsersToUserIdCache(List<User> userList) { | |||
There was a problem hiding this comment.
Can we create a new method like, addUsersToUserIdCacheOnRead. Unless developers will use this addToCache method when adding the users too. At the moment I think this method is only used for read operations right?
Author
There was a problem hiding this comment.
Yes. Having a distinguished method for read flows would be a better approach.
Comment on lines
+137
to
+138
| public void addToCacheOnRead(String serverId, int tenantId, String userName, String[] userRoleList) { | ||
| try { |
There was a problem hiding this comment.
Suggested change
| public void addToCacheOnRead(String serverId, int tenantId, String userName, String[] userRoleList) { | |
| try { | |
| public void addToCacheOnRead(String serverId, int tenantId, String userName, String[] userRoleList) { | |
| try { |
...bon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadOnlyLDAPUserStoreManager.java
Show resolved
Hide resolved
| + " for the key : " + key + " successfully"); | ||
| } else { | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("Error while getting the cache : " + TENANT_DOMAIN_CACHE + " which is under " + TENANT_DOMAIN_CACHE_MANAGER); |
There was a problem hiding this comment.
Check the line lengths, if they exceeds 120 limit let's break it to multiple lines..
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCResourceDAO.java (1)
2205-2223: Consider completing Javadoc annotations.The method is well-implemented with proper error handling consistent with
getPathFromId. The Javadoc explains the read-safe semantics well, but could be more complete with standard annotations.📝 Suggested Javadoc improvement
/** * 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` pathId the path ID to look up + * `@return` the path string corresponding to the path ID, or null if not found + * `@throws` RegistryException if database access fails */ public String getPathFromIdOnRead(int pathId) throws RegistryException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCResourceDAO.java` around lines 2205 - 2223, Add standard Javadoc annotations to the getPathFromIdOnRead method: include an `@param` describing the pathId argument, an `@return` describing the returned path string, and an `@throws` RegistryException describing when SQL errors occur; optionally add a `@see` reference to getPathFromId and mention the underlying calls JDBCPathCache.getPathCache().getPathOnRead and JDBCDatabaseTransaction.getConnection for context. Ensure the Javadoc text is concise and matches the existing style used alongside getPathFromId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@core/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCResourceDAO.java`:
- Around line 2205-2223: Add standard Javadoc annotations to the
getPathFromIdOnRead method: include an `@param` describing the pathId argument, an
`@return` describing the returned path string, and an `@throws` RegistryException
describing when SQL errors occur; optionally add a `@see` reference to
getPathFromId and mention the underlying calls
JDBCPathCache.getPathCache().getPathOnRead and
JDBCDatabaseTransaction.getConnection for context. Ensure the Javadoc text is
concise and matches the existing style used alongside getPathFromId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 90d1fb0e-3e85-4bd2-ad22-7b80e9193a36
📒 Files selected for processing (8)
core/org.wso2.carbon.registry.core/src/main/java/org/wso2/carbon/registry/core/jdbc/dao/JDBCResourceDAO.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserRolesCache.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDManger.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadOnlyLDAPUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/UniqueIDReadOnlyLDAPUserStoreManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/JDBCTenantManager.javacore/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/TenantDomainCache.java
🚧 Files skipped from review as they are similar to previous changes (5)
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserRolesCache.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/tenant/JDBCTenantManager.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/UserUniqueIDManger.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/ldap/ReadOnlyLDAPUserStoreManager.java
- core/org.wso2.carbon.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java
|
PR builder started |
|
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/23421901041
ThaminduR
approved these changes
Mar 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning