From b48e285377d23438b0bd857869c2b62fa6496008 Mon Sep 17 00:00:00 2001 From: Martok Date: Thu, 14 Dec 2023 23:36:56 +0100 Subject: [PATCH 1/3] Fix Adapter::transaction rollback on any exception --- src/Adapter/AbstractAdapter.php | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/Adapter/AbstractAdapter.php b/src/Adapter/AbstractAdapter.php index 70eda6f..233c8c0 100644 --- a/src/Adapter/AbstractAdapter.php +++ b/src/Adapter/AbstractAdapter.php @@ -185,11 +185,8 @@ public function transaction(mixed $callable, mixed $params = null): void $callable->call(); $this->commit(); } catch (\Exception $e) { - if ($this->transactionDepth == 0) { - $this->rollback(); - } else { - throw $e; - } + $this->rollback(); + throw $e; } } From c6c6ce793c41251a57499047895c4668fbfda98c Mon Sep 17 00:00:00 2001 From: Martok Date: Fri, 15 Dec 2023 01:30:15 +0100 Subject: [PATCH 2/3] Move handling of transaction nesting to TransactionManager, used by all Adapters (issue #10) --- src/Adapter/AbstractAdapter.php | 26 +++-- src/Adapter/Mysql.php | 73 ++++++------ src/Adapter/Pdo.php | 29 +++-- src/Adapter/Pgsql.php | 23 ++-- src/Adapter/Sqlite.php | 27 ++--- src/Adapter/Sqlsrv.php | 27 ++--- src/Adapter/TransactionManager.php | 177 +++++++++++++++++++++++++++++ 7 files changed, 277 insertions(+), 105 deletions(-) create mode 100644 src/Adapter/TransactionManager.php diff --git a/src/Adapter/AbstractAdapter.php b/src/Adapter/AbstractAdapter.php index 233c8c0..a5342ee 100644 --- a/src/Adapter/AbstractAdapter.php +++ b/src/Adapter/AbstractAdapter.php @@ -72,16 +72,10 @@ abstract class AbstractAdapter implements AdapterInterface protected ?Profiler\Profiler $profiler = null; /** - * Is open transaction flag - * @var bool + * Transaction manager + * @var TransactionManager|null */ - protected bool $isTransaction = false; - - /** - * Transaction depth - * @var int - */ - protected int $transactionDepth = 0; + protected ?TransactionManager $transactionManager = null; /** * Constructor @@ -146,6 +140,16 @@ abstract public function commit(): AbstractAdapter; */ abstract public function rollback(): AbstractAdapter; + /** + * Return the transaction manager object, initialize on first use + * + * @return TransactionManager + */ + protected function getTransactionManager(): TransactionManager + { + return ($this->transactionManager ??= new TransactionManager()); + } + /** * Check if adapter is in the middle of an open transaction * @@ -153,7 +157,7 @@ abstract public function rollback(): AbstractAdapter; */ public function isTransaction(): bool { - return $this->isTransaction; + return !is_null($this->transactionManager) && $this->transactionManager->isTransaction(); } /** @@ -163,7 +167,7 @@ public function isTransaction(): bool */ public function getTransactionDepth(): int { - return $this->transactionDepth; + return is_null($this->transactionManager) ? 0 : $this->transactionManager->getTransactionDepth(); } /** diff --git a/src/Adapter/Mysql.php b/src/Adapter/Mysql.php index ca30f60..645a445 100644 --- a/src/Adapter/Mysql.php +++ b/src/Adapter/Mysql.php @@ -121,19 +121,18 @@ public function hasOptions(): bool */ public function beginTransaction(?int $flags = null, ?string $name = null): Mysql { - if ($this->transactionDepth == 0) { - if (($flags !== null) && ($name !== null)) { - $this->connection->begin_transaction($flags, $name); - } else if ($flags !== null) { - $this->connection->begin_transaction($flags); - } else { - $this->connection->begin_transaction(); - } - - $this->isTransaction = true; - } - - $this->transactionDepth++; + $this->getTransactionManager()->enter( + beginFunc: function () use ($flags, $name) { + if (($flags !== null) && ($name !== null)) { + $this->connection->begin_transaction($flags, $name); + } else if ($flags !== null) { + $this->connection->begin_transaction($flags); + } else { + $this->connection->begin_transaction(); + } + }, + savepointFunc: function (string $sp) { $this->connection->savepoint($sp); }, + ); return $this; } @@ -147,19 +146,18 @@ public function beginTransaction(?int $flags = null, ?string $name = null): Mysq */ public function commit(?int $flags = null, ?string $name = null): Mysql { - $this->transactionDepth--; - - if ($this->transactionDepth == 0) { - if (($flags !== null) && ($name !== null)) { - $this->connection->commit($flags, $name); - } else if ($flags !== null) { - $this->connection->commit($flags); - } else { - $this->connection->commit(); - } - - $this->isTransaction = false; - } + $this->getTransactionManager()->leave(true, + commitFunc: function () use ($flags, $name) { + if (($flags !== null) && ($name !== null)) { + $this->connection->commit($flags, $name); + } else if ($flags !== null) { + $this->connection->commit($flags); + } else { + $this->connection->commit(); + } + }, + savepointReleaseFunc: function (string $sp) { $this->connection->release_savepoint($sp); }, + ); return $this; } @@ -173,17 +171,18 @@ public function commit(?int $flags = null, ?string $name = null): Mysql */ public function rollback(?int $flags = null, ?string $name = null): Mysql { - $this->transactionDepth = 0; - - if (($flags !== null) && ($name !== null)) { - $this->connection->rollback($flags, $name); - } else if ($flags !== null) { - $this->connection->rollback($flags); - } else { - $this->connection->rollback(); - } - - $this->isTransaction = false; + $this->getTransactionManager()->leave(false, + rollbackFunc: function () use ($flags, $name) { + if (($flags !== null) && ($name !== null)) { + $this->connection->rollback($flags, $name); + } else if ($flags !== null) { + $this->connection->rollback($flags); + } else { + $this->connection->rollback(); + } + }, + savepointRollbackFunc: function (string $sp) { $this->query('ROLLBACK TO SAVEPOINT ' . $sp); }, + ); return $this; } diff --git a/src/Adapter/Pdo.php b/src/Adapter/Pdo.php index 2f8f8cd..9cf79ac 100644 --- a/src/Adapter/Pdo.php +++ b/src/Adapter/Pdo.php @@ -182,12 +182,10 @@ public function getType(): ?string */ public function beginTransaction(): Pdo { - if ($this->transactionDepth == 0) { - $this->connection->beginTransaction(); - $this->isTransaction = true; - } - - $this->transactionDepth++; + $this->getTransactionManager()->enter( + beginFunc: function () { $this->connection->beginTransaction(); }, + savepointFunc: function (string $sp) { $this->query('SAVEPOINT ' . $sp); }, + ); return $this; } @@ -199,13 +197,11 @@ public function beginTransaction(): Pdo */ public function commit(): Pdo { - $this->transactionDepth--; - - if ($this->transactionDepth == 0) { - $this->connection->commit(); - $this->isTransaction = false; - } - + $this->getTransactionManager()->leave(true, + commitFunc: function () { $this->connection->commit(); }, + rollbackFunc: function () { $this->connection->rollBack(); }, + savepointReleaseFunc: function (string $sp) { $this->query('RELEASE SAVEPOINT ' . $sp); }, + ); return $this; } @@ -216,9 +212,10 @@ public function commit(): Pdo */ public function rollback(): Pdo { - $this->transactionDepth = 0; - $this->connection->rollBack(); - $this->isTransaction = false; + $this->getTransactionManager()->leave(false, + rollbackFunc: function () { $this->connection->rollBack(); }, + savepointRollbackFunc: function (string $sp) { $this->query('ROLLBACK TO SAVEPOINT ' . $sp); }, + ); return $this; } diff --git a/src/Adapter/Pgsql.php b/src/Adapter/Pgsql.php index 8507773..bed1a29 100644 --- a/src/Adapter/Pgsql.php +++ b/src/Adapter/Pgsql.php @@ -153,11 +153,10 @@ public function hasOptions(): bool */ public function beginTransaction(): Pgsql { - if ($this->transactionDepth == 0) { - $this->query('BEGIN TRANSACTION'); - $this->isTransaction = true; - } - $this->transactionDepth++; + $this->getTransactionManager()->enter( + beginFunc: function () { $this->query('BEGIN TRANSACTION'); }, + savepointFunc: function (string $sp) { $this->query('SAVEPOINT ' . $sp); }, + ); return $this; } @@ -169,9 +168,10 @@ public function beginTransaction(): Pgsql */ public function commit(): Pgsql { - $this->transactionDepth--; - $this->query('COMMIT'); - $this->isTransaction = false; + $this->getTransactionManager()->leave(true, + commitFunc: function () { $this->query('COMMIT'); }, + savepointReleaseFunc: function (string $sp) { $this->query('RELEASE SAVEPOINT ' . $sp); }, + ); return $this; } @@ -183,9 +183,10 @@ public function commit(): Pgsql */ public function rollback(): Pgsql { - $this->transactionDepth = 0; - $this->query('ROLLBACK'); - $this->isTransaction = false; + $this->getTransactionManager()->leave(false, + rollbackFunc: function () { $this->query('ROLLBACK'); }, + savepointRollbackFunc: function (string $sp) { $this->query('ROLLBACK TO SAVEPOINT ' . $sp); }, + ); return $this; } diff --git a/src/Adapter/Sqlite.php b/src/Adapter/Sqlite.php index 3c9e3c2..034041d 100644 --- a/src/Adapter/Sqlite.php +++ b/src/Adapter/Sqlite.php @@ -134,12 +134,10 @@ public function dbFileExists(): bool */ public function beginTransaction(): Sqlite { - if ($this->transactionDepth == 0) { - $this->query('BEGIN TRANSACTION'); - $this->isTransaction = true; - } - - $this->transactionDepth++; + $this->getTransactionManager()->enter( + beginFunc: function () { $this->query('BEGIN TRANSACTION'); }, + savepointFunc: function (string $sp) { $this->query('SAVEPOINT ' . $sp); }, + ); return $this; } @@ -151,12 +149,10 @@ public function beginTransaction(): Sqlite */ public function commit(): Sqlite { - $this->transactionDepth--; - - if ($this->transactionDepth == 0) { - $this->query('COMMIT'); - $this->isTransaction = false; - } + $this->getTransactionManager()->leave(true, + commitFunc: function () { $this->query('COMMIT'); }, + savepointReleaseFunc: function (string $sp) { $this->query('RELEASE SAVEPOINT ' . $sp); }, + ); return $this; } @@ -168,9 +164,10 @@ public function commit(): Sqlite */ public function rollback(): Sqlite { - $this->transactionDepth = 0; - $this->query('ROLLBACK'); - $this->isTransaction = false; + $this->getTransactionManager()->leave(false, + rollbackFunc: function () { $this->query('ROLLBACK'); }, + savepointRollbackFunc: function (string $sp) { $this->query('ROLLBACK TO SAVEPOINT ' . $sp); }, + ); return $this; } diff --git a/src/Adapter/Sqlsrv.php b/src/Adapter/Sqlsrv.php index 711a941..2861d67 100644 --- a/src/Adapter/Sqlsrv.php +++ b/src/Adapter/Sqlsrv.php @@ -137,12 +137,10 @@ public function hasOptions(): bool */ public function beginTransaction(): Sqlsrv { - if ($this->transactionDepth == 0) { - sqlsrv_begin_transaction($this->connection); - $this->isTransaction = true; - } - - $this->transactionDepth++; + $this->getTransactionManager()->enter( + beginFunc: function () { sqlsrv_begin_transaction($this->connection); }, + savepointFunc: function (string $sp) { $this->query('SAVE TRANSACTION ' . $sp); }, + ); return $this; } @@ -154,12 +152,10 @@ public function beginTransaction(): Sqlsrv */ public function commit(): Sqlsrv { - $this->transactionDepth--; - - if ($this->transactionDepth == 0) { - sqlsrv_commit($this->connection); - $this->isTransaction = false; - } + $this->getTransactionManager()->leave(true, + commitFunc: function () { sqlsrv_commit($this->connection); }, + savepointReleaseFunc: function (string $sp) { /* Sqlsrv does not manage successful savepoints explicitly */ }, + ); return $this; } @@ -171,9 +167,10 @@ public function commit(): Sqlsrv */ public function rollback(): Sqlsrv { - $this->transactionDepth = 0; - sqlsrv_rollback($this->connection); - $this->isTransaction = false; + $this->getTransactionManager()->leave(false, + rollbackFunc: function () { sqlsrv_rollback($this->connection); }, + savepointRollbackFunc: function (string $sp) { $this->query('ROLLBACK TRANSACTION ' . $sp); }, + ); return $this; } diff --git a/src/Adapter/TransactionManager.php b/src/Adapter/TransactionManager.php new file mode 100644 index 0000000..fb71046 --- /dev/null +++ b/src/Adapter/TransactionManager.php @@ -0,0 +1,177 @@ + + * @copyright Copyright (c) 2009-2024 NOLA Interactive, LLC. (http://www.nolainteractive.com) + * @license http://www.popphp.org/license New BSD License + */ + +/** + * @namespace + */ + +namespace Pop\Db\Adapter; + +/** + * Nested transaction manager for use in AdapaterInterface implementations + * + * @category Pop + * @package Pop\Db + * @author Nick Sagona, III + * @author Martok + * @copyright Copyright (c) 2009-2024 NOLA Interactive, LLC. (http://www.nolainteractive.com) + * @license http://www.popphp.org/license New BSD License + * @version 6.0.0 + */ +class TransactionManager +{ + + /** + * Transaction state flag + * @var int + */ + private int $transactionState = 0; + private const TS_NONE = 0; + private const TS_OPEN = 1; + private const TS_ROLLED_BACK = -1; + + /** + * Transaction depth + * @var int + */ + private int $transactionDepth = 0; + + /** + * Use savepoints or simulated nested transactions (SNTs) + * @var bool + */ + private bool $useSavepoints; + + /** + * Names of active savepoints. Count is always one less than $transactionDepth. + * @var string[] + */ + private array $savepoints = []; + private int $savepointName = 0; + + /** + * Constructor + * + * Instantiate the transaction manager object + * + * @param bool $useSavepoints Enable the use of savepoints by default + */ + public function __construct(bool $useSavepoints = true) + { + $this->useSavepoints = $useSavepoints; + } + + /** + * Check if adapter is in the middle of an open transaction + * + * @return bool + */ + public function isTransaction(): bool + { + return $this->transactionState !== self::TS_NONE; + } + + /** + * Get transaction depth + * + * @return int + */ + public function getTransactionDepth(): int + { + return $this->transactionDepth; + } + + /** + * Enter a new transaction or increase nesting level + * + * @param ?callable $beginFunc Called when a new top-level transaction must be started + * @param ?callable $savepointFunc Called when a named savepoint is created + * @return bool + */ + public function enter(?callable $beginFunc = null, ?callable $savepointFunc = null): bool + { + $this->transactionDepth++; + + // an already rolled back SNT can never turn back into a normal one + if ($this->transactionState == self::TS_ROLLED_BACK) + return false; + + if ($this->transactionDepth == 1) { + // BEGIN a new transaction + if (is_callable($beginFunc)) { + $beginFunc(); + } + $this->transactionState = self::TS_OPEN; + } else { + // increase nesting level + if ($this->useSavepoints && is_callable($savepointFunc)) { + try { + $sp = 'PopDbTxn_' . $this->savepointName++; + $savepointFunc($sp); + $this->savepoints[] = $sp; + } catch (\Exception $e) { + // if this failed, assume this Adapter doesn't actually support savepoints + $this->useSavepoints = false; + } + } + } + return true; + } + + /** + * Leave a transaction or reduce nesting level + * + * @param bool $doCommit If true, perform a commit. Rollback otherwise. + * @param ?callable $commitFunc Called when a top-level commit must be performed + * @param ?callable $rollbackFunc Called when a top-level rollback must be performed + * @param ?callable $savepointReleaseFunc Called when a savepoint is released (like commit) + * @param ?callable $savepointRollbackFunc Called when the transaction is rolled back to a savepoint + * @return bool + */ + public function leave(bool $doCommit, + ?callable $commitFunc = null, ?callable $rollbackFunc = null, + ?callable $savepointReleaseFunc = null, ?callable $savepointRollbackFunc = null): bool + { + if ($this->transactionDepth <= 0 || $this->transactionState == self::TS_NONE) + return false; + + $this->transactionDepth--; + + // Leaving the outermost transaction always commits/rolls back the transaction. + // If savepoints are enabled, leaving a nested transaction requires the rollback/release of the savepoint. + // Without savepoints, only the outermost transaction is real, becoming an automatic rollback if + // any nested transaction was a rollback. + if ($this->useSavepoints && $this->transactionDepth > 0) { + $sp = array_pop($this->savepoints); + if ($doCommit) { + if (is_callable($savepointReleaseFunc)) { + $savepointReleaseFunc($sp); + } + } else { + if (is_callable($savepointRollbackFunc)) { + $savepointRollbackFunc($sp); + } + } + } else { + if (!$doCommit) + $this->transactionState = self::TS_ROLLED_BACK; + } + + if ($this->transactionDepth == 0) { + if ($this->transactionState == self::TS_OPEN && is_callable($commitFunc)) + $commitFunc(); + elseif ($this->transactionState == self::TS_ROLLED_BACK && is_callable($rollbackFunc)) + $rollbackFunc(); + $this->transactionState = self::TS_NONE; + } + + return true; + } +} From 99501927fde45f95ba3caf3334349d67818eec91 Mon Sep 17 00:00:00 2001 From: Martok Date: Sat, 16 Dec 2023 22:12:24 +0100 Subject: [PATCH 3/3] Add test for transaction nesting (issue #10) --- tests/RecordTest.php | 52 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/RecordTest.php b/tests/RecordTest.php index 2b1e046..0de1b03 100644 --- a/tests/RecordTest.php +++ b/tests/RecordTest.php @@ -908,6 +908,58 @@ public function testGlobalTransactionRollback() }); } + public function testTransactionNesting() + { + $this->assertFalse($this->db->isTransaction()); + $this->assertEquals(0, $this->db->getTransactionDepth()); + $user = Users::start([ + 'username' => 'testuser264', + 'password' => 'password27', + 'email' => 'testuser26@test.com', + 'logins' => 1 + ]); + $this->assertEquals(1, $this->db->getTransactionDepth()); + + // Adapter transaction management + $this->db->beginTransaction(); + $this->assertEquals(2, $this->db->getTransactionDepth()); + $this->db->query("INSERT INTO users (username, password, email) values ('testuser266', 'password27', 'testuser26@test.com')"); + $this->db->commit(); + $this->assertTrue($this->db->isTransaction()); + $this->assertEquals(1, $this->db->getTransactionDepth()); + + // Record transaction management + $admin = new Users([ + 'username' => 'testuser265', + 'password' => 'password27', + 'email' => 'testuser26@test.com', + 'logins' => 1 + ]); + $admin->startTransaction(); + $this->assertEquals(2, $this->db->getTransactionDepth()); + $admin->save(); + $this->assertEquals(1, $this->db->getTransactionDepth()); + + // Adapter transaction rollback, visibility + $this->db->beginTransaction(); + $this->assertEquals(2, $this->db->getTransactionDepth()); + $this->db->query("INSERT INTO users (username, password, email) values ('testuser267', 'password27', 'testuser26@test.com')"); + $test = Users::findOne(['username' => 'testuser267']); + $this->assertNotNull($test->id); + $this->db->rollback(); + $this->assertTrue($this->db->isTransaction()); + $this->assertEquals(1, $this->db->getTransactionDepth()); + $test = Users::findOne(['username' => 'testuser267']); + $this->assertNull($test->id); + + // Commits outer transaction + $user->save(); + $this->assertFalse($this->db->isTransaction()); + $this->assertEquals(0, $this->db->getTransactionDepth()); + + $this->db->disconnect(); + } + public function testFindWhere() { $user = new Users([