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.

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

Fix filter query when using "OR" conditions #1358

Merged
merged 1 commit into from
Mar 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
79 changes: 78 additions & 1 deletion src/Filter/Filter.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

namespace Sonata\DoctrineORMAdminBundle\Filter;

use Doctrine\ORM\Query\Expr\Orx;
use Sonata\AdminBundle\Datagrid\ProxyQueryInterface as BaseProxyQueryInterface;
use Sonata\AdminBundle\Filter\Filter as BaseFilter;
use Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQueryInterface;
Expand All @@ -24,6 +25,15 @@ abstract class Filter extends BaseFilter
*/
protected $active = false;

/**
* Holds an array of `orX` expressions used by each admin when the condition
* equals the value on `FilterInterface::CONDITION_OR`, using the admin code
* as index.
*
* @var array<string, Orx>
*/
private static $orExpressionsByAdmin = [];

/**
* NEXT_MAJOR change $query type for Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQueryInterface.
*
Expand Down Expand Up @@ -96,7 +106,7 @@ protected function applyWhere(BaseProxyQueryInterface $query, $parameter)
}

if (self::CONDITION_OR === $this->getCondition()) {
$query->getQueryBuilder()->orWhere($parameter);
$this->addOrParameter($query, $parameter);
} else {
$query->getQueryBuilder()->andWhere($parameter);
}
Expand Down Expand Up @@ -141,4 +151,71 @@ protected function getNewParameterName(BaseProxyQueryInterface $query)
// by underscores.
return str_replace('.', '_', $this->getName()).'_'.$query->getUniqueParameterId();
}

/**
* Adds the parameter to the corresponding `Orx` expression used in the `where` clause.
* If it doesn't exist, a new one is created.
* This method groups the filter "OR" conditions based on the "admin_code" option. If this
* option is not set, it uses a marker (":sonata_admin_datagrid_filter_query_marker") in
* the resulting DQL in order to identify the corresponding "WHERE (...)" condition
* group each time it is required.
* It allows to get queries like "WHERE previous_condition = previous_value AND (filter_1 = value OR filter_2 = value OR ...)",
* where the logical "OR" operators added by the filters are grouped inside a condition,
* instead of having unfolded "WHERE ..." clauses like "WHERE previous_condition = previous_value OR filter_1 = value OR filter_2 = value OR ...",
* which will produce undesired results.
*
* TODO: Remove the logic related to the ":sonata_admin_datagrid_filter_query_marker" marker when
* the constraint for "sonata-project/admin-bundle" guarantees that the "admin_code" option is set.
*
* @param mixed $parameter
*/
private function addOrParameter(BaseProxyQueryInterface $query, $parameter): void
{
$adminCode = $this->getOption('admin_code');
Copy link
Member Author

Choose a reason for hiding this comment

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

Even if we usually will use the admin code which a filter belongs to as value for this option, I think we could use a more agnostic name, since this option has no actual relation with an admin code; it's just a grouping identifier used to know which filters must share the same condition.
WDYT about "group" or something similar?

$orExpression = self::$orExpressionsByAdmin[$adminCode] ?? null;
if ($orExpression instanceof Orx) {
$orExpression->add($parameter);

return;
}

$qb = $query->getQueryBuilder();
$where = $qb->getDQLPart('where');

// Search for the ":sonata_admin_datagrid_filter_query_marker" marker in order to
// get the `Orx` expression.
if (null === $adminCode && null !== $where) {
foreach ($where->getParts() as $expression) {
if (!$expression instanceof Orx) {
continue;
}

$expressionParts = $expression->getParts();

if (isset($expressionParts[0]) && \is_string($expressionParts[0]) &&
0 === strpos($expressionParts[0], ':sonata_admin_datagrid_filter_query_marker')
) {
$expression->add($parameter);

return;
}
}
}

// Create a new `Orx` expression.
$orExpression = $qb->expr()->orX();

if (null === $adminCode) {
// Add the ":sonata_admin_datagrid_filter_query_marker" parameter as marker for the `Orx` expression.
$orExpression->add($qb->expr()->isNull(':sonata_admin_datagrid_filter_query_marker'));
$qb->setParameter('sonata_admin_datagrid_filter_query_marker', 'sonata_admin.datagrid.filter_query.marker');
} else {
self::$orExpressionsByAdmin[$adminCode] = $orExpression;
}

$orExpression->add($parameter);

// Add the `Orx` expression to the `where` clause.
$qb->andWhere($orExpression);
}
}
71 changes: 69 additions & 2 deletions tests/Filter/FilterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,14 @@

namespace Sonata\DoctrineORMAdminBundle\Tests\Filter;

use PHPUnit\Framework\TestCase;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Query\Expr;
use Sonata\AdminBundle\Datagrid\ProxyQueryInterface;
use Sonata\DoctrineORMAdminBundle\Datagrid\ProxyQuery;
use Sonata\DoctrineORMAdminBundle\Filter\Filter;
use Sonata\DoctrineORMAdminBundle\Filter\StringFilter;

final class FilterTest extends TestCase
final class FilterTest extends FilterTestCase
{
/**
* @var Filter
Expand Down Expand Up @@ -58,6 +61,70 @@ public function testIsActive(): void
$this->assertFalse($this->filter->isActive());
}

/**
* @dataProvider orExpressionProvider
*/
public function testOrExpression(string $expected, array $filterOptions = []): void
{
$entityManager = $this->createStub(EntityManagerInterface::class);
$entityManager->method('getExpressionBuilder')->willReturn(new Expr());
$queryBuilder = new TestQueryBuilder($entityManager);

$queryBuilder->select('e')->from('MyEntity', 'e');

// Some custom conditions set previous to the filters.
$queryBuilder
->where($queryBuilder->expr()->eq(1, 2))
->andWhere(
$queryBuilder->expr()->orX(
$queryBuilder->expr()->eq(':parameter_1', 4),
$queryBuilder->expr()->eq(5, 6)
)
)
->setParameter('parameter_1', 3);

$proxyQuery = new ProxyQuery($queryBuilder);
$this->assertSameQuery([], $proxyQuery);

$filter1 = new StringFilter();
$filter1->setCondition(Filter::CONDITION_OR);
$filter1->initialize('field_name', $filterOptions);

$filter1->filter($proxyQuery, 'e', 'project', ['value' => 'sonata-project']);

$filter2 = new StringFilter();
$filter2->setCondition(Filter::CONDITION_OR);
$filter2->initialize('field_name', $filterOptions);

$filter2->filter($proxyQuery, 'e', 'version', ['value' => '3.x']);

// More custom conditions set after the filters.
$queryBuilder->andWhere($queryBuilder->expr()->eq(7, 8));

$this->assertSame($expected, $queryBuilder->getDQL());
}

public function orExpressionProvider(): iterable
{
yield 'Using "admin_code" option' => [
'SELECT e FROM MyEntity e WHERE 1 = 2 AND (:parameter_1 = 4 OR 5 = 6)'
.' AND (e.project LIKE :field_name_0 OR e.version LIKE :field_name_1) AND 7 = 8',
[
'allow_empty' => false,
'admin_code' => 'my_admin',
],
];

yield 'Missing "admin_code" option, fallback to DQL marker' => [
'SELECT e FROM MyEntity e WHERE 1 = 2 AND (:parameter_1 = 4 OR 5 = 6)'
.' AND (:sonata_admin_datagrid_filter_query_marker IS NULL'
.' OR e.project LIKE :field_name_0 OR e.version LIKE :field_name_1) AND 7 = 8',
[
'allow_empty' => false,
],
];
}

private function createFilter(): Filter
{
return new class() extends Filter {
Expand Down