Skip to content

Commit

Permalink
MDL-28021 Completion system can create inconsistent database rows
Browse files Browse the repository at this point in the history 
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 moodle#3

Conflicts:

	lib/db/upgrade.php
	version.php
  • Loading branch information
@sammarshallou @stronk7
sammarshallou authored and stronk7 committed Jun 27, 2011
1 parent 8e0733f commit a854bca
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 19 deletions.
15 changes: 11 additions & 4 deletions lib/completionlib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -935,13 +935,20 @@ public function get_data($cm, $wholecourse=false, $userid=0, $modinfo=null) {
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;
Expand Down
6 changes: 3 additions & 3 deletions lib/db/install.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -367,8 +367,8 @@
<KEY NAME="primary" TYPE="primary" FIELDS="id"/>
</KEYS>
<INDEXES>
<INDEX NAME="coursemoduleid" UNIQUE="false" FIELDS="coursemoduleid" COMMENT="For quick access via course-module (e.g. when displaying course module settings page and we need to determine whether anyone has completed it)." NEXT="userid"/>
<INDEX NAME="userid" UNIQUE="false" FIELDS="userid" COMMENT="Index on user ID. Used when obtaining completion information for normal course page view." PREVIOUS="coursemoduleid"/>
<INDEX NAME="coursemoduleid" UNIQUE="false" FIELDS="coursemoduleid" COMMENT="For quick access via course-module (e.g. when displaying course module settings page and we need to determine whether anyone has completed it)." NEXT="userid-coursemoduleid"/>
<INDEX NAME="userid-coursemoduleid" UNIQUE="true" FIELDS="userid, coursemoduleid" PREVIOUS="coursemoduleid"/>
</INDEXES>
</TABLE>
<TABLE NAME="course_sections" COMMENT="to define the sections for each course" PREVIOUS="course_modules_completion" NEXT="course_request">
Expand Down Expand Up @@ -2834,4 +2834,4 @@
</KEYS>
</TABLE>
</TABLES>
</XMLDB>
</XMLDB>
55 changes: 55 additions & 0 deletions lib/db/upgrade.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6572,6 +6572,61 @@ function xmldb_main_upgrade($oldversion) {
upgrade_main_savepoint(true, 2011062400.02);
}

if ($oldversion < 2011062400.03) {
// 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, 2011062400.03);
}

return true;
}

Expand Down
39 changes: 28 additions & 11 deletions lib/simpletest/testcompletionlib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

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',
Expand Down Expand Up @@ -452,24 +453,40 @@ function test_get_data() {
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();
}

Expand Down
2 changes: 1 addition & 1 deletion version.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@



$version = 2011062400.02; // YYYYMMDD = weekly release date of this DEV branch
$version = 2011062400.03; // YYYYMMDD = weekly release date of this DEV branch
// RR = release increments - 00 in DEV branches
// .XX = incremental changes

Expand Down

0 comments on commit a854bca

Please sign in to comment.