From 7597b888c308600b46ab4f55d319660835c4ccad Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 15 Jun 2015 18:35:01 +1200 Subject: [PATCH] Make SQLQuery strict semver for 3.2 --- docs/en/04_Changelogs/3.2.0.md | 41 ++++- model/DataQuery.php | 15 +- model/queries/SQLConditionalExpression.php | 12 +- model/queries/SQLQuery.php | 183 ++++++++++++++++++++- model/queries/SQLSelect.php | 2 +- tests/model/SQLQueryTest.php | 106 ++++++++++++ 6 files changed, 333 insertions(+), 26 deletions(-) diff --git a/docs/en/04_Changelogs/3.2.0.md b/docs/en/04_Changelogs/3.2.0.md index 53f84e11d..97c8c37ce 100644 --- a/docs/en/04_Changelogs/3.2.0.md +++ b/docs/en/04_Changelogs/3.2.0.md @@ -55,7 +55,7 @@ * Implementation of a parameterised query framework eliminating the need to manually escape variables for use in SQL queries. This has been integrated into nearly every level of the database ORM. * Refactor of database connectivity classes into separate components linked together through dependency injection -* Refactor of `SQLQuery` into separate objects for each query type: `SQLQuery`, `SQLDelete`, `SQLUpdate` and `SQLInsert` +* Refactor of `SQLQuery` into separate objects for each query type: `SQLSelect`, `SQLDelete`, `SQLUpdate` and `SQLInsert` * PDO is now a standard connector, and is available for all database interfaces * `DataObject::doValidate()` method visibility added to access `DataObject::validate` externally * `NumericField` now uses HTML5 "number" type instead of "text" @@ -455,6 +455,26 @@ After: $query->execute(); +When working with SQLQuery passed into user code, it is advisable to strictly +cast it into either a SQLSelect or SQLDelete. This can be done by using the new +`SQLQuery::toAppropriateExpression()` method, which will automatically convert +to the correct type based on whether the SQLQuery is set to delete or not. + +If a SQLQuery is not converted, then the result of `getWhere` will not be parameterised. +This is because user code written for 3.1 expects this list to be a flat array +of strings. This format is inherently unsafe, and should be avoided where possible. + + + :::php + getWhere(); // Will be flattened (unsafe 3.1 compatible format) + $expression = $query->toAppropriateExpression(); // Either SQLSelect or SQLDelete + $expression->getWhere(); // Will be parameterised (preferred 3.2 compatible format) + } + + + Alternatively: @@ -463,7 +483,8 @@ Alternatively: $query = SQLQuery::create() ->setFrom('"SiteTree"') ->setWhere(array('"SiteTree"."ShowInMenus"' => 0)) - ->toDelete(); + ->setDelete(true) + ->toAppropriateExpression(); $query->execute(); @@ -616,12 +637,14 @@ E.g. DB::prepared_query('DELETE FROM "MyObject" WHERE ParentID = ? OR IsValid = ?', $parameters); -#### 4. Interaction with `SQLQuery::getWhere()` method +#### 4. Interaction with `SQLSelect::getWhere()` method -As all where conditions are now parameterised, the format of the results returned by `SQLQuery::getWhere()` -will not always equate with that in FrameWork 3.1. Once this would be a list of strings, what will -now be returned is a list of conditions, each of which is an associative array mapping the -condition string to a list of parameters provided. +The `SQLSelect` class supercedes the old `SQLQuery` object for performing select queries. Although +both implement the `getWhere()` method, the results returned by `SQLSelect::getWhere()` will be +parameterised while `SQLQuery::getWhere()` will be a flattened array of strings. + +`SQLSelect::getWhere()` returns a list of conditions, each of which is an +associative array mapping the condition string to a list of parameters provided. Before: @@ -630,6 +653,7 @@ Before: getWhere(); $new = array(); foreach($conditions as $condition) { @@ -646,6 +670,7 @@ After: :::php // Increment value of a single condition + $query = new SQLSelect(/*...*/); $conditions = $query->getWhere(); $new = array(); foreach($conditions as $condition) { @@ -665,7 +690,7 @@ replace this method call with the new `getWhereParameterised($parameters)` metho applicable. This method returns a manipulated form of the where conditions stored by the query, so -that it matches the list of strings consistent with the old 3.1 `SQLQuery::getWhere()` behaviour. +that it matches the list of strings similar to the old 3.1 `SQLQuery::getWhere()` behaviour. Additionally, the list of parameters is safely extracted, flattened, and can be passed out through the `$parameters` argument which is passed by reference. diff --git a/model/DataQuery.php b/model/DataQuery.php index 94cbd7d17..b80257448 100644 --- a/model/DataQuery.php +++ b/model/DataQuery.php @@ -90,7 +90,7 @@ class DataQuery { $fieldExpression = key($fieldExpression); } - $where = $this->query->getWhere(); + $where = $this->query->toAppropriateExpression()->getWhere(); // Iterate through each condition foreach($where as $i => $condition) { @@ -836,6 +836,10 @@ class DataQuery { */ class DataQuery_SubGroup extends DataQuery implements SQLConditionGroup { + /** + * + * @var SQLQuery + */ protected $whereQuery; public function __construct(DataQuery $base, $connective) { @@ -867,11 +871,14 @@ class DataQuery_SubGroup extends DataQuery implements SQLConditionGroup { $parameters = array(); // Ignore empty conditions - $where = $this->whereQuery->getWhere(); - if(empty($where)) return null; + $query = $this->whereQuery->toAppropriateExpression(); + $where = $query->getWhere(); + if(empty($where)) { + return null; + } // Allow database to manage joining of conditions - $sql = DB::get_conn()->getQueryBuilder()->buildWhereFragment($this->whereQuery, $parameters); + $sql = DB::get_conn()->getQueryBuilder()->buildWhereFragment($query, $parameters); return preg_replace('/^\s*WHERE\s*/i', '', $sql); } } diff --git a/model/queries/SQLConditionalExpression.php b/model/queries/SQLConditionalExpression.php index 1e330ea50..24afbac16 100644 --- a/model/queries/SQLConditionalExpression.php +++ b/model/queries/SQLConditionalExpression.php @@ -371,7 +371,7 @@ abstract class SQLConditionalExpression extends SQLExpression { /** * Set a WHERE clause. * - * @see SQLQuery::addWhere() for syntax examples + * @see SQLConditionalExpression::addWhere() for syntax examples * * @param mixed $where Predicate(s) to set, as escaped SQL statements or paramaterised queries * @param mixed $where,... Unlimited additional predicates @@ -473,7 +473,7 @@ abstract class SQLConditionalExpression extends SQLExpression { } /** - * @see SQLQuery::addWhere() + * @see SQLConditionalExpression::addWhere() * * @param mixed $filters Predicate(s) to set, as escaped SQL statements or paramaterised queries * @param mixed $filters,... Unlimited additional predicates @@ -487,7 +487,7 @@ abstract class SQLConditionalExpression extends SQLExpression { } /** - * @see SQLQuery::addWhere() + * @see SQLConditionalExpression::addWhere() * * @param mixed $filters Predicate(s) to set, as escaped SQL statements or paramaterised queries * @param mixed $filters,... Unlimited additional predicates @@ -694,12 +694,12 @@ abstract class SQLConditionalExpression extends SQLExpression { } /** - * Generates an SQLQuery object using the currently specified parameters. + * Generates an SQLSelect object using the currently specified parameters. * - * @return SQLQuery + * @return SQLSelect */ public function toSelect() { - $select = new SQLQuery(); + $select = new SQLSelect(); $this->copyTo($select); return $select; } diff --git a/model/queries/SQLQuery.php b/model/queries/SQLQuery.php index 2dde438b1..58404e902 100644 --- a/model/queries/SQLQuery.php +++ b/model/queries/SQLQuery.php @@ -10,6 +10,14 @@ */ class SQLQuery extends SQLSelect { + /** + * If this is true, this statement will delete rather than select. + * + * @deprecated since version 4.0 + * @var boolean + */ + protected $isDelete = false; + /** * @deprecated since version 4.0 */ @@ -21,19 +29,180 @@ class SQLQuery extends SQLSelect { } /** - * @deprecated since version 3.2 + * @deprecated since version 4.0 */ public function setDelete($value) { - $message = 'SQLQuery->setDelete no longer works. Create a SQLDelete object instead, or use toDelete()'; - Deprecation::notice('3.2', $message); - throw new BadMethodCallException($message); + Deprecation::notice('4.0', 'SQLQuery::setDelete is deprecated. Use toDelete instead'); + $this->isDelete = $value; } /** - * @deprecated since version 3.2 + * @deprecated since version 4.0 */ public function getDelete() { - Deprecation::notice('3.2', 'Use SQLDelete object instead'); - return false; + Deprecation::notice('4.0', 'SQLQuery::getDelete is deprecated. Use SQLSelect or SQLDelete instead'); + return $this->isDelete; + } + + public function sql(&$parameters = array()) { + return $this->toAppropriateExpression()->sql($parameters); + } + + /** + * Get helper class for flattening parameterised conditions + * + * @return SQLQuery_ParameterInjector + */ + protected function getParameterInjector() { + return Injector::inst()->get('SQLQuery_ParameterInjector'); + } + + /** + * Return a list of SQL where conditions (flattened as a list of strings) + * + * @return array + */ + public function getWhere() { + Deprecation::notice( + '4.0', + 'SQLQuery::getWhere is non-parameterised for backwards compatibility. '. + 'Use ->toAppropriateExpression()->getWhere() instead' + ); + $conditions = parent::getWhere(); + + // This is where any benefits of parameterised queries die + return $this + ->getParameterInjector() + ->injectConditions($conditions); + } + + /** + * Convert this SQLQuery to a SQLExpression based on its + * internal $delete state (Normally SQLSelect or SQLDelete) + * + * @return SQLExpression + */ + public function toAppropriateExpression() { + if($this->isDelete) { + return parent::toDelete(); + } else { + return parent::toSelect(); + } + } + + public function toSelect() { + if($this->isDelete) { + user_error( + 'SQLQuery::toSelect called when $isDelete is true. Use ' . + 'toAppropriateExpression() instead', + E_USER_WARNING + ); + } + return parent::toSelect(); + } + + public function toDelete() { + if(!$this->isDelete) { + user_error( + 'SQLQuery::toDelete called when $isDelete is false. Use ' . + 'toAppropriateExpression() instead', + E_USER_WARNING + ); + } + parent::toDelete(); + } +} + +/** + * Provides conversion of parameterised SQL to flattened SQL strings + * + * @deprecated since version 4.0 + */ +class SQLQuery_ParameterInjector { + + public function __construct() { + Deprecation::notice('4.0', "Use SQLSelect / SQLDelete instead of SQLQuery"); + } + + /** + * Given a list of parameterised conditions, return a flattened + * list of condition strings + * + * @param array $conditions + * @return array + */ + public function injectConditions($conditions) { + $result = array(); + foreach($conditions as $condition) { + // Evaluate the result of SQLConditionGroup here + if($condition instanceof SQLConditionGroup) { + $predicate = $condition->conditionSQL($parameters); + if(!empty($predicate)) { + $result[] = $this->injectValues($predicate, $parameters); + } + } else { + foreach($condition as $predicate => $parameters) { + $result[] = $this->injectValues($predicate, $parameters); + } + } + } + return $result; + } + + /** + * Merge parameters into a SQL prepared condition + * + * @param string $sql + * @param array $parameters + * @return string + */ + protected function injectValues($sql, $parameters) { + $segments = preg_split('/\?/', $sql); + $joined = ''; + $inString = false; + for($i = 0; $i < count($segments); $i++) { + // Append next segment + $joined .= $segments[$i]; + // Don't add placeholder after last segment + if($i === count($segments) - 1) { + break; + } + // check string escape on previous fragment + if($this->checkStringTogglesLiteral($segments[$i])) { + $inString = !$inString; + } + // Append placeholder replacement + if($inString) { + // Literal questionmark + $joined .= '?'; + continue; + } + + // Encode and insert next parameter + $next = array_shift($parameters); + if(is_array($next) && isset($next['value'])) { + $next = $next['value']; + } + $joined .= "'".Convert::raw2sql($next)."'"; + } + return $joined; + } + + /** + * Determines if the SQL fragment either breaks into or out of a string literal + * by counting single quotes + * + * Handles double-quote escaped quotes as well as slash escaped quotes + * + * @param string $input The SQL fragment + * @return boolean True if the string breaks into or out of a string literal + */ + protected function checkStringTogglesLiteral($input) { + // Remove escaped backslashes, count them! + $input = preg_replace('/\\\\\\\\/', '', $input); + // Count quotes + $totalQuotes = substr_count($input, "'"); // Includes double quote escaped quotes + $escapedQuotes = substr_count($input, "\\'"); + return (($totalQuotes - $escapedQuotes) % 2) !== 0; } } diff --git a/model/queries/SQLSelect.php b/model/queries/SQLSelect.php index ee388d4f4..ba3ed74fb 100644 --- a/model/queries/SQLSelect.php +++ b/model/queries/SQLSelect.php @@ -8,7 +8,7 @@ * @subpackage model */ class SQLSelect extends SQLConditionalExpression { - + /** * An array of SELECT fields, keyed by an optional alias. * diff --git a/tests/model/SQLQueryTest.php b/tests/model/SQLQueryTest.php index db65c35e1..6449fa2fd 100755 --- a/tests/model/SQLQueryTest.php +++ b/tests/model/SQLQueryTest.php @@ -13,6 +13,18 @@ class SQLQueryTest extends SapphireTest { 'SQLQueryTestBase', 'SQLQueryTestChild' ); + + protected $oldDeprecation = null; + + public function setUp() { + parent::setUp(); + $this->oldDeprecation = Deprecation::dump_settings(); + } + + public function tearDown() { + Deprecation::restore_settings($this->oldDeprecation); + parent::tearDown(); + } public function testCount() { @@ -681,6 +693,100 @@ class SQLQueryTest extends SapphireTest { $this->assertEquals(array('%MyName%', '2012-08-08 12:00'), $parameters); $query->execute(); } + + /** + * Test deprecation of SQLQuery::getWhere working appropriately + */ + public function testDeprecatedGetWhere() { + // Temporarily disable deprecation + Deprecation::notification_version(null); + + $query = new SQLQuery(); + $query->setSelect(array('"SQLQueryTest_DO"."Name"')); + $query->setFrom('"SQLQueryTest_DO"'); + $query->addWhere(array( + '"SQLQueryTest_DO"."Date" > ?' => '2012-08-08 12:00' + )); + $query->addWhere('"SQLQueryTest_DO"."Name" = \'Richard\''); + $query->addWhere(array( + '"SQLQueryTest_DO"."Meta" IN (?, \'Who?\', ?)' => array('Left', 'Right') + )); + + $expectedSQL = << ?) + AND ("SQLQueryTest_DO"."Name" = 'Richard') + AND ("SQLQueryTest_DO"."Meta" IN (?, 'Who?', ?)) +EOS + ; + $expectedParameters = array('2012-08-08 12:00', 'Left', 'Right'); + + + // Check sql evaluation of this query maintains the parameters + $sql = $query->sql($parameters); + $this->assertSQLEquals($expectedSQL, $sql); + $this->assertEquals($expectedParameters, $parameters); + + // Check that ->toAppropriateExpression()->setWhere doesn't modify the query + $query->setWhere($query->toAppropriateExpression()->getWhere()); + $sql = $query->sql($parameters); + $this->assertSQLEquals($expectedSQL, $sql); + $this->assertEquals($expectedParameters, $parameters); + + // Check that getWhere are all flattened queries + $expectedFlattened = array( + '"SQLQueryTest_DO"."Date" > \'2012-08-08 12:00\'', + '"SQLQueryTest_DO"."Name" = \'Richard\'', + '"SQLQueryTest_DO"."Meta" IN (\'Left\', \'Who?\', \'Right\')' + ); + $this->assertEquals($expectedFlattened, $query->getWhere()); + } + + /** + * Test deprecation of SQLQuery::setDelete/getDelete + */ + public function testDeprecatedSetDelete() { + // Temporarily disable deprecation + Deprecation::notification_version(null); + + $query = new SQLQuery(); + $query->setSelect(array('"SQLQueryTest_DO"."Name"')); + $query->setFrom('"SQLQueryTest_DO"'); + $query->setWhere(array('"SQLQueryTest_DO"."Name"' => 'Andrew')); + + // Check SQL for select + $this->assertSQLEquals(<<sql($parameters) + ); + $this->assertEquals(array('Andrew'), $parameters); + + // Check setDelete works + $query->setDelete(true); + $this->assertSQLEquals(<<sql($parameters) + ); + $this->assertEquals(array('Andrew'), $parameters); + + // Check that setDelete back to false restores the state + $query->setDelete(false); + $this->assertSQLEquals(<<sql($parameters) + ); + $this->assertEquals(array('Andrew'), $parameters); + } } class SQLQueryTest_DO extends DataObject implements TestOnly {