Merge pull request #6545 from simPod/df-c_v3

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

Author: greg0ire

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