Merge pull request #4154 from tractorcow/pulls/3/fix-prepared-statements-beta

API Use mysql buffered statements

model/connect/MySQLQuery.php

@@ -8,43 +8,28 @@
  */
 class MySQLQuery extends SS_Query {
 
-	/**
-	 * The MySQLiConnector object that created this result set.
-	 *
-	 * @var MySQLiConnector
-	 */
-	protected $database;
-
 	/**
 	 * The internal MySQL handle that points to the result set.
+	 * Select queries will have mysqli_result as a value.
+	 * Non-select queries will not
 	 *
-	 * @var mysqli_result
+	 * @var mixed
 	 */
 	protected $handle;
 
-	/**
-	 * The related mysqli statement object if generated using a prepared query
-	 *
-	 * @var mysqli_stmt
-	 */
-	protected $statement;
-
 	/**
 	 * Hook the result-set given into a Query class, suitable for use by SilverStripe.
-	 * @param MySQLDatabase $database The database object that created this query.
-	 * @param mysqli_result $handle the internal mysql handle that is points to the resultset.
-	 * @param mysqli_stmt $statement The related statement, if present
+	 *
+	 * @param MySQLiConnector $database The database object that created this query.
+	 * @param mixed $handle the internal mysql handle that is points to the resultset.
+	 * Non-mysqli_result values could be given for non-select queries (e.g. true)
 	 */
-	public function __construct(MySQLiConnector $database, $handle = null, $statement = null) {
-		$this->database = $database;
+	public function __construct($database, $handle) {
 		$this->handle = $handle;
-		$this->statement = $statement;
 	}
 
 	public function __destruct() {
 		if (is_object($this->handle)) $this->handle->free();
-		// Don't close statement as these may be re-used across the life of this request
-		// if (is_object($this->statement)) $this->statement->close();
 	}
 
 	public function seek($row) {

model/connect/MySQLStatement.php

@@ -0,0 +1,116 @@
+<?php
+
+/**
+ * Provides a record-view for mysqli statements
+ *
+ * By default streams unbuffered data, but seek(), rewind(), or numRecords() will force the statement to
+ * buffer itself and sacrifice any potential performance benefit.
+ */
+class MySQLStatement extends SS_Query {
+
+	/**
+	 * The related mysqli statement object if generated using a prepared query
+	 *
+	 * @var mysqli_stmt
+	 */
+	protected $statement;
+
+	/**
+	 * Metadata result for this statement
+	 *
+	 * @var mysqli_result
+	 */
+	protected $metadata;
+
+	/**
+	 * Is the statement bound to the current resultset?
+	 *
+	 * @var bool
+	 */
+	protected $bound = false;
+
+	/**
+	 * List of column names
+	 *
+	 * @var array
+	 */
+	protected $columns = array();
+
+	/**
+	 * List of bound variables in the current row
+	 *
+	 * @var array
+	 */
+	protected $boundValues = array();
+
+	/**
+	 * Binds this statement to the variables
+	 */
+	protected function bind() {
+		$variables = array();
+
+		// Bind each field
+		while($field = $this->metadata->fetch_field()) {
+			$this->columns[] = $field->name;
+			// Note that while boundValues isn't initialised at this point,
+			// later calls to $this->statement->fetch() Will populate
+			// $this->boundValues later with the next result.
+			$variables[] = &$this->boundValues[$field->name];
+		}
+
+		call_user_func_array(array($this->statement, 'bind_result'), $variables);
+		$this->bound = true;
+		$this->metadata->free();
+
+		// Buffer all results
+		$this->statement->store_result();
+	}
+
+	/**
+	 * Hook the result-set given into a Query class, suitable for use by SilverStripe.
+	 * @param mysqli_stmt $statement The related statement, if present
+	 * @param mysqli_result $metadata The metadata for this statement
+	 */
+	public function __construct($statement, $metadata) {
+		$this->statement = $statement;
+		$this->metadata = $metadata;
+
+		// Immediately bind and buffer
+		$this->bind();
+	}
+
+	public function __destruct() {
+		$this->statement->close();
+		$this->closed = true;
+		$this->currentRecord = false;
+	}
+
+	public function seek($row) {
+		$this->rowNum = $row - 1;
+		$this->statement->data_seek($row);
+		return $this->next();
+	}
+
+	public function numRecords() {
+		return $this->statement->num_rows();
+	}
+
+	public function nextRecord() {
+		// Skip data if out of data
+		if (!$this->statement->fetch()) {
+			return false;
+		}
+
+		// Dereferenced row
+		$row = array();
+		foreach($this->boundValues as $key => $value) {
+			$row[$key] = $value;
+		}
+		return $row;
+	}
+
+	public function rewind() {
+		return $this->seek(0);
+	}
+
+}

model/connect/MySQLiConnector.php

@@ -33,7 +33,7 @@ class MySQLiConnector extends DBConnector {
 	 *
 	 * @param mysqli_stmt $statement
 	 */
-	public function setLastStatement($statement) {
+	protected function setLastStatement($statement) {
 		$this->lastStatement = $statement;
 	}
 
@@ -45,8 +45,9 @@ class MySQLiConnector extends DBConnector {
 	 * @return mysqli_stmt
 	 */
 	public function prepareStatement($sql, &$success) {
-		// Prepare statement with arguments
+		// Record last statement for error reporting
 		$statement = $this->dbConn->stmt_init();
+		$this->setLastStatement($statement);
 		$success = $statement->prepare($sql);
 		return $statement;
 	}
@@ -116,7 +117,7 @@ class MySQLiConnector extends DBConnector {
 		// Benchmark query
 		$conn = $this->dbConn;
 		$handle = $this->benchmarkQuery($sql, function($sql) use($conn) {
-			return $conn->query($sql);
+			return $conn->query($sql, MYSQLI_STORE_RESULT);
 		});
 
 		if (!$handle || $this->dbConn->error) {
@@ -124,10 +125,8 @@ class MySQLiConnector extends DBConnector {
 			return null;
 		}
 
-		if($handle !== true) {
-			// Some non-select queries return true on success
-			return new MySQLQuery($this, $handle);
-		}
+		// Some non-select queries return true on success
+		return new MySQLQuery($this, $handle);
 	}
 
 	/**
@@ -222,9 +221,11 @@ class MySQLiConnector extends DBConnector {
 		$lastStatement = $this->benchmarkQuery($sql, function($sql) use($parsedParameters, $blobs, $self) {
 
 			$statement = $self->prepareStatement($sql, $success);
-			if(!$success) return $statement;
+			if(!$success) return null;
 
-			$self->bindParameters($statement, $parsedParameters);
+			if($parsedParameters) {
+				$self->bindParameters($statement, $parsedParameters);
+			}
 
 			// Bind any blobs given
 			foreach($blobs as $blob) {
@@ -235,18 +236,19 @@ class MySQLiConnector extends DBConnector {
 			$statement->execute();
 			return $statement;
 		});
-
-		// check result
-		$this->setLastStatement($lastStatement);
+		
 		if (!$lastStatement || $lastStatement->error) {
 			$values = $this->parameterValues($parameters);
 			$this->databaseError($this->getLastError(), $errorLevel, $sql, $values);
 			return null;
 		}
 
-		// May not return result for non-select statements
-		if($result = $lastStatement->get_result()) {
-			return new MySQLQuery($this, $result, $lastStatement);
+		// Non-select queries will have no result data
+		if($lastStatement && ($metaData = $lastStatement->result_metadata())) {
+			return new MySQLStatement($lastStatement, $metaData);
+		} else {
+			// Replicate normal behaviour of ->query() on non-select calls
+			return new MySQLQuery($this, true);
 		}
 	}
 

tests/model/MySQLDatabaseTest.php

@@ -5,46 +5,120 @@
  */
 
 class MySQLDatabaseTest extends SapphireTest {
+	
+	protected static $fixture_file = 'MySQLDatabaseTest.yml';
+
 	protected $extraDataObjects = array(
-		'MySQLDatabaseTest_DO',
+		'MySQLDatabaseTest_Data'
 	);
 
-	public function setUp() {
-		if(DB::get_conn() instanceof MySQLDatabase) {
-			MySQLDatabaseTest_DO::config()->db = array(
-				'MultiEnum1' => 'MultiEnum("A, B, C, D","")',
-				'MultiEnum2' => 'MultiEnum("A, B, C, D","A")',
-				'MultiEnum3' => 'MultiEnum("A, B, C, D","A, B")',
-			);
+	public function testPreparedStatements() {
+		if(!(DB::get_connector() instanceof MySQLiConnector)) {
+			$this->markTestSkipped('This test requires the current DB connector is MySQLi');
 		}
-		$this->markTestSkipped('This test requires the Config API to be immutable');
-		parent::setUp();
+
+		// 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('MySQLStatement', $result1);
+		$this->assertInstanceOf('MySQLStatement', $result2);
+
+		// Also select non-prepared statement
+		$result3 = DB::get_connector()->query('SELECT "Sort", "Title" FROM "MySQLDatabaseTest_Data" ORDER BY "Sort"');
+		$this->assertInstanceOf('MySQLQuery', $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()
+		);
 	}
 
-	/**
-	 * Check that once a schema has been generated, then it doesn't need any more updating
-	 */
-	public function testFieldsDontRerequestChanges() {
-		// These are MySQL specific :-S
-		if(DB::get_conn() instanceof MySQLDatabase) {
-			$schema = DB::get_schema();
-			$test = $this;
-			DB::quiet();
-
-			// Verify that it doesn't need to be recreated
-			$schema->schemaUpdate(function() use ($test, $schema) {
-				$obj = new MySQLDatabaseTest_DO();
-				$obj->requireTable();
-				$needsUpdating = $schema->doesSchemaNeedUpdating();
-				$schema->cancelSchemaUpdate();
-
-				$test->assertFalse($needsUpdating);
-			});
+	public function testAffectedRows() {
+		if(!(DB::get_connector() instanceof MySQLiConnector)) {
+			$this->markTestSkipped('This test requires the current DB connector is MySQLi');
 		}
+
+		$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('MySQLQuery', $result);
+		$this->assertEquals(0, DB::affected_rows());
+
+		// Test update which affects some rows
+		$query->setWhere(array('Title' => 'First Item'));
+		$result = $query->execute();
+		$this->assertInstanceOf('MySQLQuery', $result);
+		$this->assertEquals(1, DB::affected_rows());
 	}
 }
 
-class MySQLDatabaseTest_DO extends DataObject implements TestOnly {
-	private static $db = array();
+class MySQLDatabaseTest_Data extends DataObject implements TestOnly {
+	private static $db = array(
+		'Title' => 'Varchar',
+		'Description' => 'Text',
+		'Enabled' => 'Boolean',
+		'Sort' => 'Int'
+	);
 
+	private static $default_sort = '"Sort" ASC';
 }

tests/model/MySQLDatabaseTest.yml

@@ -0,0 +1,17 @@
+MySQLDatabaseTest_Data:
+  data1:
+    Title: 'First Item'
+    Description: 'The content'
+    Sort: 1
+  data2:
+    Title: 'Second Item'
+    Description: 'More Content'
+    Sort: 2
+  data3:
+    Title: 'Third Item'
+    Description: ''
+    Sort: 3
+  data4:
+    Title: 'Last Item'
+    Description: 'Testing'
+    Sort: 4