From e683b2be07b3220a610089e765674c0429515c89 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Tue, 11 Jul 2017 01:22:07 -0400
Subject: [PATCH] Fix a potential sync error with child attachments

If a standalone attachment existed in a collection and then was added to
a parent (e.g., via Create Parent Item), and attachment metadata was
also changed at the same time (e.g., due to file syncing), the
'collection item must be top level' trigger could throw on another
syncing computer. To work around this, remove collections first, then
make changes to the parentItemID columns, and then add new collections.
---
 chrome/content/zotero/xpcom/data/item.js | 192 ++++++++++++-----------
 test/tests/itemTest.js                   |  16 ++
 2 files changed, 115 insertions(+), 93 deletions(-)

diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js
index 226292171e..4b0d4e5c80 100644
--- a/chrome/content/zotero/xpcom/data/item.js
+++ b/chrome/content/zotero/xpcom/data/item.js
@@ -1382,7 +1382,7 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		}
 	}
 	
-	// Parent item
+	// Parent item (DB update is done below after collection removals)
 	var parentItemKey = this.parentKey;
 	var parentItemID = this.ObjectsClass.getIDFromLibraryAndKey(this.libraryID, parentItemKey) || null;
 	if (this._changed.parentKey) {
@@ -1411,9 +1411,6 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 			}
 		}
 		else {
-			let type = Zotero.ItemTypes.getName(itemTypeID);
-			let Type = type[0].toUpperCase() + type.substr(1);
-			
 			if (parentItemKey) {
 				if (!parentItemID) {
 					// TODO: clear caches
@@ -1494,15 +1491,6 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 				}
 			}
 			
-			// Update DB, if not a note or attachment we're changing below
-			if (!this._changed.attachmentData &&
-					(!this._changed.note || !this.isNote())) {
-				let sql = "UPDATE item" + Type + "s SET parentItemID=? "
-							+ "WHERE itemID=?";
-				let bindParams = [parentItemID, this.id];
-				yield Zotero.DB.queryAsync(sql, bindParams);
-			}
-			
 			// Update the counts of the previous and new sources
 			if (oldParentItemID) {
 				reloadParentChildItems[oldParentItemID] = true;
@@ -1579,6 +1567,67 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		yield Zotero.DB.queryAsync(sql, itemID);
 	}
 	
+	// Collections
+	//
+	// Only diffing and removal are done here. Additions have to be done below after parentItemID has
+	// been updated in itemAttachments/itemNotes, since a child item that was made a standalone item and
+	// added to a collection can't be added to the collection while it still has a parent, and vice
+	// versa, due to the trigger checks on collectionItems/itemAttachments/itemNotes.
+	if (this._changed.collections) {
+		if (libraryType == 'publications') {
+			throw new Error("Items in My Publications cannot be added to collections");
+		}
+		
+		let oldCollections = this._previousData.collections || [];
+		let newCollections = this._collections;
+		
+		let toAdd = Zotero.Utilities.arrayDiff(newCollections, oldCollections);
+		let toRemove = Zotero.Utilities.arrayDiff(oldCollections, newCollections);
+		
+		env.collectionsAdded = toAdd;
+		env.collectionsRemoved = toRemove;
+		
+		if (toRemove.length) {
+			let sql = "DELETE FROM collectionItems WHERE itemID=? AND collectionID IN ("
+				+ toRemove.join(',')
+				+ ")";
+			yield Zotero.DB.queryAsync(sql, this.id);
+			
+			for (let i=0; i<toRemove.length; i++) {
+				let collectionID = toRemove[i];
+				
+				if (!env.options.skipNotifier) {
+					Zotero.Notifier.queue(
+						'remove',
+						'collection-item',
+						collectionID + '-' + this.id,
+						{},
+						env.options.notifierQueue
+					);
+				}
+			}
+			
+			// Remove this item from any loaded collections' cached item lists after commit
+			Zotero.DB.addCurrentCallback("commit", function () {
+				for (let i = 0; i < toRemove.length; i++) {
+					this.ContainerObjectsClass.unregisterChildItem(toRemove[i], this.id);
+				}
+			}.bind(this));
+		}
+	}
+	
+	// Add parent item for existing item, if note or attachment data isn't going to be updated below
+	//
+	// Technically this doesn't have to go below collection removals, but only because the
+	// 'collectionitem must be top level' trigger check applies only to INSERTs, not UPDATEs, which was
+	// probably done in an earlier attempt to solve this problem.
+	if (!isNew && this._changed.parentKey && !this._changed.note && !this._changed.attachmentData) {
+		let type = Zotero.ItemTypes.getName(itemTypeID);
+		let Type = type[0].toUpperCase() + type.substr(1);
+		let sql = "UPDATE item" + Type + "s SET parentItemID=? WHERE itemID=?";
+		yield Zotero.DB.queryAsync(sql, [parentItemID, this.id]);
+	}
+	
 	// Note
 	if ((isNew && this.isNote()) || this._changed.note) {
 		if (!isNew) {
@@ -1627,7 +1676,6 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 			reloadParentChildItems[parentItemID] = true;
 		}
 	}
-	
 	if (this._changed.attachmentData) {
 		let sql = "REPLACE INTO itemAttachments "
 			+ "(itemID, parentItemID, linkMode, contentType, charsetID, path, "
@@ -1666,6 +1714,39 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		}
 	}
 	
+	// Add to new collections
+	if (env.collectionsAdded) {
+		let toAdd = env.collectionsAdded;
+		for (let i=0; i<toAdd.length; i++) {
+			let collectionID = toAdd[i];
+			
+			let sql = "SELECT IFNULL(MAX(orderIndex)+1, 0) FROM collectionItems "
+				+ "WHERE collectionID=?";
+			let orderIndex = yield Zotero.DB.valueQueryAsync(sql, collectionID);
+			
+			sql = "INSERT OR IGNORE INTO collectionItems "
+				+ "(collectionID, itemID, orderIndex) VALUES (?, ?, ?)";
+			yield Zotero.DB.queryAsync(sql, [collectionID, this.id, orderIndex]);
+			
+			if (!env.options.skipNotifier) {
+				Zotero.Notifier.queue(
+					'add',
+					'collection-item',
+					collectionID + '-' + this.id,
+					{},
+					env.options.notifierQueue
+				);
+			}
+		}
+		
+		// Add this item to any loaded collections' cached item lists after commit
+		Zotero.DB.addCurrentCallback("commit", function () {
+			for (let i = 0; i < toAdd.length; i++) {
+				this.ContainerObjectsClass.registerChildItem(toAdd[i], this.id);
+			}
+		}.bind(this));
+	}
+	
 	// Tags
 	if (this._changed.tags) {
 		let oldTags = this._previousData.tags || [];
@@ -1720,81 +1801,6 @@ Zotero.Item.prototype._saveData = Zotero.Promise.coroutine(function* (env) {
 		}
 	}
 	
-	// Collections
-	if (this._changed.collections) {
-		if (libraryType == 'publications') {
-			throw new Error("Items in My Publications cannot be added to collections");
-		}
-		
-		let oldCollections = this._previousData.collections || [];
-		let newCollections = this._collections;
-		
-		let toAdd = Zotero.Utilities.arrayDiff(newCollections, oldCollections);
-		let toRemove = Zotero.Utilities.arrayDiff(oldCollections, newCollections);
-
-		env.collectionsAdded = toAdd;
-		env.collectionsRemoved = toRemove;
-		
-		if (toAdd.length) {
-			for (let i=0; i<toAdd.length; i++) {
-				let collectionID = toAdd[i];
-				
-				let sql = "SELECT IFNULL(MAX(orderIndex)+1, 0) FROM collectionItems "
-					+ "WHERE collectionID=?";
-				let orderIndex = yield Zotero.DB.valueQueryAsync(sql, collectionID);
-				
-				sql = "INSERT OR IGNORE INTO collectionItems "
-					+ "(collectionID, itemID, orderIndex) VALUES (?, ?, ?)";
-				yield Zotero.DB.queryAsync(sql, [collectionID, this.id, orderIndex]);
-				
-				if (!env.options.skipNotifier) {
-					Zotero.Notifier.queue(
-						'add',
-						'collection-item',
-						collectionID + '-' + this.id,
-						{},
-						env.options.notifierQueue
-					);
-				}
-			}
-			
-			// Add this item to any loaded collections' cached item lists after commit
-			Zotero.DB.addCurrentCallback("commit", function () {
-				for (let i = 0; i < toAdd.length; i++) {
-					this.ContainerObjectsClass.registerChildItem(toAdd[i], this.id);
-				}
-			}.bind(this));
-		}
-		
-		if (toRemove.length) {
-			let sql = "DELETE FROM collectionItems WHERE itemID=? AND collectionID IN ("
-				+ toRemove.join(',')
-				+ ")";
-			yield Zotero.DB.queryAsync(sql, this.id);
-			
-			for (let i=0; i<toRemove.length; i++) {
-				let collectionID = toRemove[i];
-				
-				if (!env.options.skipNotifier) {
-					Zotero.Notifier.queue(
-						'remove',
-						'collection-item',
-						collectionID + '-' + this.id,
-						{},
-						env.options.notifierQueue
-					);
-				}
-			}
-			
-			// Remove this item from any loaded collections' cached item lists after commit
-			Zotero.DB.addCurrentCallback("commit", function () {
-				for (let i = 0; i < toRemove.length; i++) {
-					this.ContainerObjectsClass.unregisterChildItem(toRemove[i], this.id);
-				}
-			}.bind(this));
-		}
-	}
-	
 	// Update child item counts and contents
 	if (reloadParentChildItems) {
 		for (let parentItemID in reloadParentChildItems) {
@@ -4157,10 +4163,6 @@ Zotero.Item.prototype.fromJSON = function (json) {
 			this.setTags(json.tags);
 			break;
 		
-		case 'collections':
-			this.setCollections(json.collections);
-			break;
-		
 		case 'relations':
 			this.setRelations(json.relations);
 			break;
@@ -4220,6 +4222,10 @@ Zotero.Item.prototype.fromJSON = function (json) {
 		}
 	}
 	
+	if (json.collections || this._collections.length) {
+		this.setCollections(json.collections);
+	}
+	
 	// Clear existing fields not specified
 	var previousFields = this.getUsedFields(true);
 	for (let field of previousFields) {
diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js
index 9bbb2bf73e..508465e421 100644
--- a/test/tests/itemTest.js
+++ b/test/tests/itemTest.js
@@ -1448,6 +1448,22 @@ describe("Zotero.Item", function () {
 			assert.strictEqual(item.getField('accessDate'), '');
 		});
 		
+		it("should remove child item from collection if 'collections' property not provided", function* () {
+			var collection = yield createDataObject('collection');
+			// Create standalone attachment in collection
+			var attachment = yield importFileAttachment('test.png', { collections: [collection.id] });
+			var item = yield createDataObject('item', { collections: [collection.id] });
+			
+			var json = attachment.toJSON();
+			json.path = 'storage:test2.png';
+			// Add to parent, which implicitly removes from collection
+			json.parentItem = item.key;
+			delete json.collections;
+			Zotero.debug(json);
+			attachment.fromJSON(json);
+			yield attachment.save();
+		});
+		
 		it("should ignore unknown fields", function* () {
 			var json = {
 				itemType: "journalArticle",