Skip to content

Commit

Permalink
CC-26467: Fixed Propel filter to criteria mapping (#10101)
Browse files Browse the repository at this point in the history 
CC-26467 Fixed Propel filter to criteria mapping
  • Loading branch information
@michbeck
michbeck authored Apr 18, 2023
1 parent 18dae85 commit 26b4e6b
Show file tree
Hide file tree
Showing 2 changed files with 154 additions and 33 deletions.
65 changes: 55 additions & 10 deletions src/Spryker/Zed/Propel/PropelFilterCriteria.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@

class PropelFilterCriteria implements PropelFilterCriteriaInterface
{
/**
* @var string
*/
protected const PATTERN_VALID_COLUMN_NAME = '/^[a-zA-Z0-9_.]+$/';

/**
* @var \Generated\Shared\Transfer\FilterTransfer
*/
Expand Down Expand Up @@ -47,23 +52,63 @@ public function setFilterTransfer(FilterTransfer $filterTransfer)
public function toCriteria()
{
$criteria = new Criteria();
$criteria = $this->addPaginationToCriteria($criteria);
$criteria = $this->addSortingToCriteria($criteria);

return $criteria;
}

/**
* @param \Propel\Runtime\ActiveQuery\Criteria $criteria
*
* @return \Propel\Runtime\ActiveQuery\Criteria
*/
protected function addPaginationToCriteria(Criteria $criteria): Criteria
{
$limit = $this->filterTransfer->getLimit();
if ($limit !== null) {
$criteria->setLimit($limit);
}

$offset = $this->filterTransfer->getOffset();
if ($offset !== null) {
$criteria->setOffset($offset);
}

return $criteria;
}

if ($this->filterTransfer->getLimit() !== null) {
$criteria->setLimit($this->filterTransfer->getLimit());
/**
* @param \Propel\Runtime\ActiveQuery\Criteria $criteria
*
* @return \Propel\Runtime\ActiveQuery\Criteria
*/
protected function addSortingToCriteria(Criteria $criteria): Criteria
{
$orderByColumnName = $this->filterTransfer->getOrderBy();
if ($orderByColumnName === null || !$this->isColumnNameValid($orderByColumnName)) {
return $criteria;
}

if ($this->filterTransfer->getOffset() !== null) {
$criteria->setOffset($this->filterTransfer->getOffset());
$orderDirection = $this->filterTransfer->getOrderDirection();
if ($orderDirection === Criteria::ASC) {
$criteria->addAscendingOrderByColumn($orderByColumnName);
}

if ($this->filterTransfer->getOrderBy() !== null) {
if ($this->filterTransfer->getOrderDirection() === 'ASC') {
$criteria->addAscendingOrderByColumn($this->filterTransfer->getOrderBy());
} elseif ($this->filterTransfer->getOrderDirection() === 'DESC') {
$criteria->addDescendingOrderByColumn($this->filterTransfer->getOrderBy());
}
if ($orderDirection === Criteria::DESC) {
$criteria->addDescendingOrderByColumn($orderByColumnName);
}

return $criteria;
}

/**
* @param string $orderByColumnName
*
* @return bool
*/
protected function isColumnNameValid(string $orderByColumnName): bool
{
return (bool)preg_match(static::PATTERN_VALID_COLUMN_NAME, $orderByColumnName);
}
}
122 changes: 99 additions & 23 deletions tests/SprykerTest/Zed/Propel/Business/PropelFilterCriteriaTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,38 +25,114 @@
class PropelFilterCriteriaTest extends Unit
{
/**
* @dataProvider paginationDataProvider
*
* @param int|null $limit
* @param int|null $offset
* @param int|null $expectedLimit
* @param int|null $expectedOffset
*
* @return void
*/
public function testToCriteriaShouldReturnEmptyCriteriaWhenNothingWasSet(): void
{
$filterTransfer = new FilterTransfer();
public function testToCriteriaShouldReturnCriteriaWithPagination(
?int $limit,
?int $offset,
?int $expectedLimit,
?int $expectedOffset
): void {
// Arrange
$filterTransfer = (new FilterTransfer())
->setLimit($limit)
->setOffset($offset);
$propelFilterCriteria = new PropelFilterCriteria($filterTransfer);

$filterCriteria = new PropelFilterCriteria($filterTransfer);
$propelCriteria = $filterCriteria->toCriteria();
// Act
$criteria = $propelFilterCriteria->toCriteria();

$this->assertInstanceOf(Criteria::class, $propelCriteria);
$this->assertSame(-1, $propelCriteria->getLimit());
$this->assertSame(0, $propelCriteria->getOffset());
$this->assertSame([], $propelCriteria->getOrderByColumns());
// Assert
$this->assertSame($expectedLimit, $criteria->getLimit());
$this->assertSame($expectedOffset, $criteria->getOffset());
}

/**
* @dataProvider sortingDataProvider
* @dataProvider invalidSortingDataProvider
*
* @param string|null $orderByColumnName
* @param string|null $orderDirection
* @param string|null $expectedOrderByColumnName
* @param string|null $expectedOrderDirection
*
* @return void
*/
public function testToCriteriaShouldReturnCriteriaWithParameters(): void
public function testToCriteriaShouldReturnCriteriaWithSorting(
?string $orderByColumnName,
?string $orderDirection,
?string $expectedOrderByColumnName,
?string $expectedOrderDirection
): void {
// Arrange
$filterTransfer = (new FilterTransfer())
->setOrderBy($orderByColumnName)
->setOrderDirection($orderDirection);
$propelFilterCriteria = new PropelFilterCriteria($filterTransfer);
$expectedOrderbyColumns = [];
if ($expectedOrderByColumnName !== null && $expectedOrderDirection !== null) {
$expectedOrderbyColumns = [sprintf('%s %s', $expectedOrderByColumnName, $expectedOrderDirection)];
}

// Act
$criteria = $propelFilterCriteria->toCriteria();

// Assert
$this->assertSame($expectedOrderbyColumns, $criteria->getOrderByColumns());
}

/**
* @return array<string, array<array-key, int|null>>
*/
protected function paginationDataProvider(): array
{
return [
'limit and offset are null' => [null, null, -1, 0],
'limit and offset are set' => [5, 10, 5, 10],
'limit is set, offset is null' => [10, null, 10, 0],
'offset is set, limit is null' => [null, 20, -1, 20],
];
}

/**
* @return array<string, array<array-key, string|null>>
*/
protected function sortingDataProvider(): array
{
return [
'empty column name and direction asc' => [null, Criteria::ASC, null, Criteria::ASC],
'column name and empty direction' => ['created_at', null, 'created_at', null],
'column name with underscore and direction asc' => ['created_at', Criteria::ASC, 'created_at', Criteria::ASC],
'column name combined with table name and direction asc' => ['spy_product.created_at', Criteria::ASC, 'spy_product.created_at', Criteria::ASC],
'column name and sort direction asc' => ['created_at', Criteria::ASC, 'created_at', Criteria::ASC],
'column name and sort direction desc' => ['created_at', Criteria::DESC, 'created_at', Criteria::DESC],
];
}

/**
* @return array<string, array<array-key, string|null>>
*/
protected function invalidSortingDataProvider(): array
{
$filterTransfer = new FilterTransfer();
$filterTransfer->setLimit(10);
$filterTransfer->setOffset(0);
$filterTransfer->setOrderDirection('DESC');
$filterTransfer->setOrderBy('foobar');

$filterCriteria = new PropelFilterCriteria($filterTransfer);
$propelCriteria = $filterCriteria->toCriteria();

$this->assertInstanceOf(Criteria::class, $propelCriteria);
$this->assertSame(10, $propelCriteria->getLimit());
$this->assertSame(0, $propelCriteria->getOffset());
$this->assertEquals(['foobar DESC'], $propelCriteria->getOrderByColumns());
return [
'invalid column name with SQL injection' => ['created_at; SELECT * FROM users;', Criteria::ASC, null, Criteria::ASC],
'invalid column name with semicolon' => ['created_at;', Criteria::ASC, null, Criteria::ASC],
'invalid column name with spaces' => ['created at', Criteria::ASC, null, Criteria::ASC],
'invalid column name with special characters' => ['created_at#', Criteria::ASC, null, Criteria::ASC],
'invalid column name with brackets' => ['my_column()', Criteria::ASC, null, Criteria::ASC],
'invalid column name with quotes' => ["my_column'", Criteria::ASC, null, Criteria::ASC],
'invalid column name with backticks' => ['`my_column`', Criteria::ASC, null, Criteria::ASC],
'invalid column name with leading/trailing spaces' => [' column ', Criteria::ASC, null, Criteria::ASC],
'invalid column name with non-word characters' => ['col_name$', Criteria::ASC, null, Criteria::ASC],
'invalid column name with non-alphanumeric characters' => ['col:name', Criteria::ASC, null, Criteria::ASC],
'invalid column name with non-ASCII characters' => ['col_名', Criteria::ASC, null, Criteria::ASC],
];
}
}

0 comments on commit 26b4e6b

Please sign in to comment.