From 42a5ccb96c70f749cf09c70d749acf57f5c3f09b Mon Sep 17 00:00:00 2001 From: "J. King" Date: Mon, 6 Nov 2017 23:32:29 -0500 Subject: [PATCH] Handle request splitting in data model rather than controllers Queries for multiple specific articles are limited in size because of limits on the number of bound query parameters. Currently this limit is somewhat arbitrarily set at 50, but it may increase. Historically controllers would be responsible for chunking input, but this will present problems when the expected output is a result set, and of course the maintenance burden increases as the number of controllers increases. This commit transfers the burden to the data model, and consequently introduces a ResultAggregate class which collects chunked result sets (currently only for articleList). In the course of making these changes the mock Result class was also largely rewritten, fixing many bugs with it. This commit does not modify the controllers nor their tests; this will be done in a subsequent commit. --- lib/Database.php | 214 +++++++++++++-------- lib/Db/AbstractResult.php | 10 +- lib/Db/ResultAggregate.php | 46 +++++ lib/Db/SQLite3/Result.php | 1 - tests/Db/TestResultAggregate.php | 101 ++++++++++ tests/REST/TinyTinyRSS/TestTinyTinyAPI.php | 2 +- tests/lib/Database/SeriesArticle.php | 9 +- tests/lib/Result.php | 25 +-- tests/phpunit.xml | 1 + 9 files changed, 310 insertions(+), 99 deletions(-) create mode 100644 lib/Db/ResultAggregate.php create mode 100644 tests/Db/TestResultAggregate.php diff --git a/lib/Database.php b/lib/Database.php index f661e142..ceff7ff4 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -11,6 +11,7 @@ use JKingWeb\Arsse\Misc\ValueInfo; class Database { const SCHEMA_VERSION = 2; + const LIMIT_ARTICLES = 50; /** @var Db\Driver */ public $db; @@ -855,8 +856,8 @@ class Database { // 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 + } elseif (sizeof($context->editions) > self::LIMIT_ARTICLES) { + throw new Db\ExceptionInput("tooLong", ['field' => "editions", 'action' => __FUNCTION__, 'max' => self::LIMIT_ARTICLES]); // @codeCoverageIgnore } list($inParams, $inTypes) = $this->generateIn($context->editions, "int"); $q->setCTE("requested_articles(id,edition)", @@ -869,8 +870,8 @@ class Database { // 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 + } elseif (sizeof($context->articles) > self::LIMIT_ARTICLES) { + throw new Db\ExceptionInput("tooLong", ['field' => "articles", 'action' => __FUNCTION__, 'max' => self::LIMIT_ARTICLES]); // @codeCoverageIgnore } list($inParams, $inTypes) = $this->generateIn($context->articles, "int"); $q->setCTE("requested_articles(id,edition)", @@ -917,27 +918,62 @@ class Database { return $q; } + protected function articleChunk(Context $context): array { + $exception = ""; + if ($context->editions()) { + // editions take precedence over articles + if (sizeof($context->editions) > self::LIMIT_ARTICLES) { + $exception = "editions"; + } + } elseif ($context->articles()) { + if (sizeof($context->articles) > self::LIMIT_ARTICLES) { + $exception = "articles"; + } + } + if ($exception) { + $out = []; + $list = array_chunk($context->$exception, self::LIMIT_ARTICLES); + foreach ($list as $chunk) { + $out[] = (clone $context)->$exception($chunk); + } + return $out; + } else { + return []; + } + } + 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()); + // if the context has more articles or editions than we can process in one query, perform a series of queries and return an aggregate result + if ($contexts = $this->articleChunk($context)) { + $out = []; + $tr = $this->begin(); + foreach ($contexts as $context) { + $out[] = $this->articleList($user, $context); + } + $tr->commit(); + return new Db\ResultAggregate(...$out); + } else { + $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()); + } } public function articleCount(string $user, Context $context = null): int { @@ -945,10 +981,21 @@ class Database { throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); } $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(); + // if the context has more articles or editions than we can process in one query, perform a series of queries and return an aggregate result + if ($contexts = $this->articleChunk($context)) { + $out = 0; + $tr = $this->begin(); + foreach ($contexts as $context) { + $out += $this->articleCount($user, $context); + } + $tr->commit(); + return $out; + } else { + $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 articleMark(string $user, array $data, Context $context = null): int { @@ -956,62 +1003,73 @@ class Database { throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); } $context = $context ?? new Context; - // sanitize input - $values = [ - isset($data['read']) ? $data['read'] : null, - isset($data['starred']) ? $data['starred'] : null, - ]; - // the two queries we want to execute to make the requested changes - $queries = [ - "UPDATE arsse_marks - set - read = case when (select honour_read from target_articles where target_articles.id is article) is 1 then (select read from target_values) else read end, - starred = coalesce((select starred from target_values),starred), - modified = CURRENT_TIMESTAMP - WHERE - subscription in (select sub from subscribed_feeds) - and article in (select id from target_articles where to_insert is 0 and (honour_read is 1 or honour_star is 1))", - "INSERT INTO arsse_marks(subscription,article,read,starred) - select - (select id from arsse_subscriptions join user on user is owner where arsse_subscriptions.feed is target_articles.feed), - id, - coalesce((select read from target_values) * honour_read,0), - coalesce((select starred from target_values),0) - from target_articles where to_insert is 1 and (honour_read is 1 or honour_star is 1)" - ]; - $out = 0; - // wrap this UPDATE and INSERT together into a transaction - $tr = $this->begin(); - // if an edition context is specified, make sure it's valid - if ($context->edition()) { - // make sure the edition exists - $edition = $this->articleValidateEdition($user, $context->edition); - // if the edition is not the latest, do not mark the read flag - if (!$edition['current']) { - $values[0] = null; + // if the context has more articles or editions than we can process in one query, perform a series of queries and return an aggregate result + if ($contexts = $this->articleChunk($context)) { + $out = 0; + $tr = $this->begin(); + foreach ($contexts as $context) { + $out += $this->articleMark($user, $data, $context); } - } elseif ($context->article()) { - // otherwise if an article context is specified, make sure it's valid - $this->articleValidateId($user, $context->article); + $tr->commit(); + return $out; + } else { + // sanitize input + $values = [ + isset($data['read']) ? $data['read'] : null, + isset($data['starred']) ? $data['starred'] : null, + ]; + // the two queries we want to execute to make the requested changes + $queries = [ + "UPDATE arsse_marks + set + read = case when (select honour_read from target_articles where target_articles.id is article) is 1 then (select read from target_values) else read end, + starred = coalesce((select starred from target_values),starred), + modified = CURRENT_TIMESTAMP + WHERE + subscription in (select sub from subscribed_feeds) + and article in (select id from target_articles where to_insert is 0 and (honour_read is 1 or honour_star is 1))", + "INSERT INTO arsse_marks(subscription,article,read,starred) + select + (select id from arsse_subscriptions join user on user is owner where arsse_subscriptions.feed is target_articles.feed), + id, + coalesce((select read from target_values) * honour_read,0), + coalesce((select starred from target_values),0) + from target_articles where to_insert is 1 and (honour_read is 1 or honour_star is 1)" + ]; + $out = 0; + // wrap this UPDATE and INSERT together into a transaction + $tr = $this->begin(); + // if an edition context is specified, make sure it's valid + if ($context->edition()) { + // make sure the edition exists + $edition = $this->articleValidateEdition($user, $context->edition); + // if the edition is not the latest, do not mark the read flag + if (!$edition['current']) { + $values[0] = null; + } + } elseif ($context->article()) { + // otherwise if an article context is specified, make sure it's valid + $this->articleValidateId($user, $context->article); + } + // 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 = $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); + // push the current query onto the CTE stack and execute the query we're actually interested in + $q->pushCTE("target_articles"); + $q->setBody($query); + $out += $this->db->prepare($q->getQuery(), $q->getTypes())->run($q->getValues())->changes(); + } + // commit the transaction + $tr->commit(); + return $out; } - // 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 = $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); - // push the current query onto the CTE stack and execute the query we're actually interested in - $q->pushCTE("target_articles"); - $q->setBody($query); - $out += $this->db->prepare($q->getQuery(), $q->getTypes())->run($q->getValues())->changes(); - } - // commit the transaction - $tr->commit(); - return $out; } public function articleStarred(string $user): array { diff --git a/lib/Db/AbstractResult.php b/lib/Db/AbstractResult.php index dce29262..59592f9e 100644 --- a/lib/Db/AbstractResult.php +++ b/lib/Db/AbstractResult.php @@ -9,17 +9,19 @@ abstract class AbstractResult implements Result { // actual public methods public function getValue() { - $this->next(); if ($this->valid()) { - $keys = array_keys($this->cur); - return $this->cur[array_shift($keys)]; + $out = array_shift($this->cur); + $this->next(); + return $out; } + $this->next(); return null; } public function getRow() { + $out = ($this->valid() ? $this->cur : null); $this->next(); - return ($this->valid() ? $this->cur : null); + return $out; } public function getAll(): array { diff --git a/lib/Db/ResultAggregate.php b/lib/Db/ResultAggregate.php new file mode 100644 index 00000000..2244bd91 --- /dev/null +++ b/lib/Db/ResultAggregate.php @@ -0,0 +1,46 @@ +data, function($sum, $value) {return $sum + $value->changes();}, 0); + } + + public function lastId() { + return $this->data[sizeof($this->data) - 1]->lastId(); + } + + // constructor/destructor + + public function __construct(Result ...$result) { + $this->data = $result; + } + + public function __destruct() { + $max = sizeof($this->data); + for ($a = 0; $a < $max; $a++) { + unset($this->data[$a]); + } + } + + // PHP iterator methods + + public function valid() { + while (!$this->cur && isset($this->data[$this->index])) { + $this->cur = $this->data[$this->index]->getRow(); + if (!$this->cur) { + $this->index++; + } + } + return (bool) $this->cur; + } +} diff --git a/lib/Db/SQLite3/Result.php b/lib/Db/SQLite3/Result.php index 9aed7a98..d0fdacdd 100644 --- a/lib/Db/SQLite3/Result.php +++ b/lib/Db/SQLite3/Result.php @@ -7,7 +7,6 @@ use JKingWeb\Arsse\Db\Exception; class Result extends \JKingWeb\Arsse\Db\AbstractResult { protected $st; protected $set; - protected $pos = 0; protected $cur = null; protected $rows = 0; protected $id = 0; diff --git a/tests/Db/TestResultAggregate.php b/tests/Db/TestResultAggregate.php new file mode 100644 index 00000000..433ef428 --- /dev/null +++ b/tests/Db/TestResultAggregate.php @@ -0,0 +1,101 @@ + */ +class TestResultAggregate extends Test\AbstractTest { + + public function testGetChangeCountAndLastInsertId() { + $in = [ + new Result([], 3, 4), + new Result([], 27, 10), + new Result([], 12, 2112), + ]; + $r = new Db\ResultAggregate(...$in); + $this->assertEquals(42, $r->changes()); + $this->assertEquals(2112, $r->lastId()); + } + + public function testIterateOverResults() { + $in = [ + new Result([['col' => 1]]), + new Result([['col' => 2]]), + new Result([['col' => 3]]), + ]; + $rows = []; + foreach (new Db\ResultAggregate(...$in) as $index => $row) { + $rows[$index] = $row['col']; + } + $this->assertEquals([0 => 1, 1 => 2, 2 => 3], $rows); + } + + public function testIterateOverResultsTwice() { + $in = [ + new Result([['col' => 1]]), + new Result([['col' => 2]]), + new Result([['col' => 3]]), + ]; + $rows = []; + $test = new Db\ResultAggregate(...$in); + foreach ($test as $row) { + $rows[] = $row['col']; + } + $this->assertEquals([1,2,3], $rows); + $this->assertException("resultReused", "Db"); + foreach ($test as $row) { + $rows[] = $row['col']; + } + } + + public function testGetSingleValues() { + $test = new Db\ResultAggregate(...[ + new Result([['year' => 1867]]), + new Result([['year' => 1970]]), + new Result([['year' => 2112]]), + ]); + $this->assertEquals(1867, $test->getValue()); + $this->assertEquals(1970, $test->getValue()); + $this->assertEquals(2112, $test->getValue()); + $this->assertSame(null, $test->getValue()); + } + + public function testGetFirstValuesOnly() { + $test = new Db\ResultAggregate(...[ + new Result([['year' => 1867, 'century' => 19]]), + new Result([['year' => 1970, 'century' => 20]]), + new Result([['year' => 2112, 'century' => 22]]), + ]); + $this->assertEquals(1867, $test->getValue()); + $this->assertEquals(1970, $test->getValue()); + $this->assertEquals(2112, $test->getValue()); + $this->assertSame(null, $test->getValue()); + } + + public function testGetRows() { + $test = new Db\ResultAggregate(...[ + new Result([['album' => '2112', 'track' => '2112']]), + new Result([['album' => 'Clockwork Angels', 'track' => 'The Wreckers']]), + ]); + $rows = [ + ['album' => '2112', 'track' => '2112'], + ['album' => 'Clockwork Angels', 'track' => 'The Wreckers'], + ]; + $this->assertEquals($rows[0], $test->getRow()); + $this->assertEquals($rows[1], $test->getRow()); + $this->assertSame(null, $test->getRow()); + } + + public function testGetAllRows() { + $test = new Db\ResultAggregate(...[ + new Result([['album' => '2112', 'track' => '2112']]), + new Result([['album' => 'Clockwork Angels', 'track' => 'The Wreckers']]), + ]); + $rows = [ + ['album' => '2112', 'track' => '2112'], + ['album' => 'Clockwork Angels', 'track' => 'The Wreckers'], + ]; + $this->assertEquals($rows, $test->getAll()); + } +} \ No newline at end of file diff --git a/tests/REST/TinyTinyRSS/TestTinyTinyAPI.php b/tests/REST/TinyTinyRSS/TestTinyTinyAPI.php index 1012d249..d92df3ec 100644 --- a/tests/REST/TinyTinyRSS/TestTinyTinyAPI.php +++ b/tests/REST/TinyTinyRSS/TestTinyTinyAPI.php @@ -929,7 +929,7 @@ class TestTinyTinyAPI extends Test\AbstractTest { $this->setUp(); Phake::when(Arsse::$db)->articleMark->thenReturn(42); Phake::when(Arsse::$db)->subscriptionList->thenReturn(new Result($this->subscriptions)); - Phake::when(Arsse::$db)->subscriptionList($this->anything(), null, false)->thenReturn(new Result(array_filter($this->subscriptions, function($value) {return is_null($value['folder']);}))); + Phake::when(Arsse::$db)->subscriptionList($this->anything(), null, false)->thenReturn(new Result($this->filterSubs(null))); Phake::when(Arsse::$db)->labelList->thenReturn(new Result($this->labels)); Phake::when(Arsse::$db)->labelList($this->anything(), false)->thenReturn(new Result($this->usedLabels)); // verify the complex contexts diff --git a/tests/lib/Database/SeriesArticle.php b/tests/lib/Database/SeriesArticle.php index 83c4d800..9731f4f1 100644 --- a/tests/lib/Database/SeriesArticle.php +++ b/tests/lib/Database/SeriesArticle.php @@ -2,6 +2,7 @@ declare(strict_types=1); namespace JKingWeb\Arsse\Test\Database; +use JKingWeb\Arsse\Database; use JKingWeb\Arsse\Arsse; use JKingWeb\Arsse\Misc\Context; use JKingWeb\Arsse\Misc\Date; @@ -346,6 +347,7 @@ trait SeriesArticle { // get all items for user $exp = [1,2,3,4,5,6,7,8,19,20]; $this->compareIds($exp, new Context); + $this->compareIds($exp, (new Context)->articles(range(1, Database::LIMIT_ARTICLES * 3))); // get items from a folder tree $exp = [5,6,7,8]; $this->compareIds($exp, (new Context)->folder(1)); @@ -610,8 +612,7 @@ trait SeriesArticle { } public function testMarkTooManyMultipleArticles() { - $this->assertException("tooLong", "Db", "ExceptionInput"); - Arsse::$db->articleMark($this->user, ['read'=>false,'starred'=>true], (new Context)->articles(range(1, 51))); + $this->assertSame(7, Arsse::$db->articleMark($this->user, ['read'=>false,'starred'=>true], (new Context)->articles(range(1, Database::LIMIT_ARTICLES * 3)))); } public function testMarkAMissingArticle() { @@ -676,8 +677,7 @@ trait SeriesArticle { } public function testMarkTooManyMultipleEditions() { - $this->assertException("tooLong", "Db", "ExceptionInput"); - Arsse::$db->articleMark($this->user, ['read'=>false,'starred'=>true], (new Context)->editions(range(1, 51))); + $this->assertSame(7, Arsse::$db->articleMark($this->user, ['read'=>false,'starred'=>true], (new Context)->editions(range(1, 51)))); } public function testMarkAStaleEditionUnread() { @@ -769,6 +769,7 @@ trait SeriesArticle { $this->assertSame(2, Arsse::$db->articleCount("john.doe@example.com", (new Context)->starred(true))); $this->assertSame(4, Arsse::$db->articleCount("john.doe@example.com", (new Context)->folder(1))); $this->assertSame(0, Arsse::$db->articleCount("jane.doe@example.com", (new Context)->starred(true))); + $this->assertSame(10, Arsse::$db->articleCount("john.doe@example.com", (new Context)->articles(range(1, Database::LIMIT_ARTICLES *3)))); } public function testCountArticlesWithoutAuthority() { diff --git a/tests/lib/Result.php b/tests/lib/Result.php index 3bf3316d..6c44d82b 100644 --- a/tests/lib/Result.php +++ b/tests/lib/Result.php @@ -13,21 +13,24 @@ class Result implements \JKingWeb\Arsse\Db\Result { // actual public methods public function getValue() { - $arr = $this->next(); if ($this->valid()) { - $keys = array_keys($arr); - return $arr[array_shift($keys)]; + $keys = array_keys($this->current()); + $out = $this->current()[array_shift($keys)]; + $this->next(); + return $out; } + $this->next(); return null; } public function getRow() { - $arr = $this->next(); - return ($this->valid() ? $arr : null); + $out = ($this->valid() ? $this->current() : null); + $this->next(); + return $out; } public function getAll(): array { - return $this->set; + return iterator_to_array($this, false); } public function changes() { @@ -52,22 +55,22 @@ class Result implements \JKingWeb\Arsse\Db\Result { // PHP iterator methods public function valid() { - return !is_null(key($this->set)); + return $this->pos < sizeof($this->set); } public function next() { - return next($this->set); + $this->pos++; } public function current() { - return current($this->set); + return $this->set[$this->key()]; } public function key() { - return key($this->set); + return array_keys($this->set)[$this->pos]; } public function rewind() { - reset($this->set); + $this->pos = 0; } } diff --git a/tests/phpunit.xml b/tests/phpunit.xml index f12fe403..68213eb6 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -47,6 +47,7 @@ Db/TestTransaction.php + Db/TestResultAggregate.php Db/SQLite3/TestDbResultSQLite3.php Db/SQLite3/TestDbStatementSQLite3.php Db/SQLite3/TestDbDriverCreationSQLite3.php