diff --git a/src/Infrastructure/Database/Database.php b/src/Infrastructure/Database/Database.php index cb350e776..0dfa82bac 100644 --- a/src/Infrastructure/Database/Database.php +++ b/src/Infrastructure/Database/Database.php @@ -79,6 +79,11 @@ final class Database implements DatabaseInterface { private ?int $lastId = null; + /** + * How many nested scopes have asked for a transaction. Only the outermost may commit. + */ + private int $transactionDepth = 0; + /** * DB constructor. * @@ -388,21 +393,41 @@ public function beginTransaction(): bool { $conn = $this->dbStorageHandler->getConnection(); - if (!$conn->inTransaction()) { - $result = $conn->beginTransaction(); + // The counter is this class's own bookkeeping; `inTransaction()` is the truth. Both are + // consulted, so a transaction opened outside these methods is still joined rather than + // begun again — PDO throws on that. + if ($this->transactionDepth > 0 || $conn->inTransaction()) { + // Joining the transaction already running, not starting one. Counted so that the + // matching `endTransaction()` knows it is not the one that may commit. + // + // It used to return `true` here with nothing recorded, and `endTransaction()` committed + // whenever a transaction was active — so the *inner* scope ended the *outer* one. + // Measured against the server: the inner commit leaves `inTransaction()` false, the + // outer rollback then fails with "There is no active transaction", and both rows + // survive. Nesting is the normal case, not an edge: `Import::doImport()` wraps the + // whole import and calls `Account::create()` per row, which opens its own; and + // `Account::update()` calls `AccountItems::updateItems()`, which opens up to five in + // sequence. + $this->transactionDepth++; + + logger('beginTransaction: joining the transaction already open'); + + return true; + } - $this->eventDispatcher->notify(new Event( - 'database.transaction.begin', - $this, - EventMessage::build()->addExtra('result', $result) - )); + $result = $conn->beginTransaction(); - return $result; + if ($result) { + $this->transactionDepth = 1; } - logger('beginTransaction: already in transaction'); + $this->eventDispatcher->notify(new Event( + 'database.transaction.begin', + $this, + EventMessage::build()->addExtra('result', $result) + )); - return true; + return $result; } /** @@ -414,8 +439,18 @@ public function endTransaction(): bool { $conn = $this->dbStorageHandler->getConnection(); + if ($this->transactionDepth > 1) { + // An inner scope finishing. Its work stays in the open transaction, to be committed or + // rolled back with everything else by whoever opened it. + $this->transactionDepth--; + + return true; + } + $result = $conn->inTransaction() && $conn->commit(); + $this->transactionDepth = 0; + $this->eventDispatcher->notify(new Event( 'database.transaction.end', $this, @@ -436,6 +471,11 @@ public function rollbackTransaction(): bool $result = $conn->inTransaction() && $conn->rollBack(); + // All of it, from whichever depth: a rollback is not something an inner scope can do on its + // own half of the work. Zeroing the depth also makes the outer scope's own rollback — which + // runs when the exception reaches it — a no-op rather than an error. + $this->transactionDepth = 0; + $this->eventDispatcher->notify(new Event( 'database.transaction.rollback', $this, diff --git a/tests/Unit/Infrastructure/Database/DatabaseTest.php b/tests/Unit/Infrastructure/Database/DatabaseTest.php index a66406890..331f2e98a 100644 --- a/tests/Unit/Infrastructure/Database/DatabaseTest.php +++ b/tests/Unit/Infrastructure/Database/DatabaseTest.php @@ -65,6 +65,95 @@ public static function bufferedDataProvider(): array ]; } + /** + * A nested scope's `endTransaction()` does not commit the transaction it joined. + * + * `beginTransaction()` answered `true` whether it started a transaction or joined one, and + * `endTransaction()` committed whenever one was active — so the *inner* scope ended the *outer* + * one. Measured against the server before the fix: the inner commit leaves `inTransaction()` + * false, the outer rollback then fails with "There is no active transaction", and every row + * written before the inner commit survives. + * + * Nesting is the ordinary case rather than an edge. `Import::doImport()` wraps the whole import + * in one transaction and calls `Account::create()` per row, which opens its own; and + * `Account::update()` calls `AccountItems::updateItems()`, which opens up to five in sequence. + * So an import or a bulk edit that failed part-way kept everything already processed. + */ + public function testANestedTransactionDoesNotCommitTheOuterOne() + { + $pdo = $this->createMock(PDO::class); + + $this->dbStorageHandler->method('getConnection')->willReturn($pdo); + + // The connection's own state, so the second `beginTransaction()` is a genuine nested call + // rather than one the stub has told there is nothing open. By reference: an arrow function + // would capture it by value and answer `false` forever. + $open = false; + $pdo->method('inTransaction')->willReturnCallback(function () use (&$open): bool { + return $open; + }); + + // One real begin, and one real commit — not two of either. + $pdo->expects($this->once()) + ->method('beginTransaction') + ->willReturnCallback(function () use (&$open): bool { + $open = true; + + return true; + }); + + $pdo->expects($this->once()) + ->method('commit') + ->willReturnCallback(function () use (&$open): bool { + $open = false; + + return true; + }); + + self::assertTrue($this->database->beginTransaction(), 'the outer scope starts one'); + self::assertTrue($this->database->beginTransaction(), 'the inner scope joins it'); + + self::assertTrue($this->database->endTransaction(), 'the inner scope finishing commits nothing'); + self::assertTrue($this->database->endTransaction(), 'the outer scope is the one that commits'); + } + + /** + * And a rollback from any depth undoes all of it, leaving nothing for the outer scope to roll + * back a second time. + * + * `transactionAware()` rolls back in its own `catch`, so when an inner failure propagates the + * outer scope calls `rollbackTransaction()` again — that must be a no-op rather than an error. + */ + public function testARollbackFromANestedScopeUndoesAllOfIt() + { + $pdo = $this->createMock(PDO::class); + + $this->dbStorageHandler->method('getConnection')->willReturn($pdo); + + // True while the transaction is open, false once it has been rolled back. + $open = true; + // By reference, not `fn()` — an arrow function captures by value when it is created, so a + // `fn(): bool => $open` here would answer `true` forever and rollBack() would run twice. + $pdo->method('inTransaction')->willReturnCallback(function () use (&$open): bool { + return $open; + }); + $pdo->method('beginTransaction')->willReturn(true); + + $pdo->expects($this->once()) + ->method('rollBack') + ->willReturnCallback(static function () use (&$open): bool { + $open = false; + + return true; + }); + + $this->database->beginTransaction(); + $this->database->beginTransaction(); + + self::assertTrue($this->database->rollbackTransaction(), 'the inner failure rolls all of it back'); + self::assertFalse($this->database->rollbackTransaction(), 'and the outer scope finds nothing left'); + } + /** * @throws Exception */