From f2cbacbd2403967672850af0da45f4544e6d2868 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Tue, 7 Mar 2023 16:48:51 +0000 Subject: [PATCH] MDL-77555 reportbuilder: method to ensure unique parameters in SQL. --- .../classes/local/helpers/database.php | 23 ++++++++++++++++ reportbuilder/classes/local/report/column.php | 12 ++++----- .../tests/local/helpers/database_test.php | 27 +++++++++++++++++++ .../tests/local/report/column_test.php | 22 +++++++++++---- reportbuilder/upgrade.txt | 5 ++++ 5 files changed, 78 insertions(+), 11 deletions(-) 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 5db3f8dd2c4..b5a344e5aaf 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/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 721b1a7b3b8..35ced84f97a 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/upgrade.txt b/reportbuilder/upgrade.txt index 110cd00adad..c6fabcb1cff 100644 --- a/reportbuilder/upgrade.txt +++ b/reportbuilder/upgrade.txt @@ -1,6 +1,11 @@ This file describes API changes in /reportbuilder/* Information provided here is intended especially for developers. +=== 4.1.3 === + +* 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) + === 4.1.2 === * The schedule helper `create_schedule` method accepts a `$timenow` parameter to use for comparisons against current date