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 f0c4fd0
Show file tree
Hide file tree
Showing 3 changed files with 154 additions and 26 deletions.
111 changes: 87 additions & 24 deletions src/ORM/DataList.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,10 @@ 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.
*
* sort() does not allow the use of raw SQL as a safe quard against SQL injection attacks
* Use sortRaw() instead if you must use raw SQL
*
* @see SS_List::sort()
* @see SQLSelect::orderby
Expand All @@ -321,46 +323,83 @@ public function distinct($value)
* @param string|array Escaped SQL statement. If passed as array, all keys and values are assumed to be escaped.
* @return static
*/
public function sort()
public function sort(...$args)
{
return $this->sortInner(false, $args);
}

/**
* Return a new DataList instance as a copy of this data list with the sort order set
*
* The methid is the same as sort(), though allows raw SQL to be used for sorting
* This method is open to SQL injection attacks so use with caution
*
* @see SS_List::sort()
* @see SQLSelect::orderby
* @example $list = $list->sort('Name'); // default ASC sorting
* @example $list = $list->sort('Mod(ID, 3) ASC, Name ASC'); // Raw SQL sort
* @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
*/
public function sortRaw(...$args)
{
$count = func_num_args();
return $this->sortInner(true, $args);
}

private function sortInner(bool $allowRawSql, array $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 and not allowing raw sql, convert string to array to allow for validation
if (is_string($sort) && !$allowRawSql) {
$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) {
// 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;
}
}
// validation
if (is_array($sort)) {
foreach ($sort as $column => $direction) {
if (!$allowRawSql) {
$this->validateColumnNotRawSQL($column);
}
$this->validateDirection($direction);
}
}

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.
Expand All @@ -371,6 +410,30 @@ public function sort()
});
}

private function validateDirection(string $direction): void
{
$dir = strtolower($direction);
if ($dir !== 'asc' && $dir !== 'desc') {
throw new InvalidArgumentException('Second argument to sort must be either ASC or DESC');
}
}

private function validateColumnNotRawSQL(string $column): void
{
// Remove anything surronding by double quotes as it's used my MySQLiConnector and postgres native
// to quote table and column names. This does allow strange characters in column names such as spaces
preg_match_all('#"([^"]+)"#', $column, $matches);
foreach ($matches[1] as $match) {
$column = preg_replace('#"' . preg_quote($match, '#') . '"#', '', $column);
}
$column = trim($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('#[\(\)\[\]{}\+\-\*/=\:\?\\;|&\'"`<>^%, ]#', $column)) {
throw new InvalidArgumentException('Raw SQL cannot be used for sort(), use sortRaw() instead');
}
}

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

/**
Expand Down
1 change: 1 addition & 0 deletions src/ORM/DataQuery.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,7 @@ public function whereAny($filter)
* @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
68 changes: 66 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 testSortByRawComplexExpression()
{
// 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()->sortRaw(
'CASE WHEN "DataObjectTest_Team"."ClassName" = \'' . $teamClass . '\' THEN 0 ELSE 1 END, "Title" DESC'
);
$this->assertEquals(
Expand All @@ -1837,6 +1839,68 @@ public function testSortByComplexExpression()
);
}

/**
* @dataProvider provideRawSqlSortException
*/
public function testRawSqlSortException(string $sort, string $failureType): void
{
if ($failureType === 'raw-sql') {
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('Raw SQL cannot be used for sort(), use sortRaw() instead');
} elseif ($failureType === 'unknown-column') {
if (!(DB::get_conn()->getConnector() instanceof MySQLiConnector)) {
$this->markTestSkipped('Database connector is not MySQLiConnector');
}
// It's surprising this is mysqli_sql_exception when it probably should be DatabaseException
$this->expectException(\mysqli_sql_exception::class);
$this->expectExceptionMessageMatches('/Unknown column/');
} elseif ($failureType === 'valid') {
$this->expectNotToPerformAssertions();
} else {
throw new Exception('Invalid $failureType');
}
// ->column('ID') is required to get the database query be actually fired off
Team::get()->sort($sort)->column('ID');
}

/**
* @dataProvider provideRawSqlSortException
*/
public function testRawSqlSortRawNoException(string $sort, string $failureType): void
{
if ($failureType === 'unknown-column') {
if (!(DB::get_conn()->getConnector() instanceof MySQLiConnector)) {
$this->markTestSkipped('Database connector is not MySQLiConnector');
}
// It's surprising this is mysqli_sql_exception when it probably should be DatabaseException
$this->expectException(\mysqli_sql_exception::class);
$this->expectExceptionMessageMatches('/Unknown column/');
} elseif ($failureType === 'valid' || $failureType == 'raw-sql') {
$this->expectNotToPerformAssertions();
} else {
throw new Exception('Invalid $failureType');
}
// ->column('ID') is required to get the database query be actually fired off
Team::get()->sortRaw($sort)->column('ID');
}

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

public function testShuffle()
{
$list = Team::get()->shuffle();
Expand Down

0 comments on commit f0c4fd0

Please sign in to comment.