www

commit d8abfa4f679342e553bce144c27913a140ed6cee
parent b9bbf007f086ec8d505fee1fcb01b9db4bab7e1c
Author: Dan Stillman <dstillman@zotero.org>
Date:   Wed, 18 May 2016 16:07:00 -0400

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.)

Diffstat:
M
chrome/content/zotero/xpcom/data/item.js
 | 
13
++++++++-----
M
chrome/content/zotero/xpcom/db.js
 | 
42
------------------------------------------
M
chrome/content/zotero/xpcom/schema.js
 | 
19
++++++++++++++++++-
M
resource/schema/userdata.sql
 | 
2
+-
M
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);