Merge pull request #566 from nextras/aggregations_fix

Aggregations fix for missing grouping

src/Collection/Aggregations/CountAggregator.php

@@ -76,13 +76,12 @@ public function aggregateExpression(
 		);
 
 		$primaryKey = $join->conventions->getStoragePrimaryKey()[0];
-		$groupBy = $expression->groupBy;
 
 		return new DbalExpressionResult(
 			'COUNT(%table.%column) >= %i AND COUNT(%table.%column) <= %i',
 			[$join->toAlias, $primaryKey, $this->atLeast, $join->toAlias, $primaryKey, $this->atMost],
 			$joins,
-			$groupBy,
+			$expression->groupBy,
 			null,
 			true,
 			null,

src/Collection/DbalCollection.php

@@ -332,8 +332,8 @@ public function getQueryBuilder(): QueryBuilder
 				null
 			);
 			$joins = $expression->joins;
-			$groupBy = $expression->groupBy;
 			if ($expression->isHavingClause) {
+				$groupBy = $expression->groupBy;
 				$this->queryBuilder->andHaving($expression->expression, ...$expression->args);
 			} else {
 				$this->queryBuilder->andWhere($expression->expression, ...$expression->args);
@@ -343,7 +343,9 @@ public function getQueryBuilder(): QueryBuilder
 
 		foreach ($this->ordering as [$expression, $direction]) {
 			$joins = array_merge($joins, $expression->joins);
-			$groupBy = array_merge($groupBy, $expression->groupBy);
+			if ($expression->isHavingClause) {
+				$groupBy = array_merge($groupBy, $expression->groupBy);
+			}
 			$orderingExpression = $helper->processOrderDirection($expression, $direction);
 			$this->queryBuilder->addOrderBy('%ex', $orderingExpression);
 		}

src/Collection/Helpers/DbalExpressionResult.php

@@ -38,6 +38,7 @@ class DbalExpressionResult
 
 	/**
 	 * List of arguments possible to pass to %ex modifier.
+	 * Those grouping expressions are applied iff the $isHavingClause is true.
 	 * @var array<array<mixed>>
 	 */
 	public $groupBy;

src/Collection/Helpers/DbalQueryBuilderHelper.php

@@ -322,12 +322,6 @@ private function processTokens(
 			}
 		}
 
-		if ($makeDistinct) {
-			$groupBy = $this->makeDistinct($builder, $this->mapper);
-		} else {
-			$groupBy = [];
-		}
-
 		$propertyMetadata = $currentEntityMetadata->getProperty($lastToken);
 		if ($propertyMetadata->wrapper === EmbeddableContainer::class) {
 			$propertyExpression = implode('->', array_merge($tokens, [$lastToken]));
@@ -342,6 +336,12 @@ private function processTokens(
 			$propertyPrefixTokens
 		);
 
+		if ($makeDistinct) {
+			$groupBy = $this->makeDistinct($builder, $this->mapper);
+		} else {
+			$groupBy = [['%column', $column]];
+		}
+
 		return new DbalExpressionResult(
 			'%column',
 			[$column],

tests/cases/integration/Entity/entity.insert.phpt

@@ -87,7 +87,7 @@ class NewEntityTest extends DataTestCase
 
 		try {
 			$this->orm->authors->persistAndFlush($author2);
-			Assert::fail('Duplicit PK exception expected.');
+			Assert::fail('Duplicated PK exception expected.');
 		} catch (Exception $e) { // general because of different mapper impl.
 		}
 

tests/cases/integration/Relationships/relationships.manyHasOne.phpt

@@ -143,6 +143,16 @@ class RelationshipManyHasOneTest extends DataTestCase
 		}
 		Assert::equal(['Nextras publisher A', 'Nextras publisher C'], $publishers);
 	}
+
+
+	public function testProperAggregation(): void
+	{
+		$books = $this->orm->books->findBy([
+			'tags->id' => 1,
+			'publisher->name' => 'Nextras publisher A',
+		]);
+		Assert::same($books->count(), 1);
+	}
 }
 
 

tests/inc/QueryChecker.php

@@ -28,14 +28,14 @@ public function __construct(string $name)
 	public function assert(): void
 	{
 		$file = __DIR__ . '/../sqls/' . $this->name . '.sql';
-		if (!file_exists($file)) {
-			$ci = getenv('GITHUB_ACTIONS') !== false;
-			if ($ci) {
-				throw new \Exception("Missing $this->name.sql file, run `compose tests` locally (with Postgres) to generate the expected SQL queries files.");
-			}
+		$ci = getenv('GITHUB_ACTIONS') !== false;
+		if (!$ci) {
 			FileSystem::createDir(dirname($file));
 			FileSystem::write($file, $this->sqls);
 		} else {
+			if (!file_exists($file)) {
+				throw new \Exception("Missing $this->name.sql file, run `composer tests` locally (with Postgres) to generate the expected SQL queries files.");
+			}
 			Assert::same(FileSystem::read($file), $this->sqls);
 		}
 	}
@@ -60,5 +60,7 @@ public function onQuery(string $sqlQuery, float $timeTaken, ?Result $result): vo
 
 	public function onQueryException(string $sqlQuery, float $timeTaken, ?DriverException $exception): void
 	{
+		if (strpos($sqlQuery, 'pg_catalog.') !== false) return;
+		$this->sqls .= "$sqlQuery;\n";
 	}
 }

tests/sqls/NextrasTests/Orm/Integration/Entity/NewEntityTest_testDuplicatePrimaryKey.sql

@@ -2,6 +2,7 @@ START TRANSACTION;
 INSERT INTO "public"."authors" ("id", "name", "born", "web", "favorite_author_id") VALUES (444, 'Jon Snow', '2021-03-21 08:23:00.000000'::timestamp, 'http://nextras.cz', NULL);
 COMMIT;
 START TRANSACTION;
+INSERT INTO "public"."authors" ("id", "name", "born", "web", "favorite_author_id") VALUES (444, 'The Imp', '2021-03-21 08:23:00.000000'::timestamp, 'http://nextras.cz/imp', NULL);
 ROLLBACK;
 START TRANSACTION;
 INSERT INTO "public"."authors" ("id", "name", "born", "web", "favorite_author_id") VALUES (445, 'The Imp', '2021-03-21 08:23:00.000000'::timestamp, 'http://nextras.cz/imp', NULL);

tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipManyHasOneTest_testProperAggregation.sql

@@ -0,0 +1 @@
+SELECT "books".* FROM "books" AS "books" LEFT JOIN "books_x_tags" AS "books_x_tags" ON ("books"."id" = "books_x_tags"."book_id") LEFT JOIN "tags" AS "tags_any" ON (("books_x_tags"."tag_id" = "tags_any"."id") AND "tags_any"."id" = 1) LEFT JOIN "publishers" AS "publisher" ON ("books"."publisher_id" = "publisher"."publisher_id") GROUP BY "books"."id", "publisher"."name" HAVING ((COUNT("tags_any"."id") > 0) AND ("publisher"."name" = 'Nextras publisher A'));

tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipOneHasManyTest_testCountStoredOnOneHasManyRelationshipCondition.sql

@@ -1,3 +1,3 @@
 SELECT "publishers".* FROM "publishers" AS "publishers" WHERE (("publishers"."publisher_id" = 1));
 SELECT "publisher_id", COUNT(DISTINCT "count") as "count" FROM (SELECT "books".*, "books"."id" AS "count" FROM "books" AS "books" LEFT JOIN "books_x_tags" AS "books_x_tags" ON ("books"."id" = "books_x_tags"."book_id") LEFT JOIN "tags" AS "tags_any" ON (("books_x_tags"."tag_id" = "tags_any"."id") AND "tags_any"."id" = 1) WHERE "books"."publisher_id" IN (1) GROUP BY "books"."id" HAVING ((COUNT("tags_any"."id") > 0))) AS "temp" GROUP BY "publisher_id";
-SELECT "publisher_id", COUNT(DISTINCT "count") as "count" FROM (SELECT "books".*, "books"."id" AS "count" FROM "books" AS "books" LEFT JOIN "books_x_tags" AS "books_x_tags" ON ("books"."id" = "books_x_tags"."book_id") LEFT JOIN "tags" AS "tags_any" ON (("books_x_tags"."tag_id" = "tags_any"."id") AND "tags_any"."id" = 1) WHERE "books"."publisher_id" IN (1) GROUP BY "books"."id" HAVING (("books"."title" = 'Book 1') OR (COUNT("tags_any"."id") > 0))) AS "temp" GROUP BY "publisher_id";
+SELECT "publisher_id", COUNT(DISTINCT "count") as "count" FROM (SELECT "books".*, "books"."id" AS "count" FROM "books" AS "books" LEFT JOIN "books_x_tags" AS "books_x_tags" ON ("books"."id" = "books_x_tags"."book_id") LEFT JOIN "tags" AS "tags_any" ON (("books_x_tags"."tag_id" = "tags_any"."id") AND "tags_any"."id" = 1) WHERE "books"."publisher_id" IN (1) GROUP BY "books"."title", "books"."id" HAVING (("books"."title" = 'Book 1') OR (COUNT("tags_any"."id") > 0))) AS "temp" GROUP BY "publisher_id";

tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipOneHasManyTest_testUniqueConstraintException.sql

@@ -1,6 +1,7 @@
 SELECT "tags".* FROM "tags" AS "tags" WHERE (("tags"."id" = 2));
 SELECT "authors".* FROM "public"."authors" AS "authors" WHERE (("authors"."id" = 2));
 START TRANSACTION;
+INSERT INTO "tag_followers" ("created_at", "author_id", "tag_id") VALUES ('2021-12-02 19:21:00.000000'::timestamptz, 2, 2);
 ROLLBACK;
 SELECT "authors".* FROM "public"."authors" AS "authors" WHERE (("authors"."id" = 1));
 START TRANSACTION;

tests/sqls/NextrasTests/Orm/Integration/Relationships/RelationshipsOneHasManyPersistenceTest_testForeignKeyInNonConnectedRelationship.sql

@@ -9,6 +9,7 @@ SELECT "users_x_users"."my_friends_id", "users_x_users"."friends_with_me_id" FRO
 SELECT "users".* FROM "users" AS "users" WHERE (("users"."id" IN (2)));
 START TRANSACTION;
 DELETE FROM "users_x_users" WHERE ("my_friends_id", "friends_with_me_id") IN ((2, 1));
+DELETE FROM "users" WHERE "id" = 1;
 ROLLBACK;
 SELECT "users".* FROM "users" AS "users" WHERE (("users"."id" IN (1, 2)));
 SELECT "user_stats".* FROM "user_stats" AS "user_stats" WHERE ((("user_stats"."user_id", "user_stats"."date") IN ((1, '2021-12-14 21:03:00.000000'::timestamptz))));