Merge pull request #10444 from mpdude/paginator-dql-cacheable

Make Paginator-internal query cacheable in the query cache

lib/Doctrine/ORM/Tools/Pagination/Paginator.php

@@ -21,7 +21,9 @@
 use function array_key_exists;
 use function array_map;
 use function array_sum;
+use function assert;
 use function count;
+use function is_string;
 
 /**
  * The paginator can handle various complex scenarios with DQL.
@@ -160,9 +162,10 @@ public function getIterator()
             $this->appendTreeWalker($whereInQuery, WhereInWalker::class);
             $whereInQuery->setHint(WhereInWalker::HINT_PAGINATOR_ID_COUNT, count($ids));
             $whereInQuery->setFirstResult(0)->setMaxResults(null);
-            $whereInQuery->setParameter(WhereInWalker::PAGINATOR_ID_ALIAS, $ids);
             $whereInQuery->setCacheable($this->query->isCacheable());
-            $whereInQuery->useQueryCache(false);
+
+            $databaseIds = $this->convertWhereInIdentifiersToDatabaseValues($ids);
+            $whereInQuery->setParameter(WhereInWalker::PAGINATOR_ID_ALIAS, $databaseIds);
 
             $result = $whereInQuery->getResult($this->query->getHydrationMode());
         } else {
@@ -265,4 +268,23 @@ private function unbindUnusedQueryParams(Query $query): void
 
         $query->setParameters($parameters);
     }
+
+    /**
+     * @param mixed[] $identifiers
+     *
+     * @return mixed[]
+     */
+    private function convertWhereInIdentifiersToDatabaseValues(array $identifiers): array
+    {
+        $query = $this->cloneQuery($this->query);
+        $query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, RootTypeWalker::class);
+
+        $connection = $this->query->getEntityManager()->getConnection();
+        $type       = $query->getSQL();
+        assert(is_string($type));
+
+        return array_map(static function ($id) use ($connection, $type) {
+            return $connection->convertToDatabaseValue($id, $type);
+        }, $identifiers);
+    }
 }

lib/Doctrine/ORM/Tools/Pagination/RootTypeWalker.php

@@ -0,0 +1,48 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\ORM\Tools\Pagination;
+
+use Doctrine\ORM\Query\AST;
+use Doctrine\ORM\Query\SqlWalker;
+use Doctrine\ORM\Utility\PersisterHelper;
+use RuntimeException;
+
+use function count;
+use function reset;
+
+/**
+ * Infers the DBAL type of the #Id (identifier) column of the given query's root entity, and
+ * returns it in place of a real SQL statement.
+ *
+ * Obtaining this type is a necessary intermediate step for \Doctrine\ORM\Tools\Pagination\Paginator.
+ * We can best do this from a tree walker because it gives us access to the AST.
+ *
+ * Returning the type instead of a "real" SQL statement is a slight hack. However, it has the
+ * benefit that the DQL -> root entity id type resolution can be cached in the query cache.
+ */
+final class RootTypeWalker extends SqlWalker
+{
+    public function walkSelectStatement(AST\SelectStatement $AST): string
+    {
+        // Get the root entity and alias from the AST fromClause
+        $from = $AST->fromClause->identificationVariableDeclarations;
+
+        if (count($from) > 1) {
+            throw new RuntimeException('Can only process queries that select only one FROM component');
+        }
+
+        $fromRoot            = reset($from);
+        $rootAlias           = $fromRoot->rangeVariableDeclaration->aliasIdentificationVariable;
+        $rootClass           = $this->getMetadataForDqlAlias($rootAlias);
+        $identifierFieldName = $rootClass->getSingleIdentifierFieldName();
+
+        return PersisterHelper::getTypeOfField(
+            $identifierFieldName,
+            $rootClass,
+            $this->getQuery()
+                ->getEntityManager()
+        )[0];
+    }
+}

lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php

@@ -17,13 +17,9 @@
 use Doctrine\ORM\Query\AST\SimpleArithmeticExpression;
 use Doctrine\ORM\Query\AST\WhereClause;
 use Doctrine\ORM\Query\TreeWalkerAdapter;
-use Doctrine\ORM\Utility\PersisterHelper;
 use RuntimeException;
 
-use function array_map;
-use function assert;
 use function count;
-use function is_array;
 use function reset;
 
 /**
@@ -80,15 +76,6 @@ public function walkSelectStatement(SelectStatement $AST)
                 $arithmeticExpression,
                 [new InputParameter(':' . self::PAGINATOR_ID_ALIAS)]
             );
-
-            $this->convertWhereInIdentifiersToDatabaseValue(
-                PersisterHelper::getTypeOfField(
-                    $identifierFieldName,
-                    $rootClass,
-                    $this->_getQuery()
-                        ->getEntityManager()
-                )[0]
-            );
         } else {
             $expression = new NullComparisonExpression($pathExpression);
         }
@@ -130,24 +117,4 @@ public function walkSelectStatement(SelectStatement $AST)
             );
         }
     }
-
-    private function convertWhereInIdentifiersToDatabaseValue(string $type): void
-    {
-        $query                = $this->_getQuery();
-        $identifiersParameter = $query->getParameter(self::PAGINATOR_ID_ALIAS);
-
-        assert($identifiersParameter !== null);
-
-        $identifiers = $identifiersParameter->getValue();
-
-        assert(is_array($identifiers));
-
-        $connection = $this->_getQuery()
-            ->getEntityManager()
-            ->getConnection();
-
-        $query->setParameter(self::PAGINATOR_ID_ALIAS, array_map(static function ($id) use ($connection, $type) {
-            return $connection->convertToDatabaseValue($id, $type);
-        }, $identifiers));
-    }
 }

psalm-baseline.xml

@@ -2959,9 +2959,6 @@
       <code>$AST-&gt;whereClause-&gt;conditionalExpression instanceof ConditionalFactor</code>
       <code>$AST-&gt;whereClause-&gt;conditionalExpression instanceof ConditionalPrimary</code>
     </DocblockTypeContradiction>
-    <MissingClosureReturnType>
-      <code>static function ($id) use ($connection, $type) {</code>
-    </MissingClosureReturnType>
     <PossiblyInvalidPropertyAssignmentValue>
       <code>$AST-&gt;whereClause-&gt;conditionalExpression</code>
     </PossiblyInvalidPropertyAssignmentValue>

psalm.xml

@@ -231,5 +231,10 @@
                 <referencedMethod name="Doctrine\DBAL\Platforms\AbstractPlatform::getGuidExpression"/>
             </errorLevel>
         </UndefinedMethod>
+        <MissingClosureReturnType>
+            <errorLevel type="suppress">
+                <file name="lib/Doctrine/ORM/Tools/Pagination/Paginator.php"/>
+            </errorLevel>
+        </MissingClosureReturnType>
     </issueHandlers>
 </psalm>

tests/Doctrine/Tests/ORM/Functional/Ticket/GH7820Test.php

@@ -16,6 +16,7 @@
 use PHPUnit\Framework\Assert;
 use Psr\Cache\CacheItemInterface;
 use Symfony\Component\Cache\Adapter\ArrayAdapter;
+use Symfony\Component\Cache\CacheItem;
 
 use function array_map;
 use function is_string;
@@ -112,6 +113,35 @@ public function save(CacheItemInterface $item): bool
         $this->fetchSongLinesWithPaginator();
     }
 
+    public function testPaginatorQueriesWillBeCached(): void
+    {
+        $cache = new class extends ArrayAdapter {
+            /** @var bool */
+            private $failOnCacheMiss = false;
+
+            public function failOnCacheMiss(): void
+            {
+                $this->failOnCacheMiss = true;
+            }
+
+            public function getItem($key): CacheItem
+            {
+                $item = parent::getItem($key);
+                Assert::assertTrue(! $this->failOnCacheMiss || $item->isHit(), 'cache was missed');
+
+                return $item;
+            }
+        };
+        $this->_em->getConfiguration()->setQueryCache($cache);
+
+        // Prime the cache
+        $this->fetchSongLinesWithPaginator();
+
+        $cache->failOnCacheMiss();
+
+        $this->fetchSongLinesWithPaginator();
+    }
+
     private function fetchSongLinesWithPaginator(): array
     {
         $query = $this->_em->getRepository(GH7820Line::class)

tests/Doctrine/Tests/ORM/Tools/Pagination/RootTypeWalkerTest.php

@@ -0,0 +1,55 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\ORM\Tools\Pagination;
+
+use Doctrine\DBAL\Types\Type;
+use Doctrine\ORM\Query;
+use Doctrine\ORM\Tools\Pagination\RootTypeWalker;
+use Doctrine\Tests\DbalTypes\Rot13Type;
+use Doctrine\Tests\Models\ValueConversionType\AuxiliaryEntity;
+use Doctrine\Tests\Models\ValueConversionType\OwningManyToOneIdForeignKeyEntity;
+use Generator;
+
+class RootTypeWalkerTest extends PaginationTestCase
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        if (! Type::hasType('rot13')) {
+            Type::addType('rot13', Rot13Type::class);
+        }
+    }
+
+    /**
+     * @dataProvider exampleQueries
+     */
+    public function testResolveTypeMapping(string $dqlQuery, string $expectedType): void
+    {
+        $query = $this->entityManager->createQuery($dqlQuery);
+        $query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, RootTypeWalker::class);
+
+        self::assertSame($expectedType, $query->getSQL());
+    }
+
+    /** @return iterable<array{string, string}> */
+    public function exampleQueries(): Generator
+    {
+        yield 'Entity with #Id column of special type' => [
+            'SELECT e.id4 FROM ' . AuxiliaryEntity::class . ' e',
+            'rot13',
+        ];
+
+        yield 'Entity where #Id is a to-one relation with special type identifier' => [
+            'SELECT e FROM ' . OwningManyToOneIdForeignKeyEntity::class . ' e',
+            'rot13',
+        ];
+
+        yield 'Simple integer ID in a query with a JOIN' => [
+            'SELECT u, g FROM Doctrine\Tests\ORM\Tools\Pagination\User u JOIN u.groups g',
+            'integer',
+        ];
+    }
+}