From d2906253f131ea11191b1fe94be9c150f4a41b85 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Sat, 3 Jun 2023 17:16:36 +0200 Subject: [PATCH 1/2] Fall back to plain setObject call for non-supported SQL type Closes gh-30556 --- .../jdbc/core/StatementCreatorUtils.java | 31 ++++++++++++++++--- .../jdbc/core/StatementCreatorUtilsTests.java | 21 ++++++++++++- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/spring-jdbc/src/main/java/org/springframework/jdbc/core/StatementCreatorUtils.java b/spring-jdbc/src/main/java/org/springframework/jdbc/core/StatementCreatorUtils.java index a440fe85f06..503a64f93e2 100644 --- a/spring-jdbc/src/main/java/org/springframework/jdbc/core/StatementCreatorUtils.java +++ b/spring-jdbc/src/main/java/org/springframework/jdbc/core/StatementCreatorUtils.java @@ -25,6 +25,7 @@ import java.sql.Clob; import java.sql.DatabaseMetaData; import java.sql.PreparedStatement; import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; import java.sql.Types; import java.time.LocalDate; import java.time.LocalDateTime; @@ -84,7 +85,7 @@ public abstract class StatementCreatorUtils { private static final Log logger = LogFactory.getLog(StatementCreatorUtils.class); - private static final Map, Integer> javaTypeToSqlTypeMap = new HashMap<>(32); + private static final Map, Integer> javaTypeToSqlTypeMap = new HashMap<>(64); static { javaTypeToSqlTypeMap.put(boolean.class, Types.BOOLEAN); @@ -106,8 +107,8 @@ public abstract class StatementCreatorUtils { javaTypeToSqlTypeMap.put(LocalDate.class, Types.DATE); javaTypeToSqlTypeMap.put(LocalTime.class, Types.TIME); javaTypeToSqlTypeMap.put(LocalDateTime.class, Types.TIMESTAMP); - javaTypeToSqlTypeMap.put(OffsetDateTime.class, Types.TIMESTAMP_WITH_TIMEZONE); javaTypeToSqlTypeMap.put(OffsetTime.class, Types.TIME_WITH_TIMEZONE); + javaTypeToSqlTypeMap.put(OffsetDateTime.class, Types.TIMESTAMP_WITH_TIMEZONE); javaTypeToSqlTypeMap.put(java.sql.Date.class, Types.DATE); javaTypeToSqlTypeMap.put(java.sql.Time.class, Types.TIME); javaTypeToSqlTypeMap.put(java.sql.Timestamp.class, Types.TIMESTAMP); @@ -290,7 +291,19 @@ public abstract class StatementCreatorUtils { ps.setNull(paramIndex, sqlType, typeName); } else { - ps.setNull(paramIndex, sqlType); + // Fall back to generic setNull call. + try { + // Try generic setNull call with SQL type specified. + ps.setNull(paramIndex, sqlType); + } + catch (SQLFeatureNotSupportedException ex) { + if (sqlType == Types.NULL) { + throw ex; + } + // Fall back to generic setNull call without SQL type specified + // (e.g. for MySQL TIME_WITH_TIMEZONE / TIMESTAMP_WITH_TIMEZONE). + ps.setNull(paramIndex, Types.NULL); + } } } @@ -415,8 +428,16 @@ public abstract class StatementCreatorUtils { } } else { - // Fall back to generic setObject call with SQL type specified. - ps.setObject(paramIndex, inValue, sqlType); + // Fall back to generic setObject call. + try { + // Try generic setObject call with SQL type specified. + ps.setObject(paramIndex, inValue, sqlType); + } + catch (SQLFeatureNotSupportedException ex) { + // Fall back to generic setObject call without SQL type specified + // (e.g. for MySQL TIME_WITH_TIMEZONE / TIMESTAMP_WITH_TIMEZONE). + ps.setObject(paramIndex, inValue); + } } } diff --git a/spring-jdbc/src/test/java/org/springframework/jdbc/core/StatementCreatorUtilsTests.java b/spring-jdbc/src/test/java/org/springframework/jdbc/core/StatementCreatorUtilsTests.java index 96d8f796e1e..a234449b826 100644 --- a/spring-jdbc/src/test/java/org/springframework/jdbc/core/StatementCreatorUtilsTests.java +++ b/spring-jdbc/src/test/java/org/springframework/jdbc/core/StatementCreatorUtilsTests.java @@ -21,10 +21,12 @@ import java.sql.DatabaseMetaData; import java.sql.ParameterMetaData; import java.sql.PreparedStatement; import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; import java.sql.Types; import java.time.LocalDate; import java.time.LocalDateTime; import java.time.LocalTime; +import java.time.OffsetDateTime; import java.time.ZoneOffset; import java.util.GregorianCalendar; import java.util.stream.Stream; @@ -36,6 +38,7 @@ import org.junit.jupiter.params.provider.MethodSource; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Named.named; +import static org.mockito.BDDMockito.doThrow; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -213,7 +216,6 @@ public class StatementCreatorUtilsTests { verify(preparedStatement).setTimestamp(1, new java.sql.Timestamp(cal.getTime().getTime()), cal); } - @ParameterizedTest @MethodSource("javaTimeTypes") public void testSetParameterValueWithJavaTimeTypes(Object o, int sqlType) throws SQLException { @@ -242,6 +244,23 @@ public class StatementCreatorUtilsTests { ); } + @Test // gh-30556 + public void testSetParameterValueWithOffsetDateTimeAndNotSupported() throws SQLException { + OffsetDateTime time = OffsetDateTime.now(); + doThrow(new SQLFeatureNotSupportedException()).when(preparedStatement).setObject(1, time, Types.TIMESTAMP_WITH_TIMEZONE); + StatementCreatorUtils.setParameterValue(preparedStatement, 1, Types.TIMESTAMP_WITH_TIMEZONE, null, time); + verify(preparedStatement).setObject(1, time, Types.TIMESTAMP_WITH_TIMEZONE); + verify(preparedStatement).setObject(1, time); + } + + @Test // gh-30556 + public void testSetParameterValueWithNullAndNotSupported() throws SQLException { + doThrow(new SQLFeatureNotSupportedException()).when(preparedStatement).setNull(1, Types.TIMESTAMP_WITH_TIMEZONE); + StatementCreatorUtils.setParameterValue(preparedStatement, 1, Types.TIMESTAMP_WITH_TIMEZONE, null, null); + verify(preparedStatement).setNull(1, Types.TIMESTAMP_WITH_TIMEZONE); + verify(preparedStatement).setNull(1, Types.NULL); + } + @Test // SPR-8571 public void testSetParameterValueWithStringAndVendorSpecificType() throws SQLException { Connection con = mock(); From 9751987dc14335c220e9bed9139098fefcf9b9f2 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Sat, 3 Jun 2023 17:17:47 +0200 Subject: [PATCH 2/2] Avoid unnecessary auto-commit check for proper transaction begin Closes gh-30508 --- .../connection/ConnectionFactoryUtils.java | 10 +-- .../connection/R2dbcTransactionManager.java | 67 +++---------------- .../R2dbcTransactionManagerUnitTests.java | 38 +---------- 3 files changed, 18 insertions(+), 97 deletions(-) diff --git a/spring-r2dbc/src/main/java/org/springframework/r2dbc/connection/ConnectionFactoryUtils.java b/spring-r2dbc/src/main/java/org/springframework/r2dbc/connection/ConnectionFactoryUtils.java index 2effb153b8e..16dbd073034 100644 --- a/spring-r2dbc/src/main/java/org/springframework/r2dbc/connection/ConnectionFactoryUtils.java +++ b/spring-r2dbc/src/main/java/org/springframework/r2dbc/connection/ConnectionFactoryUtils.java @@ -87,7 +87,7 @@ public abstract class ConnectionFactoryUtils { */ public static Mono getConnection(ConnectionFactory connectionFactory) { return doGetConnection(connectionFactory) - .onErrorMap(e -> new DataAccessResourceFailureException("Failed to obtain R2DBC Connection", e)); + .onErrorMap(ex -> new DataAccessResourceFailureException("Failed to obtain R2DBC Connection", ex)); } /** @@ -133,10 +133,10 @@ public abstract class ConnectionFactoryUtils { synchronizationManager.bindResource(connectionFactory, holderToUse); } }) // Unexpected exception from external delegation call -> close Connection and rethrow. - .onErrorResume(e -> releaseConnection(connection, connectionFactory).then(Mono.error(e)))); + .onErrorResume(ex -> releaseConnection(connection, connectionFactory).then(Mono.error(ex)))); } return con; - }).onErrorResume(NoTransactionException.class, e -> Mono.from(connectionFactory.create())); + }).onErrorResume(NoTransactionException.class, ex -> Mono.from(connectionFactory.create())); } /** @@ -161,7 +161,7 @@ public abstract class ConnectionFactoryUtils { */ public static Mono releaseConnection(Connection con, ConnectionFactory connectionFactory) { return doReleaseConnection(con, connectionFactory) - .onErrorMap(e -> new DataAccessResourceFailureException("Failed to close R2DBC Connection", e)); + .onErrorMap(ex -> new DataAccessResourceFailureException("Failed to close R2DBC Connection", ex)); } /** @@ -181,7 +181,7 @@ public abstract class ConnectionFactoryUtils { conHolder.released(); } return Mono.from(connection.close()); - }).onErrorResume(NoTransactionException.class, e -> Mono.from(connection.close())); + }).onErrorResume(NoTransactionException.class, ex -> Mono.from(connection.close())); } /** diff --git a/spring-r2dbc/src/main/java/org/springframework/r2dbc/connection/R2dbcTransactionManager.java b/spring-r2dbc/src/main/java/org/springframework/r2dbc/connection/R2dbcTransactionManager.java index 7e4a0e49db4..1143aa1a7f7 100644 --- a/spring-r2dbc/src/main/java/org/springframework/r2dbc/connection/R2dbcTransactionManager.java +++ b/spring-r2dbc/src/main/java/org/springframework/r2dbc/connection/R2dbcTransactionManager.java @@ -210,8 +210,7 @@ public class R2dbcTransactionManager extends AbstractReactiveTransactionManager connectionMono = Mono.just(txObject.getConnectionHolder().getConnection()); } - return connectionMono.flatMap(con -> switchAutoCommitIfNecessary(con, transaction) - .then(Mono.from(doBegin(definition, con))) + return connectionMono.flatMap(con -> Mono.from(doBegin(definition, con)) .then(prepareTransactionalConnection(con, definition)) .doOnSuccess(v -> { txObject.getConnectionHolder().setTransactionActive(true); @@ -223,18 +222,15 @@ public class R2dbcTransactionManager extends AbstractReactiveTransactionManager if (txObject.isNewConnectionHolder()) { synchronizationManager.bindResource(obtainConnectionFactory(), txObject.getConnectionHolder()); } - }).thenReturn(con).onErrorResume(e -> { + }).thenReturn(con).onErrorResume(ex -> { if (txObject.isNewConnectionHolder()) { return ConnectionFactoryUtils.releaseConnection(con, obtainConnectionFactory()) .doOnTerminate(() -> txObject.setConnectionHolder(null, false)) - .then(Mono.error(e)); + .then(Mono.error(ex)); } - return Mono.error(e); - })).onErrorResume(e -> { - CannotCreateTransactionException ex = new CannotCreateTransactionException( - "Could not open R2DBC Connection for transaction", e); return Mono.error(ex); - }); + })).onErrorResume(ex -> Mono.error(new CannotCreateTransactionException( + "Could not open R2DBC Connection for transaction", ex))); }).then(); } @@ -356,20 +352,18 @@ public class R2dbcTransactionManager extends AbstractReactiveTransactionManager Mono afterCleanup = Mono.empty(); - if (txObject.isMustRestoreAutoCommit()) { - Mono restoreAutoCommitStep = safeCleanupStep( - "doCleanupAfterCompletion when restoring autocommit", Mono.from(con.setAutoCommit(true))); - afterCleanup = afterCleanup.then(restoreAutoCommitStep); - } - Mono releaseConnectionStep = Mono.defer(() -> { try { if (txObject.isNewConnectionHolder()) { if (logger.isDebugEnabled()) { logger.debug("Releasing R2DBC Connection [" + con + "] after transaction"); } - return safeCleanupStep("doCleanupAfterCompletion when releasing R2DBC Connection", - ConnectionFactoryUtils.releaseConnection(con, obtainConnectionFactory())); + Mono releaseMono = ConnectionFactoryUtils.releaseConnection(con, obtainConnectionFactory()); + if (logger.isDebugEnabled()) { + releaseMono = releaseMono.doOnError( + ex -> logger.debug(String.format("Error ignored during cleanup: %s", ex))); + } + return releaseMono.onErrorComplete(); } } finally { @@ -381,35 +375,6 @@ public class R2dbcTransactionManager extends AbstractReactiveTransactionManager }); } - private Mono safeCleanupStep(String stepDescription, Mono stepMono) { - if (!logger.isDebugEnabled()) { - return stepMono.onErrorComplete(); - } - else { - return stepMono.doOnError(e -> - logger.debug(String.format("Error ignored during %s: %s", stepDescription, e))) - .onErrorComplete(); - } - } - - private Mono switchAutoCommitIfNecessary(Connection con, Object transaction) { - ConnectionFactoryTransactionObject txObject = (ConnectionFactoryTransactionObject) transaction; - Mono prepare = Mono.empty(); - - // Switch to manual commit if necessary. This is very expensive in some R2DBC drivers, - // so we don't want to do it unnecessarily (for example if we've explicitly - // configured the connection pool to set it already). - if (con.isAutoCommit()) { - txObject.setMustRestoreAutoCommit(true); - if (logger.isDebugEnabled()) { - logger.debug("Switching R2DBC Connection [" + con + "] to manual commit"); - } - prepare = prepare.then(Mono.from(con.setAutoCommit(false))); - } - - return prepare; - } - /** * Prepare the transactional {@link Connection} right after transaction begin. *

The default implementation executes a "SET TRANSACTION READ ONLY" statement if the @@ -531,8 +496,6 @@ public class R2dbcTransactionManager extends AbstractReactiveTransactionManager private boolean newConnectionHolder; - private boolean mustRestoreAutoCommit; - void setConnectionHolder(@Nullable ConnectionHolder connectionHolder, boolean newConnectionHolder) { setConnectionHolder(connectionHolder); this.newConnectionHolder = newConnectionHolder; @@ -558,14 +521,6 @@ public class R2dbcTransactionManager extends AbstractReactiveTransactionManager public boolean hasConnectionHolder() { return (this.connectionHolder != null); } - - public void setMustRestoreAutoCommit(boolean mustRestoreAutoCommit) { - this.mustRestoreAutoCommit = mustRestoreAutoCommit; - } - - public boolean isMustRestoreAutoCommit() { - return this.mustRestoreAutoCommit; - } } } diff --git a/spring-r2dbc/src/test/java/org/springframework/r2dbc/connection/R2dbcTransactionManagerUnitTests.java b/spring-r2dbc/src/test/java/org/springframework/r2dbc/connection/R2dbcTransactionManagerUnitTests.java index 857744d828b..b428d8ff7d3 100644 --- a/spring-r2dbc/src/test/java/org/springframework/r2dbc/connection/R2dbcTransactionManagerUnitTests.java +++ b/spring-r2dbc/src/test/java/org/springframework/r2dbc/connection/R2dbcTransactionManagerUnitTests.java @@ -23,7 +23,6 @@ import io.r2dbc.spi.Connection; import io.r2dbc.spi.ConnectionFactory; import io.r2dbc.spi.IsolationLevel; import io.r2dbc.spi.R2dbcBadGrammarException; -import io.r2dbc.spi.R2dbcTimeoutException; import io.r2dbc.spi.Statement; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -56,6 +55,7 @@ import static org.mockito.BDDMockito.when; * Unit tests for {@link R2dbcTransactionManager}. * * @author Mark Paluch + * @author Juergen Hoeller */ class R2dbcTransactionManagerUnitTests { @@ -67,7 +67,7 @@ class R2dbcTransactionManagerUnitTests { @BeforeEach - @SuppressWarnings({ "unchecked", "rawtypes" }) + @SuppressWarnings({"unchecked", "rawtypes"}) void before() { when(connectionFactoryMock.create()).thenReturn((Mono) Mono.just(connectionMock)); when(connectionMock.beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class))).thenReturn(Mono.empty()); @@ -96,7 +96,6 @@ class R2dbcTransactionManagerUnitTests { .verifyComplete(); assertThat(commits).hasValue(1); - verify(connectionMock).isAutoCommit(); verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class)); verify(connectionMock).commitTransaction(); verify(connectionMock).close(); @@ -185,7 +184,6 @@ class R2dbcTransactionManagerUnitTests { @Test void doesNotSetAutoCommitDisabled() { - when(connectionMock.isAutoCommit()).thenReturn(false); when(connectionMock.commitTransaction()).thenReturn(Mono.empty()); DefaultTransactionDefinition definition = new DefaultTransactionDefinition(); @@ -203,29 +201,6 @@ class R2dbcTransactionManagerUnitTests { verify(connectionMock).commitTransaction(); } - @Test - void restoresAutoCommit() { - when(connectionMock.isAutoCommit()).thenReturn(true); - when(connectionMock.setAutoCommit(anyBoolean())).thenReturn(Mono.empty()); - when(connectionMock.commitTransaction()).thenReturn(Mono.empty()); - - DefaultTransactionDefinition definition = new DefaultTransactionDefinition(); - - TransactionalOperator operator = TransactionalOperator.create(tm, definition); - - ConnectionFactoryUtils.getConnection(connectionFactoryMock).as( - operator::transactional) - .as(StepVerifier::create) - .expectNextCount(1) - .verifyComplete(); - - verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class)); - verify(connectionMock).setAutoCommit(false); - verify(connectionMock).setAutoCommit(true); - verify(connectionMock).commitTransaction(); - verify(connectionMock).close(); - } - @Test void appliesReadOnly() { when(connectionMock.commitTransaction()).thenReturn(Mono.empty()); @@ -246,7 +221,6 @@ class R2dbcTransactionManagerUnitTests { .expectNextCount(1) .verifyComplete(); - verify(connectionMock).isAutoCommit(); verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class)); verify(connectionMock).createStatement("SET TRANSACTION READ ONLY"); verify(connectionMock).commitTransaction(); @@ -268,7 +242,6 @@ class R2dbcTransactionManagerUnitTests { .as(StepVerifier::create) .verifyError(BadSqlGrammarException.class); - verify(connectionMock).isAutoCommit(); verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class)); verify(connectionMock).createStatement("foo"); verify(connectionMock).commitTransaction(); @@ -299,7 +272,6 @@ class R2dbcTransactionManagerUnitTests { assertThat(commits).hasValue(0); assertThat(rollbacks).hasValue(1); - verify(connectionMock).isAutoCommit(); verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class)); verify(connectionMock).rollbackTransaction(); verify(connectionMock).close(); @@ -322,7 +294,6 @@ class R2dbcTransactionManagerUnitTests { }).as(StepVerifier::create) .verifyError(BadSqlGrammarException.class); - verify(connectionMock).isAutoCommit(); verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class)); verify(connectionMock).createStatement("foo"); verify(connectionMock, never()).commitTransaction(); @@ -338,10 +309,7 @@ class R2dbcTransactionManagerUnitTests { TransactionalOperator operator = TransactionalOperator.create(tm); - when(connectionMock.isAutoCommit()).thenReturn(true); - when(connectionMock.setAutoCommit(true)).thenReturn(Mono.defer(() -> Mono.error(new R2dbcTimeoutException("SET AUTOCOMMIT = 1 timed out")))); when(connectionMock.setTransactionIsolationLevel(any())).thenReturn(Mono.empty()); - when(connectionMock.setAutoCommit(false)).thenReturn(Mono.empty()); operator.execute(reactiveTransaction -> ConnectionFactoryUtils.getConnection(connectionFactoryMock) .doOnNext(connection -> { @@ -352,7 +320,6 @@ class R2dbcTransactionManagerUnitTests { .hasCause(new R2dbcBadGrammarException("Rollback should fail")) ); - verify(connectionMock).isAutoCommit(); verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class)); verify(connectionMock, never()).commitTransaction(); verify(connectionMock).rollbackTransaction(); @@ -380,7 +347,6 @@ class R2dbcTransactionManagerUnitTests { }).as(StepVerifier::create) .verifyComplete(); - verify(connectionMock).isAutoCommit(); verify(connectionMock).beginTransaction(any(io.r2dbc.spi.TransactionDefinition.class)); verify(connectionMock).rollbackTransaction(); verify(connectionMock).close();