Fix update query not committing issue in RoleDAO. #7610
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...ole.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java
Outdated
Show resolved
Hide resolved
...ole.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java
Show resolved
Hide resolved
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.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | N | This info log is not required here, as it does not provide any useful information and unnecessarily increases the info log count. |
| #### Log Improvement Suggestion No: 2 | N | This info log is not required here, as it does not provide any useful information and unnecessarily increases the info log count. |
WalkthroughPerforms both UM_ID lookups and the insert for addMainRoleToSharedRoleRelationship within a single writable user DB transaction: resolves mainRoleUMId and sharedRoleUMId via prepared statements, validates neither is zero, executes the insert, commits on success, and rolls back on SQLException or validation failure. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant RoleDAO as RoleDAOImpl
participant UserDB as UserDB
Caller->>RoleDAO: addMainRoleToSharedRoleRelationship(mainRole, sharedRole)
RoleDAO->>UserDB: getConnection(write=true)
RoleDAO->>UserDB: setAutoCommit(false)
Note over RoleDAO,UserDB: Begin single transaction for lookups + insert
RoleDAO->>UserDB: prepare/execute -> resolveMainRoleUMId
UserDB-->>RoleDAO: mainRoleUMId
RoleDAO->>UserDB: prepare/execute -> resolveSharedRoleUMId
UserDB-->>RoleDAO: sharedRoleUMId
alt any ID == 0
RoleDAO->>RoleDAO: throw IllegalArgument/DAOError
RoleDAO->>UserDB: rollback()
Note over RoleDAO,UserDB: Transaction rolled back due to invalid IDs
RoleDAO->>UserDB: setAutoCommit(true)
RoleDAO->>UserDB: close()
RoleDAO-->>Caller: throw exception
else IDs valid
RoleDAO->>UserDB: prepare/execute -> INSERT_MAIN_TO_SHARED_ROLE_RELATIONSHIP
UserDB-->>RoleDAO: insert OK
RoleDAO->>UserDB: commit()
Note over RoleDAO,UserDB: Transaction committed
RoleDAO->>UserDB: setAutoCommit(true)
RoleDAO->>UserDB: close()
RoleDAO-->>Caller: return success
end
opt SQLException
RoleDAO->>UserDB: rollback()
RoleDAO-->>Caller: propagate SQLException
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a transaction management issue in the addMainRoleToSharedRoleRelationship method where database changes were not being committed properly. The fix ensures that role relationship data is persisted to the database by enabling write mode on the connection and adding explicit commit/rollback transaction handling.
- Changed database connection mode from read-only to write mode by passing
truetogetUserDBConnection() - Added explicit transaction commit after successful INSERT operation
- Added transaction rollback in the exception handler to maintain data consistency
UdeshAthukorala
changed the title
UdeshAthukorala
force-pushed
the
role-dao
branch
from
5a6b4db to
2f31998
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java (1)
1036-1078: Consider wrapping all database operations in a single transaction boundary.Currently, the SELECT queries (lines 1036-1058) execute outside the transaction boundary of the INSERT operation (lines 1065-1078). If a SQLException occurs during the SELECT operations, no rollback is performed, potentially leaving the connection in an inconsistent state.
For better transactional consistency, consider restructuring to handle all operations within a single try-catch block with rollback:
try (Connection connection = IdentityDatabaseUtil.getUserDBConnection(false)) { + try { try (NamedPreparedStatement stmt = new NamedPreparedStatement(connection, GET_ROLE_UM_ID_BY_UUID)) { stmt.setString(RoleConstants.RoleTableColumns.UM_UUID, mainRoleUUID); ResultSet resultSet = stmt.executeQuery(); while (resultSet.next()) { mainRoleUMId = resultSet.getInt(1); } - } catch (SQLException e) { - String message = "Error while resolving id of role name: %s in the tenantDomain: %s."; - throw new IdentityRoleManagementServerException(RoleConstants.Error.UNEXPECTED_SERVER_ERROR.getCode(), - String.format(message, mainRoleName, mainRoleTenantDomain), e); } try (NamedPreparedStatement stmt = new NamedPreparedStatement(connection, GET_ROLE_UM_ID_BY_UUID)) { stmt.setString(RoleConstants.RoleTableColumns.UM_UUID, sharedRoleUUID); ResultSet resultSet = stmt.executeQuery(); while (resultSet.next()) { sharedRoleUMId = resultSet.getInt(1); } - } catch (SQLException e) { - String message = "Error while resolving id of role name: %s in the tenantDomain: %s."; - throw new IdentityRoleManagementServerException(RoleConstants.Error.UNEXPECTED_SERVER_ERROR.getCode(), - String.format(message, sharedRoleName, sharedRoleTenantDomain), e); } if (mainRoleUMId == 0 || sharedRoleUMId == 0) { String message = "Error while resolving role id."; throw new IdentityRoleManagementServerException(RoleConstants.Error.UNEXPECTED_SERVER_ERROR.getCode(), message); } - try (NamedPreparedStatement preparedStatement = new NamedPreparedStatement(connection, + + try (NamedPreparedStatement preparedStatement = new NamedPreparedStatement(connection, INSERT_MAIN_TO_SHARED_ROLE_RELATIONSHIP)) { preparedStatement.setInt(RoleConstants.RoleTableColumns.UM_SHARED_ROLE_ID, sharedRoleUMId); preparedStatement.setInt(RoleConstants.RoleTableColumns.UM_MAIN_ROLE_ID, mainRoleUMId); preparedStatement.setInt(RoleConstants.RoleTableColumns.UM_SHARED_ROLE_TENANT_ID, sharedRoleTenantId); preparedStatement.setInt(RoleConstants.RoleTableColumns.UM_MAIN_ROLE_TENANT_ID, mainRoleTenantId); preparedStatement.executeUpdate(); IdentityDatabaseUtil.commitUserDBTransaction(connection); - } catch (SQLException e) { + } + } catch (SQLException e) { IdentityDatabaseUtil.rollbackUserDBTransaction(connection); String message = "Error while adding the role relationship of role: %s."; throw new IdentityRoleManagementServerException(RoleConstants.Error.UNEXPECTED_SERVER_ERROR.getCode(), String.format(message, sharedRoleName), e); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java(2 hunks)
🔇 Additional comments (1)
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java (1)
1035-1035: No issues found — the code correctly uses getUserDBConnection(true) for manual transaction management.The review comment's assumption is incorrect. When
shouldApplyTransactionis true,dbConnection.setAutoCommit(false)is called, enabling manual transaction management. The explicitcommitUserDBTransactionandrollbackUserDBTransactioncalls (lines 1072, 1074) are therefore correct and necessary.The codebase pattern is consistent: all methods requiring manual transaction control use
getUserDBConnection(true)paired with explicit commit/rollback. The PR change is sound.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7610 +/- ##
============================================
+ Coverage 50.37% 50.44% +0.06%
- Complexity 19502 19745 +243
============================================
Files 2093 2121 +28
Lines 126898 129898 +3000
Branches 26671 27056 +385
============================================
+ Hits 63927 65525 +1598
- Misses 54834 56090 +1256
- Partials 8137 8283 +146
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
UdeshAthukorala
force-pushed
the
role-dao
branch
from
2f31998 to
def1e50
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java (1)
1044-1054: Close the ResultSet handles with try-with resourcesLine 1044 and Line 1051 allocate ResultSet cursors that stay open until the statements are auto-closed at scope exit. On DB2 and Oracle we have repeatedly hit “maximum open cursors” when ResultSets are not closed explicitly before executing additional statements in the same transaction. Please wrap each
executeQuery()in its own try-with block so we release the cursor immediately after reading it.- ResultSet resultSetForMainRole = statementForGetMainRoleId.executeQuery(); - while (resultSetForMainRole.next()) { - mainRoleUMId = resultSetForMainRole.getInt(1); - } + try (ResultSet resultSetForMainRole = statementForGetMainRoleId.executeQuery()) { + while (resultSetForMainRole.next()) { + mainRoleUMId = resultSetForMainRole.getInt(1); + } + } - ResultSet resultSetForSharedRole = statementForGetSharedRoleId.executeQuery(); - while (resultSetForSharedRole.next()) { - sharedRoleUMId = resultSetForSharedRole.getInt(1); - } + try (ResultSet resultSetForSharedRole = statementForGetSharedRoleId.executeQuery()) { + while (resultSetForSharedRole.next()) { + sharedRoleUMId = resultSetForSharedRole.getInt(1); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-07T06:21:44.448Z
Learnt from: ShanChathusanda93
Repo: wso2/carbon-identity-framework PR: 7596
File: components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/AbstractApplicationAuthenticator.java:133-137
Timestamp: 2025-11-07T06:21:44.448Z
Learning: In the carbon-identity-framework project, when OrganizationManager is used in the authentication framework components (FrameworkServiceComponent and related classes), null checks are not required because OrganizationManager is declared with ReferenceCardinality.MANDATORY in the OSGi component. This means the component will not activate until OrganizationManager is available, providing an architectural guarantee that the service is always present when the code executes.
Applied to files:
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java
UdeshAthukorala
force-pushed
the
role-dao
branch
from
def1e50 to
e5e6bb3
Compare
UdeshAthukorala
force-pushed
the
role-dao
branch
from
e5e6bb3 to
c2c6d12
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java (2)
1033-1074: Transactional fix looks correct - addresses the commit issue.The refactoring properly wraps all operations in a single transaction, ensuring the insert is committed. However, there's a minor inconsistency: the
whileloops for retrieving UM_IDs (lines 1045, 1053) don't warn about unexpected multiple results, unlike similar patterns elsewhere in this class (e.g.,getRoleNameByIDat lines 2726-2731).Consider adding consistency checks:
try (ResultSet resultSetForMainRole = statementForGetMainRoleId.executeQuery()) { + int count = 0; while (resultSetForMainRole.next()) { + count++; + if (count > 1) { + LOG.warn("Multiple UM_IDs found for main role UUID: " + mainRoleUUID); + } mainRoleUMId = resultSetForMainRole.getInt(1); } }
1058-1063: Consider a more descriptive error message.The current message "Error while resolving role id." doesn't indicate which role ID failed resolution (main or shared). A more specific message would aid debugging.
if (mainRoleUMId == 0 || sharedRoleUMId == 0) { - String message = "Error while resolving role id."; + String message = String.format( + "Error while resolving role UM_ID. Main role UUID: %s (UM_ID: %d), " + + "Shared role UUID: %s (UM_ID: %d)", + mainRoleUUID, mainRoleUMId, sharedRoleUUID, sharedRoleUMId); throw new IdentityRoleManagementServerException( RoleConstants.Error.UNEXPECTED_SERVER_ERROR.getCode(), message); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{py,java,ts,tsx,js,jsx,cs,go,rb,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All public methods should have a docstring
Files:
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java
**/*.{py,java,ts,tsx,js,jsx,cs,go,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,java,ts,tsx,js,jsx,cs,go,rb,php,c,cpp,h,hpp}: Comments should start with a space and first letter capitalized
Comments should always end with a period
Files:
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: If there's a string concatenation in a debug log, then havingif (LOG.isDebugEnabled())is mandatory to avoid unnecessary computation
For simple log messages (e.g., static strings or simple variable interpolation), you can useLOG.debugdirectly without the debug check
Files:
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java
**/*.{java,ts,tsx,js,jsx,py,cs,go,php}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{java,ts,tsx,js,jsx,py,cs,go,php}: Scrutinize all user-controlled input for potential SQL Injection, Cross-Site Scripting (XSS), or Command Injection
Ensure that no sensitive user data (e.g., PII, credentials) is being logged or sent in error messages
Files:
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java
**/*.{java,ts,tsx,js,jsx,py,cs,go,php,yml,yaml,json,env,properties,conf}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Search for and eliminate any exposed secrets like API keys, passwords, or private tokens
Files:
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java
🧠 Learnings (1)
📚 Learning: 2025-11-07T06:21:44.448Z
Learnt from: ShanChathusanda93
Repo: wso2/carbon-identity-framework PR: 7596
File: components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/AbstractApplicationAuthenticator.java:133-137
Timestamp: 2025-11-07T06:21:44.448Z
Learning: In the carbon-identity-framework project, when OrganizationManager is used in the authentication framework components (FrameworkServiceComponent and related classes), null checks are not required because OrganizationManager is declared with ReferenceCardinality.MANDATORY in the OSGi component. This means the component will not activate until OrganizationManager is available, providing an architectural guarantee that the service is always present when the code executes.
Applied to files:
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java
🔇 Additional comments (1)
components/role-mgt/org.wso2.carbon.identity.role.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java (1)
95-95: LGTM!Formatting-only change for the static import.
|
3 participants
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
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.