Got rid of redundant JOINs/WHEREs of WhereInWalker

doctrine · Aug 24, 2023 · a99b11f · a99b11f
1 parent 926c04d
commit a99b11f
Show file tree

Hide file tree

Showing 4 changed files with 257 additions and 21 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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 (?)',
+            ],
+        ];
+    }
 }