diff --git a/composer.json b/composer.json index 781cf262a..a8fcdda55 100644 --- a/composer.json +++ b/composer.json @@ -24,7 +24,7 @@ "ext-ctype": "*", "nette/caching": "~3.2 || ~3.1.3", "nette/utils": "~3.0 || ~4.0", - "nextras/dbal": "^5.0.3", + "nextras/dbal": "dev-js/duplicated-joins", "phpstan/phpdoc-parser": "^1.33.0 || ^2.0.0" }, "require-dev": { diff --git a/src/Collection/DbalCollection.php b/src/Collection/DbalCollection.php index 1244a27d7..a0b00d700 100644 --- a/src/Collection/DbalCollection.php +++ b/src/Collection/DbalCollection.php @@ -38,14 +38,8 @@ class DbalCollection implements ICollection /** @var Iterator|null */ protected Iterator|null $fetchIterator = null; - /** @var array FindBy expressions for deferred processing */ - protected array $filtering = []; - - /** @var array OrderBy expression result & sorting direction */ - protected array $ordering = []; - - /** @var array{int, int|null}|null */ - protected array|null $limitBy = null; + /** @var array */ + protected array $groupByColumns = []; protected DbalQueryBuilderHelper|null $helper = null; @@ -54,7 +48,8 @@ class DbalCollection implements ICollection protected ?int $resultCount = null; protected bool $entityFetchEventTriggered = false; - protected QueryBuilder|null $queryBuilderCache = null; + /** @var array */ + private array $mysqlGroupByWorkaroundApplied = []; /** @@ -100,7 +95,24 @@ public function getByIdChecked($id): IEntity public function findBy(array $conds): ICollection { $collection = clone $this; - $collection->filtering[] = $conds; + $expression = $collection->getHelper()->processExpression( + builder: $collection->queryBuilder, + expression: $conds, + aggregator: null, + ); + $finalContext = $expression->havingExpression === null + ? ExpressionContext::FilterAnd + : ExpressionContext::FilterAndWithHavingClause; + $expression = $expression->collect($finalContext); + + $collection->applyExpressionJoins($expression); + if ($expression->expression !== null && $expression->args !== []) { + $collection->queryBuilder->andWhere($expression->expression, ...$expression->args); + } + if ($expression->havingExpression !== null && $expression->havingArgs !== []) { + $collection->queryBuilder->andHaving($expression->havingExpression, ...$expression->havingArgs); + } + $collection->applyGroupByColumns($expression->groupBy); return $collection; } @@ -108,22 +120,15 @@ public function findBy(array $conds): ICollection public function orderBy($expression, string $direction = ICollection::ASC): ICollection { $collection = clone $this; - $helper = $collection->getHelper(); if (is_array($expression) && !isset($expression[0])) { /** @var array $expression */ $expression = $expression; // no-op for PHPStan foreach ($expression as $subExpression => $subDirection) { - $collection->ordering[] = [ - $helper->processExpression($collection->queryBuilder, $subExpression, null), - $subDirection, - ]; + $collection->addOrderByExpression($subExpression, $subDirection); } } else { - $collection->ordering[] = [ - $helper->processExpression($collection->queryBuilder, $expression, null), - $direction, - ]; + $collection->addOrderByExpression($expression, $direction); } return $collection; } @@ -132,9 +137,6 @@ public function orderBy($expression, string $direction = ICollection::ASC): ICol public function resetOrderBy(): ICollection { $collection = clone $this; - $collection->ordering = []; - // reset default ordering from mapper - $collection->queryBuilder = clone $collection->queryBuilder; $collection->queryBuilder->orderBy(null); return $collection; } @@ -143,7 +145,7 @@ public function resetOrderBy(): ICollection public function limitBy(int $limit, int|null $offset = null): ICollection { $collection = clone $this; - $collection->limitBy = [$limit, $offset]; + $collection->queryBuilder->limitBy($limit, $offset); return $collection; } @@ -281,7 +283,7 @@ public function subscribeOnEntityFetch(callable $callback): void public function __clone() { - $this->queryBuilderCache = null; + $this->queryBuilder = clone $this->queryBuilder; $this->result = null; $this->resultCount = null; $this->fetchIterator = null; @@ -289,77 +291,13 @@ public function __clone() } - /** - * @internal - */ public function getQueryBuilder(): QueryBuilder { - if ($this->queryBuilderCache !== null) { - return $this->queryBuilderCache; - } - - $joins = []; - $groupBy = []; - $queryBuilder = clone $this->queryBuilder; - $helper = $this->getHelper(); - - $filtering = $this->filtering; - if (count($filtering) > 0) { - array_unshift($filtering, ICollection::AND); - $expression = $helper->processExpression( - builder: $queryBuilder, - expression: $filtering, - aggregator: null, - ); - $finalContext = $expression->havingExpression === null - ? ExpressionContext::FilterAnd - : ExpressionContext::FilterAndWithHavingClause; - $expression = $expression->collect($finalContext); - $joins = $expression->joins; - $groupBy = $expression->groupBy; - if ($expression->expression !== null && $expression->args !== []) { - $queryBuilder->andWhere($expression->expression, ...$expression->args); - } - if ($expression->havingExpression !== null && $expression->havingArgs !== []) { - $queryBuilder->andHaving($expression->havingExpression, ...$expression->havingArgs); - } - if ($this->mapper->getDatabasePlatform()->getName() === MySqlPlatform::NAME) { - $this->applyGroupByWithSameNamedColumnsWorkaround($queryBuilder, $groupBy); - } - } - - foreach ($this->ordering as [$expression, $direction]) { - $joins = array_merge($joins, $expression->joins); - $groupBy = array_merge($groupBy, $expression->groupBy); - $orderingExpression = $helper->processOrderDirection($expression, $direction); - $queryBuilder->addOrderBy('%ex', $orderingExpression); - } - - if ($this->limitBy !== null) { - $queryBuilder->limitBy($this->limitBy[0], $this->limitBy[1]); - } - - $mergedJoins = $helper->mergeJoins('%and', $joins); - foreach ($mergedJoins as $join) { - $join->applyJoin($queryBuilder); - } - - if (count($groupBy) > 0) { - foreach ($this->ordering as [$expression]) { - $groupBy = array_merge($groupBy, $expression->columns); - } - } - - if (count($groupBy) > 0) { - $unique = []; - foreach ($groupBy as $groupByFqn) { - $unique[$groupByFqn->getUnescaped()] = $groupByFqn; - } - $queryBuilder->groupBy('%column[]', array_values($unique)); - } - - $this->queryBuilderCache = $queryBuilder; - return $queryBuilder; + $this->result = null; + $this->resultCount = null; + $this->fetchIterator = null; + $this->entityFetchEventTriggered = false; + return $this->queryBuilder; } @@ -419,6 +357,70 @@ protected function getHelper(): DbalQueryBuilderHelper } + /** + * @param list $groupBy + */ + private function applyGroupByColumns(array $groupBy): void + { + if (count($groupBy) === 0) { + return; + } + + $newGroupBy = []; + foreach ($groupBy as $groupByFqn) { + $key = $groupByFqn->getUnescaped(); + if (isset($this->groupByColumns[$key])) { + continue; + } + + $this->groupByColumns[$key] = $groupByFqn; + $newGroupBy[] = $groupByFqn; + } + + if (count($newGroupBy) === 0) { + return; + } + + if ($this->queryBuilder->getClause('group')[0] === null) { + $this->queryBuilder->groupBy('%column[]', $newGroupBy); + } else { + foreach ($newGroupBy as $groupByFqn) { + $this->queryBuilder->addGroupBy('%column', $groupByFqn); + } + } + + if ($this->mapper->getDatabasePlatform()->getName() === MySqlPlatform::NAME) { + $this->applyGroupByWithSameNamedColumnsWorkaround($this->queryBuilder, array_values($this->groupByColumns)); + } + } + + /** + * @param array|string $expression + */ + private function addOrderByExpression(string|array $expression, string $direction): void + { + $expressionResult = $this->getHelper()->processExpression($this->queryBuilder, $expression, null); + $this->applyExpressionJoins($expressionResult); + $this->applyGroupByColumns($expressionResult->groupBy); + + $orderByExpression = $this->getHelper()->processOrderDirection($expressionResult, $direction); + $this->queryBuilder->addOrderBy('%ex', $orderByExpression); + + if ($this->queryBuilder->getClause('group')[0] !== null) { + $this->applyGroupByColumns($expressionResult->columns); + } + } + + + private function applyExpressionJoins(DbalExpressionResult $expression): void + { + $mergedJoins = $this->getHelper()->mergeJoins('%and', $expression->joins); + foreach ($mergedJoins as $join) { + $join->applyJoin($this->queryBuilder); + } + } + + /** * Apply workaround for MySQL that is not able to properly resolve columns when there are more same-named * columns in the GROUP BY clause, even though they are properly referenced to their tables. Orm workarounds @@ -430,17 +432,18 @@ private function applyGroupByWithSameNamedColumnsWorkaround(QueryBuilder $queryB { $map = []; foreach ($groupBy as $fqn) { - if (!isset($map[$fqn->name])) { - $map[$fqn->name] = [$fqn]; - } else { - $map[$fqn->name][] = $fqn; - } + $map[$fqn->name][$fqn->getUnescaped()] = $fqn; } - $i = 0; + foreach ($map as $fqns) { if (count($fqns) > 1) { - foreach ($fqns as $fqn) { - $queryBuilder->addSelect("%column AS __nextras_fix_" . $i++, $fqn); // @phpstan-ignore-line + foreach ($fqns as $key => $fqn) { + if (isset($this->mysqlGroupByWorkaroundApplied[$key])) { + continue; + } + + $queryBuilder->addSelect("%column AS __nextras_fix_" . count($this->mysqlGroupByWorkaroundApplied), $fqn); // @phpstan-ignore-line + $this->mysqlGroupByWorkaroundApplied[$key] = true; } } } diff --git a/src/Collection/Functions/Result/DbalTableJoin.php b/src/Collection/Functions/Result/DbalTableJoin.php index 416cb6bba..e401c4b08 100644 --- a/src/Collection/Functions/Result/DbalTableJoin.php +++ b/src/Collection/Functions/Result/DbalTableJoin.php @@ -5,6 +5,8 @@ use Nextras\Dbal\Platforms\Data\Fqn; use Nextras\Dbal\QueryBuilder\QueryBuilder; +use function md5; +use function serialize; /** @@ -41,11 +43,15 @@ public function __construct( public function applyJoin(QueryBuilder $queryBuilder): void { - $queryBuilder->joinLeft( + $queryBuilder->joinOnce( + 'LEFT', "$this->toExpression AS [$this->toAlias]", $this->onExpression, - ...$this->toArgs, - ...$this->onArgs, + [ + ...$this->toArgs, + ...$this->onArgs, + ], + hashSuffix: md5(serialize([$this->toArgs, $this->onArgs])), ); } } diff --git a/src/Mapper/Dbal/RelationshipMapperManyHasMany.php b/src/Mapper/Dbal/RelationshipMapperManyHasMany.php index d66e8aa3f..cbd892188 100644 --- a/src/Mapper/Dbal/RelationshipMapperManyHasMany.php +++ b/src/Mapper/Dbal/RelationshipMapperManyHasMany.php @@ -123,7 +123,7 @@ private function fetchByTwoPassStrategy(QueryBuilder $builder, array $values): M $hasOrderBy = $builder->getClause('order')[0] !== null; $builder = clone $builder; - $builder->joinLeft( + $builder->addLeftJoin( "%table AS %table", '%column = %column', // args @@ -232,7 +232,7 @@ private function fetchCounts(QueryBuilder $builder, array $values): array $targetTable = DbalQueryBuilderHelper::getAlias($this->joinTable); $builder = clone $builder; - $builder->joinLeft( + $builder->addLeftJoin( '%table AS %table', '%column = %column', // args diff --git a/tests/cases/integration/Collection/collection.phpt b/tests/cases/integration/Collection/collection.phpt index cdb3e6638..eb3ed1f12 100644 --- a/tests/cases/integration/Collection/collection.phpt +++ b/tests/cases/integration/Collection/collection.phpt @@ -394,6 +394,41 @@ class CollectionTest extends DataTestCase } + public function testGetQueryBuilderReturnsMutableBaseBuilder(): void + { + if ($this->section === Helper::SECTION_ARRAY) { + Environment::skip('DbalCollection only.'); + } + + $books = $this->orm->books->findAll(); + Assert::type(DbalCollection::class, $books); + + $builder = $books->getQueryBuilder(); + $builder->andWhere('%table.%column IN %any', $builder->getFromAlias(), 'id', [1, 2, 3]); + + Assert::same([1, 2, 3], $books->orderBy('id')->fetchPairs(null, 'id')); + Assert::same([2, 3], $books->findBy(['id' => [2, 3, 4]])->orderBy('id')->fetchPairs(null, 'id')); + } + + + public function testGetQueryBuilderDoesNotLeakToClones(): void + { + if ($this->section === Helper::SECTION_ARRAY) { + Environment::skip('DbalCollection only.'); + } + + $books = $this->orm->books->findAll(); + Assert::type(DbalCollection::class, $books); + + $filtered = $books->findBy(['id' => [1, 2, 3, 4]]); + $builder = $filtered->getQueryBuilder(); + $builder->andWhere('%table.%column = %any', $builder->getFromAlias(), 'id', 1); + + Assert::same([1], $filtered->orderBy('id')->fetchPairs(null, 'id')); + Assert::same([1, 2, 3, 4], $books->findBy(['id' => [1, 2, 3, 4]])->orderBy('id')->fetchPairs(null, 'id')); + } + + public function testPrefixCollectionFunction(): void { $author = $this->orm->books->findBy([TestingPrefixFunction::class, 'author->name', 'Wri']); diff --git a/tests/cases/unit/Collection/DbalTableJoinTest.phpt b/tests/cases/unit/Collection/DbalTableJoinTest.phpt new file mode 100644 index 000000000..2aa6c3e2a --- /dev/null +++ b/tests/cases/unit/Collection/DbalTableJoinTest.phpt @@ -0,0 +1,69 @@ +from('book', 'b')->select('*'); + + $joinA = new DbalTableJoin( + toExpression: '%table', + toArgs: ['book_tag'], + toAlias: 'tag', + onExpression: '%table.%column = %table.%column', + onArgs: ['b', 'tag_id', 'tag', 'id'], + ); + $joinB = new DbalTableJoin( + toExpression: '%table', + toArgs: ['author_tag'], + toAlias: 'tag', + onExpression: '%table.%column = %table.%column', + onArgs: ['b', 'tag_id', 'tag', 'id'], + ); + + $joinA->applyJoin($builder); + $joinB->applyJoin($builder); + + Assert::same( + [ + 'SELECT * FROM book AS [b] ' + . 'LEFT JOIN %table AS [tag] ON (%table.%column = %table.%column) ' + . 'LEFT JOIN %table AS [tag] ON (%table.%column = %table.%column)', + 'book_tag', + 'b', + 'tag_id', + 'tag', + 'id', + 'author_tag', + 'b', + 'tag_id', + 'tag', + 'id', + ], + [$builder->getQuerySql(), ...$builder->getQueryParameters()], + ); + } +} + + +$test = new DbalTableJoinTest(); +$test->run(); diff --git a/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationJoinTest_testAny.sql b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationJoinTest_testAny.sql index 134a2a0fc..13671cfd2 100644 --- a/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationJoinTest_testAny.sql +++ b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationJoinTest_testAny.sql @@ -39,6 +39,18 @@ WHERE GROUP BY "authors"."id"; +SELECT + "authors".* +FROM + "public"."authors" AS "authors" + LEFT JOIN "books" AS "books_any" ON ( + "authors"."id" = "books_any"."author_id" + ) +WHERE + "books_any"."title" = 'Book 1' +GROUP BY + "authors"."id"; + SELECT COUNT(*) AS count FROM @@ -55,3 +67,15 @@ FROM GROUP BY "authors"."id" ) temp; + +SELECT + "authors".* +FROM + "public"."authors" AS "authors" + LEFT JOIN "books" AS "books_any" ON ( + "authors"."id" = "books_any"."author_id" + ) +WHERE + "books_any"."title" = 'Book 1' +GROUP BY + "authors"."id"; diff --git a/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationJoinTest_testNone.sql b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationJoinTest_testNone.sql index 945885385..95b811000 100644 --- a/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationJoinTest_testNone.sql +++ b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionAggregationJoinTest_testNone.sql @@ -32,3 +32,18 @@ FROM HAVING COUNT("books_none"."id") = 0 ) temp; + +SELECT + "authors".* +FROM + "public"."authors" AS "authors" + LEFT JOIN "books" AS "books_none" ON ( + ( + "authors"."id" = "books_none"."author_id" + ) + AND "books_none"."title" = 'Book 1' + ) +GROUP BY + "authors"."id" +HAVING + COUNT("books_none"."id") = 0; diff --git a/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionTest_testGetQueryBuilderDoesNotLeakToClones.sql b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionTest_testGetQueryBuilderDoesNotLeakToClones.sql new file mode 100644 index 000000000..c45595e3b --- /dev/null +++ b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionTest_testGetQueryBuilderDoesNotLeakToClones.sql @@ -0,0 +1,2 @@ +SELECT "books".* FROM "books" AS "books" WHERE ("books"."id" IN (1, 2, 3, 4)) AND ("books"."id" = 1) ORDER BY "books"."id" ASC; +SELECT "books".* FROM "books" AS "books" WHERE "books"."id" IN (1, 2, 3, 4) ORDER BY "books"."id" ASC; diff --git a/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionTest_testGetQueryBuilderReturnsMutableBaseBuilder.sql b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionTest_testGetQueryBuilderReturnsMutableBaseBuilder.sql new file mode 100644 index 000000000..52f469715 --- /dev/null +++ b/tests/sqls/NextrasTests/Orm/Integration/Collection/CollectionTest_testGetQueryBuilderReturnsMutableBaseBuilder.sql @@ -0,0 +1,2 @@ +SELECT "books".* FROM "books" AS "books" WHERE "books"."id" IN (1, 2, 3) ORDER BY "books"."id" ASC; +SELECT "books".* FROM "books" AS "books" WHERE ("books"."id" IN (1, 2, 3)) AND ("books"."id" IN (2, 3, 4)) ORDER BY "books"."id" ASC; diff --git a/tests/sqls/NextrasTests/Orm/Integration/Repository/RepositorySTITest_testFindByFiltering.sql b/tests/sqls/NextrasTests/Orm/Integration/Repository/RepositorySTITest_testFindByFiltering.sql index 11b3c7c43..1206e7e62 100644 --- a/tests/sqls/NextrasTests/Orm/Integration/Repository/RepositorySTITest_testFindByFiltering.sql +++ b/tests/sqls/NextrasTests/Orm/Integration/Repository/RepositorySTITest_testFindByFiltering.sql @@ -1,2 +1,3 @@ SELECT "contents".* FROM "contents" AS "contents" WHERE ("contents"."type" = 'comment') AND ("contents"."replied_at" > '2020-01-01 17:00:00.000000'::timestamptz); SELECT COUNT(*) AS count FROM (SELECT "contents"."id" FROM "contents" AS "contents" WHERE ("contents"."type" = 'comment') AND ("contents"."replied_at" > '2020-01-01 17:00:00.000000'::timestamptz)) temp; +SELECT "contents".* FROM "contents" AS "contents" WHERE ("contents"."type" = 'comment') AND ("contents"."replied_at" > '2020-01-01 17:00:00.000000'::timestamptz);