Preserve Connection readOnly state for defaultReadOnly DataSource

Includes DataSourceTransactionManagerTests alignment with main branch.

Closes gh-35743

spring-jdbc/src/main/java/org/springframework/jdbc/datasource/DataSourceTransactionManager.java

@@ -126,6 +126,8 @@ public class DataSourceTransactionManager extends AbstractPlatformTransactionMan
 
 	private boolean enforceReadOnly = false;
 
+	private volatile @Nullable Boolean defaultReadOnly;
+
 
 	/**
 	 * Create a new {@code DataSourceTransactionManager} instance.
@@ -270,13 +272,18 @@ public class DataSourceTransactionManager extends AbstractPlatformTransactionMan
 				if (logger.isDebugEnabled()) {
 					logger.debug("Acquired Connection [" + newCon + "] for JDBC transaction");
 				}
+				if (definition.isReadOnly()) {
+					checkDefaultReadOnly(newCon);
+				}
 				txObject.setConnectionHolder(new ConnectionHolder(newCon), true);
 			}
 
 			txObject.getConnectionHolder().setSynchronizedWithTransaction(true);
 			con = txObject.getConnectionHolder().getConnection();
 
-			Integer previousIsolationLevel = DataSourceUtils.prepareConnectionForTransaction(con, definition);
+			Integer previousIsolationLevel = DataSourceUtils.prepareConnectionForTransaction(con,
+					definition.getIsolationLevel(),
+					(definition.isReadOnly() && !isDefaultReadOnly()));
 			txObject.setPreviousIsolationLevel(previousIsolationLevel);
 			txObject.setReadOnly(definition.isReadOnly());
 
@@ -381,8 +388,9 @@ public class DataSourceTransactionManager extends AbstractPlatformTransactionMan
 			if (txObject.isMustRestoreAutoCommit()) {
 				con.setAutoCommit(true);
 			}
-			DataSourceUtils.resetConnectionAfterTransaction(
+			DataSourceUtils.resetConnectionAfterTransaction(con,
-					con, txObject.getPreviousIsolationLevel(), txObject.isReadOnly());
+					txObject.getPreviousIsolationLevel(),
+					(txObject.isReadOnly() && !isDefaultReadOnly()));
 		}
 		catch (Throwable ex) {
 			logger.debug("Could not reset JDBC Connection after transaction", ex);
@@ -399,6 +407,37 @@ public class DataSourceTransactionManager extends AbstractPlatformTransactionMan
 	}
 
 
+	/**
+	 * Check the default {@link Connection#isReadOnly()} flag on a freshly
+	 * obtained connection from the {@code DataSource}, assuming that the
+	 * same flag applies to all connections obtained from the given setup.
+	 * @param newCon the Connection to check
+	 * @since 6.2.13
+	 * @see #isDefaultReadOnly()
+	 */
+	private void checkDefaultReadOnly(Connection newCon) {
+		if (this.defaultReadOnly == null) {
+			try {
+				this.defaultReadOnly = newCon.isReadOnly();
+			}
+			catch (Throwable ex) {
+				logger.debug("Could not determine default JDBC Connection isReadOnly - assuming false", ex);
+				this.defaultReadOnly = false;
+			}
+		}
+	}
+
+	/**
+	 * Check whether the default read-only flag has been determined as {@code true},
+	 * assuming that all encountered connections will be read-only by default and
+	 * therefore do not need explicit {@link Connection#setReadOnly} (re)setting.
+	 * @since 6.2.13
+	 * @see #checkDefaultReadOnly(Connection)
+	 */
+	private boolean isDefaultReadOnly() {
+		return (this.defaultReadOnly == Boolean.TRUE);
+	}
+
 	/**
 	 * Prepare the transactional {@code Connection} right after transaction begin.
 	 * <p>The default implementation executes a "SET TRANSACTION READ ONLY" statement

spring-jdbc/src/main/java/org/springframework/jdbc/datasource/DataSourceUtils.java

@@ -170,19 +170,38 @@ public abstract class DataSourceUtils {
 	 * @param definition the transaction definition to apply
 	 * @return the previous isolation level, if any
 	 * @throws SQLException if thrown by JDBC methods
-	 * @see #resetConnectionAfterTransaction
+	 * @see #prepareConnectionForTransaction(Connection, int, boolean)
+	 */
+	@Nullable
+	public static Integer prepareConnectionForTransaction(Connection con, @Nullable TransactionDefinition definition)
+			throws SQLException {
+
+		return prepareConnectionForTransaction(con,
+				(definition != null ? definition.getIsolationLevel() : TransactionDefinition.ISOLATION_DEFAULT),
+				(definition != null && definition.isReadOnly()));
+	}
+
+	/**
+	 * Prepare the given Connection with the given transaction semantics.
+	 * @param con the Connection to prepare
+	 * @param isolationLevel the isolation level to apply
+	 * @param setReadOnly whether to set the read-only flag
+	 * @return the previous isolation level, if any
+	 * @throws SQLException if thrown by JDBC methods
+	 * @since 6.2.13
+	 * @see #resetConnectionAfterTransaction(Connection, Integer, boolean)
 	 * @see Connection#setTransactionIsolation
 	 * @see Connection#setReadOnly
 	 */
 	@Nullable
-	public static Integer prepareConnectionForTransaction(Connection con, @Nullable TransactionDefinition definition)
+	static Integer prepareConnectionForTransaction(Connection con, int isolationLevel, boolean setReadOnly)
 			throws SQLException {
 
 		Assert.notNull(con, "No Connection specified");
 
 		boolean debugEnabled = logger.isDebugEnabled();
 		// Set read-only flag.
-		if (definition != null && definition.isReadOnly()) {
+		if (setReadOnly) {
 			try {
 				if (debugEnabled) {
 					logger.debug("Setting JDBC Connection [" + con + "] read-only");
@@ -205,15 +224,14 @@ public abstract class DataSourceUtils {
 
 		// Apply specific isolation level, if any.
 		Integer previousIsolationLevel = null;
-		if (definition != null && definition.getIsolationLevel() != TransactionDefinition.ISOLATION_DEFAULT) {
+		if (isolationLevel != TransactionDefinition.ISOLATION_DEFAULT) {
 			if (debugEnabled) {
-				logger.debug("Changing isolation level of JDBC Connection [" + con + "] to " +
+				logger.debug("Changing isolation level of JDBC Connection [" + con + "] to " + isolationLevel);
-						definition.getIsolationLevel());
 			}
 			int currentIsolation = con.getTransactionIsolation();
-			if (currentIsolation != definition.getIsolationLevel()) {
+			if (currentIsolation != isolationLevel) {
 				previousIsolationLevel = currentIsolation;
-				con.setTransactionIsolation(definition.getIsolationLevel());
+				con.setTransactionIsolation(isolationLevel);
 			}
 		}
 

spring-jdbc/src/main/java/org/springframework/jdbc/datasource/LazyConnectionDataSourceProxy.java

@@ -153,6 +153,9 @@ public class LazyConnectionDataSourceProxy extends DelegatingDataSource {
 	 */
 	public void setReadOnlyDataSource(@Nullable DataSource readOnlyDataSource) {
 		this.readOnlyDataSource = readOnlyDataSource;
+		if (getTargetDataSource() == null) {
+			setTargetDataSource(readOnlyDataSource);
+		}
 	}
 
 	/**
@@ -395,7 +398,7 @@ public class LazyConnectionDataSourceProxy extends DelegatingDataSource {
 						return null;
 					}
 					case "isReadOnly" -> {
-						return this.readOnly;
+						return (this.readOnly || getTargetDataSource() == readOnlyDataSource);
 					}
 					case "setReadOnly" -> {
 						this.readOnly = (Boolean) args[0];

spring-jdbc/src/test/java/org/springframework/jdbc/support/JdbcTransactionManagerTests.java

@@ -25,9 +25,7 @@ import org.mockito.InOrder;
 
 import org.springframework.dao.ConcurrencyFailureException;
 import org.springframework.jdbc.datasource.DataSourceTransactionManagerTests;
-import org.springframework.transaction.TransactionStatus;
 import org.springframework.transaction.TransactionSystemException;
-import org.springframework.transaction.support.TransactionCallbackWithoutResult;
 import org.springframework.transaction.support.TransactionSynchronizationManager;
 import org.springframework.transaction.support.TransactionTemplate;
 
@@ -53,17 +51,14 @@ class JdbcTransactionManagerTests extends DataSourceTransactionManagerTests {
 
 	@Override
 	@Test
-	protected void testTransactionWithExceptionOnCommit() throws Exception {
+	protected void transactionWithExceptionOnCommit() throws Exception {
 		willThrow(new SQLException("Cannot commit")).given(con).commit();
 		TransactionTemplate tt = new TransactionTemplate(tm);
 
 		// plain TransactionSystemException
 		assertThatExceptionOfType(TransactionSystemException.class).isThrownBy(() ->
-				tt.execute(new TransactionCallbackWithoutResult() {
+				tt.executeWithoutResult(status -> {
-				@Override
-				protected void doInTransactionWithoutResult(TransactionStatus status) {
 					// something transactional
-				}
 				}));
 
 		assertThat(TransactionSynchronizationManager.hasResource(ds)).isFalse();
@@ -71,18 +66,15 @@ class JdbcTransactionManagerTests extends DataSourceTransactionManagerTests {
 	}
 
 	@Test
-	void testTransactionWithDataAccessExceptionOnCommit() throws Exception {
+	void transactionWithDataAccessExceptionOnCommit() throws Exception {
 		willThrow(new SQLException("Cannot commit")).given(con).commit();
 		((JdbcTransactionManager) tm).setExceptionTranslator((task, sql, ex) -> new ConcurrencyFailureException(task));
 		TransactionTemplate tt = new TransactionTemplate(tm);
 
 		// specific ConcurrencyFailureException
 		assertThatExceptionOfType(ConcurrencyFailureException.class).isThrownBy(() ->
-				tt.execute(new TransactionCallbackWithoutResult() {
+				tt.executeWithoutResult(status -> {
-					@Override
-					protected void doInTransactionWithoutResult(TransactionStatus status) {
 					// something transactional
-					}
 				}));
 
 		assertThat(TransactionSynchronizationManager.hasResource(ds)).isFalse();
@@ -90,17 +82,14 @@ class JdbcTransactionManagerTests extends DataSourceTransactionManagerTests {
 	}
 
 	@Test
-	void testTransactionWithDataAccessExceptionOnCommitFromLazyExceptionTranslator() throws Exception {
+	void transactionWithDataAccessExceptionOnCommitFromLazyExceptionTranslator() throws Exception {
 		willThrow(new SQLException("Cannot commit", "40")).given(con).commit();
 		TransactionTemplate tt = new TransactionTemplate(tm);
 
 		// specific ConcurrencyFailureException
 		assertThatExceptionOfType(ConcurrencyFailureException.class).isThrownBy(() ->
-				tt.execute(new TransactionCallbackWithoutResult() {
+				tt.executeWithoutResult(status -> {
-					@Override
-					protected void doInTransactionWithoutResult(TransactionStatus status) {
 					// something transactional
-					}
 				}));
 
 		assertThat(TransactionSynchronizationManager.hasResource(ds)).isFalse();
@@ -109,7 +98,7 @@ class JdbcTransactionManagerTests extends DataSourceTransactionManagerTests {
 
 	@Override
 	@Test
-	protected void testTransactionWithExceptionOnCommitAndRollbackOnCommitFailure() throws Exception {
+	protected void transactionWithExceptionOnCommitAndRollbackOnCommitFailure() throws Exception {
 		willThrow(new SQLException("Cannot commit")).given(con).commit();
 
 		tm.setRollbackOnCommitFailure(true);
@@ -117,11 +106,8 @@ class JdbcTransactionManagerTests extends DataSourceTransactionManagerTests {
 
 		// plain TransactionSystemException
 		assertThatExceptionOfType(TransactionSystemException.class).isThrownBy(() ->
-				tt.execute(new TransactionCallbackWithoutResult() {
+				tt.executeWithoutResult(status -> {
-				@Override
-				protected void doInTransactionWithoutResult(TransactionStatus status) {
 					// something transactional
-				}
 				}));
 
 		assertThat(TransactionSynchronizationManager.hasResource(ds)).isFalse();
@@ -131,16 +117,14 @@ class JdbcTransactionManagerTests extends DataSourceTransactionManagerTests {
 
 	@Override
 	@Test
-	protected void testTransactionWithExceptionOnRollback() throws Exception {
+	protected void transactionWithExceptionOnRollback() throws Exception {
 		given(con.getAutoCommit()).willReturn(true);
 		willThrow(new SQLException("Cannot rollback")).given(con).rollback();
 		TransactionTemplate tt = new TransactionTemplate(tm);
 
 		// plain TransactionSystemException
 		assertThatExceptionOfType(TransactionSystemException.class).isThrownBy(() ->
-				tt.execute(new TransactionCallbackWithoutResult() {
+				tt.executeWithoutResult(status -> {
-				@Override
-				protected void doInTransactionWithoutResult(TransactionStatus status) throws RuntimeException {
 					assertThat(status.getTransactionName()).isEmpty();
 					assertThat(status.hasTransaction()).isTrue();
 					assertThat(status.isNewTransaction()).isTrue();
@@ -151,7 +135,6 @@ class JdbcTransactionManagerTests extends DataSourceTransactionManagerTests {
 					status.setRollbackOnly();
 					assertThat(status.isRollbackOnly()).isTrue();
 					assertThat(status.isCompleted()).isFalse();
-				}
 				}));
 
 		assertThat(TransactionSynchronizationManager.hasResource(ds)).isFalse();
@@ -163,7 +146,7 @@ class JdbcTransactionManagerTests extends DataSourceTransactionManagerTests {
 	}
 
 	@Test
-	void testTransactionWithDataAccessExceptionOnRollback() throws Exception {
+	void transactionWithDataAccessExceptionOnRollback() throws Exception {
 		given(con.getAutoCommit()).willReturn(true);
 		willThrow(new SQLException("Cannot rollback")).given(con).rollback();
 		((JdbcTransactionManager) tm).setExceptionTranslator((task, sql, ex) -> new ConcurrencyFailureException(task));
@@ -171,12 +154,7 @@ class JdbcTransactionManagerTests extends DataSourceTransactionManagerTests {
 
 		// specific ConcurrencyFailureException
 		assertThatExceptionOfType(ConcurrencyFailureException.class).isThrownBy(() ->
-				tt.execute(new TransactionCallbackWithoutResult() {
+				tt.executeWithoutResult(status -> status.setRollbackOnly()));
-					@Override
-					protected void doInTransactionWithoutResult(TransactionStatus status) throws RuntimeException {
-						status.setRollbackOnly();
-					}
-				}));
 
 		assertThat(TransactionSynchronizationManager.hasResource(ds)).isFalse();
 		InOrder ordered = inOrder(con);
@@ -187,16 +165,14 @@ class JdbcTransactionManagerTests extends DataSourceTransactionManagerTests {
 	}
 
 	@Test
-	void testTransactionWithDataAccessExceptionOnRollbackFromLazyExceptionTranslator() throws Exception {
+	void transactionWithDataAccessExceptionOnRollbackFromLazyExceptionTranslator() throws Exception {
 		given(con.getAutoCommit()).willReturn(true);
 		willThrow(new SQLException("Cannot rollback", "40")).given(con).rollback();
 		TransactionTemplate tt = new TransactionTemplate(tm);
 
 		// specific ConcurrencyFailureException
 		assertThatExceptionOfType(ConcurrencyFailureException.class).isThrownBy(() ->
-				tt.execute(new TransactionCallbackWithoutResult() {
+				tt.executeWithoutResult(status -> {
-					@Override
-					protected void doInTransactionWithoutResult(TransactionStatus status) throws RuntimeException {
 					assertThat(status.getTransactionName()).isEmpty();
 					assertThat(status.hasTransaction()).isTrue();
 					assertThat(status.isNewTransaction()).isTrue();
@@ -207,7 +183,6 @@ class JdbcTransactionManagerTests extends DataSourceTransactionManagerTests {
 					status.setRollbackOnly();
 					assertThat(status.isRollbackOnly()).isTrue();
 					assertThat(status.isCompleted()).isFalse();
-					}
 				}));
 
 		assertThat(TransactionSynchronizationManager.hasResource(ds)).isFalse();