Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Draft][Please review] Make Paginator use simpler queries when there are no sql joins used (#8278) #11595

Draft
wants to merge 10 commits into
base: 3.3.x
Choose a base branch
from
Draft
304 changes: 288 additions & 16 deletions src/Tools/Pagination/Paginator.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,32 @@
use ArrayIterator;
use Countable;
use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Internal\SQLResultCasing;
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\NoResultException;
use Doctrine\ORM\Query;
use Doctrine\ORM\Query\AST\Join;
use Doctrine\ORM\Query\AST\JoinAssociationDeclaration;
use Doctrine\ORM\Query\AST\Node;
use Doctrine\ORM\Query\AST\SelectExpression;
use Doctrine\ORM\Query\Parameter;
use Doctrine\ORM\Query\Parser;
use Doctrine\ORM\Query\ResultSetMapping;
use Doctrine\ORM\QueryBuilder;
use Doctrine\Persistence\Mapping\MappingException;
use IteratorAggregate;
use Traversable;

use function array_key_exists;
use function array_map;
use function array_sum;
use function assert;
use function count;
use function in_array;
use function is_string;
use function reset;
use function str_starts_with;

/**
* The paginator can handle various complex scenarios with DQL.
Expand All @@ -36,13 +47,26 @@
public const HINT_ENABLE_DISTINCT = 'paginator.distinct.enable';

private readonly Query $query;
private bool|null $useOutputWalkers = null;
private int|null $count = null;
private bool|null $useResultQueryOutputWalker = null;
private bool|null $useCountQueryOutputWalker = null;
private int|null $count = null;
/**
* The auto-detection of queries style was added a lot later to this class, and this
* class historically was by default using the more complex queries style, which means that
* the simple queries style is potentially very under-tested in production systems. The purpose
* of this variable is to not introduce breaking changes until an impression is developed that
* the simple queries style has been battle-tested enough.
*/
private bool $queryStyleAutoDetectionEnabled = false;
private bool $queryCouldHaveToManyJoins = true;

/** @param bool $fetchJoinCollection Whether the query joins a collection (true by default). */
/**
* @param bool $queryCouldProduceDuplicates Whether the query could produce partially duplicated records. One case
* when it does is when it joins a collection.
*/
public function __construct(
Query|QueryBuilder $query,
private readonly bool $fetchJoinCollection = true,
private readonly bool $queryCouldProduceDuplicates = true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs need to be updated at the very least because of this. Can you also read it and see if anything else needs to be modified, and whether any new behavior you are adding is worth documenting?

) {
if ($query instanceof QueryBuilder) {
$query = $query->getQuery();
Expand All @@ -51,6 +75,190 @@
$this->query = $query;
}

/**
* Create an instance of Paginator with auto-detection of whether the provided
* query is suitable for simple (and fast) pagination queries, or whether a complex
* set of pagination queries has to be used.
*/
public static function newWithAutoDetection(Query|QueryBuilder $query): self
{
if ($query instanceof QueryBuilder) {
$query = $query->getQuery();
}

$queryAST = $query->getAST();
[
'hasGroupByClause' => $queryHasGroupByClause,
'hasHavingClause' => $queryHasHavingClause,
'rootEntityHasSingleIdentifierFieldName' => $rootEntityHasSingleIdentifierFieldName,
'couldProduceDuplicates' => $queryCouldProduceDuplicates,
'couldHaveToManyJoins' => $queryCouldHaveToManyJoins,
] = self::autoDetectQueryFeatures($query->getEntityManager(), $queryAST);

$paginator = new self($query, $queryCouldProduceDuplicates);

$paginator->queryStyleAutoDetectionEnabled = true;
$paginator->queryCouldHaveToManyJoins = $queryCouldHaveToManyJoins;
// The following is ensuring the conditions for when the CountWalker cannot be used.
$paginator->useCountQueryOutputWalker = $queryHasHavingClause !== false
|| $rootEntityHasSingleIdentifierFieldName !== true
|| ($queryCouldHaveToManyJoins && $queryHasGroupByClause !== false);

return $paginator;
}

/**
* @return array{
* hasGroupByClause: bool|null,
* hasHavingClause: bool|null,
* rootEntityHasSingleIdentifierFieldName: bool|null,
* couldProduceDuplicates: bool,
* couldHaveToManyJoins: bool,
* }
*/
private static function autoDetectQueryFeatures(EntityManagerInterface $entityManager, Node $queryAST): array
{
$queryFeatures = [
// Null means undetermined
'hasGroupByClause' => null,
'hasHavingClause' => null,
'rootEntityHasSingleIdentifierFieldName' => null,
'couldProduceDuplicates' => true,
'couldHaveToManyJoins' => true,
];

if (! $queryAST instanceof Query\AST\SelectStatement) {
return $queryFeatures;

Check warning on line 131 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L131

Added line #L131 was not covered by tests
}

$queryFeatures['hasGroupByClause'] = $queryAST->groupByClause !== null;
$queryFeatures['hasHavingClause'] = $queryAST->havingClause !== null;

$from = $queryAST->fromClause->identificationVariableDeclarations;
if (count($from) > 1) {
return $queryFeatures;

Check warning on line 139 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L139

Added line #L139 was not covered by tests
}

$fromRoot = reset($from);
if (! $fromRoot instanceof Query\AST\IdentificationVariableDeclaration) {
return $queryFeatures;

Check warning on line 144 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L144

Added line #L144 was not covered by tests
}

if (! $fromRoot->rangeVariableDeclaration) {
return $queryFeatures;

Check warning on line 148 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L148

Added line #L148 was not covered by tests
}

$rootAlias = $fromRoot->rangeVariableDeclaration->aliasIdentificationVariable;
try {
$rootClassMetadata = $entityManager->getClassMetadata($fromRoot->rangeVariableDeclaration->abstractSchemaName);
$queryFeatures['rootEntityHasSingleIdentifierFieldName'] = (bool) $rootClassMetadata->getSingleIdentifierFieldName();
} catch (MappingException) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean the catching of the MappingException?

It's primarily needed because this is how ClassMetadata::getSingleIdentifierFieldName() communicates that "the identifier is not single column". One way to avoid the try..catch is to duplicate the if-conditions from ClassMetadata::getSingleIdentifierFieldName() in here, but having a code duplication here would introduce a maintenance cost, which I personally consider a bigger nuisance than having a try..catch block.

Also, EntityManager::getClassMetadata() throws that exception class when the $className is unrecognised. Instead of making an assumption that "there must be metadata for every class detected at this point of the code", I wanted to be more graceful and defensive, and chose to just catch the exception and assume that the "auto-detection" cannot be carried out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually avoid using exception for flow control, and I think that's because they are costly.

I think I would add a dedicated method, so as to remove the need of casting the result to a boolean as well, even if it means a little duplication.

Also, EntityManager::getClassMetadata() throws that exception class when the $className is unrecognised. Instead of making an assumption that "there must be metadata for every class detected at this point of the code", I wanted to be more graceful and defensive, and chose to just catch the exception and assume that the "auto-detection" cannot be carried out.

In what case will $className not be recognized and we wouldn't want to let the user know about that?

return $queryFeatures;

Check warning on line 156 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L155-L156

Added lines #L155 - L156 were not covered by tests
}

$rootClassName = $fromRoot->rangeVariableDeclaration->abstractSchemaName;

$aliasesToClassNameOrClassMetadataMap = [];
$aliasesToClassNameOrClassMetadataMap[$rootAlias] = $rootClassMetadata;
$toManyJoinsAliases = [];

// Check the Joins list.
foreach ($fromRoot->joins as $join) {
if (! $join instanceof Join || ! $join->joinAssociationDeclaration instanceof JoinAssociationDeclaration) {
return $queryFeatures;

Check warning on line 168 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L167-L168

Added lines #L167 - L168 were not covered by tests
}

$joinParentAlias = $join->joinAssociationDeclaration->joinAssociationPathExpression->identificationVariable;
$joinParentFieldName = $join->joinAssociationDeclaration->joinAssociationPathExpression->associationField;
$joinAlias = $join->joinAssociationDeclaration->aliasIdentificationVariable;

Check warning on line 173 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L171-L173

Added lines #L171 - L173 were not covered by tests

// Every Join descending from a ToMany Join is "in principle" also a ToMany Join
if (in_array($joinParentAlias, $toManyJoinsAliases, true)) {
$toManyJoinsAliases[] = $joinAlias;

Check warning on line 177 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L176-L177

Added lines #L176 - L177 were not covered by tests

continue;

Check warning on line 179 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L179

Added line #L179 was not covered by tests
}

$parentClassMetadata = $aliasesToClassNameOrClassMetadataMap[$joinParentAlias] ?? null;
if (! $parentClassMetadata) {
return $queryFeatures;

Check warning on line 184 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L182-L184

Added lines #L182 - L184 were not covered by tests
}

// Load entity class metadata.
if (is_string($parentClassMetadata)) {

Check warning on line 188 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L188

Added line #L188 was not covered by tests
try {
$parentClassMetadata = $entityManager->getClassMetadata($parentClassMetadata);
$aliasesToClassNameOrClassMetadataMap[$joinParentAlias] = $parentClassMetadata;
} catch (MappingException) {
return $queryFeatures;

Check warning on line 193 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L190-L193

Added lines #L190 - L193 were not covered by tests
}
}

$parentJoinAssociationMapping = $parentClassMetadata->associationMappings[$joinParentFieldName] ?? null;
if (! $parentJoinAssociationMapping) {
return $queryFeatures;

Check warning on line 199 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L197-L199

Added lines #L197 - L199 were not covered by tests
}

$aliasesToClassNameOrClassMetadataMap[$joinAlias] = $parentJoinAssociationMapping['targetEntity'];

Check warning on line 202 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L202

Added line #L202 was not covered by tests

if (! ($parentJoinAssociationMapping['type'] & ClassMetadata::TO_MANY)) {
continue;

Check warning on line 205 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L204-L205

Added lines #L204 - L205 were not covered by tests
}

// The Join is a ToMany Join.
$toManyJoinsAliases[] = $joinAlias;

Check warning on line 209 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L209

Added line #L209 was not covered by tests
}

$queryFeatures['couldHaveToManyJoins'] = count($toManyJoinsAliases) > 0;

// Check the Select list.
foreach ($queryAST->selectClause->selectExpressions as $selectExpression) {
if (! $selectExpression instanceof SelectExpression) {
return $queryFeatures;

Check warning on line 217 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L217

Added line #L217 was not covered by tests
}

// Must not use any of the ToMany aliases
if (is_string($selectExpression->expression)) {
foreach ($toManyJoinsAliases as $toManyJoinAlias) {
if (
$selectExpression->expression === $toManyJoinAlias
|| str_starts_with($selectExpression->expression, $toManyJoinAlias . '.')

Check warning on line 225 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L224-L225

Added lines #L224 - L225 were not covered by tests
) {
return $queryFeatures;

Check warning on line 227 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L227

Added line #L227 was not covered by tests
}
}
}

// If it's a function, then it has to be one from the following list. Reason: in some databases,
// there are functions that "generate rows".
if (
$selectExpression->expression instanceof Query\AST\Functions\FunctionNode
&& ! in_array($selectExpression->expression::class, [
Query\AST\Functions\CountFunction::class,
Query\AST\Functions\AvgFunction::class,
Query\AST\Functions\SumFunction::class,
Query\AST\Functions\MinFunction::class,
Query\AST\Functions\MaxFunction::class,
], true)
) {
return $queryFeatures;

Check warning on line 244 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L244

Added line #L244 was not covered by tests
}
}

// If there are ToMany Joins, then the Select clause has to use the DISTINCT keyword. Note: the foreach
// above also ensures that the ToMany Joins are not in the Select list, which is relevant.
if (
count($toManyJoinsAliases) > 0
&& ! $queryAST->selectClause->isDistinct
) {
return $queryFeatures;

Check warning on line 254 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L254

Added line #L254 was not covered by tests
}

$queryFeatures['couldProduceDuplicates'] = false;

return $queryFeatures;
}

/**
* Returns the query.
*/
Expand All @@ -60,31 +268,80 @@
}

/**
* Returns whether the query joins a collection.
* @deprecated Use ::getQueryCouldProduceDuplicates() instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UPGRADE.md needs to document the deprecations.

*
* @return bool Whether the query joins a collection.
* Returns whether the query joins a collection.
*/
public function getFetchJoinCollection(): bool
{
return $this->fetchJoinCollection;
return $this->queryCouldProduceDuplicates;
}

/**
* Returns whether the query could produce partially duplicated records.
*/
public function getQueryCouldProduceDuplicates(): bool

Check warning on line 283 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L283

Added line #L283 was not covered by tests
{
return $this->queryCouldProduceDuplicates;

Check warning on line 285 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L285

Added line #L285 was not covered by tests
}

/**
* @deprecated Use the individual ::get*OutputWalker()
*
* Returns whether the paginator will use an output walker.
*/
public function getUseOutputWalkers(): bool|null
{
return $this->useOutputWalkers;
return $this->getUseResultQueryOutputWalker() && $this->getUseCountQueryOutputWalker();

Check warning on line 295 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L295

Added line #L295 was not covered by tests
}

/**
* @deprecated Use the individual ::set*OutputWalker()
*
* Sets whether the paginator will use an output walker.
*
* @return $this
*/
public function setUseOutputWalkers(bool|null $useOutputWalkers): static
{
$this->useOutputWalkers = $useOutputWalkers;
$this->setUseResultQueryOutputWalker($useOutputWalkers);
$this->setUseCountQueryOutputWalker($useOutputWalkers);

return $this;
}

/**
* Returns whether the paginator will use an output walker for the result query.
*/
public function getUseResultQueryOutputWalker(): bool|null

Check warning on line 316 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L316

Added line #L316 was not covered by tests
{
return $this->useResultQueryOutputWalker;

Check warning on line 318 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L318

Added line #L318 was not covered by tests
}

/**
* Sets whether the paginator will use an output walker for the result query.
*/
public function setUseResultQueryOutputWalker(bool|null $useResultQueryOutputWalker): static
{
$this->useResultQueryOutputWalker = $useResultQueryOutputWalker;

return $this;
}

/**
* Returns whether the paginator will use an output walker for the count query.
*/
public function getUseCountQueryOutputWalker(): bool|null

Check warning on line 334 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L334

Added line #L334 was not covered by tests
{
return $this->useCountQueryOutputWalker;

Check warning on line 336 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L336

Added line #L336 was not covered by tests
}

/**
* Sets whether the paginator will use an output walker for the count query.
*/
public function setUseCountQueryOutputWalker(bool|null $useCountQueryOutputWalker): static
{
$this->useCountQueryOutputWalker = $useCountQueryOutputWalker;

return $this;
}
Expand Down Expand Up @@ -112,7 +369,7 @@
$offset = $this->query->getFirstResult();
$length = $this->query->getMaxResults();

if ($this->fetchJoinCollection && $length !== null) {
if ($this->queryCouldProduceDuplicates && $length !== null) {
$subQuery = $this->cloneQuery($this->query);

if ($this->useOutputWalker($subQuery)) {
Expand Down Expand Up @@ -171,13 +428,18 @@
/**
* Determines whether to use an output walker for the query.
*/
private function useOutputWalker(Query $query): bool
private function useOutputWalker(Query $query, bool $forCountQuery = false): bool
{
if ($this->useOutputWalkers === null) {
return (bool) $query->getHint(Query::HINT_CUSTOM_OUTPUT_WALKER) === false;
if (! $forCountQuery && $this->useResultQueryOutputWalker !== null) {
return $this->useResultQueryOutputWalker;
}

if ($forCountQuery && $this->useCountQueryOutputWalker !== null) {
return $this->useCountQueryOutputWalker;
}

return $this->useOutputWalkers;
// When a custom output walker already present, then do not use the Paginator's.
return $query->getHint(Query::HINT_CUSTOM_OUTPUT_WALKER) === false;
}

/**
Expand Down Expand Up @@ -205,10 +467,20 @@
$countQuery = $this->cloneQuery($this->query);

if (! $countQuery->hasHint(CountWalker::HINT_DISTINCT)) {
$countQuery->setHint(CountWalker::HINT_DISTINCT, true);
$hintDistinctDefaultTrue = true;

// When not joining onto *ToMany relations, then use a simpler COUNT query in the CountWalker.
d-ph marked this conversation as resolved.
Show resolved Hide resolved
if (
$this->queryStyleAutoDetectionEnabled
&& ! $this->queryCouldHaveToManyJoins
) {
$hintDistinctDefaultTrue = false;

Check warning on line 477 in src/Tools/Pagination/Paginator.php

View check run for this annotation

Codecov / codecov/patch

src/Tools/Pagination/Paginator.php#L477

Added line #L477 was not covered by tests
}

$countQuery->setHint(CountWalker::HINT_DISTINCT, $hintDistinctDefaultTrue);
}

if ($this->useOutputWalker($countQuery)) {
if ($this->useOutputWalker($countQuery, forCountQuery: true)) {
$platform = $countQuery->getEntityManager()->getConnection()->getDatabasePlatform(); // law of demeter win

$rsm = new ResultSetMapping();
Expand Down
Loading
Loading