From f114cff65fa988d91908cb2ecc753582626310e7 Mon Sep 17 00:00:00 2001 From: sam marshall Date: Fri, 24 Jun 2011 14:02:04 +0100 Subject: [PATCH] MDL-28021 Completion system can create inconsistent database rows This change includes: (1) update deletes older versions of inconsistent rows (2) update drops one index and replaces it with a new unique index (3) fixed to ensure that when it decides whether to insert or update it uses current database state and not cached info (4) unit tests updated to test #3 --- lib/completionlib.php | 15 +++++--- lib/db/install.xml | 4 +-- lib/db/upgrade.php | 54 ++++++++++++++++++++++++++++ lib/simpletest/testcompletionlib.php | 39 ++++++++++++++------ version.php | 2 +- 5 files changed, 96 insertions(+), 18 deletions(-) diff --git a/lib/completionlib.php b/lib/completionlib.php index 4c8467bd0b9..cce41d0d922 100644 --- a/lib/completionlib.php +++ b/lib/completionlib.php @@ -935,13 +935,20 @@ class completion_info { function internal_set_data($cm, $data) { global $USER, $SESSION, $DB; - if ($data->id) { - // Has real (nonzero) id meaning that a database row exists - $DB->update_record('course_modules_completion', $data); - } else { + $transaction = $DB->start_delegated_transaction(); + if (!$data->id) { + // Check there isn't really a row + $data->id = $DB->get_field('course_modules_completion', 'id', + array('coursemoduleid'=>$data->coursemoduleid, 'userid'=>$data->userid)); + } + if (!$data->id) { // Didn't exist before, needs creating $data->id = $DB->insert_record('course_modules_completion', $data); + } else { + // Has real (nonzero) id meaning that a database row exists, update + $DB->update_record('course_modules_completion', $data); } + $transaction->allow_commit(); if ($data->userid == $USER->id) { $SESSION->completioncache[$cm->course][$cm->id] = $data; diff --git a/lib/db/install.xml b/lib/db/install.xml index 27b3c1e5b19..c83310c103f 100644 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -367,8 +367,8 @@ - - + + diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index b47810c0c2c..54bd086443e 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -6062,6 +6062,60 @@ WHERE gradeitemid IS NOT NULL AND grademax IS NOT NULL"); upgrade_main_savepoint(true, 2011022100.01); } + if ($oldversion < 2011033003.09) { + // Completion system has issue in which possible duplicate rows are + // added to the course_modules_completion table. This change deletes + // the older version of duplicate rows and replaces an index with a + // unique one so it won't happen again. + + // This would have been a single query but because MySQL is a PoS + // and can't do subqueries in DELETE, I have made it into two. The + // system is unlikely to run out of memory as only IDs are stored in + // the array. + + // Find all rows cmc1 where there is another row cmc2 with the + // same user id and the same coursemoduleid, but a higher id (=> newer, + // meaning that cmc1 is an older row). + $rs = $DB->get_recordset_sql(" +SELECT DISTINCT + cmc1.id +FROM + {course_modules_completion} cmc1 + JOIN {course_modules_completion} cmc2 + ON cmc2.userid = cmc1.userid + AND cmc2.coursemoduleid = cmc1.coursemoduleid + AND cmc2.id > cmc1.id"); + $deleteids = array(); + foreach ($rs as $row) { + $deleteids[] = $row->id; + } + $rs->close(); + // Note: SELECT part performance tested on table with ~7m + // rows of which ~15k match, only took 30 seconds so probably okay. + + // Delete all those rows + $DB->delete_records_list('course_modules_completion', 'id', $deleteids); + + // Define index userid (not unique) to be dropped form course_modules_completion + $table = new xmldb_table('course_modules_completion'); + $index = new xmldb_index('userid', XMLDB_INDEX_NOTUNIQUE, array('userid')); + + // Conditionally launch drop index userid + if ($dbman->index_exists($table, $index)) { + $dbman->drop_index($table, $index); + } + + // Define index userid-coursemoduleid (unique) to be added to course_modules_completion + $index = new xmldb_index('userid-coursemoduleid', XMLDB_INDEX_UNIQUE, + array('userid', 'coursemoduleid')); + + // Conditionally launch add index userid-coursemoduleid + if (!$dbman->index_exists($table, $index)) { + $dbman->add_index($table, $index); + } + + upgrade_main_savepoint(true, 2011033003.09); + } return true; } diff --git a/lib/simpletest/testcompletionlib.php b/lib/simpletest/testcompletionlib.php index 66fc9cb7e52..f4288fd9eb0 100644 --- a/lib/simpletest/testcompletionlib.php +++ b/lib/simpletest/testcompletionlib.php @@ -6,6 +6,7 @@ require_once($CFG->libdir.'/completionlib.php'); global $DB; Mock::generate(get_class($DB), 'mock_database'); +Mock::generate('moodle_transaction', 'mock_transaction'); Mock::generatePartial('completion_info','completion_cutdown', array('delete_all_state','get_tracked_users','update_state', @@ -452,24 +453,40 @@ WHERE function test_internal_set_data() { global $DB,$SESSION; - $cm=(object)array('course'=>42,'id'=>13); - $c=new completion_info((object)array('id'=>42)); + $cm = (object)array('course' => 42,'id' => 13); + $c = new completion_info((object)array('id' => 42)); // 1) Test with new data - $data=(object)array('id'=>0,'userid'=>314159); - $DB->setReturnValueAt(0,'insert_record',4); - $DB->expectAt(0,'insert_record',array('course_modules_completion',$data)); - $c->internal_set_data($cm,$data); - $this->assertEqual(4,$data->id); - $this->assertEqual(array(42=>array(13=>$data)),$SESSION->completioncache); + $data = (object)array('id'=>0, 'userid' => 314159, 'coursemoduleid' => 99); + $DB->setReturnValueAt(0, 'start_delegated_transaction', new mock_transaction()); + $DB->setReturnValueAt(0, 'insert_record', 4); + $DB->expectAt(0, 'get_field', array('course_modules_completion', 'id', + array('coursemoduleid' => 99, 'userid' => 314159))); + $DB->expectAt(0, 'insert_record', array('course_modules_completion', $data)); + $c->internal_set_data($cm, $data); + $this->assertEqual(4, $data->id); + $this->assertEqual(array(42 => array(13 => $data)), $SESSION->completioncache); // 2) Test with existing data and for different user (not cached) unset($SESSION->completioncache); - $d2=(object)array('id'=>7,'userid'=>17); - $DB->expectAt(0,'update_record',array('course_modules_completion',$d2)); - $c->internal_set_data($cm,$d2); + $d2 = (object)array('id' => 7, 'userid' => 17, 'coursemoduleid' => 66); + $DB->setReturnValueAt(1, 'start_delegated_transaction', new mock_transaction()); + $DB->expectAt(0,'update_record', array('course_modules_completion', $d2)); + $c->internal_set_data($cm, $d2); $this->assertFalse(isset($SESSION->completioncache)); + // 3) Test where it THINKS the data is new (from cache) but actually + // in the database it has been set since + // 1) Test with new data + $data = (object)array('id'=>0, 'userid' => 314159, 'coursemoduleid' => 99); + $DB->setReturnValueAt(2, 'start_delegated_transaction', new mock_transaction()); + $DB->setReturnValueAt(1, 'get_field', 13); + $DB->expectAt(1, 'get_field', array('course_modules_completion', 'id', + array('coursemoduleid' => 99, 'userid' => 314159))); + $d3 = (object)array('id' => 13, 'userid' => 314159, 'coursemoduleid' => 99); + $DB->expectAt(1,'update_record', array('course_modules_completion', $d3)); + $c->internal_set_data($cm, $data); + $DB->tally(); } diff --git a/version.php b/version.php index 06757b0563e..175573d1920 100644 --- a/version.php +++ b/version.php @@ -30,7 +30,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2011033003.08; // 20110330 = branching date YYYYMMDD - do not modify! +$version = 2011033003.09; // 20110330 = branching date YYYYMMDD - do not modify! // RR = release version - do not change in weeklies // .XX = incremental changes