Folder get/set prop funcs and other changes

- Simplified folder removal; now properly relies on foreign keys for dependency resolution
- simplified *propertiesSet() methods by offloading input validation and query building to a generic function
- Implemented function to get the properties of a single folder (useful for internal use)
- Implemented a function to set the properties of a folder

lib/AbstractException.php

@@ -39,6 +39,7 @@ abstract class AbstractException extends \Exception {
         "Db/ExceptionInput.idMissing"           => 10235,
         "Db/ExceptionInput.constraintViolation" => 10236,
         "Db/ExceptionInput.typeViolation"       => 10237,
+        "Db/ExceptionInput.circularDependence"  => 10238,
         "Db/ExceptionTimeout.general"           => 10241,
         "Conf/Exception.fileMissing"            => 10301,
         "Conf/Exception.fileUnusable"           => 10302,

lib/Database.php

@@ -14,8 +14,27 @@ class Database {
     public    $db;
     private   $driver;
 
-    protected function cleanName(string $name): string {
+    protected function processUpdate(array $props, array $valid, array $where): array {
-        return (string) preg_filter("[^0-9a-zA-Z_\.]", "", $name);
+        $out = [
+            'values' => [],
+            'types'  => [],
+            'set'    => [],
+            'where'  => [],
+        ];
+        foreach($valid as $prop => $type) {
+            if(!array_key_exists($prop, $props)) continue;
+            $out['values'][] = $props[$prop];
+            $out['types'][]  = $type;
+            $out['set'][]    = "$prop = ?";
+        }
+        foreach($where as $field => $value) {
+            $out['values'][] = $value[0];
+            $out['types'][]  = $value[1];
+            $out['where'][]  = "$field is ?";
+        }
+        $out['set']   = implode(", ", $out['set']);
+        $out['where'] = implode(" and ", $out['where']);
+        return $out;
     }
 
     public function __construct(Db\Driver $db = null) {
@@ -233,16 +252,13 @@ class Database {
 
     public function userPropertiesSet(string $user, array $properties): array {
         if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
+        if(!$this->userExists($user)) throw new User\Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]);
         $valid = [ // FIXME: add future properties
             "name" => "str",
         ];
-        if(!$this->userExists($user)) throw new User\Exception("doesNotExist", ["action" => __FUNCTION__, "user" => $user]);
+        $data = $this->processUpdate($properties, $valid, ['id' => [$user, "str"]]);
-        $this->db->begin();
+        extract($data);
-        foreach($valid as $prop => $type) {
+        $this->db->prepareArray("UPDATE arsse_users set $set where $where", $types)->runArray($values);
-            if(!array_key_exists($prop, $properties)) continue;
-            $this->db->prepare("UPDATE arsse_users set $prop = ? where id is ?", $type, "str")->run($properties[$prop], $user);
-        }
-        $this->db->commit();
         return $this->userPropertiesGet($user);
     }
 
@@ -326,20 +342,68 @@ class Database {
 
     public function folderRemove(string $user, int $id): bool {
         if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
-        // if the user doesn't exist throw an exception.
+        if(!$this->userExists($user)) throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]);
-        if (!$this->userExists($user)) {
+        $changes = $this->db->prepare("DELETE FROM arsse_folders where owner is ? and id is ?", "str", "int")->run($user, $id)->changes();
-            throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]);
+        if(!$changes) throw new Db\ExceptionInput("idMissing", ["action" => __FUNCTION__, "field" => "folder", 'id' => $id]);
+        return true;
     }
-        // common table expression to list all descendant folders of the target folder
+
-        $cte = "RECURSIVE folders(id) as (SELECT id from arsse_folders where owner is ? and id is ? union select arsse_folders.id from arsse_folders join folders on arsse_folders.parent=folders.id) ";
+    public function folderPropertiesGet(string $user, int $id): array {
-        $changes = 0;
+        if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
-        $this->db->begin();
+        if(!$this->userExists($user)) throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]);
-        // first delete any feed subscriptions contained within the folder tree (this may not be necessary because of foreign keys)
+        $props = $this->db->prepare("SELECT id,name,parent from arsse_folders where owner is ? and id is ?", "str", "int")->run($user, $id)->getRow();
-        $changes += $this->db->prepare("WITH $cte"."DELETE FROM arsse_subscriptions where folder in(select id from folders)", "str", "int")->run($user, $id)->changes();
+        if(!$props) throw new Db\ExceptionInput("idMissing", ["action" => __FUNCTION__, "field" => "folder", 'id' => $id]);
-        // next delete the folders themselves
+        return $props;
-        $changes += $this->db->prepare("WITH $cte"."DELETE FROM arsse_folders where id in(select id from folders)", "str", "int")->run($user, $id)->changes();
+    }
-        $this->db->commit();
+
-        return (bool) $changes;
+    public function folderPropertiesSet(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__]);
+        // layer the existing folder properties onto the new desired one
+        $data = array_merge($this->folderPropertiesGet($user, $id), $data);
+        // if the desired folder name is missing or invalid, throw an exception
+        if(!array_key_exists("name", $data) || $data['name']=="") {
+            throw new Db\ExceptionInput("missing", ["action" => __FUNCTION__, "field" => "name"]);
+        } else if(!strlen(trim($data['name']))) {
+            throw new Db\ExceptionInput("whitespace", ["action" => __FUNCTION__, "field" => "name"]);
+        }
+        // normalize folder's parent, if there is one
+        $parent = array_key_exists("parent", $data) ? (int) $data['parent'] : 0;
+        if($parent===0) {
+            // if no parent is specified, do nothing
+            $parent = null;
+            $root = null;
+        } else {
+            // if a parent is specified, make sure it exists and belongs to the user; get its root (first-level) folder if it's a nested folder
+            $p = $this->db->prepare(
+                "WITH RECURSIVE folders(id) as (SELECT id from arsse_folders where owner is ? and id is ? union select arsse_folders.id from arsse_folders join folders on arsse_folders.parent=folders.id) ".
+                "SELECT id,root,(id not in (select id from folders)) as valid from arsse_folders where owner is ? and id is ?", 
+            "str", "int", "str", "int")->run($user, $id, $user, $parent)->getRow();
+            if(!$p) {
+                throw new Db\ExceptionInput("idMissing", ["action" => __FUNCTION__, "field" => "parent", 'id' => $parent]);
+            } else {
+                // if using the desired parent would create a circular dependence, throw a constraint violation
+                if(!$p['valid']) throw new Db\ExceptionInput("circularDependence", ["action" => __FUNCTION__, "field" => "parent", 'id' => $parent]);
+                // if the parent does not have a root specified (because it is a first-level folder) use the parent ID as the root ID
+                $root = $p['root']===null ? $parent : $p['root'];
+            }
+        }
+        $data['parent'] = $parent;
+        $data['root']   = $root;
+        // check to make sure the target folder name/location would not create a duplicate (we must di this check because null is not distinct in SQL)
+        $existing = $this->db->prepare("SELECT id from arsse_folders where owner is ? and parent is ? and name is ?", "str", "int", "str")->run($user, $data['parent'], $data['name'])->getValue();
+        if(!is_null($existing) && $existing != $id) {
+            throw new Db\ExceptionInput("constraintViolation"); // FIXME: There needs to be a practical message here
+        }
+        $valid = [
+            'name' => "str",
+            'parent' => "int",
+            'root' => "int",
+        ];
+        $data = $this->processUpdate($data, $valid, ['owner' => [$user, "str"], 'id' => [$id, "int"]]);
+        extract($data);
+        $this->db->prepareArray("UPDATE arsse_folders set $set where $where", $types)->runArray($values);
+        return true;
     }
 
     public function subscriptionAdd(string $user, string $url, string $fetchUser = "", string $fetchPassword = ""): int {

locale/en.php

@@ -55,6 +55,7 @@ return [
     'Exception.JKingWeb/Arsse/Db/ExceptionInput.tooLong'             => 'Required field "{field}" of action "{action}" has a maximum length of {max}',
     'Exception.JKingWeb/Arsse/Db/ExceptionInput.tooShort'            => 'Required field "{field}" of action "{action}" has a minimum length of {min}',
     'Exception.JKingWeb/Arsse/Db/ExceptionInput.idMissing'           => 'Referenced ID ({id}) in field "{field}" does not exist',
+    'Exception.JKingWeb/Arsse/Db/ExceptionInput.circularDependence'  => 'Referenced ID ({id}) in field "{field}" creates a circular dependence',
     'Exception.JKingWeb/Arsse/Db/ExceptionInput.constraintViolation' => '{0}',
     'Exception.JKingWeb/Arsse/Db/ExceptionInput.typeViolation'       => '{0}',
     'Exception.JKingWeb/Arsse/Db/ExceptionTimeout.general'           => '{0}',

sql/SQLite3/0.sql

@@ -53,8 +53,8 @@ create table arsse_subscriptions(
 create table arsse_folders(
     id integer primary key not null,                                                                        -- sequence number
     owner TEXT not null references arsse_users(id) on delete cascade on update cascade,                     -- owner of folder
-    parent integer default null,                                                                            -- parent folder id
+    parent integer references arsse_folders(id) on delete cascade,                                          -- parent folder id
-    root integer default null,                                                                              -- first-level folder (NextCloud folder)
+    root integer references arsse_folders(id) on delete cascade,                                            -- first-level folder (NextCloud folder)
     name TEXT not null,                                                                                     -- folder name
     modified datetime not null default CURRENT_TIMESTAMP,                                                   --
     unique(owner,name,parent)                                                                               -- cannot have multiple folders with the same name under the same parent for the same owner

tests/lib/Database/SeriesFolder.php

@@ -171,11 +171,13 @@ trait SeriesFolder {
     }
 
     function testRemoveAMissingFolder() {
-        $this->assertFalse(Data::$db->folderRemove("john.doe@example.com", 2112));
+        $this->assertException("idMissing", "Db", "ExceptionInput");
+        Data::$db->folderRemove("john.doe@example.com", 2112);
     }
 
-    function testRemoveFolderOfTheWrongOwner() {
+    function testRemoveAFolderOfTheWrongOwner() {
-        $this->assertFalse(Data::$db->folderRemove("john.doe@example.com", 4)); // folder ID 4 belongs to Jane
+        $this->assertException("idMissing", "Db", "ExceptionInput");
+        Data::$db->folderRemove("john.doe@example.com", 4); // folder ID 4 belongs to Jane
     }
 
     function testRemoveAFolderForAMissingUser() {
@@ -186,6 +188,92 @@ trait SeriesFolder {
     function testRemoveAFolderWithoutAuthority() {
         Phake::when(Data::$user)->authorize->thenReturn(false);
         $this->assertException("notAuthorized", "User", "ExceptionAuthz");
-        Data::$db->folderList("john.doe@example.com", 1);
+        Data::$db->folderRemove("john.doe@example.com", 1);
+    }
+
+    function testGetThePropertiesOfAFolder() {
+        $exp = [
+            'id'     => 6,
+            'name'   => "Politics",
+            'parent' => 2,
+        ];
+        $this->assertArraySubset($exp, Data::$db->folderPropertiesGet("john.doe@example.com", 6));
+    }
+
+    function testGetThePropertiesOfAMissingFolder() {
+        $this->assertException("idMissing", "Db", "ExceptionInput");
+        Data::$db->folderPropertiesGet("john.doe@example.com", 2112);
+    }
+
+    function testGetThePropertiesOfAFolderOfTheWrongOwner() {
+        $this->assertException("idMissing", "Db", "ExceptionInput");
+        Data::$db->folderPropertiesGet("john.doe@example.com", 4); // folder ID 4 belongs to Jane
+    }
+
+    function testGetThePropertiesOfAFolderForAMissingUser() {
+        $this->assertException("doesNotExist", "User");
+        Data::$db->folderPropertiesGet("john.doe@example.org", 1);
+    }
+
+    function testGetThePropertiesOfAFolderWithoutAuthority() {
+        Phake::when(Data::$user)->authorize->thenReturn(false);
+        $this->assertException("notAuthorized", "User", "ExceptionAuthz");
+        Data::$db->folderPropertiesGet("john.doe@example.com", 1);
+    }
+
+    function testRenameAFolder() {
+        $this->assertTrue(Data::$db->folderPropertiesSet("john.doe@example.com", 6, ['name' => "Opinion"]));
+        $state = $this->primeExpectations($this->data, ['arsse_folders' => ['id','owner', 'parent', 'root', 'name']]);
+        $state['arsse_folders']['rows'][5][4] = "Opinion";
+        $this->compareExpectations($state);
+    }
+
+    function testMoveAFolder() {
+        $this->assertTrue(Data::$db->folderPropertiesSet("john.doe@example.com", 6, ['parent' => 5]));
+        $state = $this->primeExpectations($this->data, ['arsse_folders' => ['id','owner', 'parent', 'root', 'name']]);
+        $state['arsse_folders']['rows'][5][2] = 5; // parent should have changed
+        $state['arsse_folders']['rows'][5][3] = 5; // root should also have changed
+        $this->compareExpectations($state);
+    }
+
+    function testMoveAFolderToItsDescendant() {
+        $this->assertException("circularDependence", "Db", "ExceptionInput");
+        Data::$db->folderPropertiesSet("john.doe@example.com", 1, ['parent' => 3]);
+    }
+
+    function testMoveAFolderToItself() {
+        $this->assertException("circularDependence", "Db", "ExceptionInput");
+        Data::$db->folderPropertiesSet("john.doe@example.com", 1, ['parent' => 1]);
+    }
+
+    function testMoveAFolderToAMissingParent() {
+        $this->assertException("idMissing", "Db", "ExceptionInput");
+        Data::$db->folderPropertiesSet("john.doe@example.com", 1, ['parent' => 2112]);
+    }
+
+    function testCauseAFolderCollision() {
+        $this->assertException("constraintViolation", "Db", "ExceptionInput");
+        Data::$db->folderPropertiesSet("john.doe@example.com", 6, ['parent' => null]);
+    }
+
+    function testSetThePropertiesOfAMissingFolder() {
+        $this->assertException("idMissing", "Db", "ExceptionInput");
+        Data::$db->folderPropertiesSet("john.doe@example.com", 2112, ['parent' => null]);
+    }
+
+    function testSetThePropertiesOfAFolderOfTheWrongOwner() {
+        $this->assertException("idMissing", "Db", "ExceptionInput");
+        Data::$db->folderPropertiesSet("john.doe@example.com", 4, ['parent' => null]); // folder ID 4 belongs to Jane
+    }
+
+    function testSetThePropertiesOfAFolderForAMissingUser() {
+        $this->assertException("doesNotExist", "User");
+        Data::$db->folderPropertiesSet("john.doe@example.org", 1, ['parent' => null]);
+    }
+
+    function testSetThePropertiesOfAFolderWithoutAuthority() {
+        Phake::when(Data::$user)->authorize->thenReturn(false);
+        $this->assertException("notAuthorized", "User", "ExceptionAuthz");
+        Data::$db->folderPropertiesSet("john.doe@example.com", 1, ['parent' => null]);
     }
 }