From efa9ff9b08c7eb7f0dca61ac78c24c908fe3b8ef Mon Sep 17 00:00:00 2001 From: stojg Date: Mon, 10 Dec 2012 18:11:07 +1300 Subject: [PATCH] API: Queries added by DataList::addInnerJoin() and DataList::leftJoin() come after the base joins, not before. This bug will surface when using the ORM and adding an join to DataList where a DataObject inherits another DataObject. If you for example want to restrict the number of pages that only have a related Staff object: $list = DataList::create('Page') ->InnerJoin('Staff', '"Staff"."ID" = "Page"."StaffID"); This will create a SQL query where the INNER JOIN is before the LEFT JOIN of Page and SiteTree in the resulting SQL string. In MySQL and PostgreSQL this will create an invalid query. This patch solves the problem by sorting the joins. --- model/DataQuery.php | 2 +- model/SQLQuery.php | 125 +++++++++++++++++++++++++++++----- tests/model/DataQueryTest.php | 21 ++++++ tests/model/SQLQueryTest.php | 14 ++++ 4 files changed, 145 insertions(+), 17 deletions(-) diff --git a/model/DataQuery.php b/model/DataQuery.php index e1aade7f5..39277f8a4 100644 --- a/model/DataQuery.php +++ b/model/DataQuery.php @@ -190,7 +190,7 @@ class DataQuery { } if ($joinTable) { - $query->addLeftJoin($tableClass, "\"$tableClass\".\"ID\" = \"$baseClass\".\"ID\"") ; + $query->addLeftJoin($tableClass, "\"$tableClass\".\"ID\" = \"$baseClass\".\"ID\"", $tableClass, 10) ; } } diff --git a/model/SQLQuery.php b/model/SQLQuery.php index 020473101..3e866a8a4 100644 --- a/model/SQLQuery.php +++ b/model/SQLQuery.php @@ -276,18 +276,28 @@ class SQLQuery { * Add a LEFT JOIN criteria to the FROM clause. * * @param string $table Unquoted table name - * @param string $onPredicate The "ON" SQL fragment in a "LEFT JOIN ... AS ... ON ..." statement, - * Needs to be valid (quoted) SQL. + * @param string $onPredicate The "ON" SQL fragment in a "LEFT JOIN ... AS ... ON ..." statement, Needs to be valid + * (quoted) SQL. * @param string $tableAlias Optional alias which makes it easier to identify and replace joins later on + * @param int $order A numerical index to control the order that joins are added to the query; lower order values + * will cause the query to appear first. The default is 20, and joins created automatically by the + * ORM have a value of 10. * @return SQLQuery */ - public function addLeftJoin($table, $onPredicate, $tableAlias = null) { - if(!$tableAlias) $tableAlias = $table; - $this->from[$tableAlias] = array('type' => 'LEFT', 'table' => $table, 'filter' => array($onPredicate)); + public function addLeftJoin($table, $onPredicate, $tableAlias = '', $order = 20) { + if(!$tableAlias) { + $tableAlias = $table; + } + $this->from[$tableAlias] = array( + 'type' => 'LEFT', + 'table' => $table, + 'filter' => array($onPredicate), + 'order' => $order + ); return $this; } - public function leftjoin($table, $onPredicate, $tableAlias = null) { + public function leftjoin($table, $onPredicate, $tableAlias = null, $order = 20) { Deprecation::notice('3.0', 'Please use addLeftJoin() instead!'); $this->addLeftJoin($table, $onPredicate, $tableAlias); } @@ -296,20 +306,28 @@ class SQLQuery { * Add an INNER JOIN criteria to the FROM clause. * * @param string $table Unquoted table name - * @param string $onPredicate The "ON" SQL fragment in an "INNER JOIN ... AS ... ON ..." statement. - * Needs to be valid (quoted) SQL. + * @param string $onPredicate The "ON" SQL fragment in an "INNER JOIN ... AS ... ON ..." statement. Needs to be + * valid (quoted) SQL. * @param string $tableAlias Optional alias which makes it easier to identify and replace joins later on + * @param int $order A numerical index to control the order that joins are added to the query; lower order values + * will cause the query to appear first. The default is 20, and joins created automatically by the + * ORM have a value of 10. * @return SQLQuery */ - public function addInnerJoin($table, $onPredicate, $tableAlias = null) { + public function addInnerJoin($table, $onPredicate, $tableAlias = null, $order = 20) { if(!$tableAlias) $tableAlias = $table; - $this->from[$tableAlias] = array('type' => 'INNER', 'table' => $table, 'filter' => array($onPredicate)); + $this->from[$tableAlias] = array( + 'type' => 'INNER', + 'table' => $table, + 'filter' => array($onPredicate), + 'order' => $order + ); return $this; } - public function innerjoin($table, $onPredicate, $tableAlias = null) { + public function innerjoin($table, $onPredicate, $tableAlias = null, $order = 20) { Deprecation::notice('3.0', 'Please use addInnerJoin() instead!'); - return $this->addInnerJoin($table, $onPredicate, $tableAlias); + return $this->addInnerJoin($table, $onPredicate, $tableAlias, $order); } /** @@ -876,12 +894,13 @@ class SQLQuery { // TODO: Don't require this internal-state manipulate-and-preserve - let sqlQueryToString() handle the new // syntax $origFrom = $this->from; - + // Sort the joins + $this->from = $this->getOrderedJoins($this->from); // Build from clauses foreach($this->from as $alias => $join) { // $join can be something like this array structure // array('type' => 'inner', 'table' => 'SiteTree', 'filter' => array("SiteTree.ID = 1", - // "Status = 'approved'")) + // "Status = 'approved'", 'order' => 20)) if(is_array($join)) { if(is_string($join['filter'])) $filter = $join['filter']; else if(sizeof($join['filter']) == 1) $filter = $join['filter'][0]; @@ -902,10 +921,9 @@ class SQLQuery { $this->from = $origFrom; // The query was most likely just created and then exectued. - if($sql === 'SELECT *') { + if(trim($sql) === 'SELECT * FROM') { return ''; } - return $sql; } @@ -1086,6 +1104,81 @@ class SQLQuery { $query->setLimit(1, $index); return $query; } + + /** + * Ensure that framework "auto-generated" table JOINs are first in the finalised SQL query. + * This prevents issues where developer-initiated JOINs attempt to JOIN using relations that haven't actually + * yet been scaffolded by the framework. Demonstrated by PostGres in errors like: + *"...ERROR: missing FROM-clause..." + * + * @param $from array - in the format of $this->select + * @return array - and reorderded list of selects + */ + protected function getOrderedJoins($from) { + // shift the first FROM table out from so we only deal with the JOINs + $baseFrom = array_shift($from); + $this->mergesort($from, function($firstJoin, $secondJoin) { + if($firstJoin['order'] == $secondJoin['order']) { + return 0; + } + return ($firstJoin['order'] < $secondJoin['order']) ? -1 : 1; + }); + + // Put the first FROM table back into the results + array_unshift($from, $baseFrom); + return $from; + } + /** + * Since uasort don't preserve the order of an array if the comparison is equal + * we have to resort to a merge sort. It's quick and stable: O(n*log(n)). + * + * @see http://stackoverflow.com/q/4353739/139301 + * + * @param array &$array - the array to sort + * @param callable $cmpFunction - the function to use for comparison + */ + protected function mergesort(&$array, $cmpFunction = 'strcmp') { + // Arrays of size < 2 require no action. + if (count($array) < 2) { + return; + } + // Split the array in half + $halfway = count($array) / 2; + $array1 = array_slice($array, 0, $halfway); + $array2 = array_slice($array, $halfway); + // Recurse to sort the two halves + $this->mergesort($array1, $cmpFunction); + $this->mergesort($array2, $cmpFunction); + // If all of $array1 is <= all of $array2, just append them. + if(call_user_func($cmpFunction, end($array1), reset($array2)) < 1) { + $array = array_merge($array1, $array2); + return; + } + // Merge the two sorted arrays into a single sorted array + $array = array(); + $val1 = reset($array1); + $val2 = reset($array2); + do { + if (call_user_func($cmpFunction, $val1, $val2) < 1) { + $array[key($array1)] = $val1; + $val1 = next($array1); + } else { + $array[key($array2)] = $val2; + $val2 = next($array2); + } + } while($val1 && $val2); + + // Merge the remainder + while($val1) { + $array[key($array1)] = $val1; + $val1 = next($array1); + } + while($val2) { + $array[key($array2)] = $val2; + $val2 = next($array2); + } + return; + } } diff --git a/tests/model/DataQueryTest.php b/tests/model/DataQueryTest.php index 5c074a549..1eb643462 100644 --- a/tests/model/DataQueryTest.php +++ b/tests/model/DataQueryTest.php @@ -1,6 +1,13 @@ assertEquals('DataQueryTest_B', $dq->applyRelation('ManyTestBs'), 'DataQuery::applyRelation should return the name of the related object.'); } + + public function testRelationOrderWithCustomJoin() { + $dataQuery = new DataQuery('DataQueryTest_B'); + $dataQuery->innerJoin('DataQueryTest_D', '"DataQueryTest_D"."RelationID" = "DataQueryTest_B"."ID"'); + $dataQuery->execute(); + } } @@ -56,6 +69,7 @@ class DataQueryTest_B extends DataQueryTest_A { } class DataQueryTest_C extends DataObject implements TestOnly { + public static $has_one = array( 'TestA' => 'DataQueryTest_A', 'TestB' => 'DataQueryTest_B', @@ -71,3 +85,10 @@ class DataQueryTest_C extends DataObject implements TestOnly { 'ManyTestBs' => 'DataQueryTest_B', ); } + +class DataQueryTest_D extends DataObject implements TestOnly { + + public static $has_one = array( + 'Relation' => 'DataQueryTest_B', + ); +} diff --git a/tests/model/SQLQueryTest.php b/tests/model/SQLQueryTest.php index 8e2b25984..f59c2d519 100755 --- a/tests/model/SQLQueryTest.php +++ b/tests/model/SQLQueryTest.php @@ -292,6 +292,7 @@ class SQLQueryTest extends SapphireTest { $query->sql() ); } + public function testSetWhereAny() { $query = new SQLQuery(); @@ -376,4 +377,17 @@ class SQLQueryTest_DO extends DataObject implements TestOnly { ); } +class SQLQueryTestBase extends DataObject implements TestOnly { + static $db = array( + "Title" => "Varchar", + ); +} +class SQLQueryTestChild extends SQLQueryTestBase { + static $db = array( + "Name" => "Varchar", + ); + + static $has_one = array( + ); +}