Fix double SQL finalization (#55)

Author: janedbal

README.md

@@ -51,6 +51,11 @@ class MaxExecutionTimeSqlWalker extends HintHandler
 
 SqlNode is an enum of all `walkXxx` methods in Doctrine's SqlWalker, so you are able to intercept any part of AST processing the SqlWalker does.
 
+### Limitations
+- Please note that since [doctrine/orm 3.3.0](https://github.com/doctrine/orm/pull/11188), the produced SQL gets finalized with `LIMIT` / `OFFSET` / `FOR UPDATE` after `SqlWalker` processing is done.
+  - Thus, implementors should be aware that those SQL parts can be appended to the SQL after `HintHandler` processing.
+  - This means that e.g. placing a comment at the end of the SQL breaks LIMIT functionality completely
+
 ### Implementors
 - [shipmonk/doctrine-mysql-optimizer-hints](https://github.com/shipmonk-rnd/doctrine-mysql-optimizer-hints) (since v2)
 - [shipmonk/doctrine-mysql-index-hints](https://github.com/shipmonk-rnd/doctrine-mysql-index-hints) (since v3)

composer.json

@@ -19,7 +19,9 @@
         "phpunit/phpunit": "10.5.36",
         "shipmonk/composer-dependency-analyser": "1.7.0",
         "shipmonk/phpstan-rules": "3.2.1",
-        "slevomat/coding-standard": "8.15.0"
+        "slevomat/coding-standard": "8.15.0",
+        "symfony/cache": "^6.4.13",
+        "symfony/cache-contracts": "^3.5.0"
     },
     "autoload": {
         "psr-4": {

phpcs.xml.dist

@@ -269,7 +269,6 @@
     <rule ref="SlevomatCodingStandard.Classes.EmptyLinesAroundClassBraces"/>
     <rule ref="SlevomatCodingStandard.Classes.TraitUseDeclaration" />
     <rule ref="SlevomatCodingStandard.Classes.TraitUseSpacing" />
-    <rule ref="SlevomatCodingStandard.Classes.DisallowConstructorPropertyPromotion" />
     <rule ref="SlevomatCodingStandard.Commenting.DocCommentSpacing">
         <properties>
             <property name="linesCountBeforeFirstContent" value="0"/>
@@ -305,7 +304,6 @@
     <rule ref="SlevomatCodingStandard.Functions.StaticClosure"/>
     <rule ref="SlevomatCodingStandard.Functions.UselessParameterDefaultValue"/>
     <rule ref="SlevomatCodingStandard.Functions.UnusedInheritedVariablePassedToClosure"/>
-    <rule ref="SlevomatCodingStandard.Functions.DisallowArrowFunction"/>
     <rule ref="SlevomatCodingStandard.Namespaces.UseFromSameNamespace"/>
     <rule ref="SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses">
         <properties>

src/HintDrivenSqlWalker.php

@@ -53,19 +53,15 @@
 use Doctrine\ORM\Query\AST\UpdateItem;
 use Doctrine\ORM\Query\AST\UpdateStatement;
 use Doctrine\ORM\Query\AST\WhereClause;
-use Doctrine\ORM\Query\Exec\PreparedExecutorFinalizer;
-use Doctrine\ORM\Query\Exec\SingleSelectSqlFinalizer;
-use Doctrine\ORM\Query\Exec\SqlFinalizer;
-use Doctrine\ORM\Query\OutputWalker;
 use Doctrine\ORM\Query\Parser;
-use Doctrine\ORM\Query\SqlWalker;
+use Doctrine\ORM\Query\SqlOutputWalker;
 use LogicException;
 use function is_a;
 
 /**
  * @psalm-import-type QueryComponent from Parser
  */
-class HintDrivenSqlWalker extends SqlWalker implements OutputWalker
+class HintDrivenSqlWalker extends SqlOutputWalker
 {
 
     /**
@@ -95,20 +91,10 @@ public function __construct(
         }
     }
 
-    public function getFinalizer(DeleteStatement|UpdateStatement|SelectStatement $AST): SqlFinalizer
+    protected function createSqlForFinalizer(SelectStatement $selectStatement): string
     {
-        switch (true) {
-            case $AST instanceof SelectStatement:
-                return new SingleSelectSqlFinalizer($this->walkSelectStatement($AST));
-
-            case $AST instanceof UpdateStatement:
-                return new PreparedExecutorFinalizer($this->createUpdateStatementExecutor($AST));
-
-            case $AST instanceof DeleteStatement: // @phpstan-ignore instanceof.alwaysTrue (keep it readable)
-                return new PreparedExecutorFinalizer($this->createDeleteStatementExecutor($AST));
-        }
-
-        throw new LogicException('Unexpected AST node type');
+        $selectStatementSql = parent::createSqlForFinalizer($selectStatement);
+        return $this->callWalkers(SqlNode::SelectStatement, $selectStatementSql);
     }
 
     public function walkSelectStatement(SelectStatement $AST): string

tests/HintDrivenSqlWalkerTest.php

@@ -14,63 +14,133 @@
 use PHPUnit\Framework\TestCase;
 use ShipMonk\Doctrine\Walker\Handlers\CommentWholeSqlHintHandler;
 use ShipMonk\Doctrine\Walker\Handlers\LowercaseSelectHintHandler;
-use function sprintf;
+use Symfony\Component\Cache\Adapter\ArrayAdapter;
 
 class HintDrivenSqlWalkerTest extends TestCase
 {
 
     /**
+     * @param callable(EntityManager):Query $queryCallback
      * @dataProvider walksProvider
      */
     public function testWalker(
-        string $dql,
+        callable $queryCallback,
         string $handlerClass,
         mixed $hintValue,
         string $expectedSql,
     ): void
     {
         $entityManagerMock = $this->createEntityManagerMock();
 
-        $query = new Query($entityManagerMock);
-        $query->setDQL($dql);
-
+        $query = $queryCallback($entityManagerMock);
         $query->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, HintDrivenSqlWalker::class);
         $query->setHint($handlerClass, $hintValue);
         $producedSql = $query->getSQL();
 
         self::assertSame($expectedSql, $producedSql);
     }
 
+    public function testPagination(): void
+    {
+        $pageSize = 10;
+        $entityManagerMock = $this->createEntityManagerMock();
+
+        self::assertNotNull(
+            $entityManagerMock->getConfiguration()->getQueryCache(),
+            'QueryCache needed. The purpose of this test is to ensure that we do not break the pagination by using a cache',
+        );
+
+        $expectedSqls = [
+            'select d0_.id AS id_0 FROM dummy_entity d0_ LIMIT 10',
+            'select d0_.id AS id_0 FROM dummy_entity d0_ LIMIT 10 OFFSET 10',
+            'select d0_.id AS id_0 FROM dummy_entity d0_ LIMIT 10 OFFSET 20',
+        ];
+
+        foreach ([0, 1, 2] as $page) {
+            $query = $entityManagerMock->createQueryBuilder()
+                ->select('w')
+                ->from(DummyEntity::class, 'w')
+                ->getQuery()
+                ->setHint(Query::HINT_CUSTOM_OUTPUT_WALKER, HintDrivenSqlWalker::class)
+                ->setHint(LowercaseSelectHintHandler::class, null)
+                ->setFirstResult($page * $pageSize)
+                ->setMaxResults($pageSize);
+            $producedSql = $query->getSQL();
+
+            self::assertSame($expectedSqls[$page], $producedSql, 'Page ' . $page . ' failed:');
+        }
+    }
+
     /**
-     * @return Generator<string, array{string, class-string<HintHandler>, mixed, string}>
+     * @return Generator<string, array{callable(EntityManager):Query, class-string<HintHandler>, mixed, string}>
      */
     public static function walksProvider(): iterable
     {
-        $selectDql = sprintf('SELECT w FROM %s w', DummyEntity::class);
+        $selectQueryCallback = static function (EntityManager $entityManager): Query {
+            return $entityManager->createQueryBuilder()
+                ->select('w')
+                ->from(DummyEntity::class, 'w')
+                ->getQuery();
+        };
+
+        $selectWithLimitQueryCallback = static function (EntityManager $entityManager): Query {
+            return $entityManager->createQueryBuilder()
+                ->select('w')
+                ->from(DummyEntity::class, 'w')
+                ->setMaxResults(1)
+                ->getQuery();
+        };
+
+        $updateQueryCallback = static function (EntityManager $entityManager): Query {
+            return $entityManager->createQueryBuilder()
+                ->update(DummyEntity::class, 'w')
+                ->set('w.id', 1)
+                ->getQuery();
+        };
+
+        $deleteQueryCallback = static function (EntityManager $entityManager): Query {
+            return $entityManager->createQueryBuilder()
+                ->delete(DummyEntity::class, 'w')
+                ->getQuery();
+        };
 
         yield 'Lowercase select' => [
-            $selectDql,
+            $selectQueryCallback,
             LowercaseSelectHintHandler::class,
             null,
             'select d0_.id AS id_0 FROM dummy_entity d0_',
         ];
 
+        yield 'Lowercase select with LIMIT' => [
+            $selectWithLimitQueryCallback,
+            LowercaseSelectHintHandler::class,
+            null,
+            'select d0_.id AS id_0 FROM dummy_entity d0_ LIMIT 1',
+        ];
+
         yield 'Comment whole sql - select' => [
-            $selectDql,
+            $selectQueryCallback,
             CommentWholeSqlHintHandler::class,
             'custom comment',
             'SELECT d0_.id AS id_0 FROM dummy_entity d0_ -- custom comment',
         ];
 
+        yield 'Comment whole sql - select with LIMIT' => [
+            $selectWithLimitQueryCallback,
+            CommentWholeSqlHintHandler::class,
+            'custom comment',
+            'SELECT d0_.id AS id_0 FROM dummy_entity d0_ -- custom comment LIMIT 1', // see readme limitations
+        ];
+
         yield 'Comment whole sql - update' => [
-            sprintf('UPDATE %s w SET w.id = 1', DummyEntity::class),
+            $updateQueryCallback,
             CommentWholeSqlHintHandler::class,
             'custom comment',
             'UPDATE dummy_entity SET id = 1 -- custom comment',
         ];
 
         yield 'Comment whole sql - delete' => [
-            sprintf('DELETE FROM %s w', DummyEntity::class),
+            $deleteQueryCallback,
             CommentWholeSqlHintHandler::class,
             'custom comment',
             'DELETE FROM dummy_entity -- custom comment',
@@ -82,6 +152,7 @@ private function createEntityManagerMock(): EntityManager
         $config = new Configuration();
         $config->setProxyNamespace('Tmp\Doctrine\Tests\Proxies');
         $config->setProxyDir('/tmp/doctrine');
+        $config->setQueryCache(new ArrayAdapter());
         $config->setAutoGenerateProxyClasses(false);
         $config->setSecondLevelCacheEnabled(false);
         $config->setMetadataDriverImpl(new AttributeDriver([__DIR__]));