From d8abfa4f679342e553bce144c27913a140ed6cee Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Wed, 18 May 2016 16:07:00 -0400 Subject: [PATCH] Save all item data values as string, and convert old integers to strings Before 5.0 we performed a regexp on new item data values to determine if they were integers and saved them natively in SQLite if so. We no longer do that, but setField() used strict equality when checking for changes, so an item could be marked as changed when comparing to a new string value (e.g., from a write response from the API, which always returns strings). To avoid that, this converts all old values in the DB to strings and saves all incoming values as strings automatically. (This should also help with searching and some other things.) --- chrome/content/zotero/xpcom/data/item.js | 13 +++++--- chrome/content/zotero/xpcom/db.js | 42 ------------------------ chrome/content/zotero/xpcom/schema.js | 19 ++++++++++- resource/schema/userdata.sql | 2 +- test/tests/itemTest.js | 30 +++++++++++++++-- 5 files changed, 55 insertions(+), 51 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index f882458bc..70742f02c 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -644,9 +644,16 @@ Zotero.Item.prototype.setField = function(field, value, loadIn) { throw new Error(`'${field}' value cannot be undefined`); } - if (typeof value == 'string') { + // Normalize values + if (typeof value == 'number') { + value = "" + value; + } + else if (typeof value == 'string') { value = value.trim().normalize(); } + if (value === "" || value === null || value === false) { + value = false; + } //Zotero.debug("Setting field '" + field + "' to '" + value + "' (loadIn: " + (loadIn ? 'true' : 'false') + ") for item " + this.id + " "); @@ -749,10 +756,6 @@ Zotero.Item.prototype.setField = function(field, value, loadIn) { return true; } - if (value === "" || value === null || value === false) { - value = false; - } - // Make sure to use type-specific field ID if available fieldID = Zotero.ItemFields.getFieldIDFromTypeAndBase(itemTypeID, fieldID) || fieldID; diff --git a/chrome/content/zotero/xpcom/db.js b/chrome/content/zotero/xpcom/db.js index e59692293..216de5fdc 100644 --- a/chrome/content/zotero/xpcom/db.js +++ b/chrome/content/zotero/xpcom/db.js @@ -1099,30 +1099,6 @@ Zotero.DBConnection.prototype.backupDatabase = Zotero.Promise.coroutine(function }); -/** - * Determine the necessary data type for SQLite parameter binding - * - * @return int 0 for string, 32 for int32, 64 for int64 - */ -Zotero.DBConnection.prototype.getSQLDataType = function(value) { - var strVal = value + ''; - if (strVal.match(/^[1-9]+[0-9]*$/)) { - // These upper bounds also specified in Zotero.DB - // - // Store as 32-bit signed integer - if (value <= 2147483647) { - return 32; - } - // Store as 64-bit signed integer - // 2^53 is JS's upper-bound for decimal integers - else if (value < 9007199254740992) { - return 64; - } - } - return 0; -} - - ///////////////////////////////////////////////////////////////// // // Private methods @@ -1300,24 +1276,6 @@ Zotero.DBConnection.prototype._debug = function (str, level) { } -Zotero.DBConnection.prototype._getTypedValue = function (statement, i) { - var type = statement.getTypeOfIndex(i); - // For performance, we hard-code these constants - switch (type) { - case 1: //VALUE_TYPE_INTEGER - return statement.getInt64(i); - case 3: //VALUE_TYPE_TEXT - return statement.getUTF8String(i); - case 0: //VALUE_TYPE_NULL - return null; - case 2: //VALUE_TYPE_FLOAT - return statement.getDouble(i); - case 4: //VALUE_TYPE_BLOB - return statement.getBlob(i, {}); - } -} - - // Initialize main database connection Zotero.DB = new Zotero.DBConnection('zotero'); diff --git a/chrome/content/zotero/xpcom/schema.js b/chrome/content/zotero/xpcom/schema.js index 50adb1abc..91a7f660a 100644 --- a/chrome/content/zotero/xpcom/schema.js +++ b/chrome/content/zotero/xpcom/schema.js @@ -33,7 +33,8 @@ Zotero.Schema = new function(){ var _dbVersions = []; var _schemaVersions = []; - var _maxCompatibility = 2; + // Update when adding _updateCompatibility() line to schema update step + var _maxCompatibility = 3; var _repositoryTimer; var _remoteUpdateInProgress = false, _localUpdateInProgress = false; @@ -2256,6 +2257,22 @@ Zotero.Schema = new function(){ yield Zotero.DB.queryAsync("UPDATE itemRelations SET object=? WHERE ROWID=?", [newObject, rows[i].id]); } } + + else if (i == 87) { + yield _updateCompatibility(3); + let rows = yield Zotero.DB.queryAsync("SELECT valueID, value FROM itemDataValues WHERE TYPEOF(value) = 'integer'"); + for (let i = 0; i < rows.length; i++) { + let row = rows[i]; + let valueID = yield Zotero.DB.valueQueryAsync("SELECT valueID FROM itemDataValues WHERE value=?", "" + row.value); + if (valueID) { + yield Zotero.DB.queryAsync("UPDATE itemData SET valueID=? WHERE valueID=?", [valueID, row.valueID]); + yield Zotero.DB.queryAsync("DELETE FROM itemDataValues WHERE valueID=?", row.valueID); + } + else { + yield Zotero.DB.queryAsync("UPDATE itemDataValues SET value=? WHERE valueID=?", ["" + row.value, row.valueID]); + } + } + } } yield _updateDBVersion('userdata', toVersion); diff --git a/resource/schema/userdata.sql b/resource/schema/userdata.sql index bdfbdc5b4..376c3b3a9 100644 --- a/resource/schema/userdata.sql +++ b/resource/schema/userdata.sql @@ -1,4 +1,4 @@ --- 86 +-- 87 -- Copyright (c) 2009 Center for History and New Media -- George Mason University, Fairfax, Virginia, USA diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index 31365dd7c..51059581f 100644 --- a/test/tests/itemTest.js +++ b/test/tests/itemTest.js @@ -33,6 +33,32 @@ describe("Zotero.Item", function () { assert.isTrue(item.hasChanged()); }) + it("should save an integer as a string", function* () { + var val = 1234; + var item = new Zotero.Item('book'); + item.setField('numPages', val); + yield item.saveTx(); + assert.strictEqual(item.getField('numPages'), "" + val); + // Setting again as string shouldn't register a change + assert.isFalse(item.setField('numPages', "" + val)); + + // Value should be TEXT in the DB + var sql = "SELECT TYPEOF(value) FROM itemData JOIN itemDataValues USING (valueID) " + + "WHERE itemID=? AND fieldID=?"; + var type = yield Zotero.DB.valueQueryAsync(sql, [item.id, Zotero.ItemFields.getID('numPages')]); + assert.equal(type, 'text'); + }); + + it("should save integer 0 as a string", function* () { + var val = 0; + var item = new Zotero.Item('book'); + item.setField('numPages', val); + yield item.saveTx(); + assert.strictEqual(item.getField('numPages'), "" + val); + // Setting again as string shouldn't register a change + assert.isFalse(item.setField('numPages', "" + val)); + }); + it('should clear an existing field when ""/null/false is passed', function* () { var field = 'title'; var val = 'foo'; @@ -69,9 +95,9 @@ describe("Zotero.Item", function () { assert.equal(item.getField(field), ""); }) - it('should clear a field set to 0 when a ""/null/false is passed', function* () { + it('should clear a field set to '0' when a ""/null/false is passed', function* () { var field = 'title'; - var val = 0; + var val = "0"; var fieldID = Zotero.ItemFields.getID(field); var item = new Zotero.Item('book'); item.setField(field, val);