From aa0a602b106921bdc64effa33115a1a94c3248cc Mon Sep 17 00:00:00 2001 From: Matteo Scaramuccia <moodle@matteoscaramuccia.com> Date: Sun, 19 May 2019 20:29:20 +0200 Subject: [PATCH] MDL-65811 database: Fix subsetting in MSSQL --- lib/dml/sqlsrv_native_moodle_database.php | 28 ++++- .../sqlsrv_native_moodle_database_test.php | 118 ++++++++++++++++++ 2 files changed, 144 insertions(+), 2 deletions(-) diff --git a/lib/dml/sqlsrv_native_moodle_database.php b/lib/dml/sqlsrv_native_moodle_database.php index 9174f70c600..a91317dc71d 100644 --- a/lib/dml/sqlsrv_native_moodle_database.php +++ b/lib/dml/sqlsrv_native_moodle_database.php @@ -841,6 +841,30 @@ class sqlsrv_native_moodle_database extends moodle_database { return true; } + /** + * Whether the given SQL statement has the ORDER BY clause in the main query. + * + * @param string $sql the SQL statement + * @return bool true if the main query has the ORDER BY clause; otherwise, false. + */ + protected static function has_query_order_by(string $sql) { + $sqltoupper = strtoupper($sql); + // Fail fast if there is no ORDER BY clause in the original query. + if (strpos($sqltoupper, 'ORDER BY') === false) { + return false; + } + + // Search for an ORDER BY clause in the main query, not in any subquery (not always allowed in MSSQL) + // or in clauses like OVER with a window function e.g. ROW_NUMBER() OVER (ORDER BY ...) or RANK() OVER (ORDER BY ...): + // use PHP PCRE recursive patterns to remove everything found within round brackets. + $mainquery = preg_replace('/\(((?>[^()]+)|(?R))*\)/', '()', $sqltoupper); + if (strpos($mainquery, 'ORDER BY') !== false) { + return true; + } + + return false; + } + /** * Get a number of records as a moodle_recordset using a SQL statement. * @@ -876,9 +900,9 @@ class sqlsrv_native_moodle_database extends moodle_database { } else { $needscrollable = false; // Using supported fetch/offset, no need to scroll anymore. $sql = (substr($sql, -1) === ';') ? substr($sql, 0, -1) : $sql; - // We need order by to use FETCH/OFFSET. + // We need ORDER BY to use FETCH/OFFSET. // Ordering by first column shouldn't break anything if there was no order in the first place. - if (!strpos(strtoupper($sql), "ORDER BY")) { + if (!self::has_query_order_by($sql)) { $sql .= " ORDER BY 1"; } diff --git a/lib/dml/tests/sqlsrv_native_moodle_database_test.php b/lib/dml/tests/sqlsrv_native_moodle_database_test.php index 65cd4706e7a..8736d795b8e 100644 --- a/lib/dml/tests/sqlsrv_native_moodle_database_test.php +++ b/lib/dml/tests/sqlsrv_native_moodle_database_test.php @@ -134,6 +134,124 @@ class sqlsrv_native_moodle_database_testcase extends advanced_testcase { $temptablesproperty->setValue($sqlsrv, null); $this->assertEquals($expected, $result); } + + /** + * Data provider for test_has_query_order_by + * + * @return array data for test_has_query_order_by + */ + public function has_query_order_by_provider() { + // Fixtures taken from https://docs.moodle.org/en/ad-hoc_contributed_reports. + + return [ + 'User with language => FALSE' => [ + 'sql' => <<<EOT +SELECT username, lang + FROM prefix_user +EOT + , + 'expectedmainquery' => <<<EOT +SELECT username, lang + FROM prefix_user +EOT + , + 'expectedresult' => false + ], + 'List Users with extra info (email) in current course => FALSE' => [ + 'sql' => <<<EOT +SELECT u.firstname, u.lastname, u.email + FROM prefix_role_assignments AS ra + JOIN prefix_context AS context ON context.id = ra.contextid AND context.contextlevel = 50 + JOIN prefix_course AS c ON c.id = context.instanceid AND c.id = %%COURSEID%% + JOIN prefix_user AS u ON u.id = ra.userid +EOT + , + 'expectedmainquery' => <<<EOT +SELECT u.firstname, u.lastname, u.email + FROM prefix_role_assignments AS ra + JOIN prefix_context AS context ON context.id = ra.contextid AND context.contextlevel = 50 + JOIN prefix_course AS c ON c.id = context.instanceid AND c.id = %%COURSEID%% + JOIN prefix_user AS u ON u.id = ra.userid +EOT + , + 'expectedresult' => false + ], + 'ROW_NUMBER() OVER (ORDER BY ...) => FALSE (https://github.com/jleyva/moodle-block_configurablereports/issues/120)' => [ + 'sql' => <<<EOT +SELECT COUNT(*) AS 'Users who have logged in today' + FROM ( + SELECT ROW_NUMBER() OVER(ORDER BY lastaccess DESC) AS Row + FROM mdl_user + WHERE lastaccess > DATEDIFF(s, '1970-01-01 02:00:00', (SELECT Convert(DateTime, DATEDIFF(DAY, 0, GETDATE())))) + ) AS Logins +EOT + , + 'expectedmainquery' => <<<EOT +SELECT COUNT() AS 'Users who have logged in today' + FROM () AS Logins +EOT + , + 'expectedresult' => false + ], + 'CONTRIB-7725 workaround) => TRUE' => [ + 'sql' => <<<EOT +SELECT COUNT(*) AS 'Users who have logged in today' + FROM ( + SELECT ROW_NUMBER() OVER(ORDER BY lastaccess DESC) AS Row + FROM mdl_user + WHERE lastaccess > DATEDIFF(s, '1970-01-01 02:00:00', (SELECT Convert(DateTime, DATEDIFF(DAY, 0, GETDATE())))) + ) AS Logins ORDER BY 1 +EOT + , + 'expectedmainquery' => <<<EOT +SELECT COUNT() AS 'Users who have logged in today' + FROM () AS Logins ORDER BY 1 +EOT + , + 'expectedresult' => true + ], + 'Enrolment count in each Course => TRUE' => [ + 'sql' => <<<EOT + SELECT c.fullname, COUNT(ue.id) AS Enroled + FROM prefix_course AS c + JOIN prefix_enrol AS en ON en.courseid = c.id + JOIN prefix_user_enrolments AS ue ON ue.enrolid = en.id +GROUP BY c.id +ORDER BY c.fullname +EOT + , + 'expectedmainquery' => <<<EOT + SELECT c.fullname, COUNT() AS Enroled + FROM prefix_course AS c + JOIN prefix_enrol AS en ON en.courseid = c.id + JOIN prefix_user_enrolments AS ue ON ue.enrolid = en.id +GROUP BY c.id +ORDER BY c.fullname +EOT + , + 'expectedresult' => true + ], + ]; + } + + /** + * Test has_query_order_by + * + * @dataProvider has_query_order_by_provider + * @param string $sql the query + * @param string $expectedmainquery the expected main query + * @param bool $expectedresult the expected result + */ + public function test_has_query_order_by(string $sql, string $expectedmainquery, bool $expectedresult) { + $mainquery = preg_replace('/\(((?>[^()]+)|(?R))*\)/', '()', $sql); + $this->assertSame($expectedmainquery, $mainquery); + + // The has_query_order_by static method is protected. Use Reflection to call the method. + $method = new ReflectionMethod('sqlsrv_native_moodle_database', 'has_query_order_by'); + $method->setAccessible(true); + $result = $method->invoke(null, $sql); + $this->assertSame($expectedresult, $result); + } } /**