APIv4 - Validate number of args passed to SQL functions

diff --git a/Civi/Api4/Query/SqlFunction.php b/Civi/Api4/Query/SqlFunction.php
index c6c4f93e4e041edcbb66d682b8477d727f385d4d..44923c6a5626a83345b6a85620900e28c2b999c4 100644 (file)
--- a/Civi/Api4/Query/SqlFunction.php
+++ b/Civi/Api4/Query/SqlFunction.php
@@ -53,8 +53,12 @@ abstract class SqlFunction extends SqlExpression {
         'expr' => [],
         'suffix' => NULL,
       ];
-      if ($param['expr'] && isset($prefix) || in_array('', $param['prefix']) || !$param['optional']) {
-        $this->args[$idx]['expr'] = $this->captureExpressions($arg, $param['expr'], $param['must_be'], $param['cant_be']);
+      if ($param['max_expr'] && isset($prefix) || in_array('', $param['prefix']) || !$param['optional']) {
+        $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);
       }
     }
@@ -82,13 +86,12 @@ abstract class SqlFunction extends SqlExpression {
    * Shifts 0 or more expressions off the argument string and returns them
    *
    * @param string $arg
-   * @param int $limit
    * @param array $mustBe
    * @param array $cantBe
    * @return array
    * @throws \API_Exception
    */
-  private function captureExpressions(&$arg, $limit, $mustBe, $cantBe) {
+  private function captureExpressions(&$arg, $mustBe, $cantBe) {
     $captured = [];
     $arg = ltrim($arg);
     while ($arg) {
@@ -98,7 +101,7 @@ abstract class SqlFunction extends SqlExpression {
       $this->fields = array_merge($this->fields, $expr->getFields());
       $captured[] = $expr;
       // Keep going if we have a comma indicating another expression follows
-      if (count($captured) < $limit && substr($arg, 0, 1) === ',') {
+      if (substr($arg, 0, 1) === ',') {
         $arg = ltrim(substr($arg, 1));
       }
       else {
@@ -227,7 +230,8 @@ abstract class SqlFunction extends SqlExpression {
       // Merge in defaults to ensure each param has these properties
       $params[] = $param + [
         'prefix' => [],
-        'expr' => 1,
+        'min_expr' => 1,
+        'max_expr' => 1,
         'suffix' => [],
         'optional' => FALSE,
         'must_be' => [],

diff --git a/Civi/Api4/Query/SqlFunctionABS.php b/Civi/Api4/Query/SqlFunctionABS.php
index 96d682cd576b140f2c7d3c63043d54cace49d4c3..a5909ec8181e2bb9dcc8bc6b02ff4eb67cc2eb5c 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionABS.php
+++ b/Civi/Api4/Query/SqlFunctionABS.php
@@ -20,7 +20,6 @@ class SqlFunctionABS extends SqlFunction {
 
   protected static $params = [
     [
-      'expr' => 1,
       'optional' => FALSE,
       'must_be' => ['SqlField', 'SqlNumber'],
     ],

diff --git a/Civi/Api4/Query/SqlFunctionAVG.php b/Civi/Api4/Query/SqlFunctionAVG.php
index 577f05afc7a6c70499f47e7a2e91cd0580e094ed..a9787d2e96bee47825d26cf64a55cd90a0ab5179 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionAVG.php
+++ b/Civi/Api4/Query/SqlFunctionAVG.php
@@ -21,7 +21,6 @@ class SqlFunctionAVG extends SqlFunction {
   protected static $params = [
     [
       'prefix' => ['', 'DISTINCT', 'ALL'],
-      'expr' => 1,
       'must_be' => ['SqlField'],
     ],
   ];

diff --git a/Civi/Api4/Query/SqlFunctionCOALESCE.php b/Civi/Api4/Query/SqlFunctionCOALESCE.php
index 1173106e9c7f44ba8d87766b1a8297846143855e..324d62fecf761c4ded7078729b54111141ab6281 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionCOALESCE.php
+++ b/Civi/Api4/Query/SqlFunctionCOALESCE.php
@@ -20,7 +20,7 @@ class SqlFunctionCOALESCE extends SqlFunction {
 
   protected static $params = [
     [
-      'expr' => 99,
+      'max_expr' => 99,
       'optional' => FALSE,
     ],
   ];

diff --git a/Civi/Api4/Query/SqlFunctionCONCAT.php b/Civi/Api4/Query/SqlFunctionCONCAT.php
index 5dc63348a181c05888e4d088c416e2715b37f346..fa06754078ec50a5dfbc6611a1f13ca89c9cf66f 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionCONCAT.php
+++ b/Civi/Api4/Query/SqlFunctionCONCAT.php
@@ -20,7 +20,7 @@ class SqlFunctionCONCAT extends SqlFunction {
 
   protected static $params = [
     [
-      'expr' => 99,
+      'max_expr' => 99,
       'optional' => FALSE,
       'must_be' => ['SqlField', 'SqlString'],
     ],

diff --git a/Civi/Api4/Query/SqlFunctionCOUNT.php b/Civi/Api4/Query/SqlFunctionCOUNT.php
index 7a9cda17217a8c844d422043d1fe180ef28db05a..8d52c9c73393fe0aac2e66ba4b799e9e145f956a 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionCOUNT.php
+++ b/Civi/Api4/Query/SqlFunctionCOUNT.php
@@ -21,7 +21,7 @@ class SqlFunctionCOUNT extends SqlFunction {
   protected static $params = [
     [
       'prefix' => ['', 'DISTINCT', 'ALL'],
-      'expr' => 1,
+      'max_expr' => 1,
       'must_be' => ['SqlField', 'SqlWild'],
       'cant_be' => [],
     ],

diff --git a/Civi/Api4/Query/SqlFunctionGREATEST.php b/Civi/Api4/Query/SqlFunctionGREATEST.php
index 5ce1b496a78ea00aae4b7140c5a5716b9695f776..47212da6bbe45dc2d44aa4ca40bb5a5953352497 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionGREATEST.php
+++ b/Civi/Api4/Query/SqlFunctionGREATEST.php
@@ -22,7 +22,7 @@ class SqlFunctionGREATEST extends SqlFunction {
 
   protected static $params = [
     [
-      'expr' => 99,
+      'max_expr' => 99,
       'optional' => FALSE,
     ],
   ];

diff --git a/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php b/Civi/Api4/Query/SqlFunctionGROUP_CONCAT.php
index 0e0f7525b8bad3084a8a92aa9c6e80da1fd2557d..a570bfaf3f8b437b7979abb397173d696338113e 100644 (file)
--- 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 $params = [
     [
       'prefix' => ['', 'DISTINCT', 'ALL'],
-      'expr' => 1,
+      'max_expr' => 1,
       'must_be' => ['SqlField', 'sqlFunction'],
       'optional' => FALSE,
     ],
     [
       'prefix' => ['ORDER BY'],
-      'expr' => 1,
+      'max_expr' => 1,
       'suffix' => ['', 'ASC', 'DESC'],
       'must_be' => ['SqlField'],
       'optional' => TRUE,
     ],
     [
       'prefix' => ['SEPARATOR'],
-      'expr' => 1,
+      'max_expr' => 1,
       'must_be' => ['SqlString'],
       'optional' => TRUE,
       // @see self::formatOutput()

diff --git a/Civi/Api4/Query/SqlFunctionIF.php b/Civi/Api4/Query/SqlFunctionIF.php
index 39f688a163e5ae5b14f6ed6df76ee9a33980a9c8..4a1e244c7dba77a97e98cec89bb38e4bdfa2942d 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionIF.php
+++ b/Civi/Api4/Query/SqlFunctionIF.php
@@ -20,7 +20,8 @@ class SqlFunctionIF extends SqlFunction {
 
   protected static $params = [
     [
-      'expr' => 3,
+      'min_expr' => 3,
+      'max_expr' => 3,
       'optional' => FALSE,
     ],
   ];

diff --git a/Civi/Api4/Query/SqlFunctionISNULL.php b/Civi/Api4/Query/SqlFunctionISNULL.php
index 1568c1114accc34454d65b4524e3f82f44758a5c..7b614a53811148e891ea27440111fc2ccc4b3fec 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionISNULL.php
+++ b/Civi/Api4/Query/SqlFunctionISNULL.php
@@ -20,7 +20,6 @@ class SqlFunctionISNULL extends SqlFunction {
 
   protected static $params = [
     [
-      'expr' => 1,
       'optional' => FALSE,
     ],
   ];

diff --git a/Civi/Api4/Query/SqlFunctionLEAST.php b/Civi/Api4/Query/SqlFunctionLEAST.php
index b0f315a98822c8423be15c632cdf8d5b62450040..89aa121e8cf80f217e012e060f23c9c589328fa1 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionLEAST.php
+++ b/Civi/Api4/Query/SqlFunctionLEAST.php
@@ -22,7 +22,7 @@ class SqlFunctionLEAST extends SqlFunction {
 
   protected static $params = [
     [
-      'expr' => 99,
+      'max_expr' => 99,
       'optional' => FALSE,
     ],
   ];

diff --git a/Civi/Api4/Query/SqlFunctionLOWER.php b/Civi/Api4/Query/SqlFunctionLOWER.php
index 2a6b9686f5f20408fbebccb1bd865e32bccf996f..0dc052123f025c5f25e18b3d288b4e1239005716 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionLOWER.php
+++ b/Civi/Api4/Query/SqlFunctionLOWER.php
@@ -20,7 +20,6 @@ class SqlFunctionLOWER extends SqlFunction {
 
   protected static $params = [
     [
-      'expr' => 1,
       'optional' => FALSE,
       'must_be' => ['SqlField', 'SqlString'],
     ],

diff --git a/Civi/Api4/Query/SqlFunctionMAX.php b/Civi/Api4/Query/SqlFunctionMAX.php
index c783d2cab712332f205ef60704364a691c135678..f26a2c0aa58ca93665ba50870a37e52dd4970618 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionMAX.php
+++ b/Civi/Api4/Query/SqlFunctionMAX.php
@@ -23,7 +23,6 @@ class SqlFunctionMAX extends SqlFunction {
   protected static $params = [
     [
       'prefix' => ['', 'DISTINCT', 'ALL'],
-      'expr' => 1,
       'must_be' => ['SqlField'],
     ],
   ];

diff --git a/Civi/Api4/Query/SqlFunctionMIN.php b/Civi/Api4/Query/SqlFunctionMIN.php
index f5fe4e86bd5793158142d0817b6796da128ce8da..756aeb03921dee2b55217201d49bde0d45280305 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionMIN.php
+++ b/Civi/Api4/Query/SqlFunctionMIN.php
@@ -23,7 +23,6 @@ class SqlFunctionMIN extends SqlFunction {
   protected static $params = [
     [
       'prefix' => ['', 'DISTINCT', 'ALL'],
-      'expr' => 1,
       'must_be' => ['SqlField'],
     ],
   ];

diff --git a/Civi/Api4/Query/SqlFunctionNULLIF.php b/Civi/Api4/Query/SqlFunctionNULLIF.php
index 0286a46056922bcc97ccb59b27b1374a8a589ac6..f5602cd2cda577b0d6e5e8a8481160b4a95cc735 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionNULLIF.php
+++ b/Civi/Api4/Query/SqlFunctionNULLIF.php
@@ -22,7 +22,8 @@ class SqlFunctionNULLIF extends SqlFunction {
 
   protected static $params = [
     [
-      'expr' => 2,
+      'min_expr' => 2,
+      'max_expr' => 2,
       'optional' => FALSE,
     ],
   ];

diff --git a/Civi/Api4/Query/SqlFunctionREPLACE.php b/Civi/Api4/Query/SqlFunctionREPLACE.php
index d1cdef78e5d72c0d739148b570f7a5d87119b899..b43a7ab80246673e2c2c9b76fbfb735bdb9b09b3 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionREPLACE.php
+++ b/Civi/Api4/Query/SqlFunctionREPLACE.php
@@ -20,17 +20,14 @@ class SqlFunctionREPLACE extends SqlFunction {
 
   protected static $params = [
     [
-      'expr' => 1,
       'optional' => FALSE,
       'must_be' => ['SqlField', 'SqlString'],
     ],
     [
-      'expr' => 1,
       'optional' => FALSE,
       'must_be' => ['SqlField', 'SqlString'],
     ],
     [
-      'expr' => 1,
       'optional' => FALSE,
       'must_be' => ['SqlField', 'SqlString'],
     ],

diff --git a/Civi/Api4/Query/SqlFunctionROUND.php b/Civi/Api4/Query/SqlFunctionROUND.php
index 4d5b2f084d2b236755bcccf117b7502f7b5a8bc3..28ee78caab014404bebd1f4150ec03844104c97f 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionROUND.php
+++ b/Civi/Api4/Query/SqlFunctionROUND.php
@@ -20,12 +20,10 @@ class SqlFunctionROUND extends SqlFunction {
 
   protected static $params = [
     [
-      'expr' => 1,
       'optional' => FALSE,
       'must_be' => ['SqlField', 'SqlNumber'],
     ],
     [
-      'expr' => 1,
       'optional' => TRUE,
       'must_be' => ['SqlNumber'],
     ],

diff --git a/Civi/Api4/Query/SqlFunctionSUM.php b/Civi/Api4/Query/SqlFunctionSUM.php
index b6b74ae786b3b58cb001d2d4880eae3cb90a43a2..81b49005f6de3269301138031a6a6c97d71a39b2 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionSUM.php
+++ b/Civi/Api4/Query/SqlFunctionSUM.php
@@ -21,7 +21,6 @@ class SqlFunctionSUM extends SqlFunction {
   protected static $params = [
     [
       'prefix' => ['', 'DISTINCT', 'ALL'],
-      'expr' => 1,
       'must_be' => ['SqlField'],
     ],
   ];

diff --git a/Civi/Api4/Query/SqlFunctionUPPER.php b/Civi/Api4/Query/SqlFunctionUPPER.php
index b9a714e2d5af53ac784fab8707cdfa5f06f01d31..ed4590e290771671e8b28942563f056579c38f78 100644 (file)
--- a/Civi/Api4/Query/SqlFunctionUPPER.php
+++ b/Civi/Api4/Query/SqlFunctionUPPER.php
@@ -20,7 +20,6 @@ class SqlFunctionUPPER extends SqlFunction {
 
   protected static $params = [
     [
-      'expr' => 1,
       'optional' => FALSE,
       'must_be' => ['SqlField', 'SqlString'],
     ],

diff --git a/tests/phpunit/api/v4/Action/SqlFunctionTest.php b/tests/phpunit/api/v4/Action/SqlFunctionTest.php
index 72560a1d07ed9d6e23551647cc045eac11d0b98f..078b9eeaa82af562847b5659a48312a3e052c743 100644 (file)
--- a/tests/phpunit/api/v4/Action/SqlFunctionTest.php
+++ b/tests/phpunit/api/v4/Action/SqlFunctionTest.php
@@ -34,7 +34,8 @@ class SqlFunctionTest extends UnitTestCase {
     $this->assertArrayHasKey('SUM', $functions);
     $this->assertArrayNotHasKey('', $functions);
     $this->assertArrayNotHasKey('SqlFunction', $functions);
-    $this->assertEquals(1, $functions['MAX']['params'][0]['expr']);
+    $this->assertEquals(1, $functions['MAX']['params'][0]['min_expr']);
+    $this->assertEquals(1, $functions['MAX']['params'][0]['max_expr']);
   }
 
   public function testGroupAggregates() {
@@ -176,4 +177,26 @@ class SqlFunctionTest extends UnitTestCase {
     $this->assertEquals(FALSE, $result[$aids[2]]['duration_isnull']);
   }
 
+  public function testIncorrectNumberOfArguments() {
+    try {
+      Activity::get(FALSE)
+        ->addSelect('IF(is_deleted) AS whoops')
+        ->execute();
+      $this->fail('Api should have thrown exception');
+    }
+    catch (\API_Exception $e) {
+      $this->assertEquals('Incorrect number of arguments for SQL function IF', $e->getMessage());
+    }
+
+    try {
+      Activity::get(FALSE)
+        ->addSelect('NULLIF(is_deleted, 1, 2) AS whoops')
+        ->execute();
+      $this->fail('Api should have thrown exception');
+    }
+    catch (\API_Exception $e) {
+      $this->assertEquals('Incorrect number of arguments for SQL function NULLIF', $e->getMessage());
+    }
+  }
+
 }