From 9e9feedffa6146ffb211f5a47de305b9bf591d22 Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Fri, 24 Sep 2021 09:11:45 -0400 Subject: [PATCH] APIv4 - Restructure function params, add labels --- Civi/Api4/Query/SqlEquation.php | 2 +- Civi/Api4/Query/SqlExpression.php | 9 ++--- Civi/Api4/Query/SqlFunction.php | 22 ++++++++---- Civi/Api4/Query/SqlFunctionCOALESCE.php | 5 +-- Civi/Api4/Query/SqlFunctionCONCAT.php | 4 +-- Civi/Api4/Query/SqlFunctionCONCAT_WS.php | 10 +++--- Civi/Api4/Query/SqlFunctionGREATEST.php | 5 +-- Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php | 1 + Civi/Api4/Query/SqlFunctionIF.php | 20 ++++++----- Civi/Api4/Query/SqlFunctionLEAST.php | 5 +-- Civi/Api4/Query/SqlFunctionNULLIF.php | 5 +-- Civi/Api4/Query/SqlFunctionREPLACE.php | 18 ++++++---- Civi/Api4/Query/SqlFunctionROUND.php | 14 ++++---- .../crmSearchFunction.component.js | 35 +++++++++++++------ .../ang/crmSearchAdmin/crmSearchFunction.html | 10 +++--- .../phpunit/api/v4/Action/SqlFunctionTest.php | 24 ++++++++++--- 16 files changed, 112 insertions(+), 77 deletions(-) diff --git a/Civi/Api4/Query/SqlEquation.php b/Civi/Api4/Query/SqlEquation.php index 7585ab0d08..f29fb343d8 100644 --- a/Civi/Api4/Query/SqlEquation.php +++ b/Civi/Api4/Query/SqlEquation.php @@ -53,7 +53,7 @@ class SqlEquation extends SqlExpression { $permitted = ['SqlField', 'SqlString', 'SqlNumber', 'SqlNull']; $operators = array_merge(self::$arithmeticOperators, self::$comparisonOperators); while (strlen($arg)) { - $this->args = array_merge($this->args, $this->captureExpressions($arg, $permitted, FALSE)); + $this->args = array_merge($this->args, $this->captureExpressions($arg, $permitted, 1)); $op = $this->captureKeyword($operators, $arg); if ($op) { $this->args[] = $op; diff --git a/Civi/Api4/Query/SqlExpression.php b/Civi/Api4/Query/SqlExpression.php index c052281315..4cbe46386a 100644 --- a/Civi/Api4/Query/SqlExpression.php +++ b/Civi/Api4/Query/SqlExpression.php @@ -188,7 +188,8 @@ abstract class SqlExpression { */ protected function captureKeyword($keywords, &$arg) { foreach ($keywords as $key) { - if (strpos($arg, $key . ' ') === 0) { + // Match keyword followed by a space or eol + if (strpos($arg, $key . ' ') === 0 || rtrim($arg) === $key) { $arg = ltrim(substr($arg, strlen($key))); return $key; } @@ -201,11 +202,11 @@ abstract class SqlExpression { * * @param string $arg * @param array $mustBe - * @param bool $multi + * @param int $max * @return SqlExpression[] * @throws \API_Exception */ - protected function captureExpressions(string &$arg, array $mustBe, bool $multi) { + protected function captureExpressions(string &$arg, array $mustBe, int $max) { $captured = []; $arg = ltrim($arg); while ($arg) { @@ -215,7 +216,7 @@ abstract class SqlExpression { $this->fields = array_merge($this->fields, $expr->getFields()); $captured[] = $expr; // Keep going if we have a comma indicating another expression follows - if ($multi && substr($arg, 0, 1) === ',') { + if (count($captured) < $max && substr($arg, 0, 1) === ',') { $arg = ltrim(substr($arg, 1)); } else { diff --git a/Civi/Api4/Query/SqlFunction.php b/Civi/Api4/Query/SqlFunction.php index b1d1c94093..c5bc9b46f1 100644 --- a/Civi/Api4/Query/SqlFunction.php +++ b/Civi/Api4/Query/SqlFunction.php @@ -43,19 +43,23 @@ abstract class SqlFunction extends SqlExpression { $arg = trim(substr($this->expr, strpos($this->expr, '(') + 1, -1)); foreach ($this->getParams() as $idx => $param) { $prefix = NULL; - if ($param['name']) { - $prefix = $this->captureKeyword([$param['name']], $arg); + $name = $param['name'] ?: ($idx + 1); + // If this isn't the first param it needs to start with something; + // either the name (e.g. "ORDER BY") if it has one, or a comma separating it from the previous param. + $start = $param['name'] ?: ($idx ? ',' : NULL); + if ($start) { + $prefix = $this->captureKeyword([$start], $arg); // Supply api_default if (!$prefix && isset($param['api_default'])) { $this->args[$idx] = [ - 'prefix' => [$param['name']], + 'prefix' => [$start], 'expr' => array_map([parent::class, 'convert'], $param['api_default']['expr']), 'suffix' => [], ]; continue; } if (!$prefix && !$param['optional']) { - throw new \API_Exception("Missing {$param['name']} for SQL function " . static::getName()); + throw new \API_Exception("Missing param $name for SQL function " . static::getName()); } } elseif ($param['flag_before']) { @@ -67,18 +71,21 @@ abstract class SqlFunction extends SqlExpression { 'suffix' => [], ]; if ($param['max_expr'] && (!$param['name'] || $param['name'] === $prefix)) { - $exprs = $this->captureExpressions($arg, $param['must_be'], TRUE); + $exprs = $this->captureExpressions($arg, $param['must_be'], $param['max_expr']); if ( - (count($exprs) < $param['min_expr'] || count($exprs) > $param['max_expr']) && + count($exprs) < $param['min_expr'] && !(!$exprs && $param['optional']) ) { - throw new \API_Exception('Incorrect number of arguments for SQL function ' . static::getName()); + throw new \API_Exception("Too few arguments to param $name for SQL function " . static::getName()); } $this->args[$idx]['expr'] = $exprs; $this->args[$idx]['suffix'] = (array) $this->captureKeyword(array_keys($param['flag_after']), $arg); } } + if (trim($arg)) { + throw new \API_Exception("Too many arguments given for SQL function " . static::getName()); + } } /** @@ -158,6 +165,7 @@ abstract class SqlFunction extends SqlExpression { // Merge in defaults to ensure each param has these properties $params[] = $param + [ 'name' => NULL, + 'label' => ts('Select'), 'min_expr' => 1, 'max_expr' => 1, 'flag_before' => [], diff --git a/Civi/Api4/Query/SqlFunctionCOALESCE.php b/Civi/Api4/Query/SqlFunctionCOALESCE.php index 5264e04ef1..1616b05280 100644 --- a/Civi/Api4/Query/SqlFunctionCOALESCE.php +++ b/Civi/Api4/Query/SqlFunctionCOALESCE.php @@ -25,10 +25,7 @@ class SqlFunctionCOALESCE extends SqlFunction { [ 'max_expr' => 99, 'optional' => FALSE, - 'ui_defaults' => [ - ['type' => 'SqlField', 'placeholder' => ts('If')], - ['type' => 'SqlField', 'placeholder' => ts('Else')], - ], + 'label' => ts('Value?'), ], ]; } diff --git a/Civi/Api4/Query/SqlFunctionCONCAT.php b/Civi/Api4/Query/SqlFunctionCONCAT.php index 3659fa2d13..fff67d74f9 100644 --- a/Civi/Api4/Query/SqlFunctionCONCAT.php +++ b/Civi/Api4/Query/SqlFunctionCONCAT.php @@ -26,9 +26,7 @@ class SqlFunctionCONCAT extends SqlFunction { 'max_expr' => 99, 'optional' => FALSE, 'must_be' => ['SqlField', 'SqlString'], - 'ui_defaults' => [ - ['placeholder' => ts('Plus')], - ], + 'label' => ts('And'), ], ]; } diff --git a/Civi/Api4/Query/SqlFunctionCONCAT_WS.php b/Civi/Api4/Query/SqlFunctionCONCAT_WS.php index d381baf701..92c7a2e289 100644 --- a/Civi/Api4/Query/SqlFunctionCONCAT_WS.php +++ b/Civi/Api4/Query/SqlFunctionCONCAT_WS.php @@ -22,14 +22,16 @@ class SqlFunctionCONCAT_WS extends SqlFunction { protected static function params(): array { return [ + [ + 'optional' => FALSE, + 'must_be' => ['SqlString'], + 'label' => ts('Separator'), + ], [ 'max_expr' => 99, 'optional' => FALSE, 'must_be' => ['SqlField', 'SqlString'], - 'ui_defaults' => [ - ['type' => 'SqlString', 'placeholder' => ts('Separator')], - ['type' => 'SqlField', 'placeholder' => ts('Plus')], - ], + 'label' => ts('Plus'), ], ]; } diff --git a/Civi/Api4/Query/SqlFunctionGREATEST.php b/Civi/Api4/Query/SqlFunctionGREATEST.php index a76e35b14f..5cc4211c5d 100644 --- a/Civi/Api4/Query/SqlFunctionGREATEST.php +++ b/Civi/Api4/Query/SqlFunctionGREATEST.php @@ -26,10 +26,7 @@ class SqlFunctionGREATEST extends SqlFunction { 'max_expr' => 99, 'min_expr' => 2, 'optional' => FALSE, - 'ui_defaults' => [ - ['type' => 'SqlField', 'placeholder' => ts('If')], - ['type' => 'SqlField', 'placeholder' => ts('Else')], - ], + 'label' => ts('Else'), ], ]; } diff --git a/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php b/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php index 1b67dfb908..3c90f1a323 100644 --- a/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php +++ b/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php @@ -30,6 +30,7 @@ class SqlFunctionGROUP_CONCAT extends SqlFunction { ], [ 'name' => 'ORDER BY', + 'label' => ts('Order by'), 'max_expr' => 1, 'flag_after' => ['ASC' => ts('Ascending'), 'DESC' => ts('Descending')], 'must_be' => ['SqlField'], diff --git a/Civi/Api4/Query/SqlFunctionIF.php b/Civi/Api4/Query/SqlFunctionIF.php index e9bda2705a..810cfe2d70 100644 --- a/Civi/Api4/Query/SqlFunctionIF.php +++ b/Civi/Api4/Query/SqlFunctionIF.php @@ -23,15 +23,19 @@ class SqlFunctionIF extends SqlFunction { protected static function params(): array { return [ [ - 'min_expr' => 3, - 'max_expr' => 3, 'optional' => FALSE, - 'must_be' => ['SqlEquation', 'SqlField', 'SqlFunction', 'SqlString', 'SqlNumber', 'SqlNull'], - 'ui_defaults' => [ - ['type' => 'SqlField', 'placeholder' => ts('If')], - ['type' => 'SqlField', 'placeholder' => ts('Then')], - ['type' => 'SqlField', 'placeholder' => ts('Else')], - ], + 'must_be' => ['SqlEquation', 'SqlField'], + 'label' => ts('If'), + ], + [ + 'optional' => FALSE, + 'must_be' => ['SqlField', 'SqlString', 'SqlNumber', 'SqlNull'], + 'label' => ts('Then'), + ], + [ + 'optional' => FALSE, + 'must_be' => ['SqlField', 'SqlString', 'SqlNumber', 'SqlNull'], + 'label' => ts('Else'), ], ]; } diff --git a/Civi/Api4/Query/SqlFunctionLEAST.php b/Civi/Api4/Query/SqlFunctionLEAST.php index a79ee4a8a2..c45893421d 100644 --- a/Civi/Api4/Query/SqlFunctionLEAST.php +++ b/Civi/Api4/Query/SqlFunctionLEAST.php @@ -26,10 +26,7 @@ class SqlFunctionLEAST extends SqlFunction { 'max_expr' => 99, 'min_expr' => 2, 'optional' => FALSE, - 'ui_defaults' => [ - ['type' => 'SqlField', 'placeholder' => ts('If')], - ['type' => 'SqlField', 'placeholder' => ts('Else')], - ], + 'label' => ts('Else'), ], ]; } diff --git a/Civi/Api4/Query/SqlFunctionNULLIF.php b/Civi/Api4/Query/SqlFunctionNULLIF.php index bda6a8e2e8..07e4f2df2e 100644 --- a/Civi/Api4/Query/SqlFunctionNULLIF.php +++ b/Civi/Api4/Query/SqlFunctionNULLIF.php @@ -26,10 +26,7 @@ class SqlFunctionNULLIF extends SqlFunction { 'min_expr' => 2, 'max_expr' => 2, 'optional' => FALSE, - 'ui_defaults' => [ - ['type' => 'SqlField', 'placeholder' => ts('Preferred')], - ['type' => 'SqlField', 'placeholder' => ts('Alternate')], - ], + 'label' => ts('Compare with'), ], ]; } diff --git a/Civi/Api4/Query/SqlFunctionREPLACE.php b/Civi/Api4/Query/SqlFunctionREPLACE.php index f5a1cfbbea..961d151e23 100644 --- a/Civi/Api4/Query/SqlFunctionREPLACE.php +++ b/Civi/Api4/Query/SqlFunctionREPLACE.php @@ -21,15 +21,19 @@ class SqlFunctionREPLACE extends SqlFunction { protected static function params(): array { return [ [ - 'min_expr' => 3, - 'max_expr' => 3, + 'optional' => FALSE, + 'must_be' => ['SqlField', 'SqlString'], + 'label' => ts('Source'), + ], + [ + 'optional' => FALSE, + 'must_be' => ['SqlString', 'SqlField'], + 'label' => ts('Find'), + ], + [ 'optional' => FALSE, 'must_be' => ['SqlString', 'SqlField'], - 'ui_defaults' => [ - ['type' => 'SqlField', 'placeholder' => ts('Source')], - ['type' => 'SqlString', 'placeholder' => ts('Find')], - ['type' => 'SqlString', 'placeholder' => ts('Replace')], - ], + 'label' => ts('Replace'), ], ]; } diff --git a/Civi/Api4/Query/SqlFunctionROUND.php b/Civi/Api4/Query/SqlFunctionROUND.php index 6e09945ecd..5db43b962c 100644 --- a/Civi/Api4/Query/SqlFunctionROUND.php +++ b/Civi/Api4/Query/SqlFunctionROUND.php @@ -22,13 +22,13 @@ class SqlFunctionROUND extends SqlFunction { return [ [ 'optional' => FALSE, - 'min_expr' => 1, - 'max_expr' => 2, - 'must_be' => ['SqlNumber', 'SqlField'], - 'ui_defaults' => [ - ['type' => 'SqlField', 'placeholder' => ts('Number')], - ['type' => 'SqlNumber', 'placeholder' => ts('Decimals')], - ], + 'must_be' => ['SqlField', 'SqlNumber'], + 'label' => ts('Number'), + ], + [ + 'optional' => TRUE, + 'must_be' => ['SqlNumber'], + 'label' => ts('Decimal places'), ], ]; } diff --git a/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.component.js b/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.component.js index 25e7528269..090ae17359 100644 --- a/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.component.js +++ b/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.component.js @@ -13,8 +13,6 @@ var ts = $scope.ts = CRM.ts('org.civicrm.search_kit'), ctrl = this; - var defaultUiDefaults = {type: 'SqlField', placeholder: ts('Select')}; - var allTypes = { aggregate: ts('Aggregate'), comparison: ts('Comparison'), @@ -39,7 +37,6 @@ }; this.addArg = function(exprType) { - exprType = exprType || ctrl.getUiDefault(ctrl.args.length).type; ctrl.args.push({ type: ctrl.exprTypes[exprType].type, value: exprType === 'SqlNumber' ? 0 : '' @@ -59,17 +56,33 @@ ctrl.modifier = null; } // Push args to reach the minimum - while (ctrl.args.length < ctrl.fn.params[0].min_expr) { - ctrl.addArg(); - } + _.each(ctrl.fn.params, function(param, index) { + while ( + (ctrl.args.length - index < param.min_expr) && + // TODO: Handle named params like "ORDER BY" + !param.name && + (!param.optional || param.must_be.length === 1) + ) { + ctrl.addArg(param.must_be[0]); + } + }); } - this.getUiDefault = function(index) { - if (ctrl.fn.params[0].ui_defaults) { - return ctrl.fn.params[0].ui_defaults[index] || _.last(ctrl.fn.params[0].ui_defaults); + this.getParam = function(index) { + return ctrl.fn.params[index] || _.last(ctrl.fn.params); + }; + + this.canAddArg = function() { + if (!ctrl.fn) { + return false; + } + var param = ctrl.getParam(ctrl.args.length), + index = ctrl.fn.params.indexOf(param); + // TODO: Handle named params like "ORDER BY" + if (param.name) { + return false; } - defaultUiDefaults.type = ctrl.fn.params[0].must_be[0]; - return defaultUiDefaults; + return ctrl.args.length - index < param.max_expr; }; // On-demand options for dropdown function selector diff --git a/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.html b/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.html index 67c9b6c980..89bba89a0a 100644 --- a/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.html +++ b/ext/search_kit/ang/crmSearchAdmin/crmSearchFunction.html @@ -7,17 +7,17 @@
- - - + + +
-
+
diff --git a/tests/phpunit/api/v4/Action/SqlFunctionTest.php b/tests/phpunit/api/v4/Action/SqlFunctionTest.php index 9101fad3e4..59300c3692 100644 --- a/tests/phpunit/api/v4/Action/SqlFunctionTest.php +++ b/tests/phpunit/api/v4/Action/SqlFunctionTest.php @@ -193,7 +193,7 @@ class SqlFunctionTest extends UnitTestCase { public function testStringFunctions() { $sampleData = [ - ['first_name' => 'abc', 'middle_name' => 'q', 'last_name' => 'tester1', 'source' => '123'], + ['first_name' => 'abc', 'middle_name' => 'Q', 'last_name' => 'tester1', 'source' => '123'], ]; $cid = Contact::save(FALSE) ->setRecords($sampleData) @@ -202,9 +202,15 @@ class SqlFunctionTest extends UnitTestCase { $result = Contact::get(FALSE) ->addWhere('id', '=', $cid) ->addSelect('CONCAT_WS("|", first_name, middle_name, last_name) AS concat_ws') + ->addSelect('REPLACE(first_name, "c", "cdef") AS new_first') + ->addSelect('UPPER(first_name)') + ->addSelect('LOWER(middle_name)') ->execute()->first(); - $this->assertEquals('abc|q|tester1', $result['concat_ws']); + $this->assertEquals('abc|Q|tester1', $result['concat_ws']); + $this->assertEquals('abcdef', $result['new_first']); + $this->assertEquals('ABC', $result['UPPER:first_name']); + $this->assertEquals('q', $result['LOWER:middle_name']); } public function testIncorrectNumberOfArguments() { @@ -215,7 +221,7 @@ class SqlFunctionTest extends UnitTestCase { $this->fail('Api should have thrown exception'); } catch (\API_Exception $e) { - $this->assertEquals('Incorrect number of arguments for SQL function IF', $e->getMessage()); + $this->assertEquals('Missing param 2 for SQL function IF', $e->getMessage()); } try { @@ -225,7 +231,17 @@ class SqlFunctionTest extends UnitTestCase { $this->fail('Api should have thrown exception'); } catch (\API_Exception $e) { - $this->assertEquals('Incorrect number of arguments for SQL function NULLIF', $e->getMessage()); + $this->assertEquals('Too many arguments given for SQL function NULLIF', $e->getMessage()); + } + + try { + Activity::get(FALSE) + ->addSelect('CONCAT_WS(",", ) AS whoops') + ->execute(); + $this->fail('Api should have thrown exception'); + } + catch (\API_Exception $e) { + $this->assertEquals('Too few arguments to param 2 for SQL function CONCAT_WS', $e->getMessage()); } } -- 2.25.1