Merge pull request #10600 from creative-commoners/pulls/5/sortraw

API Create orderBy() method to handle raw SQL

src/ORM/DataList.php

@@ -203,10 +203,14 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
     /**
      * Return a new DataList instance with a WHERE clause added to this list's query.
      *
+     * This method accepts raw SQL so could be vulnerable to SQL injection attacks if used incorrectly,
+     * it's preferable to use filter() instead which does not allow raw SQL.
+     *
      * Supports parameterised queries.
      * See SQLSelect::addWhere() for syntax examples, although DataList
      * won't expand multiple method arguments as SQLSelect does.
      *
+     *
      * @param string|array|SQLConditionGroup $filter Predicate(s) to set, as escaped SQL statements or
      * paramaterised queries
      * @return static
@@ -222,6 +226,9 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
      * Return a new DataList instance with a WHERE clause added to this list's query.
      * All conditions provided in the filter will be joined with an OR
      *
+     * This method accepts raw SQL so could be vulnerable to SQL injection attacks if used incorrectly,
+     * it's preferable to use filterAny() instead which does not allow raw SQL
+     *
      * Supports parameterised queries.
      * See SQLSelect::addWhere() for syntax examples, although DataList
      * won't expand multiple method arguments as SQLSelect does.
@@ -237,8 +244,6 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
         });
     }
 
-
-
     /**
      * Returns true if this DataList can be sorted by the given field.
      *
@@ -308,72 +313,118 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
     }
 
     /**
-     * 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
      *
-     * @see SS_List::sort()
-     * @see SQLSelect::orderby
+     * Raw SQL is not accepted, only actual field names can be passed
+     *
+     * @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']);
+     * @example $list = $list->sort('MyRelation.MyColumn ASC')
+     * @example $list->sort(null); // wipe any existing sort
      */
-    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);
-        }
-
-        return $this->alterDataQuery(function (DataQuery $query, DataList $list) use ($sort) {
-
-            if (is_string($sort) && $sort) {
-                if (false !== stripos($sort ?? '', ' asc') || false !== stripos($sort ?? '', ' desc')) {
-                    $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
-
-                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);
-                }
+            $sort = $args[0];
+            if (!is_string($sort) && !is_array($sort) && !is_null($sort)) {
+                throw new InvalidArgumentException('sort() arguments must either be a string, an array, or null');
             }
+            if (is_null($sort)) {
+                // Set an an empty array here to cause any existing sort on the DataLists to be wiped
+                // later on in this method
+                $sort = [];
+            } elseif (empty($sort)) {
+                throw new InvalidArgumentException('Invalid sort parameter');
+            }
+            // 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) {
+                    // Using regex instead of explode(' ') in case column name includes spaces
+                    if (preg_match('/^(.+) ([^"]+)$/i', trim($colDir), $matches)) {
+                        list($column, $direction) = [$matches[1], $matches[2]];
+                    } else {
+                        list($column, $direction) = [$colDir, 'ASC'];
+                    }
+                    $newSort[$column] = $direction;
+                }
+                $sort = $newSort;
+            }
+        }
+        foreach ($sort as $column => $direction) {
+            $this->validateSortColumn($column);
+            $this->validateSortDirection($direction);
+        }
+        return $this->alterDataQuery(function (DataQuery $query, DataList $list) use ($sort) {
+            // Wipe the sort
+            $query->sort(null, null);
+            foreach ($sort as $column => $direction) {
+                $list->applyRelation($column, $relationColumn, true);
+                $query->sort($relationColumn, $direction, false);
+            }
+        });
+    }
+
+    private function validateSortColumn(string $column): void
+    {
+        $col = trim($column);
+        // Strip double quotes from single field names e.g. '"Title"'
+        if (preg_match('#^"[^"]+"$#', $col)) {
+            $col = str_replace('"', '', $col);
+        }
+        // $columnName is a param that is passed by reference so is essentially as a return type
+        // it will be returned in quoted SQL "TableName"."ColumnName" notation
+        // if it's equal to $col however it means that it WAS orginally raw sql, which is disallowed for sort()
+        //
+        // applyRelation() will also throw an InvalidArgumentException if $column is not raw sql but
+        // the Relation.FieldName is not a valid model relationship
+        $this->applyRelation($col, $columnName, true);
+        if ($col === $columnName) {
+            throw new InvalidArgumentException("Invalid sort column $column");
+        }
+    }
+
+    private function validateSortDirection(string $direction): void
+    {
+        $dir = strtolower($direction);
+        if ($dir !== 'asc' && $dir !== 'desc') {
+            throw new InvalidArgumentException("Invalid sort direction $direction");
+        }
+    }
+
+    /**
+     * Set an explicit ORDER BY statement using raw SQL
+     *
+     * This method accepts raw SQL so could be vulnerable to SQL injection attacks if used incorrectly,
+     * it's preferable to use sort() instead which does not 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
      *
+     * Raw SQL is not accepted, only actual field names can be passed
+     *
      * @see Filterable::filter()
      *
      * @example $list = $list->filter('Name', 'bob'); // only bob in the list
@@ -433,6 +484,8 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
     /**
      * Return a copy of this list which contains items matching any of these characteristics.
      *
+     * Raw SQL is not accepted, only actual field names can be passed
+     *
      * @example // only bob in the list
      *          $list = $list->filterAny('Name', 'bob');
      *          // SQL: WHERE "Name" = 'bob'
@@ -564,7 +617,7 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
      */
     protected function isValidRelationName($field)
     {
-        return preg_match('/^[A-Z0-9._]+$/i', $field ?? '');
+        return preg_match('/^[A-Z0-9\._]+$/i', $field ?? '');
     }
 
     /**
@@ -608,6 +661,8 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
     /**
      * Return a copy of this list which does not contain any items that match all params
      *
+     * Raw SQL is not accepted, only actual field names can be passed
+     *
      * @example $list = $list->exclude('Name', 'bob'); // exclude bob from list
      * @example $list = $list->exclude('Name', array('aziz', 'bob'); // exclude aziz and bob from list
      * @example $list = $list->exclude(array('Name'=>'bob, 'Age'=>21)); // exclude bob that has Age 21
@@ -648,6 +703,8 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
     /**
      * Return a copy of this list which does not contain any items with any of these params
      *
+     * Raw SQL is not accepted, only actual field names can be passed
+     *
      * @example $list = $list->excludeAny('Name', 'bob'); // exclude bob from list
      * @example $list = $list->excludeAny('Name', array('aziz', 'bob'); // exclude aziz and bob from list
      * @example $list = $list->excludeAny(array('Name'=>'bob, 'Age'=>21)); // exclude bob or Age 21
@@ -1190,7 +1247,7 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
      */
     public function shuffle()
     {
-        return $this->sort(DB::get_conn()->random());
+        return $this->orderBy(DB::get_conn()->random());
     }
 
     /**

src/ORM/DataObject.php

@@ -2254,7 +2254,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
         // If we have a default sort set for our "join" then we should overwrite any default already set.
         $joinSort = Config::inst()->get($manyManyComponent['join'], 'default_sort');
         if (!empty($joinSort)) {
-            $result = $result->sort($joinSort);
+            $result = $result->orderBy($joinSort);
         }
 
         $this->extend('updateManyManyComponents', $result);
@@ -3327,7 +3327,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
      * @param string $callerClass The class of objects to be returned
      * @param string|array $filter A filter to be inserted into the WHERE clause.
      * Supports parameterised queries. See SQLSelect::addWhere() for syntax examples.
-     * @param string|array $sort A sort expression to be inserted into the ORDER
+     * @param string|array|null $sort Passed to DataList::sort()
      * BY clause.  If omitted, self::$default_sort will be used.
      * @param string $join Deprecated 3.0 Join clause. Use leftJoin($table, $joinClause) instead.
      * @param string|array $limit A limit expression to be inserted into the LIMIT clause.
@@ -3369,7 +3369,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
         if ($filter) {
             $result = $result->where($filter);
         }
-        if ($sort) {
+        if ($sort || is_null($sort)) {
             $result = $result->sort($sort);
         }
         if ($limit && strpos($limit ?? '', ',') !== false) {
@@ -3399,11 +3399,11 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
      * e.g. MyObject::get_one() will return a MyObject
      * @param string|array $filter A filter to be inserted into the WHERE clause.
      * @param boolean $cache Use caching
-     * @param string $orderBy A sort expression to be inserted into the ORDER BY clause.
+     * @param string|array|null $sort Passed to DataList::sort() so that DataList::first() returns the desired item
      *
      * @return DataObject|null The first item matching the query
      */
-    public static function get_one($callerClass = null, $filter = "", $cache = true, $orderBy = "")
+    public static function get_one($callerClass = null, $filter = "", $cache = true, $sort = "")
     {
         if ($callerClass === null) {
             $callerClass = static::class;
@@ -3417,12 +3417,18 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity
         /** @var DataObject $singleton */
         $singleton = singleton($callerClass);
 
-        $cacheComponents = [$filter, $orderBy, $singleton->getUniqueKeyComponents()];
+        $cacheComponents = [$filter, $sort, $singleton->getUniqueKeyComponents()];
         $cacheKey = md5(serialize($cacheComponents));
 
         $item = null;
         if (!$cache || !isset(self::$_cache_get_one[$callerClass][$cacheKey])) {
-            $dl = DataObject::get($callerClass)->where($filter)->sort($orderBy);
+            $dl = DataObject::get($callerClass);
+            if (!empty($filter)) {
+                $dl = $dl->where($filter);
+            }
+            if (!empty($sort) || is_null($sort)) {
+                $dl = $dl->sort($sort);
+            }
             $item = $dl->first();
 
             if ($cache) {

src/ORM/DataQuery.php

@@ -716,12 +716,17 @@ class DataQuery
     /**
      * 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
+     *
+     * Raw SQL can be vulnerable to SQL injection attacks if used incorrectly, so it's preferable not to use 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)
     {

src/ORM/Search/SearchContext.php

@@ -204,8 +204,10 @@ class SearchContext
         } else {
             $query = $query->limit($limit);
         }
-
-        return $query->sort($sort);
+        if (!empty($sort) || is_null($sort)) {
+            $query = $query->sort($sort);
+        }
+        return $query;
     }
 
     /**

src/Security/InheritedPermissions.php

@@ -393,7 +393,7 @@ class InheritedPermissions implements PermissionChecker, MemberCacheFlusher
         // of whether the user has permission to edit this object.
         $groupedByParent = [];
         $potentiallyInherited = $stageRecords->filter($typeField, self::INHERIT)
-            ->sort("\"{$baseTable}\".\"ID\"")
+            ->orderBy("\"{$baseTable}\".\"ID\"")
             ->dataQuery()
             ->query()
             ->setSelect([

tests/php/ORM/DataListTest.php

@@ -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;
@@ -1816,12 +1818,12 @@ class DataListTest extends SapphireTest
         $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(
@@ -1837,6 +1839,155 @@ class DataListTest extends SapphireTest
         );
     }
 
+    /**
+     * @dataProvider provideRawSqlSortException
+     */
+    public function testRawSqlSort(string $sort, string $type): void
+    {
+        $type = explode('|', $type)[0];
+        if ($type === 'valid') {
+            $this->expectNotToPerformAssertions();
+        } elseif ($type === 'invalid-direction') {
+            $this->expectException(InvalidArgumentException::class);
+            $this->expectExceptionMessageMatches('/Invalid sort direction/');
+        } 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/');
+        } elseif ($type === 'invalid-column') {
+            $this->expectException(InvalidArgumentException::class);
+            $this->expectExceptionMessageMatches('/Invalid sort column/');
+        } elseif ($type === 'unknown-relation') {
+            $this->expectException(InvalidArgumentException::class);
+            $this->expectExceptionMessageMatches('/is not a relation on model/');
+        } else {
+            throw new \Exception("Invalid type $type");
+        }
+        // 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
+    {
+        $type = explode('|', $type)[1];
+        if ($type === 'valid') {
+            if (!str_contains($sort, '"') && !(DB::get_conn()->getConnector() instanceof MySQLiConnector)) {
+                // don't test unquoted things in non-mysql
+                $this->markTestSkipped('Database connector is not MySQLiConnector');
+            }
+            $this->expectNotToPerformAssertions();
+        } else {
+            if (!(DB::get_conn()->getConnector() instanceof MySQLiConnector)) {
+                $this->markTestSkipped('Database connector is not MySQLiConnector');
+            }
+            $this->expectException(\mysqli_sql_exception::class);
+            if ($type === 'error-in-sql-syntax') {
+                $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|valid'],
+            ['Title asc', 'valid|valid'],
+            ['"Title" ASC', 'valid|valid'],
+            ['Title ASC, "DatabaseField"', 'valid|valid'],
+            ['"Title", "DatabaseField" DESC', 'valid|valid'],
+            ['Title ASC, DatabaseField DESC', 'valid|valid'],
+            ['Title ASC, , DatabaseField DESC', 'invalid-column|unknown-column'],
+            ['Captain.ShirtNumber', 'valid|unknown-column'],
+            ['Captain.ShirtNumber ASC', 'valid|unknown-column'],
+            ['"Captain"."ShirtNumber"', 'invalid-column|unknown-column'],
+            ['"Captain"."ShirtNumber" DESC', 'invalid-column|unknown-column'],
+            ['Title BACKWARDS', 'invalid-direction|error-in-sql-syntax'],
+            ['"Strange non-existent column name"', 'invalid-column|unknown-column'],
+            ['NonExistentColumn', 'unknown-column|unknown-column'],
+            ['Team.NonExistentColumn', 'unknown-relation|unknown-column'],
+            ['"DataObjectTest_Team"."NonExistentColumn" ASC', 'invalid-column|unknown-column'],
+            ['"DataObjectTest_Team"."Title" ASC', 'invalid-column|valid'],
+            ['DataObjectTest_Team.Title', 'unknown-relation|valid'],
+            ['Title, 1 = 1', 'invalid-column|valid'],
+            ["Title,'abc' = 'abc'", 'invalid-column|valid'],
+            ['Title,Mod(ID,3)=1', 'invalid-column|valid'],
+            ['(CASE WHEN ID < 3 THEN 1 ELSE 0 END)', 'invalid-column|valid'],
+        ];
+    }
+
+    /**
+     * @dataProvider provideSortDirectionValidationTwoArgs
+     */
+    public function testSortDirectionValidationTwoArgs(string $direction, string $type): void
+    {
+        if ($type === 'valid') {
+            $this->expectNotToPerformAssertions();
+        } else {
+            $this->expectException(InvalidArgumentException::class);
+            $this->expectExceptionMessageMatches('/Invalid sort direction/');
+        }
+        Team::get()->sort('Title', $direction)->column('ID');
+    }
+
+    public function provideSortDirectionValidationTwoArgs(): array
+    {
+        return [
+            ['ASC', 'valid'],
+            ['asc', 'valid'],
+            ['DESC', 'valid'],
+            ['desc', 'valid'],
+            ['BACKWARDS', 'invalid'],
+        ];
+    }
+
+    /**
+     * Test passing scalar values to sort()
+     *
+     * Explicity tests that sort(null) will wipe any existing sort on a DataList
+     *
+     * @dataProvider provideSortScalarValues
+     */
+    public function testSortScalarValues(mixed $emtpyValue, string $type): void
+    {
+        $this->assertSame(['Subteam 1'], Team::get()->limit(1)->column('Title'));
+        $list = Team::get()->sort('Title DESC');
+        $this->assertSame(['Team 3'], $list->limit(1)->column('Title'));
+        if ($type !== 'wipes-existing') {
+            $this->expectException(InvalidArgumentException::class);
+        }
+        if ($type === 'invalid-scalar') {
+            $this->expectExceptionMessage('sort() arguments must either be a string, an array, or null');
+        }
+        if ($type === 'empty-scalar') {
+            $this->expectExceptionMessage('Invalid sort parameter');
+        }
+        // $type === 'wipes-existing' is valid
+        $list = $list->sort($emtpyValue);
+        $this->assertSame(['Subteam 1'], $list->limit(1)->column('Title'));
+    }
+
+    public function provideSortScalarValues(): array
+    {
+        return [
+            [null, 'wipes-existing'],
+            ['', 'empty-scalar'],
+            [[], 'empty-scalar'],
+            [false, 'invalid-scalar'],
+            [true, 'invalid-scalar'],
+            [0, 'invalid-scalar'],
+            [1, 'invalid-scalar'],
+        ];
+    }
+
     public function testShuffle()
     {
         $list = Team::get()->shuffle();

tests/php/ORM/DataObjectTest.php

@@ -1034,10 +1034,10 @@ class DataObjectTest extends SapphireTest
         $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;
         }
@@ -1914,7 +1914,7 @@ class DataObjectTest extends SapphireTest
 
         // Check that ordering a many-many relation by an aggregate column doesn't fail
         $player = $this->objFromFixture(DataObjectTest\Player::class, 'player2');
-        $player->Teams()->sort("count(DISTINCT \"DataObjectTest_Team_Players\".\"DataObjectTest_PlayerID\") DESC");
+        $player->Teams()->orderBy("count(DISTINCT \"DataObjectTest_Team_Players\".\"DataObjectTest_PlayerID\") DESC");
     }
 
     /**

tests/php/ORM/ManyManyThroughListTest.php

@@ -102,7 +102,7 @@ class ManyManyThroughListTest extends SapphireTest
 
         $items = $parent->Items();
         if ($sort) {
-            $items = $items->sort($sort);
+            $items = $items->orderBy($sort);
         }
         $this->assertSame($expected, $items->column('Title'));
     }