From 4a1c23ba455358af0098ca377d5ccb7d6953ca10 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Thu, 29 Nov 2018 13:45:37 -0500 Subject: [PATCH] Munge PostgreSQL queries instead of adding explicit casts PDO does not adequately inform PostgreSQL of a parameter's type, so type casts are required. Rather than adding these to each query manually, the queries are instead processed to add type hints automatically. Unfortunately the queries are processed rather naively; question-mark characters in string constants, identifiers, regex patterns, or geometry operators will break things spectacularly. --- lib/Database.php | 4 +- lib/Db/PDODriver.php | 4 +- lib/Db/PDOError.php | 5 +- lib/Db/PDOStatement.php | 8 +-- lib/Db/PostgreSQL/PDODriver.php | 4 ++ lib/Db/PostgreSQL/PDOStatement.php | 77 +++++++++++++++++++++ tests/cases/Db/BaseStatement.php | 4 +- tests/cases/Db/PostgreSQL/TestStatement.php | 2 +- tests/lib/DatabaseInformation.php | 4 +- 9 files changed, 97 insertions(+), 15 deletions(-) create mode 100644 lib/Db/PostgreSQL/PDOStatement.php diff --git a/lib/Database.php b/lib/Database.php index 115ae09..c3f91c2 100644 --- a/lib/Database.php +++ b/lib/Database.php @@ -385,7 +385,7 @@ class Database { // SQL will happily accept duplicates (null is not unique), so we must do this check ourselves $p = $this->db->prepare( "WITH RECURSIVE - target as (select ? as userid, cast(? as bigint) as source, cast(? as bigint) as dest, ? as rename), + target as (select ? as userid, ? as source, ? as dest, ? as rename), folders as (SELECT id from arsse_folders join target on owner = userid and coalesce(parent,0) = source union select arsse_folders.id as id from arsse_folders join folders on arsse_folders.parent=folders.id) ". "SELECT @@ -480,7 +480,7 @@ class Database { join arsse_feeds on feed = arsse_feeds.id left join topmost on folder=f_id" ); - $q->setOrder("pinned desc, title collate nocase"); + $q->setOrder("pinned desc, coalesce(arsse_subscriptions.title, arsse_feeds.title) collate nocase"); // define common table expressions $q->setCTE("userdata(userid)", "SELECT ?", "str", $user); // the subject user; this way we only have to pass it to prepare() once // topmost folders belonging to the user diff --git a/lib/Db/PDODriver.php b/lib/Db/PDODriver.php index c6ec0d4..0eedd6b 100644 --- a/lib/Db/PDODriver.php +++ b/lib/Db/PDODriver.php @@ -28,9 +28,9 @@ trait PDODriver { } $changes = $r->rowCount(); try { - $lastId = 0; - $lastId = $this->db->lastInsertId(); + $lastId = ($changes) ? $this->db->lastInsertId() : 0; } catch (\PDOException $e) { // @codeCoverageIgnore + $lastId = 0; } return new PDOResult($r, [$changes, $lastId]); } diff --git a/lib/Db/PDOError.php b/lib/Db/PDOError.php index d9ee7c8..5aee4df 100644 --- a/lib/Db/PDOError.php +++ b/lib/Db/PDOError.php @@ -7,14 +7,15 @@ declare(strict_types=1); namespace JKingWeb\Arsse\Db; trait PDOError { - public function exceptionBuild(): array { - if ($this instanceof Statement) { + public function exceptionBuild(bool $statementError = null): array { + if ($statementError ?? ($this instanceof Statement)) { $err = $this->st->errorInfo(); } else { $err = $this->db->errorInfo(); } switch ($err[0]) { case "22P02": + case "42804": return [ExceptionInput::class, 'engineTypeViolation', $err[2]]; case "23000": case "23502": diff --git a/lib/Db/PDOStatement.php b/lib/Db/PDOStatement.php index 95b95a7..dca020f 100644 --- a/lib/Db/PDOStatement.php +++ b/lib/Db/PDOStatement.php @@ -28,10 +28,10 @@ class PDOStatement extends AbstractStatement { } public function __destruct() { - unset($this->st); + unset($this->st, $this->db); } - public function runArray(array $values = []): \JKingWeb\Arsse\Db\Result { + public function runArray(array $values = []): Result { $this->st->closeCursor(); $this->bindValues($values); try { @@ -42,9 +42,9 @@ class PDOStatement extends AbstractStatement { } $changes = $this->st->rowCount(); try { - $lastId = 0; - $lastId = $this->db->lastInsertId(); + $lastId = ($changes) ? $this->db->lastInsertId() : 0; } catch (\PDOException $e) { // @codeCoverageIgnore + $lastId = 0; } return new PDOResult($this->st, [$changes, $lastId]); } diff --git a/lib/Db/PostgreSQL/PDODriver.php b/lib/Db/PostgreSQL/PDODriver.php index af98338..d9716da 100644 --- a/lib/Db/PostgreSQL/PDODriver.php +++ b/lib/Db/PostgreSQL/PDODriver.php @@ -44,4 +44,8 @@ class PDODriver extends Driver { public static function driverName(): string { return Arsse::$lang->msg("Driver.Db.PostgreSQLPDO.Name"); } + + public function prepareArray(string $query, array $paramTypes): \JKingWeb\Arsse\Db\Statement { + return new PDOStatement($this->db, $query, $paramTypes); + } } diff --git a/lib/Db/PostgreSQL/PDOStatement.php b/lib/Db/PostgreSQL/PDOStatement.php new file mode 100644 index 0000000..9d584d0 --- /dev/null +++ b/lib/Db/PostgreSQL/PDOStatement.php @@ -0,0 +1,77 @@ + "bigint", + "float" => "decimal", + "datetime" => "timestamp", + "binary" => "bytea", + "string" => "text", + "boolean" => "smallint", // FIXME: using boolean leads to incompatibilities with versions of SQLite bundled prior to PHP 7.3 + ]; + + protected $db; + protected $st; + protected $qOriginal; + protected $qMunged; + protected $bindings; + + public function __construct(\PDO $db, string $query, array $bindings = []) { + $this->db = $db; // both db and st are the same object due to the logic of the PDOError handler + $this->qOriginal = $query; + $this->retypeArray($bindings); + } + + public function __destruct() { + unset($this->db, $this->st); + } + + public function retypeArray(array $bindings, bool $append = false): bool { + if ($append) { + return parent::retypeArray($bindings, $append); + } else { + $this->bindings = $bindings; + parent::retypeArray($bindings, $append); + $this->qMunged = self::mungeQuery($this->qOriginal, $this->types, false); + try { + $s = $this->db->prepare($this->qMunged); + $this->st = new \JKingWeb\Arsse\Db\PDOStatement($this->db, $s, $this->bindings); + } catch (\PDOException $e) { + list($excClass, $excMsg, $excData) = $this->exceptionBuild(true); + throw new $excClass($excMsg, $excData); + } + } + return true; + } + + public static function mungeQuery(string $q, array $types, bool $mungeParamMarkers = true): string { + $q = explode("?", $q); + $out = ""; + for ($b = 1; $b < sizeof($q); $b++) { + $a = $b - 1; + $mark = $mungeParamMarkers ? "\$$b" : "?"; + $type = isset($types[$a]) ? "::".self::BINDINGS[$types[$a]] : ""; + $out .= $q[$a].$mark.$type; + } + $out .= array_pop($q); + return $out; + } + + public function runArray(array $values = []): \JKingWeb\Arsse\Db\Result { + return $this->st->runArray($values); + } + + /** @codeCoverageIgnore */ + protected function bindValue($value, string $type, int $position): bool { + // stub required by abstract parent, but never used + return $value; + } +} diff --git a/tests/cases/Db/BaseStatement.php b/tests/cases/Db/BaseStatement.php index c7da01e..4ac71df 100644 --- a/tests/cases/Db/BaseStatement.php +++ b/tests/cases/Db/BaseStatement.php @@ -59,7 +59,7 @@ abstract class BaseStatement extends \JKingWeb\Arsse\Test\AbstractTest { /** @dataProvider provideBindings */ public function testBindATypedValue($value, string $type, string $exp) { if ($exp=="null") { - $query = "SELECT (cast(? as text) is null) as pass"; + $query = "SELECT (? is null) as pass"; } else { $query = "SELECT ($exp = ?) as pass"; } @@ -76,7 +76,7 @@ abstract class BaseStatement extends \JKingWeb\Arsse\Test\AbstractTest { $this->markTestSkipped("Correct handling of binary data with PostgreSQL is currently unknown"); } if ($exp=="null") { - $query = "SELECT (cast(? as text) is null) as pass"; + $query = "SELECT (? is null) as pass"; } else { $query = "SELECT ($exp = ?) as pass"; } diff --git a/tests/cases/Db/PostgreSQL/TestStatement.php b/tests/cases/Db/PostgreSQL/TestStatement.php index cfa42b4..5066b9a 100644 --- a/tests/cases/Db/PostgreSQL/TestStatement.php +++ b/tests/cases/Db/PostgreSQL/TestStatement.php @@ -13,7 +13,7 @@ class TestStatement extends \JKingWeb\Arsse\TestCase\Db\BaseStatement { protected static $implementation = "PDO PostgreSQL"; protected function makeStatement(string $q, array $types = []): array { - return [static::$interface, static::$interface->prepare($q), $types]; + return [static::$interface, $q, $types]; } protected function decorateTypeSyntax(string $value, string $type): string { diff --git a/tests/lib/DatabaseInformation.php b/tests/lib/DatabaseInformation.php index 01cc444..1eefa38 100644 --- a/tests/lib/DatabaseInformation.php +++ b/tests/lib/DatabaseInformation.php @@ -161,10 +161,10 @@ class DatabaseInformation { 'PDO PostgreSQL' => [ 'pdo' => true, 'backend' => "PostgreSQL", - 'statementClass' => \JKingWeb\Arsse\Db\PDOStatement::class, + 'statementClass' => \JKingWeb\Arsse\Db\PostgreSQL\PDOStatement::class, 'resultClass' => \JKingWeb\Arsse\Db\PDOResult::class, 'driverClass' => \JKingWeb\Arsse\Db\PostgreSQL\PDODriver::class, - 'stringOutput' => true, + 'stringOutput' => false, 'interfaceConstructor' => function() { $connString = \JKingWeb\Arsse\Db\PostgreSQL\Driver::makeConnectionString(true, Arsse::$conf->dbPostgreSQLUser, Arsse::$conf->dbPostgreSQLPass, Arsse::$conf->dbPostgreSQLDb, Arsse::$conf->dbPostgreSQLHost, Arsse::$conf->dbPostgreSQLPort, ""); try {