Bug 1328737 - Fix bug where sync wouldn't sync some bookmark keywords on insertion. r?kitcambridge draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Wed, 11 Jan 2017 11:27:58 -0500
changeset 466922 26a6c527e6b456c5a1fa458cd33377452c2c5202
parent 465144 264cbcf1b37f9d211fc432e7672c8a1b03830a12
child 543569 fd7b3818fd0e827abdaf8cd85f2a6f9c78f780de
push id43051
push userbmo:tchiovoloni@mozilla.com
push dateThu, 26 Jan 2017 22:18:44 +0000
reviewerskitcambridge
bugs1328737
milestone53.0a1
Bug 1328737 - Fix bug where sync wouldn't sync some bookmark keywords on insertion. r?kitcambridge MozReview-Commit-ID: FXW3sJ6TU46
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/tests/unit/test_sync_utils.js
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -896,16 +896,53 @@ var insertSyncLivemark = Task.async(func
     return null;
   }
 
   let livemarkItem = yield PlacesUtils.livemarks.addLivemark(livemarkInfo);
 
   return insertBookmarkMetadata(livemarkItem, insertInfo);
 });
 
+// Keywords are a 1 to 1 mapping between strings and pairs of (URL, postData).
+// (the postData is not synced, so we ignore it). Sync associates keywords with
+// bookmarks, which is not really accurate. -- We might already have a keyword
+// with that name, or we might already have another bookmark with that URL with
+// a different keyword, etc.
+//
+// If we don't handle those cases by removing the conflicting keywords first,
+// the insertion  will fail, and the keywords will either be wrong, or missing.
+// This function handles those cases.
+var removeConflictingKeywords = Task.async(function* (bookmarkURL, newKeyword) {
+  let entryForURL = yield PlacesUtils.keywords.fetch({
+    url: bookmarkURL
+  });
+  if (entryForURL && entryForURL.keyword !== newKeyword) {
+    yield PlacesUtils.keywords.remove({
+      keyword: entryForURL.keyword,
+      source: SOURCE_SYNC,
+    });
+    // This will cause us to reupload this record for this sync, but without it,
+    // we will risk data corruption.
+    yield BookmarkSyncUtils.addSyncChangesForBookmarksWithURL(entryForURL.url.href);
+  }
+  if (!newKeyword) {
+    return;
+  }
+  let entryForNewKeyword = yield PlacesUtils.keywords.fetch({
+    keyword: newKeyword
+  });
+  if (entryForNewKeyword) {
+    yield PlacesUtils.keywords.remove({
+      keyword: entryForNewKeyword.keyword,
+      source: SOURCE_SYNC,
+    });
+    yield BookmarkSyncUtils.addSyncChangesForBookmarksWithURL(entryForNewKeyword.url.href);
+  }
+});
+
 // Sets annotations, keywords, and tags on a new bookmark. Returns a Sync
 // bookmark object.
 var insertBookmarkMetadata = Task.async(function* (bookmarkItem, insertInfo) {
   let itemId = yield PlacesUtils.promiseItemId(bookmarkItem.guid);
   let newItem = yield placesBookmarkToSyncBookmark(bookmarkItem);
 
   if (insertInfo.query) {
     PlacesUtils.annotations.setItemAnnotation(itemId,
@@ -918,16 +955,17 @@ var insertBookmarkMetadata = Task.async(
   try {
     newItem.tags = yield tagItem(bookmarkItem, insertInfo.tags);
   } catch (ex) {
     BookmarkSyncLog.warn(`insertBookmarkMetadata: Error tagging item ${
       insertInfo.syncId}`, ex);
   }
 
   if (insertInfo.keyword) {
+    yield removeConflictingKeywords(bookmarkItem.url.href, insertInfo.keyword);
     yield PlacesUtils.keywords.insert({
       keyword: insertInfo.keyword,
       url: bookmarkItem.url.href,
       source: SOURCE_SYNC,
     });
     newItem.keyword = insertInfo.keyword;
   }
 
@@ -1131,25 +1169,17 @@ var updateBookmarkMetadata = Task.async(
     newItem.tags = yield tagItem(newBookmarkItem, updateInfo.tags);
   } catch (ex) {
     BookmarkSyncLog.warn(`updateBookmarkMetadata: Error tagging item ${
       updateInfo.syncId}`, ex);
   }
 
   if (updateInfo.hasOwnProperty("keyword")) {
     // Unconditionally remove the old keyword.
-    let entry = yield PlacesUtils.keywords.fetch({
-      url: oldBookmarkItem.url.href,
-    });
-    if (entry) {
-      yield PlacesUtils.keywords.remove({
-        keyword: entry.keyword,
-        source: SOURCE_SYNC,
-      });
-    }
+    yield removeConflictingKeywords(oldBookmarkItem.url.href, updateInfo.keyword);
     if (updateInfo.keyword) {
       yield PlacesUtils.keywords.insert({
         keyword: updateInfo.keyword,
         url: newItem.url.href,
         source: SOURCE_SYNC,
       });
     }
     newItem.keyword = updateInfo.keyword;
--- a/toolkit/components/places/tests/unit/test_sync_utils.js
+++ b/toolkit/components/places/tests/unit/test_sync_utils.js
@@ -614,16 +614,108 @@ add_task(function* test_update_keyword()
     });
     let entry = yield PlacesUtils.keywords.fetch({
       url: "https://mozilla.org",
     });
     ok(!entry,
       "Removing keyword for URL without existing keyword should succeed");
   }
 
+  let item2;
+  do_print("Insert removes other item's keyword if they are the same");
+  {
+    let updatedItem = yield PlacesSyncUtils.bookmarks.update({
+      syncId: item.syncId,
+      keyword: "test",
+    });
+    equal(updatedItem.keyword, "test", "Initial update succeeds");
+    item2 = yield PlacesSyncUtils.bookmarks.insert({
+      kind: "bookmark",
+      parentSyncId: "menu",
+      url: "https://mozilla.org/1",
+      syncId: makeGuid(),
+      keyword: "test",
+    });
+    equal(item2.keyword, "test", "insert with existing should succeed");
+    updatedItem = yield PlacesSyncUtils.bookmarks.fetch(item.syncId);
+    ok(!updatedItem.keyword, "initial item no longer has keyword");
+    let entry = yield PlacesUtils.keywords.fetch({
+      url: "https://mozilla.org",
+    });
+    ok(!entry, "Direct check for original url keyword gives nothing");
+    let newEntry = yield PlacesUtils.keywords.fetch("test");
+    ok(newEntry, "Keyword should exist for new item");
+    equal(newEntry.url.href, "https://mozilla.org/1", "Keyword should point to new url");
+  }
+
+  do_print("Insert updates other item's keyword if they are the same url");
+  {
+    let updatedItem = yield PlacesSyncUtils.bookmarks.update({
+      syncId: item.syncId,
+      keyword: "test2",
+    });
+    equal(updatedItem.keyword, "test2", "Initial update succeeds");
+    let newItem = yield PlacesSyncUtils.bookmarks.insert({
+      kind: "bookmark",
+      parentSyncId: "menu",
+      url: "https://mozilla.org",
+      syncId: makeGuid(),
+      keyword: "test3",
+    });
+    equal(newItem.keyword, "test3", "insert with existing should succeed");
+    updatedItem = yield PlacesSyncUtils.bookmarks.fetch(item.syncId);
+    equal(updatedItem.keyword, "test3", "initial item has new keyword");
+  }
+
+  do_print("Update removes other item's keyword if they are the same");
+  {
+    let updatedItem = yield PlacesSyncUtils.bookmarks.update({
+      syncId: item.syncId,
+      keyword: "test4",
+    });
+    equal(updatedItem.keyword, "test4", "Initial update succeeds");
+    let updatedItem2 = yield PlacesSyncUtils.bookmarks.update({
+      syncId: item2.syncId,
+      keyword: "test4",
+    });
+    equal(updatedItem2.keyword, "test4", "New update succeeds");
+    updatedItem = yield PlacesSyncUtils.bookmarks.fetch(item.syncId);
+    ok(!updatedItem.keyword, "initial item no longer has keyword");
+    let entry = yield PlacesUtils.keywords.fetch({
+      url: "https://mozilla.org",
+    });
+    ok(!entry, "Direct check for original url keyword gives nothing");
+    let newEntry = yield PlacesUtils.keywords.fetch("test4");
+    ok(newEntry, "Keyword should exist for new item");
+    equal(newEntry.url.href, "https://mozilla.org/1", "Keyword should point to new url");
+  }
+
+  do_print("Update url updates it's keyword if url already has keyword");
+  {
+    let updatedItem = yield PlacesSyncUtils.bookmarks.update({
+      syncId: item.syncId,
+      keyword: "test4",
+    });
+    equal(updatedItem.keyword, "test4", "Initial update succeeds");
+    let updatedItem2 = yield PlacesSyncUtils.bookmarks.update({
+      syncId: item2.syncId,
+      keyword: "test5",
+    });
+    equal(updatedItem2.keyword, "test5", "New update succeeds");
+    yield PlacesSyncUtils.bookmarks.update({
+      syncId: item2.syncId,
+      url: item.url.href,
+    });
+    updatedItem2 = yield PlacesSyncUtils.bookmarks.fetch(item2.syncId);
+    equal(updatedItem2.keyword, "test4", "Item keyword has been updated");
+    updatedItem = yield PlacesSyncUtils.bookmarks.fetch(item.syncId);
+    equal(updatedItem.keyword, "test4", "Initial item still has keyword");
+  }
+
+
   yield PlacesUtils.bookmarks.eraseEverything();
   yield PlacesSyncUtils.bookmarks.reset();
 });
 
 add_task(function* test_update_annos() {
   let guids = yield populateTree(PlacesUtils.bookmarks.menuGuid, {
     kind: "folder",
     title: "folder",