From 5d61ab0a5779e2c61d1d9660c0bceba3980ad860 Mon Sep 17 00:00:00 2001 From: "J. King" Date: Sun, 13 Jan 2019 23:17:19 -0500 Subject: [PATCH] Fixes for MySQL native interface Three test failures remain, but these are minor and will be resolved soon. Handling of binary data is also broken, but given that this works fine with the PDO driver, there is presumably some correct method. --- lib/Db/MySQL/Driver.php | 46 +++++++++++++--------- lib/Db/MySQL/Result.php | 6 +-- lib/Db/MySQL/Statement.php | 14 ++++--- locale/en.php | 4 +- tests/cases/DatabaseDrivers/MySQL.php | 11 +++++- tests/cases/DatabaseDrivers/MySQLPDO.php | 9 ++++- tests/cases/Db/BaseStatement.php | 4 +- tests/cases/Db/MySQL/TestDatabase.php | 21 ++++++++++ tests/cases/Db/MySQL/TestDriver.php | 50 ++++++++++++++++++++++++ tests/cases/Db/MySQL/TestResult.php | 28 +++++++++++++ tests/cases/Db/MySQL/TestStatement.php | 32 +++++++++++++++ tests/cases/Db/MySQL/TestUpdate.php | 17 ++++++++ tests/cases/Db/MySQLPDO/TestDatabase.php | 1 + tests/phpunit.xml | 7 ++++ 14 files changed, 216 insertions(+), 34 deletions(-) create mode 100644 tests/cases/Db/MySQL/TestDatabase.php create mode 100644 tests/cases/Db/MySQL/TestDriver.php create mode 100644 tests/cases/Db/MySQL/TestResult.php create mode 100644 tests/cases/Db/MySQL/TestStatement.php create mode 100644 tests/cases/Db/MySQL/TestUpdate.php diff --git a/lib/Db/MySQL/Driver.php b/lib/Db/MySQL/Driver.php index 41b4d532..8a2ab62d 100644 --- a/lib/Db/MySQL/Driver.php +++ b/lib/Db/MySQL/Driver.php @@ -49,7 +49,7 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver { $this->exec($q); } // get the maximum packet size; parameter strings larger than this size need to be chunked - $this->packetSize = $this->query("select variable_value from performance_schema.session_variables where variable_name = 'max_allowed_packet'")->getValue(); + $this->packetSize = (int) $this->query("select variable_value from performance_schema.session_variables where variable_name = 'max_allowed_packet'")->getValue(); } public static function makeSetupQueries(): array { @@ -161,7 +161,7 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver { } public static function requirementsMet(): bool { - return false; + return class_exists("mysqli"); } protected function makeConnection(string $db, string $user, string $password, string $host, int $port, string $socket) { @@ -170,39 +170,47 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver { if ($this->db->connect_errno) { echo $this->db->connect_errno.": ".$this->db->connect_error; } - $this->db->set_character_set("utf8mb4"); + $this->db->set_charset("utf8mb4"); } catch (\Exception $e) { throw $e; } } public function exec(string $query): bool { - $this->dispatch($query); + $this->dispatch($query, true); return true; } - protected function dispatch(string $query) { - $r = $this->db->query($query); - if ($this->db->sqlstate !== "00000") { - if ($this->db->sqlstate === "HY000") { - list($excClass, $excMsg, $excData) = $this->buildEngineException($this->db->errno, $this->db->error); - } else { - list($excClass, $excMsg, $excData) = $this->buildStandardException($this->db->sqlstate, $this->db->error); - } - throw new $excClass($excMsg, $excData); + protected function dispatch(string $query, bool $multi = false) { + if ($multi) { + $this->db->multi_query($query); + } else { + $this->db->real_query($query); + } + $e = null; + do { + if ($this->db->sqlstate !== "00000") { + if ($this->db->sqlstate === "HY000") { + list($excClass, $excMsg, $excData) = $this->buildEngineException($this->db->errno, $this->db->error); + } else { + list($excClass, $excMsg, $excData) = $this->buildStandardException($this->db->sqlstate, $this->db->error); + } + $e = new $excClass($excMsg, $excData, $e); + } + $r = $this->db->store_result(); + } while ($this->db->more_results() && $this->db->next_result()); + if ($e) { + throw $e; + } else { + return $r; } - return $r; } public function query(string $query): \JKingWeb\Arsse\Db\Result { $r = $this->dispatch($query); $rows = (int) $this->db->affected_rows; $id = (int) $this->db->insert_id; - if ($r === true) { - return new \JKingWeb\Arsse\Db\ResultEmpty($rows, $id); - } else { - return new ResultE($r, [$rows, $id]); - } + return new Result($r, [$rows, $id]); } public function prepareArray(string $query, array $paramTypes): \JKingWeb\Arsse\Db\Statement { diff --git a/lib/Db/MySQL/Result.php b/lib/Db/MySQL/Result.php index d96c7ebc..0a4fd4cc 100644 --- a/lib/Db/MySQL/Result.php +++ b/lib/Db/MySQL/Result.php @@ -27,9 +27,9 @@ class Result extends \JKingWeb\Arsse\Db\AbstractResult { // constructor/destructor - public function __construct(\mysqli_result $result, array $changes = [0,0], Statement $statement = null) { + public function __construct($result, array $changes = [0,0], Statement $statement = null) { $this->st = $statement; //keeps the statement from being destroyed, invalidating the result set - $this->set = $result; + $this->set = ($result instanceof \mysqli_result) ? $result : null; $this->rows = $changes[0]; $this->id = $changes[1]; } @@ -45,7 +45,7 @@ class Result extends \JKingWeb\Arsse\Db\AbstractResult { // PHP iterator methods public function valid() { - $this->cur = $this->set->fetch_assoc(); + $this->cur = $this->set ? $this->set->fetch_assoc() : null; return ($this->cur !== null); } } diff --git a/lib/Db/MySQL/Statement.php b/lib/Db/MySQL/Statement.php index b1e08baa..ca0405ec 100644 --- a/lib/Db/MySQL/Statement.php +++ b/lib/Db/MySQL/Statement.php @@ -28,9 +28,9 @@ class Statement extends \JKingWeb\Arsse\Db\AbstractStatement { protected $packetSize; protected $values; - protected $types; + protected $binds = ""; - public function __construct(\mysqli $db, string $query, array $bindings = [], int $packetSize) { + public function __construct(\mysqli $db, string $query, array $bindings = [], int $packetSize = 4194304) { $this->db = $db; $this->query = $query; $this->packetSize = $packetSize; @@ -58,11 +58,13 @@ class Statement extends \JKingWeb\Arsse\Db\AbstractStatement { $this->st->reset(); // prepare values and them all at once $this->bindValues($values); - $this->st->bind_params($this->types, ...$this->values); + if ($this->values) { + $this->st->bind_param($this->binds, ...$this->values); + } // execute the statement $this->st->execute(); // clear normalized values - $this->types = ""; + $this->binds = ""; $this->values = []; // check for errors if ($this->st->sqlstate !== "00000") { @@ -84,13 +86,13 @@ class Statement extends \JKingWeb\Arsse\Db\AbstractStatement { // this is a bit of a hack: we collect values (and MySQL bind types) here so that we can take // advantage of the work done by bindValues() even though MySQL requires everything to be bound // all at once; we also packetize large values here if necessary - if (is_string($value) && strlen($value) > $this->packetSize) { + if (($type === "binary" && !is_null($value)) || (is_string($value) && strlen($value) > $this->packetSize)) { $this->values[] = null; $this->st->send_long_data($position - 1, $value); } else { $this->values[] = $value; - $this->types .= self::BINDINGS[$type]; } + $this->binds .= self::BINDINGS[$type]; return true; } public static function mungeQuery(string $query, array $types, ...$extraData): string { diff --git a/locale/en.php b/locale/en.php index 51cb71da..fb312cb8 100644 --- a/locale/en.php +++ b/locale/en.php @@ -22,8 +22,8 @@ return [ 'Driver.Db.SQLite3PDO.Name' => 'SQLite 3 (PDO)', 'Driver.Db.PostgreSQL.Name' => 'PostgreSQL', 'Driver.Db.PostgreSQLPDO.Name' => 'PostgreSQL (PDO)', - 'Driver.Db.MySQL.Name' => 'MySQL/MariaDB', - 'Driver.Db.MySQLPDO.Name' => 'MySQL/MariaDB (PDO)', + 'Driver.Db.MySQL.Name' => 'MySQL', + 'Driver.Db.MySQLPDO.Name' => 'MySQL (PDO)', 'Driver.Service.Curl.Name' => 'HTTP (curl)', 'Driver.Service.Internal.Name' => 'Internal', 'Driver.User.Internal.Name' => 'Internal', diff --git a/tests/cases/DatabaseDrivers/MySQL.php b/tests/cases/DatabaseDrivers/MySQL.php index 5d5ba700..27dcb4af 100644 --- a/tests/cases/DatabaseDrivers/MySQL.php +++ b/tests/cases/DatabaseDrivers/MySQL.php @@ -15,9 +15,18 @@ trait MySQL { protected static $dbResultClass = \JKingWeb\Arsse\Db\MySQL\Result::class; protected static $dbStatementClass = \JKingWeb\Arsse\Db\MySQL\Statement::class; protected static $dbDriverClass = \JKingWeb\Arsse\Db\MySQL\Driver::class; - protected static $stringOutput = false; + protected static $stringOutput = true; public static function dbInterface() { + $d = new \mysqli(Arsse::$conf->dbMySQLHost, Arsse::$conf->dbMySQLUser, Arsse::$conf->dbMySQLPass, Arsse::$conf->dbMySQLDb, Arsse::$conf->dbMySQLPort); + if ($d->connect_errno) { + return; + } + $d->set_charset("utf8mb4"); + foreach (\JKingWeb\Arsse\Db\MySQL\PDODriver::makeSetupQueries() as $q) { + $d->query($q); + } + return $d; } public static function dbTableList($db): array { diff --git a/tests/cases/DatabaseDrivers/MySQLPDO.php b/tests/cases/DatabaseDrivers/MySQLPDO.php index ab2589e2..def124dd 100644 --- a/tests/cases/DatabaseDrivers/MySQLPDO.php +++ b/tests/cases/DatabaseDrivers/MySQLPDO.php @@ -30,7 +30,14 @@ trait MySQLPDO { $dsn[] = "$k=$v"; } $dsn = "mysql:".implode(";", $dsn); - return new \PDO($dsn, Arsse::$conf->dbMySQLUser, Arsse::$conf->dbMySQLPass, [\PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION, \PDO::MYSQL_ATTR_MULTI_STATEMENTS => false, \PDO::MYSQL_ATTR_INIT_COMMAND => "SET sql_mode = '".\JKingWeb\Arsse\Db\MySQL\PDODriver::SQL_MODE."'",]); + $d = new \PDO($dsn, Arsse::$conf->dbMySQLUser, Arsse::$conf->dbMySQLPass, [ + \PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION, + \PDO::MYSQL_ATTR_MULTI_STATEMENTS => false, + ]); + foreach (\JKingWeb\Arsse\Db\MySQL\PDODriver::makeSetupQueries() as $q) { + $d->exec($q); + } + return $d; } catch (\Throwable $e) { return; } diff --git a/tests/cases/Db/BaseStatement.php b/tests/cases/Db/BaseStatement.php index a9c7e780..6d198628 100644 --- a/tests/cases/Db/BaseStatement.php +++ b/tests/cases/Db/BaseStatement.php @@ -67,8 +67,8 @@ abstract class BaseStatement extends \JKingWeb\Arsse\Test\AbstractTest { /** @dataProvider provideBinaryBindings */ public function testHandleBinaryData($value, string $type, string $exp) { - if (in_array(static::$implementation, ["PostgreSQL", "PDO PostgreSQL"])) { - $this->markTestSkipped("Correct handling of binary data with PostgreSQL is currently unknown"); + if (in_array(static::$implementation, ["MySQL", "PostgreSQL", "PDO PostgreSQL"])) { + $this->markTestSkipped("Correct handling of binary data with PostgreSQL and native MySQL is currently unknown"); } if ($exp === "null") { $query = "SELECT (? is null) as pass"; diff --git a/tests/cases/Db/MySQL/TestDatabase.php b/tests/cases/Db/MySQL/TestDatabase.php new file mode 100644 index 00000000..81e4cecf --- /dev/null +++ b/tests/cases/Db/MySQL/TestDatabase.php @@ -0,0 +1,21 @@ + + * @covers \JKingWeb\Arsse\Misc\Query + */ +class TestDatabase extends \JKingWeb\Arsse\TestCase\Database\Base { + use \JKingWeb\Arsse\TestCase\DatabaseDrivers\MySQL; + + protected function nextID(string $table): int { + return (int) (static::$drv->query("SELECT auto_increment from information_schema.tables where table_name = '$table'")->getValue() ?? 1); + } +} diff --git a/tests/cases/Db/MySQL/TestDriver.php b/tests/cases/Db/MySQL/TestDriver.php new file mode 100644 index 00000000..15b21c11 --- /dev/null +++ b/tests/cases/Db/MySQL/TestDriver.php @@ -0,0 +1,50 @@ + */ +class TestDriver extends \JKingWeb\Arsse\TestCase\Db\BaseDriver { + use \JKingWeb\Arsse\TestCase\DatabaseDrivers\MySQL; + + protected $create = "CREATE TABLE arsse_test(id bigint auto_increment primary key)"; + protected $lock = ["SET lock_wait_timeout = 1", "LOCK TABLES arsse_meta WRITE"]; + protected $setVersion = "UPDATE arsse_meta set value = '#' where `key` = 'schema_version'"; + protected static $insertDefaultValues = "INSERT INTO arsse_test(id) values(default)"; + + protected function exec($q): bool { + if (is_array($q)) { + $q = implode("; ", $q); + } + static::$interface->multi_query($q); + $e = null; + do { + if (!$e && static::$interface->sqlstate !== "00000") { + $e = new \Exception(static::$interface->error); + } + } while (static::$interface->more_results() && static::$interface->next_result()); + if ($e) { + throw $e; + } + return true; + } + + protected function query(string $q) { + $r = static::$interface->query($q); + if ($r) { + $row = $r->fetch_row(); + $r->free(); + if ($row) { + return $row[0]; + } else { + return null; + } + } + return null; + } +} diff --git a/tests/cases/Db/MySQL/TestResult.php b/tests/cases/Db/MySQL/TestResult.php new file mode 100644 index 00000000..f0520e2c --- /dev/null +++ b/tests/cases/Db/MySQL/TestResult.php @@ -0,0 +1,28 @@ + + */ +class TestResult extends \JKingWeb\Arsse\TestCase\Db\BaseResult { + use \JKingWeb\Arsse\TestCase\DatabaseDrivers\MySQL; + + protected static $createMeta = "CREATE TABLE arsse_meta(`key` varchar(255) primary key not null, value text)"; + protected static $createTest = "CREATE TABLE arsse_test(id bigint auto_increment primary key)"; + protected static $insertDefault = "INSERT INTO arsse_test(id) values(default)"; + + protected function makeResult(string $q): array { + $r = static::$interface->query($q); + $rows = static::$interface->affected_rows; + $id = static::$interface->insert_id; + return [$r, [$rows, $id]]; + } +} diff --git a/tests/cases/Db/MySQL/TestStatement.php b/tests/cases/Db/MySQL/TestStatement.php new file mode 100644 index 00000000..868a87f5 --- /dev/null +++ b/tests/cases/Db/MySQL/TestStatement.php @@ -0,0 +1,32 @@ + */ +class TestStatement extends \JKingWeb\Arsse\TestCase\Db\BaseStatement { + use \JKingWeb\Arsse\TestCase\DatabaseDrivers\MySQL; + + protected function makeStatement(string $q, array $types = []): array { + return [static::$interface, $q, $types]; + } + + protected function decorateTypeSyntax(string $value, string $type): string { + switch ($type) { + case "float": + return (substr($value, -2) === ".0") ? "'".substr($value, 0, strlen($value) - 2)."'" : "'$value'"; + case "string": + if (preg_match("<^char\((\d+)\)$>", $value, $match)) { + return "'".\IntlChar::chr((int) $match[1])."'"; + } + return $value; + default: + return $value; + } + } +} diff --git a/tests/cases/Db/MySQL/TestUpdate.php b/tests/cases/Db/MySQL/TestUpdate.php new file mode 100644 index 00000000..c2cf7a1b --- /dev/null +++ b/tests/cases/Db/MySQL/TestUpdate.php @@ -0,0 +1,17 @@ + */ +class TestUpdate extends \JKingWeb\Arsse\TestCase\Db\BaseUpdate { + use \JKingWeb\Arsse\TestCase\DatabaseDrivers\MySQL; + + protected static $minimal1 = "CREATE TABLE arsse_meta(`key` varchar(255) primary key, value text); INSERT INTO arsse_meta(`key`,value) values('schema_version','1');"; + protected static $minimal2 = "UPDATE arsse_meta set value = '2' where `key` = 'schema_version';"; +} diff --git a/tests/cases/Db/MySQLPDO/TestDatabase.php b/tests/cases/Db/MySQLPDO/TestDatabase.php index 2b94823f..3af3dcfd 100644 --- a/tests/cases/Db/MySQLPDO/TestDatabase.php +++ b/tests/cases/Db/MySQLPDO/TestDatabase.php @@ -8,6 +8,7 @@ namespace JKingWeb\Arsse\TestCase\Db\MySQLPDO; /** * @group slow + * @group optional * @group coverageOptional * @covers \JKingWeb\Arsse\Database * @covers \JKingWeb\Arsse\Misc\Query diff --git a/tests/phpunit.xml b/tests/phpunit.xml index 6fa69da2..65a08939 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -69,6 +69,12 @@ cases/Db/PostgreSQLPDO/TestDriver.php cases/Db/PostgreSQLPDO/TestUpdate.php + cases/Db/MySQL/TestResult.php + cases/Db/MySQL/TestStatement.php + cases/Db/MySQL/TestCreation.php + cases/Db/MySQL/TestDriver.php + cases/Db/MySQL/TestUpdate.php + cases/Db/MySQLPDO/TestResult.php cases/Db/MySQLPDO/TestStatement.php cases/Db/MySQLPDO/TestCreation.php @@ -80,6 +86,7 @@ cases/Db/SQLite3PDO/TestDatabase.php cases/Db/PostgreSQL/TestDatabase.php cases/Db/PostgreSQLPDO/TestDatabase.php + cases/Db/MySQL/TestDatabase.php cases/Db/MySQLPDO/TestDatabase.php