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

src/Connection.php

@@ -12,12 +12,17 @@
 use Doctrine\DBAL\Connection\StaticServerVersionProvider;
 use Doctrine\DBAL\Driver\API\ExceptionConverter;
 use Doctrine\DBAL\Driver\Connection as DriverConnection;
+use Doctrine\DBAL\Driver\Exception as TheDriverException;
 use Doctrine\DBAL\Driver\Statement as DriverStatement;
 use Doctrine\DBAL\Exception\CommitFailedRollbackOnly;
 use Doctrine\DBAL\Exception\ConnectionLost;
+use Doctrine\DBAL\Exception\DeadlockException;
 use Doctrine\DBAL\Exception\DriverException;
+use Doctrine\DBAL\Exception\ForeignKeyConstraintViolationException;
 use Doctrine\DBAL\Exception\NoActiveTransaction;
 use Doctrine\DBAL\Exception\SavepointsNotSupported;
+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;
@@ -931,16 +936,35 @@
 
         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) {
+            $shouldRollback = ! (
+                $t instanceof TransactionRolledBack
+                || $t instanceof UniqueConstraintViolationException
+                || $t instanceof ForeignKeyConstraintViolationException
+                || $t instanceof DeadlockException
+            );
+
+            throw $t;
+        } finally {
+            if ($shouldRollback) {
+                $this->rollBack();
+            }
+        }
+
+        return $res;
     }
 
     /**
@@ -1019,17 +1043,26 @@
 
         $connection = $this->connect();
 
-        if ($this->transactionNestingLevel === 1) {
-            try {
-                $connection->commit();
-            } catch (Driver\Exception $e) {
-                throw $this->convertException($e);
+        try {
+            if ($this->transactionNestingLevel === 1) {
+                try {
+                    $connection->commit();
+                } catch (Driver\Exception $e) {
+                    throw $this->convertException($e);
+                }
+            } else {
+                $this->releaseSavepoint($this->_getNestedTransactionSavePointName());
             }
-        } else {
-            $this->releaseSavepoint($this->_getNestedTransactionSavePointName());
+        } finally {
+            $this->updateTransactionStateAfterCommit();
         }
+    }
 
-        --$this->transactionNestingLevel;
+    private function updateTransactionStateAfterCommit(): void
+    {
+        if ($this->transactionNestingLevel !== 0) {
+            --$this->transactionNestingLevel;
+        }
 
         if ($this->autoCommit !== false || $this->transactionNestingLevel !== 0) {
             return;

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\Exception as DriverPDOException;
 use Doctrine\DBAL\Exception\ConnectionException;
 use Doctrine\DBAL\Exception\DatabaseDoesNotExist;
 use Doctrine\DBAL\Exception\DatabaseObjectNotFoundException;
@@ -17,9 +19,15 @@
 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 assert;
+use function count;
+use function explode;
+use function str_replace;
+
 /** @internal */
 final class ExceptionConverter implements ExceptionConverterInterface
 {
@@ -40,6 +48,26 @@
             12545 => new ConnectionException($exception, $query),
             1400 => new NotNullConstraintViolationException($exception, $query),
             1918 => new DatabaseDoesNotExist($exception, $query),
+            2091 => (function () use ($exception, $query) {
+                //SQLSTATE[HY000]: General error: 2091 OCITransCommit: ORA-02091: transaction rolled back
+                //ORA-00001: unique constraint (DOCTRINE.GH3423_UNIQUE) violated
+                $lines = explode("\n", $exception->getMessage(), 2);
+                assert(count($lines) >= 2);
+
+                [, $causeError] = $lines;
+
+                [$causeCode] = explode(': ', $causeError, 2);
+                $code        = (int) str_replace('ORA-', '', $causeCode);
+
+                $sqlState = $exception->getSQLState();
+                if ($exception instanceof DriverPDOException) {
+                    $why = $this->convert(new DriverPDOException($causeError, $sqlState, $code, $exception), $query);
+                } else {
+                    $why = $this->convert(new Error($causeError, $sqlState, $code, $exception), $query);
+                }
+
+                return new TransactionRolledBack($why, $query);
+            })(),
             2289,
             2443,
             4080 => new DatabaseObjectNotFoundException($exception, $query),

src/Driver/OCI8/Connection.php

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

src/Driver/OCI8/Result.php

@@ -14,6 +14,7 @@
 use function oci_error;
 use function oci_fetch_all;
 use function oci_fetch_array;
+use function oci_field_name;
 use function oci_num_fields;
 use function oci_num_rows;
 

src/Exception/TransactionRolledBack.php

@@ -0,0 +1,10 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\DBAL\Exception;
+
+/** @psalm-immutable */
+class TransactionRolledBack extends DriverException
+{
+}

tests/ConnectionTest.php

@@ -675,4 +675,26 @@ 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 {
+        });
+    }
 }