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.

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) {
+	var primaryFields = this._ObjectsClass.primaryFields;
-		if (row[col] === undefined) {
+	for (let i=0; i<primaryFields.length; i++) {
-			Zotero.debug('Skipping missing collection field ' + col);
+		let col = primaryFields[i];
+		try {
+			var val = row[col];
 		}
+		catch (e) {
+			Zotero.debug('Skipping missing ' + this._objectType + ' field ' + col);
+			continue;
 		}
 		
-	this._id = row.collectionID;
+		switch (col) {
-	this._libraryID = parseInt(row.libraryID);
+		case this._ObjectsClass.idColumn:
-	this._key = row.key;
+			col = 'id';
-	this._name = row.name;
+			break;
-	this._parentID = row.parentID || false;
+		
-	this._parentKey = row.parentKey || false;
+		// Integer
-	this._version = parseInt(row.version);
+		case 'libraryID':
-	this._synced = !!row.synced;
+			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._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);
 		}
 	}
 });

chrome/content/zotero/xpcom/data/dataObject.js

@@ -100,7 +100,9 @@ Zotero.DataObject.prototype._get = function (field) {
 	if (this['_' + field] !== null) {
 		return this['_' + field];
 	}
+	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) {

chrome/content/zotero/xpcom/data/dataObjects.js

@@ -277,8 +277,8 @@ Zotero.DataObjects.prototype.getByLibraryAndKeyAsync = Zotero.Promise.coroutine(
 });
 
 
-Zotero.DataObjects.prototype.exists = function (itemID) {
+Zotero.DataObjects.prototype.exists = function (id) {
-	return !!this.getLibraryAndKeyFromID(itemID);
+	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;
+						this.registerObject(obj);
-						obj._inCache = true;
 					}
 				}
 				loaded[id] = obj;

chrome/content/zotero/xpcom/data/item.js

@@ -316,24 +316,27 @@ 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':
 			case 'numNotes':
@@ -686,7 +689,11 @@ Zotero.Item.prototype.setField = function(field, value, loadIn) {
 		*/
 		
 		// If field value has changed
-		if (this['_' + field] != value || field == 'synced') {
+		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
@@ -704,10 +711,6 @@ Zotero.Item.prototype.setField = function(field, value, loadIn) {
 			}
 			this._changed.primaryData[field] = true;
 		}
-		}
-		else {
-			Zotero.debug("Field '" + field + "' has not changed", 4);
-		}
 		return true;
 	}
 	

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;
+	var primaryFields = this._ObjectsClass.primaryFields;
-	this._libraryID = parseInt(row.libraryID);
+	for (let i=0; i<primaryFields.length; i++) {
-	this._key = row.key;
+		let col = primaryFields[i];
-	this._name = row.name;
+		try {
-	this._version = parseInt(row.version);
+			var val = row[col];
-	this._synced = !!row.synced;
+		}
+		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,6 +171,7 @@ Zotero.Search.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		yield Zotero.DB.queryAsync(sql, env.sqlValues);
 	}
 	
+	if (this._changed.conditions) {
 		if (!isNew) {
 			var sql = "DELETE FROM savedSearchConditions WHERE savedSearchID=?";
 			yield Zotero.DB.queryAsync(sql, this.id);
@@ -168,6 +200,7 @@ Zotero.Search.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 			yield Zotero.DB.queryAsync(sql, sqlParams);
 			i++;
 		}
+	}
 });
 
 Zotero.Search.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env) {
@@ -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) {
+	Zotero.debug("Loading conditions for search " + this.libraryKey);
-		return;
-	}
 	
 	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');
 });
 
 

test/tests/dataObjectTest.js

@@ -3,26 +3,18 @@
 describe("Zotero.DataObject", function() {
 	var types = ['collection', 'item', 'search'];
 	
-	describe("#loadAllData()", function () {
+	describe("#key", function () {
-		it("should load data on a regular item", function* () {
+		it("shouldn't update .loaded on get if unset", function* () {
-			var item = new Zotero.Item('book');
+			for (let type of types) {
-			var id = yield item.saveTx();
+				if (type == 'item') {
-			yield item.loadAllData();
+					var param = 'book';
-			assert.throws(item.getNote.bind(item), 'getNote() can only be called on notes and attachments');
+				}
-		})
+				let obj = new Zotero[Zotero.Utilities.capitalize(type)](param);
-		
+				obj.libraryID = Zotero.Libraries.userLibraryID;
-		it("should load data on an attachment item", function* () {
+				assert.isNull(obj.key);
-			var item = new Zotero.Item('attachment');
+				assert.isFalse(obj._loaded.primaryData);
-			var id = yield item.saveTx();
+				obj.key = Zotero.DataObjectUtilities.generateKey();
-			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(), '');
 		})
 	})
 	
@@ -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