From be6fc5b122517860f952ff4859decc7913228d6d Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 1 Jun 2018 08:14:51 +0800 Subject: [PATCH 1/3] MDL-62619 privacy: Improve performance of contexts loading When possible, we want to use JOIN over WHERE IN. It is known that the later performs badly in MySQL. --- privacy/classes/local/request/contextlist.php | 60 ++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/privacy/classes/local/request/contextlist.php b/privacy/classes/local/request/contextlist.php index cef56ff4b56..4aa1cf4c975 100644 --- a/privacy/classes/local/request/contextlist.php +++ b/privacy/classes/local/request/contextlist.php @@ -47,7 +47,30 @@ class contextlist extends contextlist_base { global $DB; $fields = \context_helper::get_preload_record_columns_sql('ctx'); - $wrapper = "SELECT {$fields} FROM {context} ctx WHERE id IN ({$sql})"; + if ($fieldname = $this->guess_id_field_from_sql($sql)) { + if (is_int($fieldname)) { + $wrapper = " + SELECT + {$fields} + FROM {context} ctx + WHERE ctx.id = :fieldvalue"; + $params = ['fieldvalue' => $fieldname]; + } else { + // Able to guess a field name. + $wrapper = " + SELECT + {$fields} + FROM {context} ctx + JOIN ({$sql}) target ON ctx.id = target.{$fieldname}"; + } + } else { + // No field name available. Fall back on a potentially slower version. + $wrapper = " + SELECT + {$fields} + FROM {context} ctx + WHERE ctx.id IN ({$sql})"; + } $contexts = $DB->get_recordset_sql($wrapper, $params); $contextids = []; @@ -110,4 +133,39 @@ class contextlist extends contextlist_base { public function set_component($component) { parent::set_component($component); } + + /** + * Guess the name of the contextid field from the supplied SQL. + * + * @param string $sql The SQL to guess from + * @return string The field name. + */ + protected function guess_id_field_from_sql($sql) { + // Get the list of relevant words from the SQL Query. + // We explode the SQL by the space character, then trim any extra whitespace (e.g. newlines), before we filter + // empty value, and finally we re-index the array. + $words = array_map('trim', explode(' ', $sql)); + $words = array_filter($words, function($word) { + return $word !== ''; + }); + $words = array_values($words); + + if ($firstfrom = array_search('FROM', $words)) { + // Found a FROM keyword. + // Select the previous word. + $fieldname = $words[$firstfrom - 1]; + if (is_int($fieldname)) { + return $fieldname; + } + + if ($hasdot = strpos($fieldname, '.')) { + // This field is against a table alias. Take off the alias. + $fieldname = substr($fieldname, $hasdot + 1); + } + + return $fieldname; + } + + return ''; + } } From bb478e8ddda234b0148d2bb033ebf762c2101e6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Tue, 3 Jul 2018 22:28:35 +0200 Subject: [PATCH 2/3] MDL-62619 privacy: Fix some bits of performance improvements * We need to use is_numeric() in this case as is_int() would never return true. * Extend the supported cases, add support for SQL consisting just of numerical value or selectinga numerical constant. * Do not rely on any particulat letter case in provided SQL. * Add unit tests for the new method. Even when it is a protected one, it is an essential unit to be tested on its own. --- privacy/classes/local/request/contextlist.php | 30 ++++--- privacy/tests/contextlist_test.php | 81 +++++++++++++++++++ 2 files changed, 99 insertions(+), 12 deletions(-) diff --git a/privacy/classes/local/request/contextlist.php b/privacy/classes/local/request/contextlist.php index 4aa1cf4c975..903ae7d6be9 100644 --- a/privacy/classes/local/request/contextlist.php +++ b/privacy/classes/local/request/contextlist.php @@ -48,28 +48,25 @@ class contextlist extends contextlist_base { $fields = \context_helper::get_preload_record_columns_sql('ctx'); if ($fieldname = $this->guess_id_field_from_sql($sql)) { - if (is_int($fieldname)) { + if (is_numeric($fieldname)) { $wrapper = " - SELECT - {$fields} + SELECT {$fields} FROM {context} ctx WHERE ctx.id = :fieldvalue"; $params = ['fieldvalue' => $fieldname]; } else { // Able to guess a field name. $wrapper = " - SELECT - {$fields} + SELECT {$fields} FROM {context} ctx JOIN ({$sql}) target ON ctx.id = target.{$fieldname}"; } } else { // No field name available. Fall back on a potentially slower version. $wrapper = " - SELECT - {$fields} + SELECT {$fields} FROM {context} ctx - WHERE ctx.id IN ({$sql})"; + WHERE ctx.id IN ({$sql})"; } $contexts = $DB->get_recordset_sql($wrapper, $params); @@ -138,23 +135,24 @@ class contextlist extends contextlist_base { * Guess the name of the contextid field from the supplied SQL. * * @param string $sql The SQL to guess from - * @return string The field name. + * @return string The field name or a numeric value representing the context id */ protected function guess_id_field_from_sql($sql) { // Get the list of relevant words from the SQL Query. // We explode the SQL by the space character, then trim any extra whitespace (e.g. newlines), before we filter // empty value, and finally we re-index the array. - $words = array_map('trim', explode(' ', $sql)); + $sql = rtrim($sql, ';'); + $words = array_map('trim', preg_split('/\s+/', $sql)); $words = array_filter($words, function($word) { return $word !== ''; }); $words = array_values($words); - if ($firstfrom = array_search('FROM', $words)) { + if ($firstfrom = array_search('FROM', array_map('strtoupper', $words))) { // Found a FROM keyword. // Select the previous word. $fieldname = $words[$firstfrom - 1]; - if (is_int($fieldname)) { + if (is_numeric($fieldname)) { return $fieldname; } @@ -164,6 +162,14 @@ class contextlist extends contextlist_base { } return $fieldname; + + } else if ((count($words) == 1) && (is_numeric($words[0]))) { + // Not a real SQL, just a single numerical value - such as one returned by {@link self::add_system_context()}. + return $words[0]; + + } else if ((count($words) == 2) && (strtoupper($words[0]) === 'SELECT') && (is_numeric($words[1]))) { + // SQL returning a constant numerical value. + return $words[1]; } return ''; diff --git a/privacy/tests/contextlist_test.php b/privacy/tests/contextlist_test.php index 736e523ef5d..a247b3d3ab9 100644 --- a/privacy/tests/contextlist_test.php +++ b/privacy/tests/contextlist_test.php @@ -105,4 +105,85 @@ class contextlist_test extends advanced_testcase { $this->assertContains(\context_user::instance($user1->id)->id, $contexts); $this->assertContains(\context_user::instance($user2->id)->id, $contexts); } + + /** + * Test {@link \core_privacy\local\request\contextlist::test_guess_id_field_from_sql()} implementation. + * + * @dataProvider data_guess_id_field_from_sql + * @param string $sql Input SQL we try to extract the context id field name from. + * @param string $expected Expected detected value. + */ + public function test_guess_id_field_from_sql($sql, $expected) { + + $rc = new \ReflectionClass(contextlist::class); + $rcm = $rc->getMethod('guess_id_field_from_sql'); + $rcm->setAccessible(true); + $actual = $rcm->invoke(new contextlist(), $sql); + + $this->assertEquals($expected, $actual, 'Unable to guess context id field in: '.$sql); + } + + /** + * Provides data sets for {@link self::test_guess_id_field_from_sql()}. + * + * @return array + */ + public function data_guess_id_field_from_sql() { + return [ + 'easy' => [ + 'SELECT contextid FROM {foo}', + 'contextid', + ], + 'with_distinct' => [ + 'SELECT DISTINCT contextid FROM {foo}', + 'contextid', + ], + 'with_dot' => [ + 'SELECT cx.id FROM {foo} JOIN {context} cx ON blahblahblah', + 'id', + ], + 'letter_case_does_not_matter' => [ + 'Select ctxid From {foo} Where bar = ?', + 'ctxid', + ], + 'alias' => [ + 'SELECT foo.contextid AS ctx FROM {bar} JOIN {foo} ON bar.id = foo.barid', + 'ctx', + ], + 'tabs' => [ + "SELECT\tctxid\t\tFROM foo f", + 'ctxid', + ], + 'whitespace' => [ + "SELECT + ctxid\t + \tFROM foo f", + 'ctxid', + ], + 'just_number' => [ + '1', + '1', + ], + 'select_number' => [ + 'SELECT 2', + '2', + ], + 'select_number_with_semicolon' => [ + 'SELECT 3;', + '3', + ], + 'select_number_from_table' => [ + 'SELECT 4 FROM users', + '4', + ], + 'invalid_1' => [ + 'SELECT 1+1', + '', + ], + 'invalid_2' => [ + 'muhehe', + '', + ], + ]; + } } From d6dcfee9f58f2d95f0f894b63eaf865af25e3845 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Wed, 4 Jul 2018 09:46:53 +0200 Subject: [PATCH 3/3] MDL-62619 privacy: Fix a missing alias in the blog provider class The new implementation revealed another existing bug that did not expose before due to coincidently same named table alias in the outer query. --- blog/classes/privacy/provider.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blog/classes/privacy/provider.php b/blog/classes/privacy/provider.php index fd30fd51316..f37d76ed0c1 100644 --- a/blog/classes/privacy/provider.php +++ b/blog/classes/privacy/provider.php @@ -135,7 +135,7 @@ class provider implements if ($DB->record_exists('blog_external', ['userid' => $userid])) { $sql = " SELECT ctx.id - FROM {context} + FROM {context} ctx WHERE ctx.contextlevel = :ctxlevel AND ctx.instanceid = :ctxuserid"; $params = [