diff --git a/reportbuilder/classes/local/filters/base.php b/reportbuilder/classes/local/filters/base.php index 5316a656913..86dac541fa2 100644 --- a/reportbuilder/classes/local/filters/base.php +++ b/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]] */ diff --git a/reportbuilder/classes/local/filters/category.php b/reportbuilder/classes/local/filters/category.php index 1cb4552f024..578cfc4ca5e 100644 --- a/reportbuilder/classes/local/filters/category.php +++ b/reportbuilder/classes/local/filters/category.php @@ -61,8 +61,7 @@ class category extends base { public function get_sql_filter(array $values): array { global $DB; - $fieldsql = $this->filter->get_field_sql(); - $params = $this->filter->get_field_params(); + [$fieldsql, $params] = $this->filter->get_field_sql_and_params(); $category = (int) ($values["{$this->name}_value"] ?? 0); $subcategories = !empty($values["{$this->name}_subcategories"]); @@ -79,6 +78,11 @@ class category extends base { // Sub-category matching on path of selected category. if ($subcategories) { + + // We need to re-use the original filter SQL here, while ensuring parameter uniqueness is preserved. + [$fieldsql, $params1] = $this->filter->get_field_sql_and_params(1); + $params = array_merge($params, $params1); + $paramcategorypath = database::generate_param_name(); $params[$paramcategorypath] = "%/{$category}/%"; $sql .= " OR {$fieldsql} IN ( diff --git a/reportbuilder/classes/local/filters/date.php b/reportbuilder/classes/local/filters/date.php index 59a32bdedac..5c9a8b6a38d 100644 --- a/reportbuilder/classes/local/filters/date.php +++ b/reportbuilder/classes/local/filters/date.php @@ -175,30 +175,31 @@ 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, $paramdateto] = database::generate_param_names(2); + + 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; case self::DATE_BEFORE: $param = database::generate_param_name(); @@ -219,7 +220,7 @@ class date extends base { // Generate parameters and SQL clause for the relative date comparison. [$paramdatefrom, $paramdateto] = database::generate_param_names(2); - $sql = "{$fieldsql} >= :{$paramdatefrom} AND {$fieldsql} <= :{$paramdateto}"; + $sql = "{$fieldsql} BETWEEN :{$paramdatefrom} AND :{$paramdateto}"; [ $params[$paramdatefrom], diff --git a/reportbuilder/classes/local/filters/number.php b/reportbuilder/classes/local/filters/number.php index 034c2792c8c..f884b659915 100644 --- a/reportbuilder/classes/local/filters/number.php +++ b/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; diff --git a/reportbuilder/classes/local/helpers/database.php b/reportbuilder/classes/local/helpers/database.php index de250176270..a19694c12b7 100644 --- a/reportbuilder/classes/local/helpers/database.php +++ b/reportbuilder/classes/local/helpers/database.php @@ -101,6 +101,29 @@ class database { return true; } + /** + * Replace parameter names within given SQL expression, allowing caller to specify callback to handle their replacement + * primarily to ensure uniqueness when the expression is to be used as part of a larger query + * + * @param string $sql + * @param array $params + * @param callable $callback Method that takes a single string parameter, and returns another string + * @return string + */ + public static function sql_replace_parameter_names(string $sql, array $params, callable $callback): string { + foreach ($params as $param) { + + // Pattern to look for param within the SQL. + $pattern = '/:(?' . preg_quote($param) . ')\b/'; + + $sql = preg_replace_callback($pattern, function(array $matches) use ($callback): string { + return ':' . $callback($matches['param']); + }, $sql); + } + + return $sql; + } + /** * Generate SQL expression for sorting group concatenated fields * diff --git a/reportbuilder/classes/local/report/column.php b/reportbuilder/classes/local/report/column.php index 3db57db8ba5..05c64ef49f1 100644 --- a/reportbuilder/classes/local/report/column.php +++ b/reportbuilder/classes/local/report/column.php @@ -367,12 +367,12 @@ final class column { $fields = []; foreach ($this->fields as $alias => $sql) { - // Ensure params within SQL are prefixed with column index. - foreach ($this->params as $name => $value) { - $sql = preg_replace_callback('/:(?' . preg_quote($name, '\b/') . ')/', function(array $matches): string { - return ':' . $this->unique_param_name($matches['param']); - }, $sql); - } + + // Ensure parameter names within SQL are prefixed with column index. + $params = array_keys($this->params); + $sql = database::sql_replace_parameter_names($sql, $params, function(string $param): string { + return $this->unique_param_name($param); + }); $fields[$alias] = [ 'sql' => $sql, diff --git a/reportbuilder/classes/local/report/filter.php b/reportbuilder/classes/local/report/filter.php index d45466b70d3..dbd731d778e 100644 --- a/reportbuilder/classes/local/report/filter.php +++ b/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 * diff --git a/reportbuilder/tests/local/filters/category_test.php b/reportbuilder/tests/local/filters/category_test.php index 819d158a03f..06e60ad8b09 100644 --- a/reportbuilder/tests/local/filters/category_test.php +++ b/reportbuilder/tests/local/filters/category_test.php @@ -20,6 +20,7 @@ namespace core_reportbuilder\local\filters; use advanced_testcase; use lang_string; +use core_reportbuilder\local\helpers\database; use core_reportbuilder\local\report\filter; /** @@ -88,4 +89,37 @@ class category_test extends advanced_testcase { $categories = $DB->get_fieldset_select('course_categories', 'name', $select, $params); $this->assertEqualsCanonicalizing($expectedcategories, $categories); } + + /** + * Test getting filter SQL with parameters + */ + public function test_get_sql_filter_parameters(): void { + global $DB; + + $this->resetAfterTest(); + + $category1 = $this->getDataGenerator()->create_category(['name' => 'One']); + $category2 = $this->getDataGenerator()->create_category(['name' => 'Two', 'parent' => $category1->id]); + $category3 = $this->getDataGenerator()->create_category(['name' => 'Three']); + + // Rather convoluted filter SQL, but enough to demonstrate usage of a parameter that gets used twice in the query. + $paramzero = database::generate_param_name(); + $filter = new filter( + category::class, + 'test', + new lang_string('yes'), + 'testentity', + "id + :{$paramzero}", + [$paramzero => 0] + ); + + // When including sub-categories, the filter SQL is included twice (for the category itself, plus to find descendents). + [$select, $params] = category::create($filter)->get_sql_filter([ + $filter->get_unique_identifier() . '_value' => $category1->id, + $filter->get_unique_identifier() . '_subcategories' => true, + ]); + + $categories = $DB->get_fieldset_select('course_categories', 'id', $select, $params); + $this->assertEqualsCanonicalizing([$category1->id, $category2->id], $categories); + } } diff --git a/reportbuilder/tests/local/helpers/database_test.php b/reportbuilder/tests/local/helpers/database_test.php index 5dc496b5d51..22df8614a49 100644 --- a/reportbuilder/tests/local/helpers/database_test.php +++ b/reportbuilder/tests/local/helpers/database_test.php @@ -139,4 +139,31 @@ class database_test extends advanced_testcase { $record = $DB->get_record_sql($sql, $params); $this->assertEquals($admin->id, $record->{$userfieldalias}); } + + /** + * Test replacement of parameter names within SQL statements + */ + public function test_sql_replace_parameter_names(): 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 = database::sql_replace_parameter_names($sql, [$param0, $param1, $param10], static function(string $param): string { + return "prefix_{$param}"; + }); + + $record = $DB->get_record_sql($sql, [ + "prefix_{$param0}" => 'Zero', + "prefix_{$param1}" => 'One', + "prefix_{$param10}" => 'Ten', + ]); + + $this->assertEquals((object) [ + 'field0' => 'Zero', + 'field1' => 'One', + 'field10' => 'Ten', + ], $record); + } } diff --git a/reportbuilder/tests/local/report/column_test.php b/reportbuilder/tests/local/report/column_test.php index c9483005f3a..3b29dba4dc1 100644 --- a/reportbuilder/tests/local/report/column_test.php +++ b/reportbuilder/tests/local/report/column_test.php @@ -173,19 +173,31 @@ class column_test extends advanced_testcase { * Test adding params to field, and retrieving them */ public function test_add_field_with_params(): void { - $param = database::generate_param_name(); + [$param0, $param1] = database::generate_param_names(2); $column = $this->create_column('test') ->set_index(1) - ->add_field(":{$param}", 'foo', [$param => 'bar']); + ->add_field(":{$param0}", 'foo', [$param0 => 'foo']) + ->add_field(":{$param1}", 'bar', [$param1 => 'bar']); // Select will look like the following: "p_rbparam", where index is the column index and counter is // a static value of the report helper class. - $select = $column->get_fields(); - preg_match('/:(?p1_rbparam[\d]+) AS c1_foo/', $select[0], $matches); + $fields = $column->get_fields(); + $this->assertCount(2, $fields); + preg_match('/:(?p1_rbparam[\d]+) AS c1_foo/', $fields[0], $matches); $this->assertArrayHasKey('paramname', $matches); - $this->assertEquals([$matches['paramname'] => 'bar'], $column->get_params()); + $fieldparam0 = $matches['paramname']; + + preg_match('/:(?p1_rbparam[\d]+) AS c1_bar/', $fields[1], $matches); + $this->assertArrayHasKey('paramname', $matches); + $fieldparam1 = $matches['paramname']; + + // Ensure column parameters have been renamed appropriately. + $this->assertEquals([ + $fieldparam0 => 'foo', + $fieldparam1 => 'bar', + ], $column->get_params()); } /** diff --git a/reportbuilder/tests/local/report/filter_test.php b/reportbuilder/tests/local/report/filter_test.php index ed180b5d5a6..299d599a11a 100644 --- a/reportbuilder/tests/local/report/filter_test.php +++ b/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 */ diff --git a/reportbuilder/upgrade.txt b/reportbuilder/upgrade.txt index 8414a8df627..6cd884281f6 100644 --- a/reportbuilder/upgrade.txt +++ b/reportbuilder/upgrade.txt @@ -7,6 +7,10 @@ Information provided here is intended especially for developers. * Column callbacks are now passed a fourth argument to indicate the aggregation type currently being applied, which allows for columns to define how the aggregated data is displayed * New methods `[add|get]_attributes` added to report base class, for including custom attributes in report container HTML +* 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 * The following attributes can be added to custom reports in order to control card view display (via the `add_attributes` method): - `data-force-card` to force cards view - `data-force-table` to force table view