diff --git a/lib/dml/moodle_database.php b/lib/dml/moodle_database.php index 97a41bc1bed..f9f7aada275 100644 --- a/lib/dml/moodle_database.php +++ b/lib/dml/moodle_database.php @@ -2162,6 +2162,17 @@ abstract class moodle_database { return ' CEIL(' . $fieldname . ')'; } + /** + * Return SQL for casting to char of given field/expression. Default implementation performs implicit cast using + * concatenation with an empty string + * + * @param string $field Table field or SQL expression to be cast + * @return string + */ + public function sql_cast_to_char(string $field): string { + return $this->sql_concat("''", $field); + } + /** * Returns the SQL to be used in order to CAST one CHAR column to INTEGER. * diff --git a/lib/dml/oci_native_moodle_database.php b/lib/dml/oci_native_moodle_database.php index 6db6a7bca86..06e6b3a8d45 100644 --- a/lib/dml/oci_native_moodle_database.php +++ b/lib/dml/oci_native_moodle_database.php @@ -1554,6 +1554,16 @@ class oci_native_moodle_database extends moodle_database { return 'MOD(' . $int1 . ', ' . $int2 . ')'; } + /** + * Return SQL for casting to char of given field/expression + * + * @param string $field Table field or SQL expression to be cast + * @return string + */ + public function sql_cast_to_char(string $field): string { + return "TO_CHAR({$field})"; + } + public function sql_cast_char2int($fieldname, $text=false) { if (!$text) { return ' CAST(' . $fieldname . ' AS INT) '; diff --git a/lib/dml/pgsql_native_moodle_database.php b/lib/dml/pgsql_native_moodle_database.php index dd5095002c4..092456dc1ac 100644 --- a/lib/dml/pgsql_native_moodle_database.php +++ b/lib/dml/pgsql_native_moodle_database.php @@ -1453,6 +1453,16 @@ class pgsql_native_moodle_database extends moodle_database { return '((' . $int1 . ') # (' . $int2 . '))'; } + /** + * Return SQL for casting to char of given field/expression + * + * @param string $field Table field or SQL expression to be cast + * @return string + */ + public function sql_cast_to_char(string $field): string { + return "CAST({$field} AS VARCHAR)"; + } + public function sql_cast_char2int($fieldname, $text=false) { return ' CAST(' . $fieldname . ' AS INT) '; } @@ -1493,7 +1503,7 @@ class pgsql_native_moodle_database extends moodle_database { */ public function sql_group_concat(string $field, string $separator = ', ', string $sort = ''): string { $fieldsort = $sort ? "ORDER BY {$sort}" : ''; - return "STRING_AGG(CAST({$field} AS VARCHAR), '{$separator}' {$fieldsort})"; + return "STRING_AGG(" . $this->sql_cast_to_char($field) . ", '{$separator}' {$fieldsort})"; } public function sql_regex_supported() { diff --git a/lib/dml/tests/dml_test.php b/lib/dml/tests/dml_test.php index 23f9b7a3bfa..f25fe031bd1 100644 --- a/lib/dml/tests/dml_test.php +++ b/lib/dml/tests/dml_test.php @@ -32,6 +32,7 @@ defined('MOODLE_INTERNAL') || die(); * @subpackage dml * @copyright 2008 Nicolas Connault * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @coversDefaultClass \moodle_database */ class dml_test extends database_driver_testcase { @@ -501,7 +502,7 @@ SELECT * FROM {users} -- line 74 of /lib/dml/tests/fixtures/test_dml_sql_debugging_fixture.php: call to test_dml_sql_debugging_fixture->one() -- line 83 of /lib/dml/tests/fixtures/test_dml_sql_debugging_fixture.php: call to test_dml_sql_debugging_fixture->two() -- line 92 of /lib/dml/tests/fixtures/test_dml_sql_debugging_fixture.php: call to test_dml_sql_debugging_fixture->three() --- line 497 of /lib/dml/tests/dml_test.php: call to test_dml_sql_debugging_fixture->four() +-- line 498 of /lib/dml/tests/dml_test.php: call to test_dml_sql_debugging_fixture->four() EOD; $this->assertEquals($this->unix_to_os_dirsep($expected), $out); @@ -3840,6 +3841,44 @@ EOD; $this->assertEquals(666, $DB->get_field_sql($sql)); } + /** + * Test DML libraries sql_cast_to_char method + * + * @covers ::sql_cast_to_char + */ + public function test_cast_to_char(): void { + $DB = $this->tdb; + $dbman = $DB->get_manager(); + + $tableone = $this->get_test_table('one'); + $tableone->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null); + $tableone->add_field('intfield', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, null); + $tableone->add_field('details', XMLDB_TYPE_CHAR, '255', null, XMLDB_NOTNULL, null, null); + $tableone->add_key('primary', XMLDB_KEY_PRIMARY, ['id']); + $dbman->create_table($tableone); + + $tableonename = $tableone->getName(); + $DB->insert_record($tableonename, (object) ['intfield' => 10, 'details' => 'uno']); + $DB->insert_record($tableonename, (object) ['intfield' => 20, 'details' => 'dos']); + + $tabletwo = $this->get_test_table('two'); + $tabletwo->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null); + $tabletwo->add_field('charfield', XMLDB_TYPE_CHAR, '255', null, XMLDB_NOTNULL, null, null); + $tabletwo->add_key('primary', XMLDB_KEY_PRIMARY, ['id']); + $dbman->create_table($tabletwo); + + $tabletwoname = $tabletwo->getName(); + $DB->insert_record($tabletwoname, (object) ['charfield' => '10']); + + // Test by joining a char field to a cast int field (mixing types not supported across databases). + $sql = "SELECT t1.details + FROM {{$tableonename}} t1 + JOIN {{$tabletwoname}} t2 ON t2.charfield = " . $DB->sql_cast_to_char('t1.intfield'); + + $fieldset = $DB->get_fieldset_sql($sql); + $this->assertEquals(['uno'], $fieldset); + } + public function test_cast_char2int() { $DB = $this->tdb; $dbman = $DB->get_manager(); diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 903b32b332b..a272a8df4bf 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -12,6 +12,7 @@ information provided here is intended especially for developers. an event exists with the given criteria (see MDL-72723 for details). - Breaking: 3rd party log readers implementing interface sql_reader will need to implement get_events_select_exists() * Added $strictness parameter to persistent `get_record` method, optionally allowing caller to ensure record exists +* New DML driver method `$DB->sql_cast_to_char` for casting given field/expression to char * For plugins that override secondary navigation, the namespace for the custom secondary navigation class has changed. It was (for example) mod_mymodule\local\views\secondary but is now mod_mymodule\navigation\views\secondary. The old location will continue to work, but is deprecated. diff --git a/reportbuilder/classes/local/aggregation/base.php b/reportbuilder/classes/local/aggregation/base.php index 6ddb1316fa8..19d5686a0a2 100644 --- a/reportbuilder/classes/local/aggregation/base.php +++ b/reportbuilder/classes/local/aggregation/base.php @@ -86,17 +86,13 @@ abstract class base { final protected static function get_column_fields_concat(array $sqlfields, string $delimeter = ','): string { global $DB; + // We need to ensure all values are char. + $sqlfieldrequirescast = in_array($DB->get_dbfamily(), ['oracle', 'postgres']); + $concatfields = []; foreach ($sqlfields as $sqlfield) { - - // We need to ensure all values are char (this ought to be done in the DML drivers, see MDL-72184). - switch ($DB->get_dbfamily()) { - case 'postgres' : - $sqlfield = "CAST({$sqlfield} AS VARCHAR)"; - break; - case 'oracle' : - $sqlfield = "TO_CHAR({$sqlfield})"; - break; + if ($sqlfieldrequirescast) { + $sqlfield = $DB->sql_cast_to_char($sqlfield); } // Coalesce all the SQL fields, to remove all nulls. diff --git a/reportbuilder/classes/local/aggregation/groupconcatdistinct.php b/reportbuilder/classes/local/aggregation/groupconcatdistinct.php index 22b248b724a..eb4423cf81a 100644 --- a/reportbuilder/classes/local/aggregation/groupconcatdistinct.php +++ b/reportbuilder/classes/local/aggregation/groupconcatdistinct.php @@ -69,11 +69,12 @@ class groupconcatdistinct extends groupconcat { // DB limitations mean we only support MySQL and Postgres, and each handle it differently. $fieldsort = database::sql_group_concat_sort($field); if ($DB->get_dbfamily() === 'postgres') { + $field = $DB->sql_cast_to_char($field); if ($fieldsort !== '') { $fieldsort = "ORDER BY {$fieldsort}"; } - return "STRING_AGG(DISTINCT CAST({$field} AS VARCHAR), '" . self::FIELD_VALUE_DELIMETER . "' {$fieldsort})"; + return "STRING_AGG(DISTINCT {$field}, '" . self::FIELD_VALUE_DELIMETER . "' {$fieldsort})"; } else { return $DB->sql_group_concat("DISTINCT {$field}", self::FIELD_VALUE_DELIMETER, $fieldsort); } diff --git a/reportbuilder/classes/local/helpers/database.php b/reportbuilder/classes/local/helpers/database.php index 319918086bd..de250176270 100644 --- a/reportbuilder/classes/local/helpers/database.php +++ b/reportbuilder/classes/local/helpers/database.php @@ -134,7 +134,7 @@ class database { } // Cast sort, stick the direction on the end. - $fieldsort = "CAST({$fieldsort} AS VARCHAR) {$fieldsortdirection}"; + $fieldsort = $DB->sql_cast_to_char($fieldsort) . ' ' . $fieldsortdirection; } return $fieldsort; diff --git a/reportbuilder/tests/local/aggregation/groupconcat_test.php b/reportbuilder/tests/local/aggregation/groupconcat_test.php index f10ac6c85ba..e162d09cf81 100644 --- a/reportbuilder/tests/local/aggregation/groupconcat_test.php +++ b/reportbuilder/tests/local/aggregation/groupconcat_test.php @@ -77,6 +77,32 @@ class groupconcat_test extends core_reportbuilder_testcase { ], $content); } + /** + * Test aggregation when applied to column with multiple fields + */ + public function test_column_aggregation_multiple_fields(): void { + $this->resetAfterTest(); + + $user = $this->getDataGenerator()->create_user(['firstname' => 'Adam', 'lastname' => 'Apple']); + + /** @var core_reportbuilder_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); + $report = $generator->create_report(['name' => 'Users', 'source' => users::class, 'default' => 0]); + + // This is the column we'll aggregate. + $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:fullnamewithlink']) + ->set('aggregation', groupconcat::get_class_name()) + ->update(); + + $content = $this->get_custom_report_content($report->get('id')); + $this->assertCount(1, $content); + + // Ensure users are sorted predictably (Adam -> Admin). + [$userone, $usertwo] = explode(', ', reset($content[0])); + $this->assertStringContainsString(fullname($user, true), $userone); + $this->assertStringContainsString(fullname(get_admin(), true), $usertwo); + } + /** * Test aggregation when applied to column with callback */ diff --git a/reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php b/reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php index 680a5e37127..74c5b2c12f5 100644 --- a/reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php +++ b/reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php @@ -89,6 +89,32 @@ class groupconcatdistinct_test extends core_reportbuilder_testcase { ], $content); } + /** + * Test aggregation when applied to column with multiple fields + */ + public function test_column_aggregation_multiple_fields(): void { + $this->resetAfterTest(); + + $user = $this->getDataGenerator()->create_user(['firstname' => 'Adam', 'lastname' => 'Apple']); + + /** @var core_reportbuilder_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); + $report = $generator->create_report(['name' => 'Users', 'source' => users::class, 'default' => 0]); + + // This is the column we'll aggregate. + $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:fullnamewithlink']) + ->set('aggregation', groupconcatdistinct::get_class_name()) + ->update(); + + $content = $this->get_custom_report_content($report->get('id')); + $this->assertCount(1, $content); + + // Ensure users are sorted predictably (Adam -> Admin). + [$userone, $usertwo] = explode(', ', reset($content[0])); + $this->assertStringContainsString(fullname($user, true), $userone); + $this->assertStringContainsString(fullname(get_admin(), true), $usertwo); + } + /** * Test aggregation when applied to column with callback */