Bug 1294291 - Remove missing GUID handling code from Sync and Places. r=markh
authorKit Cambridge <kcambridge@mozilla.com>
Tue, 06 Sep 2016 11:29:32 -0700
changeset 312877 d090231db437d7bf918f97f419c0c858882658b3
parent 312876 556e6c27c5ca33cd7f21bd7ad1be3c8976da909d
child 312878 2e6b9e57d5a0527bd51f86be22e930acf450cf8d
push id30663
push usercbook@mozilla.com
push dateWed, 07 Sep 2016 15:12:31 +0000
treeherdermozilla-central@3d0b41fdd93b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1294291
milestone51.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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(