From 1e1b848c62d1a0333703b284d8e977610d27736a Mon Sep 17 00:00:00 2001 From: "J. King" Date: Sat, 1 Apr 2017 14:49:31 -0400 Subject: [PATCH] Remove root field from folders table The field is no longer required with the use of recursive common table expressions, and presents a possible loss of referential integrity --- lib/Database.php | 21 +++++-------------- sql/SQLite3/0.sql | 1 - tests/lib/Database/SeriesFolder.php | 32 ++++++++++++++--------------- tests/lib/Database/Setup.php | 16 ++++++++++----- 4 files changed, 31 insertions(+), 39 deletions(-) diff --git a/lib/Database.php b/lib/Database.php index e4e8f0fd..127cbcec 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -294,16 +294,10 @@ class Database { 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("SELECT id,root from arsse_folders where owner is ? and id is ?", "str", "int")->run($user, $parent)->getRow(); - if(!$p) { - throw new Db\ExceptionInput("idMissing", ["action" => __FUNCTION__, "field" => "parent", 'id' => $parent]); - } else { - // 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']; - } + $p = $this->db->prepare("SELECT id from arsse_folders where owner is ? and id is ?", "str", "int")->run($user, $parent)->getValue(); + if(!$p) throw new Db\ExceptionInput("idMissing", ["action" => __FUNCTION__, "field" => "parent", 'id' => $parent]); } // check if a folder by the same name already exists, because nulls are wonky in SQL // FIXME: How should folder name be compared? Should a Unicode normalization be applied before comparison and insertion? @@ -311,7 +305,7 @@ class Database { throw new Db\ExceptionInput("constraintViolation"); // FIXME: There needs to be a practical message here } // actually perform the insert (!) - return $this->db->prepare("INSERT INTO arsse_folders(owner,parent,root,name) values(?,?,?,?)", "str", "int", "int", "str")->run($user, $parent, $root, $data['name'])->lastId(); + return $this->db->prepare("INSERT INTO arsse_folders(owner,parent,name) values(?,?,?)", "str", "int", "str")->run($user, $parent, $data['name'])->lastId(); } public function folderList(string $user, int $parent = null, bool $recursive = true): Db\Result { @@ -372,24 +366,20 @@ class Database { 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 ?", + "SELECT id,(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 using the desired parent would create a circular dependence, throw an exception 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) { @@ -398,7 +388,6 @@ class Database { $valid = [ 'name' => "str", 'parent' => "int", - 'root' => "int", ]; $data = $this->processUpdate($data, $valid, ['owner' => [$user, "str"], 'id' => [$id, "int"]]); extract($data); diff --git a/sql/SQLite3/0.sql b/sql/SQLite3/0.sql index e767663f..e81f765f 100644 --- a/sql/SQLite3/0.sql +++ b/sql/SQLite3/0.sql @@ -54,7 +54,6 @@ 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 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 4e889cc8..205197bf 100644 --- a/tests/lib/Database/SeriesFolder.php +++ b/tests/lib/Database/SeriesFolder.php @@ -13,7 +13,6 @@ trait SeriesFolder { 'id' => "int", 'owner' => "str", 'parent' => "int", - 'root' => "int", 'name' => "str", ], /* Layout translates to: @@ -27,12 +26,12 @@ trait SeriesFolder { Politics */ 'rows' => [ - [1, "john.doe@example.com", null, null, "Technology"], - [2, "john.doe@example.com", 1, 1, "Software"], - [3, "john.doe@example.com", 1, 1, "Rocketry"], - [4, "jane.doe@example.com", null, null, "Politics"], - [5, "john.doe@example.com", null, null, "Politics"], - [6, "john.doe@example.com", 2, 1, "Politics"], + [1, "john.doe@example.com", null, "Technology"], + [2, "john.doe@example.com", 1, "Software"], + [3, "john.doe@example.com", 1, "Rocketry"], + [4, "jane.doe@example.com", null, "Politics"], + [5, "john.doe@example.com", null, "Politics"], + [6, "john.doe@example.com", 2, "Politics"], ] ] ]; @@ -45,8 +44,8 @@ trait SeriesFolder { $user = "john.doe@example.com"; $this->assertSame(7, Data::$db->folderAdd($user, ['name' => "Entertainment"])); Phake::verify(Data::$user)->authorize($user, "folderAdd"); - $state = $this->primeExpectations($this->data, ['arsse_folders' => ['id','owner', 'parent', 'root', 'name']]); - $state['arsse_folders']['rows'][] = [7, $user, null, null, "Entertainment"]; + $state = $this->primeExpectations($this->data, ['arsse_folders' => ['id','owner', 'parent', 'name']]); + $state['arsse_folders']['rows'][] = [7, $user, null, "Entertainment"]; $this->compareExpectations($state); } @@ -59,8 +58,8 @@ trait SeriesFolder { $user = "john.doe@example.com"; $this->assertSame(7, Data::$db->folderAdd($user, ['name' => "GNOME", 'parent' => 2])); Phake::verify(Data::$user)->authorize($user, "folderAdd"); - $state = $this->primeExpectations($this->data, ['arsse_folders' => ['id','owner', 'parent', 'root', 'name']]); - $state['arsse_folders']['rows'][] = [7, $user, 2, 1, "GNOME"]; + $state = $this->primeExpectations($this->data, ['arsse_folders' => ['id','owner', 'parent', 'name']]); + $state['arsse_folders']['rows'][] = [7, $user, 2, "GNOME"]; $this->compareExpectations($state); } @@ -156,14 +155,14 @@ trait SeriesFolder { function testRemoveAFolder() { $this->assertTrue(Data::$db->folderRemove("john.doe@example.com", 6)); - $state = $this->primeExpectations($this->data, ['arsse_folders' => ['id','owner', 'parent', 'root', 'name']]); + $state = $this->primeExpectations($this->data, ['arsse_folders' => ['id','owner', 'parent', 'name']]); array_pop($state['arsse_folders']['rows']); $this->compareExpectations($state); } function testRemoveAFolderTree() { $this->assertTrue(Data::$db->folderRemove("john.doe@example.com", 1)); - $state = $this->primeExpectations($this->data, ['arsse_folders' => ['id','owner', 'parent', 'root', 'name']]); + $state = $this->primeExpectations($this->data, ['arsse_folders' => ['id','owner', 'parent', 'name']]); foreach([0,1,2,5] as $index) { unset($state['arsse_folders']['rows'][$index]); } @@ -223,16 +222,15 @@ trait SeriesFolder { 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"; + $state = $this->primeExpectations($this->data, ['arsse_folders' => ['id','owner', 'parent', 'name']]); + $state['arsse_folders']['rows'][5][3] = "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 = $this->primeExpectations($this->data, ['arsse_folders' => ['id','owner', 'parent', '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); } diff --git a/tests/lib/Database/Setup.php b/tests/lib/Database/Setup.php index 472b803c..a40bdab7 100644 --- a/tests/lib/Database/Setup.php +++ b/tests/lib/Database/Setup.php @@ -69,12 +69,18 @@ trait Setup { function compareExpectations(array $expected): bool { foreach($expected as $table => $info) { $cols = implode(",", array_keys($info['columns'])); - foreach($this->drv->prepare("SELECT $cols from $table")->run() as $num => $row) { - $row = array_values($row); - $this->assertGreaterThan(0, sizeof($info['rows']), "Expectations contain fewer rows than the database table $table"); - $exp = array_shift($info['rows']); - $this->assertSame($exp, $row, "Row ".($num+1)." of table $table does not match expectations at array index $num."); + $data = $this->drv->prepare("SELECT $cols from $table")->run()->getAll(); + $cols = array_keys($info['columns']); + foreach($info['rows'] as $index => $values) { + $row = []; + foreach($values as $key => $value) { + $row[$cols[$key]] = $value; + } + $found = array_search($row, $data); + $this->assertNotSame(false, $found, "Table $table does not contain record at array index $index."); + unset($data[$found]); } + $this->assertSame([], $data); } return true; }