From 1b6a82eef0cf27f3a00dd085bc190ab618c6f8af Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Thu, 22 Jul 2021 22:37:50 -0400 Subject: [PATCH] APIv4 - Improve metadata about SQL functions, including translated labels This splits the concept of prefix/suffix into prefix, flag_before and flag_after, Since a prefix like ORDER BY is not the same as a flag like DISTINCT. Lays the groundwork for exposing more info about SQL functiont to a UI like SearchKIt. --- Civi/Api4/Query/SqlFunction.php | 50 ++++++++++++------- Civi/Api4/Query/SqlFunctionAVG.php | 2 +- Civi/Api4/Query/SqlFunctionCOUNT.php | 2 +- Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php | 10 ++-- Civi/Api4/Query/SqlFunctionMAX.php | 2 +- Civi/Api4/Query/SqlFunctionMIN.php | 2 +- Civi/Api4/Query/SqlFunctionSUM.php | 2 +- .../crmSearchFunction.component.js | 12 +++-- .../ang/crmSearchAdmin/crmSearchFunction.html | 6 +-- .../api/v4/Query/SqlExpressionParserTest.php | 16 +++--- 10 files changed, 61 insertions(+), 43 deletions(-) diff --git a/Civi/Api4/Query/SqlFunction.php b/Civi/Api4/Query/SqlFunction.php index 542c567919..e62262d828 100644 --- a/Civi/Api4/Query/SqlFunction.php +++ b/Civi/Api4/Query/SqlFunction.php @@ -42,19 +42,38 @@ abstract class SqlFunction extends SqlExpression { protected function initialize() { $arg = trim(substr($this->expr, strpos($this->expr, '(') + 1, -1)); foreach ($this->getParams() as $idx => $param) { - $prefix = $this->captureKeyword($param['prefix'], $arg); + $prefix = NULL; + if ($param['prefix']) { + $prefix = $this->captureKeyword([$param['prefix']], $arg); + // Supply api_default + if (!$prefix && isset($param['api_default'])) { + $this->args[$idx] = [ + 'prefix' => $param['api_default']['prefix'] ?? [$param['prefix']], + 'expr' => array_map([parent::class, 'convert'], $param['api_default']['expr']), + 'suffix' => $param['api_default']['suffix'] ?? [], + ]; + continue; + } + if (!$prefix && !$param['optional']) { + throw new \API_Exception("Missing {$param['prefix']} for SQL function " . static::getName()); + } + } + elseif ($param['flag_before']) { + $prefix = $this->captureKeyword(array_keys($param['flag_before']), $arg); + } $this->args[$idx] = [ - 'prefix' => $prefix, + 'prefix' => (array) $prefix, 'expr' => [], - 'suffix' => NULL, + 'suffix' => [], ]; - if ($param['max_expr'] && isset($prefix) || in_array('', $param['prefix']) || !$param['optional']) { + if ($param['max_expr'] && (!$param['prefix'] || $param['prefix'] === $prefix)) { $exprs = $this->captureExpressions($arg, $param['must_be'], $param['cant_be']); if (count($exprs) < $param['min_expr'] || count($exprs) > $param['max_expr']) { throw new \API_Exception('Incorrect number of arguments for SQL function ' . static::getName()); } $this->args[$idx]['expr'] = $exprs; - $this->args[$idx]['suffix'] = $this->captureKeyword($param['suffix'], $arg); + + $this->args[$idx]['suffix'] = (array) $this->captureKeyword(array_keys($param['flag_after']), $arg); } } } @@ -68,7 +87,7 @@ abstract class SqlFunction extends SqlExpression { * @return mixed|null */ private function captureKeyword($keywords, &$arg) { - foreach (array_filter($keywords) as $key) { + foreach ($keywords as $key) { if (strpos($arg, $key . ' ') === 0) { $arg = ltrim(substr($arg, strlen($key))); return $key; @@ -178,23 +197,15 @@ abstract class SqlFunction extends SqlExpression { * @return string */ private function renderArg($arg, $param, $fieldList): string { - // Supply api_default - if (!isset($arg['prefix']) && !isset($arg['suffix']) && empty($arg['expr']) && !empty($param['api_default'])) { - $arg = [ - 'prefix' => $param['api_default']['prefix'] ?? reset($param['prefix']), - 'expr' => array_map([parent::class, 'convert'], $param['api_default']['expr'] ?? []), - 'suffix' => $param['api_default']['suffix'] ?? reset($param['suffix']), - ]; - } - $rendered = $arg['prefix'] ?? ''; + $rendered = implode(' ', $arg['prefix']); foreach ($arg['expr'] ?? [] as $idx => $expr) { if (strlen($rendered) || $idx) { $rendered .= $idx ? ', ' : ' '; } $rendered .= $expr->render($fieldList); } - if (isset($arg['suffix'])) { - $rendered .= (strlen($rendered) ? ' ' : '') . $arg['suffix']; + if ($arg['suffix']) { + $rendered .= (strlen($rendered) ? ' ' : '') . implode(' ', $arg['suffix']); } return $rendered; } @@ -224,10 +235,11 @@ abstract class SqlFunction extends SqlExpression { foreach (static::params() as $param) { // Merge in defaults to ensure each param has these properties $params[] = $param + [ - 'prefix' => [], + 'prefix' => NULL, 'min_expr' => 1, 'max_expr' => 1, - 'suffix' => [], + 'flag_before' => [], + 'flag_after' => [], 'optional' => FALSE, 'must_be' => [], 'cant_be' => ['SqlWild'], diff --git a/Civi/Api4/Query/SqlFunctionAVG.php b/Civi/Api4/Query/SqlFunctionAVG.php index 60aff454e8..59e6548a73 100644 --- a/Civi/Api4/Query/SqlFunctionAVG.php +++ b/Civi/Api4/Query/SqlFunctionAVG.php @@ -21,7 +21,7 @@ class SqlFunctionAVG extends SqlFunction { protected static function params(): array { return [ [ - 'prefix' => ['', 'DISTINCT', 'ALL'], + 'flag_before' => ['DISTINCT' => ts('Distinct')], 'must_be' => ['SqlField'], ], ]; diff --git a/Civi/Api4/Query/SqlFunctionCOUNT.php b/Civi/Api4/Query/SqlFunctionCOUNT.php index 6e933f011c..10808e4322 100644 --- a/Civi/Api4/Query/SqlFunctionCOUNT.php +++ b/Civi/Api4/Query/SqlFunctionCOUNT.php @@ -21,7 +21,7 @@ class SqlFunctionCOUNT extends SqlFunction { protected static function params(): array { return [ [ - 'prefix' => ['', 'DISTINCT', 'ALL'], + 'flag_before' => ['DISTINCT' => ts('Distinct')], 'max_expr' => 1, 'must_be' => ['SqlField', 'SqlWild'], 'cant_be' => [], diff --git a/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php b/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php index 6af0d62d48..f79e54a965 100644 --- a/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php +++ b/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php @@ -23,20 +23,20 @@ class SqlFunctionGROUP_CONCAT extends SqlFunction { protected static function params(): array { return [ [ - 'prefix' => ['', 'DISTINCT', 'ALL'], + 'flag_before' => ['DISTINCT' => ts('Distinct')], 'max_expr' => 1, 'must_be' => ['SqlField', 'SqlFunction'], 'optional' => FALSE, ], [ - 'prefix' => ['ORDER BY'], + 'prefix' => 'ORDER BY', 'max_expr' => 1, - 'suffix' => ['', 'ASC', 'DESC'], + 'flag_after' => ['ASC' => ts('Ascending'), 'DESC' => ts('Descending')], 'must_be' => ['SqlField'], 'optional' => TRUE, ], [ - 'prefix' => ['SEPARATOR'], + 'prefix' => 'SEPARATOR', 'max_expr' => 1, 'must_be' => ['SqlString'], 'optional' => TRUE, @@ -59,7 +59,7 @@ class SqlFunctionGROUP_CONCAT extends SqlFunction { public function formatOutputValue($value, &$dataType) { $exprArgs = $this->getArgs(); // By default, values are split into an array and formatted according to the field's dataType - if (!$exprArgs[2]['prefix']) { + if (isset($exprArgs[2]['expr'][0]->expr) && $exprArgs[2]['expr'][0]->expr === \CRM_Core_DAO::VALUE_SEPARATOR) { $value = explode(\CRM_Core_DAO::VALUE_SEPARATOR, $value); // If the first expression is another sqlFunction, allow it to control the dataType if ($exprArgs[0]['expr'][0] instanceof SqlFunction && is_callable([$exprArgs[0]['expr'][0], 'formatOutputValue'])) { diff --git a/Civi/Api4/Query/SqlFunctionMAX.php b/Civi/Api4/Query/SqlFunctionMAX.php index 71a3b1329f..6e3a880bf4 100644 --- a/Civi/Api4/Query/SqlFunctionMAX.php +++ b/Civi/Api4/Query/SqlFunctionMAX.php @@ -23,7 +23,7 @@ class SqlFunctionMAX extends SqlFunction { protected static function params(): array { return [ [ - 'prefix' => ['', 'DISTINCT', 'ALL'], + 'flag_before' => ['DISTINCT' => ts('Distinct')], 'must_be' => ['SqlField'], ], ]; diff --git a/Civi/Api4/Query/SqlFunctionMIN.php b/Civi/Api4/Query/SqlFunctionMIN.php index ca8d48a3f8..3c6a79227c 100644 --- a/Civi/Api4/Query/SqlFunctionMIN.php +++ b/Civi/Api4/Query/SqlFunctionMIN.php @@ -23,7 +23,7 @@ class SqlFunctionMIN extends SqlFunction { protected static function params(): array { return [ [ - 'prefix' => ['', 'DISTINCT', 'ALL'], + 'flag_before' => ['DISTINCT' => ts('Distinct')], 'must_be' => ['SqlField'], ], ]; diff --git a/Civi/Api4/Query/SqlFunctionSUM.php b/Civi/Api4/Query/SqlFunctionSUM.php index 0354036cd9..28522692aa 100644 --- a/Civi/Api4/Query/SqlFunctionSUM.php +++ b/Civi/Api4/Query/SqlFunctionSUM.php @@ -21,7 +21,7 @@ class SqlFunctionSUM extends SqlFunction { protected static function params(): array { return [ [ - 'prefix' => ['', 'DISTINCT', 'ALL'], + 'flag_before' => ['DISTINCT' => ts('Distinct')], 'must_be' => ['SqlField'], ], ]; diff --git a/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.component.js b/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.component.js index c0049331f3..24f1f35f58 100644 --- a/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.component.js +++ b/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.component.js @@ -23,11 +23,12 @@ function initFunction() { ctrl.fnInfo = _.find(CRM.crmSearchAdmin.functions, {name: ctrl.fn}); - if (ctrl.fnInfo && _.includes(ctrl.fnInfo.params[0].prefix, 'DISTINCT')) { - ctrl.modifierAllowed = true; + if (ctrl.fnInfo && ctrl.fnInfo.params[0] && !_.isEmpty(ctrl.fnInfo.params[0].flag_before)) { + ctrl.modifierName = _.keys(ctrl.fnInfo.params[0].flag_before)[0]; + ctrl.modifierLabel = ctrl.fnInfo.params[0].flag_before[ctrl.modifierName]; } else { - ctrl.modifierAllowed = false; + ctrl.modifierName = null; ctrl.modifier = null; } } @@ -37,6 +38,11 @@ ctrl.writeExpr(); }; + this.toggleModifier = function() { + ctrl.modifier = ctrl.modifier ? null : ctrl. modifierName; + ctrl.writeExpr(); + }; + // Make a sql-friendly alias for this expression function makeAlias() { return (ctrl.fn + '_' + (ctrl.modifier ? ctrl.modifier + '_' : '') + ctrl.path).replace(/[.:]/g, '_'); diff --git a/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.html b/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.html index 03238b825c..d8a380e780 100644 --- a/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.html +++ b/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.html @@ -1,8 +1,8 @@
-
diff --git a/tests/phpunit/api/v4/Query/SqlExpressionParserTest.php b/tests/phpunit/api/v4/Query/SqlExpressionParserTest.php index edb3053807..7bc8c59e4b 100644 --- a/tests/phpunit/api/v4/Query/SqlExpressionParserTest.php +++ b/tests/phpunit/api/v4/Query/SqlExpressionParserTest.php @@ -44,16 +44,16 @@ class SqlExpressionParserTest extends UnitTestCase { public function testAggregateFuncitons($fnName) { $className = 'Civi\Api4\Query\SqlFunction' . $fnName; $params = $className::getParams(); - $this->assertNotEmpty($params[0]['prefix']); - $this->assertEmpty($params[0]['suffix']); + $this->assertNotEmpty($params[0]['flag_before']); + $this->assertEmpty($params[0]['flag_after']); $sqlFn = new $className($fnName . '(total)'); $this->assertEquals($fnName, $sqlFn->getName()); $this->assertEquals(['total'], $sqlFn->getFields()); $args = $sqlFn->getArgs(); $this->assertCount(1, $args); - $this->assertNull($args[0]['prefix']); - $this->assertNull($args[0]['suffix']); + $this->assertEmpty($args[0]['prefix']); + $this->assertEmpty($args[0]['suffix']); $this->assertTrue(is_a($args[0]['expr'][0], 'Civi\Api4\Query\SqlField')); $sqlFn = SqlExpression::convert($fnName . '(DISTINCT stuff)'); @@ -63,8 +63,8 @@ class SqlExpressionParserTest extends UnitTestCase { $this->assertEquals(['stuff'], $sqlFn->getFields()); $args = $sqlFn->getArgs(); $this->assertCount(1, $args); - $this->assertEquals('DISTINCT', $args[0]['prefix']); - $this->assertNull($args[0]['suffix']); + $this->assertEquals('DISTINCT', $args[0]['prefix'][0]); + $this->assertEmpty($args[0]['suffix']); $this->assertTrue(is_a($args[0]['expr'][0], 'Civi\Api4\Query\SqlField')); try { @@ -72,8 +72,8 @@ class SqlExpressionParserTest extends UnitTestCase { if ($fnName === 'COUNT') { $args = $sqlFn->getArgs(); $this->assertCount(1, $args); - $this->assertNull($args[0]['prefix']); - $this->assertNull($args[0]['suffix']); + $this->assertEmpty($args[0]['prefix']); + $this->assertEmpty($args[0]['suffix']); $this->assertTrue(is_a($args[0]['expr'][0], 'Civi\Api4\Query\SqlWild')); } else { -- 2.25.1