commit f0770fa84da3c3b1a5a5ee24b90a8d3b9851fd85
parent 7f8699b9371c9a20d95ec45bd217db4342b7572e
Author: Dan Stillman <dstillman@zotero.org>
Date: Fri, 27 Oct 2017 01:07:00 -0400
Fix various conflict resolution bugs
Among other things, when choosing the local side for a conflict, the
remote version could still end up being saved.
Diffstat:
5 files changed, 233 insertions(+), 132 deletions(-)
diff --git a/chrome/content/zotero/merge.js b/chrome/content/zotero/merge.js
@@ -168,7 +168,6 @@ var Zotero_Merge_Window = new function () {
if (x.data) {
x.data.version = _conflicts[i][x.selected].version;
}
- a[i] = x.data;
})
_io.dataOut = _merged;
diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js
@@ -880,10 +880,9 @@ Zotero.Sync.Data.Engine.prototype._downloadDeletions = Zotero.Promise.coroutine(
function (chunk) {
return Zotero.DB.executeTransaction(function* () {
for (let json of chunk) {
- if (!json.deleted) continue;
- let obj = objectsClass.getByLibraryAndKey(
- this.libraryID, json.key
- );
+ let data = json.data;
+ if (!data.deleted) continue;
+ let obj = objectsClass.getByLibraryAndKey(this.libraryID, data.key);
if (!obj) {
Zotero.logError("Remotely deleted " + objectType
+ " didn't exist after conflict resolution");
diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js
@@ -835,7 +835,6 @@ Zotero.Sync.Data.Local = {
let cachedJSON = yield this.getCacheObject(
objectType, obj.libraryID, obj.key, obj.version
);
-
let result = this._reconcileChanges(
objectType,
cachedJSON.data,
@@ -844,20 +843,21 @@ Zotero.Sync.Data.Local = {
['mtime', 'md5', 'dateAdded', 'dateModified']
);
- // If no changes, update local version number and mark as synced
+ // If no changes, just update local version number and mark as synced
if (!result.changes.length && !result.conflicts.length) {
Zotero.debug("No remote changes to apply to local "
+ objectType + " " + obj.libraryKey);
-
+ saveOptions.skipData = true;
+ // If local object is different but we ignored the changes
+ // (e.g., ISBN hyphenation), save as unsynced. Since we're skipping
+ // data, the local version won't be overwritten.
+ if (result.localChanged) {
+ saveOptions.saveAsUnsynced = true;
+ }
let saveResults = yield this._saveObjectFromJSON(
obj,
jsonObject,
- {
- skipData: true,
- notifierQueue,
- // Save as unsynced
- saveAsChanged: !!result.localChanged
- }
+ saveOptions
);
results.push(saveResults);
if (!saveResults.processed) {
@@ -902,11 +902,6 @@ Zotero.Sync.Data.Local = {
jsonDataLocal[x] = jsonData[x];
})
jsonObject.data = jsonDataLocal;
-
- // Save as unsynced
- if (results.localChanged) {
- saveOptions.saveAsChanged = true;
- }
}
}
// Object doesn't exist locally
@@ -1190,19 +1185,26 @@ Zotero.Sync.Data.Local = {
let notifierQueues = [];
try {
for (let i = 0; i < mergeData.length; i++) {
- // Batch notifier updates
+ // Batch notifier updates, despite multiple transactions
if (notifierQueues.length == batchSize) {
yield Zotero.Notifier.commit(notifierQueues);
notifierQueues = [];
}
let notifierQueue = new Zotero.Notifier.Queue;
- let json = mergeData[i];
+ let json = mergeData[i].data;
let saveOptions = {};
Object.assign(saveOptions, options);
- // Tell _saveObjectFromJSON() to save as unsynced
- saveOptions.saveAsChanged = true;
+ // If choosing local object, save as unsynced with remote version (or 0 if remote is
+ // deleted) and remote object in cache, to simulate a save and edit
+ if (mergeData[i].selected == 'left') {
+ json.version = conflicts[i].right.version || 0;
+ saveOptions.saveAsUnsynced = true;
+ if (conflicts[i].right.version) {
+ saveOptions.cacheObject = conflicts[i].right;
+ }
+ }
saveOptions.notifierQueue = notifierQueue;
// Errors have to be thrown in order to roll back the transaction, so catch
@@ -1259,9 +1261,7 @@ Zotero.Sync.Data.Local = {
saveOptions.skipCache = true;
}
- let saveResults = yield this._saveObjectFromJSON(
- obj, json, saveOptions
- );
+ let saveResults = yield this._saveObjectFromJSON(obj, json, saveOptions);
results.push(saveResults);
if (!saveResults.processed) {
throw saveResults.error;
@@ -1360,7 +1360,7 @@ Zotero.Sync.Data.Local = {
yield this._checkAttachmentForDownload(obj, json.data.mtime, options.isNewObject);
}
obj.version = json.data.version;
- if (!options.saveAsChanged) {
+ if (!options.saveAsUnsynced) {
obj.synced = true;
}
yield obj.save({
@@ -1374,13 +1374,17 @@ Zotero.Sync.Data.Local = {
return;
}
});
- yield this.saveCacheObject(obj.objectType, obj.libraryID, json.data);
- results.processed = true;
-
+ let cacheJSON = options.cacheObject ? options.cacheObject : json.data;
+ yield this.saveCacheObject(obj.objectType, obj.libraryID, cacheJSON);
// Delete older versions of the object in the cache
yield this.deleteCacheObjectVersions(
- obj.objectType, obj.libraryID, json.key, null, json.version - 1
+ obj.objectType,
+ obj.libraryID,
+ json.key,
+ null,
+ cacheJSON.version - 1
);
+ results.processed = true;
// Delete from sync queue
yield this._removeObjectFromSyncQueue(obj.objectType, obj.libraryID, json.key);
@@ -1519,7 +1523,7 @@ Zotero.Sync.Data.Local = {
return {
changes: changeset2,
- conflicts: conflicts
+ conflicts
};
},
diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js
@@ -2972,9 +2972,9 @@ describe("Zotero.Sync.Data.Engine", function () {
yield Zotero.DB.queryAsync("DELETE FROM syncCache");
})
- it("should show conflict resolution window on item conflicts", function* () {
+ it("should show conflict resolution window on item conflicts", async function () {
var libraryID = Zotero.Libraries.userLibraryID;
- ({ engine, client, caller } = yield setup());
+ ({ engine, client, caller } = await setup());
var type = 'item';
var objects = [];
var values = [];
@@ -2988,7 +2988,7 @@ describe("Zotero.Sync.Data.Engine", function () {
});
// Create local object
- let obj = objects[i] = yield createDataObject(
+ let obj = objects[i] = await createDataObject(
type,
{
version: 10,
@@ -3008,7 +3008,7 @@ describe("Zotero.Sync.Data.Engine", function () {
data: jsonData
};
// Save original version in cache
- yield Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [json]);
+ await Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [json]);
// Create updated JSON for download
values[i].right.title = jsonData.title = Zotero.Utilities.randomString();
@@ -3016,7 +3016,7 @@ describe("Zotero.Sync.Data.Engine", function () {
responseJSON.push(json);
// Modify object locally
- yield modifyDataObject(obj, undefined, { skipDateModifiedUpdate: true });
+ await modifyDataObject(obj, undefined, { skipDateModifiedUpdate: true });
values[i].left.title = obj.getField('title');
values[i].left.version = obj.getField('version');
}
@@ -3056,20 +3056,29 @@ describe("Zotero.Sync.Data.Engine", function () {
}
wizard.getButton('finish').click();
})
- yield engine._downloadObjects('item', objects.map(o => o.key));
+ await engine._downloadObjects('item', objects.map(o => o.key));
assert.equal(objects[0].getField('title'), values[0].right.title);
assert.equal(objects[1].getField('title'), values[1].left.title);
assert.equal(objects[0].getField('version'), values[0].right.version);
- assert.equal(objects[1].getField('version'), values[1].left.version);
+ assert.equal(objects[1].getField('version'), values[1].right.version);
- var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', libraryID);
+ // Cache versions should match remote
+ for (let i = 0; i < 2; i++) {
+ let cacheJSON = await Zotero.Sync.Data.Local.getCacheObject(
+ 'item', libraryID, objects[i].key, values[i].right.version
+ );
+ assert.propertyVal(cacheJSON, 'version', values[i].right.version);
+ assert.nestedPropertyVal(cacheJSON, 'data.title', values[i].right.title);
+ }
+
+ var keys = await Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', libraryID);
assert.lengthOf(keys, 0);
});
- it("should show conflict resolution window on note conflicts", function* () {
+ it("should show conflict resolution window on note conflicts", async function () {
var libraryID = Zotero.Libraries.userLibraryID;
- ({ engine, client, caller } = yield setup());
+ ({ engine, client, caller } = await setup());
var type = 'item';
var objects = [];
var values = [];
@@ -3091,7 +3100,7 @@ describe("Zotero.Sync.Data.Engine", function () {
obj.dateModified = Zotero.Date.dateToSQL(
new Date(dateAdded + (i * 60000)), true
);
- yield obj.saveTx();
+ await obj.saveTx();
let jsonData = obj.toJSON();
jsonData.key = obj.key;
@@ -3102,7 +3111,7 @@ describe("Zotero.Sync.Data.Engine", function () {
data: jsonData
};
// Save original version in cache
- yield Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [json]);
+ await Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [json]);
// Create updated JSON for download
values[i].right.note = jsonData.note = Zotero.Utilities.randomString();
@@ -3111,7 +3120,7 @@ describe("Zotero.Sync.Data.Engine", function () {
// Modify object locally
obj.setNote(Zotero.Utilities.randomString());
- yield obj.saveTx({
+ await obj.saveTx({
skipDateModifiedUpdate: true
});
values[i].left.note = obj.getNote();
@@ -3153,21 +3162,33 @@ describe("Zotero.Sync.Data.Engine", function () {
}
wizard.getButton('finish').click();
});
- yield engine._downloadObjects('item', objects.map(o => o.key));
+ await engine._downloadObjects('item', objects.map(o => o.key));
assert.equal(objects[0].getNote(), values[0].right.note);
assert.equal(objects[1].getNote(), values[1].left.note);
- assert.equal(objects[0].getField('version'), values[0].right.version);
- assert.equal(objects[1].getField('version'), values[1].left.version);
+ assert.equal(objects[0].version, values[0].right.version);
+ assert.equal(objects[1].version, values[1].right.version);
+ assert.isTrue(objects[0].synced);
+ assert.isFalse(objects[1].synced);
- var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', libraryID);
+ // Cache versions should match remote
+ for (let i = 0; i < 2; i++) {
+ let cacheJSON = await Zotero.Sync.Data.Local.getCacheObject(
+ 'item', libraryID, objects[i].key, values[i].right.version
+ );
+ assert.propertyVal(cacheJSON, 'version', values[i].right.version);
+ assert.nestedPropertyVal(cacheJSON, 'data.note', values[i].right.note);
+ }
+
+ var keys = await Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', libraryID);
assert.lengthOf(keys, 0);
});
- it("should resolve all remaining conflicts with one side", function* () {
+ it("should resolve all remaining conflicts with local version", async function () {
var libraryID = Zotero.Libraries.userLibraryID;
- ({ engine, client, caller } = yield setup());
- var type = 'item';
+ ({ engine, client, caller } = await setup());
+ var collectionA = await createDataObject('collection');
+ var collectionB = await createDataObject('collection');
var objects = [];
var values = [];
var responseJSON = [];
@@ -3179,8 +3200,8 @@ describe("Zotero.Sync.Data.Engine", function () {
});
// Create object in cache
- let obj = objects[i] = yield createDataObject(
- type,
+ let obj = objects[i] = await createDataObject(
+ 'item',
{
version: 10,
dateAdded: Zotero.Date.dateToSQL(new Date(dateAdded), true),
@@ -3199,17 +3220,26 @@ describe("Zotero.Sync.Data.Engine", function () {
data: jsonData
};
// Save original version in cache
- yield Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [json]);
+ await Zotero.Sync.Data.Local.saveCacheObjects('item', libraryID, [json]);
- // Create new version in cache, simulating a download
+ // Create remote version
values[i].right.title = jsonData.title = Zotero.Utilities.randomString();
+ values[i].right.publisher = jsonData.publisher = Zotero.Utilities.randomString();
+ values[i].right.collections = jsonData.collections = [collectionB.key];
values[i].right.version = json.version = jsonData.version = 15;
responseJSON.push(json);
// Modify object locally
- yield modifyDataObject(obj, undefined, { skipDateModifiedUpdate: true });
+ obj.setField('title', Zotero.Utilities.randomString());
+ obj.setField('extra', Zotero.Utilities.randomString());
+ obj.setCollections([collectionA.key]);
+ await obj.saveTx({
+ skipDateModifiedUpdate: true
+ });
values[i].left.title = obj.getField('title');
- values[i].left.version = obj.getField('version');
+ values[i].left.extra = obj.getField('extra');
+ values[i].left.collections = [collectionA.key];
+ values[i].left.version = obj.version;
}
setResponse({
@@ -3256,18 +3286,147 @@ describe("Zotero.Sync.Data.Engine", function () {
}
wizard.getButton('finish').click();
})
- yield engine._downloadObjects('item', objects.map(o => o.key));
+ await engine._downloadObjects('item', objects.map(o => o.key));
+
+ Zotero.debug('=-=-=-=');
+ Zotero.debug(objects[0].toJSON());
+ Zotero.debug(objects[1].toJSON());
+ Zotero.debug(objects[2].toJSON());
+ // First object should match remote
assert.equal(objects[0].getField('title'), values[0].right.title);
- assert.equal(objects[0].getField('version'), values[0].right.version);
+ assert.equal(objects[0].version, values[0].right.version);
+ assert.isTrue(objects[0].synced);
+
+ // Remaining objects should be marked as unsynced, with remote versions but original values,
+ // as if they were saved and then modified
+ assert.isFalse(objects[1].synced);
+ assert.equal(objects[1].version, values[1].right.version);
assert.equal(objects[1].getField('title'), values[1].left.title);
- assert.equal(objects[1].getField('version'), values[1].left.version);
+ assert.isFalse(objects[2].synced);
assert.equal(objects[2].getField('title'), values[2].left.title);
- assert.equal(objects[2].getField('version'), values[2].left.version);
+ assert.equal(objects[2].version, values[2].right.version);
- var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', libraryID);
+ // All cache versions should match remote
+ for (let i = 0; i < 3; i++) {
+ let cacheJSON = await Zotero.Sync.Data.Local.getCacheObject(
+ 'item', libraryID, objects[i].key, values[i].right.version
+ );
+ assert.propertyVal(cacheJSON, 'version', values[i].right.version);
+ assert.nestedPropertyVal(cacheJSON, 'data.title', values[i].right.title);
+ }
+
+ var keys = await Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', libraryID);
assert.lengthOf(keys, 0);
- })
+ });
+
+
+ it("should resolve all remaining conflicts with remote version", async function () {
+ var libraryID = Zotero.Libraries.userLibraryID;
+ ({ engine, client, caller } = await setup());
+ var objects = [];
+ var values = [];
+ var responseJSON = [];
+ var dateAdded = Date.now() - 86400000;
+ for (let i = 0; i < 3; i++) {
+ values.push({
+ left: {},
+ right: {}
+ });
+
+ // Create object in cache
+ let obj = objects[i] = await createDataObject(
+ 'item',
+ {
+ version: 10,
+ dateAdded: Zotero.Date.dateToSQL(new Date(dateAdded), true),
+ // Set Date Modified values one minute apart to enforce order
+ dateModified: Zotero.Date.dateToSQL(
+ new Date(dateAdded + (i * 60000)), true
+ )
+ }
+ );
+ let jsonData = obj.toJSON();
+ jsonData.key = obj.key;
+ jsonData.version = 10;
+ let json = {
+ key: obj.key,
+ version: jsonData.version,
+ data: jsonData
+ };
+ // Save original version in cache
+ await Zotero.Sync.Data.Local.saveCacheObjects('item', libraryID, [json]);
+
+ // Create remote version
+ values[i].right.title = jsonData.title = Zotero.Utilities.randomString();
+ values[i].right.version = json.version = jsonData.version = 15;
+ responseJSON.push(json);
+
+ // Modify object locally
+ await modifyDataObject(obj, undefined, { skipDateModifiedUpdate: true });
+ values[i].left.title = obj.getField('title');
+ values[i].left.version = obj.version;
+ }
+
+ setResponse({
+ method: "GET",
+ url: `users/1/items?format=json&itemKey=${objects.map(o => o.key).join('%2C')}`
+ + `&includeTrashed=1`,
+ status: 200,
+ headers: {
+ "Last-Modified-Version": 15
+ },
+ json: responseJSON
+ });
+
+ waitForWindow('chrome://zotero/content/merge.xul', function (dialog) {
+ var doc = dialog.document;
+ var wizard = doc.documentElement;
+ var mergeGroup = wizard.getElementsByTagName('zoteromergegroup')[0];
+ var resolveAll = doc.getElementById('resolve-all');
+
+ // 1 (remote)
+ // Remote version should be selected by default
+ assert.equal(mergeGroup.rightpane.getAttribute('selected'), 'true');
+ assert.equal(
+ resolveAll.label,
+ Zotero.getString('sync.conflict.resolveAllRemoteFields')
+ );
+ wizard.getButton('next').click();
+
+ // 2 click Resolve All checkbox
+ assert.equal(mergeGroup.rightpane.getAttribute('selected'), 'true');
+ assert.equal(
+ resolveAll.label,
+ Zotero.getString('sync.conflict.resolveAllRemoteFields')
+ );
+ resolveAll.click();
+
+ if (Zotero.isMac) {
+ assert.isTrue(wizard.getButton('next').hidden);
+ assert.isFalse(wizard.getButton('finish').hidden);
+ }
+ else {
+ // TODO
+ }
+ wizard.getButton('finish').click();
+ })
+ await engine._downloadObjects('item', objects.map(o => o.key));
+
+ assert.equal(objects[0].getField('title'), values[0].right.title);
+ assert.equal(objects[0].version, values[0].right.version);
+ assert.isTrue(objects[0].synced);
+ assert.equal(objects[1].getField('title'), values[1].right.title);
+ assert.equal(objects[1].version, values[1].right.version);
+ assert.isTrue(objects[1].synced);
+ assert.equal(objects[2].getField('title'), values[2].right.title);
+ assert.equal(objects[2].version, values[2].right.version);
+ assert.isTrue(objects[2].synced);
+
+ var keys = await Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', libraryID);
+ assert.lengthOf(keys, 0);
+ });
+
// Note: Conflicts with remote deletions are handled in _startDownload()
it("should handle local item deletion, keeping deletion", function* () {
@@ -3527,67 +3686,6 @@ describe("Zotero.Sync.Data.Engine", function () {
// Deletion shouldn't be in sync delete log
assert.isFalse(yield Zotero.Sync.Data.Local.getDateDeleted('item', libraryID, obj.key));
});
-
- it("should handle note conflict", function* () {
- var libraryID = Zotero.Libraries.userLibraryID;
- ({ engine, client, caller } = yield setup());
- var type = 'item';
- var objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(type);
- var responseJSON = [];
-
- var noteText1 = "<p>A</p>";
- var noteText2 = "<p>B</p>";
-
- // Create object in cache
- var obj = new Zotero.Item('note');
- obj.setNote("");
- obj.version = 10;
- yield obj.saveTx();
- var jsonData = obj.toJSON();
- var key = jsonData.key = obj.key;
- let json = {
- key: obj.key,
- version: jsonData.version,
- data: jsonData
- };
- yield Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [json]);
-
- // Create new version in cache, simulating a download
- json.version = jsonData.version = 15;
- json.data.note = noteText2;
- responseJSON.push(json);
-
- // Modify local version
- obj.setNote(noteText1);
-
- setResponse({
- method: "GET",
- url: `users/1/items?format=json&itemKey=${key}&includeTrashed=1`,
- status: 200,
- headers: {
- "Last-Modified-Version": 15
- },
- json: responseJSON
- });
-
- waitForWindow('chrome://zotero/content/merge.xul', function (dialog) {
- var doc = dialog.document;
- var wizard = doc.documentElement;
- var mergeGroup = wizard.getElementsByTagName('zoteromergegroup')[0];
-
- // Remote version should be selected by default
- assert.equal(mergeGroup.rightpane.getAttribute('selected'), 'true');
- wizard.getButton('finish').click();
- })
- yield engine._downloadObjects('item', [key]);
-
- obj = objectsClass.getByLibraryAndKey(libraryID, key);
- assert.ok(obj);
- assert.equal(obj.getNote(), noteText2);
-
- var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', libraryID);
- assert.lengthOf(keys, 0);
- })
});
diff --git a/test/tests/syncLocalTest.js b/test/tests/syncLocalTest.js
@@ -440,15 +440,13 @@ describe("Zotero.Sync.Data.Local", function() {
var type = 'item';
let obj = yield createDataObject(type, { version: 5 });
let data = obj.toJSON();
- yield Zotero.Sync.Data.Local.saveCacheObjects(
- type, libraryID, [data]
- );
+ yield Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [data]);
// Change local title
yield modifyDataObject(obj)
var changedTitle = obj.getField('title');
- // Save remote version to cache without title but with changed place
+ // Create remote version without title but with changed place
data.key = obj.key;
data.version = 10;
var changedPlace = data.place = 'New York';
@@ -465,14 +463,14 @@ describe("Zotero.Sync.Data.Local", function() {
assert.equal(obj.getField('place'), changedPlace);
})
- it("should save item with overriding local conflict as unsynced", function* () {
+ it("should save item with overriding local conflict as unsynced", async function () {
var libraryID = Zotero.Libraries.userLibraryID;
var isbn = '978-0-335-22006-9';
var type = 'item';
let obj = createUnsavedDataObject(type, { version: 5 });
obj.setField('ISBN', isbn);
- yield obj.saveTx();
+ await obj.saveTx();
let data = obj.toJSON();
data.key = obj.key;
@@ -483,7 +481,7 @@ describe("Zotero.Sync.Data.Local", function() {
version: 10,
data
};
- var results = yield Zotero.Sync.Data.Local.processObjectsFromJSON(
+ var results = await Zotero.Sync.Data.Local.processObjectsFromJSON(
type, libraryID, [json], { stopOnError: true }
);
assert.isTrue(results[0].processed);
@@ -492,6 +490,9 @@ describe("Zotero.Sync.Data.Local", function() {
assert.equal(obj.version, 10);
assert.equal(obj.getField('ISBN'), isbn);
assert.isFalse(obj.synced);
+ // Sync cache should match remote
+ var cacheJSON = await Zotero.Sync.Data.Local.getCacheObject(type, libraryID, data.key, data.version);
+ assert.propertyVal(cacheJSON.data, "ISBN", data.ISBN);
});
it("should restore locally deleted collections and searches that changed remotely", async function () {
