Skip to content

Commit

Permalink
BUGFIX: Fixed errors caused by complex raw SQL sort() calls. (#7236)
Browse files Browse the repository at this point in the history
  • Loading branch information
Sam Minnee committed May 1, 2012
1 parent 5abf8cf commit a8e8a60
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 25 deletions.
14 changes: 12 additions & 2 deletions model/DataList.php
View file
Expand Up @@ -189,7 +189,13 @@ public function sort() {
$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);
}
}

Expand Down Expand Up @@ -250,12 +256,16 @@ public function filter() {

/**
* 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.'"';
}
Expand Down
16 changes: 12 additions & 4 deletions model/DataQuery.php
View file
Expand Up @@ -229,7 +229,7 @@ function getFinalisedQuery($queriedColumns = null) {
* @param SQLQuery $query
* @return null
*/
protected function ensureSelectContainsOrderbyColumns($query) {
protected function ensureSelectContainsOrderbyColumns($query, $originalSelect = array()) {
$tableClasses = ClassInfo::dataClassesFor($this->dataClass);
$baseClass = array_shift($tableClasses);

Expand All @@ -239,8 +239,13 @@ protected function ensureSelectContainsOrderbyColumns($query) {
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);
Expand Down Expand Up @@ -616,8 +621,11 @@ public function selectFromTable($table, $fields) {
*/
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);
}
Expand Down
36 changes: 24 additions & 12 deletions model/SQLQuery.php
View file
Expand Up @@ -343,30 +343,26 @@ public function orderby($clauses = null, $direction = null, $clear = true) {
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;
}
}

if(is_array($clauses)) {
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 {
Expand All @@ -380,9 +376,9 @@ public function orderby($clauses = null, $direction = null, $clear = true) {
// 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]);

Expand All @@ -399,6 +395,22 @@ public function orderby($clauses = null, $direction = null, $clear = true) {
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,
Expand Down
14 changes: 14 additions & 0 deletions tests/model/DataListTest.php
View file
Expand Up @@ -515,4 +515,18 @@ public function testReverse() {
$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"));
}
}
14 changes: 7 additions & 7 deletions tests/model/SQLQueryTest.php
View file
Expand Up @@ -102,7 +102,7 @@ function testSelectWithOrderbyClause() {
$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";
Expand All @@ -117,7 +117,7 @@ function testSelectWithOrderbyClause() {
$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";
Expand All @@ -127,23 +127,23 @@ function testSelectWithOrderbyClause() {
$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() {
Expand Down Expand Up @@ -174,7 +174,7 @@ public function testReverseOrderBy() {
$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() {
Expand Down

0 comments on commit a8e8a60

Please sign in to comment.