Bug 1294291 - Remove missing GUID handling code from Sync and Places. r=markh draft
authorKit Cambridge <kcambridge@mozilla.com>
Tue, 06 Sep 2016 11:29:32 -0700
changeset 410382 ab07f772354ff613a3e768db119e9b60dc49daa0
parent 409497 d0830980ffdb36a10855d02a588b4869cad6707e
child 530576 e019bab2f95587937e3e73ac53bc0d0235ae48c3
push id28732
push userbmo:kcambridge@mozilla.com
push dateTue, 06 Sep 2016 18:32:58 +0000
reviewersmarkh
bugs1294291
milestone51.0a1
Bug 1294291 - Remove missing GUID handling code from Sync and Places. r=markh MozReview-Commit-ID: CbhF4s0nNr0
services/sync/modules/engines/bookmarks.js
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/tests/unit/test_sync_utils.js
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -777,17 +777,17 @@ BookmarksStore.prototype = {
   _frecencyCols: ["frecency"],
 
   GUIDForId: function GUIDForId(id) {
     let special = BookmarkSpecialIds.specialGUIDForId(id);
     if (special)
       return special;
 
     return Async.promiseSpinningly(
-      PlacesSyncUtils.bookmarks.ensureGuidForId(id));
+      PlacesUtils.promiseItemGuid(id));
   },
 
   // noCreate is provided as an optional argument to prevent the creation of
   // non-existent special records, such as "mobile".
   idForGUID: function idForGUID(guid, noCreate) {
     // guid might be a String object rather than a string.
     guid = guid.toString();
 
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -46,42 +46,23 @@ const BookmarkSyncUtils = PlacesSyncUtil
     QUERY: "query",
     FOLDER: "folder",
     LIVEMARK: "livemark",
     SEPARATOR: "separator",
   },
 
   /**
    * Fetches a folder's children, ordered by their position within the folder.
-   * Children without a GUID will be assigned one.
    */
   fetchChildGuids: Task.async(function* (parentGuid) {
     PlacesUtils.SYNC_BOOKMARK_VALIDATORS.guid(parentGuid);
 
     let db = yield PlacesUtils.promiseDBConnection();
     let children = yield fetchAllChildren(db, parentGuid);
-    let childGuids = [];
-    let guidsToSet = new Map();
-    for (let child of children) {
-      let guid = child.guid;
-      if (!PlacesUtils.isValidGuid(guid)) {
-        // Give the child a GUID if it doesn't have one. This shouldn't happen,
-        // but the old bookmarks engine code does this, so we'll match its
-        // behavior until we're sure this can be removed.
-        guid = yield generateGuid(db);
-        BookmarkSyncLog.warn(`fetchChildGuids: Assigning ${
-          guid} to item without GUID ${child.id}`);
-        guidsToSet.set(child.id, guid);
-      }
-      childGuids.push(guid);
-    }
-    if (guidsToSet.size > 0) {
-      yield setGuids(guidsToSet);
-    }
-    return childGuids;
+    return children.map(child => child.guid);
   }),
 
   /**
    * Reorders a folder's children, based on their order in the array of GUIDs.
    * This method is similar to `Bookmarks.reorder`, but leaves missing entries
    * in place instead of moving them to the end of the folder.
    *
    * Sync uses this method to reorder all synced children after applying all
@@ -148,70 +129,38 @@ const BookmarkSyncUtils = PlacesSyncUtil
    * mobile root.
    */
   clear: Task.async(function* (folderGuid) {
     let folderId = yield PlacesUtils.promiseItemId(folderGuid);
     PlacesUtils.bookmarks.removeFolderChildren(folderId, SOURCE_SYNC);
   }),
 
   /**
-   * Ensures an item with the |itemId| has a GUID, assigning one if necessary.
-   * We should never have a bookmark without a GUID, but the old Sync bookmarks
-   * engine code does this, so we'll match its behavior until we're sure it's
-   * not needed.
-   *
-   * This method can be removed and replaced with `PlacesUtils.promiseItemGuid`
-   * once bug 1294291 lands.
-   *
-   * @return {Promise} resolved once the GUID has been updated.
-   * @resolves to the existing or new GUID.
-   * @rejects if the item does not exist.
-   */
-  ensureGuidForId: Task.async(function* (itemId) {
-    let guid;
-    try {
-      // Use the existing GUID if it exists. `promiseItemGuid` caches the GUID
-      // as a side effect, and throws if it's invalid.
-      guid = yield PlacesUtils.promiseItemGuid(itemId);
-    } catch (ex) {
-      BookmarkSyncLog.warn(`ensureGuidForId: Error fetching GUID for ${
-        itemId}`, ex);
-      if (!isInvalidCachedGuidError(ex)) {
-        throw ex;
-      }
-      // Give the item a GUID if it doesn't have one.
-      guid = yield PlacesUtils.withConnectionWrapper(
-        "BookmarkSyncUtils: ensureGuidForId", Task.async(function* (db) {
-          let guid = yield generateGuid(db);
-          BookmarkSyncLog.warn(`ensureGuidForId: Assigning ${
-            guid} to item without GUID ${itemId}`);
-          return setGuid(db, itemId, guid);
-        })
-      );
-
-    }
-    return guid;
-  }),
-
-  /**
    * Changes the GUID of an existing item.
    *
    * @return {Promise} resolved once the GUID has been changed.
    * @resolves to the new GUID.
    * @rejects if the old GUID does not exist.
    */
   changeGuid: Task.async(function* (oldGuid, newGuid) {
     PlacesUtils.SYNC_BOOKMARK_VALIDATORS.guid(oldGuid);
+    PlacesUtils.SYNC_BOOKMARK_VALIDATORS.guid(newGuid);
 
     let itemId = yield PlacesUtils.promiseItemId(oldGuid);
     if (PlacesUtils.isRootItem(itemId)) {
       throw new Error(`Cannot change GUID of Places root ${oldGuid}`);
     }
     return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: changeGuid",
-      db => setGuid(db, itemId, newGuid));
+      Task.async(function* (db) {
+        yield db.executeCached(`UPDATE moz_bookmarks SET guid = :newGuid
+          WHERE id = :itemId`, { newGuid, itemId });
+        PlacesUtils.invalidateCachedGuidFor(itemId);
+        return newGuid;
+      })
+    );
   }),
 
   /**
    * Updates a bookmark with synced properties. Only Sync should call this
    * method; other callers should use `Bookmarks.update`.
    *
    * The following properties are supported:
    *  - kind: Optional.
@@ -345,21 +294,16 @@ function updateChildIndex(children, chil
 function notify(observers, notification, args) {
   for (let observer of observers) {
     try {
       observer[notification](...args);
     } catch (ex) {}
   }
 }
 
-function isInvalidCachedGuidError(error) {
-  return error && error.message ==
-    "Trying to update the GUIDs cache with an invalid GUID";
-}
-
 // A helper for whenever we want to know if a GUID doesn't exist in the places
 // database. Primarily used to detect orphans on incoming records.
 var GUIDMissing = Task.async(function* (guid) {
   try {
     yield PlacesUtils.promiseItemId(guid);
     return false;
   } catch (ex) {
     if (ex.message == "no item found for the given GUID") {
@@ -780,33 +724,16 @@ var updateBookmarkMetadata = Task.async(
       PlacesUtils.annotations.EXPIRE_NEVER,
       SOURCE_SYNC);
     newItem.query = updateInfo.query;
   }
 
   return newItem;
 });
 
-function generateGuid(db) {
-  return db.executeCached("SELECT GENERATE_GUID() AS guid").then(rows =>
-    rows[0].getResultByName("guid"));
-}
-
-function setGuids(guids) {
-  return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: setGuids",
-    db => db.executeTransaction(function* () {
-      let promises = [];
-      for (let [itemId, newGuid] of guids) {
-        promises.push(setGuid(db, itemId, newGuid));
-      }
-      return Promise.all(promises);
-    })
-  );
-}
-
 var setGuid = Task.async(function* (db, itemId, newGuid) {
   yield db.executeCached(`UPDATE moz_bookmarks SET guid = :newGuid
     WHERE id = :itemId`, { newGuid, itemId });
   PlacesUtils.invalidateCachedGuidFor(itemId);
   return newGuid;
 });
 
 function validateNewBookmark(info) {
--- a/toolkit/components/places/tests/unit/test_sync_utils.js
+++ b/toolkit/components/places/tests/unit/test_sync_utils.js
@@ -108,33 +108,16 @@ var populateTree = Task.async(function* 
     }
 
     guids[item.title] = guid;
   }
 
   return guids;
 });
 
-function* insertWithoutGuid(info) {
-  let item = yield PlacesUtils.bookmarks.insert(info);
-  let id = yield PlacesUtils.promiseItemId(item.guid);
-
-  // All Places methods ensure we specify a valid GUID, so we insert
-  // an item and remove its GUID by modifying the DB directly.
-  yield PlacesUtils.withConnectionWrapper(
-    "test_sync_utils: insertWithoutGuid", db => db.executeCached(
-      `UPDATE moz_bookmarks SET guid = NULL WHERE guid = :guid`,
-      { guid: item.guid }
-    )
-  );
-  PlacesUtils.invalidateCachedGuidFor(id);
-
-  return { id, item };
-}
-
 add_task(function* test_order() {
   do_print("Insert some bookmarks");
   let guids = yield populateTree(PlacesUtils.bookmarks.menuGuid, {
     kind: "bookmark",
     title: "childBmk",
     url: "http://getfirefox.com",
   }, {
     kind: "bookmark",
@@ -178,94 +161,16 @@ add_task(function* test_order() {
       PlacesUtils.bookmarks.menuGuid);
     deepEqual(childGuids, [guids.childBmk, guids.siblingBmk, guids.siblingSep,
       guids.siblingFolder], "Nonexistent children should be ignored");
   }
 
   yield PlacesUtils.bookmarks.eraseEverything();
 });
 
-add_task(function* test_fetchChildGuids_ensure_guids() {
-  let firstWithGuid = yield PlacesUtils.bookmarks.insert({
-    parentGuid: PlacesUtils.bookmarks.menuGuid,
-    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
-    url: "https://mozilla.org",
-  });
-
-  let { item: secondWithoutGuid } = yield* insertWithoutGuid({
-    parentGuid: PlacesUtils.bookmarks.menuGuid,
-    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
-    url: "https://example.com",
-  });
-
-  let thirdWithGuid = yield PlacesUtils.bookmarks.insert({
-    parentGuid: PlacesUtils.bookmarks.menuGuid,
-    type: PlacesUtils.bookmarks.TYPE_FOLDER,
-  });
-
-  do_print("Children without a GUID should be assigned one");
-  let childGuids = yield PlacesSyncUtils.bookmarks.fetchChildGuids(
-    PlacesUtils.bookmarks.menuGuid);
-  equal(childGuids.length, 3, "Should include all children");
-  equal(childGuids[0], firstWithGuid.guid,
-    "Should include first child GUID");
-  notEqual(childGuids[1], secondWithoutGuid.guid,
-    "Should assign new GUID to second child");
-  equal(childGuids[2], thirdWithGuid.guid,
-    "Should include third child GUID");
-
-  yield PlacesUtils.bookmarks.eraseEverything();
-});
-
-add_task(function* test_ensureGuidForId_invalid() {
-  yield rejects(PlacesSyncUtils.bookmarks.ensureGuidForId(-1),
-    "Should reject invalid item IDs");
-
-  let item = yield PlacesUtils.bookmarks.insert({
-    parentGuid: PlacesUtils.bookmarks.menuGuid,
-    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
-    url: "https://mozilla.org",
-  });
-  let id = yield PlacesUtils.promiseItemId(item.guid);
-  yield PlacesUtils.bookmarks.remove(item);
-  yield rejects(PlacesSyncUtils.bookmarks.ensureGuidForId(id),
-    "Should reject nonexistent item IDs");
-});
-
-add_task(function* test_ensureGuidForId() {
-  do_print("Item with GUID");
-  {
-    let item = yield PlacesUtils.bookmarks.insert({
-      parentGuid: PlacesUtils.bookmarks.menuGuid,
-      type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
-      url: "https://mozilla.org",
-    });
-    let id = yield PlacesUtils.promiseItemId(item.guid);
-    let guid = yield PlacesSyncUtils.bookmarks.ensureGuidForId(id);
-    equal(guid, item.guid, "Should return GUID if one exists");
-  }
-
-  do_print("Item without GUID");
-  {
-    let { id, item } = yield* insertWithoutGuid({
-      parentGuid: PlacesUtils.bookmarks.menuGuid,
-      type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
-      url: "https://example.com",
-    });
-    let guid = yield PlacesSyncUtils.bookmarks.ensureGuidForId(id);
-    notEqual(guid, item.guid, "Should assign new GUID to item without one");
-    equal(yield PlacesUtils.promiseItemGuid(id), guid,
-      "Should map ID to new GUID");
-    equal(yield PlacesUtils.promiseItemId(guid), id,
-      "Should map new GUID to ID");
-  }
-
-  yield PlacesUtils.bookmarks.eraseEverything();
-});
-
 add_task(function* test_changeGuid_invalid() {
   yield rejects(PlacesSyncUtils.bookmarks.changeGuid(makeGuid()),
     "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(