From a99b11f6780cefbd770a7fbbf8abefde7620b762 Mon Sep 17 00:00:00 2001 From: Sergii Dolgushev Date: Thu, 24 Aug 2023 12:57:54 +0100 Subject: [PATCH] Got rid of redundant JOINs/WHEREs of WhereInWalker --- lib/Doctrine/ORM/Query/AST/WhereClause.php | 2 +- .../ORM/Tools/Pagination/Paginator.php | 40 +++++- .../ORM/Tools/Pagination/WhereInWalker.php | 101 +++++++++++-- .../ORM/Tools/Pagination/PaginatorTest.php | 135 +++++++++++++++++- 4 files changed, 257 insertions(+), 21 deletions(-) diff --git a/lib/Doctrine/ORM/Query/AST/WhereClause.php b/lib/Doctrine/ORM/Query/AST/WhereClause.php index 3e69600a928..77d3630a323 100644 --- a/lib/Doctrine/ORM/Query/AST/WhereClause.php +++ b/lib/Doctrine/ORM/Query/AST/WhereClause.php @@ -11,7 +11,7 @@ */ class WhereClause extends Node { - /** @var ConditionalExpression|ConditionalTerm */ + /** @var ConditionalExpression|ConditionalTerm|ConditionalPrimary */ public $conditionalExpression; /** @param ConditionalExpression $conditionalExpression */ diff --git a/lib/Doctrine/ORM/Tools/Pagination/Paginator.php b/lib/Doctrine/ORM/Tools/Pagination/Paginator.php index b3572edfe0d..1a4393bbad7 100644 --- a/lib/Doctrine/ORM/Tools/Pagination/Paginator.php +++ b/lib/Doctrine/ORM/Tools/Pagination/Paginator.php @@ -10,8 +10,12 @@ use Doctrine\ORM\Internal\SQLResultCasing; use Doctrine\ORM\NoResultException; use Doctrine\ORM\Query; +use Doctrine\ORM\Query\AST\SelectExpression; +use Doctrine\ORM\Query\AST\SelectStatement; +use Doctrine\ORM\Query\AST\Subselect; use Doctrine\ORM\Query\Parameter; use Doctrine\ORM\Query\Parser; +use Doctrine\ORM\Query\QueryException; use Doctrine\ORM\Query\ResultSetMapping; use Doctrine\ORM\QueryBuilder; use IteratorAggregate; @@ -155,7 +159,9 @@ public function getIterator() return new ArrayIterator([]); } - $whereInQuery = $this->cloneQuery($this->query); + // all where conditions are reset for queries without sub-select + $hasSubselect = $this->queryHasSubselect($this->query); + $whereInQuery = $this->cloneQuery($this->query, $hasSubselect); $ids = array_map('current', $foundIdRows); $this->appendTreeWalker($whereInQuery, WhereInWalker::class); @@ -178,11 +184,14 @@ public function getIterator() return new ArrayIterator($result); } - private function cloneQuery(Query $query): Query + private function cloneQuery(Query $query, bool $withParams = true): Query { $cloneQuery = clone $query; - $cloneQuery->setParameters(clone $query->getParameters()); + if ($withParams) { + $cloneQuery->setParameters(clone $query->getParameters()); + } + $cloneQuery->setCacheable(false); foreach ($query->getHints() as $name => $value) { @@ -286,4 +295,29 @@ private function convertWhereInIdentifiersToDatabaseValues(array $identifiers): return $connection->convertToDatabaseValue($id, $type); }, $identifiers); } + + private function queryHasSubselect(Query $query): bool + { + try { + $AST = $this->query->getAST(); + } catch (QueryException $e) { + return false; + } + + if (! $AST instanceof SelectStatement) { + return false; + } + + foreach ($AST->selectClause->selectExpressions as $selectExpression) { + if (! $selectExpression instanceof SelectExpression) { + continue; + } + + if ($selectExpression->expression instanceof Subselect) { + return true; + } + } + + return false; + } } diff --git a/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php b/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php index 1b8da4dffd2..17a98dcfb3a 100644 --- a/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php +++ b/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php @@ -12,14 +12,17 @@ use Doctrine\ORM\Query\AST\InputParameter; use Doctrine\ORM\Query\AST\NullComparisonExpression; use Doctrine\ORM\Query\AST\PathExpression; +use Doctrine\ORM\Query\AST\SelectExpression; use Doctrine\ORM\Query\AST\SelectStatement; use Doctrine\ORM\Query\AST\SimpleArithmeticExpression; +use Doctrine\ORM\Query\AST\Subselect; use Doctrine\ORM\Query\AST\WhereClause; use Doctrine\ORM\Query\TreeWalkerAdapter; use RuntimeException; use function count; use function in_array; +use function is_string; use function reset; /** @@ -82,34 +85,110 @@ public function walkSelectStatement(SelectStatement $AST) $conditionalPrimary = new ConditionalPrimary(); $conditionalPrimary->simpleConditionalExpression = $expression; + if ($this->hasSubselect($AST) && $AST->whereClause) { + if ($AST->whereClause->conditionalExpression instanceof ConditionalTerm) { + $AST->whereClause->conditionalExpression->conditionalFactors[] = $conditionalPrimary; + } elseif ($AST->whereClause->conditionalExpression instanceof ConditionalPrimary) { + $AST->whereClause->conditionalExpression = new ConditionalExpression( + [ + new ConditionalTerm( + [ + $AST->whereClause->conditionalExpression, + $conditionalPrimary, + ] + ), + ] + ); + } else { + $tmpPrimary = new ConditionalPrimary(); + $tmpPrimary->conditionalExpression = $AST->whereClause->conditionalExpression; + $AST->whereClause->conditionalExpression = new ConditionalTerm( + [ + $tmpPrimary, + $conditionalPrimary, + ] + ); + } + } else { + $AST->whereClause = new WhereClause( + new ConditionalExpression( + [new ConditionalTerm([$conditionalPrimary])] + ) + ); + } - $AST->whereClause = new WhereClause( - new ConditionalExpression( - [new ConditionalTerm([$conditionalPrimary])] - ) - ); - - if ($this->onlyFromIsUsedInSelect($AST)) { + if ($this->clausesAreUsingOnlyFromIdnetifications($AST)) { foreach ($AST->fromClause->identificationVariableDeclarations as $f) { $f->joins = []; } } } - /** @return bool */ - private function onlyFromIsUsedInSelect(SelectStatement $AST) + private function clausesAreUsingOnlyFromIdnetifications(SelectStatement $AST): bool { $fromAliases = []; foreach ($AST->fromClause->identificationVariableDeclarations as $f) { $fromAliases[] = $f->rangeVariableDeclaration->aliasIdentificationVariable; } - foreach ($AST->selectClause->selectExpressions as $s) { - if (! in_array($s->expression, $fromAliases, true)) { + foreach ($AST->selectClause->selectExpressions as $selectExpression) { + if (! $this->isExpressionExistsInFromClause($selectExpression->expression, $fromAliases)) { return false; } } + if ($AST->groupByClause !== null) { + foreach ($AST->groupByClause->groupByItems as $groupByItem) { + if (! $this->isExpressionExistsInFromClause($groupByItem->expression, $fromAliases)) { + return false; + } + } + } + + if ($AST->havingClause !== null) { + return false; + } + + if ($AST->orderByClause !== null) { + foreach ($AST->orderByClause->orderByItems as $orderByItem) { + if (! $this->isExpressionExistsInFromClause($orderByItem->expression, $fromAliases)) { + return false; + } + } + } + return true; } + + /** + * @param mixed $expression + * @param string[] $fromAliases + */ + private function isExpressionExistsInFromClause($expression, array $fromAliases): bool + { + $expressionIdentification = null; + + if ($expression instanceof PathExpression) { + $expressionIdentification = $expression->identificationVariable; + } elseif (is_string($expression)) { + $expressionIdentification = $expression; + } + + if ($expressionIdentification === null) { + return false; + } + + return in_array($expression, $fromAliases, true); + } + + private function hasSubselect(SelectStatement $AST): bool + { + foreach ($AST->selectClause->selectExpressions as $selectExpression) { + if ($selectExpression instanceof SelectExpression && $selectExpression->expression instanceof Subselect) { + return true; + } + } + + return false; + } } diff --git a/tests/Doctrine/Tests/ORM/Tools/Pagination/PaginatorTest.php b/tests/Doctrine/Tests/ORM/Tools/Pagination/PaginatorTest.php index 1c8b47ac0cf..0b6f7b21cab 100644 --- a/tests/Doctrine/Tests/ORM/Tools/Pagination/PaginatorTest.php +++ b/tests/Doctrine/Tests/ORM/Tools/Pagination/PaginatorTest.php @@ -15,8 +15,12 @@ use Doctrine\Tests\Mocks\ConnectionMock; use Doctrine\Tests\OrmTestCase; use Doctrine\Tests\PHPUnitCompatibility\MockBuilderCompatibilityTools; +use Generator; use PHPUnit\Framework\MockObject\MockObject; +use function call_user_func; +use function preg_replace; + class PaginatorTest extends OrmTestCase { use MockBuilderCompatibilityTools; @@ -96,18 +100,18 @@ public function testPaginatorNotCaringAboutExtraParametersWithoutOutputWalkers() public function testgetIteratorDoesCareAboutExtraParametersWithoutOutputWalkersWhenResultIsNotEmpty(): void { - $this->connection->expects(self::exactly(1))->method('executeQuery'); - $this->expectException(Query\QueryException::class); - $this->expectExceptionMessage('Too many parameters: the query defines 1 parameters and you bound 2'); + $this->connection->expects(self::exactly(2))->method('executeQuery'); - $this->createPaginatorWithExtraParametersWithoutOutputWalkers([[10]])->getIterator(); + $this->createPaginatorWithExtraParametersWithoutOutputWalkers([[10]], null)->getIterator(); } /** @param int[][] $willReturnRows */ - private function createPaginatorWithExtraParametersWithoutOutputWalkers(array $willReturnRows): Paginator + private function createPaginatorWithExtraParametersWithoutOutputWalkers(array $willReturnRows, ?array $results = []): Paginator { $this->hydrator->method('hydrateAll')->willReturn($willReturnRows); - $this->connection->method('executeQuery')->with(self::anything(), []); + if ($results !== null) { + $this->connection->method('executeQuery')->with(self::anything(), []); + } $query = new Query($this->em); $query->setDQL('SELECT u FROM Doctrine\\Tests\\Models\\CMS\\CmsUser u'); @@ -116,4 +120,123 @@ private function createPaginatorWithExtraParametersWithoutOutputWalkers(array $w return (new Paginator($query, true))->setUseOutputWalkers(false); } + + /** @dataProvider dataRedunandQueryPartsAreRemovedForWhereInWalker */ + public function testRedunandQueryPartsAreRemovedForWhereInWalker( + string $dql, + array $params, + array $expectedQueries + ): void { + $this->hydrator->method('hydrateAll')->willReturn([[10]]); + + $query = new Query($this->em); + $query->setDQL($dql); + $query->setParameters($params); + + $query->setMaxResults(1); + $paginator = (new Paginator($query, true))->setUseOutputWalkers(false); + + $queryIndex = 0; + $resultMock = $this->createMock(Result::class); + $assertEquals = [$this, 'assertEquals']; + $this->connection + ->expects($this->exactly(2)) + ->method('executeQuery') + ->willReturnCallback(static function (string $actualSql) use ($expectedQueries, $resultMock, $assertEquals, &$queryIndex): Result { + $expectedSql = preg_replace('!\s+!', ' ', $expectedQueries[$queryIndex]); + call_user_func($assertEquals, $expectedSql, $actualSql); + + $queryIndex++; + + return $resultMock; + }); + + $paginator->getIterator(); + } + + public static function dataRedunandQueryPartsAreRemovedForWhereInWalker(): Generator + { + yield 'join that is used in where only' => [ + 'SELECT u + FROM Doctrine\\Tests\\Models\\CMS\\CmsUser u + JOIN Doctrine\\Tests\\Models\\CMS\\CmsAddress a + WHERE a.city = :filterCity', + ['filterCity' => 'London'], + [ + 'SELECT DISTINCT c0_.id AS id_0 FROM cms_users c0_ INNER JOIN cms_addresses c1_ WHERE c1_.city = ? LIMIT 1', + 'SELECT c0_.id AS id_0, c0_.status AS status_1, c0_.username AS username_2, c0_.name AS name_3, c0_.email_id AS email_id_4 + FROM cms_users c0_ + WHERE c0_.id IN (?)', + ], + ]; + + yield 'join that is used in select and where' => [ + 'SELECT u, a + FROM Doctrine\\Tests\\Models\\CMS\\CmsUser u + JOIN Doctrine\\Tests\\Models\\CMS\\CmsAddress a + WHERE a.city = :filterCity', + ['filterCity' => 'London'], + [ + 'SELECT DISTINCT c0_.id AS id_0 FROM cms_users c0_ INNER JOIN cms_addresses c1_ WHERE c1_.city = ? LIMIT 1', + 'SELECT + c0_.id AS id_0, c0_.status AS status_1, c0_.username AS username_2, c0_.name AS name_3, c1_.id AS id_4, + c1_.country AS country_5, c1_.zip AS zip_6, c1_.city AS city_7, c0_.email_id AS email_id_8, c1_.user_id AS user_id_9 + FROM cms_users c0_ + INNER JOIN cms_addresses c1_ + WHERE c0_.id IN (?)', + ], + ]; + + yield 'subselect with parameter' => [ + 'SELECT u, + ( + SELECT MAX(article.version) + FROM Doctrine\\Tests\\Models\\CMS\\CmsArticle article + WHERE article.user = u AND 1 = :paramInSubSelect + ) AS HIDDEN max_version + FROM Doctrine\\Tests\\Models\\CMS\\CmsUser u + JOIN Doctrine\\Tests\\Models\\CMS\\CmsAddress a + WHERE a.city = :filterCity', + ['filterCity' => 'London', 'paramInSubSelect' => 1], + [ + 'SELECT DISTINCT c0_.id AS id_0 FROM cms_users c0_ INNER JOIN cms_addresses c1_ WHERE c1_.city = ? LIMIT 1', + 'SELECT + c0_.id AS id_0, c0_.status AS status_1, c0_.username AS username_2, c0_.name AS name_3, + (SELECT MAX(c1_.version) AS sclr_5 + FROM cms_articles c1_ + WHERE c1_.user_id = c0_.id AND 1 = ?) + AS sclr_4, + c0_.email_id AS email_id_6 FROM cms_users c0_ + INNER JOIN cms_addresses c2_ + WHERE c2_.city = ? + AND c0_.id IN (?)', + ], + ]; + + yield 'subselect without parameter' => [ + 'SELECT u, + ( + SELECT MAX(article.version) + FROM Doctrine\\Tests\\Models\\CMS\\CmsArticle article + WHERE article.user = u + ) AS HIDDEN max_version + FROM Doctrine\\Tests\\Models\\CMS\\CmsUser u + JOIN Doctrine\\Tests\\Models\\CMS\\CmsAddress a + WHERE a.city = :filterCity', + ['filterCity' => 'London'], + [ + 'SELECT DISTINCT c0_.id AS id_0 FROM cms_users c0_ INNER JOIN cms_addresses c1_ WHERE c1_.city = ? LIMIT 1', + 'SELECT + c0_.id AS id_0, c0_.status AS status_1, c0_.username AS username_2, c0_.name AS name_3, + (SELECT MAX(c1_.version) AS sclr_5 + FROM cms_articles c1_ + WHERE c1_.user_id = c0_.id) + AS sclr_4, + c0_.email_id AS email_id_6 FROM cms_users c0_ + INNER JOIN cms_addresses c2_ + WHERE c2_.city = ? + AND c0_.id IN (?)', + ], + ]; + } }