From a8e8a6060af0ecd54f0aaa80d7afa43307db595c Mon Sep 17 00:00:00 2001
From: Sam Minnee <sam@silverstripe.com>
Date: Tue, 1 May 2012 17:09:57 +1200
Subject: [PATCH] BUGFIX: Fixed errors caused by complex raw SQL sort() calls.
 (#7236)

---
 model/DataList.php           | 14 ++++++++++++--
 model/DataQuery.php          | 16 ++++++++++++----
 model/SQLQuery.php           | 36 ++++++++++++++++++++++++------------
 tests/model/DataListTest.php | 14 ++++++++++++++
 tests/model/SQLQueryTest.php | 14 +++++++-------
 5 files changed, 69 insertions(+), 25 deletions(-)

diff --git a/model/DataList.php b/model/DataList.php
index 0715a8176..1c6f16bab 100644
--- a/model/DataList.php
+++ b/model/DataList.php
@@ -189,7 +189,13 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 			$this->dataQuery->sort(null, null); // wipe the sort
 			
 			foreach(func_get_arg(0) as $col => $dir) {
-				$this->dataQuery->sort($this->getRelationName($col), $dir, false);
+				// Convert column expressions to SQL fragment, while still allowing the passing of raw SQL fragments.
+				try {
+					$relCol = $this->getRelationName($col);
+				} catch(InvalidArgumentException $e) {
+					$relCol = $col;
+				}
+				$this->dataQuery->sort($relCol, $dir, false);
 			}
 		}
 		
@@ -250,12 +256,16 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab
 
 	/**
 	 * Translates a Object relation name to a Database name and apply the relation join to 
-	 * the query
+	 * the query.  Throws an InvalidArgumentException if the $field doesn't correspond to a relation
 	 *
 	 * @param string $field
 	 * @return string
 	 */
 	public function getRelationName($field) {
+		if(!preg_match('/^[A-Z0-9._]+$/i', $field)) {
+			throw new InvalidArgumentException("Bad field expression $field");
+		}
+		
 		if(strpos($field,'.') === false) {
 			return '"'.$field.'"';
 		}
diff --git a/model/DataQuery.php b/model/DataQuery.php
index f607a7840..1759c60e4 100644
--- a/model/DataQuery.php
+++ b/model/DataQuery.php
@@ -229,7 +229,7 @@ class DataQuery {
 	 * @param SQLQuery $query
 	 * @return null
 	 */
-	protected function ensureSelectContainsOrderbyColumns($query) {
+	protected function ensureSelectContainsOrderbyColumns($query, $originalSelect = array()) {
 		$tableClasses = ClassInfo::dataClassesFor($this->dataClass);
 		$baseClass = array_shift($tableClasses);
 
@@ -239,8 +239,13 @@ class DataQuery {
 			foreach($orderby as $k => $dir) {
 				// don't touch functions in the ORDER BY or function calls 
 				// selected as fields
-				if(strpos($k, '(') !== false || preg_match('/_SortColumn/', $k)) 
+				if(strpos($k, '(') !== false) continue;
+				
+				// Pull through SortColumn references from the originalSelect variables
+				if(preg_match('/_SortColumn/', $k)) {
+					if(isset($originalSelect[$k])) $query->selectField($originalSelect[$k], $k);
 					continue;
+				}
 				
 				$col = str_replace('"', '', trim($k));
 				$parts = explode('.', $col);
@@ -616,8 +621,11 @@ class DataQuery {
 	 */
 	public function column($field = 'ID') {
 		$query = $this->getFinalisedQuery(array($field));
-		$query->select($this->expressionForField($field, $query));
-		$this->ensureSelectContainsOrderbyColumns($query);
+		$originalSelect = $query->itemisedSelect();
+		$fieldExpression = $this->expressionForField($field, $query);
+		$query->clearSelect();
+		$query->selectField($fieldExpression);
+		$this->ensureSelectContainsOrderbyColumns($query, $originalSelect);
 
 		return $query->execute()->column($field);
 	}
diff --git a/model/SQLQuery.php b/model/SQLQuery.php
index 47dc9a660..6ce1ad459 100644
--- a/model/SQLQuery.php
+++ b/model/SQLQuery.php
@@ -343,13 +343,12 @@ class SQLQuery {
 			else {
 				$sort = explode(",", $clauses);
 			}
-			
+
 			$clauses = array();
 			
 			foreach($sort as $clause) {
-				$clause = explode(" ", trim($clause));
-				$dir = (isset($clause[1])) ? $clause[1] : $direction;
-				$clauses[$clause[0]] = $dir;
+				list($column, $direction) = $this->getDirectionFromString($clause, $direction);
+				$clauses[$column] = $direction;
 			}
 		}
 		
@@ -357,16 +356,13 @@ class SQLQuery {
 			foreach($clauses as $key => $value) {
 				if(!is_numeric($key)) {
 					$column = trim($key);
-					$direction = strtoupper(trim($value));
+					$columnDir = strtoupper(trim($value));
 				}
 				else {
-					$clause = explode(" ", trim($value));
-					
-					$column = trim($clause[0]);
-					$direction = (isset($clause[1])) ? strtoupper(trim($clause[1])) : "";
+					list($column, $columnDir) = $this->getDirectionFromString($value);
 				}
 				
-				$this->orderby[$column] = $direction;
+				$this->orderby[$column] = $columnDir;
 			}
 		}
 		else {
@@ -380,9 +376,9 @@ class SQLQuery {
 		// directly in the ORDER BY
 		if($this->orderby) {
 			$i = 0;
-			
 			foreach($this->orderby as $clause => $dir) {
-				if(strpos($clause, '(') !== false) {
+				// Function calls and multi-word columns like "CASE WHEN ..."
+				if(strpos($clause, '(') !== false || strpos($clause, " ") !== false ) {
 					// remove the old orderby
 					unset($this->orderby[$clause]);
 					
@@ -399,6 +395,22 @@ class SQLQuery {
 		return $this;
 	}
 	
+	/**
+	 * Extract the direction part of a single-column order by clause.
+	 * 
+	 * Return a 2 element array: array($column, $direction)
+	 */
+	private function getDirectionFromString($value, $defaultDirection = null) {
+		if(preg_match('/^(.*)(asc|desc)$/i', $value, $matches)) {
+			$column = trim($matches[1]);
+			$direction = strtoupper($matches[2]);
+		} else {
+			$column = $value;
+			$direction = $defaultDirection ? $defaultDirection : "ASC";
+		}
+		return array($column, $direction);
+	}
+
 	/**
 	 * Returns the current order by as array if not already. To handle legacy
 	 * statements which are stored as strings. Without clauses and directions, 
diff --git a/tests/model/DataListTest.php b/tests/model/DataListTest.php
index 802b883e2..45ce64531 100755
--- a/tests/model/DataListTest.php
+++ b/tests/model/DataListTest.php
@@ -515,4 +515,18 @@ class DataListTest extends SapphireTest {
 		$this->assertEquals('Bob', $list->last()->Name, 'Last comment should be from Bob');
 		$this->assertEquals('Phil', $list->first()->Name, 'First comment should be from Phil');
 	}
+
+	public function testSortByComplexExpression() {
+		// Test an expression with both spaces and commas
+		// This test also tests that column() can be called with a complex sort expression, so keep using column() below
+		$list = DataObjectTest_Team::get()->sort('CASE WHEN "DataObjectTest_Team"."ClassName" = \'DataObjectTest_SubTeam\' THEN 0 ELSE 1 END, "Title" DESC');
+		$this->assertEquals(array(
+			'Subteam 3',
+			'Subteam 2',
+			'Subteam 1',
+			'Team 3',
+			'Team 2',
+			'Team 1',
+		), $list->column("Title"));
+	}
 }
diff --git a/tests/model/SQLQueryTest.php b/tests/model/SQLQueryTest.php
index 3591f95af..3c4018b15 100755
--- a/tests/model/SQLQueryTest.php
+++ b/tests/model/SQLQueryTest.php
@@ -102,7 +102,7 @@ class SQLQueryTest extends SapphireTest {
 		$query = new SQLQuery();
 		$query->from[] = "MyTable";
 		$query->orderby('MyName');
-		$this->assertEquals('SELECT * FROM MyTable ORDER BY MyName', $query->sql());
+		$this->assertEquals('SELECT * FROM MyTable ORDER BY MyName ASC', $query->sql());
 		
 		$query = new SQLQuery();
 		$query->from[] = "MyTable";
@@ -117,7 +117,7 @@ class SQLQueryTest extends SapphireTest {
 		$query = new SQLQuery();
 		$query->from[] = "MyTable";
 		$query->orderby('MyName ASC, Color');
-		$this->assertEquals('SELECT * FROM MyTable ORDER BY MyName ASC, Color', $query->sql());
+		$this->assertEquals('SELECT * FROM MyTable ORDER BY MyName ASC, Color ASC', $query->sql());
 
 		$query = new SQLQuery();
 		$query->from[] = "MyTable";
@@ -127,23 +127,23 @@ class SQLQueryTest extends SapphireTest {
 		$query = new SQLQuery();
 		$query->from[] = "MyTable";
 		$query->orderby(array('MyName' => 'desc', 'Color'));
-		$this->assertEquals('SELECT * FROM MyTable ORDER BY MyName DESC, Color', $query->sql());
+		$this->assertEquals('SELECT * FROM MyTable ORDER BY MyName DESC, Color ASC', $query->sql());
 		
 		$query = new SQLQuery();
 		$query->from[] = "MyTable";
 		$query->orderby('implode("MyName","Color")');
-		$this->assertEquals('SELECT implode("MyName","Color") AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0', $query->sql());
+		$this->assertEquals('SELECT *, implode("MyName","Color") AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0 ASC', $query->sql());
 		
 		$query = new SQLQuery();
 		$query->from[] = "MyTable";
 		$query->orderby('implode("MyName","Color") DESC');
-		$this->assertEquals('SELECT implode("MyName","Color") AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0 DESC', $query->sql());
+		$this->assertEquals('SELECT *, implode("MyName","Color") AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0 DESC', $query->sql());
 		
 		$query = new SQLQuery();
 		$query->from[] = "MyTable";
 		$query->orderby('RAND()');
 		
-		$this->assertEquals('SELECT RAND() AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0', $query->sql());
+		$this->assertEquals('SELECT *, RAND() AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0 ASC', $query->sql());
 	}
 	
 	public function testReverseOrderBy() {
@@ -174,7 +174,7 @@ class SQLQueryTest extends SapphireTest {
 		$query->orderby('implode("MyName","Color") DESC');
 		$query->reverseOrderBy();
 		
-		$this->assertEquals('SELECT implode("MyName","Color") AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0 ASC',$query->sql());
+		$this->assertEquals('SELECT *, implode("MyName","Color") AS "_SortColumn0" FROM MyTable ORDER BY _SortColumn0 ASC',$query->sql());
 	}
 
 	function testFiltersOnID() {