Merge pull request #685 from nextras/fix-duplicated-results-in-aggreg…

…ation

separate WHERE & HAVING in expression result & fix buggy having rewrite

src/Collection/Aggregations/AnyAggregator.php

@@ -79,11 +79,12 @@ public function aggregateExpression(
 		);
 
 		return new DbalExpressionResult(
-			expression: 'COUNT(%column) > 0',
-			args: [$join->toPrimaryKey],
+			expression: null,
+			args: [],
 			joins: $joins,
 			groupBy: $expression->groupBy,
-			isHavingClause: true,
+			havingExpression: 'COUNT(%column) > 0',
+			havingArgs: [$join->toPrimaryKey],
 		);
 	}
 }

src/Collection/Aggregations/CountAggregator.php

@@ -76,38 +76,41 @@ public function aggregateExpression(
 
 		if ($this->atLeast !== null && $this->atMost !== null) {
 			return new DbalExpressionResult(
-				expression: 'COUNT(%column) >= %i AND COUNT(%column) <= %i',
-				args: [
+				expression: null,
+				args: [],
+				joins: $joins,
+				groupBy: $expression->groupBy,
+				havingExpression: 'COUNT(%column) >= %i AND COUNT(%column) <= %i',
+				havingArgs: [
 					$join->toPrimaryKey,
 					$this->atLeast,
 					$join->toPrimaryKey,
 					$this->atMost,
 				],
-				joins: $joins,
-				groupBy: $expression->groupBy,
-				isHavingClause: true,
 			);
 		} elseif ($this->atMost !== null) {
 			return new DbalExpressionResult(
-				expression: 'COUNT(%column) <= %i',
-				args: [
+				expression: null,
+				args: [],
+				joins: $joins,
+				groupBy: $expression->groupBy,
+				havingExpression: 'COUNT(%column) <= %i',
+				havingArgs: [
 					$join->toPrimaryKey,
 					$this->atMost,
 				],
-				joins: $joins,
-				groupBy: $expression->groupBy,
-				isHavingClause: true,
 			);
 		} else {
 			return new DbalExpressionResult(
-				expression: 'COUNT(%column) >= %i',
-				args: [
+				expression: null,
+				args: [],
+				joins: $joins,
+				groupBy: $expression->groupBy,
+				havingExpression: 'COUNT(%column) >= %i',
+				havingArgs: [
 					$join->toPrimaryKey,
 					$this->atLeast,
 				],
-				joins: $joins,
-				groupBy: $expression->groupBy,
-				isHavingClause: true,
 			);
 		}
 	}

src/Collection/Aggregations/NoneAggregator.php

@@ -73,11 +73,12 @@ public function aggregateExpression(
 		);
 
 		return new DbalExpressionResult(
-			expression: 'COUNT(%column) = 0',
-			args: [$join->toPrimaryKey],
+			expression: null,
+			args: [],
 			joins: $joins,
 			groupBy: $expression->groupBy,
-			isHavingClause: true,
+			havingExpression: 'COUNT(%column) = 0',
+			havingArgs: [$join->toPrimaryKey],
 		);
 	}
 }

src/Collection/Aggregations/NumericAggregator.php

@@ -46,11 +46,12 @@ public function aggregateExpression(
 	): DbalExpressionResult
 	{
 		return new DbalExpressionResult(
-			expression: "{$this->dbalAggregationFunction}($expression->expression)",
-			args: $expression->args,
+			expression: null,
+			args: [],
 			joins: $expression->joins,
 			groupBy: $expression->groupBy,
-			isHavingClause: true,
+			havingExpression: "{$this->dbalAggregationFunction}($expression->expression)",
+			havingArgs: $expression->args,
 		);
 	}
 }

src/Collection/DbalCollection.php

@@ -299,11 +299,12 @@ public function getQueryBuilder(): QueryBuilder
 			);
 			$joins = $expression->joins;
 			$groupBy = $expression->groupBy;
-			if ($expression->isHavingClause) {
-				$this->queryBuilder->andHaving($expression->expression, ...$expression->args);
-			} else {
+			if ($expression->expression !== null) {
 				$this->queryBuilder->andWhere($expression->expression, ...$expression->args);
 			}
+			if ($expression->havingExpression !== null) {
+				$this->queryBuilder->andHaving($expression->havingExpression, ...$expression->havingArgs);
+			}
 			if ($this->mapper->getDatabasePlatform()->getName() === MySqlPlatform::NAME) {
 				$this->applyGroupByWithSameNamedColumnsWorkaround($this->queryBuilder, $groupBy);
 			}

src/Collection/Expression/ExpressionContext.php

@@ -4,8 +4,6 @@
 
 
 /**
- * @internal
- *
  * Determines if the expression is processed for AND subtree, OR subtree or as a pure expression,
  * e.g., a sorting expression.
  *

src/Collection/Functions/CompareEqualsFunction.php

@@ -32,7 +32,7 @@ protected function evaluateInDb(
 			if (count($value) > 0) {
 				// Multi-column primary key handling
 				// Construct multiOr simplification as array{list<Fqn>, modifiers: list<string>, values: list<list<mixed>>}
-				$args = $expression->getArgumentsForExpansion();
+				$args = $expression->getArgsForExpansion();
 				if (count($args) === 2 && $args[0] === '%column' && is_array($args[1]) && is_array($modifier)) {
 					$columns = $args[1];
 					$data = [];

src/Collection/Functions/CompareNotEqualsFunction.php

@@ -35,7 +35,7 @@ protected function evaluateInDb(
 			if (count($value) > 0) {
 				// Multi-column primary key handling
 				// Construct multiOr simplification as array{list<Fqn>, modifiers: list<string>, values: list<list<mixed>>}
-				$args = $expression->getArgumentsForExpansion();
+				$args = $expression->getArgsForExpansion();
 				if (count($args) === 2 && $args[0] === '%column' && is_array($args[1]) && is_array($modifier)) {
 					$columns = $args[1];
 					$data = [];

src/Collection/Functions/JunctionFunctionTrait.php

@@ -65,8 +65,8 @@ protected function processQueryBuilderExpressionWithModifier(
 		?Aggregator $aggregator,
 	): DbalExpressionResult
 	{
-		$isHavingClause = false;
 		$processedArgs = [];
+		$processedHavingArgs = [];
 		$joins = [];
 		$groupBy = [];
 		$columns = [];
@@ -80,20 +80,40 @@ protected function processQueryBuilderExpressionWithModifier(
 		foreach ($normalized as $collectionFunctionArgs) {
 			$expression = $helper->processExpression($builder, $collectionFunctionArgs, $context, $aggregator);
 			$expression = $expression->applyAggregator($builder, $context);
-			$processedArgs[] = $expression->getArgumentsForExpansion();
+			$whereArgs = $expression->getArgsForExpansion();
+			if ($whereArgs !== []) {
+				$processedArgs[] = $whereArgs;
+			}
+			$havingArgs = $expression->getHavingArgsForExpansion();
+			if ($havingArgs !== []) {
+				$processedHavingArgs[] = $havingArgs;
+			}
 			$joins = array_merge($joins, $expression->joins);
 			$groupBy = array_merge($groupBy, $expression->groupBy);
 			$columns = array_merge($columns, $expression->columns);
-			$isHavingClause = $isHavingClause || $expression->isHavingClause;
 		}
 
-		return new DbalExpressionResult(
-			expression: $dbalModifier,
-			args: [$processedArgs],
-			joins: $helper->mergeJoins($dbalModifier, $joins),
-			groupBy: $isHavingClause ? array_merge($groupBy, $columns) : $groupBy,
-			columns: $isHavingClause ? [] : $columns,
-			isHavingClause: $isHavingClause,
-		);
+		if ($context === ExpressionContext::FilterOr && $processedHavingArgs !== []) {
+			// move all where expressions to HAVING clause
+			return new DbalExpressionResult(
+				expression: null,
+				args: [],
+				joins: $helper->mergeJoins($dbalModifier, $joins),
+				groupBy: array_merge($groupBy, $columns),
+				havingExpression: $dbalModifier,
+				havingArgs: [array_merge($processedArgs, $processedHavingArgs)],
+				columns: [],
+			);
+		} else {
+			return new DbalExpressionResult(
+				expression: $processedArgs === [] ? null : $dbalModifier,
+				args: $processedArgs === [] ? [] : [$processedArgs],
+				joins: $helper->mergeJoins($dbalModifier, $joins),
+				groupBy: $groupBy,
+				havingExpression: $processedHavingArgs === [] ? null : $dbalModifier,
+				havingArgs: $processedHavingArgs === [] ? [] : [$processedHavingArgs],
+				columns: $columns,
+			);
+		}
 	}
 }

src/Collection/Functions/Result/DbalExpressionResult.php

@@ -8,6 +8,7 @@
 use Nextras\Orm\Collection\Aggregations\Aggregator;
 use Nextras\Orm\Collection\Expression\ExpressionContext;
 use Nextras\Orm\Entity\Reflection\PropertyMetadata;
+use Nextras\Orm\Exception\InvalidStateException;
 use function array_unshift;
 use function array_values;
 
@@ -31,25 +32,27 @@ class DbalExpressionResult
 
 
 	/**
-	 * @param literal-string $expression Holds expression separately from its arguments. Put Dbal's modifiers into the expression and arguments separately.
+	 * @param literal-string|null $expression Holds expression separately from its arguments. Put Dbal's modifiers into the expression and arguments separately.
 	 * @param list<mixed> $args Expression's arguments.
 	 * @param list<DbalTableJoin> $joins
 	 * @param list<Fqn> $groupBy List of columns used for grouping.
+	 * @param literal-string|null $havingExpression Holds expression for HAVING clause separately from its arguments. Put Dbal's modifiers into the expression and arguments separately.
+	 * @param list<mixed> $havingArgs HAVING clause expression's arguments.
 	 * @param list<Fqn> $columns List of columns used in the expression. If needed, this is later used to properly reference in GROUP BY clause.
 	 * @param Aggregator<mixed>|null $aggregator Result aggregator that is applied later.
-	 * @param bool $isHavingClause True if the expression represents HAVING clause instead of WHERE clause.
 	 * @param PropertyMetadata|null $propertyMetadata Reference to backing property of the expression. If null, the expression is no more a simple property expression.
 	 * @param (callable(mixed): mixed)|null $valueNormalizer Normalizes the value for better PHP comparison, it considers the backing property type.
 	 * @param literal-string|list<literal-string|null>|null $dbalModifier Dbal modifier for particular column. Array if multi-column. Null value means expression is a general expression.
 	 */
 	public function __construct(
-		public readonly string $expression,
+		public readonly string|null $expression,
 		public readonly array $args,
 		public readonly array $joins = [],
 		public readonly array $groupBy = [],
+		public readonly string|null $havingExpression = null,
+		public readonly array $havingArgs = [],
 		public readonly array $columns = [],
 		public readonly ?Aggregator $aggregator = null,
-		public readonly bool $isHavingClause = false,
 		public readonly ?PropertyMetadata $propertyMetadata = null,
 		?callable $valueNormalizer = null,
 		public readonly string|array|null $dbalModifier = null,
@@ -62,13 +65,29 @@ public function __construct(
 	/**
 	 * Appends SQL expression to the original expression.
 	 * If you need prepend or other complex expression, create new instance of DbalExpressionResult.
+	 *
+	 * It auto-detects if expression or havingExpression should be appended. If both them are used, it throws exception.
+	 *
 	 * @param literal-string $expression
 	 * @param mixed ...$args
 	 */
 	public function append(string $expression, ...$args): DbalExpressionResult
 	{
-		$args = array_values(array_merge($this->args, $args));
-		return $this->withArgs("{$this->expression} $expression", $args);
+		if ($this->expression !== null && $this->havingExpression !== null) {
+			throw new InvalidStateException(
+				'Cannot append result to a DbalExpressionResult because the both $expression (' .
+				$this->expression . ') and $havingExpression (' . $this->havingExpression . ')' .
+				'are already defined. Modify expression manually using withArgs() or withHavingArgs(). ',
+			);
+		}
+
+		if ($this->expression !== null) {
+			$args = array_values(array_merge($this->args, $args));
+			return $this->withArgs("{$this->expression} $expression", $args);
+		} else {
+			$args = array_values(array_merge($this->havingArgs, $args));
+			return $this->withHavingArgs("{$this->havingExpression} $expression", $args);
+		}
 	}
 
 
@@ -77,14 +96,29 @@ public function append(string $expression, ...$args): DbalExpressionResult
 	 * Suitable as an `%ex` modifier argument.
 	 * @return array<mixed>
 	 */
-	public function getArgumentsForExpansion(): array
+	public function getArgsForExpansion(): array
 	{
+		if ($this->expression === null) return [];
 		$args = $this->args;
 		array_unshift($args, $this->expression);
 		return $args;
 	}
 
 
+	/**
+	 * Returns all HAVING clause arguments including the HAVING expression.
+	 * Suitable as an `%ex` modifier argument.
+	 * @return array<mixed>
+	 */
+	public function getHavingArgsForExpansion(): array
+	{
+		if ($this->havingExpression === null) return [];
+		$args = $this->havingArgs;
+		array_unshift($args, $this->havingExpression);
+		return $args;
+	}
+
+
 	/**
 	 * Creates a new DbalExpression from the passed $args and keeps the original expression
 	 * properties (joins, aggregator, ...).
@@ -98,9 +132,31 @@ public function withArgs(string $expression, array $args): DbalExpressionResult
 			args: $args,
 			joins: $this->joins,
 			groupBy: $this->groupBy,
+			havingExpression: $this->havingExpression,
+			havingArgs: $this->havingArgs,
+			columns: $this->columns,
+			aggregator: $this->aggregator,
+		);
+	}
+
+
+	/**
+	 * Creates a new DbalExpression from the passed $havingArgs and keeps the original having expression
+	 * properties (joins, aggregator, ...).
+	 * @param literal-string $havingExpression
+	 * @param list<mixed> $havingArgs
+	 */
+	public function withHavingArgs(string $havingExpression, array $havingArgs): DbalExpressionResult
+	{
+		return new DbalExpressionResult(
+			expression: $this->expression,
+			args: $this->args,
+			joins: $this->joins,
+			groupBy: $this->groupBy,
+			havingExpression: $havingExpression,
+			havingArgs: $havingArgs,
 			columns: $this->columns,
 			aggregator: $this->aggregator,
-			isHavingClause: $this->isHavingClause,
 		);
 	}
 

src/Collection/Helpers/DbalQueryBuilderHelper.php

@@ -97,7 +97,11 @@ public function processExpression(
 	 */
 	public function processOrderDirection(DbalExpressionResult $expression, string $direction): array
 	{
-		$args = $expression->getArgumentsForExpansion();
+		if ($expression->expression !== null) {
+			$args = $expression->getArgsForExpansion();
+		} else {
+			$args = $expression->getHavingArgsForExpansion();
+		}
 		if ($this->platformName === 'mysql') {
 			if ($direction === ICollection::ASC || $direction === ICollection::ASC_NULLS_FIRST) {
 				return ['%ex ASC', $args];

tests/cases/integration/Collection/collection.aggregation.join.phpt

@@ -12,6 +12,7 @@ use Nextras\Orm\Collection\Aggregations\AnyAggregator;
 use Nextras\Orm\Collection\Aggregations\CountAggregator;
 use Nextras\Orm\Collection\Aggregations\NoneAggregator;
 use Nextras\Orm\Collection\Functions\CompareEqualsFunction;
+use Nextras\Orm\Collection\Functions\CompareGreaterThanFunction;
 use Nextras\Orm\Collection\Functions\CountAggregateFunction;
 use Nextras\Orm\Collection\ICollection;
 use NextrasTests\Orm\DataTestCase;
@@ -67,6 +68,32 @@ class CollectionAggregationJoinTest extends DataTestCase
 	}
 
 
+	public function testIndependentAnyWithGroupingOverPk(): void
+	{
+		// Select books that:
+		// - have tags 2 OR 3
+		// - AND have more than 1 tags
+		// Matches books #1, #2
+		$books = $this->orm->books->findBy([
+			ICollection::AND,
+			['tags->id' => [2, 3]],
+			[CompareGreaterThanFunction::class, [CountAggregateFunction::class, 'tags->id'], 1],
+		]);
+		Assert::same(2, $books->count());
+
+		// Select books that:
+		// - have tags 2 OR 3
+		// - OR have more than 1 tags
+		// Matches books #1, #2, #3
+		$books = $this->orm->books->findBy([
+			ICollection::OR,
+			['tags->id' => [2, 3]],
+			[CompareGreaterThanFunction::class, [CountAggregateFunction::class, 'tags->id'], 1],
+		]);
+		Assert::same(3, $books->count());
+	}
+
+
 	public function testAnyDependent(): void
 	{
 		/*