Make row limit an admin setting

Based on a suggestion by Adrian Perez, but mostly implemented by me.

changes.md

@@ -3,7 +3,8 @@
 ## Changes in 3.7
 
 * Report results can now be downloaded in any of data format that Moodle supports, not just CSV.
-* Admins can now set which day is considered the first of the week for weely reports.
+* Admins can control the maximum possible limit for the number of rows a query can return.
+* Admins can now set which day is considered the first of the week for weekly reports.
   This defaults to the Moodle setting for this for new installs. For existing installs,
   it stays the same as before (Saturday) but you can change it.
 

db/upgrade.php

@@ -81,7 +81,7 @@ function xmldb_report_customsql_upgrade($oldversion) {
         require_once($CFG->dirroot . '/report/customsql/locallib.php');
         $table = new xmldb_table('report_customsql_queries');
         $field = new xmldb_field('querylimit', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED,
-                XMLDB_NOTNULL, null, REPORT_CUSTOMSQL_MAX_RECORDS, 'queryparams');
+                XMLDB_NOTNULL, null, 5000, 'queryparams');
 
         if (!$dbman->field_exists($table, $field)) {
             $dbman->add_field($table, $field);
@@ -139,7 +139,7 @@ function xmldb_report_customsql_upgrade($oldversion) {
         require_once($CFG->dirroot . '/report/customsql/locallib.php');
         $table = new xmldb_table('report_customsql_queries');
         $field = new xmldb_field('querylimit', XMLDB_TYPE_INTEGER, '10', XMLDB_UNSIGNED,
-                XMLDB_NOTNULL, null, REPORT_CUSTOMSQL_MAX_RECORDS, 'queryparams');
+                XMLDB_NOTNULL, null, 5000, 'queryparams');
 
         if (!$dbman->field_exists($table, $field)) {
             $dbman->add_field($table, $field);

edit_form.php

@@ -88,7 +88,7 @@ public function definition() {
 
         $mform->addElement('text', 'querylimit', get_string('querylimit', 'report_customsql'));
         $mform->setType('querylimit', PARAM_INT);
-        $mform->setDefault('querylimit', REPORT_CUSTOMSQL_MAX_RECORDS);
+        $mform->setDefault('querylimit', get_config('report_customsql', 'querylimitdefault'));
         $mform->addRule('querylimit', get_string('requireint', 'report_customsql'),
                         'numeric', null, 'client');
 
@@ -201,9 +201,10 @@ public function validation($data, $files) {
             }
         }
 
-        // Check querylimit in range 1 .. REPORT_CUSTOMSQL_MAX_RECORDS.
-        if (empty($data['querylimit']) || $data['querylimit'] > REPORT_CUSTOMSQL_MAX_RECORDS) {
-            $errors['querylimit'] = get_string('querylimitrange', 'report_customsql', REPORT_CUSTOMSQL_MAX_RECORDS);
+        // Check querylimit is in range.
+        $maxlimit = get_config('report_customsql', 'querylimitmaximum');
+        if (empty($data['querylimit']) || $data['querylimit'] > $maxlimit) {
+            $errors['querylimit'] = get_string('querylimitrange', 'report_customsql', $maxlimit);
         }
 
         if (!empty($data['customdir'])) {

lang/en/report_customsql.php

@@ -112,7 +112,11 @@
 $string['query_edited'] = 'Query edited';
 $string['query_viewed'] = 'Query viewed';
 $string['queryfailed'] = 'Error when executing the query: {$a}';
-$string['querylimit'] = 'Limit rows returned';
+$string['querylimit'] = 'Limit on rows returned';
+$string['querylimitdefault'] = 'Default limit on rows returned';
+$string['querylimitdefault_desc'] = 'To avoid accidents where a query return a huge number of rows which might overload the server, each query has a limit to the number of rows it can return. This is the default value for that limit for new queries.';
+$string['querylimitmaximum'] = 'Maximum allowed limit on rows returned';
+$string['querylimitmaximum_desc'] = 'This is the absolute maximum limit on rows returned which a query author is allowed to set.';
 $string['querylimitrange'] = 'Number must be between 1 and {$a}';
 $string['querynote'] = '<ul>
 <li>The token <code>%%WWWROOT%%</code> in the results will be replaced with <code>{$a}</code>.</li>
@@ -145,6 +149,7 @@
 $string['scheduledqueries'] = 'Scheduled queries';
 $string['startofweek'] = 'Day to run weekly reports';
 $string['startofweek_default'] = 'Use site calendar start of week ({$a})';
+$string['startofweek_desc'] = 'This is the day which should be considered the first day of the week, for weekly scheduled reports.';
 $string['typeofresult'] = 'Type of result';
 $string['unknowndownloadfile'] = 'Unknown download file.';
 $string['usernotfound'] = 'User with the username \'{$a}\' does not exist';

locallib.php

@@ -27,12 +27,15 @@
 global $CFG;
 require_once($CFG->libdir . '/validateurlsyntax.php');
 
-define('REPORT_CUSTOMSQL_MAX_RECORDS', 5000);
+define('REPORT_CUSTOMSQL_LIMIT_EXCEEDED_MARKER', '-- ROW LIMIT EXCEEDED --');
 
-function report_customsql_execute_query($sql, $params = null,
-        $limitnum = REPORT_CUSTOMSQL_MAX_RECORDS) {
+function report_customsql_execute_query($sql, $params = null, $limitnum = null) {
     global $CFG, $DB;
 
+    if ($limitnum === null) {
+        $limitnum = get_config('report_customsql', 'querylimitdefault');
+    }
+
     $sql = preg_replace('/\bprefix_(?=\w+)/i', $CFG->prefix, $sql);
 
     foreach ($params as $name => $value) {
@@ -100,11 +103,13 @@ function report_customsql_generate_csv($report, $timenow) {
     $sql = report_customsql_prepare_sql($report, $timenow);
 
     $queryparams = !empty($report->queryparams) ? unserialize($report->queryparams) : array();
-    $querylimit  = !empty($report->querylimit) ? $report->querylimit : REPORT_CUSTOMSQL_MAX_RECORDS;
-    $rs = report_customsql_execute_query($sql, $queryparams, $querylimit);
+    $querylimit  = $report->querylimit ?? get_config('report_customsql', 'querylimitdefault');
+    // Query one extra row, so we can tell if we hit the limit.
+    $rs = report_customsql_execute_query($sql, $queryparams, $querylimit + 1);
 
     $csvfilenames = array();
     $csvtimestamp = null;
+    $count = 0;
     foreach ($rs as $row) {
         if (!$csvtimestamp) {
             list($csvfilename, $csvtimestamp) = report_customsql_csv_filename($report, $timenow);
@@ -129,15 +134,20 @@ function report_customsql_generate_csv($report, $timenow) {
             array_unshift($data, strftime('%Y-%m-%d', $timenow));
         }
         report_customsql_write_csv_row($handle, $data);
+        $count += 1;
     }
     $rs->close();
 
     if (!empty($handle)) {
+        if ($count > $querylimit) {
+            report_customsql_write_csv_row($handle, [REPORT_CUSTOMSQL_LIMIT_EXCEEDED_MARKER]);
+        }
+
         fclose($handle);
     }
 
     // Update the execution time in the DB.
-    $updaterecord = new stdClass;
+    $updaterecord = new stdClass();
     $updaterecord->id = $report->id;
     $updaterecord->lastrun = time();
     $updaterecord->lastexecutiontime = round((microtime(true) - $starttime) * 1000);

settings.php

@@ -35,8 +35,17 @@
 
     // Setting this option to -1 will use the value from the site calendar.
     $options = [-1 => get_string('startofweek_default', 'report_customsql', $days[$default])] + $days;
-    $settings->add(new admin_setting_configselect('report_customsql/startwday', get_string('startofweek', 'report_customsql'),
-        null, -1, $options));
+    $settings->add(new admin_setting_configselect('report_customsql/startwday',
+            get_string('startofweek', 'report_customsql'),
+            get_string('startofweek_desc', 'report_customsql'), -1, $options));
+
+    $settings->add(new admin_setting_configtext_with_maxlength('report_customsql/querylimitdefault',
+            get_string('querylimitdefault', 'report_customsql'),
+            get_string('querylimitdefault_desc', 'report_customsql'), 5000, PARAM_INT, null, 10));
+
+    $settings->add(new admin_setting_configtext_with_maxlength('report_customsql/querylimitmaximum',
+            get_string('querylimitmaximum', 'report_customsql'),
+            get_string('querylimitmaximum_desc', 'report_customsql'), 5000, PARAM_INT, null, 10));
 }
 
 $ADMIN->add('reports', new admin_externalpage('report_customsql',

tests/behat/report_customsql.feature

@@ -185,3 +185,17 @@ Feature: Ad-hoc database queries report
     And I should see "lastname: User"
     And I should see "[email protected]"
     And I should see "This report has 1 rows."
+
+  Scenario: Test reporting when a query exceeds the limit
+    Given the following config values are set as admin:
+      | querylimitdefault | 1 | report_customsql |
+    When I log in as "admin"
+    And I navigate to "Reports > Ad-hoc database queries" in site administration
+    And I press "Add a new query"
+    And I set the following fields to these values:
+      | Query name  | Test query                                                                                   |
+      | Description | Query that tries to return 2 rows.                                                           |
+      | Query SQL   | SELECT * FROM {config_plugins} WHERE name = 'version' AND plugin IN ('mod_quiz', 'mod_wiki') |
+    And I press "Save changes"
+    Then I should see "Test query"
+    And I should see "This query reached the limit of 1 rows. Some rows may have been omitted from the end."

view.php

@@ -173,18 +173,24 @@
         $table->id = 'report_customsql_results';
         list($table->head, $linkcolumns) = report_customsql_get_table_headers(fgetcsv($handle));
 
+        $rowlimitexceeded = false;
         while ($row = fgetcsv($handle)) {
-            $table->data[] = report_customsql_display_row($row, $linkcolumns);
-            $count += 1;
+            $data = report_customsql_display_row($row, $linkcolumns);
+            if (isset($data[0]) && $data[0] === REPORT_CUSTOMSQL_LIMIT_EXCEEDED_MARKER) {
+                $rowlimitexceeded = true;
+            } else {
+                $table->data[] = $data;
+                $count += 1;
+            }
         }
 
         fclose($handle);
         echo html_writer::table($table);
 
-        if ($count >= REPORT_CUSTOMSQL_MAX_RECORDS) {
+        if ($rowlimitexceeded) {
             echo html_writer::tag('p', get_string('recordlimitreached', 'report_customsql',
-                                                  REPORT_CUSTOMSQL_MAX_RECORDS),
-                                                  array('class' => 'admin_note'));
+                    $report->querylimit ?? get_config('report_customsql', 'querylimitdefault')),
+                    array('class' => 'admin_note'));
         } else {
             echo html_writer::tag('p', get_string('recordcount', 'report_customsql', $count),
                     array('class' => 'admin_note'));