Fix bug where inTransaction was not set to false when autocommit was switched from false to true

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
 

wrapper/src/main/java/software/amazon/jdbc/plugin/DefaultConnectionPlugin.java

@@ -94,9 +94,7 @@ public <T, E extends Exception> 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, E extends Exception> 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<NodeChangeOptions> changes) {
+  public OldConnectionSuggestedAction notifyConnectionChanged(
+      final EnumSet<NodeChangeOptions> changes) {
     return OldConnectionSuggestedAction.NO_OPINION;
   }
 

wrapper/src/main/java/software/amazon/jdbc/plugin/readwritesplitting/ReadWriteSplittingPlugin.java

@@ -306,7 +306,8 @@ public <T, E extends Exception> 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;

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<String> 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 Boolean.FALSE.equals(oldAutoCommitVal) && Boolean.TRUE.equals(newAutoCommitVal);
+  }
+
   public Boolean getAutoCommitValueFromSqlStatement(final Object[] args) {
     if (args == null || args.length < 1) {
       return null;

wrapper/src/test/java/software/amazon/jdbc/util/SqlMethodAnalyzerTest.java

@@ -17,13 +17,16 @@
 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;
@@ -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<Arguments> 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<Arguments> 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)
     );