Bug 1332897 - Ensure `removeConflictingKeywords` passes the correct params to `addSyncChangesForBookmarksWithURL`. r?tcsc draft
authorKit Cambridge <kit@yakshaving.ninja>
Thu, 02 Feb 2017 11:24:31 -0800
changeset 469803 95a59045155121f50b1a15a022ee55d0fcfcc3d9
parent 469330 f985243bb630b2c78cd57731c8d8ab191aa09527
child 544302 faa2e6173972f4b03e4bfe89a5577bb0a53769ff
push id43843
push userbmo:kit@mozilla.com
push dateThu, 02 Feb 2017 19:25:34 +0000
reviewerstcsc
bugs1332897
milestone54.0a1
Bug 1332897 - Ensure `removeConflictingKeywords` passes the correct params to `addSyncChangesForBookmarksWithURL`. r?tcsc MozReview-Commit-ID: HNXOXdJBI6M
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
@@ -905,43 +905,49 @@ var insertSyncLivemark = Task.async(func
 // (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);
-  }
-});
+function removeConflictingKeywords(bookmarkURL, newKeyword) {
+  return PlacesUtils.withConnectionWrapper(
+    "BookmarkSyncUtils: removeConflictingKeywords", Task.async(function* (db) {
+      let entryForURL = yield PlacesUtils.keywords.fetch({
+        url: bookmarkURL.href,
+      });
+      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(
+          db, entryForURL.url, 1);
+      }
+      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(
+          db, entryForNewKeyword.url, 1);
+      }
+    })
+  );
+}
 
 // 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) {
@@ -955,17 +961,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 removeConflictingKeywords(bookmarkItem.url, insertInfo.keyword);
     yield PlacesUtils.keywords.insert({
       keyword: insertInfo.keyword,
       url: bookmarkItem.url.href,
       source: SOURCE_SYNC,
     });
     newItem.keyword = insertInfo.keyword;
   }
 
@@ -1169,17 +1175,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.
-    yield removeConflictingKeywords(oldBookmarkItem.url.href, updateInfo.keyword);
+    yield removeConflictingKeywords(oldBookmarkItem.url, 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
@@ -710,16 +710,98 @@ add_task(function* test_update_keyword()
     equal(updatedItem.keyword, "test4", "Initial item still has keyword");
   }
 
 
   yield PlacesUtils.bookmarks.eraseEverything();
   yield PlacesSyncUtils.bookmarks.reset();
 });
 
+add_task(function* test_conflicting_keywords() {
+  yield ignoreChangedRoots();
+
+  do_print("Insert bookmark with new keyword");
+  let tbBmk = yield PlacesSyncUtils.bookmarks.insert({
+    kind: "bookmark",
+    syncId: makeGuid(),
+    parentSyncId: "unfiled",
+    url: "http://getthunderbird.com",
+    keyword: "tbird",
+  });
+  {
+    let entryByKeyword = yield PlacesUtils.keywords.fetch("tbird");
+    equal(entryByKeyword.url.href, "http://getthunderbird.com/",
+      "Should return new keyword entry by URL");
+    let entryByURL = yield PlacesUtils.keywords.fetch({
+      url: "http://getthunderbird.com",
+    });
+    equal(entryByURL.keyword, "tbird", "Should return new entry by keyword");
+    let changes = yield PlacesSyncUtils.bookmarks.pullChanges();
+    deepEqual(changes, {},
+      "Should not bump change counter for new keyword entry");
+  }
+
+  do_print("Insert bookmark with same URL and different keyword");
+  let dupeTbBmk = yield PlacesSyncUtils.bookmarks.insert({
+    kind: "bookmark",
+    syncId: makeGuid(),
+    parentSyncId: "toolbar",
+    url: "http://getthunderbird.com",
+    keyword: "tb",
+  });
+  {
+    let oldKeywordByURL = yield PlacesUtils.keywords.fetch("tbird");
+    ok(!oldKeywordByURL,
+      "Should remove old entry when inserting bookmark with different keyword");
+    let entryByKeyword = yield PlacesUtils.keywords.fetch("tb");
+    equal(entryByKeyword.url.href, "http://getthunderbird.com/",
+      "Should return different keyword entry by URL");
+    let entryByURL = yield PlacesUtils.keywords.fetch({
+      url: "http://getthunderbird.com",
+    });
+    equal(entryByURL.keyword, "tb", "Should return different entry by keyword");
+    let changes = yield PlacesSyncUtils.bookmarks.pullChanges();
+    deepEqual(Object.keys(changes).sort(), [
+      tbBmk.syncId,
+      dupeTbBmk.syncId,
+    ].sort(), "Should bump change counter for bookmarks with different keyword");
+    yield setChangesSynced(changes);
+  }
+
+  do_print("Update bookmark with different keyword");
+  yield PlacesSyncUtils.bookmarks.update({
+    kind: "bookmark",
+    syncId: tbBmk.syncId,
+    url: "http://getthunderbird.com",
+    keyword: "thunderbird",
+  });
+  {
+    let oldKeywordByURL = yield PlacesUtils.keywords.fetch("tb");
+    ok(!oldKeywordByURL,
+      "Should remove old entry when updating bookmark keyword");
+    let entryByKeyword = yield PlacesUtils.keywords.fetch("thunderbird");
+    equal(entryByKeyword.url.href, "http://getthunderbird.com/",
+      "Should return updated keyword entry by URL");
+    let entryByURL = yield PlacesUtils.keywords.fetch({
+      url: "http://getthunderbird.com",
+    });
+    equal(entryByURL.keyword, "thunderbird",
+      "Should return entry by updated keyword");
+    let changes = yield PlacesSyncUtils.bookmarks.pullChanges();
+    deepEqual(Object.keys(changes).sort(), [
+      tbBmk.syncId,
+      dupeTbBmk.syncId,
+    ].sort(), "Should bump change counter for bookmarks with updated keyword");
+    yield setChangesSynced(changes);
+  }
+
+  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",
   }, {
     kind: "bookmark",
     title: "bmk",
     url: "https://example.com",