Merge pull request #10778 from creative-commoners/pulls/5/aggregate-having

FIX Use OR conjuctive in filterAny aggregate queries

src/ORM/DataList.php

@@ -12,6 +12,7 @@ use InvalidArgumentException;
 use LogicException;
 use BadMethodCallException;
 use Traversable;
+use SilverStripe\ORM\DataQuery;
 
 /**
  * Implements a "lazy loading" DataObjectSet.
@@ -525,14 +526,27 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li
             throw new InvalidArgumentException('Incorrect number of arguments passed to filterAny()');
         }
 
-        return $this->alterDataQuery(function (DataQuery $query) use ($whereArguments) {
+        $list = $this->alterDataQuery(function (DataQuery $query) use ($whereArguments) {
-            $subquery = $query->disjunctiveGroup();
+            $subquery = $this->getFilterAnySubquery($query, $whereArguments);
-
             foreach ($whereArguments as $field => $value) {
                 $filter = $this->createSearchFilter($field, $value);
                 $filter->apply($subquery);
             }
         });
+
+        return $list;
+    }
+
+    private function getFilterAnySubquery(DataQuery $query, array $whereArguments): DataQuery_SubGroup
+    {
+        $clause = 'WHERE';
+        foreach (array_keys($whereArguments) as $field) {
+            if (preg_match('#\.(COUNT|SUM|AVG|MIN|MAX)\(#', strtoupper($field))) {
+                $clause = 'HAVING';
+                break;
+            }
+        }
+        return $query->disjunctiveGroup($clause);
     }
 
     /**

src/ORM/DataQuery.php

@@ -658,23 +658,41 @@ class DataQuery
      *
      * That is a subgroup joined by OR
      *
+     * @param string $clause
      * @return DataQuery_SubGroup
      */
     public function disjunctiveGroup()
     {
-        return new DataQuery_SubGroup($this, 'OR');
+        // using func_get_args to add a new param while retaining BC
+        // @deprecated - add a new param for CMS 6 - string $clause = 'WHERE'
+        $clause = 'WHERE';
+        $args = func_get_args();
+        if (count($args) > 0) {
+            $clause = $args[0];
+        }
+        return new DataQuery_SubGroup($this, 'OR', $clause);
     }
 
+
+
     /**
      * Create a conjunctive subgroup
      *
      * That is a subgroup joined by AND
      *
+     * @param string $clause
      * @return DataQuery_SubGroup
      */
     public function conjunctiveGroup()
     {
-        return new DataQuery_SubGroup($this, 'AND');
+        // using func_get_args to add a new param while retaining BC
+        // @deprecated - add a new param for CMS 6 - string $clause = 'WHERE'
+        $clause = 'WHERE';
+        $args = func_get_args();
+        if (count($args) > 0) {
+            $clause = $args[0];
+        }
+        return new DataQuery_SubGroup($this, 'AND', $clause);
     }
 
     /**

src/ORM/DataQuery_SubGroup.php

@@ -4,6 +4,8 @@ namespace SilverStripe\ORM;
 
 use SilverStripe\ORM\Queries\SQLConditionGroup;
 use SilverStripe\ORM\Queries\SQLSelect;
+use InvalidArgumentException;
+use LogicException;
 
 /**
  * Represents a subgroup inside a WHERE clause in a {@link DataQuery}
@@ -14,26 +16,54 @@ use SilverStripe\ORM\Queries\SQLSelect;
  */
 class DataQuery_SubGroup extends DataQuery implements SQLConditionGroup
 {
+    private string $clause;
 
     /**
-     *
      * @var SQLSelect
      */
     protected $whereQuery;
 
+    /**
+     * @var SQLSelect
+     */
+    protected $havingQuery;
+
+    /**
+     * @param DataQuery $base
+     * @param string $connective
+     * @param string $clause
+     */
     public function __construct(DataQuery $base, $connective)
     {
+        // using func_get_args to add a 3rd param while retaining BC
+        // @deprecated - add a 3rd param for CMS 6 - string $clause = 'WHERE'
+        $clause = 'WHERE';
+        $args = func_get_args();
+        if (count($args) > 2) {
+            $clause = $args[2];
+        }
         parent::__construct($base->dataClass);
         $this->query = $base->query;
-        $this->whereQuery = new SQLSelect();
+        $this->clause = strtoupper($clause);
-        $this->whereQuery->setConnective($connective);
+        if ($this->clause === 'WHERE') {
-
+            $this->whereQuery = new SQLSelect();
-        $base->where($this);
+            $this->whereQuery->setConnective($connective);
+            $base->where($this);
+        } elseif ($this->clause === 'HAVING') {
+            $this->havingQuery = new SQLSelect();
+            $this->havingQuery->setConnective($connective);
+            $base->having($this);
+        } else {
+            throw new InvalidArgumentException('$clause must be either WHERE or HAVING');
+        }
     }
 
     public function where($filter)
     {
-        if ($filter) {
+        if ($this->clause === 'HAVING') {
+            throw new LogicException('Cannot call where() when clause is set to HAVING');
+        }
+        if ($filter && $this->whereQuery) {
             $this->whereQuery->addWhere($filter);
         }
 
@@ -42,25 +72,46 @@ class DataQuery_SubGroup extends DataQuery implements SQLConditionGroup
 
     public function whereAny($filter)
     {
-        if ($filter) {
+        if ($this->clause === 'HAVING') {
+            throw new LogicException('Cannot call whereAny() when clause is set to HAVING');
+        }
+        if ($filter && $this->whereQuery) {
             $this->whereQuery->addWhereAny($filter);
         }
 
         return $this;
     }
 
+    public function having($filter)
+    {
+        if ($this->clause === 'WHERE') {
+            throw new LogicException('Cannot call having() when clause is set to WHERE');
+        }
+        if ($filter && $this->havingQuery) {
+            $this->havingQuery->addHaving($filter);
+        }
+
+        return $this;
+    }
+
     public function conditionSQL(&$parameters)
     {
         $parameters = [];
 
-        // Ignore empty conditions
+        if ($this->clause === 'WHERE') {
-        $where = $this->whereQuery->getWhere();
+            $where = $this->whereQuery->getWhere();
-        if (empty($where)) {
+            if (!empty($where)) {
-            return null;
+                $sql = DB::get_conn()->getQueryBuilder()->buildWhereFragment($this->whereQuery, $parameters);
+                return preg_replace('/^\s*WHERE\s*/i', '', $sql ?? '');
+            }
+        } elseif ($this->clause === 'HAVING') {
+            $having = $this->havingQuery->getHaving();
+            if (!empty($having)) {
+                $sql = DB::get_conn()->getQueryBuilder()->buildHavingFragment($this->havingQuery, $parameters);
+                return preg_replace('/^\s*HAVING\s*/i', '', $sql ?? '');
+            }
         }
 
-        // Allow database to manage joining of conditions
+        return null;
-        $sql = DB::get_conn()->getQueryBuilder()->buildWhereFragment($this->whereQuery, $parameters);
-        return preg_replace('/^\s*WHERE\s*/i', '', $sql ?? '');
     }
 }

src/ORM/ManyManyList.php

@@ -15,7 +15,6 @@ use Exception;
  */
 class ManyManyList extends RelationList
 {
-
     /**
      * @var string $joinTable
      */

tests/php/ORM/DataListTest.php

@@ -1073,6 +1073,36 @@ class DataListTest extends SapphireTest
         );
     }
 
+    private function createTeam(int $playerCount)
+    {
+        $team = Team::create();
+        $team->write();
+        for ($i = 0; $i < $playerCount; $i++) {
+            $player = Player::create();
+            $player->write();
+            $team->Players()->add($player);
+        }
+        return $team;
+    }
+
+    public function testFilterAnyManyManyAggregate()
+    {
+        Team::get()->removeAll();
+        $team1 = $this->createTeam(1);
+        $team2 = $this->createTeam(2);
+        $team3 = $this->createTeam(3);
+        $list = Team::get()->filterAny([
+            'Players.Count():LessThan' => 2,
+            'Players.Count():GreaterThan' => 2,
+        ]);
+        $match = 'HAVING ((COUNT("players_Member"."ID") < ?) OR (COUNT("players_Member"."ID") > ?))';
+        $sql = str_replace("\n", '', $list->sql());
+        $this->assertTrue(str_contains($sql, $match));
+        $ids = $list->column('ID');
+        sort($ids);
+        $this->assertSame([$team1->ID, $team3->ID], $ids);
+    }
+
     public function testFilterOnJoin()
     {
         $list = TeamComment::get()

tests/php/ORM/DataQuery_SubGroupTest.php

@@ -0,0 +1,45 @@
+<?php
+
+namespace SilverStripe\ORM\Tests;
+
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\ORM\DataQuery;
+use SilverStripe\ORM\DataQuery_SubGroup;
+use SilverStripe\ORM\DataObject;
+use SilverStripe\ORM\Tests\DataObjectTest\Team;
+use LogicException;
+use InvalidArgumentException;
+
+class DataQuery_SubGroupTest extends SapphireTest
+{
+    public function testConstructorException()
+    {
+        $this->expectException(InvalidArgumentException::class);
+        $this->expectExceptionMessage('$clause must be either WHERE or HAVING');
+        new DataQuery_SubGroup(new DataQuery(Team::class), 'AND', 'INVALID');
+    }
+
+    public function testWhereException()
+    {
+        $this->expectException(LogicException::class);
+        $this->expectExceptionMessage('Cannot call where() when clause is set to HAVING');
+        $query = new DataQuery_SubGroup(new DataQuery(Team::class), 'AND', 'HAVING');
+        $query->where([]);
+    }
+
+    public function testWhereAnyException()
+    {
+        $this->expectException(LogicException::class);
+        $this->expectExceptionMessage('Cannot call whereAny() when clause is set to HAVING');
+        $query = new DataQuery_SubGroup(new DataQuery(Team::class), 'AND', 'HAVING');
+        $query->whereAny([]);
+    }
+
+    public function testHavingException()
+    {
+        $this->expectException(LogicException::class);
+        $this->expectExceptionMessage('Cannot call having() when clause is set to WHERE');
+        $query = new DataQuery_SubGroup(new DataQuery(Team::class), 'AND', 'WHERE');
+        $query->having([]);
+    }
+}