MDL-77555 reportbuilder: improve SQL generation within filters.

Use native ANSI SQL syntax for numeric comparisons where possible,
define filter API for the case where filters must re-use the given
field SQL while ensuring uniqueness of any field parameters.

reportbuilder/classes/local/filters/base.php

@@ -102,6 +102,9 @@ abstract class base {
     /**
      * Returns the filter clauses to be used with SQL where
      *
+     * Ideally the field SQL should be included only once in the returned expression, however if that is unavoidable then
+     * use the {@see filter::get_field_sql_and_params} helper to ensure uniqueness of any parameters included within
+     *
      * @param array $values
      * @return array [$sql, [...$params]]
      */

reportbuilder/classes/local/filters/date.php

@@ -164,30 +164,32 @@ class date extends base {
 
         switch ($operator) {
             case self::DATE_NOT_EMPTY:
-                $sql = "{$fieldsql} IS NOT NULL AND {$fieldsql} <> 0";
+                $sql = "COALESCE({$fieldsql}, 0) <> 0";
                 break;
             case self::DATE_EMPTY:
-                $sql = "{$fieldsql} IS NULL OR {$fieldsql} = 0";
+                $sql = "COALESCE({$fieldsql}, 0) = 0";
                 break;
             case self::DATE_RANGE:
-                $clauses = [];
+                $sql = '';
 
                 $datefrom = (int)($values["{$this->name}_from"] ?? 0);
-                if ($datefrom > 0) {
-                    $paramdatefrom = database::generate_param_name();
-                    $clauses[] = "{$fieldsql} >= :{$paramdatefrom}";
-                    $params[$paramdatefrom] = $datefrom;
-                }
-
                 $dateto = (int)($values["{$this->name}_to"] ?? 0);
-                if ($dateto > 0) {
-                    $paramdateto = database::generate_param_name();
-                    $clauses[] = "{$fieldsql} < :{$paramdateto}";
+
+                $paramdatefrom = database::generate_param_name();
+                $paramdateto = database::generate_param_name();
+
+                if ($datefrom > 0 && $dateto > 0) {
+                    $sql = "{$fieldsql} BETWEEN :{$paramdatefrom} AND :{$paramdateto}";
+                    $params[$paramdatefrom] = $datefrom;
+                    $params[$paramdateto] = $dateto;
+                } else if ($datefrom > 0) {
+                    $sql = "{$fieldsql} >= :{$paramdatefrom}";
+                    $params[$paramdatefrom] = $datefrom;
+                } else if ($dateto > 0) {
+                    $sql = "{$fieldsql} < :{$paramdateto}";
                     $params[$paramdateto] = $dateto;
                 }
 
-                $sql = implode(' AND ', $clauses);
-
                 break;
             // Relative helper method can handle these three cases.
             case self::DATE_LAST:
@@ -202,7 +204,7 @@ class date extends base {
                 $paramdatefrom = database::generate_param_name();
                 $paramdateto = database::generate_param_name();
 
-                $sql = "{$fieldsql} >= :{$paramdatefrom} AND {$fieldsql} <= :{$paramdateto}";
+                $sql = "{$fieldsql} BETWEEN :{$paramdatefrom} AND :{$paramdateto}";
                 [
                     $params[$paramdatefrom],
                     $params[$paramdateto],

reportbuilder/classes/local/filters/number.php

@@ -132,10 +132,10 @@ class number extends base {
             case self::ANY_VALUE:
                 return ['', []];
             case self::IS_NOT_EMPTY:
-                $res = "{$fieldsql} IS NOT NULL AND {$fieldsql} <> 0";
+                $res = "COALESCE({$fieldsql}, 0) <> 0";
                 break;
             case self::IS_EMPTY:
-                $res = "{$fieldsql} IS NULL OR {$fieldsql} = 0";
+                $res = "COALESCE({$fieldsql}, 0) = 0";
                 break;
             case self::LESS_THAN:
                 $res = "{$fieldsql} < :{$param}";
@@ -158,7 +158,7 @@ class number extends base {
                 $params[$param] = $value1;
                 break;
             case self::RANGE:
-                $res = "({$fieldsql} >= :{$param} AND {$fieldsql} <= :{$param2})";
+                $res = "{$fieldsql} BETWEEN :{$param} AND :{$param2}";
                 $params[$param] = $value1;
                 $params[$param2] = $value2;
                 break;

reportbuilder/classes/local/report/filter.php

@@ -21,6 +21,7 @@ namespace core_reportbuilder\local\report;
 use lang_string;
 use moodle_exception;
 use core_reportbuilder\local\filters\base;
+use core_reportbuilder\local\helpers\database;
 use core_reportbuilder\local\models\filter as filter_model;
 
 /**
@@ -212,6 +213,38 @@ final class filter {
         return $this->fieldparams;
     }
 
+    /**
+     * Retrieve SQL expression and parameters for the field
+     *
+     * @param int $index
+     * @return array [$sql, [...$params]]
+     */
+    public function get_field_sql_and_params(int $index = 0): array {
+        $fieldsql = $this->get_field_sql();
+        $fieldparams = $this->get_field_params();
+
+        // Shortcut if there aren't any parameters.
+        if (empty($fieldparams)) {
+            return [$fieldsql, $fieldparams];
+        }
+
+        // Simple callback for replacement of parameter names within filter SQL.
+        $transform = function(string $param) use ($index): string {
+            return "{$param}_{$index}";
+        };
+
+        $paramnames = array_keys($fieldparams);
+        $sql = database::sql_replace_parameter_names($fieldsql, $paramnames, $transform);
+
+        $params = [];
+        foreach ($paramnames as $paramname) {
+            $paramnametransform = $transform($paramname);
+            $params[$paramnametransform] = $fieldparams[$paramname];
+        }
+
+        return [$sql, $params];
+    }
+
     /**
      * Set the SQL expression for the field that is being filtered. It will be passed to the filter class
      *

reportbuilder/tests/local/report/filter_test.php

@@ -108,6 +108,18 @@ class filter_test extends advanced_testcase {
         $this->assertEquals(['foo' => 'bar'], $filter->get_field_params());
     }
 
+    /**
+     * Test getting field SQL and params, while providing index for uniqueness
+     */
+    public function test_get_field_sql_and_params(): void {
+        $filter = $this->create_filter('username', 'u.username = :username AND u.idnumber = :idnumber',
+            ['username' => 'test', 'idnumber' => 'bar']);
+
+        [$sql, $params] = $filter->get_field_sql_and_params(1);
+        $this->assertEquals('u.username = :username_1 AND u.idnumber = :idnumber_1', $sql);
+        $this->assertEquals(['username_1' => 'test', 'idnumber_1' => 'bar'], $params);
+    }
+
     /**
      * Test adding single join
      */

reportbuilder/upgrade.txt

@@ -5,6 +5,8 @@ Information provided here is intended especially for developers.
 
 * New database helper method `sql_replace_parameter_names` to help ensure uniqueness of parameters within an expression (where
   that expression can be used multiple times as part of a larger query)
+* The local report filter class has a new `get_field_sql_and_params` method which should be used by filter types that re-use
+  the filter field SQL within their generated expression, to ensure SQL containing parameters works correctly
 
 === 4.0.7 ===