From 70d9b9870c895dbc0c71138ab7ed747edcfd1753 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Fri, 31 Jul 2015 04:03:09 -0400
Subject: [PATCH] Fix a few small data layer bugs, and tidy up a little

I don't think these were triggered by any client code, but I found them while
porting code to the server.
---
 .../content/zotero/xpcom/data/collection.js   |  67 ++++++++---
 .../content/zotero/xpcom/data/dataObject.js   |  28 +++--
 .../content/zotero/xpcom/data/dataObjects.js  |  10 +-
 chrome/content/zotero/xpcom/data/item.js      |  59 +++++-----
 chrome/content/zotero/xpcom/search.js         | 105 ++++++++++++------
 test/tests/dataObjectTest.js                  |  79 +++++++++----
 6 files changed, 229 insertions(+), 119 deletions(-)

diff --git a/chrome/content/zotero/xpcom/data/collection.js b/chrome/content/zotero/xpcom/data/collection.js
index 61e38fe31..719aba990 100644
--- a/chrome/content/zotero/xpcom/data/collection.js
+++ b/chrome/content/zotero/xpcom/data/collection.js
@@ -99,24 +99,57 @@ Zotero.Collection.prototype.getName = function() {
  * Populate collection data from a database row
  */
 Zotero.Collection.prototype.loadFromRow = function(row) {
-	for each(let col in this.ObjectsClass.primaryFields) {
-		if (row[col] === undefined) {
-			Zotero.debug('Skipping missing collection field ' + col);
+	var primaryFields = this._ObjectsClass.primaryFields;
+	for (let i=0; i<primaryFields.length; i++) {
+		let col = primaryFields[i];
+		try {
+			var val = row[col];
 		}
+		catch (e) {
+			Zotero.debug('Skipping missing ' + this._objectType + ' field ' + col);
+			continue;
+		}
+		
+		switch (col) {
+		case this._ObjectsClass.idColumn:
+			col = 'id';
+			break;
+		
+		// Integer
+		case 'libraryID':
+			val = parseInt(val);
+			break;
+		
+		// Integer or 0
+		case 'version':
+			val = val ? parseInt(val) : 0;
+			break;
+		
+		// Value or false
+		case 'parentKey':
+			val = val || false;
+			break;
+		
+		// Integer or false if falsy
+		case 'parentID':
+			val = val ? parseInt(val) : false;
+			break;
+		
+		// Boolean
+		case 'synced':
+		case 'hasChildCollections':
+		case 'hasChildItems':
+			val = !!val;
+			break;
+		
+		default:
+			val = val || '';
+		}
+		
+		this['_' + col] = val;
 	}
 	
-	this._id = row.collectionID;
-	this._libraryID = parseInt(row.libraryID);
-	this._key = row.key;
-	this._name = row.name;
-	this._parentID = row.parentID || false;
-	this._parentKey = row.parentKey || false;
-	this._version = parseInt(row.version);
-	this._synced = !!row.synced;
-	
-	this._hasChildCollections = !!row.hasChildCollections;
 	this._childCollectionsLoaded = false;
-	this._hasChildItems = !!row.hasChildItems;
 	this._childItemsLoaded = false;
 	
 	this._loaded.primaryData = true;
@@ -288,7 +321,7 @@ Zotero.Collection.prototype._saveData = Zotero.Promise.coroutine(function* (env)
 				this.libraryID, this.parentKey
 			);
 			Zotero.DB.addCurrentCallback("commit", function () {
-				this.ObjectsClass.registerChildCollection(parentCollectionID, this.id);
+				this.ObjectsClass.registerChildCollection(parentCollectionID, collectionID);
 			}.bind(this));
 		}
 		// Remove this from the previous parent's cached collection lists after commit,
@@ -298,12 +331,12 @@ Zotero.Collection.prototype._saveData = Zotero.Promise.coroutine(function* (env)
 				this.libraryID, this._previousData.parentKey
 			);
 			Zotero.DB.addCurrentCallback("commit", function () {
-				this.ObjectsClass.unregisterChildCollection(parentCollectionID, this.id);
+				this.ObjectsClass.unregisterChildCollection(parentCollectionID, collectionID);
 			}.bind(this));
 		}
 		
 		if (!isNew) {
-			Zotero.Notifier.queue('move', 'collection', this.id);
+			Zotero.Notifier.queue('move', 'collection', collectionID);
 		}
 	}
 });
diff --git a/chrome/content/zotero/xpcom/data/dataObject.js b/chrome/content/zotero/xpcom/data/dataObject.js
index 3538098aa..b5d3c97d3 100644
--- a/chrome/content/zotero/xpcom/data/dataObject.js
+++ b/chrome/content/zotero/xpcom/data/dataObject.js
@@ -100,7 +100,9 @@ Zotero.DataObject.prototype._get = function (field) {
 	if (this['_' + field] !== null) {
 		return this['_' + field];
 	}
-	this._requireData('primaryData');
+	if (field != 'libraryID' && field != 'key' && field != 'id') {
+		this._requireData('primaryData');
+	}
 	return null;
 }
 
@@ -769,6 +771,19 @@ Zotero.DataObject.prototype._markFieldChange = function (field, oldValue) {
 	}
 }
 
+
+Zotero.DataObject.prototype.hasChanged = function() {
+	var changed = Object.keys(this._changed).filter(dataType => this._changed[dataType]);
+	if (changed.length == 1
+			&& changed[0] == 'primaryData'
+			&& this._changed.primaryData.synced
+			&& this._previousData.synced == this._synced) {
+		return false;
+	}
+	return !!changed.length;
+}
+
+
 /**
  * Clears log of changed values
  * @param {String} [dataType] data type/field to clear. Defaults to clearing everything
@@ -904,17 +919,6 @@ Zotero.DataObject.prototype.saveTx = function (options) {
 }
 
 
-Zotero.DataObject.prototype.hasChanged = function() {
-	var changed = Object.keys(this._changed).filter(dataType => this._changed[dataType]);
-	if (changed.length == 1
-			&& changed[0] == 'primaryData'
-			&& this._changed.primaryData.synced
-			&& this._previousData.synced == this._synced) {
-		return false;
-	}
-	return !!changed.length;
-}
-
 Zotero.DataObject.prototype._initSave = Zotero.Promise.coroutine(function* (env) {
 	// Default to user library if not specified
 	if (this.libraryID === null) {
diff --git a/chrome/content/zotero/xpcom/data/dataObjects.js b/chrome/content/zotero/xpcom/data/dataObjects.js
index 952f7c456..c6f423b22 100644
--- a/chrome/content/zotero/xpcom/data/dataObjects.js
+++ b/chrome/content/zotero/xpcom/data/dataObjects.js
@@ -277,8 +277,8 @@ Zotero.DataObjects.prototype.getByLibraryAndKeyAsync = Zotero.Promise.coroutine(
 });
 
 
-Zotero.DataObjects.prototype.exists = function (itemID) {
-	return !!this.getLibraryAndKeyFromID(itemID);
+Zotero.DataObjects.prototype.exists = function (id) {
+	return !!this.getLibraryAndKeyFromID(id);
 }
 
 
@@ -383,7 +383,8 @@ Zotero.DataObjects.prototype.registerObject = function (obj) {
 	var libraryID = obj.libraryID;
 	var key = obj.key;
 	
-	Zotero.debug("Registering " + this._ZDO_object + " " + id + " as " + libraryID + "/" + key);
+	Zotero.debug("Registering " + this._ZDO_object + " " + id
+		+ " as " + libraryID + "/" + key);
 	if (!this._objectIDs[libraryID]) {
 		this._objectIDs[libraryID] = {};
 	}
@@ -568,8 +569,7 @@ Zotero.DataObjects.prototype._load = Zotero.Promise.coroutine(function* (library
 					obj = new Zotero[this._ZDO_Object];
 					obj.loadFromRow(rowObj, true);
 					if (!options || !options.noCache) {
-						this._objectCache[id] = obj;
-						obj._inCache = true;
+						this.registerObject(obj);
 					}
 				}
 				loaded[id] = obj;
diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js
index 1cad804d8..f1a862cee 100644
--- a/chrome/content/zotero/xpcom/data/item.js
+++ b/chrome/content/zotero/xpcom/data/item.js
@@ -316,23 +316,26 @@ Zotero.Item.prototype._parseRowData = function(row) {
 	for (let i=0; i<primaryFields.length; i++) {
 		let col = primaryFields[i];
 		
-		if (row[col] === undefined) {
+		try {
+			var val = row[col];
+		}
+		catch (e) {
 			Zotero.debug('Skipping missing field ' + col);
 			continue;
 		}
 		
-		let val = row[col];
-		
 		//Zotero.debug("Setting field '" + col + "' to '" + val + "' for item " + this.id);
 		
 		switch (col) {
+			// Skip
+			case 'libraryID':
+			case 'itemTypeID':
+				break;
+			
 			// Unchanged
 			case 'itemID':
 				col = 'id';
 				break;
-				
-			case 'libraryID':
-				break;
 			
 			// Integer or 0
 			case 'version':
@@ -665,15 +668,15 @@ Zotero.Item.prototype.setField = function(field, value, loadIn) {
 				
 				value = Zotero.Date.dateToSQL(date, true);
 				break;
-
+			
 			case 'version':
 				value = parseInt(value);
 				break;
-
+			
 			case 'synced':
 				value = !!value;
 				break;
-				
+			
 			default:
 				throw new Error('Primary field ' + field + ' cannot be changed in Zotero.Item.setField()');
 			
@@ -686,27 +689,27 @@ Zotero.Item.prototype.setField = function(field, value, loadIn) {
 		*/
 		
 		// If field value has changed
-		if (this['_' + field] != value || field == 'synced') {
-			Zotero.debug("Field '" + field + "' has changed from '" + this['_' + field] + "' to '" + value + "'", 4);
-			
-			// Save a copy of the field before modifying
-			this._markFieldChange(field, this['_' + field]);
-			
-			if (field == 'itemTypeID') {
-				this.setType(value, loadIn);
-			}
-			else {
-				
-				this['_' + field] = value;
-				
-				if (!this._changed.primaryData) {
-					this._changed.primaryData = {};
-				}
-				this._changed.primaryData[field] = true;
-			}
+		if (this['_' + field] === value && field != 'synced') {
+			Zotero.debug("Field '" + field + "' has not changed", 4);
+			return false;
+		}
+		
+		Zotero.debug("Field '" + field + "' has changed from '" + this['_' + field] + "' to '" + value + "'", 4);
+		
+		// Save a copy of the field before modifying
+		this._markFieldChange(field, this['_' + field]);
+		
+		if (field == 'itemTypeID') {
+			this.setType(value, loadIn);
 		}
 		else {
-			Zotero.debug("Field '" + field + "' has not changed", 4);
+			
+			this['_' + field] = value;
+			
+			if (!this._changed.primaryData) {
+				this._changed.primaryData = {};
+			}
+			this._changed.primaryData[field] = true;
 		}
 		return true;
 	}
diff --git a/chrome/content/zotero/xpcom/search.js b/chrome/content/zotero/xpcom/search.js
index 5e2407038..78fd98917 100644
--- a/chrome/content/zotero/xpcom/search.js
+++ b/chrome/content/zotero/xpcom/search.js
@@ -89,12 +89,43 @@ Zotero.defineProperty(Zotero.Search.prototype, 'conditions', {
 
 
 Zotero.Search.prototype.loadFromRow = function (row) {
-	this._id = row.savedSearchID;
-	this._libraryID = parseInt(row.libraryID);
-	this._key = row.key;
-	this._name = row.name;
-	this._version = parseInt(row.version);
-	this._synced = !!row.synced;
+	var primaryFields = this._ObjectsClass.primaryFields;
+	for (let i=0; i<primaryFields.length; i++) {
+		let col = primaryFields[i];
+		try {
+			var val = row[col];
+		}
+		catch (e) {
+			Zotero.debug('Skipping missing ' + this._objectType + ' field ' + col);
+			continue;
+		}
+		
+		switch (col) {
+		case this._ObjectsClass.idColumn:
+			col = 'id';
+			break;
+		
+		// Integer
+		case 'libraryID':
+			val = parseInt(val);
+			break;
+		
+		// Integer or 0
+		case 'version':
+			val = val ? parseInt(val) : 0;
+			break;
+		
+		// Boolean
+		case 'synced':
+			val = !!val;
+			break;
+		
+		default:
+			val = val || '';
+		}
+		
+		this['_' + col] = val;
+	}
 	
 	this._loaded.primaryData = true;
 	this._clearChanged('primaryData');
@@ -140,33 +171,35 @@ Zotero.Search.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		yield Zotero.DB.queryAsync(sql, env.sqlValues);
 	}
 	
-	if (!isNew) {
-		var sql = "DELETE FROM savedSearchConditions WHERE savedSearchID=?";
-		yield Zotero.DB.queryAsync(sql, this.id);
-	}
-	
-	var i = 0;
-	var sql = "INSERT INTO savedSearchConditions "
-		+ "(savedSearchID, searchConditionID, condition, operator, value, required) "
-		+ "VALUES (?,?,?,?,?,?)";
-	for (let id in this._conditions) {
-		let condition = this._conditions[id];
+	if (this._changed.conditions) {
+		if (!isNew) {
+			var sql = "DELETE FROM savedSearchConditions WHERE savedSearchID=?";
+			yield Zotero.DB.queryAsync(sql, this.id);
+		}
 		
-		// Convert condition and mode to "condition[/mode]"
-		let conditionString = condition.mode ?
-			condition.condition + '/' + condition.mode :
-			condition.condition
-		
-		var sqlParams = [
-			searchID,
-			i,
-			conditionString,
-			condition.operator ? condition.operator : null,
-			condition.value ? condition.value : null,
-			condition.required ? 1 : null
-		];
-		yield Zotero.DB.queryAsync(sql, sqlParams);
-		i++;
+		var i = 0;
+		var sql = "INSERT INTO savedSearchConditions "
+			+ "(savedSearchID, searchConditionID, condition, operator, value, required) "
+			+ "VALUES (?,?,?,?,?,?)";
+		for (let id in this._conditions) {
+			let condition = this._conditions[id];
+			
+			// Convert condition and mode to "condition[/mode]"
+			let conditionString = condition.mode ?
+				condition.condition + '/' + condition.mode :
+				condition.condition
+			
+			var sqlParams = [
+				searchID,
+				i,
+				conditionString,
+				condition.operator ? condition.operator : null,
+				condition.value ? condition.value : null,
+				condition.required ? 1 : null
+			];
+			yield Zotero.DB.queryAsync(sql, sqlParams);
+			i++;
+		}
 	}
 });
 
@@ -185,7 +218,6 @@ Zotero.Search.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env)
 	}
 	
 	if (!env.skipCache) {
-		yield this.loadPrimaryData(true);
 		yield this.reload();
 		// If new, there's no other data we don't have, so we can mark everything as loaded
 		if (env.isNew) {
@@ -824,11 +856,9 @@ Zotero.Search.prototype.getSQLParams = Zotero.Promise.coroutine(function* () {
 
 
 Zotero.Search.prototype.loadConditions = Zotero.Promise.coroutine(function* (reload) {
-	Zotero.debug("Loading conditions for search " + this.libraryKey);
+	if (this._loaded.conditions && !reload) return;
 	
-	if (this._loaded.conditions && !reload) {
-		return;
-	}
+	Zotero.debug("Loading conditions for search " + this.libraryKey);
 	
 	if (!this.id) {
 		throw new Error('ID not set for object before attempting to load conditions');
@@ -876,6 +906,7 @@ Zotero.Search.prototype.loadConditions = Zotero.Promise.coroutine(function* (rel
 	}
 	
 	this._loaded.conditions = true;
+	this._clearChanged('conditions');
 });
 
 
diff --git a/test/tests/dataObjectTest.js b/test/tests/dataObjectTest.js
index 9632f7e6b..c2c83cc38 100644
--- a/test/tests/dataObjectTest.js
+++ b/test/tests/dataObjectTest.js
@@ -3,26 +3,18 @@
 describe("Zotero.DataObject", function() {
 	var types = ['collection', 'item', 'search'];
 	
-	describe("#loadAllData()", function () {
-		it("should load data on a regular item", function* () {
-			var item = new Zotero.Item('book');
-			var id = yield item.saveTx();
-			yield item.loadAllData();
-			assert.throws(item.getNote.bind(item), 'getNote() can only be called on notes and attachments');
-		})
-		
-		it("should load data on an attachment item", function* () {
-			var item = new Zotero.Item('attachment');
-			var id = yield item.saveTx();
-			yield item.loadAllData();
-			assert.equal(item.getNote(), '');
-		})
-		
-		it("should load data on a note item", function* () {
-			var item = new Zotero.Item('note');
-			var id = yield item.saveTx();
-			yield item.loadAllData();
-			assert.equal(item.getNote(), '');
+	describe("#key", function () {
+		it("shouldn't update .loaded on get if unset", function* () {
+			for (let type of types) {
+				if (type == 'item') {
+					var param = 'book';
+				}
+				let obj = new Zotero[Zotero.Utilities.capitalize(type)](param);
+				obj.libraryID = Zotero.Libraries.userLibraryID;
+				assert.isNull(obj.key);
+				assert.isFalse(obj._loaded.primaryData);
+				obj.key = Zotero.DataObjectUtilities.generateKey();
+			}
 		})
 	})
 	
@@ -173,6 +165,53 @@ describe("Zotero.DataObject", function() {
 		});
 	})
 	
+	describe("#loadPrimaryData()", function () {
+		it("should load unloaded primary data if partially set", function* () {
+			var objs = {};
+			for (let type of types) {
+				let obj = createUnsavedDataObject(type);
+				yield obj.save({
+					skipCache: true
+				});
+				objs[type] = {
+					key: obj.key,
+					version: obj.version
+				};
+			}
+			
+			for (let type of types) {
+				let obj = new Zotero[Zotero.Utilities.capitalize(type)];
+				obj.libraryID = Zotero.Libraries.userLibraryID;
+				obj.key = objs[type].key;
+				yield obj.loadPrimaryData();
+				assert.equal(obj.version, objs[type].version);
+			}
+		})
+	})
+	
+	describe("#loadAllData()", function () {
+		it("should load data on a regular item", function* () {
+			var item = new Zotero.Item('book');
+			var id = yield item.saveTx();
+			yield item.loadAllData();
+			assert.throws(item.getNote.bind(item), 'getNote() can only be called on notes and attachments');
+		})
+		
+		it("should load data on an attachment item", function* () {
+			var item = new Zotero.Item('attachment');
+			var id = yield item.saveTx();
+			yield item.loadAllData();
+			assert.equal(item.getNote(), '');
+		})
+		
+		it("should load data on a note item", function* () {
+			var item = new Zotero.Item('note');
+			var id = yield item.saveTx();
+			yield item.loadAllData();
+			assert.equal(item.getNote(), '');
+		})
+	})
+	
 	describe("#save()", function () {
 		it("should add new identifiers to cache", function* () {
 			// Collection