Bug 1258127 - Move bookmark de-duping logic into `PlacesSyncUtils.bookmarks.dedupe`. r=markh

This patch moves the logic from `BookmarksEngine::_switchItemToDupe`
into `PlacesSyncUtils.bookmarks.dedupe`, and updates it to work with
the new tracker. `dedupe` returns an object containing new change
records, which the bookmarks engine merges into the initial changeset
from `PlacesSyncUtils.bookmarks.pullChanges`.

This patch also removes `changeItemID` and
`PlacesSyncUtils.bookmarks.changeGuid`, since `dedupe` subsumes them.

MozReview-Commit-ID: Iw3YRxWuZnK

--HG--
extra : rebase_source : 6fd80c64b160103e1090b87a300ed74b8ef85eed

services/sync/modules/engines.js

@@ -1791,6 +1791,11 @@ class Changeset {
     this.changes[id] = change;
   }
 
+  // Adds multiple entries to the changeset.
+  insert(changes) {
+    Object.assign(this.changes, changes);
+  }
+
   // Indicates whether an entry is in the changeset.
   has(id) {
     return id in this.changes;

services/sync/modules/engines/bookmarks.js

@@ -620,52 +620,9 @@ BookmarksEngine.prototype = {
   // Called when _findDupe returns a dupe item and the engine has decided to
   // switch the existing item to the new incoming item.
   _switchItemToDupe(localDupeGUID, incomingItem) {
-    // We unconditionally change the item's ID in case the engine knows of
+    let newChanges = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.dedupe(
-    // an item but doesn't expose it through itemExists. If the API
+      localDupeGUID, incomingItem.id, incomingItem.parentid));
-    // contract were stronger, this could be changed.
+    this._modified.insert(newChanges);
-    this._log.debug("Switching local ID to incoming: " + localDupeGUID + " -> " +
-                    incomingItem.id);
-    this._store.changeItemID(localDupeGUID, incomingItem.id);
-
-    // And mark the parent as being modified. Given we de-dupe based on the
-    // parent *name* it's possible the item having its GUID changed has a
-    // different parent from the incoming record.
-    // So we need to find the GUID of the local parent.
-    let now = this._tracker._now();
-    let localID = this._store.idForGUID(incomingItem.id);
-    let localParentID = PlacesUtils.bookmarks.getFolderIdForItem(localID);
-    let localParentGUID = this._store.GUIDForId(localParentID);
-    this._modified.set(localParentGUID, { modified: now, deleted: false });
-
-    // And we also add the parent as reflected in the incoming record as the
-    // de-dupe process might have used an existing item in a different folder.
-    // But only if the parent exists, otherwise we will upload a deleted item
-    // when it might actually be valid, just unknown to us. Note that this
-    // scenario will still leave us with inconsistent client and server states;
-    // the incoming record on the server references a parent that isn't the
-    // actual parent locally - see bug 1297955.
-    if (localParentGUID != incomingItem.parentid) {
-      let remoteParentID = this._store.idForGUID(incomingItem.parentid);
-      if (remoteParentID > 0) {
-        // The parent specified in the record does exist, so we are going to
-        // attempt a move when we come to applying the record. Mark the parent
-        // as being modified so we will later upload it with the new child
-        // reference.
-        this._modified.set(incomingItem.parentid, { modified: now, deleted: false });
-      } else {
-        // We aren't going to do a move as we don't have the parent (yet?).
-        // When applying the record we will add our special PARENT_ANNO
-        // annotation, so if it arrives in the future (either this Sync or a
-        // later one) it will be reparented.
-        this._log.debug(`Incoming duplicate item ${incomingItem.id} specifies ` +
-                        `non-existing parent ${incomingItem.parentid}`);
-      }
-    }
-
-    // The local, duplicate ID is always deleted on the server - but for
-    // bookmarks it is a logical delete.
-    // Simply adding this (now non-existing) ID to the tracker is enough.
-    this._modified.set(localDupeGUID, { modified: now, deleted: true });
   },
 
   // Cleans up the Places root, reading list items (ignored in bug 762118,
@@ -918,12 +875,6 @@ BookmarksStore.prototype = {
     return [...needUpdate];
   }),
 
-  changeItemID: function BStore_changeItemID(oldID, newID) {
-    this._log.debug("Changing GUID " + oldID + " to " + newID);
-
-    Async.promiseSpinningly(PlacesSyncUtils.bookmarks.changeGuid(oldID, newID));
-  },
-
   // Create a record starting from the weave id (places guid)
   createRecord: function createRecord(id, collection) {
     let item = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.fetch(id));

services/sync/tests/unit/test_bookmark_duping.js

@@ -293,7 +293,11 @@ add_task(function* test_dupe_reparented_locally_changed_bookmark() {
     // time further in the future than the incoming record. This will cause
     // us to issue the infamous "DATA LOSS" warning in the logs but cause us
     // to *not* apply the incoming record.
-    engine._tracker.addChangedID(bmk1_guid, Date.now() / 1000 + 60);
+    yield PlacesTestUtils.setBookmarkSyncFields({
+      guid: bmk1_guid,
+      syncChangeCounter: 1,
+      lastModified: Date.now() + 60000,
+    });
 
     _("Syncing so new dupe record is processed");
     engine.lastSync = engine.lastSync - 0.01;

toolkit/components/places/PlacesSyncUtils.jsm

@@ -290,28 +290,35 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
   }),
 
   /**
-   * Changes the GUID of an existing item. This method only allows Places GUIDs
+   * De-dupes an item by changing its sync ID to match the ID on the server.
-   * because root sync IDs cannot be changed.
+   * Sync calls this method when it detects an incoming item is a duplicate of
+   * an existing local item.
    *
-   * @return {Promise} resolved once the GUID has been changed.
+   * Note that this method doesn't move the item if the local and remote sync
-   * @resolves to the new GUID.
+   * IDs are different. That happens after de-duping, when the bookmarks engine
-   * @rejects if the old GUID does not exist.
+   * calls `update` to update the item.
+   *
+   * @param localSyncId
+   *        The local ID to change.
+   * @param remoteSyncId
+   *        The remote ID that should replace the local ID.
+   * @param remoteParentSyncId
+   *        The remote record's parent ID.
+   * @return {Promise} resolved once the ID has been changed.
+   * @resolves to an object containing new change records for the old item,
+   *           the local parent, and the remote parent if different from the
+   *           local parent. The bookmarks engine merges these records into the
+   *           changeset for the current sync.
    */
-  changeGuid: Task.async(function* (oldGuid, newGuid) {
+  dedupe: Task.async(function* (localSyncId, remoteSyncId, remoteParentSyncId) {
-    PlacesUtils.BOOKMARK_VALIDATORS.guid(oldGuid);
+    PlacesUtils.SYNC_BOOKMARK_VALIDATORS.syncId(localSyncId);
-    PlacesUtils.BOOKMARK_VALIDATORS.guid(newGuid);
+    PlacesUtils.SYNC_BOOKMARK_VALIDATORS.syncId(remoteSyncId);
+    PlacesUtils.SYNC_BOOKMARK_VALIDATORS.syncId(remoteParentSyncId);
 
-    let itemId = yield PlacesUtils.promiseItemId(oldGuid);
+    return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: dedupe", db =>
-    if (PlacesUtils.isRootItem(itemId)) {
+      dedupeSyncBookmark(db, BookmarkSyncUtils.syncIdToGuid(localSyncId),
-      throw new Error(`Cannot change GUID of Places root ${oldGuid}`);
+                         BookmarkSyncUtils.syncIdToGuid(remoteSyncId),
-    }
+                         BookmarkSyncUtils.syncIdToGuid(remoteParentSyncId))
-    return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: changeGuid",
-      Task.async(function* (db) {
-        yield db.executeCached(`UPDATE moz_bookmarks SET guid = :newGuid
-          WHERE id = :itemId`, { newGuid, itemId });
-        PlacesUtils.invalidateCachedGuidFor(itemId);
-        return newGuid;
-      })
     );
   }),
 
@@ -1385,6 +1392,93 @@ var pullSyncChanges = Task.async(function* (db) {
   return changeRecords;
 });
 
+var dedupeSyncBookmark = Task.async(function* (db, localGuid, remoteGuid,
+                                               remoteParentGuid) {
+  let rows = yield db.executeCached(`
+    SELECT b.id, p.guid AS parentGuid, b.syncStatus
+    FROM moz_bookmarks b
+    JOIN moz_bookmarks p ON p.id = b.parent
+    WHERE b.guid = :localGuid`,
+    { localGuid });
+  if (!rows.length) {
+    throw new Error(`Local item ${localGuid} does not exist`);
+  }
+
+  let localId = rows[0].getResultByName("id");
+  if (PlacesUtils.isRootItem(localId)) {
+    throw new Error(`Cannot de-dupe local root ${localGuid}`);
+  }
+
+  let localParentGuid = rows[0].getResultByName("parentGuid");
+  let sameParent = localParentGuid == remoteParentGuid;
+  let modified = PlacesUtils.toPRTime(Date.now());
+
+  yield db.executeTransaction(function* () {
+    // Change the item's old GUID to the new remote GUID. This will throw a
+    // constraint error if the remote GUID already exists locally.
+    BookmarkSyncLog.debug("dedupeSyncBookmark: Switching local GUID " +
+                          localGuid + " to incoming GUID " + remoteGuid);
+    yield db.executeCached(`UPDATE moz_bookmarks
+      SET guid = :remoteGuid
+      WHERE id = :localId`,
+      { remoteGuid, localId });
+    PlacesUtils.invalidateCachedGuidFor(localId);
+
+    // And mark the parent as being modified. Given we de-dupe based on the
+    // parent *name* it's possible the item having its GUID changed has a
+    // different parent from the incoming record.
+    // So we need to return a change record for the parent, and bump its
+    // counter to ensure we don't lose the change if the current sync is
+    // interrupted.
+    yield db.executeCached(`UPDATE moz_bookmarks
+      SET syncChangeCounter = syncChangeCounter + 1
+      WHERE guid = :localParentGuid`,
+      { localParentGuid });
+
+    // And we also add the parent as reflected in the incoming record as the
+    // de-dupe process might have used an existing item in a different folder.
+    // This statement is a no-op if we don't have the new parent yet, but that's
+    // fine: applying the record will add our special SYNC_PARENT_ANNO
+    // annotation and move it to unfiled. If the parent arrives in the future
+    // (either this Sync or a later one), the item will be reparented. Note that
+    // this scenario will still leave us with inconsistent client and server
+    // states; the incoming record on the server references a parent that isn't
+    // the actual parent locally - see bug 1297955.
+    if (!sameParent) {
+      yield db.executeCached(`UPDATE moz_bookmarks
+        SET syncChangeCounter = syncChangeCounter + 1
+        WHERE guid = :remoteParentGuid`,
+        { remoteParentGuid });
+    }
+
+    // The local, duplicate ID is always deleted on the server - but for
+    // bookmarks it is a logical delete.
+    let localSyncStatus = rows[0].getResultByName("syncStatus");
+    if (localSyncStatus == PlacesUtils.bookmarks.SYNC_STATUS.NORMAL) {
+      yield db.executeCached(`
+        INSERT INTO moz_bookmarks_deleted (guid, dateRemoved)
+        VALUES (:localGuid, :modified)`,
+        { localGuid, modified });
+    }
+  });
+
+  // TODO (Bug 1313890): Refactor the bookmarks engine to pull change records
+  // before uploading, instead of returning records to merge into the engine's
+  // initial changeset.
+  let changeRecords = yield pullSyncChanges(db);
+
+  if (BookmarkSyncLog.level <= Log.Level.Debug && !sameParent) {
+    let remoteParentSyncId = BookmarkSyncUtils.guidToSyncId(remoteParentGuid);
+    if (!changeRecords.hasOwnProperty(remoteParentSyncId)) {
+      BookmarkSyncLog.debug("dedupeSyncBookmark: Incoming duplicate item " +
+                            remoteGuid + " specifies non-existing parent " +
+                            remoteParentGuid);
+    }
+  }
+
+  return changeRecords;
+});
+
 /**
  * Updates the sync status on all "NEW" and "UNKNOWN" bookmarks to "NORMAL".
  *

toolkit/components/places/tests/unit/test_sync_utils.js

@@ -225,34 +225,126 @@ add_task(function* test_order() {
   yield PlacesSyncUtils.bookmarks.reset();
 });
 
-add_task(function* test_changeGuid_invalid() {
+add_task(function* test_dedupe() {
-  yield rejects(PlacesSyncUtils.bookmarks.changeGuid(makeGuid()),
+  yield ignoreChangedRoots();
-    "Should require a new GUID");
-  yield rejects(PlacesSyncUtils.bookmarks.changeGuid(makeGuid(), "!@#$"),
-    "Should reject invalid GUIDs");
-  yield rejects(PlacesSyncUtils.bookmarks.changeGuid(makeGuid(), makeGuid()),
-    "Should reject nonexistent item GUIDs");
-  yield rejects(
-    PlacesSyncUtils.bookmarks.changeGuid(PlacesUtils.bookmarks.menuGuid,
-      makeGuid()),
-    "Should reject roots");
-});
 
-add_task(function* test_changeGuid() {
+  let parentFolder = yield PlacesSyncUtils.bookmarks.insert({
-  let item = yield PlacesUtils.bookmarks.insert({
+    kind: "folder",
-    parentGuid: PlacesUtils.bookmarks.menuGuid,
+    syncId: makeGuid(),
-    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+    parentSyncId: "menu",
+  });
+  let differentParentFolder = yield PlacesSyncUtils.bookmarks.insert({
+    kind: "folder",
+    syncId: makeGuid(),
+    parentSyncId: "menu",
+  });
+  let mozBmk = yield PlacesSyncUtils.bookmarks.insert({
+    kind: "bookmark",
+    syncId: makeGuid(),
+    parentSyncId: parentFolder.syncId,
     url: "https://mozilla.org",
   });
-  let id = yield PlacesUtils.promiseItemId(item.guid);
+  let fxBmk = yield PlacesSyncUtils.bookmarks.insert({
+    kind: "bookmark",
+    syncId: makeGuid(),
+    parentSyncId: parentFolder.syncId,
+    url: "http://getfirefox.com",
+  });
+  let tbBmk = yield PlacesSyncUtils.bookmarks.insert({
+    kind: "bookmark",
+    syncId: makeGuid(),
+    parentSyncId: parentFolder.syncId,
+    url: "http://getthunderbird.com",
+  });
 
-  let newGuid = makeGuid();
+  yield rejects(
-  let result = yield PlacesSyncUtils.bookmarks.changeGuid(item.guid, newGuid);
+    PlacesSyncUtils.bookmarks.dedupe(makeGuid(), makeGuid(), makeGuid()),
-  equal(result, newGuid, "Should return new GUID");
+    "Should reject attempts to de-dupe nonexistent items"
+  );
+  yield rejects(PlacesSyncUtils.bookmarks.dedupe("menu", makeGuid(), "places"),
+    "Should reject attempts to de-dupe local roots");
 
-  equal(yield PlacesUtils.promiseItemId(newGuid), id, "Should map ID to new GUID");
+  do_print("De-dupe with same remote parent");
-  yield rejects(PlacesUtils.promiseItemId(item.guid), "Should not map ID to old GUID");
+  {
-  equal(yield PlacesUtils.promiseItemGuid(id), newGuid, "Should map new GUID to ID");
+    let localId = yield PlacesUtils.promiseItemId(mozBmk.syncId);
+    let newRemoteSyncId = makeGuid();
+
+    let changes = yield PlacesSyncUtils.bookmarks.dedupe(
+      mozBmk.syncId, newRemoteSyncId, parentFolder.syncId);
+    deepEqual(Object.keys(changes).sort(), [
+      parentFolder.syncId, // Parent.
+      mozBmk.syncId, // Tombstone for old sync ID.
+    ].sort(), "Should bump change counter of parent");
+    ok(changes[mozBmk.syncId].tombstone,
+      "Should write tombstone for old local sync ID");
+    ok(Object.values(changes).every(change => change.counter === 1),
+      "Change counter for every bookmark should be 1");
+
+    ok(!(yield PlacesUtils.bookmarks.fetch(mozBmk.syncId)),
+      "Bookmark with old local sync ID should not exist");
+    yield rejects(PlacesUtils.promiseItemId(mozBmk.syncId),
+      "Should invalidate GUID cache entry for old local sync ID");
+
+    let newMozBmk = yield PlacesUtils.bookmarks.fetch(newRemoteSyncId);
+    equal(newMozBmk.guid, newRemoteSyncId,
+      "Should change local sync ID to remote sync ID");
+    equal(yield PlacesUtils.promiseItemId(newRemoteSyncId), localId,
+      "Should add new remote sync ID to GUID cache");
+
+    yield setChangesSynced(changes);
+  }
+
+  do_print("De-dupe with different remote parent");
+  {
+    let localId = yield PlacesUtils.promiseItemId(fxBmk.syncId);
+    let newRemoteSyncId = makeGuid();
+
+    let changes = yield PlacesSyncUtils.bookmarks.dedupe(
+      fxBmk.syncId, newRemoteSyncId, differentParentFolder.syncId);
+    deepEqual(Object.keys(changes).sort(), [
+      parentFolder.syncId, // Old local parent.
+      differentParentFolder.syncId, // New remote parent.
+      fxBmk.syncId, // Tombstone for old sync ID.
+    ].sort(), "Should bump change counter of old parent and new parent");
+    ok(changes[fxBmk.syncId].tombstone,
+      "Should write tombstone for old local sync ID");
+    ok(Object.values(changes).every(change => change.counter === 1),
+      "Change counter for every bookmark should be 1");
+
+    let newFxBmk = yield PlacesUtils.bookmarks.fetch(newRemoteSyncId);
+    equal(newFxBmk.parentGuid, parentFolder.syncId,
+      "De-duping should not move bookmark to new parent");
+    equal(yield PlacesUtils.promiseItemId(newRemoteSyncId), localId,
+      "De-duping with different remote parent should cache new sync ID");
+
+    yield setChangesSynced(changes);
+  }
+
+  do_print("De-dupe with nonexistent remote parent");
+  {
+    let localId = yield PlacesUtils.promiseItemId(tbBmk.syncId);
+    let newRemoteSyncId = makeGuid();
+    let remoteParentSyncId = makeGuid();
+
+    let changes = yield PlacesSyncUtils.bookmarks.dedupe(
+      tbBmk.syncId, newRemoteSyncId, remoteParentSyncId);
+    deepEqual(Object.keys(changes).sort(), [
+      parentFolder.syncId, // Old local parent.
+      tbBmk.syncId, // Tombstone for old sync ID.
+    ].sort(), "Should bump change counter of old parent");
+    ok(changes[tbBmk.syncId].tombstone,
+      "Should write tombstone for old local sync ID");
+    ok(Object.values(changes).every(change => change.counter === 1),
+      "Change counter for every bookmark should be 1");
+
+    equal(yield PlacesUtils.promiseItemId(newRemoteSyncId), localId,
+      "De-duping with nonexistent remote parent should cache new sync ID");
+
+    yield setChangesSynced(changes);
+  }
+
+  yield PlacesUtils.bookmarks.eraseEverything();
+  yield PlacesSyncUtils.bookmarks.reset();
 });
 
 add_task(function* test_order_roots() {