IDEMPIERE-5262 Implement readonly protection for DB.getSQLValueEx call (#1287)

* IDEMPIERE-5262 Implement readonly protection for DB.getSQLValueEx call

* IDEMPIERE-5262 Implement readonly protection for DB.getSQLValueEx call

org.adempiere.base/src/org/compiere/model/MIssue.java

@@ -55,10 +55,10 @@ public class MIssue extends X_AD_Issue
 	{
 		if (s_log.isLoggable(Level.CONFIG))
 			s_log.config(record.getMessage());
+		if (!DB.isConnected(false))
+			return null;
 		MSystem system = MSystem.get(Env.getCtx()); 
-		if (!DB.isConnected(false)
+		if (system == null || !system.isAutoErrorReport())
-			|| system == null
-			|| !system.isAutoErrorReport())
 			return null;
 		//
 		MIssue issue = new MIssue(record);

org.adempiere.base/src/org/compiere/util/DB.java

@@ -727,11 +727,7 @@ public final class DB
 	 */
 	public static CPreparedStatement prepareStatement (String sql)
 	{
-		int concurrency = ResultSet.CONCUR_READ_ONLY;
+		return prepareStatement(sql, ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY, null);
-		String upper = sql.toUpperCase();
-		if (upper.startsWith("UPDATE ") || upper.startsWith("DELETE "))
-			concurrency = ResultSet.CONCUR_UPDATABLE;
-		return prepareStatement(sql, ResultSet.TYPE_FORWARD_ONLY, concurrency, null);
 	}	//	prepareStatement
 
 	/**
@@ -742,11 +738,7 @@ public final class DB
 	 */
 	public static CPreparedStatement prepareStatement (String sql, String trxName)
 	{
-		int concurrency = ResultSet.CONCUR_READ_ONLY;
+		return prepareStatement(sql, ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY, trxName);
-		String upper = sql.toUpperCase();
-		if (upper.startsWith("UPDATE ") || upper.startsWith("DELETE "))
-			concurrency = ResultSet.CONCUR_UPDATABLE;
-		return prepareStatement(sql, ResultSet.TYPE_FORWARD_ONLY, concurrency, trxName);
 	}	//	prepareStatement
 
 	/**
@@ -1277,8 +1269,18 @@ public final class DB
     	int retValue = -1;
     	PreparedStatement pstmt = null;
     	ResultSet rs = null;
+    	Trx trx = null; 
+    	if (trxName == null)
+    	{
+    		trxName = Trx.createTrxName("getSQLValueEx");
+    		trx = Trx.get(trxName, true);    		
+    	}
     	try
     	{
+    		if (trx != null)
+    		{
+    			trx.getConnection().setReadOnly(true);
+    		}
     		pstmt = prepareStatement(sql, trxName);
     		setParameters(pstmt, params);
     		rs = pstmt.executeQuery();
@@ -1295,6 +1297,10 @@ public final class DB
     	{
     		close(rs, pstmt);
     		rs = null; pstmt = null;
+    		if (trx != null)
+    		{
+    			trx.close();
+    		}
     	}
     	return retValue;
     }
@@ -1358,8 +1364,18 @@ public final class DB
     	String retValue = null;
     	PreparedStatement pstmt = null;
     	ResultSet rs = null;
+    	Trx trx = null; 
+    	if (trxName == null)
+    	{
+    		trxName = Trx.createTrxName("getSQLValueEx");
+    		trx = Trx.get(trxName, true);    		
+    	}
     	try
     	{
+    		if (trx != null)
+    		{
+    			trx.getConnection().setReadOnly(true);
+    		}
     		pstmt = prepareStatement(sql, trxName);
     		setParameters(pstmt, params);
     		rs = pstmt.executeQuery();
@@ -1376,6 +1392,10 @@ public final class DB
     	{
     		close(rs, pstmt);
     		rs = null; pstmt = null;
+    		if (trx != null)
+    		{
+    			trx.close();
+    		}
     	}
     	return retValue;
     }
@@ -1439,8 +1459,18 @@ public final class DB
     	BigDecimal retValue = null;
     	PreparedStatement pstmt = null;
     	ResultSet rs = null;
+    	Trx trx = null; 
+    	if (trxName == null)
+    	{
+    		trxName = Trx.createTrxName("getSQLValueEx");
+    		trx = Trx.get(trxName, true);    		
+    	}
     	try
     	{
+    		if (trx != null)
+    		{
+    			trx.getConnection().setReadOnly(true);
+    		}
     		pstmt = prepareStatement(sql, trxName);
     		setParameters(pstmt, params);
     		rs = pstmt.executeQuery();
@@ -1458,6 +1488,10 @@ public final class DB
     	{
     		close(rs, pstmt);
     		rs = null; pstmt = null;
+    		if (trx != null)
+    		{
+    			trx.close();
+    		}
     	}
     	return retValue;
     }
@@ -1522,8 +1556,18 @@ public final class DB
     	Timestamp retValue = null;
     	PreparedStatement pstmt = null;
     	ResultSet rs = null;
+    	Trx trx = null; 
+    	if (trxName == null)
+    	{
+    		trxName = Trx.createTrxName("getSQLValueEx");
+    		trx = Trx.get(trxName, true);    		
+    	}
     	try
     	{
+    		if (trx != null)
+    		{
+    			trx.getConnection().setReadOnly(true);
+    		}
     		pstmt = prepareStatement(sql, trxName);
     		setParameters(pstmt, params);
     		rs = pstmt.executeQuery();
@@ -1540,6 +1584,10 @@ public final class DB
     	{
     		close(rs, pstmt);
     		rs = null; pstmt = null;
+    		if (trx != null)
+    		{
+    			trx.close();
+    		}
     	}
     	return retValue;
     }
@@ -2512,11 +2560,7 @@ public final class DB
 	 *  @return Prepared Statement (from replica if possible, otherwise normal statement)
 	 */
 	public static PreparedStatement prepareNormalReadReplicaStatement(String sql, String trxName) {
-		int concurrency = ResultSet.CONCUR_READ_ONLY;
+		return prepareNormalReadReplicaStatement(sql, ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY, trxName);
-		String upper = sql.toUpperCase();
-		if (upper.startsWith("UPDATE ") || upper.startsWith("DELETE "))
-			concurrency = ResultSet.CONCUR_UPDATABLE;
-		return prepareNormalReadReplicaStatement(sql, ResultSet.TYPE_FORWARD_ONLY, concurrency, trxName);
 	}
 
 	/**

org.adempiere.base/src/org/compiere/util/Trx.java

@@ -496,6 +496,18 @@ public class Trx
 		}
 		finally
 		{
+			//ensure connection return to pool with readonly=false
+			try 
+			{
+				if (m_connection.isReadOnly())
+				{
+					m_connection.setReadOnly(false);
+				}
+			}
+			catch (SQLException e)
+			{
+				log.log(Level.SEVERE, m_trxName, e);
+			}	
 			try
 			{
 				m_connection.close();

org.compiere.db.oracle.provider/src/org/compiere/db/DB_Oracle.java

@@ -889,6 +889,17 @@ public class DB_Oracle implements AdempiereDatabase
         if (log.isLoggable(Level.CONFIG)) log.config(toString());
         if (m_ds != null)
         {
+        	try 
+			{
+				//wait 5 seconds if pool is still busy
+				if (m_ds.getNumBusyConnections() > 0)
+				{
+					Thread.sleep(5 * 1000);
+				}
+			} catch (Exception e)
+			{
+				e.printStackTrace();
+			}
             try
             {
                 m_ds.close();

org.compiere.db.postgresql.provider/src/org/compiere/db/DB_PostgreSQL.java

@@ -903,6 +903,18 @@ public class DB_PostgreSQL implements AdempiereDatabase
 
 		if (m_ds != null)
 		{
+			try 
+			{
+				//wait 5 seconds if pool is still busy
+				if (m_ds.getNumBusyConnections() > 0)
+				{
+					Thread.sleep(5 * 1000);
+				}
+			} catch (Exception e)
+			{
+				e.printStackTrace();
+			}
+			
 			try
 			{
 				m_ds.close();

org.idempiere.test/src/org/idempiere/test/AbstractTestCase.java

@@ -148,8 +148,9 @@ public abstract class AbstractTestCase {
 	 * tear down for each test method
 	 */
 	protected void tearDown() {
-		if (trx != null && trx.isActive()) {
+		if (trx != null) {
-			trx.rollback();
+			if (trx.isActive())
+				trx.rollback();
 			trx.close();
 		}
 	}

org.idempiere.test/src/org/idempiere/test/base/DBTest.java

@@ -50,6 +50,20 @@ public class DBTest extends AbstractTestCase
 		assertThrows(DBException.class, () -> {
 			DB.getSQLValueEx(null, "SELECT 10 FROM INEXISTENT_TABLE");
 		});
+		
+		int t_integer = DB.getSQLValueEx(null, "select t_integer from test where test_id=?", 103);
+		assertThrows(DBException.class, () -> {
+			DB.getSQLValueEx(null, "update test set t_integer=1 where test_id=?", 103);
+		});
+		int t_integer1 = DB.getSQLValueEx(null, "select t_integer from test where test_id=?", 103);
+		assertEquals(t_integer, t_integer1, "test.t_integer wrongly updated");
+		
+		assertThrows(DBException.class, () -> {
+			DB.getSQLValueEx(getTrxName(), "update test set t_integer=1 where test_id=?;select t_integer from test where test_id=?", 103);
+		});
+		rollback();
+		t_integer1 = DB.getSQLValueEx(null, "select t_integer from test where test_id=?", 103);
+		assertEquals(t_integer, t_integer1, "test.t_integer wrongly updated");
 	}
 	
 	@Test