Bug 1294291 - Remove missing GUID handling code from Sync and Places. r=markh
MozReview-Commit-ID: CbhF4s0nNr0
--- 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(