MDL-81434 reportbuilder: ensure filter/condition parameter uniqueness.

This change fixes an edge case that could be triggered by creating a
custom report that contained a filter instance that was active as both
a filter and condition, where the filter instance provides parameters
to it's SQL fragment.

reportbuilder/classes/local/helpers/database.php

@@ -106,7 +106,7 @@ class database {
      * primarily to ensure uniqueness when the expression is to be used as part of a larger query
      *
      * @param string $sql
-     * @param array $params
+     * @param array $params Parameter names
      * @param callable $callback Method that takes a single string parameter, and returns another string
      * @return string
      */
@@ -124,6 +124,27 @@ class database {
         return $sql;
     }
 
+    /**
+     * Replace parameter names within given SQL expression, returning updated SQL and parameter elements
+     *
+     * {@see sql_replace_parameter_names}
+     *
+     * @param string $sql
+     * @param array $params Parameter name/values
+     * @param callable $callback
+     * @return array [$sql, $params]
+     */
+    public static function sql_replace_parameters(string $sql, array $params, callable $callback): array {
+        $transformedsql = static::sql_replace_parameter_names($sql, array_keys($params), $callback);
+
+        $transformedparams = [];
+        foreach ($params as $name => $value) {
+            $transformedparams[$callback($name)] = $value;
+        }
+
+        return [$transformedsql, $transformedparams];
+    }
+
     /**
      * Generate SQL expression for sorting group concatenated fields
      *

reportbuilder/classes/table/base_report_table.php

@@ -74,10 +74,13 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl
             $wheres[] = $where;
         }
 
+        // Track the index of conditions/filters as we iterate over them.
+        $conditionindex = $filterindex = 0;
+
         // For each condition, we need to ensure their values are always accounted for in the report.
         $conditionvalues = $this->report->get_condition_values();
         foreach ($this->report->get_active_conditions() as $condition) {
-            [$conditionsql, $conditionparams] = $this->get_filter_sql($condition, $conditionvalues);
+            [$conditionsql, $conditionparams] = $this->get_filter_sql($condition, $conditionvalues, 'c' . $conditionindex++);
             if ($conditionsql !== '') {
                 $joins = array_merge($joins, $condition->get_joins());
                 $wheres[] = "({$conditionsql})";
@@ -89,7 +92,7 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl
         if (!$this->editing) {
             $filtervalues = $this->report->get_filter_values();
             foreach ($this->report->get_active_filters() as $filter) {
-                [$filtersql, $filterparams] = $this->get_filter_sql($filter, $filtervalues);
+                [$filtersql, $filterparams] = $this->get_filter_sql($filter, $filtervalues, 'f' . $filterindex++);
                 if ($filtersql !== '') {
                     $joins = array_merge($joins, $filter->get_joins());
                     $wheres[] = "({$filtersql})";
@@ -139,13 +142,24 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl
      *
      * @param filter $filter
      * @param array $filtervalues
+     * @param string $paramprefix
      * @return array [$sql, $params]
      */
-    private function get_filter_sql(filter $filter, array $filtervalues): array {
+    private function get_filter_sql(filter $filter, array $filtervalues, string $paramprefix): array {
         /** @var base $filterclass */
         $filterclass = $filter->get_filter_class();
 
-        return $filterclass::create($filter)->get_sql_filter($filtervalues);
+        // Retrieve SQL fragments from the filter instance, process parameters if required.
+        [$sql, $params] = $filterclass::create($filter)->get_sql_filter($filtervalues);
+        if ($paramprefix !== '' && count($params) > 0) {
+            return database::sql_replace_parameters(
+                $sql,
+                $params,
+                fn(string $param) => "{$paramprefix}_{$param}",
+            );
+        }
+
+        return [$sql, $params];
     }
 
     /**

reportbuilder/tests/local/helpers/database_test.php

@@ -30,7 +30,7 @@ use core_user;
  * @copyright   2020 Paul Holden <[email protected]>
  * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
-class database_test extends advanced_testcase {
+final class database_test extends advanced_testcase {
 
     /**
      * Test generating alias
@@ -150,9 +150,11 @@ class database_test extends advanced_testcase {
         [$param0, $param1, $param10] = ['rbparam0', 'rbparam1', 'rbparam10'];
 
         $sql = "SELECT :{$param0} AS field0, :{$param1} AS field1, :{$param10} AS field10" . $DB->sql_null_from_clause();
-        $sql = database::sql_replace_parameter_names($sql, [$param0, $param1, $param10], static function(string $param): string {
-            return "prefix_{$param}";
-        });
+        $sql = database::sql_replace_parameter_names(
+            $sql,
+            [$param0, $param1, $param10],
+            fn(string $param) => "prefix_{$param}",
+        );
 
         $record = $DB->get_record_sql($sql, [
             "prefix_{$param0}" => 'Zero',
@@ -166,4 +168,29 @@ class database_test extends advanced_testcase {
             'field10' => 'Ten',
         ], $record);
     }
+
+    /**
+     * Test replacement of parameter names within query, returning both modified query and parameters
+     */
+    public function test_sql_replace_parameters(): void {
+        global $DB;
+
+        // Predefine parameter names, to ensure they don't overwrite each other.
+        [$param0, $param1, $param10] = ['rbparam0', 'rbparam1', 'rbparam10'];
+
+        $sql = "SELECT :{$param0} AS field0, :{$param1} AS field1, :{$param10} AS field10" . $DB->sql_null_from_clause();
+        [$sql, $params] = database::sql_replace_parameters(
+            $sql,
+            [$param0 => 'Zero', $param1 => 'One', $param10 => 'Ten'],
+            fn(string $param) => "prefix_{$param}",
+        );
+
+        $record = $DB->get_record_sql($sql, $params);
+
+        $this->assertEquals((object) [
+            'field0' => 'Zero',
+            'field1' => 'One',
+            'field10' => 'Ten',
+        ], $record);
+    }
 }

reportbuilder/upgrade.txt

@@ -1,6 +1,10 @@
 This file describes API changes in /reportbuilder/*
 Information provided here is intended especially for developers.
 
+=== 4.3.5 ===
+
+* New database helper method `sql_replace_parameters` to help ensure uniqueness of parameters within a SQL expression
+
 === 4.3.4 ===
 
 * The `get_name` method has been moved to the base report class and can now be implemented for both custom and system reports, it