From ce3b5a5ace556f65a23348ed6e7bd50dd639f9e0 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 16 Jun 2015 15:04:20 +1200 Subject: [PATCH] BUG Fix major segfault on PDOConnector after any DDL BUG Fix issue in PDOQuery::first() Refactor previewWrite and benchmarkQuery into SS_Database --- model/connect/DBConnector.php | 97 +++++++++------ model/connect/Database.php | 71 ++++++++++- model/connect/MySQLiConnector.php | 45 ++++--- model/connect/PDOConnector.php | 189 ++++++++++++++++++------------ model/connect/Query.php | 1 + tests/model/PDODatabaseTest.php | 113 ++++++++++++++++++ 6 files changed, 378 insertions(+), 138 deletions(-) create mode 100644 tests/model/PDODatabaseTest.php diff --git a/model/connect/DBConnector.php b/model/connect/DBConnector.php index a3fda60fe..94cdf5c7c 100644 --- a/model/connect/DBConnector.php +++ b/model/connect/DBConnector.php @@ -10,11 +10,20 @@ abstract class DBConnector { /** * List of operations to treat as write + * Implicitly includes all ddl_operations * * @config * @var array */ - private static $write_operations = array('insert', 'update', 'delete', 'replace', 'alter', 'drop'); + private static $write_operations = array('insert', 'update', 'delete', 'replace'); + + /** + * List of operations to treat as DDL + * + * @config + * @var array + */ + private static $ddl_operations = array('alter', 'drop', 'create', 'truncate'); /** * Error handler for database errors. @@ -27,6 +36,7 @@ abstract class DBConnector { * @param integer $errorLevel The level of the error to throw. * @param string $sql The SQL related to this query * @param array $parameters Parameters passed to the query + * @throws SS_DatabaseException */ protected function databaseError($msg, $errorLevel = E_USER_ERROR, $sql = null, $parameters = array()) { // Prevent errors when error checking is set at zero level @@ -47,48 +57,59 @@ abstract class DBConnector { user_error($msg, $errorLevel); } } - + /** - * Determines if the query should be previewed, and thus interrupted silently. - * If so, this function also displays the query via the debuging system. - * Subclasess should respect the results of this call for each query, and not - * execute any queries that generate a true response. - * - * @param string $sql The query to be executed - * @return boolean Flag indicating that the query was previewed + * Determine if this SQL statement is a destructive operation (write or ddl) + * + * @param string $sql + * @return bool */ - protected function previewWrite($sql) { - // Break if not requested - if (!isset($_REQUEST['previewwrite'])) return false; - - // Break if non-write operation - $operation = strtolower(substr($sql, 0, strpos($sql, ' '))); - $writeOperations = Config::inst()->get(get_class($this), 'write_operations'); - if (!in_array($operation, $writeOperations)) { + public function isQueryMutable($sql) { + $operations = array_merge( + Config::inst()->get(get_class($this), 'write_operations'), + Config::inst()->get(get_class($this), 'ddl_operations') + ); + return $this->isQueryType($sql, $operations); + } + + /** + * Determine if this SQL statement is a DDL operation + * + * @param string $sql + * @return bool + */ + public function isQueryDDL($sql) { + $operations = Config::inst()->get(get_class($this), 'ddl_operations'); + return $this->isQueryType($sql, $operations); + } + + /** + * Determine if this SQL statement is a write operation + * (alters content but not structure) + * + * @param string $sql + * @return bool + */ + public function isQueryWrite($sql) { + $operations = Config::inst()->get(get_class($this), 'write_operations'); + return $this->isQueryType($sql, $operations); + } + + /** + * Determine if a query is of the given type + * + * @param string $sql Raw SQL + * @param string|array $type Type or list of types (first word in the query). Must be lowercase + */ + protected function isQueryType($sql, $type) { + if(!preg_match('/^(?\w+)\b/', $sql, $matches)) { return false; } - - // output preview message - Debug::message("Will execute: $sql"); - return true; - } - - /** - * Allows the display and benchmarking of queries as they are being run - * - * @param string $sql Query to run, and single parameter to callback - * @param callable $callback Callback to execute code - * @return mixed Result of query - */ - protected function benchmarkQuery($sql, $callback) { - if (isset($_REQUEST['showqueries']) && Director::isDev(true)) { - $starttime = microtime(true); - $result = $callback($sql); - $endtime = round(microtime(true) - $starttime, 4); - Debug::message("\n$sql\n{$endtime}s\n", false); - return $result; + $operation = $matches['operation']; + if(is_array($type)) { + return in_array(strtolower($operation), $type); } else { - return $callback($sql); + return strcasecmp($sql, $type) === 0; } } diff --git a/model/connect/Database.php b/model/connect/Database.php index d12896f8a..ace5071b2 100644 --- a/model/connect/Database.php +++ b/model/connect/Database.php @@ -96,7 +96,19 @@ abstract class SS_Database { * @return SS_Query */ public function query($sql, $errorLevel = E_USER_ERROR) { - return $this->connector->query($sql, $errorLevel); + // Check if we should only preview this query + if ($this->previewWrite($sql)) { + return; + } + + // Benchmark query + $connector = $this->connector; + return $this->benchmarkQuery( + $sql, + function($sql) use($connector, $errorLevel) { + return $connector->query($sql, $errorLevel); + } + ); } @@ -109,7 +121,62 @@ abstract class SS_Database { * @return SS_Query */ public function preparedQuery($sql, $parameters, $errorLevel = E_USER_ERROR) { - return $this->connector->preparedQuery($sql, $parameters, $errorLevel); + // Check if we should only preview this query + if ($this->previewWrite($sql)) { + return; + } + + // Benchmark query + $connector = $this->connector; + return $this->benchmarkQuery( + $sql, + function($sql) use($connector, $parameters, $errorLevel) { + return $connector->preparedQuery($sql, $parameters, $errorLevel); + } + ); + } + + /** + * Determines if the query should be previewed, and thus interrupted silently. + * If so, this function also displays the query via the debuging system. + * Subclasess should respect the results of this call for each query, and not + * execute any queries that generate a true response. + * + * @param string $sql The query to be executed + * @return boolean Flag indicating that the query was previewed + */ + protected function previewWrite($sql) { + // Only preview if previewWrite is set, we are in dev mode, and + // the query is mutable + if (isset($_REQUEST['previewwrite']) + && Director::isDev() + && $this->connector->isQueryMutable($sql) + ) { + // output preview message + Debug::message("Will execute: $sql"); + return true; + } else { + return false; + } + } + + /** + * Allows the display and benchmarking of queries as they are being run + * + * @param string $sql Query to run, and single parameter to callback + * @param callable $callback Callback to execute code + * @return mixed Result of query + */ + protected function benchmarkQuery($sql, $callback) { + if (isset($_REQUEST['showqueries']) && Director::isDev()) { + $starttime = microtime(true); + $result = $callback($sql); + $endtime = round(microtime(true) - $starttime, 4); + Debug::message("\n$sql\n{$endtime}s\n", false); + return $result; + } else { + return $callback($sql); + } } /** diff --git a/model/connect/MySQLiConnector.php b/model/connect/MySQLiConnector.php index f3cec3f04..84f61ac90 100644 --- a/model/connect/MySQLiConnector.php +++ b/model/connect/MySQLiConnector.php @@ -103,22 +103,22 @@ class MySQLiConnector extends DBConnector { public function getVersion() { return $this->dbConn->server_info; } - - protected function benchmarkQuery($sql, $callback) { + + /** + * Invoked before any query is executed + * + * @param string $sql + */ + protected function beforeQuery($sql) { // Clear the last statement $this->setLastStatement(null); - return parent::benchmarkQuery($sql, $callback); } public function query($sql, $errorLevel = E_USER_ERROR) { - // Check if we should only preview this query - if ($this->previewWrite($sql)) return; + $this->beforeQuery($sql); // Benchmark query - $conn = $this->dbConn; - $handle = $this->benchmarkQuery($sql, function($sql) use($conn) { - return $conn->query($sql, MYSQLI_STORE_RESULT); - }); + $handle = $this->dbConn->query($sql, MYSQLI_STORE_RESULT); if (!$handle || $this->dbConn->error) { $this->databaseError($this->getLastError(), $errorLevel, $sql); @@ -208,23 +208,20 @@ class MySQLiConnector extends DBConnector { public function preparedQuery($sql, $parameters, $errorLevel = E_USER_ERROR) { // Shortcut to basic query when not given parameters - if(empty($parameters)) return $this->query($sql, $errorLevel); + if(empty($parameters)) { + return $this->query($sql, $errorLevel); + } - // Check if we should only preview this query - if ($this->previewWrite($sql)) return; + $this->beforeQuery($sql); // Type check, identify, and prepare parameters for passing to the statement bind function $parsedParameters = $this->parsePreparedParameters($parameters, $blobs); // Benchmark query - $self = $this; - $lastStatement = $this->benchmarkQuery($sql, function($sql) use($parsedParameters, $blobs, $self) { - - $statement = $self->prepareStatement($sql, $success); - if(!$success) return null; - + $statement = $this->prepareStatement($sql, $success); + if($success) { if($parsedParameters) { - $self->bindParameters($statement, $parsedParameters); + $this->bindParameters($statement, $parsedParameters); } // Bind any blobs given @@ -234,18 +231,18 @@ class MySQLiConnector extends DBConnector { // Safely execute the statement $statement->execute(); - return $statement; - }); + } - if (!$lastStatement || $lastStatement->error) { + if (!$success || $statement->error) { $values = $this->parameterValues($parameters); $this->databaseError($this->getLastError(), $errorLevel, $sql, $values); return null; } // Non-select queries will have no result data - if($lastStatement && ($metaData = $lastStatement->result_metadata())) { - return new MySQLStatement($lastStatement, $metaData); + $metaData = $statement->result_metadata(); + if($metaData) { + return new MySQLStatement($statement, $metaData); } else { // Replicate normal behaviour of ->query() on non-select calls return new MySQLQuery($this, true); diff --git a/model/connect/PDOConnector.php b/model/connect/PDOConnector.php index a0a38e41d..62f79b56d 100644 --- a/model/connect/PDOConnector.php +++ b/model/connect/PDOConnector.php @@ -30,11 +30,18 @@ class PDOConnector extends DBConnector { protected $databaseName = null; /** - * The most recent statement returned from PDODatabase->query + * If available, the row count of the last executed statement * - * @var PDOStatement + * @var int|null */ - protected $lastStatement = null; + protected $rowCount = null; + + /** + * Error generated by the errorInfo() method of the last PDOStatement + * + * @var array|null + */ + protected $lastStatementError = null; /** * List of prepared statements, cached by SQL string @@ -58,13 +65,22 @@ class PDOConnector extends DBConnector { * @return PDOStatement */ public function getOrPrepareStatement($sql) { - if(empty($this->cachedStatements[$sql])) { - $this->cachedStatements[$sql] = $this->pdoConnection->prepare( - $sql, - array(PDO::ATTR_CURSOR => PDO::CURSOR_FWDONLY) - ); + // Return cached statements + if(isset($this->cachedStatements[$sql])) { + return $this->cachedStatements[$sql]; } - return $this->cachedStatements[$sql]; + + // Generate new statement + $statement = $this->pdoConnection->prepare( + $sql, + array(PDO::ATTR_CURSOR => PDO::CURSOR_FWDONLY) + ); + + // Only cache select statements + if(preg_match('/^(\s*)select\b/i', $sql)) { + $this->cachedStatements[$sql] = $statement; + } + return $statement; } /** @@ -166,54 +182,54 @@ class PDOConnector extends DBConnector { public function quoteString($value) { return $this->pdoConnection->quote($value); } + + /** + * Invoked before any query is executed + * + * @param string $sql + */ + protected function beforeQuery($sql) { + // Reset state + $this->rowCount = 0; + $this->lastStatementError = null; + + // Flush if necessary + if($this->isQueryDDL($sql)) { + $this->flushStatements(); + } + } /** * Executes a query that doesn't return a resultset * - * @param string $sql * @param string $sql The SQL query to execute * @param integer $errorLevel For errors to this query, raise PHP errors * using this error level. + * @return int */ public function exec($sql, $errorLevel = E_USER_ERROR) { - // Check if we should only preview this query - if ($this->previewWrite($sql)) return; - - // Reset last statement to prevent interference in case of error - $this->lastStatement = null; - - // Benchmark query - $pdo = $this->pdoConnection; - $result = $this->benchmarkQuery($sql, function($sql) use($pdo) { - return $pdo->exec($sql); - }); + $this->beforeQuery($sql); + + // Directly exec this query + $result = $this->pdoConnection->exec($sql); // Check for errors - if ($result === false) { - $this->databaseError($this->getLastError(), $errorLevel, $sql); - return null; + if ($result !== false) { + return $this->rowCount = $result; } - - return $result; + + $this->databaseError($this->getLastError(), $errorLevel, $sql); + return null; } public function query($sql, $errorLevel = E_USER_ERROR) { - // Check if we should only preview this query - if ($this->previewWrite($sql)) return; - - // Benchmark query - $pdo = $this->pdoConnection; - $this->lastStatement = $this->benchmarkQuery($sql, function($sql) use($pdo) { - return $pdo->query($sql); - }); - - // Check for errors - if (!$this->lastStatement || $this->hasError($this->lastStatement)) { - $this->databaseError($this->getLastError(), $errorLevel, $sql); - return null; - } - - return new PDOQuery($this->lastStatement); + $this->beforeQuery($sql); + + // Directly query against connection + $statement = $this->pdoConnection->query($sql); + + // Generate results + return $this->prepareResults($statement, $errorLevel, $sql); } /** @@ -272,33 +288,54 @@ class PDOConnector extends DBConnector { } public function preparedQuery($sql, $parameters, $errorLevel = E_USER_ERROR) { - // Check if we should only preview this query - if ($this->previewWrite($sql)) return; - - // Benchmark query - $self = $this; - $this->lastStatement = $this->benchmarkQuery($sql, function($sql) use($parameters, $self) { - - // Prepare statement - $statement = $self->getOrPrepareStatement($sql); - if(!$statement) return null; - - // Inject parameters - $self->bindParameters($statement, $parameters); - - // Safely execute the statement + $this->beforeQuery($sql); + + // Prepare statement + $statement = $this->getOrPrepareStatement($sql); + + // Bind and invoke statement safely + if($statement) { + $this->bindParameters($statement, $parameters); $statement->execute($parameters); - return $statement; - }); - - // Check for errors - if (!$this->lastStatement || $this->hasError($this->lastStatement)) { - $values = $this->parameterValues($parameters); - $this->databaseError($this->getLastError(), $errorLevel, $sql, $values); - return null; + } + + // Generate results + return $this->prepareResults($statement, $errorLevel, $sql); + } + + /** + * Given a PDOStatement that has just been executed, generate results + * and report any errors + * + * @param PDOStatement $statement + * @param int $errorLevel + * @param string $sql + * @param array $parameters + * @return \PDOQuery + */ + protected function prepareResults($statement, $errorLevel, $sql, $parameters = array()) { + + // Record row-count and errors of last statement + if($this->hasError($statement)) { + $this->lastStatementError = $statement->errorInfo(); + } elseif($statement) { + // Count and return results + $this->rowCount = $statement->rowCount(); + return new PDOQuery($statement); } - return new PDOQuery($this->lastStatement); + // Ensure statement is closed + if($statement) { + $statement->closeCursor(); + unset($statement); + } + + // Report any errors + if($parameters) { + $parameters = $this->parameterValues($parameters); + } + $this->databaseError($this->getLastError(), $errorLevel, $sql, $parameters); + return null; } /** @@ -309,24 +346,29 @@ class PDOConnector extends DBConnector { */ protected function hasError($resource) { // No error if no resource - if(empty($resource)) return false; + if(empty($resource)) { + return false; + } // If the error code is empty the statement / connection has not been run yet $code = $resource->errorCode(); - if(empty($code)) return false; + if(empty($code)) { + return false; + } // Skip 'ok' and undefined 'warning' types. // @see http://docstore.mik.ua/orelly/java-ent/jenut/ch08_06.htm return $code !== '00000' && $code !== '01000'; } - + public function getLastError() { - if ($this->hasError($this->lastStatement)) { - $error = $this->lastStatement->errorInfo(); + $error = null; + if ($this->lastStatementError) { + $error = $this->lastStatementError; } elseif($this->hasError($this->pdoConnection)) { $error = $this->pdoConnection->errorInfo(); } - if (isset($error)) { + if ($error) { return sprintf("%s-%s: %s", $error[0], $error[1], $error[2]); } } @@ -336,8 +378,7 @@ class PDOConnector extends DBConnector { } public function affectedRows() { - if (empty($this->lastStatement)) return 0; - return $this->lastStatement->rowCount(); + return $this->rowCount; } public function selectDatabase($name) { diff --git a/model/connect/Query.php b/model/connect/Query.php index 22dd38f00..6bd20d398 100644 --- a/model/connect/Query.php +++ b/model/connect/Query.php @@ -146,6 +146,7 @@ abstract class SS_Query implements Iterator { public function rewind() { if ($this->queryHasBegun && $this->numRecords() > 0) { $this->queryHasBegun = false; + $this->currentRecord = null; return $this->seek(0); } } diff --git a/tests/model/PDODatabaseTest.php b/tests/model/PDODatabaseTest.php new file mode 100644 index 000000000..c613a931a --- /dev/null +++ b/tests/model/PDODatabaseTest.php @@ -0,0 +1,113 @@ +markTestSkipped('This test requires the current DB connector is PDO'); + } + + // Test preparation of equivalent statemetns + $result1 = DB::get_connector()->preparedQuery( + 'SELECT "Sort", "Title" FROM "MySQLDatabaseTest_Data" WHERE "Sort" > ? ORDER BY "Sort"', + array(0) + ); + + $result2 = DB::get_connector()->preparedQuery( + 'SELECT "Sort", "Title" FROM "MySQLDatabaseTest_Data" WHERE "Sort" > ? ORDER BY "Sort"', + array(2) + ); + $this->assertInstanceOf('PDOQuery', $result1); + $this->assertInstanceOf('PDOQuery', $result2); + + // Also select non-prepared statement + $result3 = DB::get_connector()->query('SELECT "Sort", "Title" FROM "MySQLDatabaseTest_Data" ORDER BY "Sort"'); + $this->assertInstanceOf('PDOQuery', $result3); + + // Iterating one level should not buffer, but return the right result + $this->assertEquals( + array( + 'Sort' => 1, + 'Title' => 'First Item' + ), + $result1->next() + ); + $this->assertEquals( + array( + 'Sort' => 2, + 'Title' => 'Second Item' + ), + $result1->next() + ); + + // Test first + $this->assertEquals( + array( + 'Sort' => 1, + 'Title' => 'First Item' + ), + $result1->first() + ); + + // Test seek + $this->assertEquals( + array( + 'Sort' => 2, + 'Title' => 'Second Item' + ), + $result1->seek(1) + ); + + // Test count + $this->assertEquals(4, $result1->numRecords()); + + // Test second statement + $this->assertEquals( + array( + 'Sort' => 3, + 'Title' => 'Third Item' + ), + $result2->next() + ); + + // Test non-prepared query + $this->assertEquals( + array( + 'Sort' => 1, + 'Title' => 'First Item' + ), + $result3->next() + ); + } + + public function testAffectedRows() { + if(!(DB::get_connector() instanceof PDOConnector)) { + $this->markTestSkipped('This test requires the current DB connector is PDO'); + } + + $query = new SQLUpdate('MySQLDatabaseTest_Data'); + $query->setAssignments(array('Title' => 'New Title')); + + // Test update which affects no rows + $query->setWhere(array('Title' => 'Bob')); + $result = $query->execute(); + $this->assertInstanceOf('PDOQuery', $result); + $this->assertEquals(0, DB::affected_rows()); + + // Test update which affects some rows + $query->setWhere(array('Title' => 'First Item')); + $result = $query->execute(); + $this->assertInstanceOf('PDOQuery', $result); + $this->assertEquals(1, DB::affected_rows()); + } +} \ No newline at end of file