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

MDL-62619 privacy: Fix some bits of performance improvements

Browse Source 
* 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.
This commit is contained in:
David Mudrák 2018-07-03 22:28:35 +02:00
parent d7cb7550c7
commit cea03cbd43
2 changed files with 99 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
privacy/classes/local/request/contextlist.php
View File

@ -48,26 +48,23 @@ 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})";
}
@ -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(string $sql) : string {
// 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 '';

81
privacy/tests/contextlist_test.php
View File

@ -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',
'',
],
];
}
}