Repository: doctrine/orm (php)

Issue:
Not throwing proper exception when in transaction
### Bug Report

|    Q        |   A
|------------ | ------
| Version     | 2.6.3

#### Summary

EntityManager tries to rollback the transaction when exception is thrown during commit operation. The rollback is performed through DBAL Connection. The problem is that the transaction is no longer active and when DBAL Connection tries to rollback through `PDOConnection`, _There is no active transaction_ exception is thrown instead and the original exception is lost. So user has no easy way to know actual exception and gets _There is no active transaction_ instead.

#### Current behavior

1. `PDOException` _A_ `SQLSTATE[23505]: Unique violation: 7 ERROR:  duplicate key value violates unique constraint ...` occurs on commit in https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/EntityManager.php#L238
2. Transaction is attempting to rollback https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/EntityManager.php#L243 before exception _A_ is thrown on https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/EntityManager.php#L245
3. DBAL `Connection::rollback()` is called where `$this->_conn->rollBack();` is called. `_conn` is instance of `PDOConnection` that throws _There is no active transaction_ exception instead and the original exception is lost.

#### Expected behavior

The original exception is properly thrown.



Pull request description:
Run risky code in finally block
`catch` blocks are not supposed to fail. If you want to do something despite an exception happening, you should do it in a `finally` block.

(finally) Fixes #7545

Patch:
diff --git a/src/EntityManager.php b/src/EntityManager.php
index f7d47d7b1..b677c79dc 100644
--- a/src/EntityManager.php
+++ b/src/EntityManager.php
@@ -32,7 +32,6 @@ use Doctrine\ORM\Repository\RepositoryFactory;
 use Doctrine\Persistence\Mapping\MappingException;
 use Doctrine\Persistence\ObjectRepository;
 use InvalidArgumentException;
-use Throwable;
 
 use function array_keys;
 use function class_exists;
@@ -246,18 +245,24 @@ class EntityManager implements EntityManagerInterface
 
         $this->conn->beginTransaction();
 
+        $successful = false;
+
         try {
             $return = $func($this);
 
             $this->flush();
             $this->conn->commit();
 
-            return $return ?: true;
-        } catch (Throwable $e) {
-            $this->close();
-            $this->conn->rollBack();
+            $successful = true;
 
-            throw $e;
+            return $return ?: true;
+        } finally {
+            if (! $successful) {
+                $this->close();
+                if ($this->conn->isTransactionActive()) {
+                    $this->conn->rollBack();
+                }
+            }
         }
     }
 
@@ -268,18 +273,24 @@ class EntityManager implements EntityManagerInterface
     {
         $this->conn->beginTransaction();
 
+        $successful = false;
+
         try {
             $return = $func($this);
 
             $this->flush();
             $this->conn->commit();
 
-            return $return;
-        } catch (Throwable $e) {
-            $this->close();
-            $this->conn->rollBack();
+            $successful = true;
 
-            throw $e;
+            return $return;
+        } finally {
+            if (! $successful) {
+                $this->close();
+                if ($this->conn->isTransactionActive()) {
+                    $this->conn->rollBack();
+                }
+            }
         }
     }
 
diff --git a/src/Query/Exec/SingleTableDeleteUpdateExecutor.php b/src/Query/Exec/SingleTableDeleteUpdateExecutor.php
index 4a4a29864..7f7496e39 100644
--- a/src/Query/Exec/SingleTableDeleteUpdateExecutor.php
+++ b/src/Query/Exec/SingleTableDeleteUpdateExecutor.php
@@ -14,6 +14,8 @@ use Doctrine\ORM\Query\SqlWalker;
  * that are mapped to a single table.
  *
  * @link        www.doctrine-project.org
+ *
+ * @todo This is exactly the same as SingleSelectExecutor. Unify in SingleStatementExecutor.
  */
 class SingleTableDeleteUpdateExecutor extends AbstractSqlExecutor
 {
diff --git a/src/UnitOfWork.php b/src/UnitOfWork.php
index 39ba6b68b..97d80862e 100644
--- a/src/UnitOfWork.php
+++ b/src/UnitOfWork.php
@@ -49,7 +49,6 @@ use Doctrine\Persistence\PropertyChangedListener;
 use Exception;
 use InvalidArgumentException;
 use RuntimeException;
-use Throwable;
 use UnexpectedValueException;
 
 use function array_chunk;
@@ -427,6 +426,8 @@ class UnitOfWork implements PropertyChangedListener
         $conn = $this->em->getConnection();
         $conn->beginTransaction();
 
+        $successful = false;
+
         try {
             // Collection deletions (deletions of complete collections)
             foreach ($this->collectionDeletions as $collectionToDelete) {
@@ -478,16 +479,18 @@ class UnitOfWork implements PropertyChangedListener
 
                 throw new OptimisticLockException('Commit failed', $object);
             }
-        } catch (Throwable $e) {
-            $this->em->close();
 
-            if ($conn->isTransactionActive()) {
-                $conn->rollBack();
-            }
+            $successful = true;
+        } finally {
+            if (! $successful) {
+                $this->em->close();
 
-            $this->afterTransactionRolledBack();
+                if ($conn->isTransactionActive()) {
+                    $conn->rollBack();
+                }
 
-            throw $e;
+                $this->afterTransactionRolledBack();
+            }
         }
 
         $this->afterTransactionComplete();


Test patch:
diff --git a/tests/Tests/ORM/EntityManagerTest.php b/tests/Tests/ORM/EntityManagerTest.php
index c3ad9f559..88ff5ed92 100644
--- a/tests/Tests/ORM/EntityManagerTest.php
+++ b/tests/Tests/ORM/EntityManagerTest.php
@@ -21,16 +21,21 @@ use Doctrine\ORM\QueryBuilder;
 use Doctrine\ORM\UnitOfWork;
 use Doctrine\Persistence\Mapping\Driver\MappingDriver;
 use Doctrine\Persistence\Mapping\MappingException;
+use Doctrine\Tests\Mocks\ConnectionMock;
+use Doctrine\Tests\Mocks\EntityManagerMock;
 use Doctrine\Tests\Models\CMS\CmsUser;
 use Doctrine\Tests\Models\GeoNames\Country;
 use Doctrine\Tests\OrmTestCase;
+use Exception;
 use Generator;
 use InvalidArgumentException;
+use PHPUnit\Framework\Assert;
 use stdClass;
 use TypeError;
 
 use function get_class;
 use function random_int;
+use function sprintf;
 use function sys_get_temp_dir;
 use function uniqid;
 
@@ -382,4 +387,61 @@ class EntityManagerTest extends OrmTestCase
 
         $this->entityManager->flush($entity);
     }
+
+    /** @dataProvider entityManagerMethodNames */
+    public function testItPreservesTheOriginalExceptionOnRollbackFailure(string $methodName): void
+    {
+        $entityManager = new EntityManagerMock(new class extends ConnectionMock {
+            public function rollBack(): bool
+            {
+                throw new Exception('Rollback exception');
+            }
+        });
+
+        try {
+            $entityManager->transactional(static function (): void {
+                throw new Exception('Original exception');
+            });
+            self::fail('Exception expected');
+        } catch (Exception $e) {
+            self::assertSame('Rollback exception', $e->getMessage());
+            self::assertNotNull($e->getPrevious());
+            self::assertSame('Original exception', $e->getPrevious()->getMessage());
+        }
+    }
+
+    /** @dataProvider entityManagerMethodNames */
+    public function testItDoesNotAttemptToRollbackIfNoTransactionIsActive(string $methodName): void
+    {
+        $entityManager = new EntityManagerMock(
+            new class extends ConnectionMock {
+                public function commit(): bool
+                {
+                    throw new Exception('Commit exception that happens after doing the actual commit');
+                }
+
+                public function rollBack(): bool
+                {
+                    Assert::fail('Should not attempt to rollback if no transaction is active');
+                }
+
+                public function isTransactionActive(): bool
+                {
+                    return false;
+                }
+            }
+        );
+
+        $this->expectExceptionMessage('Commit exception');
+        $entityManager->$methodName(static function (): void {
+        });
+    }
+
+    /** @return Generator<string, array{string}> */
+    public function entityManagerMethodNames(): Generator
+    {
+        foreach (['transactional', 'wrapInTransaction'] as $methodName) {
+            yield sprintf('%s()', $methodName) => [$methodName];
+        }
+    }
 }
diff --git a/tests/Tests/ORM/UnitOfWorkTest.php b/tests/Tests/ORM/UnitOfWorkTest.php
index ee475e729..ae6b1d282 100644
--- a/tests/Tests/ORM/UnitOfWorkTest.php
+++ b/tests/Tests/ORM/UnitOfWorkTest.php
@@ -41,6 +41,7 @@ use Doctrine\Tests\Models\GeoNames\City;
 use Doctrine\Tests\Models\GeoNames\Country;
 use Doctrine\Tests\OrmTestCase;
 use Doctrine\Tests\PHPUnitCompatibility\MockBuilderCompatibilityTools;
+use Exception;
 use PHPUnit\Framework\MockObject\MockObject;
 use stdClass;
 
@@ -971,6 +972,43 @@ class UnitOfWorkTest extends OrmTestCase
 
         $this->_unitOfWork->persist($phone2);
     }
+
+    public function testItPreservesTheOriginalExceptionOnRollbackFailure(): void
+    {
+        $this->_connectionMock = new class extends ConnectionMock {
+            public function commit(): bool
+            {
+                return false; // this should cause an exception
+            }
+
+            public function rollBack(): bool
+            {
+                throw new Exception('Rollback exception');
+            }
+        };
+        $this->_emMock         = new EntityManagerMock($this->_connectionMock);
+        $this->_unitOfWork     = new UnitOfWorkMock($this->_emMock);
+        $this->_emMock->setUnitOfWork($this->_unitOfWork);
+
+        // Setup fake persister and id generator
+        $userPersister = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata(ForumUser::class));
+        $userPersister->setMockIdGeneratorType(ClassMetadata::GENERATOR_TYPE_IDENTITY);
+        $this->_unitOfWork->setEntityPersister(ForumUser::class, $userPersister);
+
+        // Create a test user
+        $user           = new ForumUser();
+        $user->username = 'Jasper';
+        $this->_unitOfWork->persist($user);
+
+        try {
+            $this->_unitOfWork->commit();
+            self::fail('Exception expected');
+        } catch (Exception $e) {
+            self::assertSame('Rollback exception', $e->getMessage());
+            self::assertNotNull($e->getPrevious());
+            self::assertSame('Commit failed', $e->getPrevious()->getMessage());
+        }
+    }
 }
 
 /** @Entity */