Fix transactions for non-autocommitting handles

If a handle is by default using autocommit == false, then transactions
could be commited (or rolled back) without having to call
Handle#begin() first. This broke in 3.41.1.

Restore the old behavior. Fixes jdbi#2491

RELEASE_NOTES.md

@@ -1,6 +1,7 @@
 # Unreleased
 
   - Deprecate the `otjPostgres` support in jdbi-testing. This will be undeprecated if they ship a version that provides an automatic module name for JPMS, otherwise it will be removed when Jdbi ships with full JPMS support.
+  - Restore pre-3.41.0 behavior for handles using auto-commit == false where transactions don't need `Handle#begin()` before `Handle#commit()` (#2491, thanks @grigorem)
 
 
 # 3.41.1

core/src/main/java/org/jdbi/v3/core/Handle.java

@@ -114,7 +114,7 @@ private Handle(Jdbi jdbi,
 
         // both of these methods are bad because they leak a reference to this handle before the c'tor finished.
         this.transactionHandler = transactionHandler.specialize(this);
-        this.forceEndTransactions = !transactionHandler.isInTransaction(this);
+        this.forceEndTransactions = !this.transactionHandler.isInTransaction(this);
 
         addCleanable(() ->
             statementBuilder.close(connection));

core/src/main/java/org/jdbi/v3/core/transaction/LocalTransactionHandler.java

@@ -108,10 +108,11 @@ enum State {
         }
         private final Map<String, Savepoint> savepoints = new HashMap<>();
         private boolean initialAutocommit;
-        private State handlerState = State.OUTSIDE_TRANSACTION;
+        private State handlerState;
 
         BoundLocalTransactionHandler(Handle handle) throws SQLException {
             this.initialAutocommit = handle.getConnection().getAutoCommit();
+            this.handlerState = initialAutocommit ? State.OUTSIDE_TRANSACTION : State.AFTER_BEGIN;
         }
 
         @Override
@@ -253,8 +254,9 @@ public <R, X extends Exception> R inTransaction(Handle handle,
 
         private void restoreAutoCommitState(Handle handle) {
             try {
+                handle.getConnection().setAutoCommit(initialAutocommit);
+
                 if (initialAutocommit) {
-                    handle.getConnection().setAutoCommit(initialAutocommit);
                     savepoints.clear();
                 }
             } catch (SQLException e) {

core/src/test/java/org/jdbi/v3/core/transaction/TestTransactions.java

@@ -14,9 +14,12 @@
 package org.jdbi.v3.core.transaction;
 
 import java.io.IOException;
+import java.sql.Connection;
+import java.sql.DriverManager;
 import java.util.List;
 
 import org.jdbi.v3.core.Handle;
+import org.jdbi.v3.core.Jdbi;
 import org.jdbi.v3.core.Something;
 import org.jdbi.v3.core.junit5.H2DatabaseExtension;
 import org.jdbi.v3.core.statement.StatementContext;
@@ -96,6 +99,30 @@ public void testDoubleOpen() throws Exception {
         assertThat(h.getConnection().getAutoCommit()).isTrue();
     }
 
+    @Test
+    public void testNoBeginTransaction() throws Exception {
+
+        Jdbi jdbi = Jdbi.create(() -> {
+            // create connection with auto-commit == false
+            Connection connection = DriverManager.getConnection(h2Extension.getUri());
+            connection.setAutoCommit(false);
+            return connection;
+        });
+
+        jdbi.useTransaction(h -> {
+            assertThat(h.getConnection().getAutoCommit()).isFalse();
+            h.execute("INSERT INTO something (id, name ) VALUES (1, 'foo')");
+            h.commit();
+        });
+
+        List<Integer> result = h.createQuery("SELECT count(1) from something")
+            .mapTo(Integer.class)
+            .list();
+        assertThat(result)
+            .hasSize(1)
+            .containsExactly(1);
+    }
+
     @Test
     public void testExceptionAbortsTransaction() {
         assertThatThrownBy(() ->

core/src/test/java/org/jdbi/v3/core/transaction/TestTransactionsAutoCommit.java

@@ -26,10 +26,11 @@
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyString;
-import static org.mockito.Mockito.atLeastOnce;
+import static org.mockito.Mockito.calls;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.when;
 
 public class TestTransactionsAutoCommit {
@@ -41,18 +42,17 @@ public void restoreAutoCommitInitialStateOnUnexpectedError() throws Exception {
 
         final Connection connection = mock(Connection.class);
         final PreparedStatement statement = mock(PreparedStatement.class);
-        InOrder inOrder = inOrder(connection, statement);
-
-        try (Handle h = Jdbi.create(() -> connection).open()) {
 
-            when(connection.getAutoCommit()).thenReturn(true);
-            when(connection.prepareStatement(anyString(), anyInt(), anyInt())).thenReturn(statement);
-            when(statement.execute()).thenReturn(true);
-            when(statement.getUpdateCount()).thenReturn(1);
-            // throw e.g some underlying database error
-            doThrow(new SQLException("infrastructure error")).when(connection).commit();
+        when(connection.getAutoCommit()).thenReturn(true);
+        when(connection.prepareStatement(anyString(), anyInt(), anyInt())).thenReturn(statement);
+        when(statement.execute()).thenReturn(true);
+        when(statement.getUpdateCount()).thenReturn(1);
+        // throw e.g some underlying database error
+        doThrow(new SQLException("infrastructure error")).when(connection).commit();
 
+        try (Handle h = Jdbi.create(() -> connection).open()) {
             h.begin();
+
             assertThatExceptionOfType(Exception.class).isThrownBy(() -> {
                 h.execute(SAMPLE_SQL, 1L, "Tom");
 
@@ -61,9 +61,11 @@ public void restoreAutoCommitInitialStateOnUnexpectedError() throws Exception {
             });
         }
 
+        InOrder inOrder = inOrder(connection, statement);
+
         // expected behaviour chain:
         // 1. store initial auto-commit state
-        inOrder.verify(connection, atLeastOnce()).getAutoCommit();
+        inOrder.verify(connection, calls(1)).getAutoCommit();
 
         // 2. turn off auto-commit
         inOrder.verify(connection).setAutoCommit(false);
@@ -78,6 +80,10 @@ public void restoreAutoCommitInitialStateOnUnexpectedError() throws Exception {
 
         // 5. set auto-commit back to initial state
         inOrder.verify(connection).setAutoCommit(true);
+
+        inOrder.verify(connection, times(1)).close();
+
+        inOrder.verifyNoMoreInteractions();
     }
 
 }