Refactor cache operations to use read-safe methods for concurrent environments#3129
Merged
ThaminduR merged 13 commits intowso2-extensions:masterfrom Mar 25, 2026
Merged
Refactor cache operations to use read-safe methods for concurrent environments#3129ThaminduR merged 13 commits intowso2-extensions:masterfrom
ThaminduR merged 13 commits intowso2-extensions:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces many cache-write calls on DB-read paths with read-specific APIs ( Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller (service / validator)
participant Cache as Cache Layer
participant DAO as Database / DAO
participant Session as SessionDataStore
rect rgba(0, 150, 136, 0.5)
Note over Caller,DAO: Previous flow (addToCache)
Caller->>Cache: getValueFromCache(key)
Cache-->>Caller: miss
Caller->>DAO: read entity
DAO-->>Caller: return entity
Caller->>Cache: addToCache(key, entity)
Cache->>Cache: store entry (standard write)
Caller-->>Caller: continue
end
rect rgba(33, 150, 243, 0.5)
Note over Caller,DAO: New flow (addToCacheOnRead)
Caller->>Cache: getValueFromCache(key)
Cache-->>Caller: miss
Caller->>DAO: read entity
DAO-->>Caller: return entity
Caller->>Cache: addToCacheOnRead(key, entity)
Cache->>Cache: extract tenant/scope metadata, log read-path
Cache->>Cache: store primary and derived keys (e.g., token-id)
Cache->>Cache: update/invalidate related bindings if needed
Caller-->>Caller: continue
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...arbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java
Show resolved
Hide resolved
...arbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCache.java
Show resolved
Hide resolved
...arbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCache.java
Outdated
Show resolved
Hide resolved
....carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/OAuth2ScopeService.java
Show resolved
Hide resolved
...y.oauth/src/main/java/org/wso2/carbon/identity/oauth2/authz/AuthorizationHandlerManager.java
Show resolved
Hide resolved
...c/main/java/org/wso2/carbon/identity/oauth2/authz/handlers/util/ResponseTypeHandlerUtil.java
Show resolved
Hide resolved
...c/main/java/org/wso2/carbon/identity/oauth2/authz/handlers/util/ResponseTypeHandlerUtil.java
Show resolved
Hide resolved
...ain/java/org/wso2/carbon/identity/oauth2/dao/CacheBackedOAuthUserConsentedScopesDAOImpl.java
Show resolved
Hide resolved
.../org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java
Show resolved
Hide resolved
.../org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java
Show resolved
Hide resolved
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
Outdated
Show resolved
Hide resolved
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
Show resolved
Hide resolved
...va/org/wso2/carbon/identity/oauth2/validators/JDBCPermissionBasedInternalScopeValidator.java
Show resolved
Hide resolved
...auth/src/main/java/org/wso2/carbon/identity/oauth2/validators/jwt/JWKSourceDataProvider.java
Show resolved
Hide resolved
...auth/src/main/java/org/wso2/carbon/identity/oauth2/validators/jwt/JWKSourceDataProvider.java
Show resolved
Hide resolved
...ty.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/cache/OIDCScopeClaimCache.java
Show resolved
Hide resolved
...ain/java/org/wso2/carbon/identity/openidconnect/dao/CacheBackedScopeClaimMappingDAOImpl.java
Show resolved
Hide resolved
...ain/java/org/wso2/carbon/identity/openidconnect/dao/CacheBackedScopeClaimMappingDAOImpl.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java (1)
841-847:⚠️ Potential issue | 🟠 MajorComplete the read-safe refactor for both username-case branches.
Line 842 still uses
addToCache(...)on the same DB-read cache-fill path where Line 845 now usesaddToCacheOnRead(...). In case-sensitive deployments, this bypasses the intended read-safe behavior.Suggested fix
if (isUsernameCaseSensitive) { - OAuthCache.getInstance() - .addToCache(new OAuthCacheKey(clientId + ":" + username), new ClientCredentialDO(username)); + OAuthCache.getInstance() + .addToCacheOnRead(new OAuthCacheKey(clientId + ":" + username), + new ClientCredentialDO(username)); } else { OAuthCache.getInstance().addToCacheOnRead( new OAuthCacheKey(clientId + ":" + username.toLowerCase()),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java` around lines 841 - 847, The branch under isUsernameCaseSensitive currently calls OAuthCache.getInstance().addToCache(...) which bypasses the read-safe API; change it to use the same read-safe call used in the else branch (OAuthCache.getInstance().addToCacheOnRead(...)) so both branches invoke addToCacheOnRead with the appropriate OAuthCacheKey (clientId + ":" + username for case-sensitive, clientId + ":" + username.toLowerCase() for case-insensitive) and the same ClientCredentialDO(username) payload to ensure consistent read-safe cache population in OAuth2Util.components/org.wso2.carbon.identity.oauth.ui/pom.xml (1)
53-55:⚠️ Potential issue | 🟡 MinorRemove duplicate
org.wso2.carbon.identity.oauth.stubdependency declaration.The dependency appears twice in the pom.xml file with identical groupId and artifactId (lines 53–55 and 87–89). Remove one to eliminate the duplication.
♻️ Proposed fix
- <dependency> - <groupId>org.wso2.carbon.identity.inbound.auth.oauth2</groupId> - <artifactId>org.wso2.carbon.identity.oauth.stub</artifactId> - </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth.ui/pom.xml` around lines 53 - 55, Remove the duplicated Maven dependency declaration for org.wso2.carbon.identity.oauth.stub (groupId org.wso2.carbon.identity.inbound.auth.oauth2, artifactId org.wso2.carbon.identity.oauth.stub) so only a single dependency entry remains; locate both occurrences of the dependency in the pom and delete one of the identical <dependency> blocks (keeping the other intact) to eliminate the duplicate.pom.xml (1)
276-279:⚠️ Potential issue | 🟡 MinorRemove duplicate
dependencyManagemententry fororg.wso2.carbon.identity.configuration.mgt.core.This artifact appears twice within the same
dependencyManagementblock (lines 276-279 and 783-786) with identical configuration, causing unnecessary duplication.♻️ Proposed fix
- <dependency> - <groupId>org.wso2.carbon.identity.framework</groupId> - <artifactId>org.wso2.carbon.identity.configuration.mgt.core</artifactId> - <version>${carbon.identity.framework.version}</version> - </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 276 - 279, Remove the duplicate dependencyManagement entry for the artifact org.wso2.carbon.identity.configuration.mgt.core: locate the two identical entries that declare groupId "org.wso2.carbon.identity.framework", artifactId "org.wso2.carbon.identity.configuration.mgt.core" and version "${carbon.identity.framework.version}" and delete one of them so the artifact is declared only once in the dependencyManagement block; ensure the remaining entry preserves the same coordinates and version variable.components/org.wso2.carbon.identity.oauth.stub/pom.xml (1)
93-95:⚠️ Potential issue | 🟡 MinorPin
build-helper-maven-pluginversion in this module.The plugin is missing an explicit version, which Maven should warn about. Add version
3.6.1to ensure deterministic builds.♻️ Proposed fix
<plugin> <groupId>org.codehaus.mojo</groupId> <artifactId>build-helper-maven-plugin</artifactId> + <version>3.6.1</version> <executions>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth.stub/pom.xml` around lines 93 - 95, The build-helper-maven-plugin declaration for groupId org.codehaus.mojo and artifactId build-helper-maven-plugin is missing a version; add a <version>3.6.1</version> element inside that plugin declaration in the pom so the plugin is explicitly pinned (ensure you place the version element alongside the existing groupId/artifactId inside the plugin block).components/org.wso2.carbon.identity.oauth.par/pom.xml (1)
74-80:⚠️ Potential issue | 🟡 MinorRemove duplicated dependency declaration for authentication framework.
org.wso2.carbon.identity.application.authentication.frameworkis declared twice, which matches the effective-model warning and should be deduplicated.Proposed fix
<dependency> <groupId>org.wso2.carbon.identity.framework</groupId> <artifactId>org.wso2.carbon.identity.application.authentication.framework</artifactId> </dependency> - <dependency> - <groupId>org.wso2.carbon.identity.framework</groupId> - <artifactId>org.wso2.carbon.identity.application.authentication.framework</artifactId> - </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth.par/pom.xml` around lines 74 - 80, The POM contains a duplicated dependency for org.wso2.carbon.identity.application.authentication.framework (artifactId org.wso2.carbon.identity.application.authentication.framework under groupId org.wso2.carbon.identity.framework); remove the redundant dependency element so the artifact is declared only once, ensuring only one <dependency> block with that groupId/artifactId remains in the file.
🧹 Nitpick comments (2)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/dao/CacheBackedScopeClaimMappingDAOImpl.java (1)
186-204: Avoid duplicating cache-load helpers; this can drift over time.
loadOIDCScopeClaimsOnRead(...)andloadOIDCScopeClaims(...)now carry almost identical miss/load logic with different insertion calls. Consider extracting a single internal helper to keep null checks, logging, and miss behavior in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/dao/CacheBackedScopeClaimMappingDAOImpl.java` around lines 186 - 204, Both loadOIDCScopeClaimsOnRead(...) and loadOIDCScopeClaims(...) duplicate the same cache-miss, DB-load, null-check and logging behavior; extract a private helper (e.g., ensureOIDCScopeClaimsLoaded) that accepts tenantId and a functional insertion handler (or a boolean flag) to perform the shared logic (null/empty check, debug logs, call to scopeClaimMappingDAOImpl.getScopes, setScopeClaimMapping) and then invoke either oidcScopeClaimCache.addScopeClaimMapOnRead or the other insertion method from that helper; update loadOIDCScopeClaimsOnRead and loadOIDCScopeClaims to call this helper to avoid drift.components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCache.java (1)
78-90: Extract shared binding-cache invalidation logic to avoid path drift.
addToCache(...)andaddToCacheOnRead(...)now duplicate the same scope-binding cache clear loop. Pull it into one private helper so both write paths stay consistent.♻️ Proposed refactor
public void addToCache(OAuthScopeCacheKey key, Scope entry, int tenantId) { @@ - for (ScopeBinding scopeBinding : entry.getScopeBindings()) { - OAuthScopeBindingCache.getInstance().clearCacheEntry(new OAuthScopeBindingCacheKey(scopeBinding - .getBindingType()), tenantId); - } + clearScopeBindingCacheEntries(entry, tenantId); } @@ public void addToCacheOnRead(OAuthScopeCacheKey key, Scope entry, int tenantId) { @@ - for (ScopeBinding scopeBinding : entry.getScopeBindings()) { - OAuthScopeBindingCache.getInstance().clearCacheEntry(new OAuthScopeBindingCacheKey(scopeBinding - .getBindingType()), tenantId); - } + clearScopeBindingCacheEntries(entry, tenantId); } + + private void clearScopeBindingCacheEntries(Scope entry, int tenantId) { + + for (ScopeBinding scopeBinding : entry.getScopeBindings()) { + OAuthScopeBindingCache.getInstance().clearCacheEntry( + new OAuthScopeBindingCacheKey(scopeBinding.getBindingType()), tenantId); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCache.java` around lines 78 - 90, Both addToCache(...) and addToCacheOnRead(...) duplicate the same loop that clears scope-binding cache entries; extract that loop into a private helper method (e.g., clearScopeBindingCacheEntries(Scope entry, int tenantId)) inside OAuthScopeCache and call it from both addToCache and addToCacheOnRead. The helper should iterate over entry.getScopeBindings(), build OAuthScopeBindingCacheKey(scopeBinding.getBindingType()), and call OAuthScopeBindingCache.getInstance().clearCacheEntry(..., tenantId); update both methods to remove the duplicated loop and invoke the new helper to keep invalidation logic consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java`:
- Around line 1054-1069: In
AbstractAuthorizationGrantHandler.addTokenToCacheOnRead, replace the
inconsistent call to oauthCache.addToCache(cacheKey, existingAccessTokenDO) with
oauthCache.addToCacheOnRead(cacheKey, existingAccessTokenDO) so both cache
insertions use addToCacheOnRead (matching the subsequent accessTokenCacheKey
usage and the pattern in ResponseTypeHandlerUtil.addTokenToCacheOnRead); ensure
you keep the creation/usage of OAuthCacheKey accessTokenCacheKey and
existingAccessTokenDO unchanged.
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/dao/CacheBackedScopeClaimMappingDAOImpl.java`:
- Around line 189-190: The null-check currently uses
oidcScopeClaimCacheEntry.getScopeClaimMapping().size() which can NPE if
getScopeClaimMapping() is null; update the conditional in
CacheBackedScopeClaimMappingDAOImpl so it also checks
oidcScopeClaimCacheEntry.getScopeClaimMapping() == null (or uses a null-safe
isEmpty check) before calling size()/isEmpty(), e.g. change the if that
references oidcScopeClaimCacheEntry and getScopeClaimMapping() to include a null
check for getScopeClaimMapping().
---
Outside diff comments:
In `@components/org.wso2.carbon.identity.oauth.par/pom.xml`:
- Around line 74-80: The POM contains a duplicated dependency for
org.wso2.carbon.identity.application.authentication.framework (artifactId
org.wso2.carbon.identity.application.authentication.framework under groupId
org.wso2.carbon.identity.framework); remove the redundant dependency element so
the artifact is declared only once, ensuring only one <dependency> block with
that groupId/artifactId remains in the file.
In `@components/org.wso2.carbon.identity.oauth.stub/pom.xml`:
- Around line 93-95: The build-helper-maven-plugin declaration for groupId
org.codehaus.mojo and artifactId build-helper-maven-plugin is missing a version;
add a <version>3.6.1</version> element inside that plugin declaration in the pom
so the plugin is explicitly pinned (ensure you place the version element
alongside the existing groupId/artifactId inside the plugin block).
In `@components/org.wso2.carbon.identity.oauth.ui/pom.xml`:
- Around line 53-55: Remove the duplicated Maven dependency declaration for
org.wso2.carbon.identity.oauth.stub (groupId
org.wso2.carbon.identity.inbound.auth.oauth2, artifactId
org.wso2.carbon.identity.oauth.stub) so only a single dependency entry remains;
locate both occurrences of the dependency in the pom and delete one of the
identical <dependency> blocks (keeping the other intact) to eliminate the
duplicate.
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java`:
- Around line 841-847: The branch under isUsernameCaseSensitive currently calls
OAuthCache.getInstance().addToCache(...) which bypasses the read-safe API;
change it to use the same read-safe call used in the else branch
(OAuthCache.getInstance().addToCacheOnRead(...)) so both branches invoke
addToCacheOnRead with the appropriate OAuthCacheKey (clientId + ":" + username
for case-sensitive, clientId + ":" + username.toLowerCase() for
case-insensitive) and the same ClientCredentialDO(username) payload to ensure
consistent read-safe cache population in OAuth2Util.
In `@pom.xml`:
- Around line 276-279: Remove the duplicate dependencyManagement entry for the
artifact org.wso2.carbon.identity.configuration.mgt.core: locate the two
identical entries that declare groupId "org.wso2.carbon.identity.framework",
artifactId "org.wso2.carbon.identity.configuration.mgt.core" and version
"${carbon.identity.framework.version}" and delete one of them so the artifact is
declared only once in the dependencyManagement block; ensure the remaining entry
preserves the same coordinates and version variable.
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCache.java`:
- Around line 78-90: Both addToCache(...) and addToCacheOnRead(...) duplicate
the same loop that clears scope-binding cache entries; extract that loop into a
private helper method (e.g., clearScopeBindingCacheEntries(Scope entry, int
tenantId)) inside OAuthScopeCache and call it from both addToCache and
addToCacheOnRead. The helper should iterate over entry.getScopeBindings(), build
OAuthScopeBindingCacheKey(scopeBinding.getBindingType()), and call
OAuthScopeBindingCache.getInstance().clearCacheEntry(..., tenantId); update both
methods to remove the duplicated loop and invoke the new helper to keep
invalidation logic consistent.
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/dao/CacheBackedScopeClaimMappingDAOImpl.java`:
- Around line 186-204: Both loadOIDCScopeClaimsOnRead(...) and
loadOIDCScopeClaims(...) duplicate the same cache-miss, DB-load, null-check and
logging behavior; extract a private helper (e.g., ensureOIDCScopeClaimsLoaded)
that accepts tenantId and a functional insertion handler (or a boolean flag) to
perform the shared logic (null/empty check, debug logs, call to
scopeClaimMappingDAOImpl.getScopes, setScopeClaimMapping) and then invoke either
oidcScopeClaimCache.addScopeClaimMapOnRead or the other insertion method from
that helper; update loadOIDCScopeClaimsOnRead and loadOIDCScopeClaims to call
this helper to avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f75571b3-1762-4499-a4a7-d34d82c001ec
📒 Files selected for processing (28)
components/org.wso2.carbon.identity.discovery/pom.xmlcomponents/org.wso2.carbon.identity.oauth.ciba/pom.xmlcomponents/org.wso2.carbon.identity.oauth.common/pom.xmlcomponents/org.wso2.carbon.identity.oauth.dcr/pom.xmlcomponents/org.wso2.carbon.identity.oauth.extension/pom.xmlcomponents/org.wso2.carbon.identity.oauth.par/pom.xmlcomponents/org.wso2.carbon.identity.oauth.rar/pom.xmlcomponents/org.wso2.carbon.identity.oauth.stub/pom.xmlcomponents/org.wso2.carbon.identity.oauth.ui/pom.xmlcomponents/org.wso2.carbon.identity.oauth/pom.xmlcomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthCache.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeBindingCache.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCache.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/OAuth2ScopeService.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/authz/AuthorizationHandlerManager.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/authz/handlers/util/ResponseTypeHandlerUtil.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/dao/CacheBackedOAuthUserConsentedScopesDAOImpl.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/validators/JDBCPermissionBasedInternalScopeValidator.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/validators/jwt/JWKSourceDataProvider.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/cache/OIDCScopeClaimCache.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/dao/CacheBackedScopeClaimMappingDAOImpl.javacomponents/org.wso2.carbon.identity.oidc.dcr/pom.xmlcomponents/org.wso2.carbon.identity.oidc.session/pom.xmlcomponents/org.wso2.carbon.identity.webfinger/pom.xmlpom.xml
.../org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java
Show resolved
Hide resolved
...ain/java/org/wso2/carbon/identity/openidconnect/dao/CacheBackedScopeClaimMappingDAOImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java (2)
814-816:⚠️ Potential issue | 🟡 MinorThe username cache key still doesn't match the lookup key.
Line 815 reads
clientId + ":" + usernamebeforeusernameis populated, so the lookup key is alwaysclientId:null. TheseaddToCacheOnRead(...)calls write underclientId + ":" + actualUsername, which means the cache warm-up is still ineffective and the DAO path is hit on every call.Also applies to: 834-847
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java` around lines 814 - 816, The cache lookup key is constructed before username is populated so OAuthCache.getInstance().getValueFromCache(new OAuthCacheKey(clientId + ":" + username)) always uses "clientId:null"; change the code so the username is resolved/populated first and then build the OAuthCacheKey (clientId + ":" + username) for the lookup, matching how addToCacheOnRead(...) writes entries; apply the same fix to the similar block around the addToCacheOnRead usage (lines 834-847) so lookups and writes use the identical key construction.
2278-2283:⚠️ Potential issue | 🟠 MajorDon't mix
includeExpiredand active-only cache entries.This method uses the same
OAuthCacheKey(accessTokenIdentifier)for both active-only andincludeExpired=truereads. If an expired/revoked token is loaded once withincludeExpired=true, a later active-only lookup can hit the cache first and return that same object without re-checking state.💡 Suggested guard
- if (!cacheHit & OAuth2Util.isHashDisabled()) { + if (!includeExpired && !cacheHit && OAuth2Util.isHashDisabled()) { OAuthCache.getInstance().addToCacheOnRead(cacheKey, accessTokenDO); if (log.isDebugEnabled()) { log.debug("Access Token Info object was added back to the cache."); } }Also applies to: 2310-2317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java` around lines 2278 - 2283, The cache is being shared between active-only and includeExpired reads causing stale/expired tokens to be returned; modify the cache key usage in the method that calls new OAuthCacheKey(accessTokenIdentifier) (and the similar block at 2310-2317) to include the includeExpired flag (or otherwise namespace keys by includeExpired) so cached entries are segregated, or add an explicit guard before returning a cached AccessTokenDO that verifies the current includeExpired parameter matches the cached entry's expiry/revocation state; update OAuthCacheKey construction (or its equals/hash) and all cache get/put sites in this method to use the new key form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java`:
- Around line 814-816: The cache lookup key is constructed before username is
populated so OAuthCache.getInstance().getValueFromCache(new
OAuthCacheKey(clientId + ":" + username)) always uses "clientId:null"; change
the code so the username is resolved/populated first and then build the
OAuthCacheKey (clientId + ":" + username) for the lookup, matching how
addToCacheOnRead(...) writes entries; apply the same fix to the similar block
around the addToCacheOnRead usage (lines 834-847) so lookups and writes use the
identical key construction.
- Around line 2278-2283: The cache is being shared between active-only and
includeExpired reads causing stale/expired tokens to be returned; modify the
cache key usage in the method that calls new
OAuthCacheKey(accessTokenIdentifier) (and the similar block at 2310-2317) to
include the includeExpired flag (or otherwise namespace keys by includeExpired)
so cached entries are segregated, or add an explicit guard before returning a
cached AccessTokenDO that verifies the current includeExpired parameter matches
the cached entry's expiry/revocation state; update OAuthCacheKey construction
(or its equals/hash) and all cache get/put sites in this method to use the new
key form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8bc72ff-e02d-4542-9ca3-d468233b7f8e
📒 Files selected for processing (6)
components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/AuthzUtil.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/AuthorizationGrantCache.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/validators/JDBCScopeValidator.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/validators/scope/RoleBasedScopeIssuer.java
…urrent environments
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCache.java (1)
72-77: Minor: Javadoc is missing@param tenantId.The parameter
tenantIdis documented inaddToCache()(line 56) but omitted here.📝 Proposed Javadoc fix
/** * Add a cache entry during a READ operation. * * `@param` key Key which cache entry is indexed. * `@param` entry Actual object where cache entry is placed. + * `@param` tenantId Tenant where the cache is placed. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCache.java` around lines 72 - 77, The Javadoc for the read-mode addToCache overload in OAuthScopeCache is missing the `@param` tenantId tag; update the Javadoc above the addToCache method (the variant that accepts tenantId during READ operations) to include a brief `@param` tenantId description (e.g., "tenantId of the tenant the cache entry belongs to") so parameter documentation matches the method signature and the other overload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeBindingCache.java`:
- Around line 66-78: The addToCacheOnRead method in OAuthScopeBindingCache
should be wrapped with the same cache-enabled guard used in OAuthScopeCache:
call IdentityUtil.getIdentityCacheConfig(IDENTITY_CACHE_MANAGER,
OAUTH_SCOPE_CACHE_NAME).isEnabled() and only perform super.addToCacheOnRead(...)
and logging if that returns true; add an import for IdentityUtil and ensure the
constants IDENTITY_CACHE_MANAGER and OAUTH_SCOPE_CACHE_NAME are referenced the
same way as in OAuthScopeCache to match behavior; also expand the method Javadoc
to document the `@param` tenantId parameter.
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCache.java`:
- Around line 87-90: The loop that clears OAuthScopeBindingCache entries in
addToCacheOnRead() should be removed so read-path caching does not invalidate
related binding caches; locate addToCacheOnRead() and delete the for-loop that
iterates entry.getScopeBindings() and calls
OAuthScopeBindingCache.getInstance().clearCacheEntry(new
OAuthScopeBindingCacheKey(scopeBinding.getBindingType()), tenantId), leaving the
invalidation logic only in addToCache() (which is appropriate for writes), or if
needed, add a clear comment explaining why addToCache() alone handles binding
invalidation.
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCache.java`:
- Around line 72-77: The Javadoc for the read-mode addToCache overload in
OAuthScopeCache is missing the `@param` tenantId tag; update the Javadoc above the
addToCache method (the variant that accepts tenantId during READ operations) to
include a brief `@param` tenantId description (e.g., "tenantId of the tenant the
cache entry belongs to") so parameter documentation matches the method signature
and the other overload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34ad5a03-3c18-45b7-b264-a68af0ce420c
📒 Files selected for processing (32)
components/org.wso2.carbon.identity.discovery/pom.xmlcomponents/org.wso2.carbon.identity.oauth.ciba/pom.xmlcomponents/org.wso2.carbon.identity.oauth.common/pom.xmlcomponents/org.wso2.carbon.identity.oauth.dcr/pom.xmlcomponents/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/AuthzUtil.javacomponents/org.wso2.carbon.identity.oauth.extension/pom.xmlcomponents/org.wso2.carbon.identity.oauth.par/pom.xmlcomponents/org.wso2.carbon.identity.oauth.rar/pom.xmlcomponents/org.wso2.carbon.identity.oauth.stub/pom.xmlcomponents/org.wso2.carbon.identity.oauth.ui/pom.xmlcomponents/org.wso2.carbon.identity.oauth/pom.xmlcomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/AuthorizationGrantCache.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthCache.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeBindingCache.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCache.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/OAuth2ScopeService.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/authz/AuthorizationHandlerManager.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/authz/handlers/util/ResponseTypeHandlerUtil.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/dao/CacheBackedOAuthUserConsentedScopesDAOImpl.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/validators/JDBCPermissionBasedInternalScopeValidator.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/validators/JDBCScopeValidator.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/validators/jwt/JWKSourceDataProvider.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/validators/scope/RoleBasedScopeIssuer.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/cache/OIDCScopeClaimCache.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/dao/CacheBackedScopeClaimMappingDAOImpl.javacomponents/org.wso2.carbon.identity.oidc.dcr/pom.xmlcomponents/org.wso2.carbon.identity.oidc.session/pom.xmlcomponents/org.wso2.carbon.identity.webfinger/pom.xmlpom.xml
✅ Files skipped from review due to trivial changes (3)
- components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/validators/jwt/JWKSourceDataProvider.java
- components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/OAuth2ScopeService.java
- components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/authz/AuthorizationHandlerManager.java
🚧 Files skipped from review as they are similar to previous changes (16)
- components/org.wso2.carbon.identity.oauth.common/pom.xml
- components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/dao/CacheBackedOAuthUserConsentedScopesDAOImpl.java
- components/org.wso2.carbon.identity.oauth.ui/pom.xml
- components/org.wso2.carbon.identity.oauth.extension/pom.xml
- components/org.wso2.carbon.identity.discovery/pom.xml
- components/org.wso2.carbon.identity.oauth/pom.xml
- components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthAdminServiceImpl.java
- components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/dao/CacheBackedScopeClaimMappingDAOImpl.java
- components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/cache/OIDCScopeClaimCache.java
- components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/validators/JDBCPermissionBasedInternalScopeValidator.java
- components/org.wso2.carbon.identity.oidc.dcr/pom.xml
- components/org.wso2.carbon.identity.oauth.ciba/pom.xml
- components/org.wso2.carbon.identity.oidc.session/pom.xml
- components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthCache.java
- components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/authz/handlers/util/ResponseTypeHandlerUtil.java
- components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
...dentity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeBindingCache.java
Show resolved
Hide resolved
...arbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCache.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (4)
components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/dao/CacheBackedOAuthUserConsentedScopesDAOImplTest.java (1)
106-130: Consider adding verification thataddToCachewas never called.For consistency with
testGetUserConsentCacheMissUsesAddToCacheOnRead, this test should also verify that the write-path methodaddToCachewas not invoked. Both code paths (cache miss and cache hit with different appId) exercise the same DAO-fetch-then-cache-write logic.♻️ Suggested addition
assertEquals(result, dbConsent, "DB result should be returned when cached entry has different appId."); verify(mockDao).getUserConsentForApplication(USER_ID, APP_ID, TENANT_ID); verify(mockCache).addToCacheOnRead(eq(USER_ID), any(OAuthUserConsentedScopeCacheEntry.class), eq(TENANT_ID)); + verify(mockCache, never()).addToCache(anyString(), any(), anyInt()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/dao/CacheBackedOAuthUserConsentedScopesDAOImplTest.java` around lines 106 - 130, The test testGetUserConsentCacheHitWithDifferentAppIdFetchesFromDb should also assert that the cache write-path was not invoked; after calling daoImpl.getUserConsentForApplication(...) add a verification that mockCache.addToCache(...) was never called (use verify(mockCache, never()).addToCache(...) or equivalent), referencing the OAuthUserConsentedScopeCache mock and the CacheBackedOAuthUserConsentedScopesDAOImpl behavior so the test ensures only addToCacheOnRead is used and addToCache is not invoked for a cache hit with a different appId.components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeBindingCacheTest.java (2)
62-63: Strengthen the retrieval assertion to validate payload correctness.At Line 63, asserting non-null alone can miss wrong-value regressions. Add at least one content assertion (e.g., scope name or binding type) for the retrieved entry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeBindingCacheTest.java` around lines 62 - 63, The test currently only asserts non-null for the result of cache.getValueFromCache in OAuthScopeBindingCacheTest; strengthen it by asserting the retrieved payload matches expected content: after Scope[] retrieved = cache.getValueFromCache(key, TENANT_ID) add an assertion that checks a known property (for example assertEquals(expectedScopeName, retrieved[0].getKey()/getName() or assertEquals(expectedBindingType, retrieved[0].getBinding())) to verify the correct scope name or binding type was cached when addToCacheOnRead was called.
51-64: Isolate singleton-cache mutations to avoid cross-test coupling.
OAuthScopeBindingCacheis shared process-wide, but these tests leave inserted entries behind. That can cause order-dependent failures when other tests reuseTENANT_ID/keys.♻️ Proposed fix
public void testAddToCacheOnReadStoresEntry() { OAuthScopeBindingCache cache = OAuthScopeBindingCache.getInstance(); OAuthScopeBindingCacheKey key = new OAuthScopeBindingCacheKey("ROLE"); @@ - // Should not throw - cache.addToCacheOnRead(key, entry, TENANT_ID); - - // Entry should be retrievable after addToCacheOnRead - Scope[] retrieved = cache.getValueFromCache(key, TENANT_ID); - assertNotNull(retrieved, "Entry stored via addToCacheOnRead should be retrievable from cache."); + try { + cache.addToCacheOnRead(key, entry, TENANT_ID); + Scope[] retrieved = cache.getValueFromCache(key, TENANT_ID); + assertNotNull(retrieved, "Entry stored via addToCacheOnRead should be retrievable from cache."); + } finally { + cache.clearCacheEntry(key, TENANT_ID); + } } @@ public void testAddToCacheOnReadParityWithAddToCache() { OAuthScopeBindingCache cache = OAuthScopeBindingCache.getInstance(); @@ - cache.addToCache(keyWrite, entry, TENANT_ID); - cache.addToCacheOnRead(keyRead, entry, TENANT_ID); - - assertNotNull(cache.getValueFromCache(keyWrite, TENANT_ID), - "Entry added via addToCache should be retrievable."); - assertNotNull(cache.getValueFromCache(keyRead, TENANT_ID), - "Entry added via addToCacheOnRead should be retrievable."); + try { + cache.addToCache(keyWrite, entry, TENANT_ID); + cache.addToCacheOnRead(keyRead, entry, TENANT_ID); + + assertNotNull(cache.getValueFromCache(keyWrite, TENANT_ID), + "Entry added via addToCache should be retrievable."); + assertNotNull(cache.getValueFromCache(keyRead, TENANT_ID), + "Entry added via addToCacheOnRead should be retrievable."); + } finally { + cache.clearCacheEntry(keyWrite, TENANT_ID); + cache.clearCacheEntry(keyRead, TENANT_ID); + } }Also applies to: 73-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeBindingCacheTest.java` around lines 51 - 64, The test mutates the process-wide OAuthScopeBindingCache (obtained via OAuthScopeBindingCache.getInstance()) and leaves entries behind, causing cross-test coupling; fix by isolating mutations: before the test ensure the cache is clean (e.g., call a cache-clear method) and after the test remove the entry you added—use OAuthScopeBindingCache.getInstance() to call the appropriate cleanup method (preferably cache.clearCacheEntry(key, TENANT_ID) or cache.clearCache()/equivalent) surrounding the use of addToCacheOnRead(key, entry, TENANT_ID) and getValueFromCache(key, TENANT_ID) so the singleton has no leftover entries for other tests.components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCacheTest.java (1)
92-93: ClearOAuthScopeCacheentries after each test to reduce suite flakiness.These tests mutate the singleton cache via real
addToCache*calls but don’t clean up inserted keys. Please clear test keys infinallyblocks (or an@AfterMethod) to keep tests hermetic.♻️ Example cleanup pattern
- OAuthScopeCache.getInstance().addToCacheOnRead(key, scope, TENANT_ID); - - verify(mockBindingCache, never()).clearCacheEntry(any(OAuthScopeBindingCacheKey.class), any(Integer.class)); + try { + OAuthScopeCache.getInstance().addToCacheOnRead(key, scope, TENANT_ID); + verify(mockBindingCache, never()).clearCacheEntry(any(OAuthScopeBindingCacheKey.class), any(Integer.class)); + } finally { + OAuthScopeCache.getInstance().clearCacheEntry(key, TENANT_ID); + }Also applies to: 121-122, 150-151, 178-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCacheTest.java` around lines 92 - 93, Tests call OAuthScopeCache.getInstance().addToCacheOnRead/addToCacheOnWrite which mutates a singleton cache but never removes entries; update each test to remove inserted keys in a finally block or add an `@AfterMethod` teardown that calls the cache cleanup API (e.g., OAuthScopeCache.getInstance().clearCacheEntry(key, TENANT_ID) for each key used, or OAuthScopeCache.getInstance().clear() if available) to ensure entries added by OAuthScopeCache.getInstance().addToCacheOnRead and addToCacheOnWrite are cleared after the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeBindingCacheTest.java`:
- Around line 62-63: The test currently only asserts non-null for the result of
cache.getValueFromCache in OAuthScopeBindingCacheTest; strengthen it by
asserting the retrieved payload matches expected content: after Scope[]
retrieved = cache.getValueFromCache(key, TENANT_ID) add an assertion that checks
a known property (for example assertEquals(expectedScopeName,
retrieved[0].getKey()/getName() or assertEquals(expectedBindingType,
retrieved[0].getBinding())) to verify the correct scope name or binding type was
cached when addToCacheOnRead was called.
- Around line 51-64: The test mutates the process-wide OAuthScopeBindingCache
(obtained via OAuthScopeBindingCache.getInstance()) and leaves entries behind,
causing cross-test coupling; fix by isolating mutations: before the test ensure
the cache is clean (e.g., call a cache-clear method) and after the test remove
the entry you added—use OAuthScopeBindingCache.getInstance() to call the
appropriate cleanup method (preferably cache.clearCacheEntry(key, TENANT_ID) or
cache.clearCache()/equivalent) surrounding the use of addToCacheOnRead(key,
entry, TENANT_ID) and getValueFromCache(key, TENANT_ID) so the singleton has no
leftover entries for other tests.
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCacheTest.java`:
- Around line 92-93: Tests call
OAuthScopeCache.getInstance().addToCacheOnRead/addToCacheOnWrite which mutates a
singleton cache but never removes entries; update each test to remove inserted
keys in a finally block or add an `@AfterMethod` teardown that calls the cache
cleanup API (e.g., OAuthScopeCache.getInstance().clearCacheEntry(key, TENANT_ID)
for each key used, or OAuthScopeCache.getInstance().clear() if available) to
ensure entries added by OAuthScopeCache.getInstance().addToCacheOnRead and
addToCacheOnWrite are cleared after the test.
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/dao/CacheBackedOAuthUserConsentedScopesDAOImplTest.java`:
- Around line 106-130: The test
testGetUserConsentCacheHitWithDifferentAppIdFetchesFromDb should also assert
that the cache write-path was not invoked; after calling
daoImpl.getUserConsentForApplication(...) add a verification that
mockCache.addToCache(...) was never called (use verify(mockCache,
never()).addToCache(...) or equivalent), referencing the
OAuthUserConsentedScopeCache mock and the
CacheBackedOAuthUserConsentedScopesDAOImpl behavior so the test ensures only
addToCacheOnRead is used and addToCache is not invoked for a cache hit with a
different appId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f90ed7c-952e-4abe-86f1-793bdfacfe33
📒 Files selected for processing (4)
components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeBindingCacheTest.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/cache/OAuthScopeCacheTest.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/dao/CacheBackedOAuthUserConsentedScopesDAOImplTest.javacomponents/org.wso2.carbon.identity.oauth/src/test/resources/testng.xml
✅ Files skipped from review due to trivial changes (1)
- components/org.wso2.carbon.identity.oauth/src/test/resources/testng.xml
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
approved these changes
Mar 24, 2026
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/23494839538
Malith-19
reviewed
Mar 25, 2026
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/cache/OAuthCache.java
Outdated
Show resolved
Hide resolved
Malith-19
reviewed
Mar 25, 2026
...c/main/java/org/wso2/carbon/identity/oauth2/authz/handlers/util/ResponseTypeHandlerUtil.java
Outdated
Show resolved
Hide resolved
Malith-19
reviewed
Mar 25, 2026
.../org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java
Show resolved
Hide resolved
Malith-19
reviewed
Mar 25, 2026
.../org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java
Outdated
Show resolved
Hide resolved
|
PR builder started |
Malith-19
previously approved these changes
Mar 25, 2026
|
PR builder completed |
jenkins-is-staging
approved these changes
Mar 25, 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/23526197038
ThaminduR
reviewed
Mar 25, 2026
| <Export-Package> | ||
| org.wso2.carbon.identity.oauth.common.*;version="${identity.inbound.auth.oauth.exp.pkg.version}" | ||
| </Export-Package> | ||
| <_contract>!JavaServlet</_contract> |
Contributor
There was a problem hiding this comment.
Do we need these changes
Contributor
Author
There was a problem hiding this comment.
Yes. otherwise the build fails with the following error:
Contracts [...] declare the same packages in their uses: directive: [javax.servlet, ...]
This issue occurs after upgrading the kernel version. It indicates that multiple JARs are attempting to declare themselves as the provider of the same package (javax.servlet), which leads to a conflict in OSGi package resolution.
In this case, the conflicting JARs are:
tomcat-servlet-apiorg.eclipse.equinox.http.service.api
Contributor
Author
There was a problem hiding this comment.
Importantly, this is not a runtime conflict. Instead, it occurs during the build phase, where bnd enforces strict validation while generating the MANIFEST.MF. Due to the ambiguous metadata, bnd refuses to proceed.
ThaminduR
reviewed
Mar 25, 2026
Zeta201
force-pushed
the
fix-26225
branch
2 times, most recently
from
7be4386 to
f579af8
Compare
Malith-19
approved these changes
Mar 25, 2026
ThaminduR
approved these changes
Mar 25, 2026
ThaminduR
added a commit
that referenced
this pull request
Mar 26, 2026
[Sync][master -> next][#3129]: Refactor cache operations to use read-safe methods for concurrent environments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proposed changes in this pull request
When should this PR be merged
[Please describe any preconditions that need to be addressed before we
can merge this pull request.]
Follow up actions
[List any possible follow-up actions here; for instance, testing data
migrations, software that we need to install on staging and production
environments.]
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Checklist (for reviewing)
General
Functionality
Code
Tests
Security
Documentation
Summary by CodeRabbit