From 9d5c12bee446222a9c3c750a8688ff23f745958f Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Tue, 13 Sep 2016 03:38:56 +0200 Subject: [PATCH 1/3] MDL-29332 dml: new sql_equal() to force cs/ci behavior on varchar matching --- lib/dml/moodle_database.php | 27 +++++++++++++++++++++++ lib/dml/mssql_native_moodle_database.php | 18 +++++++++++++++ lib/dml/mysqli_native_moodle_database.php | 18 +++++++++++++++ lib/dml/sqlsrv_native_moodle_database.php | 18 +++++++++++++++ lib/upgrade.txt | 2 ++ 5 files changed, 83 insertions(+) diff --git a/lib/dml/moodle_database.php b/lib/dml/moodle_database.php index e78005d3046..bf37ffdbdbb 100644 --- a/lib/dml/moodle_database.php +++ b/lib/dml/moodle_database.php @@ -2050,6 +2050,33 @@ abstract class moodle_database { return $this->sql_order_by_text($fieldname, $numchars); } + /** + * Returns an equal (=) or not equal (<>) part of a query. + * + * Note the use of this method may lead to slower queries (full scans) so + * use it only when needed and against already reduced data sets. + * + * @since Moodle 3.2 + * + * @param string $fieldname Usually the name of the table column. + * @param string $param Usually the bound query parameter (?, :named). + * @param bool $casesensitive Use case sensitive search when set to true (default). + * @param bool $accentsensitive Use accent sensitive search when set to true (default). (not all databases support accent insensitive) + * @param bool $notequal True means not equal (<>) + * @return string The SQL code fragment. + */ + public function sql_equal($fieldname, $param, $casesensitive = true, $accentsensitive = true, $notequal = false) { + // Note that, by default, it's assumed that the correct sql equal operations are + // case sensitive. Only databases not observing this behavior must override the method. + // Also, accent sensitiveness only will be handled by databases supporting it. + $equalop = $notequal ? '<>' : '='; + if ($casesensitive) { + return "$fieldname $equalop $param"; + } else { + return "LOWER($fieldname) $equalop LOWER($param)"; + } + } + /** * Returns 'LIKE' part of a query. * diff --git a/lib/dml/mssql_native_moodle_database.php b/lib/dml/mssql_native_moodle_database.php index fd1721abfb0..7215308c33b 100644 --- a/lib/dml/mssql_native_moodle_database.php +++ b/lib/dml/mssql_native_moodle_database.php @@ -1221,6 +1221,24 @@ class mssql_native_moodle_database extends moodle_database { return $this->collation; } + public function sql_equal($fieldname, $param, $casesensitive = true, $accentsensitive = true, $notequal = false) { + $equalop = $notequal ? '<>' : '='; + $collation = $this->get_collation(); + + if ($casesensitive) { + $collation = str_replace('_CI', '_CS', $collation); + } else { + $collation = str_replace('_CS', '_CI', $collation); + } + if ($accentsensitive) { + $collation = str_replace('_AI', '_AS', $collation); + } else { + $collation = str_replace('_AS', '_AI', $collation); + } + + return "$fieldname COLLATE $collation $equalop $param"; + } + /** * Returns 'LIKE' part of a query. * diff --git a/lib/dml/mysqli_native_moodle_database.php b/lib/dml/mysqli_native_moodle_database.php index 663bd6a6d88..6048cddaf6c 100644 --- a/lib/dml/mysqli_native_moodle_database.php +++ b/lib/dml/mysqli_native_moodle_database.php @@ -1511,6 +1511,24 @@ class mysqli_native_moodle_database extends moodle_database { return ' CAST(' . $fieldname . ' AS DECIMAL(65,7)) '; } + public function sql_equal($fieldname, $param, $casesensitive = true, $accentsensitive = true, $notequal = false) { + $equalop = $notequal ? '<>' : '='; + if ($casesensitive) { + // Current MySQL versions do not support case sensitive and accent insensitive. + return "$fieldname COLLATE utf8_bin $equalop $param"; + } else if ($accentsensitive) { + // Case insensitive and accent sensitive, we can force a binary comparison once all texts are using the same case. + return "LOWER($fieldname) COLLATE utf8_bin $equalop LOWER($param)"; + } else { + // Case insensitive and accent insensitive. All collations are that way, but utf8_bin. + $collation = ''; + if ($this->get_dbcollation() == 'utf8_bin') { + $collation = 'COLLATE utf8_unicode_ci'; + } + return "$fieldname $collation $equalop $param"; + } + } + /** * Returns 'LIKE' part of a query. * diff --git a/lib/dml/sqlsrv_native_moodle_database.php b/lib/dml/sqlsrv_native_moodle_database.php index e2d253f1669..5de4ff06d88 100644 --- a/lib/dml/sqlsrv_native_moodle_database.php +++ b/lib/dml/sqlsrv_native_moodle_database.php @@ -1288,6 +1288,24 @@ class sqlsrv_native_moodle_database extends moodle_database { return $this->collation; } + public function sql_equal($fieldname, $param, $casesensitive = true, $accentsensitive = true, $notequal = false) { + $equalop = $notequal ? '<>' : '='; + $collation = $this->get_collation(); + + if ($casesensitive) { + $collation = str_replace('_CI', '_CS', $collation); + } else { + $collation = str_replace('_CS', '_CI', $collation); + } + if ($accentsensitive) { + $collation = str_replace('_AI', '_AS', $collation); + } else { + $collation = str_replace('_AS', '_AI', $collation); + } + + return "$fieldname COLLATE $collation $equalop $param"; + } + /** * Returns 'LIKE' part of a query. * diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 0b6e23972ce..f453fc8d294 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -78,6 +78,8 @@ information provided here is intended especially for developers. - $CFG->wwwroot: http://example.com/moodle - $CFG->alternateloginurl : /my/super/login.php - Login url will be: http://example.com/moodle/my/super/login.php (moodle root based) +* Database (DML) layer: + - new sql_equal() method available for places where case sensitive/insensitive varchar comparisons are required. === 3.1 === From 03d27c303d621ad44db35bf1d29ce488d16088c8 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Wed, 14 Sep 2016 02:57:06 +0200 Subject: [PATCH 2/3] MDL-29332 tests: Cover the new sql_equal() function - For CS, CI and AS - Not for AI (it's not cross-db) - equal (=) and not equal (<>) - delete old test_sql_binary_equal() now with cross-db solution. --- lib/dml/tests/dml_test.php | 51 ++++++++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/lib/dml/tests/dml_test.php b/lib/dml/tests/dml_test.php index 7851e2e933f..85658f01c48 100644 --- a/lib/dml/tests/dml_test.php +++ b/lib/dml/tests/dml_test.php @@ -3829,7 +3829,7 @@ class core_dml_testcase extends database_driver_testcase { } } - public function test_sql_binary_equal() { + public function test_sql_equal() { $DB = $this->tdb; $dbman = $DB->get_manager(); @@ -3838,20 +3838,51 @@ class core_dml_testcase extends database_driver_testcase { $table->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null); $table->add_field('name', XMLDB_TYPE_CHAR, '255', null, null, null, null); + $table->add_field('name2', XMLDB_TYPE_CHAR, '255', null, null, null, null); $table->add_key('primary', XMLDB_KEY_PRIMARY, array('id')); $dbman->create_table($table); - $DB->insert_record($tablename, array('name'=>'aaa')); - $DB->insert_record($tablename, array('name'=>'aáa')); - $DB->insert_record($tablename, array('name'=>'aäa')); - $DB->insert_record($tablename, array('name'=>'bbb')); - $DB->insert_record($tablename, array('name'=>'BBB')); + $DB->insert_record($tablename, array('name' => 'one', 'name2' => 'one')); + $DB->insert_record($tablename, array('name' => 'ONE', 'name2' => 'ONE')); + $DB->insert_record($tablename, array('name' => 'two', 'name2' => 'TWO')); + $DB->insert_record($tablename, array('name' => 'öne', 'name2' => 'one')); + $DB->insert_record($tablename, array('name' => 'öne', 'name2' => 'ÖNE')); - $records = $DB->get_records_sql("SELECT * FROM {{$tablename}} WHERE name = ?", array('bbb')); - $this->assertEquals(1, count($records), 'SQL operator "=" is expected to be case sensitive'); + // Case sensitive and accent sensitive (equal and not equal). + $sql = "SELECT * FROM {{$tablename}} WHERE " . $DB->sql_equal('name', '?', true, true, false); + $records = $DB->get_records_sql($sql, array('one')); + $this->assertCount(1, $records); + $sql = "SELECT * FROM {{$tablename}} WHERE " . $DB->sql_equal('name', ':name', true, true, true); + $records = $DB->get_records_sql($sql, array('name' => 'one')); + $this->assertCount(4, $records); + // And with column comparison instead of params. + $sql = "SELECT * FROM {{$tablename}} WHERE " . $DB->sql_equal('name', 'name2', true, true, false); + $records = $DB->get_records_sql($sql); + $this->assertCount(2, $records); - $records = $DB->get_records_sql("SELECT * FROM {{$tablename}} WHERE name = ?", array('aaa')); - $this->assertEquals(1, count($records), 'SQL operator "=" is expected to be accent sensitive'); + // Case insensitive and accent sensitive (equal and not equal). + $sql = "SELECT * FROM {{$tablename}} WHERE " . $DB->sql_equal('name', '?', false, true, false); + $records = $DB->get_records_sql($sql, array('one')); + $this->assertCount(2, $records); + $sql = "SELECT * FROM {{$tablename}} WHERE " . $DB->sql_equal('name', ':name', false, true, true); + $records = $DB->get_records_sql($sql, array('name' => 'one')); + $this->assertCount(3, $records); + // And with column comparison instead of params. + $sql = "SELECT * FROM {{$tablename}} WHERE " . $DB->sql_equal('name', 'name2', false, true, false); + $records = $DB->get_records_sql($sql); + $this->assertCount(4, $records); + + // TODO: Accent insensitive is not cross-db, only some drivers support it, so just verify the queries work. + $sql = "SELECT * FROM {{$tablename}} WHERE " . $DB->sql_equal('name', '?', true, false); + $records = $DB->get_records_sql($sql, array('one')); + $this->assertGreaterThanOrEqual(1, count($records)); // At very least, there is 1 record with CS/AI "one". + $sql = "SELECT * FROM {{$tablename}} WHERE " . $DB->sql_equal('name', '?', false, false); + $records = $DB->get_records_sql($sql, array('one')); + $this->assertGreaterThanOrEqual(2, count($records)); // At very least, there are 2 records with CI/AI "one". + // And with column comparison instead of params. + $sql = "SELECT * FROM {{$tablename}} WHERE " . $DB->sql_equal('name', 'name2', false, false); + $records = $DB->get_records_sql($sql); + $this->assertGreaterThanOrEqual(4, count($records)); // At very least, there are 4 records with CI/AI names matching. } public function test_sql_like() { From e7ba25f1b13f1e5510e17d726b3c9f1f81cc27a6 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Mon, 19 Sep 2016 10:37:43 +0800 Subject: [PATCH 3/3] MDL-29332 question: calculated variables may differ in case only --- lib/db/install.xml | 5 +---- lib/db/upgrade.php | 15 +++++++++++++++ question/engine/datalib.php | 15 ++++++++++++--- question/type/calculated/questiontype.php | 8 ++++---- version.php | 2 +- 5 files changed, 33 insertions(+), 12 deletions(-) diff --git a/lib/db/install.xml b/lib/db/install.xml index 51265b0ee07..c038172af5c 100644 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -1,5 +1,5 @@ - @@ -1353,9 +1353,6 @@ - - - diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index c413aa6a96c..557f93c5dc0 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -2177,5 +2177,20 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2016082200.00); } + if ($oldversion < 2016091501.00) { + + // Define index attemptstepid-name (unique) to be dropped from question_attempt_step_data. + $table = new xmldb_table('question_attempt_step_data'); + $index = new xmldb_index('attemptstepid-name', XMLDB_INDEX_UNIQUE, array('attemptstepid', 'name')); + + // Conditionally launch drop index attemptstepid-name. + if ($dbman->index_exists($table, $index)) { + $dbman->drop_index($table, $index); + } + + // Main savepoint reached. + upgrade_main_savepoint(true, 2016091501.00); + } + return true; } diff --git a/question/engine/datalib.php b/question/engine/datalib.php index 0731252ff0b..e486e4699ee 100644 --- a/question/engine/datalib.php +++ b/question/engine/datalib.php @@ -287,10 +287,19 @@ class question_engine_data_mapper { */ public function update_question_attempt_metadata(question_attempt $qa, array $names) { global $DB; - list($condition, $params) = $DB->get_in_or_equal($names); - $params[] = $qa->get_step(0)->get_id(); + if (!$names) { + return []; + } + // Use case-sensitive function sql_equal() and not get_in_or_equal(). + // Some databases may use case-insensitive collation, we don't want to delete 'X' instead of 'x'. + $sqls = []; + $params = [$qa->get_step(0)->get_id()]; + foreach ($names as $name) { + $sqls[] = $DB->sql_equal('name', '?'); + $params[] = $name; + } $DB->delete_records_select('question_attempt_step_data', - 'name ' . $condition . ' AND attemptstepid = ?', $params); + 'attemptstepid = ? AND (' . join(' OR ', $sqls) . ')', $params); return $this->insert_question_attempt_metadata($qa, $names); } diff --git a/question/type/calculated/questiontype.php b/question/type/calculated/questiontype.php index 896def424f5..8ab1e5e6164 100644 --- a/question/type/calculated/questiontype.php +++ b/question/type/calculated/questiontype.php @@ -281,7 +281,7 @@ class qtype_calculated extends question_type { if ($sharedatasetdefs = $DB->get_records_select( 'question_dataset_definitions', "type = '1' - AND name = ? + AND " . $DB->sql_equal('name', '?') . " AND category = ? ORDER BY id DESC ", array($dataset->name, $question->category) )) { // So there is at least one. @@ -1400,7 +1400,7 @@ class qtype_calculated extends question_type { // can manage to automatically take care of // some possible realtime concurrence. if ($olderdatasetdefs = $DB->get_records_select('question_dataset_definitions', - "type = ? AND name = ? AND category = ? AND id < ? + "type = ? AND " . $DB->sql_equal('name', '?') . " AND category = ? AND id < ? ORDER BY id DESC", array($datasetdef->type, $datasetdef->name, $datasetdef->category, $datasetdef->id))) { @@ -1484,7 +1484,7 @@ class qtype_calculated extends question_type { // Construct question local options. $sql = "SELECT a.* FROM {question_dataset_definitions} a, {question_datasets} b - WHERE a.id = b.datasetdefinition AND a.type = '1' AND b.question = ? AND a.name = ?"; + WHERE a.id = b.datasetdefinition AND a.type = '1' AND b.question = ? AND " . $DB->sql_equal('a.name', '?'); $currentdatasetdef = $DB->get_record_sql($sql, array($form->id, $name)); if (!$currentdatasetdef) { $currentdatasetdef = new stdClass(); @@ -1506,7 +1506,7 @@ class qtype_calculated extends question_type { WHERE a.id = b.datasetdefinition AND a.type = '1' AND a.category = ? - AND a.name = ?", array($form->category, $name)); + AND " . $DB->sql_equal('a.name', '?'), array($form->category, $name)); $type = 1; $key = "{$type}-{$form->category}-{$name}"; if (!empty($categorydatasetdefs)) { diff --git a/version.php b/version.php index 6ff6fe99793..7eea9fce0ac 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2016091500.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2016091501.00; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes.