fix missing GROUP BY, rework its handling & internal representation

[closes #518][refs #548]
nextras · Mar 20, 2024 · 44cebf8 · 44cebf8
1 parent 527369d
commit 44cebf8
Show file tree

Hide file tree

Showing 35 changed files with 287 additions and 168 deletions.
diff --git a/src/Collection/Aggregations/AnyAggregator.php b/src/Collection/Aggregations/AnyAggregator.php
@@ -10,7 +10,6 @@
 use Nextras\Orm\Exception\InvalidArgumentException;
 use function array_merge;
 use function array_pop;
-use function count;
 
 
 /**
@@ -65,14 +64,9 @@ public function aggregateExpression(
 		if ($join === null) {
 			throw new InvalidArgumentException('Any aggregation applied over expression without a relationship.');
 		}
-		if (count($join->groupByColumns) === 0) {
+		if ($join->toPrimaryKey === null) {
 			throw new InvalidArgumentException(
-				'Aggregation applied over a table join without specifying a group-by column (primary key).',
-			);
-		}
-		if (count($join->groupByColumns) > 1) {
-			throw new InvalidArgumentException(
-				'Aggregation applied over a table join with multiple group-by columns; currently, this is not supported.',
+				'Aggregation applied over a table-join without specifying a toPrimaryKey.',
 			);
 		}
 
@@ -82,12 +76,11 @@ public function aggregateExpression(
 			toAlias: $join->toAlias,
 			onExpression: "($join->onExpression) AND $expression->expression",
 			onArgs: array_merge($join->onArgs, $expression->args),
-			groupByColumns: $join->groupByColumns,
 		);
 
 		return new DbalExpressionResult(
-			expression: 'COUNT(%table.%column) > 0',
-			args: [$join->toAlias, $join->groupByColumns[0]],
+			expression: 'COUNT(%column) > 0',
+			args: [$join->toPrimaryKey],
 			joins: $joins,
 			groupBy: $expression->groupBy,
 			isHavingClause: true,

diff --git a/src/Collection/Aggregations/CountAggregator.php b/src/Collection/Aggregations/CountAggregator.php
@@ -60,14 +60,9 @@ public function aggregateExpression(
 		if ($join === null) {
 			throw new InvalidArgumentException('Count aggregation applied over expression without a relationship.');
 		}
-		if (count($join->groupByColumns) === 0) {
+		if ($join->toPrimaryKey === null) {
 			throw new InvalidArgumentException(
-				'Aggregation applied over a table join without specifying a group-by column (primary key).',
-			);
-		}
-		if (count($join->groupByColumns) > 1) {
-			throw new InvalidArgumentException(
-				'Aggregation applied over a table join with multiple group-by columns; currently, this is not supported.',
+				'Aggregation applied over a table-join without specifying a toPrimaryKey.',
 			);
 		}
 
@@ -77,18 +72,15 @@ public function aggregateExpression(
 			toAlias: $join->toAlias,
 			onExpression: "($join->onExpression) AND $expression->expression",
 			onArgs: array_merge($join->onArgs, $expression->args),
-			groupByColumns: $join->groupByColumns,
 		);
 
 		if ($this->atLeast !== null && $this->atMost !== null) {
 			return new DbalExpressionResult(
-				expression: 'COUNT(%table.%column) >= %i AND COUNT(%table.%column) <= %i',
+				expression: 'COUNT(%column) >= %i AND COUNT(%column) <= %i',
 				args: [
-					$join->toAlias,
-					$join->groupByColumns[0],
+					$join->toPrimaryKey,
 					$this->atLeast,
-					$join->toAlias,
-					$join->groupByColumns[0],
+					$join->toPrimaryKey,
 					$this->atMost,
 				],
 				joins: $joins,
@@ -97,10 +89,9 @@ public function aggregateExpression(
 			);
 		} elseif ($this->atMost !== null) {
 			return new DbalExpressionResult(
-				expression: 'COUNT(%table.%column) <= %i',
+				expression: 'COUNT(%column) <= %i',
 				args: [
-					$join->toAlias,
-					$join->groupByColumns[0],
+					$join->toPrimaryKey,
 					$this->atMost,
 				],
 				joins: $joins,
@@ -109,10 +100,9 @@ public function aggregateExpression(
 			);
 		} else {
 			return new DbalExpressionResult(
-				expression: 'COUNT(%table.%column) >= %i',
+				expression: 'COUNT(%column) >= %i',
 				args: [
-					$join->toAlias,
-					$join->groupByColumns[0],
+					$join->toPrimaryKey,
 					$this->atLeast,
 				],
 				joins: $joins,

diff --git a/src/Collection/Aggregations/NoneAggregator.php b/src/Collection/Aggregations/NoneAggregator.php
@@ -10,7 +10,6 @@
 use Nextras\Orm\Exception\InvalidArgumentException;
 use function array_merge;
 use function array_pop;
-use function count;
 
 
 /**
@@ -59,14 +58,9 @@ public function aggregateExpression(
 		if ($join === null) {
 			throw new InvalidArgumentException('None aggregation applied over expression without a relationship.');
 		}
-		if (count($join->groupByColumns) === 0) {
+		if ($join->toPrimaryKey === null) {
 			throw new InvalidArgumentException(
-				'Aggregation applied over a table join without specifying a group-by column (primary key).',
-			);
-		}
-		if (count($join->groupByColumns) > 1) {
-			throw new InvalidArgumentException(
-				'Aggregation applied over a table join with multiple group-by columns; currently, this is not supported.',
+				'Aggregation applied over a table-join without specifying a toPrimaryKey.',
 			);
 		}
 
@@ -76,12 +70,11 @@ public function aggregateExpression(
 			toAlias: $join->toAlias,
 			onExpression: "($join->onExpression) AND $expression->expression",
 			onArgs: array_merge($join->onArgs, $expression->args),
-			groupByColumns: $join->groupByColumns,
 		);
 
 		return new DbalExpressionResult(
-			expression: 'COUNT(%table.%column) = 0',
-			args: [$join->toAlias, $join->groupByColumns[0]],
+			expression: 'COUNT(%column) = 0',
+			args: [$join->toPrimaryKey],
 			joins: $joins,
 			groupBy: $expression->groupBy,
 			isHavingClause: true,

diff --git a/src/Collection/DbalCollection.php b/src/Collection/DbalCollection.php
@@ -17,7 +17,6 @@
 use Nextras\Orm\Mapper\IRelationshipMapper;
 use function count;
 use function is_array;
-use function str_repeat;
 
 
 /**
@@ -314,8 +313,8 @@ public function getQueryBuilder(): QueryBuilder
 				aggregator: 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);
@@ -325,24 +324,29 @@ public function getQueryBuilder(): QueryBuilder
 
 		foreach ($this->ordering as [$expression, $direction]) {
 			$joins = array_merge($joins, $expression->joins);
-			if ($expression->isHavingClause) {
-				$groupBy = array_merge($groupBy, $expression->groupBy);
-			}
+			$groupBy = array_merge($groupBy, $expression->groupBy);
 			$orderingExpression = $helper->processOrderDirection($expression, $direction);
 			$this->queryBuilder->addOrderBy('%ex', $orderingExpression);
 		}
-		$this->ordering = [];
 
 		$mergedJoins = $helper->mergeJoins('%and', $joins);
 		foreach ($mergedJoins as $join) {
 			$join->applyJoin($this->queryBuilder);
 		}
 
 		if (count($groupBy) > 0) {
-			$this->queryBuilder->groupBy(
-				'%ex' . str_repeat(', %ex', count($groupBy) - 1),
-				...$groupBy,
-			);
+			foreach ($this->ordering as [$expression]) {
+				$groupBy = array_merge($groupBy, $expression->columns);
+			}
+		}
+		$this->ordering = [];
+
+		if (count($groupBy) > 0) {
+			$unique = [];
+			foreach ($groupBy as $groupByFqn) {
+				$unique[$groupByFqn->getUnescaped()] = $groupByFqn;
+			}
+			$this->queryBuilder->groupBy('%column[]', array_values($unique));
 		}
 
 		return $this->queryBuilder;

diff --git a/src/Collection/Functions/CompareEqualsFunction.php b/src/Collection/Functions/CompareEqualsFunction.php
@@ -4,9 +4,6 @@
 
 
 use Nextras\Orm\Collection\Functions\Result\DbalExpressionResult;
-use Nextras\Orm\Exception\InvalidArgumentException;
-use function array_combine;
-use function array_map;
 use function count;
 use function explode;
 use function in_array;
@@ -34,29 +31,25 @@ protected function evaluateInDb(
 		if (is_array($value)) {
 			if (count($value) > 0) {
 				// Multi-column primary key handling
-				// extract column names for multiOr simplification
-				// array{%column, array<string>}
+				// Construct multiOr simplification as array{list<Fqn>, modifiers: list<string>, values: list<list<mixed>>}
 				$args = $expression->getArgumentsForExpansion();
 				if (count($args) === 2 && $args[0] === '%column' && is_array($args[1])) {
-					$modifiers = explode(',', $modifier);
-					$columns = [];
-					foreach ($args[1] as $i => $column) {
-						$columns[] = $column . $modifiers[$i];
-					}
-					$value = array_map(function ($value) use ($columns): array {
-						$combined = array_combine($columns, $value);
-						if ($combined === false) { // @phpstan-ignore-line
-							$pn = count($columns);
-							$vn = count($value);
-							throw new InvalidArgumentException("Number of values ($vn) does not match number of properties ($pn).");
+					$columns = $args[1];
+					$modifiers = array_map(
+						fn (string $modifier): ?string => strlen($modifier) === 0 ? null : $modifier,
+						explode(',', $modifier)
+					);
+					$data = [];
+					foreach ($value as $dataSet) {
+						$set = [];
+						foreach ($dataSet as $i => $dataSetValue) {
+							$set[] = [$columns[$i], $dataSetValue, $modifiers[$i] ?? null];
 						}
-						return $combined;
-					}, $value);
-					return $expression->withArgs('%multiOr', [$value]);
-				} else {
-					if ($modifier !== '%any') {
-						$modifier .= '[]';
+						$data[] = $set;
 					}
+					return $expression->withArgs('%multiOr', [$data]);
+				} else {
+					if ($modifier !== '%any') $modifier .= '[]';
 					return $expression->append("IN $modifier", $value);
 				}
 			} else {

diff --git a/src/Collection/Functions/CompareNotEqualsFunction.php b/src/Collection/Functions/CompareNotEqualsFunction.php
@@ -34,29 +34,25 @@ protected function evaluateInDb(
 		if (is_array($value)) {
 			if (count($value) > 0) {
 				// Multi-column primary key handling
-				// extract column names for multiOr simplification
-				// array{%column, array<string>}
+				// Construct multiOr simplification as array{list<Fqn>, modifiers: list<string>, values: list<list<mixed>>}
 				$args = $expression->getArgumentsForExpansion();
 				if (count($args) === 2 && $args[0] === '%column' && is_array($args[1])) {
-					$modifiers = explode(',', $modifier);
-					$columns = [];
-					foreach ($args[1] as $i => $column) {
-						$columns[] = $column . $modifiers[$i];
-					}
-					$value = array_map(function ($value) use ($columns): array {
-						$combined = array_combine($columns, $value);
-						if ($combined === false) { // @phpstan-ignore-line
-							$pn = count($columns);
-							$vn = count($value);
-							throw new InvalidArgumentException("Number of values ($vn) does not match number of properties ($pn).");
+					$columns = $args[1];
+					$modifiers = array_map(
+						fn (string $modifier): ?string => strlen($modifier) === 0 ? null : $modifier,
+						explode(',', $modifier)
+					);
+					$data = [];
+					foreach ($value as $dataSet) {
+						$set = [];
+						foreach ($dataSet as $i => $dataSetValue) {
+							$set[] = [$columns[$i], $dataSetValue, $modifiers[$i] ?? null];
 						}
-						return $combined;
-					}, $value);
-					return $expression->withArgs('NOT (%multiOr)', [$value]);
-				} else {
-					if ($modifier !== '%any') {
-						$modifier .= '[]';
+						$data[] = $set;
 					}
+					return $expression->withArgs('NOT (%multiOr)', [$data]);
+				} else {
+					if ($modifier !== '%any') $modifier .= '[]';
 					return $expression->append("NOT IN $modifier", $value);
 				}
 			} else {