From 7a2de95c7005dfe396e3034c92d5ca1c8686552a Mon Sep 17 00:00:00 2001 From: "J. King" Date: Fri, 6 Oct 2017 20:26:22 -0400 Subject: [PATCH] Consolidate article context handling into articleQuery function Also consolidated article star counting into a generic articleCount function which accepts a context. This may lead to slight efficiency losses in either listing or marking (and more significant ones in counting starred), but the advantages of centralized context handling are significant with the future addition of labels and the need to count articles under various future contexts in TTRSS. --- lib/Database.php | 230 +++++++++++------------ lib/Misc/Query.php | 31 ++- lib/REST/NextCloudNews/V1_2.php | 2 +- tests/REST/NextCloudNews/TestNCNV1_2.php | 2 +- tests/lib/Database/SeriesArticle.php | 10 +- 5 files changed, 144 insertions(+), 131 deletions(-) diff --git a/lib/Database.php b/lib/Database.php index e50312c..9bc975e 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -301,16 +301,22 @@ class Database { throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); } // check to make sure the parent exists, if one is specified - $parent = $this->folderValidateId($user, $parent)['id']; - // if we're not returning a recursive list we can use a simpler query + $parent = $this->folderValidateId($user, $parent)['id']; + $q = new Query( + "SELECT + id,name,parent, + (select count(*) from arsse_folders as parents where parents.parent is arsse_folders.id) as children + FROM arsse_folders" + ); if (!$recursive) { - return $this->db->prepare("SELECT id,name,parent from arsse_folders where owner is ? and parent is ?", "str", "int")->run($user, $parent); + $q->setWhere("owner is ?", "str", $user); + $q->setWhere("parent is ?", "int", $parent); } else { - return $this->db->prepare( - "WITH RECURSIVE folders(id) as (SELECT id from arsse_folders where owner is ? and parent is ? union select arsse_folders.id from arsse_folders join folders on arsse_folders.parent=folders.id) ". - "SELECT id,name,parent from arsse_folders where id in (SELECT id from folders) order by name", - "str", "int")->run($user, $parent); + $q->setCTE("folders", "SELECT id from arsse_folders where owner is ? and parent is ? union select arsse_folders.id from arsse_folders join folders on arsse_folders.parent=folders.id", ["str", "int"], [$user, $parent]); + $q->setWhere("id in (SELECT id from folders)"); } + $q->setOrder("name"); + return $this->db->prepare($q->getQuery(), $q->getTypes())->run($q->getValues()); } public function folderRemove(string $user, $id): bool { @@ -794,35 +800,25 @@ class Database { )->run($feedID, $ids, $hashesUT, $hashesUC, $hashesTC); } - public function articleList(string $user, Context $context = null): Db\Result { - if (!Arsse::$user->authorize($user, __FUNCTION__)) { - throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - } - if (!$context) { - $context = new Context; + protected function articleQuery(string $user, Context $context, array $extraColumns = []): Query { + $extraColumns = implode(",", $extraColumns); + if (strlen($extraColumns)) { + $extraColumns .= ","; } $q = new Query( "SELECT + $extraColumns arsse_articles.id as id, - arsse_articles.url as url, - title,author,content,guid, - published as published_date, - edited as edited_date, + arsse_articles.feed as feed, max( - modified, + arsse_articles.modified, coalesce((select modified from arsse_marks where article is arsse_articles.id and subscription in (select sub from subscribed_feeds)),'') ) as modified_date, NOT (select count(*) from arsse_marks where article is arsse_articles.id and read is 1 and subscription in (select sub from subscribed_feeds)) as unread, (select count(*) from arsse_marks where article is arsse_articles.id and starred is 1 and subscription in (select sub from subscribed_feeds)) as starred, (select max(id) from arsse_editions where article is arsse_articles.id) as edition, - subscribed_feeds.sub as subscription, - url_title_hash||':'||url_content_hash||':'||title_content_hash as fingerprint, - arsse_enclosures.url as media_url, - arsse_enclosures.type as media_type - FROM arsse_articles - join subscribed_feeds on arsse_articles.feed is subscribed_feeds.id - left join arsse_enclosures on arsse_enclosures.article is arsse_articles.id - " + subscribed_feeds.sub as subscription + FROM arsse_articles" ); $q->setOrder("edition".($context->reverse ? " desc" : "")); $q->setLimit($context->limit, $context->offset); @@ -831,17 +827,56 @@ class Database { // if a subscription is specified, make sure it exists $id = $this->subscriptionValidateId($user, $context->subscription)['feed']; // add a basic CTE that will join in only the requested subscription - $q->setCTE("subscribed_feeds(id,sub)", "SELECT ?,?", ["int","int"], [$id,$context->subscription]); + $q->setCTE("subscribed_feeds(id,sub)", "SELECT ?,?", ["int","int"], [$id,$context->subscription], "join subscribed_feeds on feed is subscribed_feeds.id"); } elseif ($context->folder()) { // if a folder is specified, make sure it exists $this->folderValidateId($user, $context->folder); // if it does exist, add a common table expression to list it and its children so that we select from the entire subtree $q->setCTE("folders(folder)", "SELECT ? union select id from arsse_folders join folders on parent is folder", "int", $context->folder); // add another CTE for the subscriptions within the folder - $q->setCTE("subscribed_feeds(id,sub)", "SELECT feed,id from arsse_subscriptions join user on user is owner join folders on arsse_subscriptions.folder is folders.folder"); + $q->setCTE("subscribed_feeds(id,sub)", "SELECT feed,id from arsse_subscriptions join user on user is owner join folders on arsse_subscriptions.folder is folders.folder", [], [], "join subscribed_feeds on feed is subscribed_feeds.id"); } else { // otherwise add a CTE for all the user's subscriptions - $q->setCTE("subscribed_feeds(id,sub)", "SELECT feed,id from arsse_subscriptions join user on user is owner"); + $q->setCTE("subscribed_feeds(id,sub)", "SELECT feed,id from arsse_subscriptions join user on user is owner", [], [], "join subscribed_feeds on feed is subscribed_feeds.id"); + } + if ($context->edition()) { + // if an edition is specified, filter for its previously identified article + $q->setWhere("arsse_articles.id is (select article from arsse_editions where id is ?)", "int", $context->edition); + } elseif ($context->article()) { + // if an article is specified, filter for it (it has already been validated above) + $q->setWhere("arsse_articles.id is ?", "int", $context->article); + } + if ($context->editions()) { + // if multiple specific editions have been requested, prepare a CTE to list them and their articles + if (!$context->editions) { + throw new Db\ExceptionInput("tooShort", ['field' => "editions", 'action' => __FUNCTION__, 'min' => 1]); // must have at least one array element + } elseif (sizeof($context->editions) > 50) { + throw new Db\ExceptionInput("tooLong", ['field' => "editions", 'action' => __FUNCTION__, 'max' => 50]); // must not have more than 50 array elements + } + list($inParams, $inTypes) = $this->generateIn($context->editions, "int"); + $q->setCTE("requested_articles(id,edition)", + "SELECT article,id as edition from arsse_editions where edition in ($inParams)", + $inTypes, + $context->editions + ); + $q->setWhere("arsse_articles.id in (select id from requested_articles)"); + } elseif ($context->articles()) { + // if multiple specific articles have been requested, prepare a CTE to list them and their articles + if (!$context->articles) { + throw new Db\ExceptionInput("tooShort", ['field' => "articles", 'action' => __FUNCTION__, 'min' => 1]); // must have at least one array element + } elseif (sizeof($context->articles) > 50) { + throw new Db\ExceptionInput("tooLong", ['field' => "articles", 'action' => __FUNCTION__, 'max' => 50]); // must not have more than 50 array elements + } + list($inParams, $inTypes) = $this->generateIn($context->articles, "int"); + $q->setCTE("requested_articles(id,edition)", + "SELECT id,(select max(id) from arsse_editions where article is arsse_articles.id) as edition from arsse_articles where arsse_articles.id in ($inParams)", + $inTypes, + $context->articles + ); + $q->setWhere("arsse_articles.id in (select id from requested_articles)"); + } else { + // if neither list is specified, mock an empty table + $q->setCTE("requested_articles(id,edition)", "SELECT 'empty','table' where 1 is 0"); } // filter based on edition offset if ($context->oldestEdition()) { @@ -864,6 +899,29 @@ class Database { if ($context->starred()) { $q->setWhere("starred is ?", "bool", $context->starred); } + // return the query + return $q; + } + + public function articleList(string $user, Context $context = null): Db\Result { + if (!Arsse::$user->authorize($user, __FUNCTION__)) { + throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); + } + $context = $context ?? new Context; + $columns = [ + "arsse_articles.url as url", + "title", + "author", + "content", + "guid", + "published as published_date", + "edited as edited_date", + "url_title_hash||':'||url_content_hash||':'||title_content_hash as fingerprint", + "arsse_enclosures.url as media_url", + "arsse_enclosures.type as media_type", + ]; + $q = $this->articleQuery($user, $context, $columns); + $q->setJoin("left join arsse_enclosures on arsse_enclosures.article is arsse_articles.id"); // perform the query and return results return $this->db->prepare($q->getQuery(), $q->getTypes())->run($q->getValues()); } @@ -916,94 +974,15 @@ class Database { // execute each query in sequence foreach ($queries as $query) { // first build the query which will select the target articles; we will later turn this into a CTE for the actual query that manipulates the articles - $q = new Query( - "SELECT - arsse_articles.id as id, - feed, - (select max(id) from arsse_editions where article is arsse_articles.id) as edition, - max(arsse_articles.modified, - coalesce((select modified from arsse_marks where article is arsse_articles.id and subscription in (select sub from subscribed_feeds)),'') - ) as modified_date, - (not exists(select article from arsse_marks where article is arsse_articles.id and subscription in (select sub from subscribed_feeds))) as to_insert, - ((select read from target_values) is not null and (select read from target_values) is not (coalesce((select read from arsse_marks where article is arsse_articles.id and subscription in (select sub from subscribed_feeds)),0)) and (not exists(select * from requested_articles) or (select max(id) from arsse_editions where article is arsse_articles.id) in (select edition from requested_articles))) as honour_read, - ((select starred from target_values) is not null and (select starred from target_values) is not (coalesce((select starred from arsse_marks where article is arsse_articles.id and subscription in (select sub from subscribed_feeds)),0))) as honour_star - FROM arsse_articles" - ); - // common table expression for the affected user - $q->setCTE("user(user)", "SELECT ?", "str", $user); + $q = $this->articleQuery($user, $context, [ + "(not exists(select article from arsse_marks where article is arsse_articles.id and subscription in (select sub from subscribed_feeds))) as to_insert", + "((select read from target_values) is not null and (select read from target_values) is not (coalesce((select read from arsse_marks where article is arsse_articles.id and subscription in (select sub from subscribed_feeds)),0)) and (not exists(select * from requested_articles) or (select max(id) from arsse_editions where article is arsse_articles.id) in (select edition from requested_articles))) as honour_read", + "((select starred from target_values) is not null and (select starred from target_values) is not (coalesce((select starred from arsse_marks where article is arsse_articles.id and subscription in (select sub from subscribed_feeds)),0))) as honour_star", + ]); // common table expression with the values to set $q->setCTE("target_values(read,starred)", "SELECT ?,?", ["bool","bool"], $values); - if ($context->subscription()) { - // if a subscription is specified, make sure it exists - $id = $this->subscriptionValidateId($user, $context->subscription)['feed']; - // add a basic CTE that will join in only the requested subscription - $q->setCTE("subscribed_feeds(id,sub)", "SELECT ?,?", ["int","int"], [$id,$context->subscription], "join subscribed_feeds on feed is subscribed_feeds.id"); - } elseif ($context->folder()) { - // if a folder is specified, make sure it exists - $this->folderValidateId($user, $context->folder); - // if it does exist, add a common table expression to list it and its children so that we select from the entire subtree - $q->setCTE("folders(folder)", "SELECT ? union select id from arsse_folders join folders on parent is folder", "int", $context->folder); - // add another CTE for the subscriptions within the folder - $q->setCTE("subscribed_feeds(id,sub)", "SELECT feed,id from arsse_subscriptions join user on user is owner join folders on arsse_subscriptions.folder is folders.folder", [], [], "join subscribed_feeds on feed is subscribed_feeds.id"); - } else { - // otherwise add a CTE for all the user's subscriptions - $q->setCTE("subscribed_feeds(id,sub)", "SELECT feed,id from arsse_subscriptions join user on user is owner", [], [], "join subscribed_feeds on feed is subscribed_feeds.id"); - } - if ($context->edition()) { - // if an edition is specified, filter for its previously identified article - $q->setWhere("arsse_articles.id is ?", "int", $edition['article']); - } elseif ($context->article()) { - // if an article is specified, filter for it (it has already been validated above) - $q->setWhere("arsse_articles.id is ?", "int", $context->article); - } - if ($context->editions()) { - // if multiple specific editions have been requested, prepare a CTE to list them and their articles - if (!$context->editions) { - throw new Db\ExceptionInput("tooShort", ['field' => "editions", 'action' => __FUNCTION__, 'min' => 1]); // must have at least one array element - } elseif (sizeof($context->editions) > 50) { - throw new Db\ExceptionInput("tooLong", ['field' => "editions", 'action' => __FUNCTION__, 'max' => 50]); // must not have more than 50 array elements - } - list($inParams, $inTypes) = $this->generateIn($context->editions, "int"); - $q->setCTE("requested_articles(id,edition)", - "SELECT article,id as edition from arsse_editions where edition in ($inParams)", - $inTypes, - $context->editions - ); - $q->setWhere("arsse_articles.id in (select id from requested_articles)"); - } elseif ($context->articles()) { - // if multiple specific articles have been requested, prepare a CTE to list them and their articles - if (!$context->articles) { - throw new Db\ExceptionInput("tooShort", ['field' => "articles", 'action' => __FUNCTION__, 'min' => 1]); // must have at least one array element - } elseif (sizeof($context->articles) > 50) { - throw new Db\ExceptionInput("tooLong", ['field' => "articles", 'action' => __FUNCTION__, 'max' => 50]); // must not have more than 50 array elements - } - list($inParams, $inTypes) = $this->generateIn($context->articles, "int"); - $q->setCTE("requested_articles(id,edition)", - "SELECT id,(select max(id) from arsse_editions where article is arsse_articles.id) as edition from arsse_articles where arsse_articles.id in ($inParams)", - $inTypes, - $context->articles - ); - $q->setWhere("arsse_articles.id in (select id from requested_articles)"); - } else { - // if neither list is specified, mock an empty table - $q->setCTE("requested_articles(id,edition)", "SELECT 'empty','table' where 1 is 0"); - } - // filter based on edition offset - if ($context->oldestEdition()) { - $q->setWhere("edition >= ?", "int", $context->oldestEdition); - } - if ($context->latestEdition()) { - $q->setWhere("edition <= ?", "int", $context->latestEdition); - } - // filter based on lastmod time - if ($context->modifiedSince()) { - $q->setWhere("modified_date >= ?", "datetime", $context->modifiedSince); - } - if ($context->notModifiedSince()) { - $q->setWhere("modified_date <= ?", "datetime", $context->notModifiedSince); - } // push the current query onto the CTE stack and execute the query we're actually interested in - $q->pushCTE("target_articles(id,feed,edition,modified_date,to_insert,honour_read,honour_star)"); + $q->pushCTE("target_articles"); $q->setBody($query); $out += $this->db->prepare($q->getQuery(), $q->getTypes())->run($q->getValues())->changes(); } @@ -1012,11 +991,15 @@ class Database { return (bool) $out; } - public function articleStarredCount(string $user): int { + public function articleCount(string $user, Context $context = null): int { if (!Arsse::$user->authorize($user, __FUNCTION__)) { throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); } - return $this->db->prepare("SELECT count(*) from arsse_marks where starred is 1 and subscription in (select id from arsse_subscriptions where owner is ?)", "str")->run($user)->getValue(); + $context = $context ?? new Context; + $q = $this->articleQuery($user, $context); + $q->pushCTE("selected_articles"); + $q->setBody("SELECT count(*) from selected_articles"); + return $this->db->prepare($q->getQuery(), $q->getTypes())->run($q->getValues())->getValue(); } public function articleCleanup(): bool { @@ -1105,9 +1088,7 @@ class Database { if (!Arsse::$user->authorize($user, __FUNCTION__)) { throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); } - if (!$context) { - $context = new Context; - } + $context = $context ?? new Context; $q = new Query("SELECT max(arsse_editions.id) from arsse_editions left join arsse_articles on article is arsse_articles.id left join arsse_feeds on arsse_articles.feed is arsse_feeds.id"); if ($context->subscription()) { // if a subscription is specified, make sure it exists @@ -1141,10 +1122,15 @@ class Database { return $this->db->prepare( "SELECT id,name, - (select count(*) from arsse_label_members where owner is ? and label is arsse_labels.id) as articles + (select count(*) from arsse_label_members join arsse_subscriptions on arsse_subscriptions.owner is arsse_labels.owner where label is arsse_labels.id) as articles, + (select count(*) from arsse_label_members + join arsse_marks on arsse_label_members.article is arsse_marks.article and arsse_label_members.subscription is arsse_marks.subscription + join arsse_subscriptions on arsse_subscriptions.owner is arsse_labels.owner + where label is arsse_labels.id and read is 1 + ) as read FROM arsse_labels where owner is ? and articles >= ? - ", "str", "str", "int" - )->run($user, $user, !$includeEmpty); + ", "str", "int" + )->run($user, !$includeEmpty); } public function labelRemove(string $user, $id, bool $byName = false): bool { diff --git a/lib/Misc/Query.php b/lib/Misc/Query.php index b480d3b..24445d5 100644 --- a/lib/Misc/Query.php +++ b/lib/Misc/Query.php @@ -10,6 +10,9 @@ class Query { protected $tCTE = []; // Common table expression type bindings protected $vCTE = []; // Common table expression binding values protected $jCTE = []; // Common Table Expression joins + protected $qJoin = []; // JOIN clause components + protected $tJoin = []; // JOIN clause type bindings + protected $vJoin = []; // JOIN clause binding values protected $qWhere = []; // WHERE clause components protected $tWhere = []; // WHERE clause type bindings protected $vWhere = []; // WHERE clause binding values @@ -43,6 +46,15 @@ class Query { return true; } + public function setJoin(string $join, $types = null, $values = null): bool { + $this->qJoin[] = $join; + if (!is_null($types)) { + $this->tJoin[] = $types; + $this->vJoin[] = $values; + } + return true; + } + public function setWhere(string $where, $types = null, $values = null): bool { $this->qWhere[] = $where; if (!is_null($types)) { @@ -77,6 +89,9 @@ class Query { $this->qWhere = []; $this->tWhere = []; $this->vWhere = []; + $this->qJoin = []; + $this->tJoin = []; + $this->vJoin = []; $this->order = []; $this->setLimit(0, 0); if (strlen($join)) { @@ -101,11 +116,19 @@ class Query { } public function getTypes(): array { - return [$this->tCTE, $this->tBody, $this->tWhere]; + return [$this->tCTE, $this->tBody, $this->tJoin, $this->tWhere]; } public function getValues(): array { - return [$this->vCTE, $this->vBody, $this->vWhere]; + return [$this->vCTE, $this->vBody, $this->vJoin, $this->vWhere]; + } + + public function getJoinTypes(): array { + return $this->tJoin; + } + + public function getJoinValues(): array { + return $this->vJoin; } public function getWhereTypes(): array { @@ -132,6 +155,10 @@ class Query { // add any joins against CTEs $out .= " ".implode(" ", $this->jCTE); } + // add any JOINs + if (sizeof($this->qJoin)) { + $out .= " ".implode(" ", $this->qJoin); + } // add any WHERE terms if (sizeof($this->qWhere)) { $out .= " WHERE ".implode(" AND ", $this->qWhere); diff --git a/lib/REST/NextCloudNews/V1_2.php b/lib/REST/NextCloudNews/V1_2.php index bab902e..09f8546 100644 --- a/lib/REST/NextCloudNews/V1_2.php +++ b/lib/REST/NextCloudNews/V1_2.php @@ -395,7 +395,7 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { $out[] = $this->feedTranslate($sub); } $out = ['feeds' => $out]; - $out['starredCount'] = Arsse::$db->articleStarredCount(Arsse::$user->id); + $out['starredCount'] = Arsse::$db->articleCount(Arsse::$user->id, (new Context)->starred(true)); $newest = Arsse::$db->editionLatest(Arsse::$user->id); if ($newest) { $out['newestItemId'] = $newest; diff --git a/tests/REST/NextCloudNews/TestNCNV1_2.php b/tests/REST/NextCloudNews/TestNCNV1_2.php index 0d8e08e..d3fe141 100644 --- a/tests/REST/NextCloudNews/TestNCNV1_2.php +++ b/tests/REST/NextCloudNews/TestNCNV1_2.php @@ -475,7 +475,7 @@ class TestNCNV1_2 extends Test\AbstractTest { 'newestItemId' => 4758915, ]; Phake::when(Arsse::$db)->subscriptionList(Arsse::$user->id)->thenReturn(new Result([]))->thenReturn(new Result($this->feeds['db'])); - Phake::when(Arsse::$db)->articleStarredCount(Arsse::$user->id)->thenReturn(0)->thenReturn(5); + Phake::when(Arsse::$db)->articleCount(Arsse::$user->id, (new Context)->starred(true))->thenReturn(0)->thenReturn(5); Phake::when(Arsse::$db)->editionLatest(Arsse::$user->id)->thenReturn(0)->thenReturn(4758915); $exp = new Response(200, $exp1); $this->assertEquals($exp, $this->h->dispatch(new Request("GET", "/feeds"))); diff --git a/tests/lib/Database/SeriesArticle.php b/tests/lib/Database/SeriesArticle.php index 0487661..3f363fb 100644 --- a/tests/lib/Database/SeriesArticle.php +++ b/tests/lib/Database/SeriesArticle.php @@ -731,16 +731,16 @@ trait SeriesArticle { } public function testCountStarredArticles() { - $this->assertSame(2, Arsse::$db->articleStarredCount("john.doe@example.com")); - $this->assertSame(2, Arsse::$db->articleStarredCount("john.doe@example.org")); - $this->assertSame(2, Arsse::$db->articleStarredCount("john.doe@example.net")); - $this->assertSame(0, Arsse::$db->articleStarredCount("jane.doe@example.com")); + $this->assertSame(2, Arsse::$db->articleCount("john.doe@example.com", (new Context)->starred(true))); + $this->assertSame(2, Arsse::$db->articleCount("john.doe@example.org", (new Context)->starred(true))); + $this->assertSame(2, Arsse::$db->articleCount("john.doe@example.net", (new Context)->starred(true))); + $this->assertSame(0, Arsse::$db->articleCount("jane.doe@example.com", (new Context)->starred(true))); } public function testCountStarredArticlesWithoutAuthority() { Phake::when(Arsse::$user)->authorize->thenReturn(false); $this->assertException("notAuthorized", "User", "ExceptionAuthz"); - Arsse::$db->articleStarredCount($this->user); + Arsse::$db->articleCount($this->user, (new Context)->starred(true)); } public function testFetchLatestEdition() {