BUG Fix data query not always joining necessary tables

Fixes #2846

model/DataQuery.php

@@ -149,11 +149,13 @@ class DataQuery {
 		}
 
 		$query = clone $this->query;
+		$ancestorTables = ClassInfo::ancestry($this->dataClass, true);
 
-		// Generate the list of tables to iterate over and the list of columns required by any existing where clauses.
+		// Generate the list of tables to iterate over and the list of columns required
-		// This second step is skipped if we're fetching the whole dataobject as any required columns will get selected
+		// by any existing where clauses. This second step is skipped if we're fetching
-		// regardless.
+		// the whole dataobject as any required columns will get selected regardless.
 		if($queriedColumns) {
+			// Specifying certain columns allows joining of child tables
 			$tableClasses = ClassInfo::dataClassesFor($this->dataClass);
 
 			foreach ($query->getWhere() as $where) {
@@ -163,8 +165,9 @@ class DataQuery {
 					if (!in_array($matches[1], $queriedColumns)) $queriedColumns[] = $matches[1];
 				}
 			}
+		} else {
+			$tableClasses = $ancestorTables;
 		}
-		else $tableClasses = ClassInfo::ancestry($this->dataClass, true);
 
 		$tableNames = array_keys($tableClasses);
 		$baseClass = $tableNames[0];
@@ -172,30 +175,26 @@ class DataQuery {
 		// Iterate over the tables and check what we need to select from them. If any selects are made (or the table is
 		// required for a select)
 		foreach($tableClasses as $tableClass) {
-			$joinTable = false;
 
-			// If queriedColumns is set, then check if any of the fields are in this table.
+			// Determine explicit columns to select
+			$selectColumns = null;
 			if ($queriedColumns) {
+				// Restrict queried columns to that on the selected table
 				$tableFields = DataObject::database_fields($tableClass);
-				$selectColumns = array();
+				$selectColumns = array_intersect($queriedColumns, array_keys($tableFields));
-				// Look through columns specifically requested in query (or where clause)
-				foreach ($queriedColumns as $queriedColumn) {
-					if (array_key_exists($queriedColumn, $tableFields)) {
-						$selectColumns[] = $queriedColumn;
-					}
 			}
 			
+			// If this is a subclass without any explicitly requested columns, omit this from the query
+			if(!in_array($tableClass, $ancestorTables) && empty($selectColumns)) continue;
+			
+			// Select necessary columns (unless an explicitly empty array)
+			if($selectColumns !== array()) {
 				$this->selectColumnsFromTable($query, $tableClass, $selectColumns);
-				if ($selectColumns && $tableClass != $baseClass) {
-					$joinTable = true;
-				}
-			} else {
-				$this->selectColumnsFromTable($query, $tableClass);
-				if ($tableClass != $baseClass) $joinTable = true;
 			}
 
-			if ($joinTable) {
+			// Join if not the base table
-				$query->addLeftJoin($tableClass, "\"$tableClass\".\"ID\" = \"$baseClass\".\"ID\"", $tableClass, 10) ;
+			if($tableClass !== $baseClass) {
+				$query->addLeftJoin($tableClass, "\"$tableClass\".\"ID\" = \"$baseClass\".\"ID\"", $tableClass, 10);
 			}
 		}
 		
@@ -687,7 +686,8 @@ class DataQuery {
 	/**
 	 * Query the given field column from the database and return as an array.
 	 * 
-	 * @param String $field See {@link expressionForField()}.
+	 * @param string $field See {@link expressionForField()}.
+	 * @return array List of column values for the specified column
 	 */
 	public function column($field = 'ID') {
 		$fieldExpression = $this->expressionForField($field);

tests/model/DataObjectLazyLoadingTest.php

@@ -35,6 +35,7 @@ class DataObjectLazyLoadingTest extends SapphireTest {
 			'"DataObjectTest_Team"."ClassName" IS NOT NULL THEN "DataObjectTest_Team"."ClassName" ELSE ' .
 			$db->prepStringForDB('DataObjectTest_Team').' END AS "RecordClassName", "DataObjectTest_Team"."Title" '.
 			'FROM "DataObjectTest_Team" ' .
+			'LEFT JOIN "DataObjectTest_SubTeam" ON "DataObjectTest_SubTeam"."ID" = "DataObjectTest_Team"."ID" ' .
 			'WHERE ("DataObjectTest_Team"."ClassName" IN ('.$db->prepStringForDB('DataObjectTest_SubTeam').'))' .
 			' ORDER BY "DataObjectTest_Team"."Title" ASC';
 		$this->assertEquals($expected, $playerList->sql());
@@ -62,7 +63,8 @@ class DataObjectLazyLoadingTest extends SapphireTest {
 		$expected = 'SELECT DISTINCT "DataObjectTest_Team"."ClassName", "DataObjectTest_Team"."Created", ' .
 			'"DataObjectTest_Team"."LastEdited", "DataObjectTest_Team"."Title", "DataObjectTest_Team"."ID", ' .
 			'CASE WHEN "DataObjectTest_Team"."ClassName" IS NOT NULL THEN "DataObjectTest_Team"."ClassName" ELSE ' .
-			$db->prepStringForDB('DataObjectTest_Team').' END AS "RecordClassName" FROM "DataObjectTest_Team" WHERE ' .
+			$db->prepStringForDB('DataObjectTest_Team').' END AS "RecordClassName" FROM "DataObjectTest_Team" ' . 
+			'LEFT JOIN "DataObjectTest_SubTeam" ON "DataObjectTest_SubTeam"."ID" = "DataObjectTest_Team"."ID" WHERE ' .
 			'("DataObjectTest_Team"."ClassName" IN ('.$db->prepStringForDB('DataObjectTest_SubTeam').')) ' .
 			'ORDER BY "DataObjectTest_Team"."Title" ASC';
 		$this->assertEquals($expected, $playerList->sql());

tests/model/DataQueryTest.php

@@ -2,10 +2,14 @@
 
 class DataQueryTest extends SapphireTest {
 	
+	protected static $fixture_file = 'DataQueryTest.yml';
+
 	protected $extraDataObjects = array(
 		'DataQueryTest_A',
 		'DataQueryTest_B',
+		'DataQueryTest_C',
 		'DataQueryTest_D',
+		'DataQueryTest_E',
 	);
 
 	/**
@@ -124,6 +128,12 @@ class DataQueryTest extends SapphireTest {
 			$dq->sql()
 		);
 	}
+	
+	public function testDefaultSort() {
+		$query = new DataQuery('DataQueryTest_E');
+		$result = $query->column('Title');
+		$this->assertEquals(array('First', 'Second', 'Last'), $result);
+	}
 }
 
 
@@ -149,6 +159,10 @@ class DataQueryTest_B extends DataQueryTest_A {
 
 class DataQueryTest_C extends DataObject implements TestOnly {
 	
+	private static $db = array(
+		'Title' => 'Varchar'
+	);
+
 	private static $has_one = array(
 		'TestA' => 'DataQueryTest_A',
 		'TestB' => 'DataQueryTest_B',
@@ -171,3 +185,12 @@ class DataQueryTest_D extends DataObject implements TestOnly {
 		'Relation' => 'DataQueryTest_B',
 	);
 }
+
+class DataQueryTest_E extends DataQueryTest_C implements TestOnly {
+	
+	private static $db = array(
+		'SortOrder' => 'Int'
+	);
+	
+	private static $default_sort = '"DataQueryTest_E"."SortOrder" ASC';
+}

tests/model/DataQueryTest.yml

@@ -0,0 +1,10 @@
+DataQueryTest_E:
+  query1:
+    Title: 'Last'
+    SortOrder: 3
+  query2:
+    Title: 'First'
+    SortOrder: 1
+  query3:
+    Title: 'Second'
+    SortOrder: 2