From 483874e21d0652cfde874dc390b28679fe287bce Mon Sep 17 00:00:00 2001 From: "J. King" Date: Sun, 18 Jun 2017 10:23:37 -0400 Subject: [PATCH] Implemented query contexts - Fixes #55 - Included test for Context - Adjusted Database::editionLatest() to use Context - Adjusted NCN handler and tests accordingly - Also refined experimental Database::articleList() method and added experimental Database::articlePropertiesSet() method --- lib/Database.php | 199 ++++++++++++++++++----- lib/Db/SQLite3/Driver.php | 2 +- lib/Misc/Context.php | 87 ++++++++++ lib/{Database => Misc}/Query.php | 70 +++++--- lib/REST/NextCloudNews/V1_2.php | 3 +- tests/Misc/TestContext.php | 51 ++++++ tests/REST/NextCloudNews/TestNCNV1_2.php | 5 +- tests/phpunit.xml | 3 + 8 files changed, 356 insertions(+), 64 deletions(-) create mode 100644 lib/Misc/Context.php rename lib/{Database => Misc}/Query.php (69%) create mode 100644 tests/Misc/TestContext.php diff --git a/lib/Database.php b/lib/Database.php index 5935a1f..13587f6 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -2,7 +2,8 @@ declare(strict_types=1); namespace JKingWeb\Arsse; use PasswordGenerator\Generator as PassGen; -use JKingWeb\Arsse\Database\Query; +use JKingWeb\Arsse\Misc\Query; +use JKingWeb\Arsse\Misc\Context; class Database { @@ -86,20 +87,7 @@ class Database { } public function settingGet(string $key) { - $row = $this->db->prepare("SELECT value, type from arsse_settings where key = ?", "str")->run($key)->getRow(); - if(!$row) return null; - switch($row['type']) { - case "int": return (int) $row['value']; - case "numeric": return (float) $row['value']; - case "text": return $row['value']; - case "json": return json_decode($row['value']); - case "timestamp": return date_create_from_format("!".self::FORMAT_TS, $row['value'], new DateTimeZone("UTC")); - case "date": return date_create_from_format("!".self::FORMAT_DATE, $row['value'], new DateTimeZone("UTC")); - case "time": return date_create_from_format("!".self::FORMAT_TIME, $row['value'], new DateTimeZone("UTC")); - case "bool": return (bool) $row['value']; - case "null": return null; - default: return $row['value']; - } + return $this->db->prepare("SELECT value from arsse_settings where key is ?", "str")->run($key)->getValue(); } public function begin(): Db\Transaction { @@ -590,30 +578,46 @@ class Database { return $this->db->prepare("SELECT count(*) from arsse_marks where owner is ? and starred is 1", "str")->run($user)->getValue(); } - public function editionLatest(string $user, array $context = []): int { + public function editionLatest(string $user, Context $context = null): int { if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - if(array_key_exists("subscription", $context)) { - $id = $context['subscription']; - $sub = $this->subscriptionValidateId($user, $id); - return (int) $this->db->prepare( - "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 - where arsse_feeds.id is ?", - "int" - )->run($sub['feed'])->getValue(); - } - return (int) $this->db->prepare("SELECT max(id) from arsse_editions")->run()->getValue(); // FIXME: this is incorrect; it's not restricted to the user's subscriptions - } - - public function articleList(string $user): Db\Result { + if(!$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 + $id = $this->subscriptionValidateId($user, $context->subscription)['feed']; + // a simple WHERE clause is required here + $q->setWhere("arsse_feeds.id is ?", "int", $id); + } else { + $q->setCTE("user(user) as (SELECT ?)", "str", $user); + if($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) as (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( + "feeds(feed) as (SELECT feed from arsse_subscriptions join user on user is owner join folders on arsse_subscription.folder is folders.folder)", + [], // binding types + [], // binding values + "join feeds on arsse_articles.feed is feeds.feed" // join expression + ); + } else { + // if no folder is specified, a single CTE is added + $q->setCTE( + "feeds(feed) as (SELECT feed from arsse_subscriptions join user on user is owner)", + [], // binding types + [], // binding values + "join feeds on arsse_articles.feed is feeds.feed" // join expression + ); + } + } + return (int) $this->db->prepare($q)->run()->getValue(); + } + + public function articleList(string $user, Context $context = null): Db\Result { if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); - return $this->db->prepare( - "WITH - user(user) as (SELECT ?), - subscribed_feeds(id) as (SELECT feed from arsse_subscriptions join user on user is owner) - ". + if(!$context) $context = new Context; + $q = new Query( "SELECT arsse_articles.id, arsse_articles.url, @@ -634,7 +638,124 @@ class Database { join subscribed_feeds on arsse_articles.feed is subscribed_feeds.id left join arsse_enclosures on arsse_enclosures.article is arsse_articles.id ", - "str","str","str" - )-run($user, $this->dateFormatDefault, $this->dateFormatDefault, $this->dateFormatDefault); + "", // WHERE clause + "latestEdition".(!$context->reverse ? " desc" : ""), // ORDER BY clause + $context->limit, + $context->offset + ); + $q->setCTE("user(user) as (SELECT ?)", "str", $user); + 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) as (SELECT ?)", "int", $id); + } else if($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) as (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) as (SELECT feed from arsse_subscriptions join user on user is owner join folders on arsse_subscription.folder is folders.folder)"); + } else { + // otherwise add a CTE for all the user's subscriptions + $q->setCTE("subscribed_feeds(id) as (SELECT feed from arsse_subscriptions join user on user is owner)"); + } + // filter based on edition offset + if($context->oldestEdition()) { + $q->setWhere("latestEdition >= ?", "int", $context->oldestEdition); + } else if($context->latestEdition()) { + $q->setWhere("latestEdition <= ?", "int", $context->oldestEdition); + } + // filter based on lastmod time + if($context->modifiedSince()) $q->setWhere("modified >= ?", "datetime", $context->modifiedSince); + // filter for un/read and un/starred status if specified + if($context->unread()) $q->setWhere("unread is ?", "bool", $context->unread); + if($context->starred()) $q->setWhere("starred is ?", "bool", $context->starred); + // perform the query and return results + return $this->db->prepare($q, "str", "str", "str")-run($this->dateFormatDefault, $this->dateFormatDefault, $this->dateFormatDefault); + } + + public function articlePropertiesSet(string $user, array $data, Context $context = null): bool { + if(!Data::$user->authorize($user, __FUNCTION__)) throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); + if(!$context) $context = new Context; + // sanitize input + $valid = [ + 'read' => "strict bool", + 'starred' => "strict bool", + ]; + list($setClause, $setTypes, $setValues) = $this->generateSet($data, $valid); + $insValues = [ + isset($data['read']) ? $data['read'] : false, + isset($data['starred']) ? $data['starred'] : false, + ]; + // the two queries we want to execute to make the requested changes + $queries = [ + [ + 'body' => "UPDATE arsse_marks set $setClause, modified = CURRENT_TIMESTAMP", + 'where' => "owner is ? and article in (select id from target_articles and exists is 1)", + 'types' => [$setTypes, "str"], + 'values' => [$setValues, $user] + ], + [ + 'body' => "INSERT INTO arsse_marks(article,owner,read,starred) SELECT id, ?, ?, ? from target_articles", + 'where' => "exists is 0", + 'types' => ["str", "strict bool", "strict bool"], + 'values' => [$user, $insValues] + ] + ]; + $out = 0; + // wrap this UPDATE and INSERT together into a transaction + $tr = $this->begin(); + // 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, + max(arsse_editions.id) as latestEdition + (select count(*) from arsse_marks join user on user is owner where article is arsse_articles.id) as exists + FROM arsse_articles + join subscribed_feeds on feed is subscribed_feeds.id + join arsse_editions on arsse_articles.id is arsse_editions.article + " + ); + $q->setCTE("user(user) as (SELECT ?)", "str", $user); + 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) as (SELECT ?)", "int", $id); + } else if($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) as (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) as (SELECT feed from arsse_subscriptions join user on user is owner join folders on arsse_subscription.folder is folders.folder)"); + } else { + // otherwise add a CTE for all the user's subscriptions + $q->setCTE("subscribed_feeds(id) as (SELECT feed from arsse_subscriptions join user on user is owner)"); + } + // filter for specific article or edition + if($context->article()) $q->setWhere("arsse_article.id is ?", "int", $context->article); + if($context->edition()) $q->setWhere("arsse_article.id is (SELECT article from arsse_editions where id is ?)", "int", $context->edition); + // filter for un/read and un/starred status if specified + if($context->unread()) $q->setWhere("unread is ?", "bool", $context->unread); + if($context->starred()) $q->setWhere("starred is ?", "bool", $context->starred); + // filter based on lastmod time + if($context->modifiedSince()) $q->setWhere("modified >= ?", "datetime", $context->modifiedSince); + // push the current query onto the CTE stack and execute the query we're actually interested in + $q->pushCTE( + "target_articles(id, exists)", // CTE table specification + [], // CTE types + [], // CTE values + $query['body'], // new query body + $query['where'] // new query WHERE clause + ); + $out += $this->db->prepare($q, $query['types'])->run($query['values'])->changes(); + } + // commit the transaction + $tr->commit(); + return (bool) $out; } } \ No newline at end of file diff --git a/lib/Db/SQLite3/Driver.php b/lib/Db/SQLite3/Driver.php index 7125531..d1583bd 100644 --- a/lib/Db/SQLite3/Driver.php +++ b/lib/Db/SQLite3/Driver.php @@ -126,7 +126,7 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver { } public function prepareArray($query, array $paramTypes): \JKingWeb\Arsse\Db\Statement { - if($query instanceof \JKingWeb\Arsse\Database\Query) { + if($query instanceof \JKingWeb\Arsse\Misc\Query) { $preValues = $query->getCTEValues(); $postValues = $query->getWhereValues(); $paramTypes = [$query->getCTETypes(), $paramTypes, $query->getWhereTypes()]; diff --git a/lib/Misc/Context.php b/lib/Misc/Context.php new file mode 100644 index 0000000..babf822 --- /dev/null +++ b/lib/Misc/Context.php @@ -0,0 +1,87 @@ +props[$prop] = true; + $this->$prop = $value; + return $this; + } else { + return isset($this->props[$prop]); + } + } + + function reverse(bool $spec = null) { + return $this->act(__FUNCTION__, func_num_args(), $spec); + } + + function limit(int $spec = null) { + return $this->act(__FUNCTION__, func_num_args(), $spec); + } + + function offset(int $spec = null) { + return $this->act(__FUNCTION__, func_num_args(), $spec); + } + + function folder(int $spec = null) { + return $this->act(__FUNCTION__, func_num_args(), $spec); + } + + function subscription(int $spec = null) { + return $this->act(__FUNCTION__, func_num_args(), $spec); + } + + function latestEdition(int $spec = null) { + return $this->act(__FUNCTION__, func_num_args(), $spec); + } + + function oldestEdition(int $spec = null) { + return $this->act(__FUNCTION__, func_num_args(), $spec); + } + + function unread(bool $spec = null) { + return $this->act(__FUNCTION__, func_num_args(), $spec); + } + + function starred(bool $spec = null) { + return $this->act(__FUNCTION__, func_num_args(), $spec); + } + + function modifiedSince($spec = null) { + $spec = $this->dateNormalize($spec); + return $this->act(__FUNCTION__, func_num_args(), $spec); + } + + function notModifiedSince($spec = null) { + $spec = $this->dateNormalize($spec); + return $this->act(__FUNCTION__, func_num_args(), $spec); + } + + function edition(int $spec = null) { + return $this->act(__FUNCTION__, func_num_args(), $spec); + } + + function article(int $spec = null) { + return $this->act(__FUNCTION__, func_num_args(), $spec); + } +} \ No newline at end of file diff --git a/lib/Database/Query.php b/lib/Misc/Query.php similarity index 69% rename from lib/Database/Query.php rename to lib/Misc/Query.php index 92faa98..7f8bb0c 100644 --- a/lib/Database/Query.php +++ b/lib/Misc/Query.php @@ -1,6 +1,6 @@ qCTE); + function getQuery(): string { $out = ""; - if($cte) { + if(sizeof($this->qCTE)) { // start with common table expressions $out .= "WITH RECURSIVE ".implode(", ", $this->qCTE)." "; } // add the body + $out .= $this->buildQueryBody(); + return $out; + } + + function pushCTE(string $tableSpec, $types, $values, string $body, string $where = "", string $order = "", int $limit = 0, int $offset = 0): bool { + // this function takes the query body and converts it to a common table expression, putting it at the bottom of the existing CTE stack + // all WHERE and ORDER BY parts belong to the new CTE and are removed from the main query + $b = $this->buildQueryBody(); + array_push($types, $this->getWhereTypes()); + array_push($values, $this->getWhereValues()); + if($this->limit) { + array_push($types, "strict int"); + array_push($values, $this->limit); + } + if($this->offset) { + array_push($types, "strict int"); + array_push($values, $this->offset); + } + $this->setCTE($tableSpec." as (".$this->buildQueryBody().")", $types, $value); + $this->tWhere = []; + $this->vWhere = []; + $this->order = []; + $this->__construct($body, $where, $order, $limit, $offset); + return true; + } + + function getWhereTypes(): array { + return $this->tWhere; + } + + function getWhereValues(): array { + return $this->vWhere; + } + + function getCTETypes(): array { + return $this->tCTE; + } + + function getCTEValues(): array { + return $this->vCTE; + } + + protected function buildQueryBody(): string { + $out = ""; + // add the body $out .= $this->body; - if($cte) { + if(sizeof($this->qCTE)) { // add any joins against CTEs $out .= " ".implode(" ", $this->jCTE); } @@ -85,20 +129,4 @@ class Query { } return $out; } - - function getWhereTypes(): array { - return $this->tWhere; - } - - function getWhereValues(): array { - return $this->vWhere; - } - - function getCTETypes(): array { - return $this->tCTE; - } - - function getCTEValues(): array { - return $this->vCTE; - } } \ No newline at end of file diff --git a/lib/REST/NextCloudNews/V1_2.php b/lib/REST/NextCloudNews/V1_2.php index ccff17f..f230b54 100644 --- a/lib/REST/NextCloudNews/V1_2.php +++ b/lib/REST/NextCloudNews/V1_2.php @@ -3,6 +3,7 @@ declare(strict_types=1); namespace JKingWeb\Arsse\REST\NextCloudNews; use JKingWeb\Arsse\Data; use JKingWeb\Arsse\User; +use JKingWeb\Arsse\Misc\Context; use JKingWeb\Arsse\AbstractException; use JKingWeb\Arsse\Db\ExceptionInput; use JKingWeb\Arsse\Feed\Exception as FeedException; @@ -281,7 +282,7 @@ class V1_2 extends \JKingWeb\Arsse\REST\AbstractHandler { $feed = Data::$db->subscriptionPropertiesGet(Data::$user->id, $id); $feed = $this->feedTranslate($feed); $out = ['feeds' => [$feed]]; - $newest = Data::$db->editionLatest(Data::$user->id, ['subscription' => $id]); + $newest = Data::$db->editionLatest(Data::$user->id, (new Context)->subscription($id)); if($newest) $out['newestItemId'] = $newest; return new Response(200, $out); } diff --git a/tests/Misc/TestContext.php b/tests/Misc/TestContext.php new file mode 100644 index 0000000..5fd09ae --- /dev/null +++ b/tests/Misc/TestContext.php @@ -0,0 +1,51 @@ +getMethods(\ReflectionMethod::IS_PUBLIC) as $m) { + if($m->isConstructor() || $m->isStatic()) continue; + $method = $m->name; + $this->assertFalse($c->$method(), "Context method $method did not initially return false"); + $this->assertEquals(null, $c->$method, "Context property $method is not initially falsy"); + } + } + + function testSetContextOptions() { + $v = [ + 'reverse' => true, + 'limit' => 10, + 'offset' => 5, + 'folder' => 42, + 'subscription' => 2112, + 'article' => 255, + 'edition' => 65535, + 'latestEdition' => 47, + 'oldestEdition' => 1337, + 'unread' => true, + 'starred' => true, + 'modifiedSince' => new \DateTime(), + 'notModifiedSince' => new \DateTime(), + ]; + $times = ['modifiedSince','notModifiedSince']; + $c = new Context; + foreach((new \ReflectionObject($c))->getMethods(\ReflectionMethod::IS_PUBLIC) as $m) { + if($m->isConstructor() || $m->isStatic()) continue; + $method = $m->name; + $this->assertArrayHasKey($method, $v, "Context method $method not included in test"); + $this->assertInstanceOf(Context::class, $c->$method($v[$method])); + $this->assertTrue($c->$method()); + if(in_array($method, $times)) { + $this->assertTime($c->$method, $v[$method]); + } else { + $this->assertSame($c->$method, $v[$method]); + } + } + } +} \ No newline at end of file diff --git a/tests/REST/NextCloudNews/TestNCNV1_2.php b/tests/REST/NextCloudNews/TestNCNV1_2.php index 7e85a4a..ff427e3 100644 --- a/tests/REST/NextCloudNews/TestNCNV1_2.php +++ b/tests/REST/NextCloudNews/TestNCNV1_2.php @@ -4,6 +4,7 @@ namespace JKingWeb\Arsse; use JKingWeb\Arsse\REST\Request; use JKingWeb\Arsse\REST\Response; use JKingWeb\Arsse\Test\Result; +use JKingWeb\Arsse\Misc\Context; use Phake; @@ -275,8 +276,8 @@ class TestNCNV1_2 extends \PHPUnit\Framework\TestCase { Phake::when(Data::$db)->subscriptionAdd(Data::$user->id, "http://example.org/news.atom")->thenReturn( 42 )->thenThrow(new \JKingWeb\Arsse\Db\ExceptionInput("constraintViolation")); // error on the second call Phake::when(Data::$db)->subscriptionPropertiesGet(Data::$user->id, 2112)->thenReturn($this->feeds['db'][0]); Phake::when(Data::$db)->subscriptionPropertiesGet(Data::$user->id, 42)->thenReturn($this->feeds['db'][1]); - Phake::when(Data::$db)->editionLatest(Data::$user->id, ['subscription' => 2112])->thenReturn(0); - Phake::when(Data::$db)->editionLatest(Data::$user->id, ['subscription' => 42])->thenReturn(4758915); + Phake::when(Data::$db)->editionLatest(Data::$user->id, (new Context)->subscription(2112))->thenReturn(0); + Phake::when(Data::$db)->editionLatest(Data::$user->id, (new Context)->subscription( 42))->thenReturn(4758915); Phake::when(Data::$db)->subscriptionPropertiesSet(Data::$user->id, 2112, ['folder' => 3])->thenThrow(new \JKingWeb\Arsse\Db\ExceptionInput("idMissing")); // folder ID 3 does not exist Phake::when(Data::$db)->subscriptionPropertiesSet(Data::$user->id, 42, ['folder' => 8])->thenReturn(true); // set up a mock for a bad feed diff --git a/tests/phpunit.xml b/tests/phpunit.xml index 7366d9b..6288679 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -32,6 +32,9 @@ Feed/TestFeedFetching.php Feed/TestFeed.php + + Misc/TestContext.php + Db/SQLite3/TestDbResultSQLite3.php Db/SQLite3/TestDbStatementSQLite3.php