From ff2a73cb2a6bb07460edc0a368149a333dafd637 Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Wed, 30 Mar 2022 08:55:00 -0400 Subject: [PATCH] SearchKit - Fix ORDER BY calculated fields when using GROUP BY Before: Calculated field functions were repeated verbabim in the SELECT, ORDER BY & GROUP BY clause, this would cause mySql error when using FULL GROUP BY mode. After: Calculated field function defined in SELECT clause, alias used in ORDER BY & GROUP BY. No error when grouping by an ordering by the same calculated field. Side-benefit: When selecting and ordering by RAND(), the selected random numbers will be in order, as it's now the same random function used for both instead of a different one. --- Civi/Api4/Query/Api4SelectQuery.php | 22 +++++++++++++++++-- .../v4/SearchSegment/SearchSegmentTest.php | 18 ++++++++------- .../phpunit/api/v4/Action/SqlFunctionTest.php | 17 ++++++++------ 3 files changed, 40 insertions(+), 17 deletions(-) diff --git a/Civi/Api4/Query/Api4SelectQuery.php b/Civi/Api4/Query/Api4SelectQuery.php index 68a624428c..7b111afba9 100644 --- a/Civi/Api4/Query/Api4SelectQuery.php +++ b/Civi/Api4/Query/Api4SelectQuery.php @@ -327,7 +327,7 @@ class Api4SelectQuery { try { $expr = $this->getExpression($item); - $column = $expr->render($this->apiFieldSpec); + $column = $this->renderExpr($expr); // Use FIELD() function to sort on pseudoconstant values $suffix = strstr($item, ':'); @@ -376,7 +376,7 @@ class Api4SelectQuery { */ protected function buildGroupBy() { foreach ($this->getGroupBy() as $item) { - $this->query->groupBy($this->getExpression($item)->render($this->apiFieldSpec)); + $this->query->groupBy($this->renderExpr($this->getExpression($item))); } } @@ -1287,6 +1287,24 @@ class Api4SelectQuery { ]; } + /** + * Returns rendered expression or alias if it is already aliased in the SELECT clause. + * + * @param $expr + * @return mixed|string + */ + protected function renderExpr($expr) { + $exprVal = explode(':', $expr->getExpr())[0]; + // If this expression is already in use in the select clause, use the existing alias + // This allows calculated fields to be reused in SELECT, GROUP BY and ORDER BY + foreach ($this->selectAliases as $alias => $selectVal) { + if ($exprVal === explode(':', $selectVal)[0]) { + return "`$alias`"; + } + } + return $expr->render($this->apiFieldSpec); + } + /** * Add something to the api's debug output if debugging is enabled * diff --git a/ext/search_kit/tests/phpunit/api/v4/SearchSegment/SearchSegmentTest.php b/ext/search_kit/tests/phpunit/api/v4/SearchSegment/SearchSegmentTest.php index dfc45233b6..6976123d1b 100644 --- a/ext/search_kit/tests/phpunit/api/v4/SearchSegment/SearchSegmentTest.php +++ b/ext/search_kit/tests/phpunit/api/v4/SearchSegment/SearchSegmentTest.php @@ -97,26 +97,28 @@ class SearchSegmentTest extends \PHPUnit\Framework\TestCase implements HeadlessI 'having' => [], ], ], + 'sort' => [['segment_Giving_Tier:label', 'ASC']], ]; $result = civicrm_api4('SearchDisplay', 'run', $params); $this->assertCount(4, $result); - $this->assertEquals('Low ball', $result[0]['columns'][0]['val']); - $this->assertEquals(1.5, $result[0]['data']['AVG_total_amount']); + // Results should be in alphabetical order by giving tier + $this->assertEquals('Heavy hitter', $result[0]['columns'][0]['val']); + $this->assertEquals(56.0, $result[0]['data']['AVG_total_amount']); $this->assertEquals(1, $result[0]['data']['COUNT_total_amount']); - $this->assertEquals('Minor league', $result[1]['columns'][0]['val']); - $this->assertEquals(15.0, $result[1]['data']['AVG_total_amount']); - $this->assertEquals(2, $result[1]['data']['COUNT_total_amount']); + $this->assertEquals('Low ball', $result[1]['columns'][0]['val']); + $this->assertEquals(1.5, $result[1]['data']['AVG_total_amount']); + $this->assertEquals(1, $result[1]['data']['COUNT_total_amount']); $this->assertEquals('Major league', $result[2]['columns'][0]['val']); $this->assertEquals(30.0, $result[2]['data']['AVG_total_amount']); $this->assertEquals(3, $result[2]['data']['COUNT_total_amount']); - $this->assertEquals('Heavy hitter', $result[3]['columns'][0]['val']); - $this->assertEquals(56.0, $result[3]['data']['AVG_total_amount']); - $this->assertEquals(1, $result[3]['data']['COUNT_total_amount']); + $this->assertEquals('Minor league', $result[3]['columns'][0]['val']); + $this->assertEquals(15.0, $result[3]['data']['AVG_total_amount']); + $this->assertEquals(2, $result[3]['data']['COUNT_total_amount']); } } diff --git a/tests/phpunit/api/v4/Action/SqlFunctionTest.php b/tests/phpunit/api/v4/Action/SqlFunctionTest.php index 1a9e631b1c..f56096fb84 100644 --- a/tests/phpunit/api/v4/Action/SqlFunctionTest.php +++ b/tests/phpunit/api/v4/Action/SqlFunctionTest.php @@ -246,20 +246,23 @@ class SqlFunctionTest extends UnitTestCase { } public function testRandFunction() { - $cid = Contact::create(FALSE) - ->addValue('first_name', 'hello') - ->execute()->first()['id']; + Contact::save(FALSE) + ->setRecords(array_fill(0, 6, [])) + ->execute(); $result = Contact::get(FALSE) ->addSelect('RAND() AS rand') ->addOrderBy('RAND()') ->setDebug(TRUE) - ->setLimit(1) + ->setLimit(6) ->execute(); - $this->assertStringContainsString('ORDER BY RAND()', $result->debug['sql'][0]); - $this->assertGreaterThanOrEqual(0, $result[0]['rand']); - $this->assertLessThan(1, $result[0]['rand']); + // Random numbers should have been ordered from least to greatest + $this->assertGreaterThanOrEqual($result[0]['rand'], $result[1]['rand']); + $this->assertGreaterThanOrEqual($result[1]['rand'], $result[2]['rand']); + $this->assertGreaterThanOrEqual($result[2]['rand'], $result[3]['rand']); + $this->assertGreaterThanOrEqual($result[3]['rand'], $result[4]['rand']); + $this->assertGreaterThanOrEqual($result[4]['rand'], $result[5]['rand']); } public function testYearInWhereClause() { -- 2.25.1