Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Create orderBy() method to handle raw SQL #10600

Merged
merged 1 commit into from
Dec 7, 2022
Merged

API Create orderBy() method to handle raw SQL #10600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 104 additions & 47 deletions src/ORM/DataList.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,14 @@ public function sql(&$parameters = [])
/**
* 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
Expand All @@ -222,6 +226,9 @@ public function where($filter)
* 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.
Expand All @@ -237,8 +244,6 @@ public function whereAny($filter)
});
}



/**
* Returns true if this DataList can be sorted by the given field.
*
Expand Down Expand Up @@ -308,72 +313,118 @@ 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 field names 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');
GuySartorelli marked this conversation as resolved.
Show resolved Hide resolved
* @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) {
GuySartorelli marked this conversation as resolved.
Show resolved Hide resolved
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');
list($column, $direction) = $args;
$sort = [$column => $direction];
} else {
$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
GuySartorelli marked this conversation as resolved.
Show resolved Hide resolved
if (preg_match('/^(.+) ([^"]+)$/i', trim($colDir), $matches)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will still match against something like "Column Name" - it's funtionally equivalent to using explode for any string with only one space in it.
If the column name has a space in it, and no direction was supplied, it should not match here so that the default direction can be added in the else block.

I think we should explicitly be checking for asc and desc here instead.

Suggested change
if (preg_match('/^(.+) ([^"]+)$/i', trim($colDir), $matches)) {
if (preg_match('/^(.+) (asc|desc)$/i', trim($colDir), $matches)) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't match "Column Name" - it's pinned with $ at the end of the regex

It would match Column Name - however this will fail on validateSortDirection()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, I should have omitted the quote marks which were intended to mean "this is the whole string"

My point is that it shouldn't fail on validateSortDirection(), because Column on its own won't fail that.
So currently passing Column is fine but Column Name is not fine, even if they're both valid columns.

Copy link
Member Author

@emteknetnz emteknetnz Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think think we need to over think for edge cases like this. An exception will still be thrown and it should be pretty obvious at that point they need to wrap their column in quotes since they'll get a 'Invalid sort direction Name' exception message

Even if we change this to (asc|desc), then it'll still fail on the edge case of someone calling naming column MySortDir ASC. There's a workaround available which is to wrap the weird column in double quotes e.g. ->sort('"Column Name" ASC')

I think it's reasonable to put the onus on end users to do this here - they'll be well used to do extra steps to get things to actually work. Other parts of Silverstripe will already be broken e.g. calling $myDataObject->{'Column Name'} to get values (I'm assuming that actually works)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree but I'll merge it 'cause it's not worth arguing about. Someone can raise an issue when it causes problems for them.

list($column, $direction) = [$matches[1], $matches[2]];
} else {
list($column, $direction) = [$colDir, 'ASC'];
GuySartorelli marked this conversation as resolved.
Show resolved Hide resolved
}
$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);
}
});
}

$sort = [$col => $dir];
} else {
$sort = func_get_arg(0);
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");
}
}

return $this->alterDataQuery(function (DataQuery $query, DataList $list) use ($sort) {
private function validateSortDirection(string $direction): void
{
$dir = strtolower($direction);
if ($dir !== 'asc' && $dir !== 'desc') {
throw new InvalidArgumentException("Invalid sort direction $direction");
}
}

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);
}
}
/**
* 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
Expand Down Expand Up @@ -433,6 +484,8 @@ public function addFilter($filterArray)
/**
* 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'
Expand Down Expand Up @@ -564,7 +617,7 @@ function (DataQuery $query) use ($field, &$columnName, $linearOnly) {
*/
protected function isValidRelationName($field)
{
return preg_match('/^[A-Z0-9._]+$/i', $field ?? '');
return preg_match('/^[A-Z0-9\._]+$/i', $field ?? '');
}

/**
Expand Down Expand Up @@ -608,6 +661,8 @@ protected function createSearchFilter($filter, $value)
/**
* 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
Expand Down Expand Up @@ -648,6 +703,8 @@ public function exclude()
/**
* 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
Expand Down Expand Up @@ -1190,7 +1247,7 @@ public function removeByFilter($filter)
*/
public function shuffle()
{
return $this->sort(DB::get_conn()->random());
return $this->orderBy(DB::get_conn()->random());
}

/**
Expand Down
20 changes: 13 additions & 7 deletions src/ORM/DataObject.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2254,7 +2254,7 @@ public function getManyManyComponents($componentName, $id = null)
// 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);
Expand Down Expand Up @@ -3327,7 +3327,7 @@ public function getReverseAssociation($className)
* @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.
Expand Down Expand Up @@ -3369,7 +3369,7 @@ public static function get(
if ($filter) {
$result = $result->where($filter);
}
if ($sort) {
if ($sort || is_null($sort)) {
$result = $result->sort($sort);
}
if ($limit && strpos($limit ?? '', ',') !== false) {
Expand Down Expand Up @@ -3399,11 +3399,11 @@ public static function get(
* 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;
Expand All @@ -3417,12 +3417,18 @@ public static function get_one($callerClass = null, $filter = "", $cache = true,
/** @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) {
Expand Down
5 changes: 5 additions & 0 deletions src/ORM/DataQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,12 +716,17 @@ 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
GuySartorelli marked this conversation as resolved.
Show resolved Hide resolved
*
* 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)
{
Expand Down
6 changes: 4 additions & 2 deletions src/ORM/Search/SearchContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,10 @@ private function prepareQuery($sort, $limit, ?DataList $existingQuery): DataList
} else {
$query = $query->limit($limit);
}

return $query->sort($sort);
if (!empty($sort) || is_null($sort)) {
$query = $query->sort($sort);
}
return $query;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/Security/InheritedPermissions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ protected function batchPermissionCheckForStage(
// 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([
Expand Down
Loading