mirror_php/moodle
1
0
Fork 0
mirror of https://github.com/moodle/moodle.git synced 2025-01-17 05:28:30 +01:00
Code Issues Projects Releases Wiki Activity

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

Browse Source 
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.

There is only one such filter present currently with which we can test
this, see 2f9001cbe9.
This commit is contained in:
Paul Holden 2024-04-04 12:26:18 +01:00
parent f88dbfcafc
commit 1c2f5dc1df
No known key found for this signature in database
GPG Key ID: A81A96D6045F6164
5 changed files with 119 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
badges/tests/reportbuilder/datasource/badges_test.php
View File

@ -22,6 +22,7 @@ use core_badges_generator;
use core_reportbuilder_generator;
use core_reportbuilder_testcase;
use core_reportbuilder\local\filters\{boolean_select, date, select, tags, text};
use core_reportbuilder\manager;
defined('MOODLE_INTERNAL') || die();
@ -37,7 +38,7 @@ require_once("{$CFG->libdir}/badgeslib.php");
* @copyright 2022 Paul Holden <paulh@moodle.com>
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
class badges_test extends core_reportbuilder_testcase {
final class badges_test extends core_reportbuilder_testcase {
/**
* Test default datasource
@ -200,12 +201,53 @@ class badges_test extends core_reportbuilder_testcase {
$this->assertEquals($course->fullname, $coursename);
}
/**
* Test creating a report containing "expiry" as both a condition and a filter
*
* This is really testing that it's possible to do so, for a filter instance that returns SQL parameters
*/
public function test_report_expiry_condition_and_filter(): void {
$this->resetAfterTest();
/** @var core_badges_generator $generator */
$generator = $this->getDataGenerator()->get_plugin_generator('core_badges');
$badgeone = $generator->create_badge(['name' => 'Badge 1', 'expiredate' => 10]);
$badgetwo = $generator->create_badge(['name' => 'Badge 2', 'expiredate' => 20]);
$badgethree = $generator->create_badge(['name' => 'Badge 3', 'expiredate' => 30]);
/** @var core_reportbuilder_generator $generator */
$generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder');
$report = $generator->create_report(['name' => 'Badges', 'source' => badges::class, 'default' => 0]);
$generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'badge:name']);
$generator->create_condition(['reportid' => $report->get('id'), 'uniqueidentifier' => 'badge:expiry']);
$generator->create_filter(['reportid' => $report->get('id'), 'uniqueidentifier' => 'badge:expiry']);
// Load report instance, set condition.
$instance = manager::get_report_from_persistent($report);
$instance->set_condition_values([
'badge:expiry_operator' => date::DATE_RANGE,
'badge:expiry_from' => 15,
]);
// Set filter.
$content = $this->get_custom_report_content($report->get('id'), 0, [
'badge:expiry_operator' => date::DATE_RANGE,
'badge:expiry_to' => 25,
]);
$this->assertEquals([
[$badgetwo->name],
], array_map('array_values', $content));
}
/**
* Data provider for {@see test_datasource_filters}
*
* @return array[]
*/
public function datasource_filters_provider(): array {
public static function datasource_filters_provider(): array {
return [
// Badge.
'Filter badge name' => ['badge:name', [

23
reportbuilder/classes/local/helpers/database.php
View File

@ -108,7 +108,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
*/
@ -126,6 +126,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
*

22
reportbuilder/classes/table/base_report_table.php
View File

@ -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];
}
/**

35
reportbuilder/tests/local/helpers/database_test.php
View File

@ -30,7 +30,7 @@ use core_user;
* @copyright 2020 Paul Holden <paulh@moodle.com>
* @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
@ -176,9 +176,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',
@ -192,4 +194,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);
}
}

4
reportbuilder/upgrade.txt
View File

@ -1,6 +1,10 @@
This file describes API changes in /reportbuilder/*
Information provided here is intended especially for developers.
=== 4.4.1 ===
* New database helper method `sql_replace_parameters` to help ensure uniqueness of parameters within a SQL expression
=== 4.4 ===
* New methods `get_identity_[columns|filters]` in user entity, for retrieving all user identity field report elements