Browse Source

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.
microsub
J. King 7 years ago
parent
commit
42a5ccb96c
  1. 66
      lib/Database.php
  2. 10
      lib/Db/AbstractResult.php
  3. 46
      lib/Db/ResultAggregate.php
  4. 1
      lib/Db/SQLite3/Result.php
  5. 101
      tests/Db/TestResultAggregate.php
  6. 2
      tests/REST/TinyTinyRSS/TestTinyTinyAPI.php
  7. 9
      tests/lib/Database/SeriesArticle.php
  8. 25
      tests/lib/Result.php
  9. 1
      tests/phpunit.xml

66
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,11 +918,45 @@ 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;
// 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",
@ -939,23 +974,45 @@ class Database {
// 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 {
if (!Arsse::$user->authorize($user, __FUNCTION__)) {
throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
}
$context = $context ?? new Context;
// 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 {
if (!Arsse::$user->authorize($user, __FUNCTION__)) {
throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]);
}
$context = $context ?? new Context;
// 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);
}
$tr->commit();
return $out;
} else {
// sanitize input
$values = [
isset($data['read']) ? $data['read'] : null,
@ -1013,6 +1070,7 @@ class Database {
$tr->commit();
return $out;
}
}
public function articleStarred(string $user): array {
if (!Arsse::$user->authorize($user, __FUNCTION__)) {

10
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 {

46
lib/Db/ResultAggregate.php

@ -0,0 +1,46 @@
<?php
declare(strict_types=1);
namespace JKingWeb\Arsse\Db;
use JKingWeb\Arsse\Db\Exception;
class ResultAggregate extends AbstractResult {
protected $data;
protected $index = 0;
protected $cur = null;
// actual public methods
public function changes() {
return array_reduce($this->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;
}
}

1
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;

101
tests/Db/TestResultAggregate.php

@ -0,0 +1,101 @@
<?php
declare(strict_types=1);
namespace JKingWeb\Arsse;
use JKingWeb\Arsse\Test\Result;
/** @covers \JKingWeb\Arsse\Db\ResultAggregate<extended> */
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());
}
}

2
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

9
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() {

25
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;
}
}

1
tests/phpunit.xml

@ -47,6 +47,7 @@
</testsuite>
<testsuite name="Database drivers">
<file>Db/TestTransaction.php</file>
<file>Db/TestResultAggregate.php</file>
<file>Db/SQLite3/TestDbResultSQLite3.php</file>
<file>Db/SQLite3/TestDbStatementSQLite3.php</file>
<file>Db/SQLite3/TestDbDriverCreationSQLite3.php</file>

Loading…
Cancel
Save