diff --git a/docs/using-the-jdbc-driver/using-plugins/UsingTheReadWriteSplittingPlugin.md b/docs/using-the-jdbc-driver/using-plugins/UsingTheReadWriteSplittingPlugin.md index 973c0f155..8413098f6 100644 --- a/docs/using-the-jdbc-driver/using-plugins/UsingTheReadWriteSplittingPlugin.md +++ b/docs/using-the-jdbc-driver/using-plugins/UsingTheReadWriteSplittingPlugin.md @@ -42,7 +42,7 @@ Once this parameter is enabled and `setReadOnly(true)` has been called on the `C - After executing `COMMIT`, `ROLLBACK` or `ABORT` as a SQL statement - After executing any SQL statement while autocommit is on, with the following exceptions: - The statement started a transaction via `BEGIN` or `START TRANSACTION` - - The statement began with `SET` (eg `SET time_zone = "+00:00"`) + - The statement began with `SET`, `USE`, or `SHOW` ### Limitations with Reader Load Balancing diff --git a/wrapper/src/main/java/software/amazon/jdbc/plugin/DefaultConnectionPlugin.java b/wrapper/src/main/java/software/amazon/jdbc/plugin/DefaultConnectionPlugin.java index ce57ab3bf..ec6dad72f 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/plugin/DefaultConnectionPlugin.java +++ b/wrapper/src/main/java/software/amazon/jdbc/plugin/DefaultConnectionPlugin.java @@ -94,9 +94,7 @@ public T execute( throws E { LOGGER.finest( - () -> Messages.get( - "DefaultConnectionPlugin.executingMethod", - new Object[] {methodName})); + () -> Messages.get("DefaultConnectionPlugin.executingMethod", new Object[] {methodName})); final T result = jdbcMethodFunc.call(); Connection currentConn = this.pluginService.getCurrentConnection(); @@ -109,7 +107,11 @@ public T execute( if (sqlMethodAnalyzer.doesOpenTransaction(currentConn, methodName, jdbcMethodArgs)) { this.pluginManagerService.setInTransaction(true); - } else if (sqlMethodAnalyzer.doesCloseTransaction(methodName, jdbcMethodArgs)) { + } else if ( + sqlMethodAnalyzer.doesCloseTransaction(currentConn, methodName, jdbcMethodArgs) + // According to the JDBC spec, transactions are committed if autocommit is switched from false to true. + || sqlMethodAnalyzer.doesSwitchAutoCommitFalseTrue(currentConn, methodName, + jdbcMethodArgs)) { this.pluginManagerService.setInTransaction(false); } @@ -160,7 +162,8 @@ public void initHostProvider( } @Override - public OldConnectionSuggestedAction notifyConnectionChanged(final EnumSet changes) { + public OldConnectionSuggestedAction notifyConnectionChanged( + final EnumSet changes) { return OldConnectionSuggestedAction.NO_OPINION; } diff --git a/wrapper/src/main/java/software/amazon/jdbc/plugin/readwritesplitting/ReadWriteSplittingPlugin.java b/wrapper/src/main/java/software/amazon/jdbc/plugin/readwritesplitting/ReadWriteSplittingPlugin.java index 711f37cf6..26dd94208 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/plugin/readwritesplitting/ReadWriteSplittingPlugin.java +++ b/wrapper/src/main/java/software/amazon/jdbc/plugin/readwritesplitting/ReadWriteSplittingPlugin.java @@ -306,7 +306,8 @@ public T execute( } private boolean isTransactionBoundary(final String methodName, final Object[] args) { - if (sqlMethodAnalyzer.doesCloseTransaction(methodName, args)) { + final Connection currentConnection = this.pluginService.getCurrentConnection(); + if (sqlMethodAnalyzer.doesCloseTransaction(currentConnection, methodName, args)) { return true; } @@ -316,7 +317,6 @@ private boolean isTransactionBoundary(final String methodName, final Object[] ar final boolean autocommit; try { - final Connection currentConnection = this.pluginService.getCurrentConnection(); autocommit = currentConnection.getAutoCommit(); } catch (final SQLException e) { return false; diff --git a/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java b/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java index 622700669..b8371f370 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java +++ b/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java @@ -25,7 +25,8 @@ public class SqlMethodAnalyzer { - public boolean doesOpenTransaction(final Connection currentConn, final String methodName, final Object[] args) { + public boolean doesOpenTransaction(final Connection conn, final String methodName, + final Object[] args) { if (!(methodName.contains("execute") && args != null && args.length >= 1)) { return false; } @@ -37,7 +38,7 @@ public boolean doesOpenTransaction(final Connection currentConn, final String me final boolean autocommit; try { - autocommit = currentConn.getAutoCommit(); + autocommit = conn.getAutoCommit(); } catch (final SQLException e) { return false; } @@ -67,12 +68,17 @@ private List parseMultiStatementQueries(String query) { return Arrays.stream(query.split(";")).collect(Collectors.toList()); } - public boolean doesCloseTransaction(final String methodName, final Object[] args) { + public boolean doesCloseTransaction(final Connection conn, final String methodName, + final Object[] args) { if (methodName.equals("Connection.commit") || methodName.equals("Connection.rollback") || methodName.equals("Connection.close") || methodName.equals("Connection.abort")) { return true; } + if (doesSwitchAutoCommitFalseTrue(conn, methodName, args)) { + return true; + } + if (!(methodName.contains("execute") && args != null && args.length >= 1)) { return false; } @@ -93,11 +99,9 @@ public boolean isExecuteDml(final String methodName, final Object[] args) { public boolean isStatementDml(final String statement) { return !isStatementStartingTransaction(statement) && !isStatementClosingTransaction(statement) - && !isStatementSettingState(statement); - } - - public boolean isStatementSettingState(final String statement) { - return statement.startsWith("SET "); + && !statement.startsWith("SET ") + && !statement.startsWith("USE ") + && !statement.startsWith("SHOW "); } public boolean isStatementStartingTransaction(final String statement) { @@ -120,6 +124,31 @@ public boolean isStatementSettingAutoCommit(final String methodName, final Objec return statement.startsWith("SET AUTOCOMMIT"); } + public boolean doesSwitchAutoCommitFalseTrue(final Connection conn, final String methodName, + final Object[] jdbcMethodArgs) { + boolean isStatementSettingAutoCommit = isStatementSettingAutoCommit( + methodName, jdbcMethodArgs); + if (!methodName.contains("setAutoCommit") && !isStatementSettingAutoCommit) { + return false; + } + + boolean oldAutoCommitVal; + Boolean newAutoCommitVal = null; + try { + oldAutoCommitVal = conn.getAutoCommit(); + } catch (SQLException e) { + return false; + } + + if (methodName.contains("setAutoCommit") && jdbcMethodArgs.length > 0) { + newAutoCommitVal = (Boolean) jdbcMethodArgs[0]; + } else if (isStatementSettingAutoCommit) { + newAutoCommitVal = getAutoCommitValueFromSqlStatement(jdbcMethodArgs); + } + + return !oldAutoCommitVal && Boolean.TRUE.equals(newAutoCommitVal); + } + public Boolean getAutoCommitValueFromSqlStatement(final Object[] args) { if (args == null || args.length < 1) { return null; diff --git a/wrapper/src/test/java/software/amazon/jdbc/util/SqlMethodAnalyzerTest.java b/wrapper/src/test/java/software/amazon/jdbc/util/SqlMethodAnalyzerTest.java index 8d2da8625..cf6c8b7cd 100644 --- a/wrapper/src/test/java/software/amazon/jdbc/util/SqlMethodAnalyzerTest.java +++ b/wrapper/src/test/java/software/amazon/jdbc/util/SqlMethodAnalyzerTest.java @@ -17,6 +17,8 @@ package software.amazon.jdbc.util; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.when; import java.sql.Connection; @@ -24,6 +26,7 @@ import java.util.stream.Stream; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -32,8 +35,8 @@ class SqlMethodAnalyzerTest { - @Mock - Connection conn; + @Mock Connection conn; + private final SqlMethodAnalyzer sqlMethodAnalyzer = new SqlMethodAnalyzer(); private AutoCloseable closeable; @@ -50,7 +53,8 @@ void tearDown() throws Exception { @ParameterizedTest @MethodSource("openTransactionQueries") - void testOpenTransaction(final String methodName, final String sql, final boolean autocommit, final boolean expected) + void testOpenTransaction(final String methodName, final String sql, final boolean autocommit, + final boolean expected) throws SQLException { final Object[] args; if (sql != null) { @@ -74,10 +78,36 @@ void testCloseTransaction(final String methodName, final String sql, final boole args = new Object[] {}; } - final boolean actual = sqlMethodAnalyzer.doesCloseTransaction(methodName, args); + final boolean actual = sqlMethodAnalyzer.doesCloseTransaction(conn, methodName, args); assertEquals(expected, actual); } + @Test + void testDoesSwitchAutoCommitFalseTrue() throws SQLException { + assertFalse(sqlMethodAnalyzer.doesSwitchAutoCommitFalseTrue(conn, "Connection.setAutoCommit", + new Object[] {false})); + assertFalse(sqlMethodAnalyzer.doesSwitchAutoCommitFalseTrue(conn, "Statement.execute", + new Object[] {"SET autocommit = 0"})); + + assertTrue(sqlMethodAnalyzer.doesSwitchAutoCommitFalseTrue(conn, "Connection.setAutoCommit", + new Object[] {true})); + assertTrue(sqlMethodAnalyzer.doesSwitchAutoCommitFalseTrue(conn, "Statement.execute", + new Object[] {"SET autocommit = 1"})); + + when(conn.getAutoCommit()).thenReturn(true); + + assertFalse(sqlMethodAnalyzer.doesSwitchAutoCommitFalseTrue(conn, "Connection.setAutoCommit", + new Object[] {false})); + assertFalse(sqlMethodAnalyzer.doesSwitchAutoCommitFalseTrue(conn, "Statement.execute", + new Object[] {"SET autocommit = 0"})); + assertFalse(sqlMethodAnalyzer.doesSwitchAutoCommitFalseTrue(conn, "Connection.setAutoCommit", + new Object[] {true})); + assertFalse(sqlMethodAnalyzer.doesSwitchAutoCommitFalseTrue(conn, "Statement.execute", + new Object[] {"SET autocommit = 1"})); + assertFalse(sqlMethodAnalyzer.doesSwitchAutoCommitFalseTrue(conn, "Statement.execute", + new Object[] {"SET TIME ZONE 'UTC'"})); + } + @ParameterizedTest @MethodSource("isExecuteDmlQueries") void testIsExecuteDml(final String methodName, final String sql, final boolean expected) { @@ -94,7 +124,8 @@ void testIsExecuteDml(final String methodName, final String sql, final boolean e @ParameterizedTest @MethodSource("isSettingAutoCommitQueries") - void testIsStatementSettingAutoCommit(final String methodName, final String sql, final boolean expected) { + void testIsStatementSettingAutoCommit(final String methodName, final String sql, + final boolean expected) { final Object[] args; if (sql != null) { args = new Object[] {sql}; @@ -135,13 +166,16 @@ private static Stream openTransactionQueries() { Arguments.of("Statement.execute", "START/* COMMENT */TRANSACTION;", true, true), Arguments.of("Statement.execute", "START /* COMMENT */ TRANSACTION;", true, true), Arguments.of("Statement.executeUpdate", "START /*COMMENT*/TRANSACTION;", true, true), - Arguments.of("Statement.executeUpdate", "/*COMMENT*/START /*COMMENT*/TRANSACTION;", true, true), - Arguments.of("Statement.executeUpdate", " /*COMMENT*/ START /*COMMENT*/TRANSACTION;", true, true), + Arguments.of("Statement.executeUpdate", "/*COMMENT*/START /*COMMENT*/TRANSACTION;", true, + true), + Arguments.of("Statement.executeUpdate", " /*COMMENT*/ START /*COMMENT*/TRANSACTION;", + true, true), Arguments.of("Statement.executeUpdate", " /*COMMENT*/ begin", true, true), Arguments.of("Statement.executeUpdate", "commit", false, false), Arguments.of("Statement.executeQuery", " select 1", true, false), Arguments.of("Statement.executeQuery", " SELECT 1", false, true), - Arguments.of("Statement.executeUpdate", " INSERT INTO test_table VALUES (1) ; ", false, true), + Arguments.of("Statement.executeUpdate", " INSERT INTO test_table VALUES (1) ; ", false, + true), Arguments.of("Statement.executeUpdate", " set autocommit = 1 ", false, false), Arguments.of("Connection.commit", null, false, false) ); @@ -170,6 +204,8 @@ private static Stream isExecuteDmlQueries() { Arguments.of("Statement.execute", " begin ; ", false), Arguments.of("Statement.execute", " rollback ; ", false), Arguments.of("Statement.execute", " SET autocommit = 0 ; ", false), + Arguments.of("Statement.execute", " SHOW DATABASES ; ", false), + Arguments.of("Statement.execute", " USE mydatabase ; ", false), Arguments.of("Statement.executeQuery", " SELECT 1; ", true), Arguments.of("Statement.executeUpdate", "INSERT INTO test_table VALUES (1)", true) );