Skip to content

pagination test #528

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

Open
wants to merge 8 commits into
base: 2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ORM.php
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ public function with(
schema: $schema ?? $this->schema,
commandGenerator: $this->commandGenerator,
heap: $heap,
options: $options,
options: $options ?? $this->options,
);
}

Expand Down
21 changes: 21 additions & 0 deletions src/Options.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,16 @@ final class Options
{
/**
* @readonly
* @note will be set to TRUE in the next major version.
*/
public bool $ignoreUninitializedRelations = true;

/**
* @readonly
* @note will be set to TRUE in the next major version.
*/
public bool $groupByToDeduplicate = false;

/**
* If TRUE, ORM will ignore relations on uninitialized Entity properties.
* In this case, `unset($entity->relation)` will not change the relation when saving,
Expand All @@ -29,4 +36,18 @@ public function withIgnoreUninitializedRelations(bool $value): static
$clone->ignoreUninitializedRelations = $value;
return $clone;
}

/**
* If TRUE, ORM will use GROUP BY to deduplicate entities in Select queries in cases where
* `limit` and `offset` with JOINs are used.
*
* If FALSE, ORM will not use GROUP BY, which may lead wrong results in cases where
* `limit` and `offset` are used with JOINs.
*/
public function withGroupByToDeduplicate(bool $value): static
{
$clone = clone $this;
$clone->groupByToDeduplicate = $value;
return $clone;
}
}
72 changes: 53 additions & 19 deletions src/Select.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ class Select implements \IteratorAggregate, \Countable, PaginableInterface
// load related data after the query
public const OUTER_QUERY = JoinableLoader::POSTLOAD;

protected int $limit = 0;
protected int $offset = 0;
protected bool $allowGroupBy;
private RootLoader $loader;
private QueryBuilder $builder;
private MapperProviderInterface $mapperProvider;
Expand All @@ -75,6 +78,7 @@ public function __construct(
$this->schema = $orm->getSchema();
$this->mapperProvider = $orm->getService(MapperProviderInterface::class);
$this->entityFactory = $orm->getService(EntityFactoryInterface::class);
$this->allowGroupBy = $orm->getService(Options::class)->groupByToDeduplicate;
$this->loader = new RootLoader(
$orm->getSchema(),
$orm->getService(SourceProviderInterface::class),
Expand Down Expand Up @@ -109,7 +113,7 @@ public function getBuilder(): QueryBuilder
*/
public function buildQuery(): SelectQuery
{
return $this->loader->buildQuery();
return $this->addGroupByPK()->loader->buildQuery();
}

/**
Expand Down Expand Up @@ -141,11 +145,10 @@ public function wherePK(string|int|array|object ...$ids): self
public function count(?string $column = null): int
{
if ($column === null) {
// @tuneyourserver solves the issue with counting on queries with joins.
$pk = $this->loader->getPK();
$column = \is_array($pk)
$pk = (array) $this->loader->getPK();
$column = \count($pk) > 1
? '*'
: \sprintf('DISTINCT(%s)', $pk);
: \sprintf('DISTINCT(%s)', \reset($pk));
}

return (int) $this->__call('count', [$column]);
Expand All @@ -156,6 +159,7 @@ public function count(?string $column = null): int
*/
public function limit(int $limit): self
{
$this->limit = $limit;
$this->loader->getQuery()->limit($limit);

return $this;
Expand All @@ -166,6 +170,7 @@ public function limit(int $limit): self
*/
public function offset(int $offset): self
{
$this->offset = $offset;
$this->loader->getQuery()->offset($offset);

return $this;
Expand Down Expand Up @@ -369,9 +374,7 @@ public function with(string|array $relation, array $options = []): self
public function fetchOne(?array $query = null): ?object
{
$select = (clone $this)->where($query)->limit(1);
$node = $select->loader->createNode();
$select->loader->loadData($node, true);
$data = $node->getResult();
$data = $select->loadData();

if (!isset($data[0])) {
return null;
Expand All @@ -396,15 +399,12 @@ public function fetchAll(): iterable
*/
public function getIterator(bool $findInHeap = false): Iterator
{
$node = $this->loader->createNode();
$this->loader->loadData($node, true);

return Iterator::createWithServices(
$this->heap,
$this->schema,
$this->entityFactory,
$this->loader->getTarget(),
$node->getResult(),
$this->loadData(),
$findInHeap,
typecast: true,
);
Expand All @@ -413,20 +413,17 @@ public function getIterator(bool $findInHeap = false): Iterator
/**
* Load data tree from database and linked loaders in a form of array.
*
* @return array<array-key, array<string, mixed>>
* @return array<array-key, array<non-empty-string, mixed>>
*/
public function fetchData(bool $typecast = true): iterable
{
$node = $this->loader->createNode();
$this->loader->loadData($node, false);

if (!$typecast) {
return $node->getResult();
return $this->loadData(false);
}

$mapper = $this->mapperProvider->getMapper($this->loader->getTarget());

return \array_map([$mapper, 'cast'], $node->getResult());
return \array_map([$mapper, 'cast'], $this->loadData(false));
}

/**
Expand All @@ -451,7 +448,7 @@ public function __call(string $name, array $arguments): mixed
if (\in_array(\strtoupper($name), ['AVG', 'MIN', 'MAX', 'SUM', 'COUNT'])) {
// aggregations
return $this->builder->withQuery(
$this->loader->buildQuery(),
$this->buildQuery(),
)->__call($name, $arguments);
}

Expand Down Expand Up @@ -482,6 +479,18 @@ public function __destruct()
unset($this->loader, $this->builder);
}

/**
* @param bool $addRole If true, the role name with the key `@role` will be added to the result set.
* @return array<array-key, array<non-empty-string, mixed>>
*/
protected function loadData(bool $addRole = true): array
{
$self = $this->addGroupByPK();
$node = $self->loader->createNode();
$self->loader->loadData($node, $addRole);
return $node->getResult();
}

/**
* @param list<non-empty-string> $pk
* @param list<array|int|object|string> $args
Expand Down Expand Up @@ -521,4 +530,29 @@ private function buildCompositePKQuery(array $pk, array $args): self

return $this;
}

/**
* Add group by for all primary keys if necessary.
*
* This is required to prevent duplicates in the result set when using LIMIT and OFFSET.
*
* @return static<TEntity> Original $this or cloned instance with group by added.
*/
private function addGroupByPK(): self
{
if (!$this->allowGroupBy || $this->limit <= 1 && $this->offset === 0) {
return $this;
}

// Check if there are no joins in the query
if ($this->loader->getJoinedLoaders() === []) {
// No joins, we can safely return the original instance
return $this;
}

$self = clone $this;
$self->loader->forceGroupBy();

return $self;
}
}
10 changes: 10 additions & 0 deletions src/Select/AbstractLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,16 @@ public function getParentLoader(): ?LoaderInterface
return $this->inherit;
}

/**
* Returns all loaders that are joined to the current loader.
*
* @return LoaderInterface[]
*/
public function getJoinedLoaders(): array
{
return $this->join;
}

/**
* Indicates that loader loads data.
*/
Expand Down
3 changes: 2 additions & 1 deletion src/Select/Loader/ManyToManyLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -223,9 +223,10 @@ protected function mountColumns(
bool $minify = false,
string $prefix = '',
bool $overwrite = false,
bool $addToGroup = false,
): SelectQuery {
// columns are reset on earlier stage to allow pivot loader mount it's own aliases
return parent::mountColumns($query, $minify, $prefix, false);
return parent::mountColumns($query, $minify, $prefix, false, $addToGroup);
}

protected function initNode(): AbstractNode
Expand Down
15 changes: 14 additions & 1 deletion src/Select/RootLoader.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ final class RootLoader extends AbstractLoader
'scope' => true,
];
private SelectQuery $query;
private bool $forceGroupBy = false;

/**
* @param bool $loadRelations Define loading eager relations and JTI hierarchy.
Expand Down Expand Up @@ -132,6 +133,18 @@ public function isLoaded(): bool
return true;
}

/**
* Add selected columns to GROUP BY clause.
*
* Might be useful when deduplication is required because of JOINs or other conditions.
*
* @param bool $force When set to true, GROUP BY will be forced.
*/
public function forceGroupBy(bool $force = true): void
{
$this->forceGroupBy = $force;
}

/**
* Clone the underlying query.
*/
Expand All @@ -144,7 +157,7 @@ public function __clone()
protected function configureQuery(SelectQuery $query): SelectQuery
{
return parent::configureQuery(
$this->mountColumns($query, true, '', true),
$this->mountColumns($query, true, '', true, $this->forceGroupBy),
);
}

Expand Down
10 changes: 6 additions & 4 deletions src/Select/Traits/ColumnsTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,17 @@ public function fieldAlias(string $field): ?string
/**
* Set columns into SelectQuery.
*
* @param bool $minify Minify column names (will work in case when query parsed in
* FETCH_NUM mode).
* @param string $prefix Prefix to be added for each column name.
* @param bool $overwrite When set to true existed columns will be removed.
* @param bool $minify Minify column names (will work in case when query parsed in FETCH_NUM mode).
* @param string $prefix Prefix to be added for each column name.
* @param bool $overwrite When set to true existed columns will be removed.
* @param bool $addToGroup When set to true columns will be added to GROUP BY clause.
*/
protected function mountColumns(
SelectQuery $query,
bool $minify = false,
string $prefix = '',
bool $overwrite = false,
bool $addToGroup = false,
): SelectQuery {
$alias = $this->getAlias();
$columns = $overwrite ? [] : $query->getColumns();
Expand All @@ -59,6 +60,7 @@ protected function mountColumns(
}

$columns[] = "{$alias}.{$external} AS {$prefix}{$name}";
$addToGroup and $query->groupBy("{$alias}.{$external}");
}

return $query->columns($columns);
Expand Down
4 changes: 1 addition & 3 deletions src/Transaction/TupleStorage.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,10 @@

namespace Cycle\ORM\Transaction;

use IteratorAggregate;

/**
* @internal
*
* @implements IteratorAggregate<object, Tuple>
* @implements \IteratorAggregate<object, Tuple>
*/
final class TupleStorage implements \IteratorAggregate, \Countable
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
use Cycle\ORM\Tests\Functional\Driver\Common\Integration\IntegrationTestTrait;
use Cycle\ORM\Tests\Traits\TableTrait;

abstract class CaseTest extends BaseTest
abstract class TestCase extends BaseTest
{
use IntegrationTestTrait;
use TableTrait;
Expand Down
Loading
Loading