BAH-4717|Shilpa|Forcing execution of SqlSearchService read only - #323
BAH-4717|Shilpa|Forcing execution of SqlSearchService read only#323shilpa-iplit wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds ChangesRead-only Transaction Semantics for SQL Search
Test helper consolidation
POM testResources split
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/SqlSearchServiceImpl.java (1)
47-48: 💤 Low valueConsider simplifying to a single try-with-resources statement.
The nested try-with-resources blocks are functionally correct but more verbose than necessary. Java's try-with-resources supports multiple resources in a single declaration, which is more concise and equally safe.
♻️ Simplified try-with-resources structure
try (Connection conn = DatabaseUpdater.getConnection()) { conn.setReadOnly(true); - try (PreparedStatement statement = sqlQueryHelper.constructPreparedStatement(query, mergedParams, conn); - ResultSet resultSet = statement.executeQuery()) { + try (PreparedStatement statement = sqlQueryHelper.constructPreparedStatement(query, mergedParams, conn); + ResultSet resultSet = statement.executeQuery()) { RowMapper rowMapper = new RowMapper(); while (resultSet.next()) { results.add(rowMapper.mapRow(resultSet));Note: The original single-level try with semicolon-separated resources is also valid and even more concise, unless the explicit nesting was intentional for readability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/SqlSearchServiceImpl.java` around lines 47 - 48, In SqlSearchServiceImpl.java, simplify the nested try-with-resources around PreparedStatement and ResultSet into a single try declaration: obtain the PreparedStatement from sqlQueryHelper.constructPreparedStatement(query, mergedParams, conn) and in the same try list the ResultSet from statement.executeQuery(); replace the inner nested try block with a single try (PreparedStatement statement = ...; ResultSet resultSet = statement.executeQuery()) { ... } so both resources are closed automatically; keep the existing variables (query, mergedParams, conn) and existing result processing logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/SqlSearchServiceImpl.java`:
- Around line 45-54: The code in SqlSearchServiceImpl uses a raw JDBC Connection
from DatabaseUpdater.getConnection() and then calls conn.setReadOnly(true),
which means the `@Transactional`(readOnly = true) on the interface method is
ineffective; either remove the `@Transactional` annotation from the service
interface/method to avoid misleading semantics, or refactor SqlSearchServiceImpl
to obtain connections via the Spring-managed DataSource (inject DataSource or
use JdbcTemplate/NamedParameterJdbcTemplate) so Spring transaction management
and `@Transactional`(readOnly = true) actually control the connection; locate
DatabaseUpdater.getConnection(), the conn.setReadOnly(true) call, and the
service method signature (with `@Transactional`) to implement one of these two
fixes consistently across the service.
In
`@bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/SqlSearchService.java`:
- Line 13: The `@Transactional` on SqlSearchService is misleading because
DatabaseUpdater.getConnection() uses DriverManager.getConnection() and bypasses
Spring transactions; remove the `@Transactional`(readOnly = true) annotation from
the method or class and either (a) refactor the code that uses
DatabaseUpdater.getConnection() to obtain connections from Spring-managed
DataSource (e.g., inject DataSource and call dataSource.getConnection()) so
operations participate in transactions, or (b) explicitly manage transactions
around the DriverManager connection; update SqlSearchService and any methods
that call DatabaseUpdater.getConnection() accordingly and add
documentation/comments indicating the chosen approach.
---
Nitpick comments:
In
`@bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/SqlSearchServiceImpl.java`:
- Around line 47-48: In SqlSearchServiceImpl.java, simplify the nested
try-with-resources around PreparedStatement and ResultSet into a single try
declaration: obtain the PreparedStatement from
sqlQueryHelper.constructPreparedStatement(query, mergedParams, conn) and in the
same try list the ResultSet from statement.executeQuery(); replace the inner
nested try block with a single try (PreparedStatement statement = ...; ResultSet
resultSet = statement.executeQuery()) { ... } so both resources are closed
automatically; keep the existing variables (query, mergedParams, conn) and
existing result processing logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b3b0cc7-8426-44e6-8737-37764275856b
📒 Files selected for processing (2)
bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/SqlSearchService.javabahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/SqlSearchServiceImpl.java
| try (Connection conn = DatabaseUpdater.getConnection()) { | ||
| conn.setReadOnly(true); | ||
| try (PreparedStatement statement = sqlQueryHelper.constructPreparedStatement(query, mergedParams, conn); | ||
| ResultSet resultSet = statement.executeQuery()) { | ||
| RowMapper rowMapper = new RowMapper(); | ||
| while (resultSet.next()) { | ||
| results.add(rowMapper.mapRow(resultSet)); | ||
| } | ||
| return results; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other services in the codebase handle transactions with JDBC
rg -n -B5 -A10 'DatabaseUpdater\.getConnection' --type=java
# Look for DataSource usage with `@Transactional` in the codebase
rg -nP -B2 -A8 '`@Transactional`.*\n.*DataSource' --type=java
# Check if there's a pattern of using Spring-managed connections elsewhere
rg -n -C5 'JdbcTemplate|DataSource' --type=java -g '*Service*.java'Repository: Bahmni/bahmni-core
Length of output: 2500
🏁 Script executed:
# Find the DatabaseUpdater class to understand getConnection()
fd -n "DatabaseUpdater.java" --type fRepository: Bahmni/bahmni-core
Length of output: 293
🏁 Script executed:
# Find the interface and method definition for the search method
rg -n "interface SqlSearchService|search\(" bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/SqlSearchServiceImpl.java -B 5 -A 15Repository: Bahmni/bahmni-core
Length of output: 1319
🏁 Script executed:
# Look for `@Transactional` annotation on this specific method
rg -n "`@Transactional`" bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/SqlSearchServiceImpl.java -B 2 -A 5Repository: Bahmni/bahmni-core
Length of output: 44
🏁 Script executed:
# Find DatabaseUpdater class
find . -name "DatabaseUpdater.java" -type fRepository: Bahmni/bahmni-core
Length of output: 44
🏁 Script executed:
# Find SqlSearchService interface definition
find . -name "SqlSearchService.java" -type fRepository: Bahmni/bahmni-core
Length of output: 152
🏁 Script executed:
# Look for class-level `@Transactional` on SqlSearchServiceImpl
rg -B 10 "public class SqlSearchServiceImpl" bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/SqlSearchServiceImpl.javaRepository: Bahmni/bahmni-core
Length of output: 358
🏁 Script executed:
# Read the SqlSearchService interface to check for `@Transactional`
cat bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/SqlSearchService.javaRepository: Bahmni/bahmni-core
Length of output: 498
🏁 Script executed:
# Check if DatabaseUpdater is defined in this repo or if it's from OpenMRS
find . -name "DatabaseUpdater.java" -type fRepository: Bahmni/bahmni-core
Length of output: 44
🏁 Script executed:
# Search for other methods that use DatabaseUpdater.getConnection() to see if this is a pattern
rg -n "DatabaseUpdater" bahmnicore-api/src/main/java --type=javaRepository: Bahmni/bahmni-core
Length of output: 369
🏁 Script executed:
# Check if there are other `@Transactional` methods in the codebase and how they handle connections
rg -n "`@Transactional`" bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/ --type=java -A 3 | head -50Repository: Bahmni/bahmni-core
Length of output: 6251
Address the mismatch between Spring transaction management and actual JDBC connection handling.
The @Transactional(readOnly = true) annotation on the interface method cannot manage the connection obtained via DatabaseUpdater.getConnection(). Since DatabaseUpdater.getConnection() returns a raw JDBC connection from OpenMRS (external utility), Spring's transaction manager has no control over it. The explicit conn.setReadOnly(true) at line 46 is the only enforcement mechanism that actually works, but this creates confusion about transaction semantics:
- Developers reading this code see
@Transactionaland assume Spring is enforcing read-only semantics - The annotation has no actual effect on the database connection
- This pattern is inconsistent with the rest of the codebase, where
@Transactionalproperly manages connections through DAOs
Either remove the @Transactional annotation (since the connection is manually managed), or refactor to obtain the connection through Spring's DataSource so the annotation becomes meaningful.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/SqlSearchServiceImpl.java`
around lines 45 - 54, The code in SqlSearchServiceImpl uses a raw JDBC
Connection from DatabaseUpdater.getConnection() and then calls
conn.setReadOnly(true), which means the `@Transactional`(readOnly = true) on the
interface method is ineffective; either remove the `@Transactional` annotation
from the service interface/method to avoid misleading semantics, or refactor
SqlSearchServiceImpl to obtain connections via the Spring-managed DataSource
(inject DataSource or use JdbcTemplate/NamedParameterJdbcTemplate) so Spring
transaction management and `@Transactional`(readOnly = true) actually control the
connection; locate DatabaseUpdater.getConnection(), the conn.setReadOnly(true)
call, and the service method signature (with `@Transactional`) to implement one of
these two fixes consistently across the service.
|
|
||
| public interface SqlSearchService { | ||
|
|
||
| @Transactional(readOnly = true) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for DataSourceTransactionManager or transaction manager configuration
rg -n -C5 --type=xml 'transactionManager|DataSourceTransactionManager'
# Check DatabaseUpdater.getConnection() implementation
rg -n -C10 'class DatabaseUpdater' --type=java
# Look for how DatabaseUpdater is used with transactions in the codebase
rg -n -C5 'DatabaseUpdater\.getConnection\(\)' --type=java
# Check if there are other services using `@Transactional` with DatabaseUpdater
rg -nP -C3 '`@Transactional`.*DatabaseUpdater' --type=javaRepository: Bahmni/bahmni-core
Length of output: 15362
🌐 Web query:
Does OpenMRS DatabaseUpdater.getConnection() participate in Spring transaction management?
💡 Result:
No, the OpenMRS DatabaseUpdater.getConnection method does not participate in Spring transaction management [1][2]. The method is designed to provide a raw database connection specifically for Liquibase to perform database schema updates [1][3][2]. Its implementation retrieves connection details directly from the OpenMRS runtime properties and establishes a new connection using DriverManager.getConnection [1]: public static Connection getConnection throws Exception { Properties props = Context.getRuntimeProperties; //... retrieves driver, url, username, password from properties... Class.forName(driver); return DriverManager.getConnection(url, username, password); } Because it uses DriverManager directly rather than a Spring-managed DataSource or a transaction-aware utility (such as Spring's DataSourceUtils), any connection obtained through this method operates outside the scope of Spring's transaction synchronization [4]. Consequently, it will not be aware of, nor participate in, any active Spring-managed transactions [4][5].
Citations:
- 1: https://www.javatips.net/api/openmrs-core-master/api/src/main/java/org/openmrs/util/DatabaseUpdater.java
- 2: https://docs.openmrs.org/doc/org/openmrs/util/DatabaseUpdater.html
- 3: https://resources.openmrs.org/doc-1.7/org/openmrs/util/DatabaseUpdater.html
- 4: https://stackoverflow.com/questions/11779469/how-do-i-get-the-connection-inside-of-a-spring-transaction
- 5: https://devmanual.openmrs.org/technology/architecture/
The @Transactional annotation is ineffective with DatabaseUpdater.getConnection().
OpenMRS's DatabaseUpdater.getConnection() uses DriverManager.getConnection() directly and does not participate in Spring's transaction management. Any SQL executed on a connection obtained this way operates outside Spring's transaction scope and will not be aware of or participate in any active Spring-managed transactions. This makes the annotation misleading—it creates the false impression that the method is transaction-managed when the actual database operations are not.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/SqlSearchService.java`
at line 13, The `@Transactional` on SqlSearchService is misleading because
DatabaseUpdater.getConnection() uses DriverManager.getConnection() and bypasses
Spring transactions; remove the `@Transactional`(readOnly = true) annotation from
the method or class and either (a) refactor the code that uses
DatabaseUpdater.getConnection() to obtain connections from Spring-managed
DataSource (e.g., inject DataSource and call dataSource.getConnection()) so
operations participate in transactions, or (b) explicitly manage transactions
around the DriverManager connection; update SqlSearchService and any methods
that call DatabaseUpdater.getConnection() accordingly and add
documentation/comments indicating the chosen approach.
1 participant
Summary by CodeRabbit