From c1035b7a9651dd7af7fa819a534a5af3ec88efd0 Mon Sep 17 00:00:00 2001 From: Aaron Congo Date: Wed, 7 Dec 2022 14:29:58 -0800 Subject: [PATCH 1/7] doesOpenTransaction and doesCloseTransaction checks current transaction status --- .../jdbc/plugin/DefaultConnectionPlugin.java | 15 +++--- .../ReadWriteSplittingPlugin.java | 2 +- .../amazon/jdbc/util/SqlMethodAnalyzer.java | 49 +++++++++++++++++-- .../jdbc/util/SqlMethodAnalyzerTest.java | 36 ++++++++++++-- 4 files changed, 88 insertions(+), 14 deletions(-) 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..d99dc93dc 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/plugin/DefaultConnectionPlugin.java +++ b/wrapper/src/main/java/software/amazon/jdbc/plugin/DefaultConnectionPlugin.java @@ -29,6 +29,7 @@ import java.util.Set; import java.util.logging.Logger; import java.util.stream.Collectors; + import software.amazon.jdbc.ConnectionPlugin; import software.amazon.jdbc.ConnectionProvider; import software.amazon.jdbc.HostAvailability; @@ -94,9 +95,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(); @@ -107,9 +106,12 @@ public T execute( return result; } - if (sqlMethodAnalyzer.doesOpenTransaction(currentConn, methodName, jdbcMethodArgs)) { + if (sqlMethodAnalyzer.doesOpenTransaction(this.pluginService, methodName, jdbcMethodArgs)) { this.pluginManagerService.setInTransaction(true); - } else if (sqlMethodAnalyzer.doesCloseTransaction(methodName, jdbcMethodArgs)) { + } else if ( + sqlMethodAnalyzer.doesCloseTransaction(this.pluginService, methodName, jdbcMethodArgs) + || 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..f0993c973 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,7 @@ public T execute( } private boolean isTransactionBoundary(final String methodName, final Object[] args) { - if (sqlMethodAnalyzer.doesCloseTransaction(methodName, args)) { + if (sqlMethodAnalyzer.doesCloseTransaction(this.pluginService, methodName, args)) { return true; } 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..98af56961 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java +++ b/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java @@ -23,9 +23,16 @@ import java.util.List; import java.util.stream.Collectors; +import software.amazon.jdbc.PluginService; + public class SqlMethodAnalyzer { - public boolean doesOpenTransaction(final Connection currentConn, final String methodName, final Object[] args) { + public boolean doesOpenTransaction(final PluginService pluginService, final String methodName, + final Object[] args) { + if (pluginService.isInTransaction()) { + return false; + } + if (!(methodName.contains("execute") && args != null && args.length >= 1)) { return false; } @@ -35,9 +42,10 @@ public boolean doesOpenTransaction(final Connection currentConn, final String me return true; } + final Connection conn = pluginService.getCurrentConnection(); final boolean autocommit; try { - autocommit = currentConn.getAutoCommit(); + autocommit = conn.getAutoCommit(); } catch (final SQLException e) { return false; } @@ -67,12 +75,22 @@ 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 PluginService pluginService, final String methodName, + final Object[] args) { + if (!pluginService.isInTransaction()) { + return false; + } + if (methodName.equals("Connection.commit") || methodName.equals("Connection.rollback") || methodName.equals("Connection.close") || methodName.equals("Connection.abort")) { return true; } + final Connection conn = pluginService.getCurrentConnection(); + if (doesSwitchAutoCommitFalseTrue(conn, methodName, args)) { + return true; + } + if (!(methodName.contains("execute") && args != null && args.length >= 1)) { return false; } @@ -120,6 +138,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 Boolean.FALSE.equals(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..91a648f4e 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,16 +26,21 @@ 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; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import software.amazon.jdbc.PluginService; + class SqlMethodAnalyzerTest { - @Mock - Connection conn; + @Mock Connection conn; + + @Mock PluginService pluginService; + private final SqlMethodAnalyzer sqlMethodAnalyzer = new SqlMethodAnalyzer(); private AutoCloseable closeable; @@ -41,6 +48,7 @@ class SqlMethodAnalyzerTest { @BeforeEach void setUp() { closeable = MockitoAnnotations.openMocks(this); + when(pluginService.getCurrentConnection()).thenReturn(conn); } @AfterEach @@ -60,10 +68,20 @@ void testOpenTransaction(final String methodName, final String sql, final boolea } when(conn.getAutoCommit()).thenReturn(autocommit); - final boolean actual = sqlMethodAnalyzer.doesOpenTransaction(conn, methodName, args); + final boolean actual = sqlMethodAnalyzer.doesOpenTransaction(pluginService, methodName, args); assertEquals(expected, actual); } + @Test + void testOpenTransactionChecksTransactionStatus() { + final Object[] args = new Object[] {"START TRANSACTION"}; + + assertTrue(sqlMethodAnalyzer.doesOpenTransaction(pluginService, "Statement.execute", args)); + + when(pluginService.isInTransaction()).thenReturn(true); + assertFalse(sqlMethodAnalyzer.doesOpenTransaction(pluginService, "Statement.execute", args)); + } + @ParameterizedTest @MethodSource("closeTransactionQueries") void testCloseTransaction(final String methodName, final String sql, final boolean expected) { @@ -74,10 +92,20 @@ 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(pluginService, methodName, args); assertEquals(expected, actual); } + @Test + void testCloseTransactionChecksTransactionStatus() { + final Object[] args = new Object[] {"COMMIT"}; + + assertFalse(sqlMethodAnalyzer.doesCloseTransaction(pluginService, "Statement.execute", args)); + + when(pluginService.isInTransaction()).thenReturn(true); + assertTrue(sqlMethodAnalyzer.doesCloseTransaction(pluginService, "Statement.execute", args)); + } + @ParameterizedTest @MethodSource("isExecuteDmlQueries") void testIsExecuteDml(final String methodName, final String sql, final boolean expected) { From 5c977aed6b95bb02486dc216dfcf47842ef723a8 Mon Sep 17 00:00:00 2001 From: Aaron Congo Date: Wed, 7 Dec 2022 14:37:11 -0800 Subject: [PATCH 2/7] doesOpenTransaction and doesCloseTransaction do not check current transaction status --- .../jdbc/plugin/DefaultConnectionPlugin.java | 4 +- .../ReadWriteSplittingPlugin.java | 4 +- .../amazon/jdbc/util/SqlMethodAnalyzer.java | 14 +------ .../jdbc/util/SqlMethodAnalyzerTest.java | 38 +++---------------- 4 files changed, 11 insertions(+), 49 deletions(-) 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 d99dc93dc..64fa98bd2 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/plugin/DefaultConnectionPlugin.java +++ b/wrapper/src/main/java/software/amazon/jdbc/plugin/DefaultConnectionPlugin.java @@ -106,10 +106,10 @@ public T execute( return result; } - if (sqlMethodAnalyzer.doesOpenTransaction(this.pluginService, methodName, jdbcMethodArgs)) { + if (sqlMethodAnalyzer.doesOpenTransaction(currentConn, methodName, jdbcMethodArgs)) { this.pluginManagerService.setInTransaction(true); } else if ( - sqlMethodAnalyzer.doesCloseTransaction(this.pluginService, methodName, jdbcMethodArgs) + sqlMethodAnalyzer.doesCloseTransaction(currentConn, methodName, jdbcMethodArgs) || sqlMethodAnalyzer.doesSwitchAutoCommitFalseTrue(currentConn, methodName, jdbcMethodArgs)) { this.pluginManagerService.setInTransaction(false); 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 f0993c973..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(this.pluginService, 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 98af56961..bd1dbb22e 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java +++ b/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java @@ -27,12 +27,8 @@ public class SqlMethodAnalyzer { - public boolean doesOpenTransaction(final PluginService pluginService, final String methodName, + public boolean doesOpenTransaction(final Connection conn, final String methodName, final Object[] args) { - if (pluginService.isInTransaction()) { - return false; - } - if (!(methodName.contains("execute") && args != null && args.length >= 1)) { return false; } @@ -42,7 +38,6 @@ public boolean doesOpenTransaction(final PluginService pluginService, final Stri return true; } - final Connection conn = pluginService.getCurrentConnection(); final boolean autocommit; try { autocommit = conn.getAutoCommit(); @@ -75,18 +70,13 @@ private List parseMultiStatementQueries(String query) { return Arrays.stream(query.split(";")).collect(Collectors.toList()); } - public boolean doesCloseTransaction(final PluginService pluginService, final String methodName, + public boolean doesCloseTransaction(final Connection conn, final String methodName, final Object[] args) { - if (!pluginService.isInTransaction()) { - return false; - } - if (methodName.equals("Connection.commit") || methodName.equals("Connection.rollback") || methodName.equals("Connection.close") || methodName.equals("Connection.abort")) { return true; } - final Connection conn = pluginService.getCurrentConnection(); if (doesSwitchAutoCommitFalseTrue(conn, methodName, args)) { return true; } 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 91a648f4e..958659e84 100644 --- a/wrapper/src/test/java/software/amazon/jdbc/util/SqlMethodAnalyzerTest.java +++ b/wrapper/src/test/java/software/amazon/jdbc/util/SqlMethodAnalyzerTest.java @@ -17,30 +17,24 @@ 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; -import java.sql.SQLException; -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; import org.mockito.Mock; import org.mockito.MockitoAnnotations; -import software.amazon.jdbc.PluginService; +import java.sql.Connection; +import java.sql.SQLException; +import java.util.stream.Stream; class SqlMethodAnalyzerTest { @Mock Connection conn; - @Mock PluginService pluginService; - private final SqlMethodAnalyzer sqlMethodAnalyzer = new SqlMethodAnalyzer(); private AutoCloseable closeable; @@ -48,7 +42,6 @@ class SqlMethodAnalyzerTest { @BeforeEach void setUp() { closeable = MockitoAnnotations.openMocks(this); - when(pluginService.getCurrentConnection()).thenReturn(conn); } @AfterEach @@ -68,20 +61,10 @@ void testOpenTransaction(final String methodName, final String sql, final boolea } when(conn.getAutoCommit()).thenReturn(autocommit); - final boolean actual = sqlMethodAnalyzer.doesOpenTransaction(pluginService, methodName, args); + final boolean actual = sqlMethodAnalyzer.doesOpenTransaction(conn, methodName, args); assertEquals(expected, actual); } - @Test - void testOpenTransactionChecksTransactionStatus() { - final Object[] args = new Object[] {"START TRANSACTION"}; - - assertTrue(sqlMethodAnalyzer.doesOpenTransaction(pluginService, "Statement.execute", args)); - - when(pluginService.isInTransaction()).thenReturn(true); - assertFalse(sqlMethodAnalyzer.doesOpenTransaction(pluginService, "Statement.execute", args)); - } - @ParameterizedTest @MethodSource("closeTransactionQueries") void testCloseTransaction(final String methodName, final String sql, final boolean expected) { @@ -92,20 +75,9 @@ void testCloseTransaction(final String methodName, final String sql, final boole args = new Object[] {}; } - final boolean actual = sqlMethodAnalyzer.doesCloseTransaction(pluginService, methodName, args); + final boolean actual = sqlMethodAnalyzer.doesCloseTransaction(conn, methodName, args); assertEquals(expected, actual); } - - @Test - void testCloseTransactionChecksTransactionStatus() { - final Object[] args = new Object[] {"COMMIT"}; - - assertFalse(sqlMethodAnalyzer.doesCloseTransaction(pluginService, "Statement.execute", args)); - - when(pluginService.isInTransaction()).thenReturn(true); - assertTrue(sqlMethodAnalyzer.doesCloseTransaction(pluginService, "Statement.execute", args)); - } - @ParameterizedTest @MethodSource("isExecuteDmlQueries") void testIsExecuteDml(final String methodName, final String sql, final boolean expected) { From b5b4f0df2112b0123733ede4acad2250d85e87f3 Mon Sep 17 00:00:00 2001 From: Aaron Congo Date: Wed, 7 Dec 2022 15:13:51 -0800 Subject: [PATCH 3/7] Add unit test for SqlMethodAnalyzer.doesSwitchAutoCommitFalseTrue --- .../jdbc/util/SqlMethodAnalyzerTest.java | 45 ++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-) 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 958659e84..fb6ad55fd 100644 --- a/wrapper/src/test/java/software/amazon/jdbc/util/SqlMethodAnalyzerTest.java +++ b/wrapper/src/test/java/software/amazon/jdbc/util/SqlMethodAnalyzerTest.java @@ -17,10 +17,13 @@ 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 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; @@ -51,7 +54,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) { @@ -78,6 +82,33 @@ void testCloseTransaction(final String methodName, final String sql, final boole 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 +125,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 +167,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) ); From c71e03b68cd965b2ff4c946a70cf4018e51f83bb Mon Sep 17 00:00:00 2001 From: Aaron Congo Date: Wed, 7 Dec 2022 15:39:05 -0800 Subject: [PATCH 4/7] Add clarifying comment --- .../software/amazon/jdbc/plugin/DefaultConnectionPlugin.java | 1 + 1 file changed, 1 insertion(+) 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 64fa98bd2..fba8106fd 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/plugin/DefaultConnectionPlugin.java +++ b/wrapper/src/main/java/software/amazon/jdbc/plugin/DefaultConnectionPlugin.java @@ -110,6 +110,7 @@ public T execute( this.pluginManagerService.setInTransaction(true); } 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); From 0bfa023bb319a7e042bc3a88db401ce7567f1ee7 Mon Sep 17 00:00:00 2001 From: Aaron Congo Date: Fri, 9 Dec 2022 10:37:57 -0800 Subject: [PATCH 5/7] Fix checkstyle, exclude USE/SHOW from isExecuteDml --- .../using-plugins/UsingTheReadWriteSplittingPlugin.md | 2 +- .../amazon/jdbc/plugin/DefaultConnectionPlugin.java | 1 - .../software/amazon/jdbc/util/SqlMethodAnalyzer.java | 10 +++------- .../amazon/jdbc/util/SqlMethodAnalyzerTest.java | 9 +++++---- 4 files changed, 9 insertions(+), 13 deletions(-) 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 fba8106fd..ec6dad72f 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/plugin/DefaultConnectionPlugin.java +++ b/wrapper/src/main/java/software/amazon/jdbc/plugin/DefaultConnectionPlugin.java @@ -29,7 +29,6 @@ import java.util.Set; import java.util.logging.Logger; import java.util.stream.Collectors; - import software.amazon.jdbc.ConnectionPlugin; import software.amazon.jdbc.ConnectionProvider; import software.amazon.jdbc.HostAvailability; 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 bd1dbb22e..422b8c7f6 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java +++ b/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java @@ -23,8 +23,6 @@ import java.util.List; import java.util.stream.Collectors; -import software.amazon.jdbc.PluginService; - public class SqlMethodAnalyzer { public boolean doesOpenTransaction(final Connection conn, final String methodName, @@ -101,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) { 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 fb6ad55fd..cf6c8b7cd 100644 --- a/wrapper/src/test/java/software/amazon/jdbc/util/SqlMethodAnalyzerTest.java +++ b/wrapper/src/test/java/software/amazon/jdbc/util/SqlMethodAnalyzerTest.java @@ -21,6 +21,9 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.when; +import java.sql.Connection; +import java.sql.SQLException; +import java.util.stream.Stream; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -30,10 +33,6 @@ import org.mockito.Mock; import org.mockito.MockitoAnnotations; -import java.sql.Connection; -import java.sql.SQLException; -import java.util.stream.Stream; - class SqlMethodAnalyzerTest { @Mock Connection conn; @@ -205,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) ); From baefa955fe8c915db7a4727cb677f3a347f6e5c8 Mon Sep 17 00:00:00 2001 From: Aaron Congo Date: Fri, 9 Dec 2022 12:07:03 -0800 Subject: [PATCH 6/7] Fix qodana warning --- .../java/software/amazon/jdbc/util/SqlMethodAnalyzer.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 422b8c7f6..f6f7d4ca0 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java +++ b/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java @@ -132,7 +132,7 @@ public boolean doesSwitchAutoCommitFalseTrue(final Connection conn, final String return false; } - Boolean oldAutoCommitVal; + boolean oldAutoCommitVal; Boolean newAutoCommitVal = null; try { oldAutoCommitVal = conn.getAutoCommit(); @@ -146,7 +146,7 @@ public boolean doesSwitchAutoCommitFalseTrue(final Connection conn, final String newAutoCommitVal = getAutoCommitValueFromSqlStatement(jdbcMethodArgs); } - return Boolean.FALSE.equals(oldAutoCommitVal) && Boolean.TRUE.equals(newAutoCommitVal); + return oldAutoCommitVal == false && Boolean.TRUE.equals(newAutoCommitVal); } public Boolean getAutoCommitValueFromSqlStatement(final Object[] args) { From 3d8b2f4549d654d8ad0fd362489094f9d097ae16 Mon Sep 17 00:00:00 2001 From: Aaron Congo Date: Wed, 14 Dec 2022 09:48:45 -0800 Subject: [PATCH 7/7] Fix qodana warning --- .../main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 f6f7d4ca0..b8371f370 100644 --- a/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java +++ b/wrapper/src/main/java/software/amazon/jdbc/util/SqlMethodAnalyzer.java @@ -146,7 +146,7 @@ public boolean doesSwitchAutoCommitFalseTrue(final Connection conn, final String newAutoCommitVal = getAutoCommitValueFromSqlStatement(jdbcMethodArgs); } - return oldAutoCommitVal == false && Boolean.TRUE.equals(newAutoCommitVal); + return !oldAutoCommitVal && Boolean.TRUE.equals(newAutoCommitVal); } public Boolean getAutoCommitValueFromSqlStatement(final Object[] args) {