diff --git a/src/ORM.php b/src/ORM.php index 87211ba1..e3b0604a 100644 --- a/src/ORM.php +++ b/src/ORM.php @@ -176,7 +176,7 @@ public function with( schema: $schema ?? $this->schema, commandGenerator: $this->commandGenerator, heap: $heap, - options: $options, + options: $options ?? $this->options, ); } diff --git a/src/Options.php b/src/Options.php index f938ea50..ba86b6c2 100644 --- a/src/Options.php +++ b/src/Options.php @@ -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, @@ -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; + } } diff --git a/src/Select.php b/src/Select.php index 1e299224..6172bd1c 100644 --- a/src/Select.php +++ b/src/Select.php @@ -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; @@ -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), @@ -109,7 +113,7 @@ public function getBuilder(): QueryBuilder */ public function buildQuery(): SelectQuery { - return $this->loader->buildQuery(); + return $this->addGroupByPK()->loader->buildQuery(); } /** @@ -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]); @@ -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; @@ -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; @@ -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; @@ -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, ); @@ -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> + * @return array> */ 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)); } /** @@ -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); } @@ -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> + */ + protected function loadData(bool $addRole = true): array + { + $self = $this->addGroupByPK(); + $node = $self->loader->createNode(); + $self->loader->loadData($node, $addRole); + return $node->getResult(); + } + /** * @param list $pk * @param list $args @@ -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 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; + } } diff --git a/src/Select/AbstractLoader.php b/src/Select/AbstractLoader.php index 7b6f5616..b07eb0f5 100644 --- a/src/Select/AbstractLoader.php +++ b/src/Select/AbstractLoader.php @@ -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. */ diff --git a/src/Select/Loader/ManyToManyLoader.php b/src/Select/Loader/ManyToManyLoader.php index e0655d94..f0502349 100644 --- a/src/Select/Loader/ManyToManyLoader.php +++ b/src/Select/Loader/ManyToManyLoader.php @@ -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 diff --git a/src/Select/RootLoader.php b/src/Select/RootLoader.php index 687e2c33..4af7c0a2 100644 --- a/src/Select/RootLoader.php +++ b/src/Select/RootLoader.php @@ -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. @@ -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. */ @@ -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), ); } diff --git a/src/Select/Traits/ColumnsTrait.php b/src/Select/Traits/ColumnsTrait.php index 9ca5ab54..21d3156d 100644 --- a/src/Select/Traits/ColumnsTrait.php +++ b/src/Select/Traits/ColumnsTrait.php @@ -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(); @@ -59,6 +60,7 @@ protected function mountColumns( } $columns[] = "{$alias}.{$external} AS {$prefix}{$name}"; + $addToGroup and $query->groupBy("{$alias}.{$external}"); } return $query->columns($columns); diff --git a/src/Transaction/TupleStorage.php b/src/Transaction/TupleStorage.php index 0d182949..015e63e6 100644 --- a/src/Transaction/TupleStorage.php +++ b/src/Transaction/TupleStorage.php @@ -4,12 +4,10 @@ namespace Cycle\ORM\Transaction; -use IteratorAggregate; - /** * @internal * - * @implements IteratorAggregate + * @implements \IteratorAggregate */ final class TupleStorage implements \IteratorAggregate, \Countable { diff --git a/tests/ORM/Functional/Driver/Common/Integration/CaseTemplate/CaseTest.php b/tests/ORM/Functional/Driver/Common/Integration/CaseTemplate/TestCase.php similarity index 99% rename from tests/ORM/Functional/Driver/Common/Integration/CaseTemplate/CaseTest.php rename to tests/ORM/Functional/Driver/Common/Integration/CaseTemplate/TestCase.php index 511f57f4..cd66b75c 100644 --- a/tests/ORM/Functional/Driver/Common/Integration/CaseTemplate/CaseTest.php +++ b/tests/ORM/Functional/Driver/Common/Integration/CaseTemplate/TestCase.php @@ -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; diff --git a/tests/ORM/Functional/Driver/Common/Integration/Issue528/AbstractTestCase.php b/tests/ORM/Functional/Driver/Common/Integration/Issue528/AbstractTestCase.php new file mode 100644 index 00000000..9dd8c9f7 --- /dev/null +++ b/tests/ORM/Functional/Driver/Common/Integration/Issue528/AbstractTestCase.php @@ -0,0 +1,145 @@ +orm, Country::class)) + ->load('translations') + ->where('translations.locale_id', 1); + /** @var Paginator $paginator */ + $paginator = (new Paginator(2))->paginate($select); + $data = $select->fetchData(); + $this->assertSame(10, $paginator->count()); + $this->assertCount(2, $data); + } + + public function testWithLeft(): void + { + $select = (new Select($this->orm, Country::class)) + ->with('translations', [ + 'as' => 'trans', + 'alias' => 'trans', + 'method' => JoinableLoader::LEFT_JOIN, + ]); + /** @var Paginator $paginator */ + $paginator = (new Paginator(2))->paginate($select); + $data = $select->fetchData(); + $this->assertSame(10, $paginator->count()); + $this->assertCount(2, $data); + } + + public function testWithInner(): void + { + $select = (new Select($this->orm, Country::class)) + ->with('translations', [ + 'as' => 'trans', + 'alias' => 'trans', + 'method' => JoinableLoader::JOIN, + ]); + /** @var Paginator $paginator */ + $paginator = (new Paginator(2))->paginate($select); + $data = $select->fetchData(); + $this->assertSame(10, $paginator->count()); + $this->assertCount(2, $data); + } + + public function setUp(): void + { + // Init DB + parent::setUp(); + $this->orm = $this->orm->with( + options: (new Options())->withGroupByToDeduplicate(true), + ); + $this->makeTables(); + $this->fillData(); + + $this->loadSchema(__DIR__ . '/schema.php'); + } + + private function makeTables(): void + { + // Make tables + $this->makeTable('country', [ + 'id' => 'primary', // autoincrement + 'name' => 'string', + ]); + + $this->makeTable('locale', [ + 'id' => 'primary', + 'code' => 'string', + ]); + + $this->makeTable('translation', [ + 'id' => 'primary', + 'title' => 'string', + 'country_id' => 'int', + 'locale_id' => 'int', + ]); + $this->makeFK('translation', 'country_id', 'country', 'id', 'NO ACTION', 'NO ACTION'); + $this->makeFK('translation', 'locale_id', 'locale', 'id', 'NO ACTION', 'NO ACTION'); + } + + private function fillData(): void + { + $this->getDatabase()->table('translation')->delete()->run(); + $this->getDatabase()->table('country')->delete()->run(); + $this->getDatabase()->table('locale')->delete()->run(); + + $locales = [ + ['en'], + ['ru'], + ]; + $this->getDatabase()->table('locale')->insertMultiple( + ['code'], + $locales, + ); + $countries = [ + ['Russia'], + ['USA'], + ['China'], + ['Kazakhstan'], + ['Japan'], + ['Germany'], + ['France'], + ['Italy'], + ['Spain'], + ['India'], + ]; + $this->getDatabase()->table('country')->insertMultiple( + ['name'], + $countries, + ); + + $values = []; + foreach ($countries as $i => $country) { + // Duplicate translations for each locale. + // There are 2 same translations for each country. + for ($k = 0; $k < 2; $k++) { + foreach ($locales as $j => $locale) { + $values[] = [$i + 1, $j + 1, "{$country[0]}-{$locale[0]}"]; + } + } + } + $this->getDatabase()->table('translation')->insertMultiple( + ['country_id', 'locale_id', 'title'], + $values, + ); + } +} diff --git a/tests/ORM/Functional/Driver/Common/Integration/Issue528/Entity/Country.php b/tests/ORM/Functional/Driver/Common/Integration/Issue528/Entity/Country.php new file mode 100644 index 00000000..345bb08e --- /dev/null +++ b/tests/ORM/Functional/Driver/Common/Integration/Issue528/Entity/Country.php @@ -0,0 +1,19 @@ + */ + public iterable $translations = []; + + public function __construct(string $name) + { + $this->name = $name; + } +} diff --git a/tests/ORM/Functional/Driver/Common/Integration/Issue528/Entity/Locale.php b/tests/ORM/Functional/Driver/Common/Integration/Issue528/Entity/Locale.php new file mode 100644 index 00000000..d4e3f6c1 --- /dev/null +++ b/tests/ORM/Functional/Driver/Common/Integration/Issue528/Entity/Locale.php @@ -0,0 +1,19 @@ + */ + public iterable $translations = []; + + public function __construct(string $code) + { + $this->code = $code; + } +} diff --git a/tests/ORM/Functional/Driver/Common/Integration/Issue528/Entity/Translation.php b/tests/ORM/Functional/Driver/Common/Integration/Issue528/Entity/Translation.php new file mode 100644 index 00000000..66b0bb2f --- /dev/null +++ b/tests/ORM/Functional/Driver/Common/Integration/Issue528/Entity/Translation.php @@ -0,0 +1,20 @@ +country = $country; + $this->locale = $locale; + $this->title = $title; + } +} diff --git a/tests/ORM/Functional/Driver/Common/Integration/Issue528/schema.php b/tests/ORM/Functional/Driver/Common/Integration/Issue528/schema.php new file mode 100644 index 00000000..d7c93e0e --- /dev/null +++ b/tests/ORM/Functional/Driver/Common/Integration/Issue528/schema.php @@ -0,0 +1,132 @@ + [ + Schema::ENTITY => Country::class, + Schema::SOURCE => Source::class, + Schema::DATABASE => 'default', + Schema::MAPPER => Mapper::class, + Schema::TABLE => 'country', + Schema::PRIMARY_KEY => ['id'], + Schema::FIND_BY_KEYS => ['id'], + Schema::COLUMNS => [ + 'id' => 'id', + 'name' => 'name', + ], + Schema::RELATIONS => [ + 'translations' => [ + Relation::TYPE => Relation::HAS_MANY, + Relation::TARGET => 'translation', + Relation::COLLECTION_TYPE => 'array', + Relation::LOAD => Relation::LOAD_PROMISE, + Relation::SCHEMA => [ + Relation::CASCADE => true, + Relation::NULLABLE => false, + Relation::WHERE => [], + Relation::ORDER_BY => [], + Relation::INNER_KEY => ['id'], + Relation::OUTER_KEY => 'country_id', + ], + ], + ], + Schema::TYPECAST => [ + 'id' => 'int', + 'name' => 'string', + 'code' => 'string', + 'isFriendly' => 'bool', + ], + Schema::SCHEMA => [], + ], + 'translation' => [ + Schema::ENTITY => Translation::class, + Schema::SOURCE => Source::class, + Schema::DATABASE => 'default', + Schema::TABLE => 'translation', + Schema::PRIMARY_KEY => ['id'], + Schema::FIND_BY_KEYS => ['id'], + Schema::COLUMNS => [ + 'id' => 'id', + 'country_id' => 'country_id', + 'locale_id' => 'locale_id', + 'title' => 'title', + ], + Schema::RELATIONS => [ + 'country' => [ + Relation::TYPE => Relation::BELONGS_TO, + Relation::TARGET => 'country', + Relation::LOAD => Relation::LOAD_EAGER, + Relation::SCHEMA => [ + Relation::CASCADE => true, + Relation::NULLABLE => false, + Relation::INNER_KEY => 'country_id', + Relation::OUTER_KEY => ['id'], + ], + ], + 'locale' => [ + Relation::TYPE => Relation::BELONGS_TO, + Relation::TARGET => 'locale', + Relation::LOAD => Relation::LOAD_PROMISE, + Relation::SCHEMA => [ + Relation::CASCADE => true, + Relation::NULLABLE => false, + Relation::INNER_KEY => 'locale_id', + Relation::OUTER_KEY => ['id'], + ], + ], + ], + Schema::TYPECAST => [ + 'id' => 'int', + 'country_id' => 'int', + 'locale_id' => 'int', + 'title' => 'string', + ], + Schema::SCHEMA => [], + ], + 'locale' => [ + Schema::ENTITY => Locale::class, + Schema::MAPPER => Mapper::class, + Schema::SOURCE => Source::class, + Schema::REPOSITORY => Repository::class, + Schema::DATABASE => 'default', + Schema::TABLE => 'locale', + Schema::PRIMARY_KEY => ['id'], + Schema::FIND_BY_KEYS => ['id'], + Schema::COLUMNS => [ + 'id' => 'id', + 'code' => 'code', + ], + Schema::RELATIONS => [ + 'translations' => [ + Relation::TYPE => Relation::HAS_MANY, + Relation::TARGET => 'translation', + Relation::COLLECTION_TYPE => 'array', + Relation::LOAD => Relation::LOAD_PROMISE, + Relation::SCHEMA => [ + Relation::CASCADE => true, + Relation::NULLABLE => false, + Relation::WHERE => [], + Relation::ORDER_BY => [], + Relation::INNER_KEY => ['id'], + Relation::OUTER_KEY => 'locale_id', + ], + ], + ], + Schema::SCOPE => null, + Schema::TYPECAST => [ + 'id' => 'int', + 'code' => 'string', + ], + Schema::SCHEMA => [], + ], +]; diff --git a/tests/ORM/Functional/Driver/MySQL/Integration/CaseTemplate/CaseTest.php b/tests/ORM/Functional/Driver/MySQL/Integration/CaseTemplate/TestCase.php similarity index 80% rename from tests/ORM/Functional/Driver/MySQL/Integration/CaseTemplate/CaseTest.php rename to tests/ORM/Functional/Driver/MySQL/Integration/CaseTemplate/TestCase.php index 543bcde8..024de30a 100644 --- a/tests/ORM/Functional/Driver/MySQL/Integration/CaseTemplate/CaseTest.php +++ b/tests/ORM/Functional/Driver/MySQL/Integration/CaseTemplate/TestCase.php @@ -5,13 +5,13 @@ namespace Cycle\ORM\Tests\Functional\Driver\MySQL\Integration\CaseTemplate; // phpcs:ignore -use Cycle\ORM\Tests\Functional\Driver\Common\Integration\CaseTemplate\CaseTest as CommonClass; +use Cycle\ORM\Tests\Functional\Driver\Common\Integration\CaseTemplate\TestCase as CommonClass; /** * @group driver * @group driver-mysql */ -class CaseTest extends CommonClass +class TestCase extends CommonClass { public const DRIVER = 'mysql'; } diff --git a/tests/ORM/Functional/Driver/MySQL/Integration/Issue482/AbstractTestCase.php b/tests/ORM/Functional/Driver/MySQL/Integration/Issue482/AbstractTestCase.php new file mode 100644 index 00000000..7ce41974 --- /dev/null +++ b/tests/ORM/Functional/Driver/MySQL/Integration/Issue482/AbstractTestCase.php @@ -0,0 +1,17 @@ +