Permalink
Browse files

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.
  • Loading branch information...
stojg committed Dec 10, 2012
1 parent 3c0bd40 commit efa9ff9b08c7eb7f0dca61ac78c24c908fe3b8ef
Showing with 145 additions and 17 deletions.
  1. +1 −1 model/DataQuery.php
  2. +109 −16 model/SQLQuery.php
  3. +21 −0 tests/model/DataQueryTest.php
  4. +14 −0 tests/model/SQLQueryTest.php
View
@@ -190,7 +190,7 @@ public function getFinalisedQuery($queriedColumns = null) {
}
if ($joinTable) {
- $query->addLeftJoin($tableClass, "\"$tableClass\".\"ID\" = \"$baseClass\".\"ID\"") ;
+ $query->addLeftJoin($tableClass, "\"$tableClass\".\"ID\" = \"$baseClass\".\"ID\"", $tableClass, 10) ;
}
}
View
@@ -276,18 +276,28 @@ public function from($from) {
* 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 @@ public function leftjoin($table, $onPredicate, $tableAlias = null) {
* 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 @@ public function sql() {
// 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 @@ public function sql() {
$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 @@ public function lastRow() {
$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;
+ }
}
@@ -1,6 +1,13 @@
<?php
class DataQueryTest extends SapphireTest {
+
+ protected $extraDataObjects = array(
+ 'DataQueryTest_A',
+ 'DataQueryTest_B',
+ 'DataQueryTest_D',
+ );
+
/**
* Test the leftJoin() and innerJoin method of the DataQuery object
*/
@@ -32,6 +39,12 @@ public function testRelationReturn() {
$this->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',
+ );
+}
@@ -292,6 +292,7 @@ public function testInnerJoin() {
$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(
+ );
+}

0 comments on commit efa9ff9

Please sign in to comment.