From a111bcc231b690aa69d42ce64a0182856de71f1a Mon Sep 17 00:00:00 2001 From: "J. King" Date: Sat, 1 Apr 2017 10:27:26 -0400 Subject: [PATCH] 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 | 1 + lib/Database.php | 152 ++++++++++++++++++++-------- locale/en.php | 1 + sql/SQLite3/0.sql | 4 +- tests/lib/Database/SeriesFolder.php | 96 +++++++++++++++++- 5 files changed, 204 insertions(+), 50 deletions(-) diff --git a/lib/AbstractException.php b/lib/AbstractException.php index 7476699..bd7855d 100644 --- a/lib/AbstractException.php +++ b/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, diff --git a/lib/Database.php b/lib/Database.php index 2974d09..e4e8f0f 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -14,8 +14,27 @@ class Database { public $db; private $driver; - protected function cleanName(string $name): string { - return (string) preg_filter("[^0-9a-zA-Z_\.]", "", $name); + protected function processUpdate(array $props, array $valid, array $where): array { + $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]); - $this->db->begin(); - foreach($valid as $prop => $type) { - 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(); + $data = $this->processUpdate($properties, $valid, ['id' => [$user, "str"]]); + extract($data); + $this->db->prepareArray("UPDATE arsse_users set $set where $where", $types)->runArray($values); return $this->userPropertiesGet($user); } @@ -260,11 +276,11 @@ class Database { public function folderAdd(string $user, array $data): int { // If the user isn't authorized to perform this action then throw an exception. - if (!Data::$user->authorize($user, __FUNCTION__)) { + 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)) { + if(!$this->userExists($user)) { throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]); } // if the desired folder name is missing or invalid, throw an exception @@ -300,11 +316,11 @@ class Database { public function folderList(string $user, int $parent = null, bool $recursive = true): Db\Result { // if the user isn't authorized to perform this action then throw an exception. - if (!Data::$user->authorize($user, __FUNCTION__)) { + 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)) { + if(!$this->userExists($user)) { throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]); } // check to make sure the parent exists, if one is specified @@ -326,29 +342,77 @@ 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)) throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]); + $changes = $this->db->prepare("DELETE FROM arsse_folders where owner is ? and id is ?", "str", "int")->run($user, $id)->changes(); + if(!$changes) throw new Db\ExceptionInput("idMissing", ["action" => __FUNCTION__, "field" => "folder", 'id' => $id]); + return true; + } + + public function folderPropertiesGet(string $user, int $id): 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", ["user" => $user, "action" => __FUNCTION__]); + $props = $this->db->prepare("SELECT id,name,parent from arsse_folders where owner is ? and id is ?", "str", "int")->run($user, $id)->getRow(); + if(!$props) throw new Db\ExceptionInput("idMissing", ["action" => __FUNCTION__, "field" => "folder", 'id' => $id]); + return $props; + } + + 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"]); } - // 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) "; - $changes = 0; - $this->db->begin(); - // first delete any feed subscriptions contained within the folder tree (this may not be necessary because of foreign keys) - $changes += $this->db->prepare("WITH $cte"."DELETE FROM arsse_subscriptions where folder in(select id from folders)", "str", "int")->run($user, $id)->changes(); - // next delete the folders themselves - $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; + // 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 { // If the user isn't authorized to perform this action then throw an exception. - if (!Data::$user->authorize($user, __FUNCTION__)) { + 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)) { + if(!$this->userExists($user)) { throw new User\Exception("doesNotExist", ["user" => $user, "action" => __FUNCTION__]); } @@ -358,7 +422,7 @@ class Database { // after determining its validity with PicoFeed. $qFeed = $this->db->prepare("SELECT id from arsse_feeds where url is ? and username is ? and password is ?", "str", "str", "str"); $feed = $qFeed->run($url, $fetchUser, $fetchPassword)->getValue(); - if ($feed === null) { + if($feed === null) { $feed = new Feed($url); $feed->parse(); @@ -381,7 +445,7 @@ class Database { )->lastId(); // Add each of the articles to the database. - foreach ($feed->data->items as $i) { + foreach($feed->data->items as $i) { $this->articleAdd($i); } } @@ -427,8 +491,8 @@ class Database { $this->db->begin(); $categories = $article->getTag('category'); - if (count($categories) > 0) { - foreach ($categories as $c) { + if(count($categories) > 0) { + foreach($categories as $c) { $this->db->prepare('INSERT INTO arsse_categories(article,name) values(?,?)', 'int', 'str')->run($id, $c); } } @@ -438,7 +502,7 @@ class Database { public function updateFeeds(): int { $feeds = $this->db->query('SELECT id, url, username, password, DATEFORMAT("http", modified) AS lastmodified, etag FROM arsse_feeds')->getAll(); - foreach ($feeds as $f) { + foreach($feeds as $f) { // Feed object throws an exception when there are problems, but that isn't ideal // here. When an exception is occurred it should update the database with the // error instead of failing. @@ -454,34 +518,34 @@ class Database { } // If the feed has been updated then update the database. - if ($feed->resource->isModified()) { + if($feed->resource->isModified()) { $feed->parse(); $this->db->begin(); $articles = $this->db->prepare('SELECT id, url, title, author, DATEFORMAT("http", edited) AS edited_date, guid, content, url_title_hash, url_content_hash, title_content_hash FROM arsse_articles WHERE feed is ? ORDER BY id', 'int')->run($f['id'])->getAll(); - foreach ($feed->data->items as $i) { + foreach($feed->data->items as $i) { // Iterate through the articles in the database to determine a match for the one // in the just-parsed feed. $match = null; - foreach ($articles as $a) { + foreach($articles as $a) { // If the id exists and is equal to one in the database then this is the post. - if ($i->id) { - if ($i->id === $a['guid']) { + if($i->id) { + if($i->id === $a['guid']) { $match = $a; } } // Otherwise if the id doesn't exist and any of the hashes match then this is // the post. - elseif ($i->urlTitleHash === $a['url_title_hash'] || $i->urlContentHash === $a['url_content_hash'] || $i->titleContentHash === $a['title_content_hash']) { + elseif($i->urlTitleHash === $a['url_title_hash'] || $i->urlContentHash === $a['url_content_hash'] || $i->titleContentHash === $a['title_content_hash']) { $match = $a; } } // If there is no match then this is a new post and must be added to the // database. - if (!$match) { + if(!$match) { $this->articleAdd($i); continue; } @@ -490,18 +554,18 @@ class Database { // If there is an updated date, and it doesn't match the database's then update // the post. $update = false; - if ($i->updatedDate) { - if ($i->updatedDate !== $match['edited_date']) { + if($i->updatedDate) { + if($i->updatedDate !== $match['edited_date']) { $update = true; } } // Otherwise if there isn't an updated date and any of the hashes don't match // then update the post. - elseif ($i->urlTitleHash !== $match['url_title_hash'] || $i->urlContentHash !== $match['url_content_hash'] || $i->titleContentHash !== $match['title_content_hash']) { + elseif($i->urlTitleHash !== $match['url_title_hash'] || $i->urlContentHash !== $match['url_content_hash'] || $i->titleContentHash !== $match['title_content_hash']) { $update = true; } - if ($update) { + if($update) { $this->db->prepare('UPDATE arsse_articles SET url = ?, title = ?, author = ?, published = ?, edited = ?, modified = ?, guid = ?, content = ?, url_title_hash = ?, url_content_hash = ?, title_content_hash = ? WHERE id is ?', 'str', 'str', 'str', 'datetime', 'datetime', 'datetime', 'str', 'str', 'str', 'str', 'str', 'int')->run( $i->url, $i->title, diff --git a/locale/en.php b/locale/en.php index 7a74c16..b4c351f 100644 --- a/locale/en.php +++ b/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}', diff --git a/sql/SQLite3/0.sql b/sql/SQLite3/0.sql index 18d0ab8..e767663 100644 --- a/sql/SQLite3/0.sql +++ b/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 - root integer default null, -- first-level folder (NextCloud folder) + parent integer references arsse_folders(id) on delete cascade, -- parent folder id + 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 diff --git a/tests/lib/Database/SeriesFolder.php b/tests/lib/Database/SeriesFolder.php index 61c6cb6..4e889cc 100644 --- a/tests/lib/Database/SeriesFolder.php +++ b/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() { - $this->assertFalse(Data::$db->folderRemove("john.doe@example.com", 4)); // folder ID 4 belongs to Jane + function testRemoveAFolderOfTheWrongOwner() { + $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]); } } \ No newline at end of file