Skip to content

Commit

Permalink
Fix different first/max result values taking up query cache space (#1…
Browse files Browse the repository at this point in the history
…1188)

* Add a test covering the #11112 issue

* Add new OutputWalker and SqlFinalizer interfaces

* Add a SingleSelectSqlFinalizer that can take care of adding offset/limit as well as locking mode statements to a given SQL query.

Add a FinalizedSelectExecutor that executes given, finalized SQL statements.

* In SqlWalker, split SQL query generation into the two parts that shall happen before and after the finalization phase.

Move the part that generates "pre-finalization" SQL into a dedicated method. Use a side channel in SingleSelectSqlFinalizer to access the "finalization" logic and avoid duplication.

* Fix CS violations

* Skip the GH11112 test while applying refactorings

* Avoid a Psalm complaint due to invalid (?) docblock syntax

* Establish alternate code path - queries can obtain the sql executor through the finalizer, parser knows about output walkers yielding finalizers

* Remove a possibly premature comment

* Re-enable the #11112 test

* Fix CS

* Make RootTypeWalker inherit from SqlOutputWalker so it becomes finalizer-aware

* Update QueryCacheTest, since first/max results no longer need extra cache entries

* Fix ParserResultSerializationTest by forcing the parser to produce a ParserResult of the old kind (with the executor already constructed)

* Fix WhereInWalkerTest

* Update lib/Doctrine/ORM/Query/Exec/PreparedExecutorFinalizer.php

Co-authored-by: Grégoire Paris <[email protected]>

* Fix tests

* Fix a Psalm complaint

* Fix a test

* Fix CS

* Make the NullSqlWalker an instance of SqlOutputWalker

* Avoid multiple cache entries caused by LimitSubqueryOutputWalker

* Fix Psalm complaints

* Fix static analysis complaints

* Remove experimental code that I committed accidentally

* Remove unnecessary baseline entry

* Make AddUnknownQueryComponentWalker subclass SqlOutputWalker

That way, we have no remaining classes in the codebase subclassing SqlWalker but not SqlOutputWalker

* Use more expressive exception classes

* Add a deprecation message

* Move SqlExecutor creation to ParserResult, to minimize public methods available on it

* Avoid keeping the SqlExecutor in the Query, since it must be generated just in time (e. g. in case Query parameters change)

* Address PHPStan complaints

* Fix tests

* Small refactorings

* Add an upgrade notice

* Small refactorings

* Update the Psalm baseline

* Add a missing namespace import

* Update Psalm baseline

* Fix CS

* Fix Psalm baseline

---------

Co-authored-by: Grégoire Paris <[email protected]>
  • Loading branch information
mpdude and greg0ire authored Oct 10, 2024
1 parent bea454e commit 39d2136
Show file tree
Hide file tree
Showing 23 changed files with 591 additions and 104 deletions.
9 changes: 9 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
14 changes: 9 additions & 5 deletions psalm-baseline.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand Down
34 changes: 31 additions & 3 deletions src/Query.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -196,7 +199,7 @@ class Query extends AbstractQuery
*/
public function getSQL()
{
return $this->parse()->getSqlExecutor()->getSqlStatements();
return $this->getSqlExecutor()->getSqlStatements();
}

/**
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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'
);
}
Expand All @@ -836,4 +859,9 @@ public function __clone()

$this->state = self::STATE_DIRTY;
}

private function getSqlExecutor(): AbstractSqlExecutor
{
return $this->parse()->prepareSqlExecutor($this);
}
}
33 changes: 33 additions & 0 deletions src/Query/Exec/FinalizedSelectExecutor.php
Original file line number Diff line number Diff line change
@@ -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);
}
}
28 changes: 28 additions & 0 deletions src/Query/Exec/PreparedExecutorFinalizer.php
Original file line number Diff line number Diff line change
@@ -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;
}
}
64 changes: 64 additions & 0 deletions src/Query/Exec/SingleSelectSqlFinalizer.php
Original file line number Diff line number Diff line change
@@ -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));
}
}
26 changes: 26 additions & 0 deletions src/Query/Exec/SqlFinalizer.php
Original file line number Diff line number Diff line change
@@ -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;
}
28 changes: 28 additions & 0 deletions src/Query/OutputWalker.php
Original file line number Diff line number Diff line change
@@ -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;
}
22 changes: 19 additions & 3 deletions src/Query/Parser.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}
Expand Down
Loading

0 comments on commit 39d2136

Please sign in to comment.