From 551edbb9a608d1228fe17f134ac537b04ff6fa89 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Tue, 7 Mar 2023 22:09:32 +0000 Subject: [PATCH] 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 | 3 ++ reportbuilder/classes/local/filters/date.php | 32 +++++++++--------- .../classes/local/filters/number.php | 6 ++-- reportbuilder/classes/local/report/filter.php | 33 +++++++++++++++++++ .../tests/local/report/filter_test.php | 12 +++++++ reportbuilder/upgrade.txt | 2 ++ 6 files changed, 70 insertions(+), 18 deletions(-) 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/date.php b/reportbuilder/classes/local/filters/date.php index 696625b1201..f4f618cd4a4 100644 --- a/reportbuilder/classes/local/filters/date.php +++ b/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], diff --git a/reportbuilder/classes/local/filters/number.php b/reportbuilder/classes/local/filters/number.php index ae0efa82258..482bd737c3f 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/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/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 d7e36ec9179..5585c2881a1 100644 --- a/reportbuilder/upgrade.txt +++ b/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 ===