mirror_php/moodle
1
0
Fork 0
mirror of https://github.com/moodle/moodle.git synced 2025-04-21 00:12:56 +02:00
Code Issues Projects Releases Wiki Activity

MDL-34251 question engine: possible infinite loop loading usages

Browse Source 
In the case where either a question_attempt had not steps, or a
question_usage had not question_attempts, the load_from_records methods
could get stuck in an infinite loop.

This fix ensures that does not happen, with unit tests to verify it. At
the same time, I noticed an error in the existing tests, which this
patch fixes.
This commit is contained in:
Tim Hunt 2012-07-17 17:32:56 +01:00
parent 3294034b80
commit dd7aa58386
3 changed files with 81 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
question/engine/questionattempt.php
View File

@ -1213,6 +1213,15 @@ class question_attempt {
$qa->behaviour = question_engine::make_behaviour(
$record->behaviour, $qa, $preferredbehaviour);
// If attemptstepid is null (which should not happen, but has happened
// due to corrupt data, see MDL-34251) then the current pointer in $records
// will not be advanced in the while loop below, and we get stuck in an
// infinite loop, since this method is supposed to always consume at
// least one record. Therefore, in this case, advance the record here.
if (is_null($record->attemptstepid)) {
$records->next();
}
$i = 0;
while ($record && $record->questionattemptid == $questionattemptid && !is_null($record->attemptstepid)) {
$qa->steps[$i] = question_attempt_step::load_from_records($records, $record->attemptstepid);

8
question/engine/questionusage.php
View File

@ -714,6 +714,14 @@ class question_usage_by_activity {
$quba->observer = new question_engine_unit_of_work($quba);
// If slot is null then the current pointer in $records will not be
// advanced in the while loop below, and we get stuck in an infinite loop,
// since this method is supposed to always consume at least one record.
// Therefore, in this case, advance the record here.
if (is_null($record->slot)) {
$records->next();
}
while ($record && $record->qubaid == $qubaid && !is_null($record->slot)) {
$quba->questionattempts[$record->slot] =
question_attempt::load_from_records($records,

68
question/engine/tests/questionusagebyactivity_test.php
View File

@ -169,16 +169,17 @@ class question_usage_by_activity_test extends advanced_testcase {
*/
class question_usage_db_test extends data_loading_method_test_base {
public function test_load() {
$scid = context_system::instance()->id;
$records = new question_test_recordset(array(
array('qubaid', 'contextid', 'component', 'preferredbehaviour',
'questionattemptid', 'contextid', 'questionusageid', 'slot',
'questionattemptid', 'questionusageid', 'slot',
'behaviour', 'questionid', 'variant', 'maxmark', 'minfraction', 'flagged',
'questionsummary', 'rightanswer', 'responsesummary', 'timemodified',
'attemptstepid', 'sequencenumber', 'state', 'fraction',
'timecreated', 'userid', 'name', 'value'),
array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 1, 0, 'todo', null, 1256233700, 1, null, null),
array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 2, 1, 'todo', null, 1256233705, 1, 'answer', '1'),
array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 5, 2, 'gradedright', 1.0000000, 1256233720, 1, '-finish', '1'),
array(1, $scid, 'unit_test', 'interactive', 1, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 1, 0, 'todo', null, 1256233700, 1, null, null),
array(1, $scid, 'unit_test', 'interactive', 1, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 2, 1, 'todo', null, 1256233705, 1, 'answer', '1'),
array(1, $scid, 'unit_test', 'interactive', 1, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 5, 2, 'gradedright', 1.0000000, 1256233720, 1, '-finish', '1'),
));
$question = test_question_maker::make_question('truefalse', 'true');
@ -221,4 +222,63 @@ class question_usage_db_test extends data_loading_method_test_base {
$this->assertEquals(1, $step->get_user_id());
$this->assertEquals(array('-finish' => '1'), $step->get_all_data());
}
public function test_load_data_no_steps() {
// The code had a bug where if one question_attempt had no steps,
// load_from_records got stuck in an infinite loop. This test is to
// verify that no longer happens.
$scid = context_system::instance()->id;
$records = new question_test_recordset(array(
array('qubaid', 'contextid', 'component', 'preferredbehaviour',
'questionattemptid', 'questionusageid', 'slot',
'behaviour', 'questionid', 'variant', 'maxmark', 'minfraction', 'flagged',
'questionsummary', 'rightanswer', 'responsesummary', 'timemodified',
'attemptstepid', 'sequencenumber', 'state', 'fraction',
'timecreated', 'userid', 'name', 'value'),
array(1, $scid, 'unit_test', 'interactive', 1, 1, 1, 'interactive', 0, 1, 1.0000000, 0.0000000, 0, 'This question is missing. Unable to display anything.', '', '', 0, null, null, null, null, null, null, null, null),
array(1, $scid, 'unit_test', 'interactive', 2, 1, 2, 'interactive', 0, 1, 1.0000000, 0.0000000, 0, 'This question is missing. Unable to display anything.', '', '', 0, null, null, null, null, null, null, null, null),
array(1, $scid, 'unit_test', 'interactive', 3, 1, 3, 'interactive', 0, 1, 1.0000000, 0.0000000, 0, 'This question is missing. Unable to display anything.', '', '', 0, null, null, null, null, null, null, null, null),
));
question_bank::start_unit_test();
$quba = question_usage_by_activity::load_from_records($records, 1);
question_bank::end_unit_test();
$this->assertEquals('unit_test', $quba->get_owning_component());
$this->assertEquals(1, $quba->get_id());
$this->assertInstanceOf('question_engine_unit_of_work', $quba->get_observer());
$this->assertEquals('interactive', $quba->get_preferred_behaviour());
$this->assertEquals(array(1, 2, 3), $quba->get_slots());
$qa = $quba->get_question_attempt(1);
$this->assertEquals(0, $qa->get_num_steps());
}
public function test_load_data_no_qas() {
// The code had a bug where if a question_usage had no question_attempts,
// load_from_records got stuck in an infinite loop. This test is to
// verify that no longer happens.
$scid = context_system::instance()->id;
$records = new question_test_recordset(array(
array('qubaid', 'contextid', 'component', 'preferredbehaviour',
'questionattemptid', 'questionusageid', 'slot',
'behaviour', 'questionid', 'variant', 'maxmark', 'minfraction', 'flagged',
'questionsummary', 'rightanswer', 'responsesummary', 'timemodified',
'attemptstepid', 'sequencenumber', 'state', 'fraction',
'timecreated', 'userid', 'name', 'value'),
array(1, $scid, 'unit_test', 'interactive', null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null),
));
question_bank::start_unit_test();
$quba = question_usage_by_activity::load_from_records($records, 1);
question_bank::end_unit_test();
$this->assertEquals('unit_test', $quba->get_owning_component());
$this->assertEquals(1, $quba->get_id());
$this->assertInstanceOf('question_engine_unit_of_work', $quba->get_observer());
$this->assertEquals('interactive', $quba->get_preferred_behaviour());
$this->assertEquals(array(), $quba->get_slots());
}
}