Fix different first/max result values taking up query cache space

UPGRADE.md

@@ -1,5 +1,14 @@
 # Upgrade to 2.20
 
+## Add `Doctrine\ORM\Query\OutputWalker` interface, deprecate `Doctrine\ORM\Query\SqlWalker::getExecutor()`
+
+Output walkers should implement the new `\Doctrine\ORM\Query\OutputWalker` interface and create
+`Doctrine\ORM\Query\Exec\SqlFinalizer` instances instead of `Doctrine\ORM\Query\Exec\AbstractSqlExecutor`s.
+The output walker must not base its workings on the query `firstResult`/`maxResult` values, so that the 
+`SqlFinalizer` can be kept in the query cache and used regardless of the actual `firstResult`/`maxResult` values.
+Any operation dependent on `firstResult`/`maxResult` should take place within the `SqlFinalizer::createExecutor()`
+method. Details can be found at https://github.com/doctrine/orm/pull/11188.
+
 ## Explictly forbid property hooks
 
 Property hooks are not supported yet by Doctrine ORM. Until support is added,

psalm-baseline.xml

@@ -1868,6 +1868,12 @@
       <code><![CDATA[$this->_sqlStatements = &$this->sqlStatements]]></code>
     </UnsupportedPropertyReferenceUsage>
   </file>
+  <file src="src/Query/Exec/FinalizedSelectExecutor.php">
+    <PropertyNotSetInConstructor>
+      <code><![CDATA[FinalizedSelectExecutor]]></code>
+      <code><![CDATA[FinalizedSelectExecutor]]></code>
+    </PropertyNotSetInConstructor>
+  </file>
   <file src="src/Query/Exec/MultiTableDeleteExecutor.php">
     <InvalidReturnStatement>
       <code><![CDATA[$numDeleted]]></code>
@@ -1996,6 +2002,9 @@
     <ArgumentTypeCoercion>
       <code><![CDATA[$stringPattern]]></code>
     </ArgumentTypeCoercion>
+    <DeprecatedMethod>
+      <code><![CDATA[setSqlExecutor]]></code>
+    </DeprecatedMethod>
     <InvalidNullableReturnType>
       <code><![CDATA[AST\SelectStatement|AST\UpdateStatement|AST\DeleteStatement]]></code>
     </InvalidNullableReturnType>
@@ -2057,11 +2066,6 @@
       <code><![CDATA[$token === TokenType::T_IDENTIFIER]]></code>
     </RedundantConditionGivenDocblockType>
   </file>
-  <file src="src/Query/ParserResult.php">
-    <PropertyNotSetInConstructor>
-      <code><![CDATA[$sqlExecutor]]></code>
-    </PropertyNotSetInConstructor>
-  </file>
   <file src="src/Query/QueryExpressionVisitor.php">
     <InvalidReturnStatement>
       <code><![CDATA[new ArrayCollection($this->parameters)]]></code>

src/Query.php

@@ -18,6 +18,8 @@
 use Doctrine\ORM\Query\AST\SelectStatement;
 use Doctrine\ORM\Query\AST\UpdateStatement;
 use Doctrine\ORM\Query\Exec\AbstractSqlExecutor;
+use Doctrine\ORM\Query\Exec\SqlFinalizer;
+use Doctrine\ORM\Query\OutputWalker;
 use Doctrine\ORM\Query\Parameter;
 use Doctrine\ORM\Query\ParameterTypeInferer;
 use Doctrine\ORM\Query\Parser;
@@ -33,6 +35,7 @@
 use function count;
 use function get_debug_type;
 use function in_array;
+use function is_a;
 use function is_int;
 use function ksort;
 use function md5;
@@ -196,7 +199,7 @@ class Query extends AbstractQuery
      */
     public function getSQL()
     {
-        return $this->parse()->getSqlExecutor()->getSqlStatements();
+        return $this->getSqlExecutor()->getSqlStatements();
     }
 
     /**
@@ -285,7 +288,7 @@ private function parse(): ParserResult
      */
     protected function _doExecute()
     {
-        $executor = $this->parse()->getSqlExecutor();
+        $executor = $this->getSqlExecutor();
 
         if ($this->_queryCacheProfile) {
             $executor->setQueryCacheProfile($this->_queryCacheProfile);
@@ -813,11 +816,31 @@ protected function getQueryCacheId(): string
     {
         ksort($this->_hints);
 
+        if (! $this->hasHint(self::HINT_CUSTOM_OUTPUT_WALKER)) {
+            // Assume Parser will create the SqlOutputWalker; save is_a call, which might trigger a class load
+            $firstAndMaxResult = '';
+        } else {
+            $outputWalkerClass = $this->getHint(self::HINT_CUSTOM_OUTPUT_WALKER);
+            if (is_a($outputWalkerClass, OutputWalker::class, true)) {
+                $firstAndMaxResult = '';
+            } else {
+                Deprecation::trigger(
+                    'doctrine/orm',
+                    'https://github.com/doctrine/orm/pull/11188/',
+                    'Your output walker class %s should implement %s in order to provide a %s. This also means the output walker should not use the query firstResult/maxResult values, which should be read from the query by the SqlFinalizer only.',
+                    $outputWalkerClass,
+                    OutputWalker::class,
+                    SqlFinalizer::class
+                );
+                $firstAndMaxResult = '&firstResult=' . $this->firstResult . '&maxResult=' . $this->maxResults;
+            }
+        }
+
         return md5(
             $this->getDQL() . serialize($this->_hints) .
             '&platform=' . get_debug_type($this->getEntityManager()->getConnection()->getDatabasePlatform()) .
             ($this->_em->hasFilters() ? $this->_em->getFilters()->getHash() : '') .
-            '&firstResult=' . $this->firstResult . '&maxResult=' . $this->maxResults .
+            $firstAndMaxResult .
             '&hydrationMode=' . $this->_hydrationMode . '&types=' . serialize($this->parsedTypes) . 'DOCTRINE_QUERY_CACHE_SALT'
         );
     }
@@ -836,4 +859,9 @@ public function __clone()
 
         $this->state = self::STATE_DIRTY;
     }
+
+    private function getSqlExecutor(): AbstractSqlExecutor
+    {
+        return $this->parse()->prepareSqlExecutor($this);
+    }
 }

src/Query/Exec/FinalizedSelectExecutor.php

@@ -0,0 +1,33 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\ORM\Query\Exec;
+
+use Doctrine\DBAL\Connection;
+use Doctrine\DBAL\Result;
+use Doctrine\DBAL\Types\Type;
+
+/**
+ * SQL executor for a given, final, single SELECT SQL query
+ *
+ * @method string getSqlStatements()
+ */
+class FinalizedSelectExecutor extends AbstractSqlExecutor
+{
+    public function __construct(string $sql)
+    {
+        parent::__construct();
+
+        $this->sqlStatements = $sql;
+    }
+
+    /**
+     * @param list<mixed>|array<string, mixed>                                     $params
+     * @param array<int, int|string|Type|null>|array<string, int|string|Type|null> $types
+     */
+    public function execute(Connection $conn, array $params, array $types): Result
+    {
+        return $conn->executeQuery($this->getSqlStatements(), $params, $types, $this->queryCacheProfile);
+    }
+}

src/Query/Exec/PreparedExecutorFinalizer.php

@@ -0,0 +1,28 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\ORM\Query\Exec;
+
+use Doctrine\ORM\Query;
+
+/**
+ * PreparedExecutorFinalizer is a wrapper for the SQL finalization
+ * phase that does nothing - it is constructed with the sql executor
+ * already.
+ */
+final class PreparedExecutorFinalizer implements SqlFinalizer
+{
+    /** @var AbstractSqlExecutor */
+    private $executor;
+
+    public function __construct(AbstractSqlExecutor $exeutor)
+    {
+        $this->executor = $exeutor;
+    }
+
+    public function createExecutor(Query $query): AbstractSqlExecutor
+    {
+        return $this->executor;
+    }
+}

src/Query/Exec/SingleSelectSqlFinalizer.php

@@ -0,0 +1,64 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\ORM\Query\Exec;
+
+use Doctrine\DBAL\LockMode;
+use Doctrine\ORM\Query;
+use Doctrine\ORM\Query\QueryException;
+use Doctrine\ORM\Utility\LockSqlHelper;
+
+/**
+ * SingleSelectSqlFinalizer finalizes a given SQL query by applying
+ * the query's firstResult/maxResult values as well as extra read lock/write lock
+ * statements, both through the platform-specific methods.
+ *
+ * The resulting, "finalized" SQL is passed to a FinalizedSelectExecutor.
+ */
+class SingleSelectSqlFinalizer implements SqlFinalizer
+{
+    use LockSqlHelper;
+
+    /** @var string */
+    private $sql;
+
+    public function __construct(string $sql)
+    {
+        $this->sql = $sql;
+    }
+
+    /**
+     * This method exists temporarily to support old SqlWalker interfaces.
+     *
+     * @internal
+     *
+     * @psalm-internal Doctrine\ORM
+     */
+    public function finalizeSql(Query $query): string
+    {
+        $platform = $query->getEntityManager()->getConnection()->getDatabasePlatform();
+
+        $sql = $platform->modifyLimitQuery($this->sql, $query->getMaxResults(), $query->getFirstResult());
+
+        $lockMode = $query->getHint(Query::HINT_LOCK_MODE) ?: LockMode::NONE;
+
+        if ($lockMode !== LockMode::NONE && $lockMode !== LockMode::OPTIMISTIC && $lockMode !== LockMode::PESSIMISTIC_READ && $lockMode !== LockMode::PESSIMISTIC_WRITE) {
+            throw QueryException::invalidLockMode();
+        }
+
+        if ($lockMode === LockMode::PESSIMISTIC_READ) {
+            $sql .= ' ' . $this->getReadLockSQL($platform);
+        } elseif ($lockMode === LockMode::PESSIMISTIC_WRITE) {
+            $sql .= ' ' . $this->getWriteLockSQL($platform);
+        }
+
+        return $sql;
+    }
+
+    /** @return FinalizedSelectExecutor */
+    public function createExecutor(Query $query): AbstractSqlExecutor
+    {
+        return new FinalizedSelectExecutor($this->finalizeSql($query));
+    }
+}

src/Query/Exec/SqlFinalizer.php

@@ -0,0 +1,26 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\ORM\Query\Exec;
+
+use Doctrine\ORM\Query;
+
+/**
+ * SqlFinalizers are created by OutputWalkers that traversed the DQL AST.
+ * The SqlFinalizer instance can be kept in the query cache and re-used
+ * at a later time.
+ *
+ * Once the SqlFinalizer has been created or retrieved from the query cache,
+ * it receives the Query object again in order to yield the AbstractSqlExecutor
+ * that will then be used to execute the query.
+ *
+ * The SqlFinalizer may assume that the DQL that was used to build the AST
+ * and run the OutputWalker (which created the SqlFinalizer) is equivalent to
+ * the query that will be passed to the createExecutor() method. Potential differences
+ * are the parameter values or firstResult/maxResult settings.
+ */
+interface SqlFinalizer
+{
+    public function createExecutor(Query $query): AbstractSqlExecutor;
+}

src/Query/OutputWalker.php

@@ -0,0 +1,28 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\ORM\Query;
+
+use Doctrine\ORM\Query\Exec\SqlFinalizer;
+
+/**
+ * Interface for output walkers
+ *
+ * Output walkers, like tree walkers, can traverse the DQL AST to perform
+ * their purpose.
+ *
+ * The goal of an OutputWalker is to ultimately provide the SqlFinalizer
+ * which produces the final, executable SQL statement in a "finalization" phase.
+ *
+ * It must be possible to use the same SqlFinalizer for Queries with different
+ * firstResult/maxResult values. In other words, SQL produced by the
+ * output walker should not depend on those values, and any SQL generation/modification
+ * specific to them should happen in the finalizer's `\Doctrine\ORM\Query\Exec\SqlFinalizer::createExecutor()`
+ * method instead.
+ */
+interface OutputWalker
+{
+    /** @param AST\DeleteStatement|AST\UpdateStatement|AST\SelectStatement $AST */
+    public function getFinalizer($AST): SqlFinalizer;
+}

src/Query/Parser.php

@@ -10,6 +10,7 @@
 use Doctrine\ORM\Mapping\ClassMetadata;
 use Doctrine\ORM\Query;
 use Doctrine\ORM\Query\AST\Functions;
+use Doctrine\ORM\Query\Exec\SqlFinalizer;
 use LogicException;
 use ReflectionClass;
 
@@ -396,11 +397,26 @@ public function parse()
             $this->queryComponents = $treeWalkerChain->getQueryComponents();
         }
 
-        $outputWalkerClass = $this->customOutputWalker ?: SqlWalker::class;
+        $outputWalkerClass = $this->customOutputWalker ?: SqlOutputWalker::class;
         $outputWalker      = new $outputWalkerClass($this->query, $this->parserResult, $this->queryComponents);
 
-        // Assign an SQL executor to the parser result
-        $this->parserResult->setSqlExecutor($outputWalker->getExecutor($AST));
+        if ($outputWalker instanceof OutputWalker) {
+            $finalizer = $outputWalker->getFinalizer($AST);
+            $this->parserResult->setSqlFinalizer($finalizer);
+        } else {
+            Deprecation::trigger(
+                'doctrine/orm',
+                'https://github.com/doctrine/orm/pull/11188/',
+                'Your output walker class %s should implement %s in order to provide a %s. This also means the output walker should not use the query firstResult/maxResult values, which should be read from the query by the SqlFinalizer only.',
+                $outputWalkerClass,
+                OutputWalker::class,
+                SqlFinalizer::class
+            );
+            // @phpstan-ignore method.deprecated
+            $executor = $outputWalker->getExecutor($AST);
+            // @phpstan-ignore method.deprecated
+            $this->parserResult->setSqlExecutor($executor);
+        }
 
         return $this->parserResult;
     }