Data layer fixes

- Fixes some saving and erasing issues with collections and searches
- Adds Zotero.DataObject::eraseTx() to automatically start transaction,
  and have .erase() log a warning like .save()
- Adds createUnsavedDataObject() and createDataObject() helper functions
  for tests

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

@@ -590,72 +590,70 @@ Zotero.Collection.prototype.clone = function (includePrimary, newCollection) {
 /**
 * Deletes collection and all descendent collections (and optionally items)
 **/
-Zotero.Collection.prototype.erase = function(deleteItems) {
+Zotero.Collection.prototype._eraseData = Zotero.Promise.coroutine(function* (deleteItems) {
+	Zotero.DB.requireTransaction();
+	
 	var collections = [this.id];
+	
+	var descendents = yield this.getDescendents(false, null, true);
+	var items = [];
+	
 	var notifierData = {};
+	notifierData[this.id] = {
+		libraryID: this.libraryID,
+		key: this.key
+	};
 	
-	return Zotero.DB.executeTransaction(function* () {
+	var del = [];
-		var descendents = yield this.getDescendents(false, null, true);
+	for(var i=0, len=descendents.length; i<len; i++) {
-		var items = [];
+		// Descendent collections
-		notifierData[this.id] = {
+		if (descendents[i].type == 'collection') {
-			libraryID: this.libraryID,
+			collections.push(descendents[i].id);
-			key: this.key
+			var c = yield this.ObjectsClass.getAsync(descendents[i].id);
-		};
+			if (c) {
-		
+				notifierData[c.id] = {
-		var del = [];
+					libraryID: c.libraryID,
-		for(var i=0, len=descendents.length; i<len; i++) {
+					key: c.key
-			// Descendent collections
+				};
-			if (descendents[i].type == 'collection') {
-				collections.push(descendents[i].id);
-				var c = yield this.ObjectsClass.getAsync(descendents[i].id);
-				if (c) {
-					notifierData[c.id] = {
-						libraryID: c.libraryID,
-						key: c.key
-					};
-				}
-			}
-			// Descendent items
-			else {
-				// Delete items from DB
-				if (deleteItems) {
-					del.push(descendents[i].id);
-				}
 			}
 		}
-		if (del.length) {
+		// Descendent items
-			yield this.ChildObjects.trash(del);
+		else {
+			// Delete items from DB
+			if (deleteItems) {
+				del.push(descendents[i].id);
+			}
 		}
+	}
+	if (del.length) {
+		yield this.ChildObjects.trash(del);
+	}
 	
-		// Remove relations
+	// Remove relations
-		var uri = Zotero.URI.getCollectionURI(this);
+	var uri = Zotero.URI.getCollectionURI(this);
-		yield Zotero.Relations.eraseByURI(uri);
+	yield Zotero.Relations.eraseByURI(uri);
 	
-		var placeholders = collections.map(function () '?').join();
+	var placeholders = collections.map(function () '?').join();
 	
-		// Remove item associations for all descendent collections
+	// Remove item associations for all descendent collections
-		yield Zotero.DB.queryAsync('DELETE FROM collectionItems WHERE collectionID IN '
+	yield Zotero.DB.queryAsync('DELETE FROM collectionItems WHERE collectionID IN '
-			+ '(' + placeholders + ')', collections);
+		+ '(' + placeholders + ')', collections);
 	
-		// Remove parent definitions first for FK check
+	// Remove parent definitions first for FK check
-		yield Zotero.DB.queryAsync('UPDATE collections SET parentCollectionID=NULL '
+	yield Zotero.DB.queryAsync('UPDATE collections SET parentCollectionID=NULL '
-			+ 'WHERE parentCollectionID IN (' + placeholders + ')', collections);
+		+ 'WHERE parentCollectionID IN (' + placeholders + ')', collections);
 	
-		// And delete all descendent collections
+	// And delete all descendent collections
-		yield Zotero.DB.queryAsync ('DELETE FROM collections WHERE collectionID IN '
+	yield Zotero.DB.queryAsync ('DELETE FROM collections WHERE collectionID IN '
-			+ '(' + placeholders + ')', collections);
+		+ '(' + placeholders + ')', collections);
 	
-		// TODO: Update member items
+	// TODO: Update member items
-	}.bind(this))
+	// Clear deleted collection from internal memory
-	.then(() => {
+	this.ObjectsClass.unload(collections);
-		// Clear deleted collection from internal memory
+	//return Zotero.Collections.reloadAll();
-		this.ObjectsClass.unload(collections);
+	
-		//return Zotero.Collections.reloadAll();
+	Zotero.Notifier.trigger('delete', 'collection', collections, notifierData);
-	})
+});
-	.then(function () {
-		Zotero.Notifier.trigger('delete', 'collection', collections, notifierData);
-	});
-}
 
 
 Zotero.Collection.prototype.isCollection = function() {

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

@@ -209,7 +209,7 @@ Zotero.Collections = function() {
 			}
 			
 			this.unload(ids);
-		});
+		}.bind(this));
 	};
 	
 	Zotero.DataObjects.call(this);

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

@@ -714,20 +714,45 @@ Zotero.DataObject.prototype._recoverFromSaveError = Zotero.Promise.coroutine(fun
 /**
  * Delete object from database
  */
-Zotero.DataObject.prototype.erase = Zotero.Promise.coroutine(function* () {
+Zotero.DataObject.prototype.erase = Zotero.Promise.coroutine(function* (options) {
-	var env = {};
+	options = options || {};
+	var env = {
+		options: options
+	};
+	
+	if (!env.options.tx && !Zotero.DB.inTransaction()) {
+		Zotero.logError("erase() called on Zotero." + this._ObjectType + " without a wrapping "
+			+ "transaction -- use eraseTx() instead");
+		Zotero.debug((new Error).stack, 2);
+		env.options.tx = true;
+	}
 	
 	var proceed = yield this._eraseInit(env);
 	if (!proceed) return false;
 	
 	Zotero.debug('Deleting ' + this.objectType + ' ' + this.id);
 	
-	Zotero.DB.requireTransaction();
+	if (env.options.tx) {
-	yield this._eraseData(env);
+		return Zotero.DB.executeTransaction(function* () {
-	yield this._erasePreCommit(env);
+			yield this._eraseData(env);
-	return this._erasePostCommit(env);
+			yield this._erasePreCommit(env);
+			return this._erasePostCommit(env);
+		}.bind(this))
+	}
+	else {
+		Zotero.DB.requireTransaction();
+		yield this._eraseData(env);
+		yield this._erasePreCommit(env);
+		return this._erasePostCommit(env);
+	}
 });
 
+Zotero.DataObject.prototype.eraseTx = function (options) {
+	options = options || {};
+	options.tx = true;
+	return this.erase(options);
+};
+
 Zotero.DataObject.prototype._eraseInit = function(env) {
 	if (!this.id) return Zotero.Promise.resolve(false);
 	

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

@@ -1174,10 +1174,7 @@ Zotero.Item.prototype.isEditable = function() {
 }
 
 Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
-	// Sanity check
+	Zotero.DB.requireTransaction();
-	if (!Zotero.DB.inTransaction()) {
-		throw new Error("Not in transaction saving item " + this.libraryKey);
-	}
 	
 	var isNew = env.isNew;
 	var options = env.options;
@@ -1729,10 +1726,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		}
 	}
 	
-	// Sanity check
+	Zotero.DB.requireTransaction();
-	if (!Zotero.DB.inTransaction()) {
-		throw new Error("Not in transaction saving item " + this.libraryKey);
-	}
 });
 
 Zotero.Item.prototype._finalizeSave = Zotero.Promise.coroutine(function* (env) {
@@ -3919,6 +3913,8 @@ Zotero.Item.prototype._eraseInit = Zotero.Promise.coroutine(function* (env) {
 });
 
 Zotero.Item.prototype._eraseData = Zotero.Promise.coroutine(function* (env) {
+	Zotero.DB.requireTransaction();
+	
 	// Remove item from parent collections
 	var parentCollectionIDs = this.collections;
 	if (parentCollectionIDs) {

chrome/content/zotero/xpcom/search.js

@@ -92,25 +92,28 @@ Zotero.Search.prototype._set = function (field, value) {
 		return this._setIdentifier(field, value);
 	}
 	
-	switch (field) {
-	case 'name':
-		value = value.trim().normalize();
-		break;
-	
-	case 'version':
-		value = parseInt(value);
-		break;
-	
-	case 'synced':
-		value = !!value;
-		break;
-	}
-	
 	this._requireData('primaryData');
 	
+	switch (field) {
+		case 'name':
+			value = value.trim().normalize();
+			break;
+		
+		case 'version':
+			value = parseInt(value);
+			break;
+		
+		case 'synced':
+			value = !!value;
+			break;
+	}
+	
 	if (this['_' + field] != value) {
 		this._markFieldChange(field, this['_' + field]);
-		this._changed.primaryData = true;
+		if (!this._changed.primaryData) {
+			this._changed.primaryData = {};
+		}
+		this._changed.primaryData[field] = true;
 		
 		switch (field) {
 			default:
@@ -119,13 +122,12 @@ Zotero.Search.prototype._set = function (field, value) {
 	}
 }
 
-
 Zotero.Search.prototype.loadFromRow = function (row) {
 	this._id = row.savedSearchID;
-	this._libraryID = row.libraryID;
+	this._libraryID = parseInt(row.libraryID);
 	this._key = row.key;
 	this._name = row.name;
-	this._version = row.version;
+	this._version = parseInt(row.version);
 	this._synced = !!row.synced;
 	
 	this._loaded.primaryData = true;
@@ -147,8 +149,7 @@ Zotero.Search.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 	var searchID = env.id = this._id = this.id ? this.id : yield Zotero.ID.get('savedSearches');
 	
 	env.sqlColumns.push(
-		'savedSearchName',
+		'savedSearchName'
-		'savedSearchID'
 	);
 	env.sqlValues.push(
 		{ string: this.name }
@@ -251,6 +252,25 @@ Zotero.Search.prototype.clone = function (libraryID) {
 };
 
 
+Zotero.Search.prototype._eraseData = Zotero.Promise.coroutine(function* () {
+	Zotero.DB.requireTransaction();
+	
+	var notifierData = {};
+	notifierData[this.id] = {
+		libraryID: this.libraryID,
+		key: this.key
+	};
+	
+	var sql = "DELETE FROM savedSearchConditions WHERE savedSearchID=?";
+	yield Zotero.DB.queryAsync(sql, this.id);
+	
+	var sql = "DELETE FROM savedSearches WHERE savedSearchID=?";
+	yield Zotero.DB.queryAsync(sql, this.id);
+	
+	Zotero.Notifier.trigger('delete', 'search', this.id, notifierData);
+});
+
+
 Zotero.Search.prototype.addCondition = function (condition, operator, value, required) {
 	this._requireData('conditions');
 	
@@ -1690,29 +1710,13 @@ Zotero.Searches = function() {
 	 */
 	this.erase = Zotero.Promise.coroutine(function* (ids) {
 		ids = Zotero.flattenArguments(ids);
-		var notifierData = {};
 		
 		yield Zotero.DB.executeTransaction(function* () {
 			for (let i=0; i<ids.length; i++) {
-				let id = ids[i];
+				let search = yield Zotero.Searches.getAsync(ids[i]);
-				var search = new Zotero.Search;
+				yield search.erase();
-				search.id = id;
-				yield search.loadPrimaryData();
-				yield search.loadConditions();
-				notifierData[id] = {
-					libraryID: this.libraryID,
-					old: search.serialize() // TODO: replace with toJSON()
-				};
-				
-				var sql = "DELETE FROM savedSearchConditions WHERE savedSearchID=?";
-				yield Zotero.DB.queryAsync(sql, id);
-				
-				var sql = "DELETE FROM savedSearches WHERE savedSearchID=?";
-				yield Zotero.DB.queryAsync(sql, id);
 			}
 		}.bind(this));
-		
-		Zotero.Notifier.trigger('delete', 'search', ids, notifierData);
 	});
 	
 	

test/content/support.js

@@ -145,6 +145,37 @@ function waitForCallback(cb, interval, timeout) {
 	return deferred.promise;
 }
 
+//
+// Data objects
+//
+function createUnsavedDataObject(objectType, params) {
+	params = params || {};
+	if (objectType == 'item') {
+		var param = 'book';
+	}
+	var obj = new Zotero[Zotero.Utilities.capitalize(objectType)](param);
+	switch (objectType) {
+	case 'collection':
+	case 'search':
+		obj.name = params.name !== undefined ? params.name : "Test";
+		break;
+	}
+	if (params.version !== undefined) {
+		obj.version = params.version
+	}
+	if (params.synced !== undefined) {
+		obj.synced = params.synced
+	}
+	return obj;
+}
+
+var createDataObject = Zotero.Promise.coroutine(function* (objectType, params, saveOptions) {
+	var obj = createUnsavedDataObject(objectType, params);
+	var id = yield obj.saveTx(saveOptions);
+	var objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType);
+	return objectsClass.getAsync(id);
+});
+
 /**
  * Return a promise for the error thrown by a promise, or false if none
  */

test/tests/dataObjectTest.js

@@ -29,13 +29,32 @@ describe("Zotero.DataObject", function() {
 		})
 	})
 	
+	describe("#version", function () {
+		it("should be set to 0 after creating object", function* () {
+			for (let type of types) {
+				let obj = yield createDataObject(type);
+				assert.equal(obj.version, 0);
+				yield obj.eraseTx();
+			}
+		})
+		
+		it("should be set after creating object", function* () {
+			for (let type of types) {
+				Zotero.logError(type);
+				let obj = yield createDataObject(type, { version: 1234 });
+				assert.equal(obj.version, 1234, type + " version mismatch");
+				yield obj.eraseTx();
+			}
+		})
+	})
+	
 	describe("#synced", function () {
 		it("should be set to false after creating item", function* () {
 			var item = new Zotero.Item("book");
 			var id = yield item.saveTx();
 			item = yield Zotero.Items.getAsync(id);
 			assert.isFalse(item.synced);
-			yield Zotero.Items.erase(id);
+			item.eraseTx();
 		});
 		
 		it("should be set to true when changed", function* () {
@@ -44,10 +63,10 @@ describe("Zotero.DataObject", function() {
 			item = yield Zotero.Items.getAsync(id);
 			
 			item.synced = 1;
-			yield item.save();
+			yield item.saveTx();
 			assert.ok(item.synced);
 			
-			yield Zotero.Items.erase(id);
+			item.eraseTx();
 		});
 		
 		it("should be set to false after modifying item", function* () {
@@ -56,14 +75,14 @@ describe("Zotero.DataObject", function() {
 			item = yield Zotero.Items.getAsync(id);
 			
 			item.synced = 1;
-			yield item.save();
+			yield item.saveTx();
 			
 			yield item.loadItemData();
 			item.setField('title', 'test');
-			yield item.save();
+			yield item.saveTx();
 			assert.isFalse(item.synced);
 			
-			yield Zotero.Items.erase(id);
+			item.eraseTx();
 		});
 		
 		it("should be unchanged if skipSyncedUpdate passed", function* () {
@@ -72,16 +91,16 @@ describe("Zotero.DataObject", function() {
 			item = yield Zotero.Items.getAsync(id);
 			
 			item.synced = 1;
-			yield item.save();
+			yield item.saveTx();
 			
 			yield item.loadItemData();
 			item.setField('title', 'test');
-			yield item.save({
+			yield item.saveTx({
 				skipSyncedUpdate: true
 			});
 			assert.ok(item.synced);
 			
-			yield Zotero.Items.erase(id);
+			item.eraseTx();
 		});
 	})