Fix incorrect transactional() handling when DB auto-rolled back the transaction

Let's get rid of There's no active transaction exception which occurs e.g. when using deferred constraints so the violation is checked at the end of the transaction and not during it.

- Do not rollback only on exceptions where we know the transaction is no longer active
- Introduce TransactionRolledBack exception
- Transform Oracle's "transaction rolled back" exception and use the underlying one that DBAL supports

Author: simPod

src/Connection.php

@@ -9,14 +9,19 @@
 use Doctrine\DBAL\Cache\QueryCacheProfile;
 use Doctrine\DBAL\Driver\API\ExceptionConverter;
 use Doctrine\DBAL\Driver\Connection as DriverConnection;
+use Doctrine\DBAL\Driver\Exception as TheDriverException;
 use Doctrine\DBAL\Driver\ServerInfoAwareConnection;
 use Doctrine\DBAL\Driver\Statement as DriverStatement;
 use Doctrine\DBAL\Event\TransactionBeginEventArgs;
 use Doctrine\DBAL\Event\TransactionCommitEventArgs;
 use Doctrine\DBAL\Event\TransactionRollBackEventArgs;
 use Doctrine\DBAL\Exception\ConnectionLost;
+use Doctrine\DBAL\Exception\DeadlockException;
 use Doctrine\DBAL\Exception\DriverException;
+use Doctrine\DBAL\Exception\ForeignKeyConstraintViolationException;
 use Doctrine\DBAL\Exception\InvalidArgumentException;
+use Doctrine\DBAL\Exception\TransactionRolledBack;
+use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
 use Doctrine\DBAL\Platforms\AbstractPlatform;
 use Doctrine\DBAL\Query\Expression\ExpressionBuilder;
 use Doctrine\DBAL\Query\QueryBuilder;
@@ -1283,16 +1288,36 @@ public function transactional(Closure $func)
 
         try {
             $res = $func($this);
-            $this->commit();
 
             $successful = true;
-
-            return $res;
         } finally {
             if (! $successful) {
                 $this->rollBack();
             }
         }
+
+        $shouldRollback = true;
+        try {
+            $this->commit();
+
+            $shouldRollback = false;
+        } catch (TheDriverException $t) {
+            $convertedException = $this->handleDriverException($t, null);
+            $shouldRollback     = ! (
+                $convertedException instanceof TransactionRolledBack
+                || $convertedException instanceof UniqueConstraintViolationException
+                || $convertedException instanceof ForeignKeyConstraintViolationException
+                || $convertedException instanceof DeadlockException
+            );
+
+            throw $t;
+        } finally {
+            if ($shouldRollback) {
+                $this->rollBack();
+            }
+        }
+
+        return $res;
     }
 
     /**
@@ -1424,12 +1449,21 @@ public function commit()
 
         $connection = $this->getWrappedConnection();
 
-        if ($this->transactionNestingLevel === 1) {
-            $result = $this->doCommit($connection);
-        } elseif ($this->nestTransactionsWithSavepoints) {
-            $this->releaseSavepoint($this->_getNestedTransactionSavePointName());
+        try {
+            if ($this->transactionNestingLevel === 1) {
+                $result = $this->doCommit($connection);
+            } elseif ($this->nestTransactionsWithSavepoints) {
+                $this->releaseSavepoint($this->_getNestedTransactionSavePointName());
+            }
+        } finally {
+            $this->updateTransactionStateAfterCommit();
         }
 
+        return $result;
+    }
+
+    private function updateTransactionStateAfterCommit(): void
+    {
         --$this->transactionNestingLevel;
 
         $eventManager = $this->getEventManager();
@@ -1446,12 +1480,10 @@ public function commit()
         }
 
         if ($this->autoCommit !== false || $this->transactionNestingLevel !== 0) {
-            return $result;
+            return;
         }
 
         $this->beginTransaction();
-
-        return $result;
     }
 
     /**

src/Driver/API/OCI/ExceptionConverter.php

@@ -6,6 +6,8 @@
 
 use Doctrine\DBAL\Driver\API\ExceptionConverter as ExceptionConverterInterface;
 use Doctrine\DBAL\Driver\Exception;
+use Doctrine\DBAL\Driver\OCI8\Exception\Error;
+use Doctrine\DBAL\Driver\PDO\PDOException;
 use Doctrine\DBAL\Exception\ConnectionException;
 use Doctrine\DBAL\Exception\DatabaseDoesNotExist;
 use Doctrine\DBAL\Exception\DatabaseObjectNotFoundException;
@@ -17,16 +19,30 @@
 use Doctrine\DBAL\Exception\SyntaxErrorException;
 use Doctrine\DBAL\Exception\TableExistsException;
 use Doctrine\DBAL\Exception\TableNotFoundException;
+use Doctrine\DBAL\Exception\TransactionRolledBack;
 use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
 use Doctrine\DBAL\Query;
 
+use function explode;
+use function str_replace;
+
 /** @internal */
 final class ExceptionConverter implements ExceptionConverterInterface
 {
     /** @link http://www.dba-oracle.com/t_error_code_list.htm */
     public function convert(Exception $exception, ?Query $query): DriverException
     {
-        switch ($exception->getCode()) {
+        /** @psalm-var int|'HY000' $code */
+        $code = $exception->getCode();
+        /** @psalm-suppress NoInterfaceProperties */
+        if ($code === 'HY000' && isset($exception->errorInfo[1], $exception->errorInfo[2])) {
+            $errorInfo            = $exception->errorInfo;
+            $exception            = new PDOException($errorInfo[2], $errorInfo[1]);
+            $exception->errorInfo = $errorInfo;
+            $code                 = $exception->getCode();
+        }
+
+        switch ($code) {
             case 1:
             case 2299:
             case 38911:
@@ -58,6 +74,22 @@ public function convert(Exception $exception, ?Query $query): DriverException
             case 1918:
                 return new DatabaseDoesNotExist($exception, $query);
 
+            case 2091:
+                //ORA-02091: transaction rolled back
+                //ORA-00001: unique constraint (DOCTRINE.GH3423_UNIQUE) violated
+                [, $causeError] = explode("\n", $exception->getMessage(), 2);
+
+                [$causeCode] = explode(': ', $causeError, 2);
+                $code        = (int) str_replace('ORA-', '', $causeCode);
+
+                if ($exception instanceof PDOException) {
+                    $why = $this->convert(new PDOException($causeError, $code, $exception), $query);
+                } else {
+                    $why = $this->convert(new Error($causeError, null, $code, $exception), $query);
+                }
+
+                return new TransactionRolledBack($why, $query);
+
             case 2289:
             case 2443:
             case 4080:

src/Driver/OCI8/Connection.php

@@ -142,7 +142,7 @@ public function beginTransaction(): bool
 
     public function commit(): bool
     {
-        if (! oci_commit($this->connection)) {
+        if (! @oci_commit($this->connection)) {
             throw Error::new($this->connection);
         }
 

src/Exception/TransactionRolledBack.php

@@ -0,0 +1,8 @@
+<?php
+
+namespace Doctrine\DBAL\Exception;
+
+/** @psalm-immutable */
+class TransactionRolledBack extends DriverException
+{
+}

tests/ConnectionTest.php

@@ -1095,6 +1095,28 @@ public function rollBack(): void
             self::assertSame('Original exception', $e->getPrevious()->getMessage());
         }
     }
+
+    /**
+     * We are not sure if this can happen in real life scenario
+     */
+    public function testItFailsDuringCommitBeforeTouchingDb(): void
+    {
+        $connection = new class (['memory' => true], new Driver\SQLite3\Driver()) extends Connection {
+            public function commit(): void
+            {
+                throw new \Exception('Fail before touching the db');
+            }
+
+            public function rollBack(): void
+            {
+                throw new \Exception('Rollback got triggered');
+            }
+        };
+
+        $this->expectExceptionMessage('Rollback got triggered');
+        $connection->transactional(static function (): void {
+        });
+    }
 }
 
 interface ConnectDispatchEventListener