From c6fd2d59e42c367badcecf8f92f140d3fd95fe30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Mon, 16 May 2022 22:31:46 +0200 Subject: [PATCH] Remove support for transaction nesting without savepoints --- UPGRADE.md | 19 +++ docs/en/reference/transactions.rst | 119 ++---------------- src/Connection.php | 67 ++++------ ...TransactionWithSavepointsInTransaction.php | 18 --- tests/ConnectionTest.php | 25 +++- tests/Functional/ConnectionTest.php | 72 ++--------- 6 files changed, 84 insertions(+), 236 deletions(-) delete mode 100644 src/Exception/MayNotAlterNestedTransactionWithSavepointsInTransaction.php diff --git a/UPGRADE.md b/UPGRADE.md index 868187d9ef1..43a3af8c865 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -8,6 +8,25 @@ awareness about deprecated code. # Upgrade to 4.0 +## BC BREAK: remove support for transaction nesting without savepoints + +Starting a transaction inside another transaction with +`Doctrine\DBAL\Connection::beginTransaction()` now always results in +savepoints being used. + +In case your platform does not support savepoints, you will have to +rework your application logic so as to avoid nested transaction blocks. + +## Deprecated: configuration methods related to transaction nesting + +Since it is no longer possible to configure whether transaction nesting is +emulated with savepoints or not, configuring that behavior has no effect and it +is deprecated to attempt to change it or to know how it is configured. As a +result, the following methods are deprecated: + +- `Connection::setNestTransactionsWithSavepoints()` +- `Connection::getNestTransactionsWithSavepoints()` + ## BC BREAK: Auto-increment columns on PostgreSQL are implemented as `IDENTITY`, not `SERIAL`. Instead of using `SERIAL*` column types for `autoincrement` columns, the DBAL will now use diff --git a/docs/en/reference/transactions.rst b/docs/en/reference/transactions.rst index 5dc03048e0d..adfcf6c6d36 100644 --- a/docs/en/reference/transactions.rst +++ b/docs/en/reference/transactions.rst @@ -67,48 +67,12 @@ Transaction Nesting ------------------- Calling ``beginTransaction()`` while already in a transaction will -result in two very different behaviors depending on whether transaction -nesting with savepoints is enabled or not. In both cases though, there -won't be an actual transaction inside a transaction, even if your RDBMS -supports it. There is always only a single, real database transaction. +not result in an actual transaction inside a transaction, even if your +RDBMS supports it. Instead, transaction nesting is emulated by resorting +to SQL savepoints. There is always only a single, real database +transaction. -By default, transaction nesting at the SQL level with savepoints is -disabled. The value for that setting can be set on a per-connection -basis, with -``Doctrine\DBAL\Connection#setNestTransactionsWithSavepoints()``. - -Nesting transactions without savepoints is deprecated, but is the -default behavior for backward compatibility reasons. - -Dummy mode -~~~~~~~~~~ -.. warning:: - - This behavior is deprecated, avoid it with - ``Doctrine\DBAL\Connection#setNestTransactionsWithSavepoints(true)``. - -When transaction nesting with savepoints is disabled, what happens is -not so much transaction nesting as propagating transaction control up -the call stack. For that purpose, the ``Connection`` class keeps an -internal counter that represents the nesting level and is -increased/decreased as ``beginTransaction()``, ``commit()`` and -``rollBack()`` are invoked. ``beginTransaction()`` increases the nesting -level whilst ``commit()`` and ``rollBack()`` decrease the nesting level. -The nesting level starts at 0. -Whenever the nesting level transitions from 0 to 1, -``beginTransaction()`` is invoked on the underlying driver connection -and whenever the nesting level transitions from 1 to 0, ``commit()`` or -``rollBack()`` is invoked on the underlying driver, depending on whether -the transition was caused by ``Connection#commit()`` or -``Connection#rollBack()``. - -What this means is that transaction control is basically passed to -code higher up in the call stack and the inner transaction block does -not actually result in an SQL transaction. It is not ignored either -though. - -To visualize what this means in practice, consider the following -example: +Let's examine what happens with an example :: @@ -121,13 +85,13 @@ example: // nested transaction block, this might be in some other API/library code that is // unaware of the outer transaction. - $conn->beginTransaction(); // 1 => 2 + $conn->beginTransaction(); // 1 => 2, savepoint created try { ... $conn->commit(); // 2 => 1 } catch (\Exception $e) { - $conn->rollBack(); // 2 => 1, transaction marked for rollback only + $conn->rollBack(); // 2 => 1, rollback to savepoint throw $e; } @@ -139,33 +103,9 @@ example: throw $e; } -However, **a rollback in a nested transaction block will always mark the -current transaction so that the only possible outcome of the transaction -is to be rolled back**. -That means in the above example, the rollback in the inner -transaction block marks the whole transaction for rollback only. -Even if the nested transaction block would not rethrow the -exception, the transaction is marked for rollback only and the -commit of the outer transaction would trigger an exception, leading -to the final rollback. This also means that you cannot -successfully commit some changes in an outer transaction if an -inner transaction block fails and issues a rollback, even if this -would be the desired behavior (i.e. because the nested operation is -"optional" for the purpose of the outer transaction block). To -achieve that, you need to resort to transaction nesting with savepoint. - -All that is guaranteed to the inner transaction is that it still -happens atomically, all or nothing, the transaction just gets a -wider scope and the control is handed to the outer scope. - -.. note:: - - The transaction nesting described here is a debated - feature that has its critics. Form your own opinion. We recommend - avoiding nesting transaction blocks when possible, and most of the - time, it is possible. Transaction control should mostly be left to - a service layer and not be handled in data access objects or - similar. +Everything is handled at the SQL level: the main transaction is not +marked for rollback only, but the inner emulated transaction is rolled +back to the savepoint. .. warning:: @@ -177,45 +117,6 @@ wider scope and the control is handed to the outer scope. nesting level, causing errors with broken transaction boundaries that may be hard to debug. -Emulated Transaction Nesting with Savepoints -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -Let's now examine what happens when transaction nesting with savepoints -is enabled, with the same example as above - -:: - - beginTransaction(); // 0 => 1, "real" transaction started - try { - - ... - - // nested transaction block, this might be in some other API/library code that is - // unaware of the outer transaction. - $conn->beginTransaction(); // 1 => 2, savepoint created - try { - ... - - $conn->commit(); // 2 => 1 - } catch (\Exception $e) { - $conn->rollBack(); // 2 => 1, rollback to savepoint - throw $e; - } - - ... - - $conn->commit(); // 1 => 0, "real" transaction committed - } catch (\Exception $e) { - $conn->rollBack(); // 1 => 0, "real" transaction rollback - throw $e; - } - -This time, everything is handled at the SQL level: the main transaction -is not marked for rollback only, but the inner emulated transaction is -rolled back to the savepoint. - Auto-commit mode ---------------- diff --git a/src/Connection.php b/src/Connection.php index f89eab993ea..827e5f3d745 100644 --- a/src/Connection.php +++ b/src/Connection.php @@ -22,7 +22,6 @@ use Doctrine\DBAL\Exception\DriverException; use Doctrine\DBAL\Exception\EmptyCriteriaNotAllowed; use Doctrine\DBAL\Exception\InvalidPlatformType; -use Doctrine\DBAL\Exception\MayNotAlterNestedTransactionWithSavepointsInTransaction; use Doctrine\DBAL\Exception\NoActiveTransaction; use Doctrine\DBAL\Exception\SavepointsNotSupported; use Doctrine\DBAL\Platforms\AbstractPlatform; @@ -97,11 +96,6 @@ class Connection implements ServerVersionProvider */ private ?int $transactionIsolationLevel = null; - /** - * If nested transactions should use savepoints. - */ - private bool $nestTransactionsWithSavepoints = false; - /** * The parameters used during creation of the Connection instance. * @@ -1045,39 +1039,37 @@ public function transactional(Closure $func): mixed /** * Sets if nested transactions should use savepoints. * + * @deprecated No replacement planned + * * @throws Exception */ public function setNestTransactionsWithSavepoints(bool $nestTransactionsWithSavepoints): void { - if (! $nestTransactionsWithSavepoints) { - Deprecation::trigger( - 'doctrine/dbal', - 'https://github.com/doctrine/dbal/pull/5383', - <<<'DEPRECATION' - Nesting transactions without enabling savepoints is deprecated. - Call %s::setNestTransactionsWithSavepoints(true) to enable savepoints. - DEPRECATION, - self::class - ); - } - - if ($this->transactionNestingLevel > 0) { - throw MayNotAlterNestedTransactionWithSavepointsInTransaction::new(); - } - - if (! $this->getDatabasePlatform()->supportsSavepoints()) { - throw SavepointsNotSupported::new(); - } - - $this->nestTransactionsWithSavepoints = $nestTransactionsWithSavepoints; + Deprecation::trigger( + 'doctrine/dbal', + 'https://github.com/doctrine/dbal/pull/5383', + '%s is deprecated and will be removed in 5.0', + __METHOD__ + ); } /** * Gets if nested transactions should use savepoints. + * + * @deprecated No replacement planned + * + * @return true */ public function getNestTransactionsWithSavepoints(): bool { - return $this->nestTransactionsWithSavepoints; + Deprecation::trigger( + 'doctrine/dbal', + 'https://github.com/doctrine/dbal/pull/5383', + '%s is deprecated and will be removed in 5.0', + __METHOD__ + ); + + return true; } /** @@ -1099,18 +1091,8 @@ public function beginTransaction(): void if ($this->transactionNestingLevel === 1) { $connection->beginTransaction(); - } elseif ($this->nestTransactionsWithSavepoints) { - $this->createSavepoint($this->_getNestedTransactionSavePointName()); } else { - Deprecation::trigger( - 'doctrine/dbal', - 'https://github.com/doctrine/dbal/pull/5383', - <<<'DEPRECATION' - Nesting transactions without enabling savepoints is deprecated. - Call %s::setNestTransactionsWithSavepoints(true) to enable savepoints. - DEPRECATION, - self::class - ); + $this->createSavepoint($this->_getNestedTransactionSavePointName()); } $this->getEventManager()->dispatchEvent(Events::onTransactionBegin, new TransactionBeginEventArgs($this)); @@ -1137,7 +1119,7 @@ public function commit(): void } catch (Driver\Exception $e) { throw $this->convertException($e); } - } elseif ($this->nestTransactionsWithSavepoints) { + } else { $this->releaseSavepoint($this->_getNestedTransactionSavePointName()); } @@ -1197,11 +1179,8 @@ public function rollBack(): void $this->beginTransaction(); } } - } elseif ($this->nestTransactionsWithSavepoints) { - $this->rollbackSavepoint($this->_getNestedTransactionSavePointName()); - --$this->transactionNestingLevel; } else { - $this->isRollbackOnly = true; + $this->rollbackSavepoint($this->_getNestedTransactionSavePointName()); --$this->transactionNestingLevel; } diff --git a/src/Exception/MayNotAlterNestedTransactionWithSavepointsInTransaction.php b/src/Exception/MayNotAlterNestedTransactionWithSavepointsInTransaction.php deleted file mode 100644 index 4519229bc81..00000000000 --- a/src/Exception/MayNotAlterNestedTransactionWithSavepointsInTransaction.php +++ /dev/null @@ -1,18 +0,0 @@ -commit(); } - public function testTransactionCommitEventNotCalledAfterRollBack(): void + public function testTransactionCommitEventCalledAfterRollBack(): void { $eventManager = new EventManager(); - $driverMock = $this->createMock(Driver::class); - $conn = new Connection([], $driverMock, new Configuration(), $eventManager); + $platform = $this->createStub(AbstractPlatform::class); + $platform + ->method('supportsSavepoints') + ->willReturn(true); + + $driverMock = $this->createMock(Driver::class); + $driverMock + ->method('getDatabasePlatform') + ->willReturn($platform); + + $conn = new Connection([], $driverMock, new Configuration(), $eventManager); $rollBackListenerMock = $this->createMock(TransactionRollBackDispatchEventListener::class); $rollBackListenerMock @@ -187,7 +196,7 @@ static function (TransactionRollBackEventArgs $eventArgs) use ($conn): bool { $eventManager->addEventListener([Events::onTransactionRollBack], $rollBackListenerMock); $commitListenerMock = $this->createMock(TransactionCommitDispatchEventListener::class); - $commitListenerMock->expects(self::never())->method('onTransactionCommit'); + $commitListenerMock->expects(self::exactly(1))->method('onTransactionCommit'); $eventManager->addEventListener([Events::onTransactionCommit], $commitListenerMock); $conn->beginTransaction(); @@ -345,7 +354,15 @@ public function testRollBackStartsTransactionInNoAutoCommitMode(): void public function testSwitchingAutoCommitModeCommitsAllCurrentTransactions(): void { + $platform = $this->createStub(AbstractPlatform::class); + $platform + ->method('supportsSavepoints') + ->willReturn(true); + $driverMock = $this->createMock(Driver::class); + $driverMock + ->method('getDatabasePlatform') + ->willReturn($platform); $conn = new Connection([], $driverMock); diff --git a/tests/Functional/ConnectionTest.php b/tests/Functional/ConnectionTest.php index 7c9d26b741a..ca528a67104 100644 --- a/tests/Functional/ConnectionTest.php +++ b/tests/Functional/ConnectionTest.php @@ -48,47 +48,27 @@ public function testCommitWithRollbackOnlyThrowsException(): void $this->connection->commit(); } - public function testNestingTransactionsWithoutSavepointsIsDeprecated(): void - { - if (! $this->connection->getDatabasePlatform()->supportsSavepoints()) { - self::markTestSkipped('This test is only supported on platforms that support savepoints.'); - } - - $this->expectDeprecationWithIdentifier('https://github.com/doctrine/dbal/pull/5383'); - $this->connection->setNestTransactionsWithSavepoints(false); - } - public function testTransactionNestingBehavior(): void { $this->createTestTable(); + $this->connection->beginTransaction(); + self::assertSame(1, $this->connection->getTransactionNestingLevel()); + try { $this->connection->beginTransaction(); - self::assertSame(1, $this->connection->getTransactionNestingLevel()); - - try { - $this->expectDeprecationWithIdentifier('https://github.com/doctrine/dbal/pull/5383'); - $this->connection->beginTransaction(); - self::assertSame(2, $this->connection->getTransactionNestingLevel()); + self::assertSame(2, $this->connection->getTransactionNestingLevel()); - $this->connection->insert(self::TABLE, ['id' => 1]); - self::fail('Expected exception to be thrown because of the unique constraint.'); - } catch (Throwable $e) { - self::assertInstanceOf(UniqueConstraintViolationException::class, $e); - $this->connection->rollBack(); - self::assertSame(1, $this->connection->getTransactionNestingLevel()); - } - - self::assertTrue($this->connection->isRollbackOnly()); - - $this->connection->commit(); // should throw exception - self::fail('Transaction commit after failed nested transaction should fail.'); - } catch (ConnectionException $e) { - self::assertSame(1, $this->connection->getTransactionNestingLevel()); + $this->connection->insert(self::TABLE, ['id' => 1]); + self::fail('Expected exception to be thrown because of the unique constraint.'); + } catch (Throwable $e) { + self::assertInstanceOf(UniqueConstraintViolationException::class, $e); $this->connection->rollBack(); - self::assertSame(0, $this->connection->getTransactionNestingLevel()); + self::assertSame(1, $this->connection->getTransactionNestingLevel()); } + $this->connection->commit(); + $this->connection->beginTransaction(); $this->connection->close(); $this->connection->beginTransaction(); @@ -133,7 +113,6 @@ public function testTransactionNestingBehaviorWithSavepoints(): void $this->createTestTable(); - $this->connection->setNestTransactionsWithSavepoints(true); try { $this->connection->beginTransaction(); self::assertSame(1, $this->connection->getTransactionNestingLevel()); @@ -155,12 +134,6 @@ public function testTransactionNestingBehaviorWithSavepoints(): void } self::assertFalse($this->connection->isRollbackOnly()); - try { - $this->connection->setNestTransactionsWithSavepoints(false); - self::fail('Should not be able to disable savepoints in usage inside a nested open transaction.'); - } catch (ConnectionException $e) { - self::assertTrue($this->connection->getNestTransactionsWithSavepoints()); - } $this->connection->commit(); // should not throw exception } catch (ConnectionException $e) { @@ -168,17 +141,6 @@ public function testTransactionNestingBehaviorWithSavepoints(): void } } - public function testTransactionNestingBehaviorCantBeChangedInActiveTransaction(): void - { - if (! $this->connection->getDatabasePlatform()->supportsSavepoints()) { - self::markTestSkipped('This test requires the platform to support savepoints.'); - } - - $this->connection->beginTransaction(); - $this->expectException(ConnectionException::class); - $this->connection->setNestTransactionsWithSavepoints(true); - } - public function testTransactionIsInactiveAfterConnectionClose(): void { $this->connection->beginTransaction(); @@ -187,18 +149,6 @@ public function testTransactionIsInactiveAfterConnectionClose(): void self::assertFalse($this->connection->isTransactionActive()); } - public function testSetNestedTransactionsThroughSavepointsNotSupportedThrowsException(): void - { - if ($this->connection->getDatabasePlatform()->supportsSavepoints()) { - self::markTestSkipped('This test requires the platform not to support savepoints.'); - } - - $this->expectException(ConnectionException::class); - $this->expectExceptionMessage('Savepoints are not supported by this driver.'); - - $this->connection->setNestTransactionsWithSavepoints(true); - } - public function testCreateSavepointsNotSupportedThrowsException(): void { if ($this->connection->getDatabasePlatform()->supportsSavepoints()) {