Fix nested transactions may be rolled back or committed unexpectedly

Refs #10348

src/Database/Connection.php

@@ -18,6 +18,7 @@ use Cake\Core\App;
 use Cake\Database\Exception\MissingConnectionException;
 use Cake\Database\Exception\MissingDriverException;
 use Cake\Database\Exception\MissingExtensionException;
+use Cake\Database\Exception\NestedTransactionRollbackException;
 use Cake\Database\Log\LoggedQuery;
 use Cake\Database\Log\LoggingStatement;
 use Cake\Database\Log\QueryLogger;
@@ -92,6 +93,14 @@ class Connection implements ConnectionInterface
     protected $_schemaCollection = null;
 
     /**
+     * NestedTransactionRollbackException object instance, will be stored if
+     * the rollback method is called in some nested transaction.
+     *
+     * @var \Cake\Database\Exception\NestedTransactionRollbackException|null
+     */
+    protected $_nestedTransactionRollbackException = null;
+
+    /**
      * Constructor.
      *
      * @param array $config configuration for connecting to database
@@ -441,6 +450,7 @@ class Connection implements ConnectionInterface
             $this->_driver->beginTransaction();
             $this->_transactionLevel = 0;
             $this->_transactionStarted = true;
+            $this->_nestedTransactionRollbackException = null;
 
             return;
         }
@@ -463,7 +473,12 @@ class Connection implements ConnectionInterface
         }
 
         if ($this->_transactionLevel === 0) {
+            if ($this->wasNestedTransactionRolledback()) {
+                throw $this->_nestedTransactionRollbackException;
+            }
+
             $this->_transactionStarted = false;
+            $this->_nestedTransactionRollbackException = null;
             if ($this->_logQueries) {
                 $this->log('COMMIT');
             }
@@ -482,18 +497,24 @@ class Connection implements ConnectionInterface
     /**
      * Rollback current transaction.
      *
+     * @param bool|null $toBeginning Whether or not the transaction should be rolled back to the
+     * beginning of it. Defaults to false if using savepoints, or true if not.
      * @return bool
      */
-    public function rollback()
+    public function rollback($toBeginning = null)
     {
         if (!$this->_transactionStarted) {
             return false;
         }
 
         $useSavePoint = $this->isSavePointsEnabled();
-        if ($this->_transactionLevel === 0 || !$useSavePoint) {
+        if ($toBeginning === null) {
+            $toBeginning = !$useSavePoint;
+        }
+        if ($this->_transactionLevel === 0 || $toBeginning) {
             $this->_transactionLevel = 0;
             $this->_transactionStarted = false;
+            $this->_nestedTransactionRollbackException = null;
             if ($this->_logQueries) {
                 $this->log('ROLLBACK');
             }
@@ -502,8 +523,11 @@ class Connection implements ConnectionInterface
             return true;
         }
 
+        $savePoint = $this->_transactionLevel--;
         if ($useSavePoint) {
-            $this->rollbackSavepoint($this->_transactionLevel--);
+            $this->rollbackSavepoint($savePoint);
+        } elseif ($this->_nestedTransactionRollbackException === null) {
+            $this->_nestedTransactionRollbackException = new NestedTransactionRollbackException();
         }
 
         return true;
@@ -653,22 +677,37 @@ class Connection implements ConnectionInterface
         try {
             $result = $callback($this);
         } catch (\Exception $e) {
-            $this->rollback();
+            $this->rollback(false);
             throw $e;
         }
 
         if ($result === false) {
-            $this->rollback();
+            $this->rollback(false);
 
             return false;
         }
 
-        $this->commit();
+        try {
+            $this->commit();
+        } catch (NestedTransactionRollbackException $e) {
+            $this->rollback(false);
+            throw $e;
+        }
 
         return $result;
     }
 
     /**
+     * Returns whether some nested transaction has been already rolled back.
+     *
+     * @return bool
+     */
+    public function wasNestedTransactionRolledback()
+    {
+        return $this->_nestedTransactionRollbackException instanceof NestedTransactionRollbackException;
+    }
+
+    /**
      * {@inheritDoc}
      *
      * ### Example:

src/Database/Exception/NestedTransactionRollbackException.php

@@ -0,0 +1,36 @@
+<?php
+/**
+ * CakePHP(tm) : Rapid Development Framework (http://cakephp.org)
+ * Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org)
+ *
+ * Licensed under The MIT License
+ * For full copyright and license information, please see the LICENSE.txt
+ * Redistributions of files must retain the above copyright notice.
+ *
+ * @copyright     Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org)
+ * @link          http://cakephp.org CakePHP(tm) Project
+ * @since         3.4.3
+ * @license       http://www.opensource.org/licenses/mit-license.php MIT License
+ */
+namespace Cake\Database\Exception;
+
+use Cake\Core\Exception\Exception;
+
+class NestedTransactionRollbackException extends Exception
+{
+
+    /**
+     * Constructor
+     *
+     * @param string|null $message If no message is given a default meesage will be used.
+     * @param int $code Status code, defaults to 500.
+     * @param \Exception|null $previous the previous exception.
+     */
+    public function __construct($message = null, $code = 500, $previous = null)
+    {
+        if ($message === null) {
+            $message = 'Cannot commit transaction - rollback() has been already called in the nested transaction';
+        }
+        parent::__construct($message, $code, $previous);
+    }
+}

tests/TestCase/Database/ConnectionTest.php

@@ -17,6 +17,7 @@ namespace Cake\Test\TestCase\Database;
 use Cake\Core\Configure;
 use Cake\Database\Connection;
 use Cake\Database\Driver\Mysql;
+use Cake\Database\Exception\NestedTransactionRollbackException;
 use Cake\Datasource\ConnectionManager;
 use Cake\TestSuite\TestCase;
 
@@ -987,4 +988,116 @@ class ConnectionTest extends TestCase
         $connection->schemaCollection($schema);
         $this->assertSame($schema, $connection->schemaCollection());
     }
+
+    /**
+     * Tests that allowed nesting of commit/rollback operations doesn't
+     * throw any exceptions.
+     *
+     * @return void
+     */
+    public function testNestedTransactionRollbackExceptionNotThrown()
+    {
+        $this->connection->transactional(function () {
+            $this->connection->transactional(function () {
+                return true;
+            });
+
+            return true;
+        });
+        $this->assertFalse($this->connection->inTransaction());
+
+        $this->connection->transactional(function () {
+            $this->connection->transactional(function () {
+                return true;
+            });
+
+            return false;
+        });
+        $this->assertFalse($this->connection->inTransaction());
+
+        $this->connection->transactional(function () {
+            $this->connection->transactional(function () {
+                return false;
+            });
+
+            return false;
+        });
+        $this->assertFalse($this->connection->inTransaction());
+    }
+
+    /**
+     * Tests that not allowed nesting of commit/rollback operations throws
+     * a NestedTransactionRollbackException.
+     *
+     * @return void
+     * @expectedException \Cake\Database\Exception\NestedTransactionRollbackException
+     * @expectedExceptionMessage Cannot commit transaction - rollback() has been already called in the nested transaction
+     */
+    public function testNestedTransactionRollbackExceptionThrown()
+    {
+        // ROLLBACK -> COMMIT
+        $this->connection->transactional(function () {
+            $this->connection->transactional(function () {
+                return false;
+            });
+
+            return true;
+        });
+    }
+
+    /**
+     * Tests mor detail about that not allowed nesting of rollback/commit
+     * operations throws a NestedTransactionRollbackException by trace.
+     *
+     * @return void
+     */
+    public function testNestedTransactionRollbackExceptionByTrace()
+    {
+        $rollbackSourceLine = -1;
+        $nestedTransactionStates = [];
+
+        $e = null;
+        try {
+            $this->connection->transactional(function () use (&$rollbackSourceLine, &$nestedTransactionStates) {
+                $nestedTransactionStates[] = $this->connection->wasNestedTransactionRolledback();
+
+                $this->connection->transactional(function () {
+                    return true;
+                });
+
+                $this->connection->transactional(function () use (&$rollbackSourceLine, &$nestedTransactionStates) {
+                    $nestedTransactionStates[] = $this->connection->wasNestedTransactionRolledback();
+
+                    $this->connection->transactional(function () {
+                        return false;
+                    });
+                    $rollbackSourceLine = __LINE__ - 1;
+
+                    $nestedTransactionStates[] = $this->connection->wasNestedTransactionRolledback();
+
+                    return true;
+                });
+
+                $this->connection->transactional(function () {
+                    return false;
+                });
+
+                $nestedTransactionStates[] = $this->connection->wasNestedTransactionRolledback();
+
+                return true;
+            });
+        } catch (NestedTransactionRollbackException $e) {
+        }
+
+        $nestedTransactionStates[] = $this->connection->wasNestedTransactionRolledback();
+
+        $this->assertInstanceOf(NestedTransactionRollbackException::class, $e);
+
+        $this->assertSame([false, false, true, true, false], $nestedTransactionStates);
+        $this->assertFalse($this->connection->inTransaction());
+
+        $trace = $e->getTrace();
+        $this->assertEquals(__FILE__, $trace[1]['file']);
+        $this->assertEquals($rollbackSourceLine, $trace[1]['line']);
+    }
 }