Add aggregate field support to having in postgres and sqlserver (#14057)

Replace aliases in having conditions with their referenced expressions
in SQLServer and Postgres. This adds functional parity with MySQL and
SQLite.

Co-authored-by: stickler-ci <support@stickler-ci.com>
Co-authored-by: Mark Story <mark@mark-story.com>

src/Database/PostgresCompiler.php

@@ -16,6 +16,8 @@ declare(strict_types=1);
  */
 namespace Cake\Database;
 
+use Cake\Database\Expression\FunctionExpression;
+
 /**
  * Responsible for compiling a Query object into its SQL representation
  * for Postgres
@@ -32,4 +34,60 @@ class PostgresCompiler extends QueryCompiler
      * @var bool
      */
     protected $_quotedSelectAliases = true;
+
+    /**
+     * {@inheritDoc}
+     */
+    protected $_templates = [
+        'delete' => 'DELETE',
+        'where' => ' WHERE %s',
+        'group' => ' GROUP BY %s ',
+        'order' => ' %s',
+        'limit' => ' LIMIT %s',
+        'offset' => ' OFFSET %s',
+        'epilog' => ' %s',
+    ];
+
+    /**
+     * Helper function used to build the string representation of a HAVING clause,
+     * it constructs the field list taking care of aliasing and
+     * converting expression objects to string.
+     *
+     * @param array $parts list of fields to be transformed to string
+     * @param \Cake\Database\Query $query The query that is being compiled
+     * @param \Cake\Database\ValueBinder $generator the placeholder generator to be used in expressions
+     * @return string
+     */
+    protected function _buildHavingPart($parts, $query, $generator)
+    {
+        $selectParts = $query->clause('select');
+
+        foreach ($selectParts as $selectKey => $selectPart) {
+            if (!$selectPart instanceof FunctionExpression) {
+                continue;
+            }
+            foreach ($parts as $k => $p) {
+                if (!is_string($p)) {
+                    continue;
+                }
+                preg_match_all(
+                    '/\b' . trim($selectKey, '"') . '\b/i',
+                    $p,
+                    $matches
+                );
+
+                if (empty($matches[0])) {
+                    continue;
+                }
+
+                $parts[$k] = preg_replace(
+                    ['/"/', '/\b' . trim($selectKey, '"') . '\b/i'],
+                    ['', $selectPart->sql($generator)],
+                    $p
+                );
+            }
+        }
+
+        return sprintf('HAVING %s', implode(', ', $parts));
+    }
 }

src/Database/SqlserverCompiler.php

@@ -16,6 +16,8 @@ declare(strict_types=1);
  */
 namespace Cake\Database;
 
+use Cake\Database\Expression\FunctionExpression;
+
 /**
  * Responsible for compiling a Query object into its SQL representation
  * for SQL Server
@@ -38,7 +40,6 @@ class SqlserverCompiler extends QueryCompiler
         'delete' => 'DELETE',
         'where' => ' WHERE %s',
         'group' => ' GROUP BY %s ',
-        'having' => ' HAVING %s ',
         'order' => ' %s',
         'offset' => ' OFFSET %s ROWS',
         'epilog' => ' %s',
@@ -93,4 +94,47 @@ class SqlserverCompiler extends QueryCompiler
 
         return sprintf(' FETCH FIRST %d ROWS ONLY', $limit);
     }
+
+    /**
+     * Helper function used to build the string representation of a HAVING clause,
+     * it constructs the field list taking care of aliasing and
+     * converting expression objects to string.
+     *
+     * @param array $parts list of fields to be transformed to string
+     * @param \Cake\Database\Query $query The query that is being compiled
+     * @param \Cake\Database\ValueBinder $generator the placeholder generator to be used in expressions
+     * @return string
+     */
+    protected function _buildHavingPart($parts, $query, $generator)
+    {
+        $selectParts = $query->clause('select');
+
+        foreach ($selectParts as $selectKey => $selectPart) {
+            if (!$selectPart instanceof FunctionExpression) {
+                continue;
+            }
+            foreach ($parts as $k => $p) {
+                if (!is_string($p)) {
+                    continue;
+                }
+                preg_match_all(
+                    '/\b' . trim($selectKey, '[]') . '\b/i',
+                    $p,
+                    $matches
+                );
+
+                if (empty($matches[0])) {
+                    continue;
+                }
+
+                $parts[$k] = preg_replace(
+                    ['/\[|\]/', '/\b' . trim($selectKey, '[]') . '\b/i'],
+                    ['', $selectPart->sql($generator)],
+                    $p
+                );
+            }
+        }
+
+        return sprintf('HAVING %s', implode(', ', $parts));
+    }
 }

tests/TestCase/Database/Driver/PostgresTest.php

@@ -173,4 +173,72 @@ class PostgresTest extends TestCase
         $query = $translator($query);
         $this->assertSame('FOO', $query->clause('epilog'));
     }
+
+    /**
+     * Test that having queries replace the aggregated alias field.
+     *
+     * @return void
+     */
+    public function testHavingReplacesAlias()
+    {
+        $driver = $this->getMockBuilder('Cake\Database\Driver\Postgres')
+            ->setMethods(['connect', 'getConnection', 'version'])
+            ->setConstructorArgs([[]])
+            ->getMock();
+
+        $connection = $this->getMockBuilder('\Cake\Database\Connection')
+            ->setMethods(['connect', 'getDriver', 'setDriver'])
+            ->setConstructorArgs([['log' => false]])
+            ->getMock();
+        $connection->expects($this->any())
+            ->method('getDriver')
+            ->will($this->returnValue($driver));
+
+        $query = new Query($connection);
+        $query
+            ->select([
+                'posts.author_id',
+                'post_count' => $query->func()->count('posts.id'),
+            ])
+            ->group(['posts.author_id'])
+            ->having([$query->newExpr()->gte('post_count', 2, 'integer')]);
+
+        $expected = 'SELECT posts.author_id, (COUNT(posts.id)) AS "post_count" ' .
+            'GROUP BY posts.author_id HAVING COUNT(posts.id) >= :c0';
+        $this->assertEquals($expected, $query->sql());
+    }
+
+    /**
+     * Test that having queries replaces nothing if no alias is used.
+     *
+     * @return void
+     */
+    public function testHavingWhenNoAliasIsUsed()
+    {
+        $driver = $this->getMockBuilder('Cake\Database\Driver\Postgres')
+            ->setMethods(['connect', 'getConnection', 'version'])
+            ->setConstructorArgs([[]])
+            ->getMock();
+
+        $connection = $this->getMockBuilder('\Cake\Database\Connection')
+            ->setMethods(['connect', 'getDriver', 'setDriver'])
+            ->setConstructorArgs([['log' => false]])
+            ->getMock();
+        $connection->expects($this->any())
+            ->method('getDriver')
+            ->will($this->returnValue($driver));
+
+        $query = new Query($connection);
+        $query
+            ->select([
+                'posts.author_id',
+                'post_count' => $query->func()->count('posts.id'),
+            ])
+            ->group(['posts.author_id'])
+            ->having([$query->newExpr()->gte('posts.author_id', 2, 'integer')]);
+
+        $expected = 'SELECT posts.author_id, (COUNT(posts.id)) AS "post_count" ' .
+            'GROUP BY posts.author_id HAVING posts.author_id >= :c0';
+        $this->assertEquals($expected, $query->sql());
+    }
 }

tests/TestCase/Database/Driver/SqlserverTest.php

@@ -387,4 +387,78 @@ class SqlserverTest extends TestCase
         $expected = 'INSERT INTO articles (title) OUTPUT INSERTED.* VALUES (:c0)';
         $this->assertEquals($expected, $query->sql());
     }
+
+    /**
+     * Test that having queries replace the aggregated alias field.
+     *
+     * @return void
+     */
+    public function testHavingReplacesAlias()
+    {
+        $driver = $this->getMockBuilder('Cake\Database\Driver\Sqlserver')
+            ->setMethods(['connect', 'getConnection', 'version'])
+            ->setConstructorArgs([[]])
+            ->getMock();
+        $driver->expects($this->any())
+            ->method('version')
+            ->will($this->returnValue('8'));
+
+        $connection = $this->getMockBuilder('\Cake\Database\Connection')
+            ->setMethods(['connect', 'getDriver', 'setDriver'])
+            ->setConstructorArgs([['log' => false]])
+            ->getMock();
+        $connection->expects($this->any())
+            ->method('getDriver')
+            ->will($this->returnValue($driver));
+
+        $query = new Query($connection);
+        $query
+            ->select([
+                'posts.author_id',
+                'post_count' => $query->func()->count('posts.id'),
+            ])
+            ->group(['posts.author_id'])
+            ->having([$query->newExpr()->gte('post_count', 2, 'integer')]);
+
+        $expected = 'SELECT posts.author_id, (COUNT(posts.id)) AS post_count ' .
+            'GROUP BY posts.author_id HAVING COUNT(posts.id) >= :c0';
+        $this->assertEquals($expected, $query->sql());
+    }
+
+    /**
+     * Test that having queries replaces nothing is no alias is used.
+     *
+     * @return void
+     */
+    public function testHavingWhenNoAliasIsUsed()
+    {
+        $driver = $this->getMockBuilder('Cake\Database\Driver\Sqlserver')
+            ->setMethods(['connect', 'getConnection', 'version'])
+            ->setConstructorArgs([[]])
+            ->getMock();
+        $driver->expects($this->any())
+            ->method('version')
+            ->will($this->returnValue('8'));
+
+        $connection = $this->getMockBuilder('\Cake\Database\Connection')
+            ->setMethods(['connect', 'getDriver', 'setDriver'])
+            ->setConstructorArgs([['log' => false]])
+            ->getMock();
+        $connection->expects($this->any())
+            ->method('getDriver')
+            ->will($this->returnValue($driver));
+
+        $query = new Query($connection);
+        $query
+            ->select([
+                'posts.author_id',
+                'post_count' => $query->func()->count('posts.id'),
+            ])
+            ->group(['posts.author_id'])
+            ->having([$query->newExpr()->gte('posts.author_id', 2, 'integer')]);
+
+        $expected = 'SELECT posts.author_id, (COUNT(posts.id)) AS post_count ' .
+            'GROUP BY posts.author_id HAVING posts.author_id >= :c0';
+        $this->assertEquals($expected, $query->sql());
+    }
 }

tests/TestCase/ORM/QueryTest.php

@@ -3830,4 +3830,36 @@ class QueryTest extends TestCase
                 ->find()
                 ->selectAllExcept([], ['body']);
     }
+
+    /**
+     * Tests that using Having on an aggregated field returns the correct result
+     * model in the query
+     *
+     * @return void
+     */
+    public function testHavingOnAnAggregatedField()
+    {
+        $post = $this->getTableLocator()->get('posts');
+
+        $query = new Query($this->connection, $post);
+
+        $results = $query
+            ->select([
+                'posts.author_id',
+                'post_count' => $query->func()->count('posts.id'),
+            ])
+            ->group(['posts.author_id'])
+            ->having([$query->newExpr()->gte('post_count', 2, 'integer')])
+            ->enableHydration(false)
+            ->toArray();
+
+        $expected = [
+            [
+                'author_id' => 1,
+                'post_count' => 2,
+            ],
+        ];
+
+        $this->assertEquals($expected, $results);
+    }
 }