Refactor cache operations to use read-safe methods for concurrent environments#4551
Open
Zeta201 wants to merge 2 commits intowso2:4.12.xfrom
Open
Refactor cache operations to use read-safe methods for concurrent environments#4551Zeta201 wants to merge 2 commits intowso2:4.12.xfrom
Zeta201 wants to merge 2 commits intowso2:4.12.xfrom
Conversation
| No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds "OnRead" cache-population APIs and switches selected cache writes and lookups to use conditional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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)
📝 Coding Plan
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 Tip You can customize the tone of the review comments and chat replies.Configure the |
![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. Comment on lines +97 to +99
| if (cache != null && !cache.containsKey(key)) { | ||
| cache.putOnRead(key, entry); | ||
| if (log.isDebugEnabled()) { |
There was a problem hiding this comment.
Remove containsKey pre-check before putOnRead
Line 97 performs a check-then-act sequence. Under concurrency, another thread/node can populate between containsKey and putOnRead. Rely directly on putOnRead for conditional insertion.
Suggested fix
- if (cache != null && !cache.containsKey(key)) { + if (cache != null) { cache.putOnRead(key, entry); if (log.isDebugEnabled()) { log.debug(String.format("Cache: %s which is under %s, added the entry: %s for the key: " + "%s successfully", cacheName, GROUP_ID_RESOLVER_CACHE_MANAGER, entry, key)); } }📝 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
| if (cache != null && !cache.containsKey(key)) { | |
| cache.putOnRead(key, entry); | |
| if (log.isDebugEnabled()) { | |
| if (cache != null) { | |
| cache.putOnRead(key, entry); | |
| if (log.isDebugEnabled()) { | |
| log.debug(String.format("Cache: %s which is under %s, added the entry: %s for the key: " + | |
| "%s successfully", cacheName, GROUP_ID_RESOLVER_CACHE_MANAGER, entry, key)); | |
| } | |
| } |
🤖 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/GroupIdResolverCache.java` around lines 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. 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
| PermissionTreeUtil.toComponenets(resourceId)); | ||
| if (sr.getLastNodeAllowedAccess()) { | ||
| authorizationCache.addToCache(cacheIdentifier, tenantId, userName, resourceId, action, true); | ||
| authorizationCache.addToCacheOnRead(cacheIdentifier, tenantId, userName, resourceId, action, true); |
There was a problem hiding this comment.
Avoid caching fallback-deny results with putOnRead when role checks partially fail.
At Line 373, addToCacheOnRead(..., userAllowed) can persist a transient false decision (from swallowed UserStoreException paths) because put-if-absent prevents later overwrite by a subsequent successful evaluation. The same stickiness also affects the positive write at Line 290 when a stale false is inserted first by another thread. This can cause prolonged incorrect denies.
Suggested fix
@@ - boolean userAllowed = false; + boolean userAllowed = false; + boolean authorizationEvaluationFailed = false; @@ try { roles = userRealm.getUserStoreManager().getRoleListOfUser(userName); } catch (UserStoreException e) { + authorizationEvaluationFailed = true; if (log.isDebugEnabled()) { log.debug("Error getting role list of user : " + userName, e); } } @@ } catch (UserStoreException e) { + authorizationEvaluationFailed = true; if (log.isDebugEnabled()) { log.debug(userName + " user is not in role : " + role, e); } } } } } @@ - authorizationCache.addToCacheOnRead(cacheIdentifier, this.tenantId, userName, resourceId, action, - userAllowed); + if (!authorizationEvaluationFailed || userAllowed) { + authorizationCache.addToCacheOnRead(cacheIdentifier, this.tenantId, userName, resourceId, action, + userAllowed); + }Also applies to: 373-374
🤖 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/authorization/JDBCAuthorizationManager.java` at 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. 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
Comment on lines +2213 to +2214
| public String getPathFromIdOnRead(int pathId) throws RegistryException { | ||
| try { |
There was a problem hiding this comment.
Suggested change
| public String getPathFromIdOnRead(int pathId) throws RegistryException { | |
| try { | |
| public String getPathFromIdOnRead(int pathId) throws RegistryException { | |
| try { |
| @@ -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?
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 { |
Comment on lines +4677 to +4678
| protected void putToUserCacheOnRead(String name, LdapName value) { | ||
| try { |
There was a problem hiding this comment.
Suggested change
| protected void putToUserCacheOnRead(String name, LdapName value) { | |
| try { | |
| protected void putToUserCacheOnRead(String name, LdapName value) { | |
| try { |
| + " 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..
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.
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning