From 25dbc8a9f69ea33a489d94bb69705297664550c2 Mon Sep 17 00:00:00 2001
From: Harald Eilertsen <haraldei@anduin.net>
Date: Mon, 29 Jan 2024 14:30:52 +0100
Subject: include/dba: Make Dba driver transaction aware.

This patch introduced database transaction support to the Dba driver via
the DbaTransaction class.

The goal of this is to allow the driver control over the creation and
finalization of database transactions.

Until now code that has needed transaction support has done so directly
by issuing "BEGIN", "ROLLBACK" and "COMMIT" commands to the underlying
database directly.

This has several disadvantages:

  - We do have no control or knowledge of whether any transactions being
	active.

  - Since transactions can not be nested, we run the risk of unrelated
	code trying to create a transaction when one is already active.

  - Code using transactions are not testable, as the test runner wraps
	all tests within a transaction to begin with.

This patch should eliminate all these problems.

A transaction is started by instantiating the DbaTransaction class:

    $my_transaction = new \DbaTransaction();

The transaction will automatically be _rolled back_ if it has not been
committed before the instance is destroyed. (When the variable holding
it goes out of scope, i.e when the containing function returns.)

A transaction is committed like this:

    $my_transaction->commit();

This will immediately commit the changes in the transaction, and the
transaction will be marked as committed, so it will not be attempted to
be rolled back on destruction.

I have chosen to "ignore" the problem of nested transactions by having
the DbaTransaction class _not_ initiate a new transaction if one is
already active. This also makes the rollback and commit actions of the
DbaTransaction class into no-ops.

An alternative would be to simulate nested transactions by using save
points if a transaction is already active. However, I'm unsure about
wether there's any safe way to avoid all potential pitfalls when doing
that.

In any case, nested transactions should preferably be avoided, and
afaict we don't rely on that in any of the existing code. The reason we
need to support it in some way is that it's needed for testing where the
code under test is creating a transaction on it's own. (Since each test
is run within a db transaction to begin with.)

Also, I have taken the liberty to assume a PDO based db driver for this
stuff. I don't think that's going to be a problem, as that's the only
thing supported by the rest of the code in any case.
---
 include/dba/dba_transaction.php             |  64 +++++++++
 tests/fakes/fake_dba.php                    |  18 +++
 tests/unit/UnitTestCase.php                 |  64 ++++-----
 tests/unit/includes/dba/TransactionTest.php | 207 ++++++++++++++++++++++++++++
 4 files changed, 321 insertions(+), 32 deletions(-)
 create mode 100644 include/dba/dba_transaction.php
 create mode 100644 tests/fakes/fake_dba.php
 create mode 100644 tests/unit/includes/dba/TransactionTest.php

diff --git a/include/dba/dba_transaction.php b/include/dba/dba_transaction.php
new file mode 100644
index 000000000..02e9945ca
--- /dev/null
+++ b/include/dba/dba_transaction.php
@@ -0,0 +1,64 @@
+<?php
+/**
+ * Class to represent a database transaction.
+ *
+ * A database transaction is initiated upon construction of an object of this
+ * class. The transaction will be automatically rolled back upon destruction
+ * unless it has been explicitly committed by calling the `commit` method.
+ *
+ * Wrapping multiple database operation within a transaction ensures that all
+ * (or none) of the operations are successfully completed at the same time.
+ *
+ * If a transaction is already active when constructing an object of this
+ * class, it will _not_ try to initiate a transaction, but constructs an object
+ * that will in practice be a stub. This prevents that "nested" transactions
+ * will cause problems with the existing active transaction.
+ *
+ * It also means that any rollbacks or commits perfomed on the "nested"
+ * transaction will be ignored, and postponed to the outer transaction is
+ * committed or rolled back.
+ *
+ * Also note that any modification to the database schema will implicitly
+ * commit active transactions in most cases, so be careful about relying on
+ * transactions in those cases.
+ *
+ * @Note This class assumes the actual underlying database driver is PDO.
+ */
+class DbaTransaction {
+	private bool $committed = false;
+	private bool $active = false;
+
+	/**
+	 * Creates a database transaction object.
+	 *
+	 * If a transaction is already active for this db connection,
+	 * no transaction is initiated, and the constructed object will
+	 * not perform any commit or rollback actions.
+	 */
+	public function __construct(private dba_driver $dba) {
+		if (! $this->dba->db->inTransaction()) {
+			$this->active = $this->dba->db->beginTransaction();
+		}
+	}
+
+	/**
+	 * Roll back the transaction if it is active and not already committed.
+	 */
+	public function __destruct() {
+		if ($this->active && ! $this->committed) {
+			$this->dba->db->rollBack();
+		}
+	}
+
+	/**
+	 * Commit the transaction if active.
+	 *
+	 * This will also mark the transaction as committed, preventing it from
+	 * being attempted rolled back on destruction.
+	 */
+	public function commit(): void {
+		if ($this->active && ! $this->committed) {
+			$this->committed = $this->dba->db->commit();
+		}
+	}
+}
diff --git a/tests/fakes/fake_dba.php b/tests/fakes/fake_dba.php
new file mode 100644
index 000000000..2289f5c80
--- /dev/null
+++ b/tests/fakes/fake_dba.php
@@ -0,0 +1,18 @@
+<?php
+namespace Zotlabs\Tests\Fakes;
+
+require_once 'include/dba/dba_pdo.php';
+
+/**
+ * Fake dba_driver implementation.
+ *
+ * This is a subclass of the dba_pdo class, that essentially lets us inject a
+ * stub for the PDO class that is the actual database driver.
+ */
+class FakeDba extends \dba_pdo {
+	public function __construct($stub) {
+		$this->db = $stub;
+		$this->connected = true;
+	}
+}
+
diff --git a/tests/unit/UnitTestCase.php b/tests/unit/UnitTestCase.php
index 0bf7b547a..18467d91e 100644
--- a/tests/unit/UnitTestCase.php
+++ b/tests/unit/UnitTestCase.php
@@ -23,6 +23,7 @@
 namespace Zotlabs\Tests\Unit;
 
 use PHPUnit\Framework\TestCase;
+use PHPUnit\Framework\TestResult;
 
 /*
  * Make sure global constants and the global App object is available to the
@@ -41,10 +42,39 @@ require_once 'include/dba/dba_driver.php' ;
  * @author Klaus Weidenbach
  */
 class UnitTestCase extends TestCase {
-	private bool $in_transaction = false;
 	protected array $fixtures = array();
 
-	public static function setUpBeforeClass() : void {
+	/**
+	 * Override the PHPUnit\Framework\TestCase::run method, so we can
+	 * wrap it in a database transaction.
+	 *
+	 * @SuppressWarnings(PHPMD.UnusedLocalVariable)
+	 */
+	public function run(TestResult $result = null): TestResult {
+		// $myclass = get_class($this);
+		// logger("[*] Running test: {$myclass}::{$this->getName(true)}", LOGGER_DEBUG);
+
+		if (! \DBA::$dba) {
+			//logger('[*] Connecting to test db...');
+			$this->connect_to_test_db();
+		}
+
+		// The $transactuion variable is needed to hold the transaction until the
+		// function returns.
+		$transaction = new \DbaTransaction(\DBA::$dba);
+
+		$this->loadFixtures();
+
+		// Make sure app config is reset and loaded from fixtures
+		\App::$config = array();
+		\Zotlabs\Lib\Config::Load('system');
+
+		$result = parent::run($result);
+
+		return $result;
+	}
+
+	protected function connect_to_test_db() : void {
 		if ( !\DBA::$dba ) {
 			\DBA::dba_factory(
 				getenv('HZ_TEST_DB_HOST') ?: 'db',
@@ -71,36 +101,6 @@ class UnitTestCase extends TestCase {
 		}
 	}
 
-	protected function setUp() : void {
-		$myclass = get_class($this);
-		logger("[*] Running test: {$myclass}::{$this->getName(true)}", LOGGER_DEBUG);
-		if ( \DBA::$dba->connected ) {
-			// Create a transaction, so that any actions taken by the
-			// tests does not change the actual contents of the database.
-			$this->in_transaction = \DBA::$dba->db->beginTransaction();
-
-			$this->loadFixtures();
-		}
-
-		// Make sure app config is reset and loaded from fixtures
-		\App::$config = array();
-		\Zotlabs\Lib\Config::Load('system');
-	}
-
-	protected function tearDown() : void {
-		if ( \DBA::$dba->connected && $this->in_transaction ) {
-			// Roll back the transaction, restoring the db to the
-			// state it was before the test was run.
-			if ( \DBA::$dba->db->rollBack() ) {
-				$this->in_transaction = false;
-			} else {
-				throw new \Exception(
-					"Transaction rollback failed! Error is: "
-					. \DBA::$dba->db->errorInfo());
-			}
-		}
-	}
-
 	private static function dbtype(string $type): int {
 		if (trim(strtolower($type)) === 'postgres') {
 			return DBTYPE_POSTGRES;
diff --git a/tests/unit/includes/dba/TransactionTest.php b/tests/unit/includes/dba/TransactionTest.php
new file mode 100644
index 000000000..99e3f459d
--- /dev/null
+++ b/tests/unit/includes/dba/TransactionTest.php
@@ -0,0 +1,207 @@
+<?php
+/*
+ * Copyright (c) 2024 Hubzilla
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+require_once 'tests/fakes/fake_dba.php';
+require_once 'include/dba/dba_transaction.php';
+
+use \PHPUnit\Framework\TestCase;
+use \Zotlabs\Tests\Fakes\FakeDba;
+
+/**
+ * Test database transactions.
+ *
+ * This class subclass the base PHPUnit TestCase class, since we do _not_
+ * want a real database connection for these tests. We're testing functionality
+ * of the database adapter itself, so we choose to stub the underlying db driver
+ * to be able to assert that the adapter behaves as it should.
+ */
+class DbaTransactionTest extends TestCase {
+	private $pdo_stub;
+
+	public function setUp(): void {
+		$this->pdo_stub = $this->createStub(PDO::class);
+	}
+
+
+	/**
+	 * Test that creating a DbaTransaction object initiates a database transaction.
+	 *
+	 * @SuppressWarnings(PHPMD.UnusedLocalVariable)
+	 */
+	public function test_transaction_initialized_on_construction(): void {
+		// Stub PDO::inTransaction()
+		// Expect that it's called once, and return false to simulate that no
+		// transactions are active.
+		$this->pdo_stub
+			->expects($this->once())
+			->method('inTransaction')
+			->willReturn(false);
+
+		// Stub PDO::beginTransaction to ensure that it is being called.
+		$this->pdo_stub
+			->expects($this->once())
+			->method('beginTransaction')
+			->willReturn(true);
+
+		$dba = new FakeDba($this->pdo_stub);
+
+		$transaction = new DbaTransaction($dba);
+	}
+
+	/**
+	 * Test that a transaction is rolled back when the DbaTransaction object
+	 * is destroyed.
+	 *
+	 * @SuppressWarnings(PHPMD.UnusedLocalVariable)
+	 */
+	public function test_uncommitted_transaction_is_rolled_back_on_destruction(): void {
+		// Stub PDO::inTransaction()
+		// Expect that it's called once, and return false to simulate that no
+		// transactions are active.
+		$this->pdo_stub
+			->expects($this->once())
+			->method('inTransaction')
+			->willReturn(false);
+
+		// Stub PDO::beginTransaction to ensure that it is being called.
+		$this->pdo_stub
+			->expects($this->once())
+			->method('beginTransaction')
+			->willReturn(true);
+
+		// Stub PDO::rollBack to make sure we test it is being called.
+		$this->pdo_stub
+			->expects($this->once())
+			->method('rollBack')
+			->willReturn(true);
+
+		$dba = new FakeDba($this->pdo_stub);
+
+		$transaction = new DbaTransaction($dba);
+	}
+
+	/**
+	 * Test that a committed transaction is not rolled back when the
+	 * DbaTransaction object goes out of scope.
+	 */
+	public function test_committed_transaction_is_not_rolled_back(): void {
+		// Stub PDO::inTransaction()
+		// Return false to simulate that no transaction is active when called.
+		$this->pdo_stub
+			->expects($this->once())
+			->method('inTransaction')
+			->willReturn(false);
+
+		// Stub PDO::beginTransaction to ensure that it is being called.
+		$this->pdo_stub
+			->expects($this->once())
+			->method('beginTransaction')
+			->willReturn(true);
+
+		// Stub PDO::rollBack to ensure it is _not_ called
+		$this->pdo_stub
+			->expects($this->never())
+			->method('rollBack');
+
+		// Stub PDO::commit to make the test check that it is being called
+		$this->pdo_stub
+			->expects($this->once())
+			->method('commit')
+			->willReturn(true);
+
+		$dba = new FakeDba($this->pdo_stub);
+
+		$transaction = new DbaTransaction($dba);
+		$transaction->commit();
+	}
+
+	/**
+	 * Test that commiting a transaction more than once is a no-op.
+	 */
+	public function test_that_committing_an_already_committed_transaction_does_nothing(): void {
+		// Stub PDO::inTransaction()
+		// Return false to simulate that no transaction is active when called.
+		$this->pdo_stub
+			->expects($this->once())
+			->method('inTransaction')
+			->willReturn(false);
+
+		// Stub PDO::beginTransaction to ensure that it is being called.
+		$this->pdo_stub
+			->expects($this->once())
+			->method('beginTransaction')
+			->willReturn(true);
+
+		// Stub PDO::rollBack to ensure it is _not_ called
+		$this->pdo_stub
+			->expects($this->never())
+			->method('rollBack');
+
+		// Stub PDO::commit to make the test check that it is being called
+		$this->pdo_stub
+			->expects($this->once())
+			->method('commit')
+			->willReturn(true);
+
+		$dba = new FakeDba($this->pdo_stub);
+
+		$transaction = new DbaTransaction($dba);
+		$transaction->commit();
+		$transaction->commit();
+	}
+
+	/**
+	 * Test simulating constructing a DbaTransaction object when a transaction
+	 * is already active.
+	 *
+	 * This should _not_ initiate an actual DB transaction, not call the rollBack
+	 * method on destruction.
+	 *
+	 * @SuppressWarnings(PHPMD.UnusedLocalVariable)
+	 */
+	public function test_that_nesting_a_transaction_does_not_create_a_new_transaction_in_db(): void {
+		// Stub PDO::inTransaction()
+		// We simulate that a transaction is already active, by returning true from
+		// this method.
+		$this->pdo_stub
+			->expects($this->once())
+			->method('inTransaction')
+			->willReturn(true);
+
+		// Stub PDO::beginTransaction
+		// Since a transaction is already active, we should _not_ initiate
+		// a new transaction when the DbaTransaction object is constructed.
+		$this->pdo_stub
+			->expects($this->never())
+			->method('beginTransaction');
+
+		// Stub PDO::rollBack to ensure it is _not_ called
+		$this->pdo_stub
+			->expects($this->never())
+			->method('rollBack');
+
+		$dba = new FakeDba($this->pdo_stub);
+
+		$transaction = new DbaTransaction($dba);
+	}
+}
-- 
cgit v1.2.3