Skip to content

Commit

Permalink
API Create sortRaw() method to handle raw SQL
Browse files Browse the repository at this point in the history
  • Loading branch information
@emteknetnz
emteknetnz committed Dec 1, 2022
1 parent 7860e46 commit 083e7bd
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 42 deletions.
99 changes: 63 additions & 36 deletions src/ORM/DataList.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use Exception;
use InvalidArgumentException;
use LogicException;
use SilverStripe\ORM\Connect\DatabaseException;

/**
* Implements a "lazy loading" DataObjectSet.
Expand Down Expand Up @@ -237,8 +238,6 @@ public function whereAny($filter)
});
}



/**
* Returns true if this DataList can be sorted by the given field.
*
Expand Down Expand Up @@ -308,69 +307,97 @@ public function distinct($value)
}

/**
* Return a new DataList instance as a copy of this data list with the sort
* order set.
* Return a new DataList instance as a copy of this data list with the sort order set
* Raw SQL is not accepted, only actual columns can be passed
*
* @see SS_List::sort()
* @see SQLSelect::orderby
* @param string|array $args
* @example $list = $list->sort('Name'); // default ASC sorting
* @example $list = $list->sort('Name DESC'); // DESC sorting
* @example $list = $list->sort('Name ASC, Age DESC');
* @example $list = $list->sort('Name', 'ASC');
* @example $list = $list->sort(array('Name'=>'ASC', 'Age'=>'DESC'));
*
* @param string|array Escaped SQL statement. If passed as array, all keys and values are assumed to be escaped.
* @return static
* @example $list = $list->sort(['Name' => 'ASC', 'Age' => 'DESC']);
*/
public function sort()
public function sort(...$args): static
{
$count = func_num_args();

$count = count($args);
if ($count == 0) {
return $this;
}

if ($count > 2) {
throw new InvalidArgumentException('This method takes zero, one or two arguments');
}

if ($count == 2) {
$col = null;
$dir = null;
list($col, $dir) = func_get_args();

// Validate direction
if (!in_array(strtolower($dir ?? ''), ['desc', 'asc'])) {
user_error('Second argument to sort must be either ASC or DESC');
}

$sort = [$col => $dir];
list($column, $direction) = $args;
$sort = [$column => $direction];
} else {
$sort = func_get_arg(0);
$sort = $args[0];
// If $sort is string then convert string to array to allow for validation
if (is_string($sort)) {
$newSort = [];
// Making the assumption here there are no commas in column names
// Other parts of silverstripe will break if there are commas in column names
foreach (explode(',', $sort) as $colDir) {
if (empty($colDir)) {
continue;
}
// Using regex instead of explode(' ') in case column name includes spaces
if (preg_match('/^(.+) (asc|desc)$/i', trim($colDir), $matches)) {
list($column, $direction) = [$matches[1], $matches[2]];
} else {
list($column, $direction) = [$colDir, 'ASC'];
}
$newSort[$column] = $direction;
}
$sort = $newSort;
}
}
// Validate that column names are not raw sql
if (is_array($sort)) {
foreach (array_keys($sort) as $column) {
$this->validateColumnHasNoRawSql($column);
}
}

return $this->alterDataQuery(function (DataQuery $query, DataList $list) use ($sort) {

if (is_string($sort) && $sort) {
if (false !== stripos($sort ?? '', ' asc') || false !== stripos($sort ?? '', ' desc')) {
if (preg_match('/^(.+) (asc|desc)$/i', $sort)) {
$query->sort($sort);
} else {
$list->applyRelation($sort, $column, true);
$query->sort($column, 'ASC');
}
} elseif (is_array($sort)) {
// sort(array('Name'=>'desc'));
$query->sort(null, null); // wipe the sort

// Wipe the sort
$query->sort(null, null);
foreach ($sort as $column => $direction) {
// Convert column expressions to SQL fragment, while still allowing the passing of raw SQL
// fragments.
$list->applyRelation($column, $relationColumn, true);
$query->sort($relationColumn, $direction, false);
}
}
});
}

private function validateColumnHasNoRawSql(string $column): void
{
$col = trim(str_replace('"', '', $column));
// Search for non-alphanumeric characters in the column name rather than ^[^a-zA-Z0-9_]$ to account
// for column names that include macrons etc
if (preg_match('#[\(\)\[\]{}\+\-\*/=\:\?\\;|&\'"`<>^%, ]#', $col)) {
throw new InvalidArgumentException("Invalid column name $column");
}
}

/**
* Set an explicit ORDER BY statement using raw SQL
*
* Note: this may be vulnerable to SQL injection so most of the time you should
* use sort() instead which doesn't allow raw SQL
*/
public function orderBy(string $orderBy): static
{
return $this->alterDataQuery(function (DataQuery $query) use ($orderBy) {
$query->sort($orderBy, null, true);
});
}

/**
* Return a copy of this list which only includes items with these characteristics
*
Expand Down Expand Up @@ -1190,7 +1217,7 @@ public function removeByFilter($filter)
*/
public function shuffle()
{
return $this->sort(DB::get_conn()->random());
return $this->orderBy(DB::get_conn()->random());
}

/**
Expand Down
3 changes: 3 additions & 0 deletions src/ORM/DataQuery.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -716,12 +716,15 @@ public function whereAny($filter)
/**
* Set the ORDER BY clause of this query
*
* Note: while the similarly named DataList::sort() does not allow raw SQL, DataQuery::sort() does allow it
*
* @see SQLSelect::orderby()
*
* @param string $sort Column to sort on (escaped SQL statement)
* @param string $direction Direction ("ASC" or "DESC", escaped SQL statement)
* @param bool $clear Clear existing values
* @return $this
*
*/
public function sort($sort = null, $direction = null, $clear = true)
{
Expand Down
67 changes: 65 additions & 2 deletions tests/php/ORM/DataListTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@

namespace SilverStripe\ORM\Tests;

use Exception;
use InvalidArgumentException;
use SilverStripe\Core\Convert;
use SilverStripe\Core\Injector\InjectorNotFoundException;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\ORM\Connect\MySQLiConnector;
use SilverStripe\ORM\DataList;
use SilverStripe\ORM\DataQuery;
use SilverStripe\ORM\DB;
Expand Down Expand Up @@ -1816,12 +1818,12 @@ public function testReverse()
$this->assertEquals('Phil', $list->first()->Name, 'First comment should be from Phil');
}

public function testSortByComplexExpression()
public function testOrderByComplexExpression()
{
// 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
$teamClass = Convert::raw2sql(SubTeam::class);
$list = Team::get()->sort(
$list = Team::get()->orderBy(
'CASE WHEN "DataObjectTest_Team"."ClassName" = \'' . $teamClass . '\' THEN 0 ELSE 1 END, "Title" DESC'
);
$this->assertEquals(
Expand All @@ -1837,6 +1839,67 @@ public function testSortByComplexExpression()
);
}

/**
* @dataProvider provideRawSqlSortException
*/
public function testRawSqlSort(string $sort, string $type): void
{
if ($type === 'valid') {
$this->expectNotToPerformAssertions();
} elseif ($type === 'unknown-column') {
if (!(DB::get_conn()->getConnector() instanceof MySQLiConnector)) {
$this->markTestSkipped('Database connector is not MySQLiConnector');
}
$this->expectException(\mysqli_sql_exception::class);
$this->expectExceptionMessageMatches('/Unknown column/');
} else {
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessageMatches('/Invalid column name/');
}
// column('ID') is required to get the database query be actually fired off
Team::get()->sort($sort)->column('ID');
}

/**
* @dataProvider provideRawSqlSortException
*/
public function testRawSqlOrderBy(string $sort, string $type): void
{
if ($type === 'valid' || $type === 'order-by-only') {
$this->expectNotToPerformAssertions();
} else {
if (!(DB::get_conn()->getConnector() instanceof MySQLiConnector)) {
$this->markTestSkipped('Database connector is not MySQLiConnector');
}
$this->expectException(\mysqli_sql_exception::class);
if ($sort === 'Title BACKWARDS') {
$this->expectExceptionMessageMatches('/You have an error in your SQL syntax/');
} else {
$this->expectExceptionMessageMatches('/Unknown column/');
}
}
// column('ID') is required to get the database query be actually fired off
Team::get()->orderBy($sort)->column('ID');
}

public function provideRawSqlSortException(): array
{
return [
['Title', 'valid'],
['Title asc', 'valid'],
['"Title" ASC', 'valid'],
['Title ASC, "DatabaseField"', 'valid'],
['"Title", "DatabaseField" DESC', 'valid'],
['Title BACKWARDS', 'invalid'],
['NonExistentColumn', 'unknown-column'],
['"Strange non-existent column name"', 'invalid'],
['Title, 1 = 1', 'order-by-only'],
["Title,'abc' = 'abc'", 'order-by-only'],
['Title,Mod(ID,3)=1', 'order-by-only'],
['(CASE WHEN ID < 3 THEN 1 ELSE 0 END)', 'order-by-only'],
];
}

public function testShuffle()
{
$list = Team::get()->shuffle();
Expand Down
8 changes: 4 additions & 4 deletions tests/php/ORM/DataObjectTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1034,10 +1034,10 @@ public function testRandomSort()
$this->assertFalse($obj->isChanged());

/* If we perform the same random query twice, it shouldn't return the same results */
$itemsA = DataObject::get(DataObjectTest\TeamComment::class, "", DB::get_conn()->random());
$itemsB = DataObject::get(DataObjectTest\TeamComment::class, "", DB::get_conn()->random());
$itemsC = DataObject::get(DataObjectTest\TeamComment::class, "", DB::get_conn()->random());
$itemsD = DataObject::get(DataObjectTest\TeamComment::class, "", DB::get_conn()->random());
$itemsA = DataObject::get(DataObjectTest\TeamComment::class, "")->orderBy(DB::get_conn()->random());
$itemsB = DataObject::get(DataObjectTest\TeamComment::class)->orderBy(DB::get_conn()->random());
$itemsC = DataObject::get(DataObjectTest\TeamComment::class)->orderBy(DB::get_conn()->random());
$itemsD = DataObject::get(DataObjectTest\TeamComment::class)->orderBy(DB::get_conn()->random());
foreach ($itemsA as $item) {
$keysA[] = $item->ID;
}
Expand Down

0 comments on commit 083e7bd

Please sign in to comment.