From d0f780d4e6e880c018d937a253419118ecfcc126 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Wed, 11 Sep 2019 15:25:26 -0400 Subject: [PATCH 1/7] Unit tests for IN() clause generator --- .../Database/{Base.php => AbstractTest.php} | 2 +- tests/cases/Database/TestDatabase.php | 58 +++++++++++++++++++ tests/cases/Db/MySQL/TestDatabase.php | 2 +- tests/cases/Db/MySQLPDO/TestDatabase.php | 2 +- tests/cases/Db/PostgreSQL/TestDatabase.php | 2 +- tests/cases/Db/PostgreSQLPDO/TestDatabase.php | 2 +- tests/cases/Db/SQLite3/TestDatabase.php | 2 +- tests/cases/Db/SQLite3PDO/TestDatabase.php | 2 +- tests/phpunit.dist.xml | 1 + 9 files changed, 66 insertions(+), 7 deletions(-) rename tests/cases/Database/{Base.php => AbstractTest.php} (98%) create mode 100644 tests/cases/Database/TestDatabase.php diff --git a/tests/cases/Database/Base.php b/tests/cases/Database/AbstractTest.php similarity index 98% rename from tests/cases/Database/Base.php rename to tests/cases/Database/AbstractTest.php index f9a92f43..1bccfde2 100644 --- a/tests/cases/Database/Base.php +++ b/tests/cases/Database/AbstractTest.php @@ -10,7 +10,7 @@ use JKingWeb\Arsse\Test\Database; use JKingWeb\Arsse\Arsse; use JKingWeb\Arsse\User; -abstract class Base extends \JKingWeb\Arsse\Test\AbstractTest { +abstract class AbstractTest extends \JKingWeb\Arsse\Test\AbstractTest { use SeriesMiscellany; use SeriesMeta; use SeriesUser; diff --git a/tests/cases/Database/TestDatabase.php b/tests/cases/Database/TestDatabase.php new file mode 100644 index 00000000..bd07e469 --- /dev/null +++ b/tests/cases/Database/TestDatabase.php @@ -0,0 +1,58 @@ +assertSame($exp, self::$db->generateIn($inV, $inT)); + } + + public function provideInClauses() { + $l = Database::LIMIT_SET_SIZE + 1; + $strings = array_fill(0, $l, ""); + $ints = range(1, $l); + $longString = str_repeat("0", Database::LIMIT_SET_STRING_LENGTH + 1); + $params = implode(",", array_fill(0, $l, "?")); + $intList = implode(",", $ints); + $stringList = implode(",", array_fill(0, $l, "''")); + return [ + ["null", [], [], "str"], + ["?", [1], [1], "int"], + ["?", ["1"], ["1"], "int"], + ["?,?", [null, null], [null, null], "str"], + ["null", [], array_fill(0, $l, null), "str"], + ["$intList", [], $ints, "int"], + ["$intList,".($l+1), [], array_merge($ints, [$l+1]), "int"], + ["$intList,0", [], array_merge($ints, ["OOK"]), "int"], + ["$intList", [], array_merge($ints, [null]), "int"], + ["$stringList,''", [], array_merge($strings, [""]), "str"], + ["$stringList", [], array_merge($strings, [null]), "str"], + ["$stringList,?", [$longString], array_merge($strings, [$longString]), "str"], + ["$stringList,'A''s'", [], array_merge($strings, ["A's"]), "str"], + ["$params", $ints, $ints, "bool"], + ]; + } +} diff --git a/tests/cases/Db/MySQL/TestDatabase.php b/tests/cases/Db/MySQL/TestDatabase.php index eaf19bb8..4364170d 100644 --- a/tests/cases/Db/MySQL/TestDatabase.php +++ b/tests/cases/Db/MySQL/TestDatabase.php @@ -12,7 +12,7 @@ namespace JKingWeb\Arsse\TestCase\Db\MySQL; * @covers \JKingWeb\Arsse\Database * @covers \JKingWeb\Arsse\Misc\Query */ -class TestDatabase extends \JKingWeb\Arsse\TestCase\Database\Base { +class TestDatabase extends \JKingWeb\Arsse\TestCase\Database\AbstractTest { use \JKingWeb\Arsse\Test\DatabaseDrivers\MySQL; protected function nextID(string $table): int { diff --git a/tests/cases/Db/MySQLPDO/TestDatabase.php b/tests/cases/Db/MySQLPDO/TestDatabase.php index 1b125662..6a7550ea 100644 --- a/tests/cases/Db/MySQLPDO/TestDatabase.php +++ b/tests/cases/Db/MySQLPDO/TestDatabase.php @@ -13,7 +13,7 @@ namespace JKingWeb\Arsse\TestCase\Db\MySQLPDO; * @covers \JKingWeb\Arsse\Database * @covers \JKingWeb\Arsse\Misc\Query */ -class TestDatabase extends \JKingWeb\Arsse\TestCase\Database\Base { +class TestDatabase extends \JKingWeb\Arsse\TestCase\Database\AbstractTest { use \JKingWeb\Arsse\Test\DatabaseDrivers\MySQLPDO; protected function nextID(string $table): int { diff --git a/tests/cases/Db/PostgreSQL/TestDatabase.php b/tests/cases/Db/PostgreSQL/TestDatabase.php index f2277afc..9fda4d97 100644 --- a/tests/cases/Db/PostgreSQL/TestDatabase.php +++ b/tests/cases/Db/PostgreSQL/TestDatabase.php @@ -12,7 +12,7 @@ namespace JKingWeb\Arsse\TestCase\Db\PostgreSQL; * @covers \JKingWeb\Arsse\Database * @covers \JKingWeb\Arsse\Misc\Query */ -class TestDatabase extends \JKingWeb\Arsse\TestCase\Database\Base { +class TestDatabase extends \JKingWeb\Arsse\TestCase\Database\AbstractTest { use \JKingWeb\Arsse\Test\DatabaseDrivers\PostgreSQL; protected function nextID(string $table): int { diff --git a/tests/cases/Db/PostgreSQLPDO/TestDatabase.php b/tests/cases/Db/PostgreSQLPDO/TestDatabase.php index 810c13c7..6f8ef2a1 100644 --- a/tests/cases/Db/PostgreSQLPDO/TestDatabase.php +++ b/tests/cases/Db/PostgreSQLPDO/TestDatabase.php @@ -13,7 +13,7 @@ namespace JKingWeb\Arsse\TestCase\Db\PostgreSQLPDO; * @covers \JKingWeb\Arsse\Database * @covers \JKingWeb\Arsse\Misc\Query */ -class TestDatabase extends \JKingWeb\Arsse\TestCase\Database\Base { +class TestDatabase extends \JKingWeb\Arsse\TestCase\Database\AbstractTest { use \JKingWeb\Arsse\Test\DatabaseDrivers\PostgreSQLPDO; protected function nextID(string $table): int { diff --git a/tests/cases/Db/SQLite3/TestDatabase.php b/tests/cases/Db/SQLite3/TestDatabase.php index 3fb11399..eab0970e 100644 --- a/tests/cases/Db/SQLite3/TestDatabase.php +++ b/tests/cases/Db/SQLite3/TestDatabase.php @@ -11,7 +11,7 @@ namespace JKingWeb\Arsse\TestCase\Db\SQLite3; * @covers \JKingWeb\Arsse\Database * @covers \JKingWeb\Arsse\Misc\Query */ -class TestDatabase extends \JKingWeb\Arsse\TestCase\Database\Base { +class TestDatabase extends \JKingWeb\Arsse\TestCase\Database\AbstractTest { use \JKingWeb\Arsse\Test\DatabaseDrivers\SQLite3; protected function nextID(string $table): int { diff --git a/tests/cases/Db/SQLite3PDO/TestDatabase.php b/tests/cases/Db/SQLite3PDO/TestDatabase.php index 504775e7..079bcc14 100644 --- a/tests/cases/Db/SQLite3PDO/TestDatabase.php +++ b/tests/cases/Db/SQLite3PDO/TestDatabase.php @@ -10,7 +10,7 @@ namespace JKingWeb\Arsse\TestCase\Db\SQLite3PDO; * @covers \JKingWeb\Arsse\Database * @covers \JKingWeb\Arsse\Misc\Query */ -class TestDatabase extends \JKingWeb\Arsse\TestCase\Database\Base { +class TestDatabase extends \JKingWeb\Arsse\TestCase\Database\AbstractTest { use \JKingWeb\Arsse\Test\DatabaseDrivers\SQLite3PDO; protected function nextID(string $table): int { diff --git a/tests/phpunit.dist.xml b/tests/phpunit.dist.xml index cae3734f..5489f948 100644 --- a/tests/phpunit.dist.xml +++ b/tests/phpunit.dist.xml @@ -98,6 +98,7 @@ cases/Db/MySQLPDO/TestUpdate.php + cases/Database/TestDatabase.php cases/Db/SQLite3/TestDatabase.php cases/Db/SQLite3PDO/TestDatabase.php cases/Db/PostgreSQL/TestDatabase.php From fb6e2babb9afefe12bab75a9d278c3fd4eaf6f6c Mon Sep 17 00:00:00 2001 From: "J. King" Date: Thu, 12 Sep 2019 08:32:40 -0400 Subject: [PATCH 2/7] Change some conditions to asserts --- RoboFile.php | 2 +- lib/Database.php | 12 +++--------- lib/Db/AbstractStatement.php | 4 +--- tests/bootstrap.php | 2 ++ 4 files changed, 7 insertions(+), 13 deletions(-) diff --git a/RoboFile.php b/RoboFile.php index 93c94443..e47c8fc7 100644 --- a/RoboFile.php +++ b/RoboFile.php @@ -126,7 +126,7 @@ class RoboFile extends \Robo\Tasks { $execpath = norm(BASE."vendor-bin/phpunit/vendor/phpunit/phpunit/phpunit"); $confpath = realpath(BASE_TEST."phpunit.dist.xml") ?: norm(BASE_TEST."phpunit.xml"); $this->taskServer(8000)->host("localhost")->dir(BASE_TEST."docroot")->rawArg("-n")->arg(BASE_TEST."server.php")->rawArg($this->blackhole())->background()->run(); - return $this->taskExec($executor)->arg($execpath)->option("-c", $confpath)->args(array_merge($set, $args))->run(); + return $this->taskExec($executor)->option("-d", "zend.assertions=1")->arg($execpath)->option("-c", $confpath)->args(array_merge($set, $args))->run(); } /** Packages a given commit of the software into a release tarball diff --git a/lib/Database.php b/lib/Database.php index 71febcc1..37524035 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -198,13 +198,11 @@ class Database { * @param boolean $matchAny Whether the search is successful when it matches any (true) or all (false) terms */ protected function generateSearch(array $terms, array $cols, bool $matchAny = false): array { - if (!$cols) { - throw new Exception("arrayEmpty", "cols"); // @codeCoverageIgnore - } $clause = []; $types = []; $values = []; $like = $this->db->sqlToken("like"); + assert(sizeof($cols) > 0, new Exception("arrayEmpty", "cols")); $embedSet = sizeof($terms) > ((int) (self::LIMIT_SET_SIZE / sizeof($cols))); foreach ($terms as $term) { $embedTerm = ($embedSet && strlen($term) <= self::LIMIT_SET_STRING_LENGTH); @@ -2081,9 +2079,7 @@ class Database { * @param boolean $byName Whether to interpret the $id parameter as the label's name (true) or identifier (false) */ public function labelArticlesSet(string $user, $id, Context $context, int $mode = self::ASSOC_ADD, bool $byName = false): int { - if (!in_array($mode, [self::ASSOC_ADD, self::ASSOC_REMOVE, self::ASSOC_REPLACE])) { - throw new Exception("constantUnknown", $mode); // @codeCoverageIgnore - } + assert(in_array($mode, [self::ASSOC_ADD, self::ASSOC_REMOVE, self::ASSOC_REPLACE]), new Exception("constantUnknown", $mode)); if (!Arsse::$user->authorize($user, __FUNCTION__)) { throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); } @@ -2386,9 +2382,7 @@ class Database { * @param boolean $byName Whether to interpret the $id parameter as the tag's name (true) or identifier (false) */ public function tagSubscriptionsSet(string $user, $id, array $subscriptions, int $mode = self::ASSOC_ADD, bool $byName = false): int { - if (!in_array($mode, [self::ASSOC_ADD, self::ASSOC_REMOVE, self::ASSOC_REPLACE])) { - throw new Exception("constantUnknown", $mode); // @codeCoverageIgnore - } + assert(in_array($mode, [self::ASSOC_ADD, self::ASSOC_REMOVE, self::ASSOC_REPLACE]), new Exception("constantUnknown", $mode)); if (!Arsse::$user->authorize($user, __FUNCTION__)) { throw new User\ExceptionAuthz("notAuthorized", ["action" => __FUNCTION__, "user" => $user]); } diff --git a/lib/Db/AbstractStatement.php b/lib/Db/AbstractStatement.php index abf9f77f..cc8ce422 100644 --- a/lib/Db/AbstractStatement.php +++ b/lib/Db/AbstractStatement.php @@ -56,9 +56,7 @@ abstract class AbstractStatement implements Statement { $this->retypeArray($binding, true); } else { $bindId = self::TYPES[trim(strtolower($binding))] ?? 0; - if (!$bindId) { - throw new Exception("paramTypeInvalid", $binding); // @codeCoverageIgnore - } + assert($bindId, new Exception("paramTypeInvalid", $binding)); $this->types[] = $bindId; } } diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 68c7ea8b..33dac91b 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -10,5 +10,7 @@ const NS_BASE = __NAMESPACE__."\\"; define(NS_BASE."BASE", dirname(__DIR__).DIRECTORY_SEPARATOR); const DOCROOT = BASE."tests".DIRECTORY_SEPARATOR."docroot".DIRECTORY_SEPARATOR; ini_set("memory_limit", "-1"); +ini_set("zend.assertions", "1"); +ini_set("assert.exception", "true"); error_reporting(\E_ALL); require_once BASE."vendor".DIRECTORY_SEPARATOR."autoload.php"; From be5ad50f54df9dfc933c3854090dd043cd1fa2a3 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Thu, 12 Sep 2019 09:41:01 -0400 Subject: [PATCH 3/7] Tests for text search clause generator --- lib/Database.php | 5 +++-- tests/cases/Database/TestDatabase.php | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/lib/Database.php b/lib/Database.php index 37524035..ec373162 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -219,10 +219,11 @@ class Database { $values[] = $term; } } - $clause[] = "(".implode(" or ", $spec).")"; + $spec = sizeof($spec) > 1 ? "(".implode(" or ", $spec).")" : (string) array_pop($spec); + $clause[] = $spec; } $glue = $matchAny ? "or" : "and"; - $clause = $clause ? "(".implode(" $glue ", $clause).")" : ""; + $clause = sizeof($clause) > 1 ? "(".implode(" $glue ", $clause).")" : (string) array_pop($clause); return [$clause, $types, $values]; } diff --git a/tests/cases/Database/TestDatabase.php b/tests/cases/Database/TestDatabase.php index bd07e469..93f6f398 100644 --- a/tests/cases/Database/TestDatabase.php +++ b/tests/cases/Database/TestDatabase.php @@ -55,4 +55,25 @@ class TestDatabase extends \JKingWeb\Arsse\Test\AbstractTest { ["$params", $ints, $ints, "bool"], ]; } + + /** @dataProvider provideSearchClauses */ + public function testGenerateSearchClause(string $clause, array $values, array $inV, array $inC, bool $inAny) { + // this is not an exhaustive test; integration tests already cover the ins and outs of the functionality + $types = array_fill(0, sizeof($values), "str"); + $exp = [$clause, $types, $values]; + $this->assertSame($exp, self::$db->generateSearch($inV, $inC, $inAny)); + } + + public function provideSearchClauses() { + $terms = array_fill(0, Database::LIMIT_SET_SIZE + 1, "a"); + $clause = array_fill(0, Database::LIMIT_SET_SIZE + 1, "test like '%a%' escape '^'"); + $longString = str_repeat("0", Database::LIMIT_SET_STRING_LENGTH + 1); + return [ + ["test like ? escape '^'", ["%a%"], ["a"], ["test"], true], + ["(col1 like ? escape '^' or col2 like ? escape '^')", ["%a%", "%a%"], ["a"], ["col1", "col2"], true], + ["(".implode(" or ", $clause).")", [], $terms, ["test"], true], + ["(".implode(" and ", $clause).")", [], $terms, ["test"], false], + ["(".implode(" or ", $clause)." or test like ? escape '^')", ["%$longString%"], array_merge($terms, [$longString]), ["test"], true], + ]; + } } From 3da884dfbcd760b2f3f5ceb8fa2ed57f60efb0dc Mon Sep 17 00:00:00 2001 From: "J. King" Date: Thu, 12 Sep 2019 09:53:43 -0400 Subject: [PATCH 4/7] Don't embed ito SQL strings with question marks Fixes #175 --- lib/Database.php | 4 ++-- tests/cases/Database/TestDatabase.php | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/Database.php b/lib/Database.php index ec373162..b50f0409 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -165,7 +165,7 @@ class Database { // nulls are pointless to have continue; } elseif (is_string($v)) { - if (strlen($v) > self::LIMIT_SET_STRING_LENGTH) { + if (strlen($v) > self::LIMIT_SET_STRING_LENGTH || strpos($v, "?") !== false) { $clause[] = "?"; $params[] = $v; } else { @@ -205,7 +205,7 @@ class Database { assert(sizeof($cols) > 0, new Exception("arrayEmpty", "cols")); $embedSet = sizeof($terms) > ((int) (self::LIMIT_SET_SIZE / sizeof($cols))); foreach ($terms as $term) { - $embedTerm = ($embedSet && strlen($term) <= self::LIMIT_SET_STRING_LENGTH); + $embedTerm = ($embedSet && strlen($term) <= self::LIMIT_SET_STRING_LENGTH && strpos($term, "?") === false); $term = str_replace(["%", "_", "^"], ["^%", "^_", "^^"], $term); $term = "%$term%"; $term = $embedTerm ? $this->db->literalString($term) : $term; diff --git a/tests/cases/Database/TestDatabase.php b/tests/cases/Database/TestDatabase.php index 93f6f398..53376340 100644 --- a/tests/cases/Database/TestDatabase.php +++ b/tests/cases/Database/TestDatabase.php @@ -52,6 +52,7 @@ class TestDatabase extends \JKingWeb\Arsse\Test\AbstractTest { ["$stringList", [], array_merge($strings, [null]), "str"], ["$stringList,?", [$longString], array_merge($strings, [$longString]), "str"], ["$stringList,'A''s'", [], array_merge($strings, ["A's"]), "str"], + ["$stringList,?", ["???"], array_merge($strings, ["???"]), "str"], ["$params", $ints, $ints, "bool"], ]; } @@ -74,6 +75,8 @@ class TestDatabase extends \JKingWeb\Arsse\Test\AbstractTest { ["(".implode(" or ", $clause).")", [], $terms, ["test"], true], ["(".implode(" and ", $clause).")", [], $terms, ["test"], false], ["(".implode(" or ", $clause)." or test like ? escape '^')", ["%$longString%"], array_merge($terms, [$longString]), ["test"], true], + ["(".implode(" or ", $clause)." or test like ? escape '^')", ["%Eh?%"], array_merge($terms, ["Eh?"]), ["test"], true], + ["(".implode(" or ", $clause)." or test like ? escape '^')", ["%?%"], array_merge($terms, ["?"]), ["test"], true], ]; } } From a143c8613639f860d0f85a2c557a7f94d5bf692b Mon Sep 17 00:00:00 2001 From: "J. King" Date: Thu, 12 Sep 2019 11:14:30 -0400 Subject: [PATCH 5/7] Set up test better --- tests/cases/Database/TestDatabase.php | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/tests/cases/Database/TestDatabase.php b/tests/cases/Database/TestDatabase.php index 53376340..60926498 100644 --- a/tests/cases/Database/TestDatabase.php +++ b/tests/cases/Database/TestDatabase.php @@ -10,16 +10,20 @@ use JKingWeb\Arsse\Database; /** @covers \JKingWeb\Arsse\Database */ class TestDatabase extends \JKingWeb\Arsse\Test\AbstractTest { - static $db = null; + protected $db = null; - public static function setUpBeforeClass() { + public function setUp() { self::clearData(); self::setConf(); - self::$db = \Phake::makeVisible(\Phake::partialMock(Database::class)); + try { + $this->db = \Phake::makeVisible(\Phake::partialMock(Database::class)); + } catch (\JKingWeb\Arsse\Db\Exception $e) { + $this->markTestSkipped("SQLite 3 database driver not available"); + } } - public static function tearDownAfterClass() { - self::$db = null; + public function tearDown() { + $this->db = null; self::clearData(); } @@ -27,7 +31,7 @@ class TestDatabase extends \JKingWeb\Arsse\Test\AbstractTest { public function testGenerateInClause(string $clause, array $values, array $inV, string $inT) { $types = array_fill(0, sizeof($values), $inT); $exp = [$clause, $types, $values]; - $this->assertSame($exp, self::$db->generateIn($inV, $inT)); + $this->assertSame($exp, $this->db->generateIn($inV, $inT)); } public function provideInClauses() { @@ -62,7 +66,7 @@ class TestDatabase extends \JKingWeb\Arsse\Test\AbstractTest { // this is not an exhaustive test; integration tests already cover the ins and outs of the functionality $types = array_fill(0, sizeof($values), "str"); $exp = [$clause, $types, $values]; - $this->assertSame($exp, self::$db->generateSearch($inV, $inC, $inAny)); + $this->assertSame($exp, $this->db->generateSearch($inV, $inC, $inAny)); } public function provideSearchClauses() { From 67bde97e0c457383212a5895f3c2a4d42c79e0aa Mon Sep 17 00:00:00 2001 From: "J. King" Date: Thu, 12 Sep 2019 11:50:33 -0400 Subject: [PATCH 6/7] Update changelog --- CHANGELOG | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index 8a8383d1..153dfeb5 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,8 @@ Version 0.8.1 (2019-??-??) Bug fixes: - Don't crash updating feeds cached solely via ETag - Don't fail adding a feed which collides with another via redirection +- Don't fail on very long text-search queries containing question marks + when using PostgreSQL or MySQL Changes: - Include a user manual From c5337b37b4233bcac7a3dac9f787f12b01e064da Mon Sep 17 00:00:00 2001 From: "J. King" Date: Wed, 25 Sep 2019 18:30:53 -0400 Subject: [PATCH 7/7] Consolidate creation of synthetic server requests --- lib/Misc/URL.php | 30 +++++++++ tests/cases/Misc/TestURL.php | 16 +++++ tests/cases/REST/Fever/TestAPI.php | 32 ++-------- tests/cases/REST/NextCloudNews/TestV1_2.php | 42 ++---------- .../cases/REST/NextCloudNews/TestVersions.php | 10 +-- tests/cases/REST/TinyTinyRSS/TestAPI.php | 25 ++------ tests/cases/REST/TinyTinyRSS/TestIcon.php | 17 +---- tests/lib/AbstractTest.php | 64 +++++++++++++++++++ 8 files changed, 130 insertions(+), 106 deletions(-) diff --git a/lib/Misc/URL.php b/lib/Misc/URL.php index 6b9fcce4..da49effb 100644 --- a/lib/Misc/URL.php +++ b/lib/Misc/URL.php @@ -10,6 +10,12 @@ namespace JKingWeb\Arsse\Misc; * A collection of functions for manipulating URLs */ class URL { + + /** Returns whether a URL is absolute i.e. has a scheme */ + public static function absolute(string $url): bool { + return (bool) strlen((string) parse_url($url, \PHP_URL_SCHEME)); + } + /** Normalizes a URL * * Normalizations performed are: @@ -137,4 +143,28 @@ class URL { $out = ($absolute ? "/" : "").$out.($index ? "/" : ""); return str_replace("//", "/", $out); } + + /** Appends data to a URL's query component + * + * @param string $url The input URL + * @param string $data The data to append. This should already be escaped where necessary and not start with any delimiter + * @param string $glue The query subcomponent delimiter, usually "&". If the URL has no query, "?" will be prepended instead + */ + public static function queryAppend(string $url, string $data, string $glue = "&"): string { + if (!strlen($data)) { + return $url; + } + $insPos = strpos($url, "#"); + $insPos = $insPos === false ? strlen($url) : $insPos; + $qPos = strpos($url, "?"); + $hasQuery = $qPos !== false; + $glue = $hasQuery ? $glue : "?"; + if ($hasQuery && $insPos > 0) { + if ($url[$insPos - 1] === $glue || ($insPos - 1) == $qPos) { + // if the URL already has excess glue, use it + $glue = ""; + } + } + return substr($url, 0, $insPos).$glue.$data.substr($url, $insPos); + } } diff --git a/tests/cases/Misc/TestURL.php b/tests/cases/Misc/TestURL.php index 9d06933e..8260c0b0 100644 --- a/tests/cases/Misc/TestURL.php +++ b/tests/cases/Misc/TestURL.php @@ -75,4 +75,20 @@ class TestURL extends \JKingWeb\Arsse\Test\AbstractTest { [" ", "%20"], ]; } + + /** @dataProvider provideQueries */ + public function testAppendQueryParameters(string $url, string $query, string $exp) { + $this->assertSame($exp, URL::queryAppend($url, $query)); + } + + public function provideQueries() { + return [ + ["/", "ook=eek", "/?ook=eek"], + ["/?", "ook=eek", "/?ook=eek"], + ["/#ack", "ook=eek", "/?ook=eek#ack"], + ["/?Huh?", "ook=eek", "/?Huh?&ook=eek"], + ["/?Eh?&Huh?&", "ook=eek", "/?Eh?&Huh?&ook=eek"], + ["/#ack", "", "/#ack"], + ]; + } } diff --git a/tests/cases/REST/Fever/TestAPI.php b/tests/cases/REST/Fever/TestAPI.php index e453151e..183e0037 100644 --- a/tests/cases/REST/Fever/TestAPI.php +++ b/tests/cases/REST/Fever/TestAPI.php @@ -145,33 +145,11 @@ class TestAPI extends \JKingWeb\Arsse\Test\AbstractTest { return $value; } - protected function req($dataGet, $dataPost = "", string $method = "POST", string $type = null, string $url = "", string $user = null): ServerRequest { - $url = "/fever/".$url; + protected function req($dataGet, $dataPost = "", string $method = "POST", string $type = null, string $target = "", string $user = null): ServerRequest { + $prefix = "/fever/"; + $url = $prefix.$target; $type = $type ?? "application/x-www-form-urlencoded"; - $server = [ - 'REQUEST_METHOD' => $method, - 'REQUEST_URI' => $url, - 'HTTP_CONTENT_TYPE' => $type, - ]; - $req = new ServerRequest($server, [], $url, $method, "php://memory", ['Content-Type' => $type]); - if (!is_array($dataGet)) { - parse_str($dataGet, $dataGet); - } - $req = $req->withRequestTarget($url)->withQueryParams($dataGet); - if (is_array($dataPost)) { - $req = $req->withParsedBody($dataPost); - } else { - parse_str($dataPost, $arr); - $req = $req->withParsedBody($arr); - } - if (isset($user)) { - if (strlen($user)) { - $req = $req->withAttribute("authenticated", true)->withAttribute("authenticatedUser", $user); - } else { - $req = $req->withAttribute("authenticationFailed", true); - } - } - return $req; + return $this->serverRequest($method, $url, $prefix, [], [], $dataPost, $type, $dataGet, $user); } public function setUp() { @@ -457,7 +435,7 @@ class TestAPI extends \JKingWeb\Arsse\Test\AbstractTest { return [ 'Not an API request' => [$this->req(""), new EmptyResponse(404)], 'Wrong method' => [$this->req("api", "", "GET"), new EmptyResponse(405, ['Allow' => "OPTIONS,POST"])], - 'Wrong content type' => [$this->req("api", "", "POST", "application/json"), new EmptyResponse(415, ['Accept' => "application/x-www-form-urlencoded"])], + 'Wrong content type' => [$this->req("api", '{"api_key":"validToken"}', "POST", "application/json"), new EmptyResponse(415, ['Accept' => "application/x-www-form-urlencoded"])], ]; } diff --git a/tests/cases/REST/NextCloudNews/TestV1_2.php b/tests/cases/REST/NextCloudNews/TestV1_2.php index da0b951b..5ae10feb 100644 --- a/tests/cases/REST/NextCloudNews/TestV1_2.php +++ b/tests/cases/REST/NextCloudNews/TestV1_2.php @@ -298,40 +298,10 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { ], ]; - protected function req(string $method, string $target, string $data = "", array $headers = []): ResponseInterface { - $url = "/index.php/apps/news/api/v1-2".$target; - $server = [ - 'REQUEST_METHOD' => $method, - 'REQUEST_URI' => $url, - 'PHP_AUTH_USER' => "john.doe@example.com", - 'PHP_AUTH_PW' => "secret", - 'REMOTE_USER' => "john.doe@example.com", - ]; - if (strlen($data)) { - $server['HTTP_CONTENT_TYPE'] = "application/json"; - } - $req = new ServerRequest($server, [], $url, $method, "php://memory"); - if (Arsse::$user->auth("john.doe@example.com", "secret")) { - $req = $req->withAttribute("authenticated", true)->withAttribute("authenticatedUser", "john.doe@example.com"); - } - foreach ($headers as $key => $value) { - if (!is_null($value)) { - $req = $req->withHeader($key, $value); - } else { - $req = $req->withoutHeader($key); - } - } - if (strlen($data)) { - $body = $req->getBody(); - $body->write($data); - $req = $req->withBody($body); - } - $q = $req->getUri()->getQuery(); - if (strlen($q)) { - parse_str($q, $q); - $req = $req->withQueryParams($q); - } - $req = $req->withRequestTarget($target); + protected function req(string $method, string $target, string $data = "", array $headers = [], bool $authenticated = true): ResponseInterface { + $prefix = "/index.php/apps/news/api/v1-2"; + $url = $prefix.$target; + $req = $this->serverRequest($method, $url, $prefix, $headers, [], $data, "application/json", [], $authenticated ? "john.doe@example.com" : ""); return $this->h->dispatch($req); } @@ -340,7 +310,6 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { self::setConf(); // create a mock user manager Arsse::$user = \Phake::mock(User::class); - \Phake::when(Arsse::$user)->auth->thenReturn(true); Arsse::$user->id = "john.doe@example.com"; // create a mock database interface Arsse::$db = \Phake::mock(Database::class); @@ -357,9 +326,8 @@ class TestV1_2 extends \JKingWeb\Arsse\Test\AbstractTest { } public function testSendAuthenticationChallenge() { - \Phake::when(Arsse::$user)->auth->thenReturn(false); $exp = new EmptyResponse(401); - $this->assertMessage($exp, $this->req("GET", "/")); + $this->assertMessage($exp, $this->req("GET", "/", "", [], false)); } public function testRespondToInvalidPaths() { diff --git a/tests/cases/REST/NextCloudNews/TestVersions.php b/tests/cases/REST/NextCloudNews/TestVersions.php index c803f8d6..dff02afa 100644 --- a/tests/cases/REST/NextCloudNews/TestVersions.php +++ b/tests/cases/REST/NextCloudNews/TestVersions.php @@ -19,13 +19,9 @@ class TestVersions extends \JKingWeb\Arsse\Test\AbstractTest { } protected function req(string $method, string $target): ResponseInterface { - $url = "/index.php/apps/news/api".$target; - $server = [ - 'REQUEST_METHOD' => $method, - 'REQUEST_URI' => $url, - ]; - $req = new ServerRequest($server, [], $url, $method, "php://memory"); - $req = $req->withRequestTarget($target); + $prefix = "/index.php/apps/news/api"; + $url = $prefix.$target; + $req = $this->serverRequest($method, $url, $prefix); return (new Versions)->dispatch($req); } diff --git a/tests/cases/REST/TinyTinyRSS/TestAPI.php b/tests/cases/REST/TinyTinyRSS/TestAPI.php index d3d5870d..7c8d230b 100644 --- a/tests/cases/REST/TinyTinyRSS/TestAPI.php +++ b/tests/cases/REST/TinyTinyRSS/TestAPI.php @@ -126,27 +126,10 @@ LONG_STRING; } protected function req($data, string $method = "POST", string $target = "", string $strData = null, string $user = null): ResponseInterface { - $url = "/tt-rss/api".$target; - $server = [ - 'REQUEST_METHOD' => $method, - 'REQUEST_URI' => $url, - 'HTTP_CONTENT_TYPE' => "application/x-www-form-urlencoded", - ]; - $req = new ServerRequest($server, [], $url, $method, "php://memory"); - $body = $req->getBody(); - if (!is_null($strData)) { - $body->write($strData); - } else { - $body->write(json_encode($data)); - } - $req = $req->withBody($body)->withRequestTarget($target); - if (isset($user)) { - if (strlen($user)) { - $req = $req->withAttribute("authenticated", true)->withAttribute("authenticatedUser", $user); - } else { - $req = $req->withAttribute("authenticationFailed", true); - } - } + $prefix = "/tt-rss/api"; + $url = $prefix.$target; + $body = $strData ?? json_encode($data); + $req = $this->serverRequest($method, $url, $prefix, [], ['HTTP_CONTENT_TYPE' => "application/x-www-form-urlencoded"], $body, "application/json", [], $user); return $this->h->dispatch($req); } diff --git a/tests/cases/REST/TinyTinyRSS/TestIcon.php b/tests/cases/REST/TinyTinyRSS/TestIcon.php index 1d5b8a7b..dc986787 100644 --- a/tests/cases/REST/TinyTinyRSS/TestIcon.php +++ b/tests/cases/REST/TinyTinyRSS/TestIcon.php @@ -34,20 +34,9 @@ class TestIcon extends \JKingWeb\Arsse\Test\AbstractTest { } protected function req(string $target, string $method = "GET", string $user = null): ResponseInterface { - $url = "/tt-rss/feed-icons/".$target; - $server = [ - 'REQUEST_METHOD' => $method, - 'REQUEST_URI' => $url, - ]; - $req = new ServerRequest($server, [], $url, $method, "php://memory"); - $req = $req->withRequestTarget($target); - if (isset($user)) { - if (strlen($user)) { - $req = $req->withAttribute("authenticated", true)->withAttribute("authenticatedUser", $user); - } else { - $req = $req->withAttribute("authenticationFailed", true); - } - } + $prefix = "/tt-rss/feed-icons/"; + $url = $prefix.$target; + $req = $this->serverRequest($method, $url, $prefix, [], [], null, "", [], $user); return $this->h->dispatch($req); } diff --git a/tests/lib/AbstractTest.php b/tests/lib/AbstractTest.php index 6334e5c5..a2e66a31 100644 --- a/tests/lib/AbstractTest.php +++ b/tests/lib/AbstractTest.php @@ -13,10 +13,12 @@ use JKingWeb\Arsse\Db\Driver; use JKingWeb\Arsse\Db\Result; use JKingWeb\Arsse\Misc\Date; use JKingWeb\Arsse\Misc\ValueInfo; +use JKingWeb\Arsse\Misc\URL; use Psr\Http\Message\MessageInterface; use Psr\Http\Message\RequestInterface; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Message\ResponseInterface; +use Zend\Diactoros\ServerRequest; use Zend\Diactoros\Response\JsonResponse; use Zend\Diactoros\Response\XmlResponse; @@ -61,6 +63,68 @@ abstract class AbstractTest extends \PHPUnit\Framework\TestCase { Arsse::$conf = (($force ? null : Arsse::$conf) ?? (new Conf))->import($defaults)->import($conf); } + protected function serverRequest(string $method, string $url, string $urlPrefix, array $headers = [], array $vars = [], $body = null, string $type = "", $params = [], string $user = null): ServerRequestInterface { + $server = [ + 'REQUEST_METHOD' => $method, + 'REQUEST_URI' => $url, + ]; + if (strlen($type)) { + $server['HTTP_CONTENT_TYPE'] = $type; + } + if (isset($params)) { + if (is_array($params)) { + $params = implode("&", array_map(function($v, $k) { + return rawurlencode($k).(isset($v) ? "=".rawurlencode($v) : ""); + }, $params, array_keys($params))); + } + $url = URL::queryAppend($url, (string) $params); + } + $q = parse_url($url, \PHP_URL_QUERY); + if (strlen($q ?? "")) { + parse_str($q, $params); + } else { + $params = []; + } + $parsedBody = null; + if (isset($body)) { + if (is_string($body) && in_array(strtolower($type), ["", "application/x-www-form-urlencoded"])) { + parse_str($body, $parsedBody); + } elseif (!is_string($body) && in_array(strtolower($type), ["application/json", "text/json"])) { + $body = json_encode($body, \JSON_UNESCAPED_SLASHES | \JSON_UNESCAPED_UNICODE); + } elseif (!is_string($body) && in_array(strtolower($type), ["", "application/x-www-form-urlencoded"])) { + $parsedBody = $body; + $body = http_build_query($body, "a", "&"); + } + } + $server = array_merge($server, $vars); + $req = new ServerRequest($server, [], $url, $method, "php://memory", [], [], $params, $parsedBody); + if (isset($user)) { + if (strlen($user)) { + $req = $req->withAttribute("authenticated", true)->withAttribute("authenticatedUser", $user); + } else { + $req = $req->withAttribute("authenticationFailed", true); + } + } + if (strlen($type) &&strlen($body ?? "")) { + $req = $req->withHeader("Content-Type", $type); + } + foreach ($headers as $key => $value) { + if (!is_null($value)) { + $req = $req->withHeader($key, $value); + } else { + $req = $req->withoutHeader($key); + } + } + $target = substr(URL::normalize($url), strlen($urlPrefix)); + $req = $req->withRequestTarget($target); + if (strlen($body ?? "")) { + $p = $req->getBody(); + $p->write($body); + $req = $req->withBody($p); + } + return $req; + } + public function assertException($msg = "", string $prefix = "", string $type = "Exception") { if (func_num_args()) { if ($msg instanceof \JKingWeb\Arsse\AbstractException) {