From bc206fdec3688256253d8159b86c8f0943cdef06 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Thu, 10 Mar 2022 17:24:45 +0000 Subject: [PATCH] MDL-74165 reportbuilder: helpers to create multiple params/aliases. --- reportbuilder/classes/local/filters/date.php | 6 +- .../classes/local/filters/number.php | 4 +- .../classes/local/helpers/database.php | 25 +++++ .../reportbuilder/audience/systemrole.php | 14 +-- .../tests/local/helpers/database_test.php | 103 ++++++++++++++---- reportbuilder/upgrade.txt | 5 + 6 files changed, 124 insertions(+), 33 deletions(-) create mode 100644 reportbuilder/upgrade.txt diff --git a/reportbuilder/classes/local/filters/date.php b/reportbuilder/classes/local/filters/date.php index 93771eac2d0..4d292298a8d 100644 --- a/reportbuilder/classes/local/filters/date.php +++ b/reportbuilder/classes/local/filters/date.php @@ -199,10 +199,10 @@ class date extends base { return ['', []]; } - $paramdatefrom = database::generate_param_name(); - $paramdateto = database::generate_param_name(); - + // Generate parameters and SQL clause for the relative date comparison. + [$paramdatefrom, $paramdateto] = database::generate_param_names(2); $sql = "{$fieldsql} >= :{$paramdatefrom} AND {$fieldsql} <= :{$paramdateto}"; + [ $params[$paramdatefrom], $params[$paramdateto], diff --git a/reportbuilder/classes/local/filters/number.php b/reportbuilder/classes/local/filters/number.php index 73eb38983bc..187a7d5756a 100644 --- a/reportbuilder/classes/local/filters/number.php +++ b/reportbuilder/classes/local/filters/number.php @@ -122,8 +122,8 @@ class number extends base { return ['', []]; } - $param = database::generate_param_name(); - $param2 = database::generate_param_name(); + [$param, $param2] = database::generate_param_names(2); + $fieldsql = $this->filter->get_field_sql(); $params = $this->filter->get_field_params(); diff --git a/reportbuilder/classes/local/helpers/database.php b/reportbuilder/classes/local/helpers/database.php index 9bb3d082feb..319918086bd 100644 --- a/reportbuilder/classes/local/helpers/database.php +++ b/reportbuilder/classes/local/helpers/database.php @@ -46,6 +46,19 @@ class database { return static::GENERATE_ALIAS_PREFIX . ($aliascount++); } + + /** + * Generate multiple unique table/column aliases, see {@see generate_alias} for info + * + * @param int $count + * @return string[] + */ + public static function generate_aliases(int $count): array { + return array_map([ + static::class, 'generate_alias' + ], array_fill(0, $count, null)); + } + /** * Generates unique parameter name that must be used in generated SQL * @@ -57,6 +70,18 @@ class database { return static::GENERATE_PARAM_PREFIX . ($paramcount++); } + /** + * Generate multiple unique parameter names, see {@see generate_param_name} for info + * + * @param int $count + * @return string[] + */ + public static function generate_param_names(int $count): array { + return array_map([ + static::class, 'generate_param_name' + ], array_fill(0, $count, null)); + } + /** * Validate that parameter names were generated using {@see generate_param_name}. * diff --git a/reportbuilder/classes/reportbuilder/audience/systemrole.php b/reportbuilder/classes/reportbuilder/audience/systemrole.php index 315aa2d7b43..7dff0bf88c5 100644 --- a/reportbuilder/classes/reportbuilder/audience/systemrole.php +++ b/reportbuilder/classes/reportbuilder/audience/systemrole.php @@ -57,17 +57,17 @@ class systemrole extends base { $prefix = database::generate_param_name() . '_'; [$insql, $inparams] = $DB->get_in_or_equal($roles, SQL_PARAMS_NAMED, $prefix); - $contextid = database::generate_param_name(); - $ra = database::generate_alias(); - $ctx = database::generate_alias(); + // Ensure parameter names and aliases are unique, as the same audience type can be added multiple times to a report. + $paramcontextid = database::generate_param_name(); + [$roleassignments, $context] = database::generate_aliases(2); $join = " - JOIN {role_assignments} {$ra} ON {$ra}.userid = {$usertablealias}.id - JOIN {context} {$ctx} ON {$ctx}.id = {$ra}.contextid"; + JOIN {role_assignments} {$roleassignments} ON {$roleassignments}.userid = {$usertablealias}.id + JOIN {context} {$context} ON {$context}.id = {$roleassignments}.contextid"; - $where = "{$ra}.contextid = :{$contextid} AND {$ra}.roleid {$insql}"; + $where = "{$roleassignments}.contextid = :{$paramcontextid} AND {$roleassignments}.roleid {$insql}"; - return [$join, $where, $inparams + [$contextid => context_system::instance()->id]]; + return [$join, $where, $inparams + [$paramcontextid => context_system::instance()->id]]; } /** diff --git a/reportbuilder/tests/local/helpers/database_test.php b/reportbuilder/tests/local/helpers/database_test.php index 286c3be47ec..5dc496b5d51 100644 --- a/reportbuilder/tests/local/helpers/database_test.php +++ b/reportbuilder/tests/local/helpers/database_test.php @@ -33,41 +33,67 @@ use core_user; class database_test extends advanced_testcase { /** - * Test generating table alias and parameter names + * Test generating alias */ - public function test_generate_alias_params(): void { - global $DB; + public function test_generate_alias(): void { + $this->assertMatchesRegularExpression('/^rbalias(\d+)$/', database::generate_alias()); + } - $admin = core_user::get_user_by_username('admin'); + /** + * Test generating multiple aliases + */ + public function test_generate_aliases(): void { + $aliases = database::generate_aliases(3); - $usertablealias = database::generate_alias(); - $usertablealiasjoin = database::generate_alias(); - $useridalias = database::generate_alias(); - - $paramuserid = database::generate_param_name(); - $paramuserdeleted = database::generate_param_name(); + $this->assertCount(3, $aliases); + [$aliasone, $aliastwo, $aliasthree] = $aliases; // Ensure they are different. - $this->assertNotEquals($usertablealias, $usertablealiasjoin); - $this->assertNotEquals($paramuserid, $paramuserdeleted); + $this->assertNotEquals($aliasone, $aliastwo); + $this->assertNotEquals($aliasone, $aliasthree); + $this->assertNotEquals($aliastwo, $aliasthree); + } - $sql = "SELECT {$usertablealias}.id AS {$useridalias} - FROM {user} {$usertablealias} - JOIN {user} {$usertablealiasjoin} ON {$usertablealiasjoin}.id = {$usertablealias}.id - WHERE {$usertablealias}.id = :{$paramuserid} AND {$usertablealias}.deleted = :{$paramuserdeleted}"; - $params = [$paramuserid => $admin->id, $paramuserdeleted => 0]; + /** + * Test generating parameter name + */ + public function test_generate_param_name(): void { + $this->assertMatchesRegularExpression('/^rbparam(\d+)$/', database::generate_param_name()); + } - $validated = database::validate_params($params); - $this->assertTrue($validated); + /** + * Test generating multiple parameter names + */ + public function test_generate_param_names(): void { + $params = database::generate_param_names(3); - $record = $DB->get_record_sql($sql, $params); - $this->assertEquals($admin->id, $record->{$useridalias}); + $this->assertCount(3, $params); + [$paramone, $paramtwo, $paramthree] = $params; + + // Ensure they are different. + $this->assertNotEquals($paramone, $paramtwo); + $this->assertNotEquals($paramone, $paramthree); + $this->assertNotEquals($paramtwo, $paramthree); } /** * Test parameter validation */ public function test_validate_params(): void { + [$paramone, $paramtwo] = database::generate_param_names(2); + + $params = [ + $paramone => 1, + $paramtwo => 2, + ]; + + $this->assertTrue(database::validate_params($params)); + } + + /** + * Test parameter validation for invalid parameters + */ + public function test_validate_params_invalid(): void { $params = [ database::generate_param_name() => 1, 'invalidfoo' => 2, @@ -78,4 +104,39 @@ class database_test extends advanced_testcase { $this->expectExceptionMessage('Invalid parameter names (invalidfoo, invalidbar)'); database::validate_params($params); } + + /** + * Generate aliases and parameters and confirm they can be used within a query + */ + public function test_generated_data_in_query(): void { + global $DB; + + // Unique aliases. + [ + $usertablealias, + $userfieldalias, + ] = database::generate_aliases(2); + + // Unique parameters. + [ + $paramuserid, + $paramuserdeleted, + ] = database::generate_param_names(2); + + // Simple query to retrieve the admin user. + $sql = "SELECT {$usertablealias}.id AS {$userfieldalias} + FROM {user} {$usertablealias} + WHERE {$usertablealias}.id = :{$paramuserid} + AND {$usertablealias}.deleted = :{$paramuserdeleted}"; + + $admin = core_user::get_user_by_username('admin'); + + $params = [ + $paramuserid => $admin->id, + $paramuserdeleted => 0, + ]; + + $record = $DB->get_record_sql($sql, $params); + $this->assertEquals($admin->id, $record->{$userfieldalias}); + } } diff --git a/reportbuilder/upgrade.txt b/reportbuilder/upgrade.txt new file mode 100644 index 00000000000..5494855158e --- /dev/null +++ b/reportbuilder/upgrade.txt @@ -0,0 +1,5 @@ +This file describes API changes in /reportbuilder/* +Information provided here is intended especially for developers. + +=== 4.1 === +* New database helper methods for generating multiple unique values: `generate_aliases` and `generate_param_names`