Change transactions to auto-rollback on exceptions

lib/Database.php

@@ -458,7 +458,7 @@ class Database {
     public function subscriptionPropertiesSet(string $user, int $id, array $data): bool {
         if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
         if(!$this->userExists($user)) throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]);
-        $this->db->begin();
+        $tr = $this->db->begin();
         if(!$this->db->prepare("SELECT count(*) from arsse_subscriptions where owner is ? and id is ?", "str", "int")->run($user, $id)->getValue()) {
             // if the ID doesn't exist or doesn't belong to the user, throw an exception
             throw new Db\ExceptionInput("idMissing", ["action" => __FUNCTION__, "field" => "feed", 'id' => $id]);
@@ -470,12 +470,13 @@ class Database {
             'pinned'     => "strict bool",
         ];
         list($setClause, $setTypes, $setValues) = $this->generateSet($data, $valid);
-        return (bool) $this->db->prepare("UPDATE arsse_subscriptions set $setClause where owner is ? and id is ?", $setTypes, "str", "int")->run($setValues, $user, $id)->changes();
+        $out = (bool) $this->db->prepare("UPDATE arsse_subscriptions set $setClause where owner is ? and id is ?", $setTypes, "str", "int")->run($setValues, $user, $id)->changes();
+        $tr->commit();
+        return $out;
     }
 
     public function feedUpdate(int $feedID, bool $throwError = false): bool {
-        $this->db->begin();
+        $tr = $this->db->begin();
-        try {
         // check to make sure the feed exists
         $f = $this->db->prepare('SELECT url, username, password, DATEFORMAT("http", modified) AS lastmodified, etag, err_count FROM arsse_feeds where id is ?', "int")->run($feedID)->getRow();
         if(!$f) throw new Db\ExceptionInput("idMissing", ["action" => __FUNCTION__, "field" => "feed", 'id' => $feedID]);
@@ -487,7 +488,7 @@ class Database {
             if(!$feed->modified) {
                 // if the feed hasn't changed, just compute the next fetch time and record it
                 $this->db->prepare('UPDATE arsse_feeds SET updated = CURRENT_TIMESTAMP, next_fetch = ? WHERE id is ?', 'datetime', 'int')->run($feed->nextFetch, $feedID);
-                    $this->db->commit();
+                $tr->commit();
                 return false;
             }
         } catch (Feed\Exception $e) {
@@ -496,12 +497,9 @@ class Database {
                 'UPDATE arsse_feeds SET updated = CURRENT_TIMESTAMP, next_fetch = ?, err_count = err_count + 1, err_msg = ? WHERE id is ?', 
                 'datetime', 'str', 'int'
             )->run(Feed::nextFetchOnError($f['err_count']), $e->getMessage(),$feedID);
-                $this->db->commit();
+            $tr->commit();
             if($throwError) throw $e;
             return false;
-            } catch(\Throwable $e) {
-                $this->db->rollback();
-                throw $e;
         }
         //prepare the necessary statements to perform the update
         if(sizeof($feed->newItems) || sizeof($feed->changedItems)) {
@@ -577,11 +575,7 @@ class Database {
             $feed->nextFetch,
             $feedID
         );
-        } catch(\Throwable $e) {
+        $tr->commit();
-            $this->db->rollback();
-            throw $e;
-        }
-        $this->db->commit();
         return true;
     }
 

lib/Db/AbstractDriver.php

@@ -14,12 +14,16 @@ abstract class AbstractDriver implements Driver {
         }
     }
 
-    public function begin(): bool {
+    public function begin(): Transaction {
+        return new Transaction($this);
+    }
+    
+    public function savepointCreate(): bool {
         $this->exec("SAVEPOINT arsse_".(++$this->transDepth));
         return true;
     }
 
-    public function commit(bool $all = false): bool {
+    public function savepointRelease(bool $all = false): bool {
         if($this->transDepth==0) return false;
         if(!$all) {
             $this->exec("RELEASE SAVEPOINT arsse_".($this->transDepth--));
@@ -30,7 +34,7 @@ abstract class AbstractDriver implements Driver {
         return true;
     }
 
-    public function rollback(bool $all = false): bool {
+    public function savepointUndo(bool $all = false): bool {
         if($this->transDepth==0) return false;
         if(!$all) {
             $this->exec("ROLLBACK TRANSACTION TO SAVEPOINT arsse_".($this->transDepth));

lib/Db/Driver.php

@@ -8,12 +8,14 @@ interface Driver {
     static function driverName(): string;
     // returns the version of the scheme of the opened database; if uninitialized should return 0
     function schemaVersion(): int;
-    // begin a real or synthetic transactions, with real or synthetic nesting
+    // return a Transaction object
-    function begin(): bool;
+    function begin(): Transaction;
-    // commit either the latest or all pending nested transactions; use of this method should assume a partial commit is a no-op
+    // manually begin a real or synthetic transactions, with real or synthetic nesting
-    function commit(bool $all = false): bool;
+    function savepointCreate(): bool;
-    // rollback either the latest or all pending nested transactions; use of this method should assume a partial rollback will not work
+    // manually commit either the latest or all pending nested transactions
-    function rollback(bool $all = false): bool;
+    function savepointRelease(bool $all = false): bool;
+    // manually rollback either the latest or all pending nested transactions
+    function savepointUndo(bool $all = false): bool;
     // attempt to advise other processes that they should not attempt to access the database; used during live upgrades
     function lock(): bool;
     function unlock(): bool;

lib/Db/SQLite3/Driver.php

@@ -69,9 +69,9 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver {
         $sep = \DIRECTORY_SEPARATOR;
         $path = Data::$conf->dbSchemaBase.$sep."SQLite3".$sep;
         $this->lock();
-        $this->begin();
+        $this->savepointCreate();
         for($a = $ver; $a < $to; $a++) {
-            $this->begin();
+            $this->savepointCreate();
             try {
                 $file = $path.$a.".sql";
                 if(!file_exists($file)) throw new Exception("updateFileMissing", ['file' => $file, 'driver_name' => $this->driverName(), 'current' => $a]);
@@ -86,17 +86,17 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver {
                 if($this->schemaVersion() != $a+1) throw new Exception("updateFileIncomplete", ['file' => $file, 'driver_name' => $this->driverName(), 'current' => $a]);
             } catch(\Throwable $e) {
                 // undo any partial changes from the failed update
-                $this->rollback();
+                $this->savepointUndo();
                 $this->unlock();
                 // commit any successful updates if updating by more than one version
-                $this->commit(true);
+                $this->savepointRelease(true);
                 // throw the error received
                 throw $e;
             }
-            $this->commit();
+            $this->savepointRelease();
         }
         $this->unlock();
-        $this->commit();
+        $this->savepointRelease();
         return true;
     }
 

lib/Db/Transaction.php

@@ -0,0 +1,44 @@
+<?php
+declare(strict_types=1);
+namespace JKingWeb\Arsse\Db;
+
+class Transaction {
+    protected $pending = false;
+    protected $drv;
+
+    function __construct(Driver $drv) {
+        $drv->savepointCreate();
+        $this->drv = $drv;
+        $this->pending = true;
+    }
+
+    function __destruct() {
+        if($this->pending) {
+            try {
+                $this->drv->savepointUndo();
+            } catch(\Throwable $e) {
+                // do nothing
+            }
+        }
+    }
+
+    function commit(): bool {
+        if($this->pending) {
+            $this->drv->savepointRelease();
+            $this->pending = false;
+            return true;
+        } else {
+            return false;
+        }
+    }
+
+    function rollback(): bool {
+        if($this->pending) {
+            $this->drv->savepointUndo();
+            $this->pending = false;
+            return true;
+        } else {
+            return false;
+        }
+    }
+}

tests/Db/SQLite3/TestDbDriverSQLite3.php

@@ -106,7 +106,7 @@ class TestDbDriverSQLite3 extends \PHPUnit\Framework\TestCase {
         $insert = "INSERT INTO test(id) values(null)";
         $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
-        $this->drv->begin();
+        $tr = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
@@ -120,11 +120,11 @@ class TestDbDriverSQLite3 extends \PHPUnit\Framework\TestCase {
         $insert = "INSERT INTO test(id) values(null)";
         $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
-        $this->drv->begin();
+        $tr = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->commit();
+        $tr->commit();
         $this->assertEquals(1, $this->drv->query($select)->getValue());
         $this->assertEquals(1, $ch->querySingle($select));
     }
@@ -134,11 +134,11 @@ class TestDbDriverSQLite3 extends \PHPUnit\Framework\TestCase {
         $insert = "INSERT INTO test(id) values(null)";
         $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
-        $this->drv->begin();
+        $tr = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->rollback();
+        $tr->rollback();
         $this->assertEquals(0, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
     }
@@ -148,11 +148,11 @@ class TestDbDriverSQLite3 extends \PHPUnit\Framework\TestCase {
         $insert = "INSERT INTO test(id) values(null)";
         $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
-        $this->drv->begin();
+        $tr1 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->begin();
+        $tr2 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(2, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
@@ -163,17 +163,17 @@ class TestDbDriverSQLite3 extends \PHPUnit\Framework\TestCase {
         $insert = "INSERT INTO test(id) values(null)";
         $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
-        $this->drv->begin();
+        $tr1 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->begin();
+        $tr2 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(2, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->commit();
+        $tr2->commit();
         $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->commit();
+        $tr1->commit();
         $this->assertEquals(2, $ch->querySingle($select));
     }
 
@@ -182,18 +182,18 @@ class TestDbDriverSQLite3 extends \PHPUnit\Framework\TestCase {
         $insert = "INSERT INTO test(id) values(null)";
         $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
-        $this->drv->begin();
+        $tr1 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->begin();
+        $tr2 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(2, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->rollback();
+        $tr2->rollback();
         $this->assertEquals(1, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->rollback();
+        $tr1->rollback();
         $this->assertEquals(0, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
     }
@@ -203,58 +203,22 @@ class TestDbDriverSQLite3 extends \PHPUnit\Framework\TestCase {
         $insert = "INSERT INTO test(id) values(null)";
         $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
-        $this->drv->begin();
+        $tr1 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->begin();
+        $tr2 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(2, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->rollback();
+        $tr2->rollback();
         $this->assertEquals(1, $this->drv->query($select)->getValue());
         $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->commit();
+        $tr1->commit();
         $this->assertEquals(1, $this->drv->query($select)->getValue());
         $this->assertEquals(1, $ch->querySingle($select));
     }
 
-    function testFullyRollbackChainedTransactions() {
-        $select = "SELECT count(*) FROM test";
-        $insert = "INSERT INTO test(id) values(null)";
-        $ch = new \SQLite3(Data::$conf->dbSQLite3File);
-        $this->drv->exec("CREATE TABLE test(id integer primary key)");
-        $this->drv->begin();
-        $this->drv->query($insert);
-        $this->assertEquals(1, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->begin();
-        $this->drv->query($insert);
-        $this->assertEquals(2, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->rollback(true);
-        $this->assertEquals(0, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
-    }
-
-    function testFullyCommitChainedTransactions() {
-        $select = "SELECT count(*) FROM test";
-        $insert = "INSERT INTO test(id) values(null)";
-        $ch = new \SQLite3(Data::$conf->dbSQLite3File);
-        $this->drv->exec("CREATE TABLE test(id integer primary key)");
-        $this->drv->begin();
-        $this->drv->query($insert);
-        $this->assertEquals(1, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->begin();
-        $this->drv->query($insert);
-        $this->assertEquals(2, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
-        $this->drv->commit(true);
-        $this->assertEquals(2, $this->drv->query($select)->getValue());
-        $this->assertEquals(2, $ch->querySingle($select));
-    }
-
     function testFetchSchemaVersion() {
         $this->assertSame(0, $this->drv->schemaVersion());
         $this->drv->exec("PRAGMA user_version=1");

tests/lib/Database/Setup.php

@@ -52,7 +52,7 @@ trait Setup {
     }
 
     function primeDatabase(array $data): bool {
-        $this->drv->begin();
+        $tr = $this->drv->begin();
         foreach($data as $table => $info) {
             $cols = implode(",", array_keys($info['columns']));
             $bindings = array_values($info['columns']);
@@ -62,7 +62,7 @@ trait Setup {
                 $this->assertEquals(1, $s->runArray($row)->changes());
             }
         }
-        $this->drv->commit();
+        $tr->commit();
         return true;
     }