Skip to content

BAH-4717|Shilpa|Forcing execution of SqlSearchService read only #323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shilpa-iplit wants to merge 2 commits into
base: master
Choose a base branch
from
Open

BAH-4717|Shilpa|Forcing execution of SqlSearchService read only #323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f6ec2f
BAH-4717|Shilpa|Forcing execution of SqlSearchService read only
shilpa-iplit May 18, 2026
3c23bd2
BAH-4717|Shilpa|Forcing execution of SqlSearchService read only
shilpa-iplit May 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions .../bahmniemrapi/encountertransaction/advice/BahmniEncounterTransactionUpdateAdviceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
import static org.powermock.api.mockito.PowerMockito.when;

import org.apache.commons.lang.StringUtils;
;

import java.io.File;
import java.net.URISyntaxException;

@RunWith(PowerMockRunner.class)
@PrepareForTest({ Context.class, OpenmrsUtil.class})
Expand All @@ -29,10 +31,14 @@ public class BahmniEncounterTransactionUpdateAdviceTest {
@Mock
private AdministrationService administrationService;

private String getTestResourcesDirectory() throws URISyntaxException {
return new File(getClass().getClassLoader().getResource("").toURI()).getAbsolutePath() + File.separator;
}

@Test
public void shouldExecuteObsValueCalculatorFromApplicationDataDirectory() throws Throwable {
PowerMockito.mockStatic(OpenmrsUtil.class);
when(OpenmrsUtil.getApplicationDataDirectory()).thenReturn(getClass().getClassLoader().getResource("").getPath());
when(OpenmrsUtil.getApplicationDataDirectory()).thenReturn(getTestResourcesDirectory());
PowerMockito.mockStatic(Context.class);
when(Context.getAdministrationService()).thenReturn(administrationService);
when(administrationService.getGlobalProperty(BAHMNI_EXECUTE_GROOVY_SCRIPT)).thenReturn("true");
Expand All @@ -46,10 +52,8 @@ public void shouldExecuteObsValueCalculatorFromApplicationDataDirectory() throws
@Test
public void shouldLoadpplicationDataDirectoryPath() throws Throwable {
PowerMockito.mockStatic(OpenmrsUtil.class);
String path = getClass().getClassLoader().getResource("").getPath();
// remove the trailing "/"
String path = getTestResourcesDirectory();
path = StringUtils.chop(path);
System.out.println(path);
when(OpenmrsUtil.getApplicationDataDirectory()).thenReturn(path);
PowerMockito.mockStatic(Context.class);
when(Context.getAdministrationService()).thenReturn(administrationService);
Expand All @@ -64,7 +68,7 @@ public void shouldLoadpplicationDataDirectoryPath() throws Throwable {
@Test
public void shouldNotFailIfobscalculatorDirectoryDoesNotExist() throws Throwable {
PowerMockito.mockStatic(OpenmrsUtil.class);
when(OpenmrsUtil.getApplicationDataDirectory()).thenReturn(getClass().getClassLoader().getResource("").getPath() + "nonExistentDirectory");
when(OpenmrsUtil.getApplicationDataDirectory()).thenReturn(getTestResourcesDirectory() + "nonExistentDirectory");
PowerMockito.mockStatic(Context.class);
when(Context.getAdministrationService()).thenReturn(administrationService);
when(administrationService.getGlobalProperty(BAHMNI_EXECUTE_GROOVY_SCRIPT)).thenReturn("true");
Expand Down
12 changes: 12 additions & 0 deletions bahmnicore-api/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,18 @@
<testResource>
<directory>src/test/resources</directory>
<filtering>true</filtering>
<excludes>
<exclude>**/*.mkv</exclude>
<exclude>**/*.mov</exclude>
</excludes>
</testResource>
<testResource>
<directory>src/test/resources</directory>
<filtering>false</filtering>
<includes>
<include>**/*.mkv</include>
<include>**/*.mov</include>
</includes>
</testResource>
</testResources>
<plugins>
Expand Down
3 changes: 3 additions & 0 deletions bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/SqlSearchService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@
import java.util.List;
import java.util.Map;

import org.springframework.transaction.annotation.Transactional;

public interface SqlSearchService {

@Transactional(readOnly = true)

@coderabbitai coderabbitai Bot May 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

🧩 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=java

Repository: 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:

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.

@Authorized
public List<SimpleObject> search(String sqlQuery, Map<String, String[]> params);

Expand Down
16 changes: 9 additions & 7 deletions ...ore-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/SqlSearchServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,16 @@ public List<SimpleObject> search(String queryId, Map<String, String[]> params)
SqlQueryHelper sqlQueryHelper = new SqlQueryHelper();
String query = getSql(queryId);
debugPrintQueryParams(queryId, mergedParams);
try( Connection conn = DatabaseUpdater.getConnection();
PreparedStatement statement = sqlQueryHelper.constructPreparedStatement(query,mergedParams,conn);
ResultSet resultSet = statement.executeQuery()) {
RowMapper rowMapper = new RowMapper();
while (resultSet.next()) {
results.add(rowMapper.mapRow(resultSet));
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;
}
Comment on lines +45 to 54

@coderabbitai coderabbitai Bot May 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 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 f

Repository: 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 15

Repository: 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 5

Repository: Bahmni/bahmni-core

Length of output: 44

🏁 Script executed:

# Find DatabaseUpdater class
find . -name "DatabaseUpdater.java" -type f

Repository: Bahmni/bahmni-core

Length of output: 44

🏁 Script executed:

# Find SqlSearchService interface definition
find . -name "SqlSearchService.java" -type f

Repository: 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.java

Repository: 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.java

Repository: 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 f

Repository: 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=java

Repository: 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 -50

Repository: 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 @Transactional and 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 @Transactional properly 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.

return results;
} catch (Exception e) {
throw new RuntimeException(e);
}
Expand Down
Loading