[liquibase#98] Disable auto-commit and restore on close

liquigraph-core/src/main/java/org/liquigraph/core/configuration/ConnectionConfigurationByUri.java

@@ -54,13 +54,7 @@ public ConnectionConfigurationByUri(String uri,
 
     @Override
     public Connection get() {
-        try {
-            Connection connection = driverManager.apply(uri());
-            connection.setAutoCommit(false);
-            return connection;
-        } catch (SQLException e) {
-            throw propagate(e);
-        }
+        return driverManager.apply(uri());
     }
 
     private String uri() {

liquigraph-core/src/main/java/org/liquigraph/core/exception/LiquigraphLockException.java

@@ -20,6 +20,8 @@
 public class LiquigraphLockException extends RuntimeException {
 
     public LiquigraphLockException(String message, SQLException cause) {
-        super(message, cause);
+        super(message, cause, true, true);
     }
+
+
 }

liquigraph-core/src/main/java/org/liquigraph/core/io/lock/LockableConnection.java

@@ -15,6 +15,8 @@
  */
 package org.liquigraph.core.io.lock;
 
+import com.google.common.annotations.VisibleForTesting;
+
 import java.sql.Array;
 import java.sql.Blob;
 import java.sql.CallableStatement;
@@ -34,6 +36,8 @@
 import java.util.Properties;
 import java.util.concurrent.Executor;
 
+import static com.google.common.base.Throwables.propagate;
+
 /**
  * This JDBC connection decorator writes a (:__LiquigraphLock)
  * Neo4j node in order to prevent concurrent executions.
@@ -42,28 +46,45 @@
  *
  * A shutdown hook is executed to remove the lock node if and
  * only if the connection has not been properly closed.
+ *
+ * Please note that any {@link Connection} passed to this decorator
+ * will be set to auto-commit: false`. The auto-commit property
+ * is restored on close. The latter is done in order to minimize
+ * side effects when connections are recycled by a connection pool,
+ * for instance.
  */
 public final class LockableConnection implements Connection {
     private final Connection delegate;
     private final LiquigraphLock lock;
+    private final boolean previousAutoCommit;
 
-    private LockableConnection(Connection delegate, LiquigraphLock lock) {
+    private LockableConnection(Connection delegate, boolean previousAutoCommit, LiquigraphLock lock) {
         this.delegate = delegate;
+        this.previousAutoCommit = previousAutoCommit;
         this.lock = lock;
     }
 
     public static LockableConnection acquire(Connection delegate, LiquigraphLock lock) {
-        LockableConnection connection = new LockableConnection(delegate, lock);
+        LockableConnection connection = null;
         try {
+            boolean previousAutoCommit = delegate.getAutoCommit();
+            delegate.setAutoCommit(false);
+
+            connection = new LockableConnection(delegate, previousAutoCommit, lock);
             lock.acquire(connection);
             return connection;
-        } catch (RuntimeException e) {
+        } catch (SQLException | RuntimeException e) {
             try {
-                connection.close();
+                if (connection != null) {
+                    connection.close();
+                }
+                else {
+                    delegate.close();
+                }
             } catch (SQLException exception) {
                 e.addSuppressed(exception);
             }
-            throw e;
+            throw propagate(e);
         }
     }
 
@@ -72,11 +93,18 @@ public static LockableConnection acquire(Connection delegate, LiquigraphLock loc
      * task shutdown hook before closing the underlying
      * connection.
      *
+     * Pending transactions are explicitly rolled back
+     * before resetting auto-commit. They could otherwise
+     * end up being committed in {@link this#delegate#close()}
+     * if auto-commit was reset to true.
+     *
      * @see ShutdownTask
      */
     @Override
     public void close() throws SQLException {
         lock.release(this);
+        rollback();
+        delegate.setAutoCommit(previousAutoCommit);
         delegate.close();
     }
 

liquigraph-core/src/test/java/org/liquigraph/core/io/lock/LockableConnectionTest.java

@@ -0,0 +1,112 @@
+/**
+ * Copyright 2014-2016 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.liquigraph.core.io.lock;
+
+import org.junit.Test;
+import org.liquigraph.core.exception.LiquigraphLockException;
+import org.mockito.InOrder;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.fail;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.inOrder;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+public class LockableConnectionTest {
+
+    @Test
+    public void disables_auto_commit() throws Exception {
+        Connection delegate = mock(Connection.class);
+
+        LockableConnection.acquire(delegate, mock(LiquigraphLock.class));
+
+        verify(delegate).setAutoCommit(false);
+    }
+
+    @Test
+    public void restores_auto_commit_upon_close() throws Exception {
+        Connection delegate = mock(Connection.class);
+        when(delegate.getAutoCommit()).thenReturn(true);
+
+        LockableConnection.acquire(delegate, mock(LiquigraphLock.class)).close();
+
+        InOrder inOrder = inOrder(delegate);
+        inOrder.verify(delegate).rollback();
+        inOrder.verify(delegate).setAutoCommit(true);
+        inOrder.verify(delegate).close(); // cannot set auto-commit after close
+    }
+
+    @Test
+    public void closes_connection_upon_lock_acquire_error() throws SQLException {
+        Connection connection = mock(Connection.class);
+        LiquigraphLock lock = mock(LiquigraphLock.class);
+        doThrow(LiquigraphLockException.class).when(lock).acquire(any(Connection.class));
+
+        try {
+            LockableConnection.acquire(connection, lock);
+            fail("Should propagate lock exception");
+        }
+        catch (RuntimeException ex) {
+            assertThat(ex).isInstanceOf(LiquigraphLockException.class);
+        }
+
+        verify(connection).close();
+    }
+
+    @Test
+    public void closes_connection_upon_autocommit_disabling_error() throws SQLException {
+        Connection connection = mock(Connection.class);
+        doThrow(SQLException.class).when(connection).setAutoCommit(false);
+        LiquigraphLock lock = mock(LiquigraphLock.class);
+
+        try {
+            LockableConnection.acquire(connection, lock);
+            fail("Should propagate autocommit exception");
+        }
+        catch (RuntimeException ex) {
+            assertThat(ex).hasCauseInstanceOf(SQLException.class);
+        }
+
+        verify(connection).close();
+    }
+
+    @Test
+    @SuppressWarnings("unchecked")
+    public void add_suppressed_close_exception_connection_upon_acquire_error() throws SQLException {
+        Connection connection = mock(Connection.class);
+        SQLException sqlException = mock(SQLException.class);
+        doThrow(sqlException).when(connection).close();
+        LiquigraphLock lock = mock(LiquigraphLock.class);
+        doThrow(new LiquigraphLockException("Locking will fail", sqlException)).when(lock).acquire(any(Connection.class));
+
+        try {
+            LockableConnection.acquire(connection, lock);
+            fail("Should add suppressed close exception");
+        }
+        catch (RuntimeException ex) {
+            assertThat(ex).isInstanceOf(LiquigraphLockException.class);
+            assertThat(ex.getSuppressed())
+                .containsExactly(sqlException);
+        }
+    }
+
+}