From 5b9e6497af8b758561c32f2cbbb420413f9f0dde Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Mon, 16 Nov 2020 18:01:16 -0500 Subject: [PATCH] Schema integrity check improvements - Create userdata tables and indexes that are missing - Delete tables and triggers that should no longer exist - Run schema integrity check before user data migration - Run schema integrity check after restart error This is meant to address two problems: 1) Database damage, and subsequent use of the DB Repair Tool, that results in missing tables 2) A small number of cases of schema update steps somehow not being reflected in users' databases despite their having updated userdata numbers, which are set within the same transaction. Until we figure out how that's happening, we should start adding conditional versions of schema update steps to the integrity check. This is currently only running the update check after a restart error, which might not occur for all missed schema update steps, so we might want other triggers for calling setIntegrityCheckRequired(). --- chrome/content/zotero/xpcom/db.js | 31 +++-- chrome/content/zotero/xpcom/retractions.js | 8 -- chrome/content/zotero/xpcom/schema.js | 131 +++++++++++++++++---- test/tests/dbTest.js | 20 ++++ test/tests/schemaTest.js | 8 ++ 5 files changed, 155 insertions(+), 43 deletions(-) diff --git a/chrome/content/zotero/xpcom/db.js b/chrome/content/zotero/xpcom/db.js index ba868ef877..582f63c1bf 100644 --- a/chrome/content/zotero/xpcom/db.js +++ b/chrome/content/zotero/xpcom/db.js @@ -807,12 +807,15 @@ Zotero.DBConnection.prototype.columnExists = async function (table, column) { }; -/** - * Parse SQL string and execute transaction with all statements - * - * @return {Promise} - */ -Zotero.DBConnection.prototype.executeSQLFile = Zotero.Promise.coroutine(function* (sql) { +Zotero.DBConnection.prototype.indexExists = async function (index, db) { + await this._getConnectionAsync(); + var prefix = db ? db + '.' : ''; + var sql = `SELECT COUNT(*) FROM ${prefix}sqlite_master WHERE type='index' AND name=?`; + return !!await this.valueQueryAsync(sql, [index]); +}; + + +Zotero.DBConnection.prototype.parseSQLFile = function (sql) { var nonCommentRE = /^[^-]/; var trailingCommentRE = /^(.*?)(?:--.+)?$/; @@ -830,13 +833,23 @@ Zotero.DBConnection.prototype.executeSQLFile = Zotero.Promise.coroutine(function var statements = sql.split(";") .map(x => x.replace(/TEMPSEMI/g, ";")); + return statements; +}; + + +/** + * Parse SQL string and execute transaction with all statements + * + * @return {Promise} + */ +Zotero.DBConnection.prototype.executeSQLFile = async function (sql) { this.requireTransaction(); - + var statements = this.parseSQLFile(sql); var statement; while (statement = statements.shift()) { - yield this.queryAsync(statement); + await this.queryAsync(statement); } -}); +}; /* diff --git a/chrome/content/zotero/xpcom/retractions.js b/chrome/content/zotero/xpcom/retractions.js index 2d31588090..707ce7a2d1 100644 --- a/chrome/content/zotero/xpcom/retractions.js +++ b/chrome/content/zotero/xpcom/retractions.js @@ -48,14 +48,6 @@ Zotero.Retractions = { return; } - // TEMP: Until we can figure out why some schema updates aren't going through despite the - // version number being incremented, create table here if it's missing - await Zotero.DB.queryAsync("CREATE TABLE IF NOT EXISTS retractedItems (\n itemID INTEGER PRIMARY KEY,\n data TEXT,\n FOREIGN KEY (itemID) REFERENCES items(itemID) ON DELETE CASCADE\n);"); - try { - await Zotero.DB.queryAsync("ALTER TABLE retractedItems ADD COLUMN flag INT DEFAULT 0"); - } - catch (e) {} - // Load mappings of keys (DOI hashes and PMIDs) to items and vice versa and register for // item changes so they can be kept up to date in notify(). await this._cacheKeyMappings(); diff --git a/chrome/content/zotero/xpcom/schema.js b/chrome/content/zotero/xpcom/schema.js index 5339c6b19d..b7b605a3ab 100644 --- a/chrome/content/zotero/xpcom/schema.js +++ b/chrome/content/zotero/xpcom/schema.js @@ -194,6 +194,7 @@ Zotero.Schema = new function(){ // Auto-repair databases flagged for repair or coming from the DB Repair Tool if (integrityCheck) { await this.integrityCheck(true); + options.skipIntegrityCheck = true; } updated = await _migrateUserDataSchema(userdata, options); @@ -1740,27 +1741,10 @@ Zotero.Schema = new function(){ // so that we don't try to wipe out all data if (!(yield Zotero.DB.valueQueryAsync("SELECT COUNT(*) FROM fieldsCombined")) || !(yield Zotero.DB.valueQueryAsync("SELECT COUNT(*) FROM itemTypeFieldsCombined"))) { + Zotero.logError("Combined field tables are empty -- skipping integrity check"); return false; } - // Check foreign keys - var rows = yield Zotero.DB.queryAsync("PRAGMA foreign_key_check"); - if (rows.length && !fix) { - let suffix1 = rows.length == 1 ? '' : 's'; - let suffix2 = rows.length == 1 ? 's' : ''; - Zotero.debug(`Found ${rows.length} row${suffix1} that violate${suffix2} foreign key constraints`, 1); - return false; - } - // If fixing, delete rows that violate FK constraints - for (let row of rows) { - try { - yield Zotero.DB.queryAsync(`DELETE FROM ${row.table} WHERE ROWID=?`, row.rowid); - } - catch (e) { - Zotero.logError(e); - } - } - var attachmentID = parseInt(yield Zotero.DB.valueQueryAsync( "SELECT itemTypeID FROM itemTypes WHERE typeName='attachment'" )); @@ -1768,14 +1752,104 @@ Zotero.Schema = new function(){ "SELECT itemTypeID FROM itemTypes WHERE typeName='note'" )); - - // Non-foreign key checks - // // The first position is for testing and the second is for repairing. Can be either SQL - // statements or promise-returning functions. For statements, the repair entry can be either a - // string or an array with multiple statements. Functions should avoid assuming any global state - // (e.g., loaded data). + // statements or promise-returning functions. For statements, the repair entry can be either + // a string or an array with multiple statements. Check functions should return false if no + // error, and either true or data to pass to the repair function on error. Functions should + // avoid assuming any global state (e.g., loaded data). var checks = [ + // Create any tables or indexes that are missing and delete any tables or triggers that + // still exist but should have been deleted + [ + async function () { + var statementsToRun = []; + + // Get all existing tables, indexes, and triggers + var sql = "SELECT " + + "CASE type " + + "WHEN 'table' THEN 'table:' || tbl_name " + + "WHEN 'index' THEN 'index:' || name " + + "WHEN 'trigger' THEN 'trigger:' || name " + + "END " + + "FROM sqlite_master WHERE type IN ('table', 'index', 'trigger')"; + var schema = new Set(await Zotero.DB.columnQueryAsync(sql)); + + // Check for deleted tables and triggers that still exist + var deletedTables = [ + "transactionSets", + "transactions", + "transactionLog", + ]; + var deletedTriggers = [ + "insert_date_field", + "update_date_field", + "fki_itemAttachments", + "fku_itemAttachments", + "fki_itemNotes", + "fku_itemNotes", + ]; + for (let table of deletedTables) { + if (schema.has('table:' + table)) { + statementsToRun.push("DROP TABLE " + table); + } + } + for (let trigger of deletedTriggers) { + if (schema.has('trigger:' + trigger)) { + statementsToRun.push("DROP TRIGGER " + trigger); + } + } + + // Check for missing tables and indexes + var statements = await Zotero.DB.parseSQLFile(await _getSchemaSQL('userdata')); + for (let statement of statements) { + var matches = statement.match(/^CREATE TABLE\s+([^\s]+)/); + if (matches) { + let table = matches[1]; + if (!schema.has('table:' + table)) { + Zotero.debug(`Table ${table} is missing`, 2); + statementsToRun.push(statement); + } + continue; + } + + matches = statement.match(/^CREATE INDEX\s+([^\s]+)/); + if (matches) { + let index = matches[1]; + if (!schema.has('index:' + index)) { + Zotero.debug(`Index ${index} is missing`, 2); + statementsToRun.push(statement); + } + continue; + } + } + + return statementsToRun.length ? statementsToRun : false; + }, + async function (statements) { + for (let statement of statements) { + await Zotero.DB.queryAsync(statement); + } + } + ], + + // Foreign key checks + [ + async function () { + var rows = await Zotero.DB.queryAsync("PRAGMA foreign_key_check"); + if (!rows.length) return false; + var suffix1 = rows.length == 1 ? '' : 's'; + var suffix2 = rows.length == 1 ? 's' : ''; + Zotero.debug(`Found ${rows.length} row${suffix1} that violate${suffix2} foreign key constraints`, 1); + return rows; + }, + // If fixing, delete rows that violate FK constraints + async function (rows) { + for (let row of rows) { + await Zotero.DB.queryAsync(`DELETE FROM ${row.table} WHERE ROWID=?`, row.rowid); + } + } + ], + // Can't be a FK with itemTypesCombined [ "SELECT COUNT(*) > 0 FROM items WHERE itemTypeID IS NULL", @@ -1957,7 +2031,9 @@ Zotero.Schema = new function(){ } // Function else { - yield check[1](); + // If data was provided by the check function, pass that to the fix function + let checkData = typeof errorsFound != 'boolean' ? errorsFound : null; + yield check[1](checkData); } continue; } @@ -2419,6 +2495,10 @@ Zotero.Schema = new function(){ return false; } + if (!options.skipIntegrityCheck) { + yield Zotero.Schema.integrityCheck(true); + } + Zotero.debug('Updating user data tables from version ' + fromVersion + ' to ' + toVersion); if (options.onBeforeUpdate) { @@ -3029,7 +3109,6 @@ Zotero.Schema = new function(){ } } - // Duplicated in retractions.js::init() due to undiagnosed schema update bug else if (i == 105) { // This was originally in 103 and then 104, but some schema update steps are being // missed for some people, so run again with IF NOT EXISTS until we figure out diff --git a/test/tests/dbTest.js b/test/tests/dbTest.js index 74c1dc7e33..81134c86cb 100644 --- a/test/tests/dbTest.js +++ b/test/tests/dbTest.js @@ -356,4 +356,24 @@ describe("Zotero.DB", function() { assert.isFalse(await Zotero.DB.columnExists('foo', 'itemID')); }); }); + + + describe("#indexExists()", function () { + it("should return true if an index exists", async function () { + assert.isTrue(await Zotero.DB.indexExists('items_synced')); + }); + + it("should return false if an index doesn't exists", async function () { + assert.isFalse(await Zotero.DB.indexExists('foo')); + }); + }); + + + describe("#parseSQLFile", function () { + it("should extract tables and indexes from userdata SQL file", async function () { + var sql = Zotero.File.getResource(`resource://zotero/schema/userdata.sql`); + var statements = await Zotero.DB.parseSQLFile(sql); + assert.isTrue(statements.some(x => x.startsWith('CREATE TABLE items'))); + }); + }); }); diff --git a/test/tests/schemaTest.js b/test/tests/schemaTest.js index 7c0b8d989f..462070389d 100644 --- a/test/tests/schemaTest.js +++ b/test/tests/schemaTest.js @@ -248,6 +248,14 @@ describe("Zotero.Schema", function() { }); }) + it("should repair a missing userdata table", async function () { + await Zotero.DB.queryAsync("DROP TABLE retractedItems"); + assert.isFalse(await Zotero.DB.tableExists('retractedItems')); + assert.isFalse(await Zotero.Schema.integrityCheck()); + assert.isTrue(await Zotero.Schema.integrityCheck(true)); + assert.isTrue(await Zotero.DB.tableExists('retractedItems')); + }); + it("should repair a foreign key violation", function* () { yield assert.eventually.isTrue(Zotero.Schema.integrityCheck());