Skip to content

Commit

Permalink
Fix a potential sync error with child attachments
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@dstillman
dstillman committed Jul 11, 2017
1 parent 3272387 commit e683b2b
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 93 deletions.
192 changes: 99 additions & 93 deletions chrome/content/zotero/xpcom/data/item.js
View file
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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, "
Expand Down Expand Up @@ -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 || [];
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
16 changes: 16 additions & 0 deletions test/tests/itemTest.js
View file
Expand Up @@ -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",
Expand Down

0 comments on commit e683b2b

Please sign in to comment.