L-electron-Rare/moodle
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

MDL-85640 questions: Add unique index on question_versions

Browse Source 
Adds a unique index on questionbankentryid and version in the
question_versions table, to prevent various issues and query
performance.

Although there should not be, it's possible that there are existing
versions that violate this index, so the upgrade step finds these and
renumbers them.
This commit is contained in:
Mark Johnson
2026-02-03 10:59:52 +00:00
parent 5f23e7ec57
commit 8a020b3637
6 changed files with 522 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
public/lib/db/install.xml
+4 -1
View File
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8" ?>
<XMLDB PATH="lib/db" VERSION="20251205" COMMENT="XMLDB file for core Moodle tables"
<XMLDB PATH="lib/db" VERSION="20260112" COMMENT="XMLDB file for core Moodle tables"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="../../lib/xmldb/xmldb.xsd"
>
@@ -1508,6 +1508,9 @@
<KEY NAME="questionbankentryid" TYPE="foreign" FIELDS="questionbankentryid" REFTABLE="question_bank_entries" REFFIELDS="id"/>
<KEY NAME="questionid" TYPE="foreign" FIELDS="questionid" REFTABLE="question" REFFIELDS="id"/>
</KEYS>
<INDEXES>
<INDEX NAME="questionbankentryid-version" UNIQUE="true" FIELDS="questionbankentryid, version"/>
</INDEXES>
</TABLE>
<TABLE NAME="question_references" COMMENT="Records where a specific question is used.">
<FIELDS>
public/lib/db/upgrade.php
+13
View File
@@ -1670,5 +1670,18 @@ function xmldb_main_upgrade($oldversion) {
upgrade_main_savepoint(true, 2026011600.01);
}
if ($oldversion < 2026013000.01) {
core_question\versions::resolve_unique_version_violations();
// Define index questionbankentryid-version (unique) to be added to question_versions.
$table = new xmldb_table('question_versions');
$index = new xmldb_index('questionbankentryid-version', XMLDB_INDEX_UNIQUE, ['questionbankentryid', 'version']);
// Conditionally launch add index questionbankentryid-version.
if (!$dbman->index_exists($table, $index)) {
$dbman->add_index($table, $index);
}
upgrade_main_savepoint(true, 2026013000.01);
}
return true;
}
public/question/classes/local/bank/question_bank_helper.php
+2
View File
@@ -20,7 +20,9 @@ use cm_info;
use context;
use context_course;
use core\context_helper;
use core\di;
use core\task\manager;
use moodle_database;
use moodle_url;
use stdClass;
public/question/classes/versions.php
+81
View File
@@ -16,6 +16,10 @@
namespace core_question;
use core\attribute\deprecated;
use core\di;
use moodle_database;
/**
* Methods for finding and manipulating question versions
*
@@ -73,4 +77,81 @@ class versions {
['id' => $questionbankentryid],
);
}
/**
* Given an array of question version records, safely renumber them and any references.
*
* To guarantee we will not overlap with existing versions, we first find the highest version number and the count of versions,
* and pick the higher of the two. We then re-number versions in order starting from that number + 1. This ensures
* we avoid collisions with any existing version number.
*
* This means if we start with versions with the following id => version mapping:
* 10 => 1, 11 => 1, 12 => 3, 13 => 4, 14 => 4, 15 => 5.
* We end up with:
* 10 => 7, 11 => 8, 12 => 9, 13 => 10, 14 => 11, 15 => 12.
*
* @param array $versions question_version records (with at least id, version fields) to be renumbered.
* @return array Objects with oldversion and newversion properties, keyed by version ID.
* @todo Deprecate in 6.0 MDL-87844 for removal in 7.0 MDL-87845.
*/
public static function renumber_versions(array $versions) {
$renumbers = [];
$nextversion = array_reduce(
$versions,
fn($highest, $version) => max($highest, $version->version),
count($versions),
);
foreach ($versions as $version) {
$nextversion++;
$renumbers[$version->id] = (object) ['oldversion' => $version->version, 'newversion' => $nextversion];
}
return $renumbers;
}
/**
* Find any question bank entries which have multiple versions with the same number, and renumber the versions.
*
* @todo Deprecate in 6.0 MDL-87844 for removal in 7.0 MDL-87845.
*/
public static function resolve_unique_version_violations(): void {
$db = di::get(moodle_database::class);
// Find questionbankentryid-version uniqueness violations.
$violations = $db->get_fieldset_sql("
SELECT DISTINCT questionbankentryid
FROM {question_versions}
GROUP BY questionbankentryid, version
HAVING COUNT(1) > 1
");
foreach ($violations as $questionbankentryid) {
// Renumber the entry's versions based on the order in which they were created.
$versions = $db->get_records_sql(
"SELECT qv.id, qv.version, q.timecreated
FROM {question_versions} qv
JOIN {question} q ON qv.questionid = q.id
WHERE qv.questionbankentryid = :entryid
ORDER BY timecreated, id",
['entryid' => $questionbankentryid],
);
$renumbers = self::renumber_versions($versions);
// Now update the records in reverse order (highest new version first). This means if there are references to a
// duplicated version number, those references will end up pointing to the latest version that had that number.
foreach (array_reverse($renumbers, true) as $id => $renumber) {
$db->set_field(
'question_references',
'version',
$renumber->newversion,
['questionbankentryid' => $questionbankentryid, 'version' => $renumber->oldversion],
);
$db->set_field(
'question_versions',
'version',
$renumber->newversion,
['id' => $id],
);
}
// Reset the nextversion field for the question bank entry based on the new highest version.
$db->set_field('question_bank_entries', 'nextversion', null, ['id' => $questionbankentryid]);
self::get_next_version($questionbankentryid, false);
}
}
}
public/question/tests/versions_test.php
+421
View File
@@ -16,7 +16,10 @@
namespace core_question;
use core\attribute\deprecated;
use core\context\module;
use core\di;
use mod_quiz\quiz_settings;
/**
* Unit tests for versions
@@ -115,4 +118,422 @@ final class versions_test extends \advanced_testcase {
// The nextversion value has not changed.
$this->assertEquals(3, $DB->get_field('question_bank_entries', 'nextversion', ['id' => $qbe2]));
}
/**
* Return input version and expected output renumbers for versions::renumber_versions.
*
* @return array
* @todo Deprecate in 6.0 MDL-87844 for removal in 7.0 MDL-87845.
*/
public static function renumber_versions_provider(): array {
return [
'simple sequential versions' => [
'versions' => [
(object) [
'id' => 1,
'version' => 1,
],
(object) [
'id' => 2,
'version' => 2,
],
(object) [
'id' => 3,
'version' => 3,
],
],
'expectedrenumbers' => [
1 => (object) [
'oldversion' => 1,
'newversion' => 4,
],
2 => (object) [
'oldversion' => 2,
'newversion' => 5,
],
3 => (object) [
'oldversion' => 3,
'newversion' => 6,
],
],
],
'duplicate versions' => [
'versions' => [
(object) [
'id' => 1,
'version' => 2,
],
(object) [
'id' => 2,
'version' => 2,
],
(object) [
'id' => 3,
'version' => 2,
],
],
'expectedrenumbers' => [
1 => (object) [
'oldversion' => 2,
'newversion' => 4,
],
2 => (object) [
'oldversion' => 2,
'newversion' => 5,
],
3 => (object) [
'oldversion' => 2,
'newversion' => 6,
],
],
],
'non-sequential duplicate versions' => [
'versions' => [
(object) [
'id' => 1,
'version' => 2,
],
(object) [
'id' => 2,
'version' => 2,
],
(object) [
'id' => 3,
'version' => 3,
],
(object) [
'id' => 4,
'version' => 2,
],
(object) [
'id' => 5,
'version' => 3,
],
(object) [
'id' => 6,
'version' => 4,
],
],
'expectedrenumbers' => [
1 => (object) [
'oldversion' => 2,
'newversion' => 7,
],
2 => (object) [
'oldversion' => 2,
'newversion' => 8,
],
3 => (object) [
'oldversion' => 3,
'newversion' => 9,
],
4 => (object) [
'oldversion' => 2,
'newversion' => 10,
],
5 => (object) [
'oldversion' => 3,
'newversion' => 11,
],
6 => (object) [
'oldversion' => 4,
'newversion' => 12,
],
],
],
'duplicate versions with gaps, count higher than max' => [
'versions' => [
(object) [
'id' => 1,
'version' => 1,
],
(object) [
'id' => 2,
'version' => 1,
],
(object) [
'id' => 3,
'version' => 1,
],
(object) [
'id' => 4,
'version' => 3,
],
(object) [
'id' => 5,
'version' => 4,
],
(object) [
'id' => 6,
'version' => 4,
],
],
'expectedrenumbers' => [
1 => (object) [
'oldversion' => 1,
'newversion' => 7,
],
2 => (object) [
'oldversion' => 1,
'newversion' => 8,
],
3 => (object) [
'oldversion' => 1,
'newversion' => 9,
],
4 => (object) [
'oldversion' => 3,
'newversion' => 10,
],
5 => (object) [
'oldversion' => 4,
'newversion' => 11,
],
6 => (object) [
'oldversion' => 4,
'newversion' => 12,
],
],
],
'duplicate versions with gaps, max higher than count' => [
'versions' => [
(object) [
'id' => 1,
'version' => 1,
],
(object) [
'id' => 2,
'version' => 1,
],
(object) [
'id' => 3,
'version' => 1,
],
(object) [
'id' => 4,
'version' => 5,
],
(object) [
'id' => 5,
'version' => 5,
],
(object) [
'id' => 6,
'version' => 7,
],
],
'expectedrenumbers' => [
1 => (object) [
'oldversion' => 1,
'newversion' => 8,
],
2 => (object) [
'oldversion' => 1,
'newversion' => 9,
],
3 => (object) [
'oldversion' => 1,
'newversion' => 10,
],
4 => (object) [
'oldversion' => 5,
'newversion' => 11,
],
5 => (object) [
'oldversion' => 5,
'newversion' => 12,
],
6 => (object) [
'oldversion' => 7,
'newversion' => 13,
],
],
],
];
}
/**
* Test renumbering question versions, ensuring any duplicate version numbers are correctly resolved.
*
* @param $versions
* @param $expectedrenumbers
* @return array
* @dataProvider renumber_versions_provider
* @todo Deprecate in 6.0 MDL-87844 for removal in 7.0 MDL-87845.
*/
public function test_renumber_versions(array $versions, array $expectedrenumbers): void {
$this->assertEquals($expectedrenumbers, versions::renumber_versions($versions));
}
/**
* Resolve any violations of the unique questionbankentryid-version key on the question_versions table.
*
* It is not possible to set up actual records with duplicate version numbers, as this would violate the key. Instead
* we use a mocked DB object to return records that look like they violate the key, then we make sure the actual records are
* updated in a way that doesn't result in any violations.
*
* @todo Deprecate in 6.0 MDL-87844 for removal in 7.0 MDL-87845.
*/
public function test_resolve_unique_version_violations(): void {
$this->resetAfterTest();
$this->preventResetByRollback(); // Required for mock DB.
$generator = $this->getDataGenerator();
$course = $generator->create_course();
$qbank = $generator->create_module('qbank', ['course' => $course->id]);
$questiongenerator = $generator->get_plugin_generator('core_question');
$questions = $questiongenerator->create_categories_and_questions(
module::instance($qbank->cmid),
[
'testcategory' => [
'q1' => 'truefalse',
'q2' => 'truefalse',
],
],
);
// Generate 2 questions, with 6 versions each.
$q1v1 = $questions['testcategory']['q1'];
$q1v2 = $questiongenerator->update_question($q1v1, overrides: ['name' => 'q1 version 2']);
$q1v3 = $questiongenerator->update_question($q1v2, overrides: ['name' => 'q1 version 3']);
$q1v4 = $questiongenerator->update_question($q1v3, overrides: ['name' => 'q1 version 4']);
$q1v5 = $questiongenerator->update_question($q1v4, overrides: ['name' => 'q1 version 5']);
$q1v6 = $questiongenerator->update_question($q1v5, overrides: ['name' => 'q1 version 6']);
$q2v1 = $questions['testcategory']['q2'];
$q2v2 = $questiongenerator->update_question($q2v1, overrides: ['name' => 'q2 version 2']);
$q2v3 = $questiongenerator->update_question($q2v2, overrides: ['name' => 'q2 version 3']);
$q2v4 = $questiongenerator->update_question($q2v3, overrides: ['name' => 'q2 version 4']);
$q2v5 = $questiongenerator->update_question($q2v4, overrides: ['name' => 'q2 version 5']);
$q2v6 = $questiongenerator->update_question($q2v5, overrides: ['name' => 'q2 version 6']);
// Create some quizzes referencing different versions of the questions.
$quiz1 = $generator->create_module('quiz', ['course' => $course->id]);
$quiz2 = $generator->create_module('quiz', ['course' => $course->id]);
$quiz3 = $generator->create_module('quiz', ['course' => $course->id]);
// Quiz 1 has version 2 of each question.
quiz_add_quiz_question($q1v1->id, $quiz1);
quiz_add_quiz_question($q2v1->id, $quiz1);
$quiz1structure = quiz_settings::create($quiz1->id)->get_structure();
$quiz1slot1 = $quiz1structure->get_slot_by_number(1);
$quiz1slot2 = $quiz1structure->get_slot_by_number(2);
$quiz1structure->update_slot_version($quiz1slot1->id, 2);
$quiz1structure->update_slot_version($quiz1slot2->id, 2);
// Quiz 2 has version 5 of each question.
quiz_add_quiz_question($q1v1->id, $quiz2);
quiz_add_quiz_question($q2v1->id, $quiz2);
$quiz2structure = quiz_settings::create($quiz2->id)->get_structure();
$quiz2slot1 = $quiz2structure->get_slot_by_number(1);
$quiz2slot2 = $quiz2structure->get_slot_by_number(2);
$quiz2structure->update_slot_version($quiz2slot1->id, 5);
$quiz2structure->update_slot_version($quiz2slot2->id, 5);
// Quiz 3 has the latest version of each question.
quiz_add_quiz_question($q1v1->id, $quiz3);
quiz_add_quiz_question($q2v1->id, $quiz3);
// Set up mock records showing that question 1 has multiple versions with version number 2.
$mockviolations = [
$q1v1->questionbankentryid,
];
$mockversions = [
(object) [
'id' => $q1v1->versionid,
'version' => 1,
'timecreated' => time() - 100,
],
(object) [
'id' => $q1v2->versionid,
'version' => 2,
'timecreated' => time() - 99,
],
(object) [
'id' => $q1v3->versionid,
'version' => 2,
'timecreated' => time() - 98,
],
(object) [
'id' => $q1v4->versionid,
'version' => 4,
'timecreated' => time() - 97,
],
(object) [
'id' => $q1v5->versionid,
'version' => 5,
'timecreated' => time() - 96,
],
(object) [
'id' => $q1v6->versionid,
'version' => 6,
'timecreated' => time() - 95,
],
];
// Mock the get_fieldset_sql and get_records_sql to return our mocked results in response to the specific queries we expect.
// Any other queries will run as normal on the real $db object.
$db = di::get(\moodle_database::class);
$mockdb = $this->getMockBuilder($db::class)
->onlyMethods(['get_fieldset_sql', 'get_records_sql'])
->getMock();
$mockdb->method('get_fieldset_sql')
->willReturnCallback(fn($sql, $params) => match (true) {
str_starts_with(trim($sql), 'SELECT DISTINCT questionbankentryid') => $mockviolations,
default => $db->get_fieldset_sql($sql, $params),
});
$mockdb->method('get_records_sql')
->willReturnCallback(fn($sql, $params) => match (true) {
str_starts_with(trim($sql), 'SELECT qv.id, qv.version, q.timecreated') => $mockversions,
default => $db->get_records_sql($sql, $params),
});
$cfg = $db->export_dbconfig();
if (!isset($cfg->dboptions)) {
$cfg->dboptions = [];
}
$mockdb->connect($cfg->dbhost, $cfg->dbuser, $cfg->dbpass, $cfg->dbname, $cfg->prefix, $cfg->dboptions);
di::set(\moodle_database::class, $mockdb);
versions::resolve_unique_version_violations();
// Question 1 should have had its versions renumbered, and corresponding references updated. Question 2 should not.
$expectedversions = [
$q1v1->id => 7,
$q1v2->id => 8,
$q1v3->id => 9,
$q1v4->id => 10,
$q1v5->id => 11,
$q1v6->id => 12,
$q2v1->id => 1,
$q2v2->id => 2,
$q2v3->id => 3,
$q2v4->id => 4,
$q2v5->id => 5,
$q2v6->id => 6,
];
foreach ($expectedversions as $id => $expectedversion) {
$this->assertEquals($expectedversion, $db->get_field('question_versions', 'version', ['questionid' => $id]), $id);
}
$quiz1structure = quiz_settings::create($quiz1->id)->get_structure();
$quiz1q1 = $quiz1structure->get_question_in_slot(1);
$this->assertEquals($quiz1q1->name, $q1v3->name);
$this->assertEquals(
$expectedversions[$q1v3->id],
$db->get_field('question_references', 'version', ['itemid' => $quiz1q1->slotid, 'component' => 'mod_quiz']),
);
$quiz1q2 = $quiz1structure->get_question_in_slot(2);
$this->assertEquals($quiz1q2->name, $q2v2->name);
$quiz2structure = quiz_settings::create($quiz2->id)->get_structure();
$quiz2q1 = $quiz2structure->get_question_in_slot(1);
$this->assertEquals($quiz2q1->name, $q1v5->name);
$quiz2q2 = $quiz2structure->get_question_in_slot(2);
$this->assertEquals($quiz2q2->name, $q2v5->name);
$quiz3structure = quiz_settings::create($quiz3->id)->get_structure();
$quiz3q1 = $quiz3structure->get_question_in_slot(1);
$this->assertEquals($quiz3q1->name, $q1v6->name);
$quiz3q2 = $quiz3structure->get_question_in_slot(2);
$this->assertEquals($quiz3q2->name, $q2v6->name);
}
}
public/version.php
+1 -1
View File
@@ -29,7 +29,7 @@
defined('MOODLE_INTERNAL') || die();
$version = 2026013000.00; // YYYYMMDD = weekly release date of this DEV branch.
$version = 2026013000.01; // YYYYMMDD = weekly release date of this DEV branch.
// RR = release increments - 00 in DEV branches.
// .XX = incremental changes.
$release = '5.2dev (Build: 20260130)'; // Human-friendly version name