From d48969d47aa6f4cf38c507e91c6e6e83f9e126d3 Mon Sep 17 00:00:00 2001 From: henk Date: Sat, 18 Dec 2021 16:04:35 +0100 Subject: [PATCH] Issue #2 Does this work with Collections? - added two workarounds - adapted tests - adapted readme --- README.md | 74 +++++++++++++++++++---- src/Entity/TestEntity.php | 13 ++++ src/Entity/TestRelated.php | 54 +++++++++++++++++ src/Filter/AddFakeLeftJoin.php | 47 +++++++++++++++ src/Filter/FilterLogic.php | 50 +++++++++++++++- src/Filter/RemoveFakeLeftJoin.php | 53 +++++++++++++++++ tests/Filter/FilterLogicTest.php | 98 +++++++++++++++++++++++++------ tests/Utils/Reflection.php | 11 ++++ 8 files changed, 365 insertions(+), 35 deletions(-) create mode 100644 src/Entity/TestRelated.php create mode 100644 src/Filter/AddFakeLeftJoin.php create mode 100644 src/Filter/RemoveFakeLeftJoin.php diff --git a/README.md b/README.md index 75bfc4c..74fa6bb 100644 --- a/README.md +++ b/README.md @@ -47,6 +47,12 @@ Because of the nesting of or the criteria for the description are combined toget AND with the criterium for price, which must allways be true while only one of the criteria for the desciption needs to be true for an order to be returned. +You can in/exclude filters by class name by configuring classExp. For example: +```php docblock +* @ApiFilter(FilterLogic::class, arguments={"classExp"="/ApiPlatform\\Core\\Bridge\\Doctrine\\Orm\\Filter\\+/"}) +``` +will only apply API Platform ORM Filters in logic context. + Installation ------------ With composer: @@ -61,38 +67,80 @@ Then add the bundle to your api config/bundles.php: ]; ``` +Nested properties workaround +---------------------------- +The built-in filters of Api Platform normally generate INNER JOINs. As a result +combining them with OR may not produce results as expected for properties +nested over nullable and to many associations, , see [this issue](https://github.com/metaclass-nl/filter-bundle/issues/2). + +As a workaround FilterLogic can convert all inner joins into left joins: +```php8 +#[ApiResource] +#[ApiFilter(SearchFilter::class, properties: ['title' => 'partial', 'keywords.word' => 'exact'])] +#[ApiFilter(FilterLogic::class, arguments: ['innerJoinsLeft' => true])] +class Article { +// ... +} +``` + +In case you do not like FilterLogic messing with the joins you can make +the built-in filters of Api Platform generate left joins themselves by first adding +a left join and removing it later: +```php8 +#[ApiResource] +#[ApiFilter(AddFakeLeftJoin::class)] +#[ApiFilter(SearchFilter::class, properties: ['title' => 'partial', 'keywords.word' => 'exact'])] +#[ApiFilter(FilterLogic::class)] +#[ApiFilter(RemoveFakeLeftJoin::class)] +class Article { +// ... +} +``` + +With one fo these workarounds the following will find Articles whose title contains 'pro' +as well as those whose keywords contain one whose word is 'php'. +```uri +/articles/?or[title]=pro&or[keywords.word]=php +``` +Without a workaround Articles without any keywords will not be found, +even if their titles contain 'pro'. + +Both workarounds do change the behavior of ExistsFilter =false with nested properties. +Normally this filter only finds entities that reference at least one entity +whose nested property contains NULL, but with left joins it will also find entities +whose reference itself is empty or NULL. This does break backward compatibility. +This can be solved by extending ExistsFilter, but that is not included +in this Bundle because IMHO the old behavior is not like one would expect given +the semantics of "exists" and therefore should be considered a bug unless it is +documented explicitly to be intentional. + Limitations ----------- Combining filters through OR and nested logic may be a harder task for your database and require different indexes. Except for small tables performance testing and analysis is advisable prior to deployment. -Only works with filters implementing ContextAwareFilterInterface, other filters -are ignoored. Works with built in filters of Api Platform, except for DateFilter +Works with built in filters of Api Platform, except for DateFilter with EXCLUDE_NULL. A DateFilter subclass is provided to correct this. However, -the built in filters of Api Platform contain a bug with respect to the JOINs -they generate, see [this issue](https://github.com/metaclass-nl/filter-bundle/issues/2). +the built in filters of Api Platform IMHO contain a bug with respect to the JOINs +they generate. As a result, combining them with OR does not work as expected with properties -nested over to-many and nullable associations. +nested over to-many and nullable associations. Workarounds are provided, but they +do change the behavior of ExistsFilter =false. Assumes that filters create semantically complete expressions in the sense that expressions added to the QueryBundle through ::andWhere or ::orWhere do not depend on one another so that the intended logic is not compromised if they are recombined with the others by either Doctrine\ORM\Query\Expr\Andx or Doctrine\ORM\Query\Expr\Orx. -May Fail if a filter uses QueryBuilder::where or ::add. +Only works with filters implementing ContextAwareFilterInterface, other filters +are ignoored. -You are advised to check the code of all custom and third party Filters and +May Fail if a filter uses QueryBuilder::where or ::add. You are advised to check the code of all custom and third party Filters and not to combine those that use QueryBuilder::where or ::add with FilterLogic or that produce complex logic that is not semantically complete. For an example of semantically complete and incomplete expressions see [DateFilterTest](./tests/Filter/DateFilterTest.php). -You can in/exclude filters by class name by configuring classExp. For example: -```php docblock -* @ApiFilter(FilterLogic::class, arguments={"classExp"="/ApiPlatform\\Core\\Bridge\\Doctrine\\Orm\\Filter\\+/"}) -``` -will only apply API Platform ORM Filters in logic context. - Credits and License ------------------- Copyright (c) [MetaClass](https://www.metaclass.nl/), Groningen, 2021. [MetaClass](https://www.metaclass.nl/) offers software development and support in the Netherlands for Symfony, API Platform, React.js and Next.js diff --git a/src/Entity/TestEntity.php b/src/Entity/TestEntity.php index 7706905..899241b 100644 --- a/src/Entity/TestEntity.php +++ b/src/Entity/TestEntity.php @@ -3,6 +3,7 @@ namespace Metaclass\FilterBundle\Entity; +use Doctrine\Common\Collections\Collection; use Doctrine\ORM\Mapping as ORM; /** @@ -45,4 +46,16 @@ class TestEntity * @ORM\Column(type="boolean", nullable=true) */ public $bool; + + /** + * @var TestRelated + * @ORM\ManyToOne(targetEntity="Metaclass\FilterBundle\Entity\TestRelated", inversedBy="toMany") + */ + public $toOneNullable; + + /** + * @var Collection + * @ORM\OneToMany(targetEntity="Metaclass\FilterBundle\Entity\TestRelated", mappedBy="project") + */ + public $toMany; } \ No newline at end of file diff --git a/src/Entity/TestRelated.php b/src/Entity/TestRelated.php new file mode 100644 index 0000000..c748c09 --- /dev/null +++ b/src/Entity/TestRelated.php @@ -0,0 +1,54 @@ +leftJoin(self::$FAKEJOIN, null); + $rootAliases = $queryBuilder->getRootAliases(); + $join = new Join( + Join::LEFT_JOIN, + self::$FAKEJOIN + ); + $queryBuilder->add('join', [$rootAliases[0] => $join], true); + } +} \ No newline at end of file diff --git a/src/Filter/FilterLogic.php b/src/Filter/FilterLogic.php index afebf7b..a7f3d66 100644 --- a/src/Filter/FilterLogic.php +++ b/src/Filter/FilterLogic.php @@ -5,7 +5,6 @@ use ApiPlatform\Core\Api\FilterCollection; use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\AbstractContextAwareFilter; use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\ContextAwareFilterInterface; -use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\FilterInterface; use ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\OrderFilter; use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface; use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface; @@ -15,12 +14,16 @@ use Doctrine\Persistence\ManagerRegistry; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; -use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Serializer\NameConverter\NameConverterInterface; +use Doctrine\ORM\Query\Expr\Join; /** * Combines existing API Platform ORM Filters with AND and OR. * For usage and limitations see https://gist.github.com/metaclass-nl/790a5c8e9064f031db7d3379cc47c794 + * WARNING: $innerJoinsLeft=true changes the behavior of ExistsFilter =false, + * and though it makes it more like one would expect given the semantics of its name, + * it does break backward compatibility. + * * Copyright (c) MetaClass, Groningen, 2021. MIT License */ class FilterLogic extends AbstractContextAwareFilter @@ -33,19 +36,25 @@ class FilterLogic extends AbstractContextAwareFilter private $classExp; /** @var ContextAwareFilterInterface[] */ private $filters; + /** @var bool Wheather to replace all inner joins by left joins */ + private $innerJoinsLeft; /** * @param ResourceMetadataFactoryInterface $resourceMetadataFactory * @param ContainerInterface|FilterCollection $filterLocator * @param $regExp string Filter classes must match this to be applied with logic + * @param $innerJoinsLeft bool Wheather to replace all inner joins by left joins. + * This makes the standard Api Platform filters combine properly with OR, + * but also changes the behavior of ExistsFilter =false. * {@inheritdoc} */ - public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, $filterLocator, ManagerRegistry $managerRegistry, LoggerInterface $logger = null, array $properties = null, NameConverterInterface $nameConverter = null, string $classExp='//') + public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory, $filterLocator, ManagerRegistry $managerRegistry, LoggerInterface $logger = null, array $properties = null, NameConverterInterface $nameConverter = null, string $classExp='//', $innerJoinsLeft=false) { parent::__construct($managerRegistry, null, $logger, $properties, $nameConverter); $this->resourceMetadataFactory = $resourceMetadataFactory; $this->filterLocator = $filterLocator; $this->classExp = $classExp; + $this->innerJoinsLeft = $innerJoinsLeft; } /** {@inheritdoc } */ @@ -65,6 +74,7 @@ public function apply(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $q if (!isset($context['filters']) || !\is_array($context['filters'])) { throw new \InvalidArgumentException('::apply without $context[filters] not supported'); } + $this->filters = $this->getFilters($resourceClass, $operationName); if (isset($context['filters']['and']) ) { @@ -86,6 +96,10 @@ public function apply(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $q $queryBuilder->orWhere($exp); }; } + + if ($this->innerJoinsLeft) { + $this->replaceInnerJoinsByLeftJoins($queryBuilder); + } } /** @@ -258,4 +272,34 @@ protected function getFilters($resourceClass, $operationName) } return $result; } + + /** + * The filters that come standard with Api Platform create inner joins, + * but for nullable and to many references we need Left Joins for OR + * to also produce results that are not related. + * WARNING: This changes the behavior of ExistsFilter =false, consider + * using ExistFilter included in this bundle instead. + * @param QueryBuilder $queryBuilder + */ + protected function replaceInnerJoinsByLeftJoins(QueryBuilder $queryBuilder) { + $joinPart = $queryBuilder->getDQLPart('join'); + $result = []; + foreach ($joinPart as $rootAlias => $joins) { + foreach ($joins as $i => $joinExp) { + if (Join::INNER_JOIN === $joinExp->getJoinType()) { + $result[$rootAlias][$i] = new Join( + Join::LEFT_JOIN, + $joinExp->getJoin(), + $joinExp->getAlias(), + $joinExp->getConditionType(), + $joinExp->getCondition(), + $joinExp->getIndexBy() + ); + } else { + $result[$rootAlias][$i] = $joinExp; + } + } + } + $queryBuilder->add('join', $result); + } } \ No newline at end of file diff --git a/src/Filter/RemoveFakeLeftJoin.php b/src/Filter/RemoveFakeLeftJoin.php new file mode 100644 index 0000000..896129a --- /dev/null +++ b/src/Filter/RemoveFakeLeftJoin.php @@ -0,0 +1,53 @@ +getDQLPart('join'); + $result = []; + foreach ($joinPart as $rootAlias => $joins) { + foreach ($joins as $i => $joinExp) { + if (AddFakeLeftJoin::$FAKEJOIN !== $joinExp->getJoin()) { + $result[$rootAlias][$i] = $joinExp; + } + } + } + $queryBuilder->add('join', $result); + } +} \ No newline at end of file diff --git a/tests/Filter/FilterLogicTest.php b/tests/Filter/FilterLogicTest.php index 9e99573..98ce3de 100644 --- a/tests/Filter/FilterLogicTest.php +++ b/tests/Filter/FilterLogicTest.php @@ -52,8 +52,8 @@ public function setUp() $this->filters[] = new DateFilter($this->doctrine, $requestStack, $logger, ['dd' => null]); $this->filters[] = new NumericFilter($this->doctrine, $requestStack, $logger, ['numb' => null]); $this->filters[] = new RangeFilter($this->doctrine, $requestStack, $logger, ['numb' => null]); - $this->filters[] = new SearchFilter($this->doctrine, $requestStack, $iriConverter, null, $logger, ['text' => null], null, $nameConverter); - $this->filters[] = new ExistsFilter($this->doctrine, $requestStack, $logger, ['bool' => null, 'dd' => null]); + $this->filters[] = new SearchFilter($this->doctrine, $requestStack, $iriConverter, null, $logger, ['text' => null, 'toOneNullable.text'=>'start', 'toMany.text'=>'exact'], null, $nameConverter); + $this->filters[] = new ExistsFilter($this->doctrine, $requestStack, $logger, ['bool' => null, 'dd' => null, 'toOneNullable.dd'=>null, 'toMany.bool'=>null]); $this->filters[] = new BooleanFilter($this->doctrine, $requestStack, $logger, ['bool' => null]); $this->filters[] = new FilterToTestAssumptions(); Reflection::setProperty($this->filterLogic, 'filters', $this->filters); @@ -258,7 +258,7 @@ public function testWithSearchFilter() { $operator = 'or'; $reqData = null; - parse_str('or[][numb]=55&or[][numb]=2.7', $reqData); + parse_str('or[][text]=Foo&or[][text]=Bar', $reqData); // var_dump($reqData); $context = ['filters' => $reqData]; $args = [$this->qb, $this->queryNameGen, TestEntity::class, 'get', $context]; @@ -266,23 +266,23 @@ public function testWithSearchFilter() $this->assertEquals(1, count($result), 'number of expressions'); $this->assertEquals( - "o.numb = :numb_p1 OR o.numb = :numb_p2", + "o.text = :text_p1 OR o.text = :text_p2", $result[0], 'expression 0'); $this->assertEquals( - 55, - $this->qb->getParameter('numb_p1')->getValue(), - 'Parameter numb_p1'); + 'Foo', + $this->qb->getParameter('text_p1')->getValue(), + 'Parameter text_p1'); $this->assertEquals( - 2.7, - $this->qb->getParameter('numb_p2')->getValue(), - 'Parameter numb_p2'); + 'Bar', + $this->qb->getParameter('text_p2')->getValue(), + 'Parameter text_p2'); } public function testNotWithSearchFilter() { $reqData = null; - parse_str('not[][numb]=55¬[][numb]=2.7', $reqData); + parse_str('not[][text]=Foo¬[][text]=Bar', $reqData); // var_dump($reqData); $context = ['filters' => $reqData]; $args = [$this->qb, $this->queryNameGen, TestEntity::class, 'get', $context]; @@ -290,21 +290,21 @@ public function testNotWithSearchFilter() $this->assertEquals(2, count($result), 'number of expressions'); $this->assertEquals( - "NOT(o.numb = :numb_p1)", + "NOT(o.text = :text_p1)", (string) $result[0], 'expression 0'); $this->assertEquals( - "NOT(o.numb = :numb_p2)", + "NOT(o.text = :text_p2)", (string) $result[1], 'expression 1'); $this->assertEquals( - 55, - $this->qb->getParameter('numb_p1')->getValue(), - 'Parameter numb_p1'); + 'Foo', + $this->qb->getParameter('text_p1')->getValue(), + 'Parameter text_p1'); $this->assertEquals( - 2.7, - $this->qb->getParameter('numb_p2')->getValue(), - 'Parameter numb_p2'); + 'Bar', + $this->qb->getParameter('text_p2')->getValue(), + 'Parameter text_p2'); } public function testOrNotWithSearchFilter() @@ -437,4 +437,64 @@ public function testAssumptionWhereEmptyOrx() $args = [$this->qb, $this->queryNameGen, TestEntity::class, 'get', $context]; $result = Reflection::callMethod($this->filterLogic, 'doGenerate', $args); } + + public function testInnerJoinsLeftWithSearchFilter() + { + $reqData = null; + parse_str('and[][toMany.text]=Foo&and[][toOneNullable.text]=Bar', $reqData); + // var_dump($reqData); + $context = ['filters' => $reqData]; + $args = [$this->qb, $this->queryNameGen, TestEntity::class, 'get', $context]; + $result = Reflection::callMethod($this->filterLogic, 'doGenerate', $args); + + $this->assertEquals(1, count($result), 'number of expressions'); + $this->assertEquals( + "toMany_a1.text = :text_p1 AND toOneNullable_a2.text LIKE CONCAT(:text_p2_0, '%')", + (string) $result[0], + 'expression 0'); + $this->assertEquals( + 'Foo', + $this->qb->getParameter('text_p1')->getValue(), + 'Parameter text_p1'); + $this->assertEquals( + 'Bar', + $this->qb->getParameter('text_p2_0')->getValue(), + 'Parameter text_p2_0'); + + $this->assertEquals( + 'SELECT o FROM Metaclass\FilterBundle\Entity\TestEntity o INNER JOIN o.toMany toMany_a1 INNER JOIN o.toOneNullable toOneNullable_a2', + $this->qb->getDQL(), + 'DQL before replaceInnerJoinsByLeftJoins' + ); + Reflection::callMethod( + $this->filterLogic, + 'replaceInnerJoinsByLeftJoins', + [$this->qb]); + $this->assertEquals( + 'SELECT o FROM Metaclass\FilterBundle\Entity\TestEntity o LEFT JOIN o.toMany toMany_a1 LEFT JOIN o.toOneNullable toOneNullable_a2', + $this->qb->getDQL(), + 'DQL after replaceInnerJoinsByLeftJoins' + ); + } + + public function testInnerJoinsLeftWithExistsFilter() + { + $operator = 'or'; + $reqData = null; + parse_str('or[exists][toMany.bool]=true&or[exists][toOneNullable.dd]=false', $reqData); + // var_dump($reqData); + $context = ['filters' => $reqData]; + $args = [$this->qb, $this->queryNameGen, TestEntity::class, 'get', $context]; + $result = Reflection::callMethod($this->filterLogic, 'doGenerate', $args); + + $this->assertEquals(1, count($result), 'number of expressions'); + $this->assertEquals( + str_replace(' +', '', "toMany_a1.bool IS NOT NULL OR toOneNullable_a2.dd IS NULL"), + (string) $result[0], + 'DQL'); + } + + + #TODO: add tests for Exists filter with nested prop } \ No newline at end of file diff --git a/tests/Utils/Reflection.php b/tests/Utils/Reflection.php index b2c0f9d..383485d 100644 --- a/tests/Utils/Reflection.php +++ b/tests/Utils/Reflection.php @@ -5,6 +5,17 @@ class Reflection { + /** @throws \ReflectionException */ + public static function getProperty($objectOrClass, $name) + { + $rp = new \ReflectionProperty($objectOrClass, $name); + $rp->setAccessible(true); + + return is_string($objectOrClass) + ? $rp->getValue() + : $rp->getValue($objectOrClass); + } + /** @throws \ReflectionException */ public static function setProperty($objectOrClass, $name, $value) {