Bug 1380718 - Include the operation name in bookmark validation errors. r=mak
MozReview-Commit-ID: H4vp6ZULSq7
--- a/browser/components/extensions/test/xpcshell/test_ext_bookmarks.js
+++ b/browser/components/extensions/test/xpcshell/test_ext_bookmarks.js
@@ -107,17 +107,17 @@ add_task(async function test_bookmarks()
});
browser.bookmarks.onRemoved.addListener((id, info) => {
collectedEvents.push({event: "onRemoved", id, info});
});
browser.bookmarks.get(["not-a-bookmark-guid"]).then(expectedError, invalidGuidError => {
browser.test.assertTrue(
- invalidGuidError.message.includes("Invalid value for property 'guid': not-a-bookmark-guid"),
+ invalidGuidError.message.includes("Invalid value for property 'guid': \"not-a-bookmark-guid\""),
"Expected error thrown when trying to get a bookmark using an invalid guid"
);
return browser.bookmarks.get([nonExistentId]).then(expectedError, nonExistentIdError => {
browser.test.assertTrue(
nonExistentIdError.message.includes("Bookmark not found"),
"Expected error thrown when trying to get a bookmark using a non-existent Id"
);
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -179,17 +179,17 @@ var Bookmarks = Object.freeze({
*/
insert(info) {
let now = new Date();
let addedTime = (info && info.dateAdded) || now;
let modTime = addedTime;
if (addedTime > now) {
modTime = now;
}
- let insertInfo = validateBookmarkObject(info,
+ let insertInfo = validateBookmarkObject("Bookmarks.jsm: insert", info,
{ type: { defaultValue: this.TYPE_BOOKMARK },
index: { defaultValue: this.DEFAULT_INDEX },
url: { requiredIf: b => b.type == this.TYPE_BOOKMARK,
validIf: b => b.type == this.TYPE_BOOKMARK },
parentGuid: { required: true },
title: { defaultValue: "",
validIf: b => b.type == this.TYPE_BOOKMARK ||
b.type == this.TYPE_FOLDER ||
@@ -342,17 +342,18 @@ var Bookmarks = Object.freeze({
if (!info.hasOwnProperty("guid")) {
info.guid = PlacesUtils.history.makeGuid();
}
// Set the correct parent guid.
info.parentGuid = parentGuid;
// Ensure to use the same date for dateAdded and lastModified, even if
// dateAdded may be imposed by the caller.
let time = (info && info.dateAdded) || fallbackLastAdded;
- let insertInfo = validateBookmarkObject(info, {
+ let insertInfo = validateBookmarkObject("Bookmarks.jsm: insertTree",
+ info, {
type: { defaultValue: TYPE_BOOKMARK },
url: { requiredIf: b => b.type == TYPE_BOOKMARK,
validIf: b => b.type == TYPE_BOOKMARK },
parentGuid: { required: true },
title: { defaultValue: "",
validIf: b => b.type == TYPE_BOOKMARK ||
b.type == TYPE_FOLDER ||
b.title === "" },
@@ -508,17 +509,17 @@ var Bookmarks = Object.freeze({
* @resolves to an object representing the updated bookmark.
* @rejects if it's not possible to update the given bookmark.
* @throws if the arguments are invalid.
*/
update(info) {
// The info object is first validated here to ensure it's consistent, then
// it's compared to the existing item to remove any properties that don't
// need to be updated.
- let updateInfo = validateBookmarkObject(info,
+ let updateInfo = validateBookmarkObject("Bookmarks.jsm: update", info,
{ guid: { required: true },
index: { requiredIf: b => b.hasOwnProperty("parentGuid"),
validIf: b => b.index >= 0 || b.index == this.DEFAULT_INDEX },
source: { defaultValue: this.SOURCES.DEFAULT }
});
// There should be at last one more property in addition to guid and source.
if (Object.keys(updateInfo).length < 3)
@@ -542,17 +543,17 @@ var Bookmarks = Object.freeze({
const now = new Date();
let lastModifiedDefault = now;
// In the case where `dateAdded` is specified, but `lastModified` is not,
// we only update `lastModified` if it is older than the new `dateAdded`.
if (!("lastModified" in updateInfo) &&
"dateAdded" in updateInfo) {
lastModifiedDefault = new Date(Math.max(item.lastModified, updateInfo.dateAdded));
}
- updateInfo = validateBookmarkObject(updateInfo,
+ updateInfo = validateBookmarkObject("Bookmarks.jsm: update", updateInfo,
{ url: { validIf: () => item.type == this.TYPE_BOOKMARK },
title: { validIf: () => [ this.TYPE_BOOKMARK,
this.TYPE_FOLDER ].includes(item.type) },
lastModified: { defaultValue: lastModifiedDefault,
validIf: b => b.lastModified >= now ||
b.lastModified >= (b.dateAdded || item.dateAdded) },
dateAdded: { defaultValue: item.dateAdded }
});
@@ -728,17 +729,17 @@ var Bookmarks = Object.freeze({
}
if (!("source" in options)) {
options.source = Bookmarks.SOURCES.DEFAULT;
}
// Even if we ignore any other unneeded property, we still validate any
// known property to reduce likelihood of hidden bugs.
- let removeInfo = validateBookmarkObject(info);
+ let removeInfo = validateBookmarkObject("Bookmarks.jsm: remove", info);
return (async function() {
let item = await fetchBookmark(removeInfo);
if (!item)
throw new Error("No bookmarks found for the provided GUID.");
item = await removeBookmark(item, options);
@@ -909,17 +910,18 @@ var Bookmarks = Object.freeze({
index: { requiredIf: b => b.hasOwnProperty("parentGuid"),
validIf: b => typeof(b.index) == "number" &&
b.index >= 0 || b.index == this.DEFAULT_INDEX }
};
}
// Even if we ignore any other unneeded property, we still validate any
// known property to reduce likelihood of hidden bugs.
- let fetchInfo = validateBookmarkObject(info, behavior);
+ let fetchInfo = validateBookmarkObject("Bookmarks.jsm: fetch", info,
+ behavior);
return (async function() {
let results;
if (fetchInfo.hasOwnProperty("url"))
results = await fetchBookmarksByURL(fetchInfo, options && options.concurrent);
else if (fetchInfo.hasOwnProperty("guid"))
results = await fetchBookmark(fetchInfo, options && options.concurrent);
else if (fetchInfo.hasOwnProperty("parentGuid") && fetchInfo.hasOwnProperty("index"))
@@ -1034,17 +1036,18 @@ var Bookmarks = Object.freeze({
* Defaults to nsINavBookmarksService::SOURCE_DEFAULT.
*
* @return {Promise} resolved when reordering is complete.
* @rejects if an error happens while reordering.
* @throws if the arguments are invalid.
*/
reorder(parentGuid, orderedChildrenGuids, options = {}) {
let info = { guid: parentGuid };
- info = validateBookmarkObject(info, { guid: { required: true } });
+ info = validateBookmarkObject("Bookmarks.jsm: reorder", info,
+ { guid: { required: true } });
if (!Array.isArray(orderedChildrenGuids) || !orderedChildrenGuids.length)
throw new Error("Must provide a sorted array of children GUIDs.");
try {
orderedChildrenGuids.forEach(PlacesUtils.BOOKMARK_VALIDATORS.guid);
} catch (ex) {
throw new Error("Invalid GUID found in the sorted children array.");
}
@@ -2027,18 +2030,18 @@ function rowsToItemsArray(rows) {
configurable: true });
}
}
return item;
});
}
-function validateBookmarkObject(input, behavior) {
- return PlacesUtils.validateItemProperties(
+function validateBookmarkObject(name, input, behavior) {
+ return PlacesUtils.validateItemProperties(name,
PlacesUtils.BOOKMARK_VALIDATORS, input, behavior);
}
/**
* Updates frecency for a list of URLs.
*
* @param db
* the Sqlite.jsm connection handle.
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -358,28 +358,31 @@ const BookmarkSyncUtils = PlacesSyncUtil
*
* @param changeRecords
* A changeset containing sync change records, as returned by
* `pullChanges`.
* @return {Promise} resolved once all records have been updated.
*/
pushChanges(changeRecords) {
return PlacesUtils.withConnectionWrapper(
- "BookmarkSyncUtils.pushChanges", async function(db) {
+ "BookmarkSyncUtils: pushChanges", async function(db) {
let skippedCount = 0;
let syncedTombstoneGuids = [];
let syncedChanges = [];
for (let syncId in changeRecords) {
// Validate change records to catch coding errors.
- let changeRecord = validateChangeRecord(changeRecords[syncId], {
- tombstone: { required: true },
- counter: { required: true },
- synced: { required: true },
- });
+ let changeRecord = validateChangeRecord(
+ "BookmarkSyncUtils: pushChanges",
+ changeRecords[syncId], {
+ tombstone: { required: true },
+ counter: { required: true },
+ synced: { required: true },
+ }
+ );
// Sync sets the `synced` flag for reconciled or successfully
// uploaded items. If upload failed, ignore the change; we'll
// try again on the next sync.
if (!changeRecord.synced) {
skippedCount++;
continue;
}
@@ -631,19 +634,18 @@ const BookmarkSyncUtils = PlacesSyncUtil
* object representing a bookmark-item, as defined above.
*
* @return {Promise} resolved when the update is complete.
* @resolves to an object representing the updated bookmark.
* @rejects if it's not possible to update the given bookmark.
* @throws if the arguments are invalid.
*/
async update(info) {
- let updateInfo = validateSyncBookmarkObject(info,
- { syncId: { required: true }
- });
+ let updateInfo = validateSyncBookmarkObject("BookmarkSyncUtils: update",
+ info, { syncId: { required: true } });
return updateSyncBookmark(updateInfo);
},
/**
* Inserts a synced bookmark into the tree. Only Sync should call this
* method; other callers should use `Bookmarks.insert`.
*
@@ -665,17 +667,17 @@ const BookmarkSyncUtils = PlacesSyncUtil
* object representing a synced bookmark.
*
* @return {Promise} resolved when the creation is complete.
* @resolves to an object representing the created bookmark.
* @rejects if it's not possible to create the requested bookmark.
* @throws if the arguments are invalid.
*/
async insert(info) {
- let insertInfo = validateNewBookmark(info);
+ let insertInfo = validateNewBookmark("BookmarkSyncUtils: insert", info);
return insertSyncBookmark(insertInfo);
},
/**
* Fetches a Sync bookmark object for an item in the tree. The object contains
* the following properties, depending on the item's kind:
*
* - kind (all): A string representing the item's kind.
@@ -845,25 +847,25 @@ const BookmarkSyncUtils = PlacesSyncUtil
return Math.min(...possible);
}
});
XPCOMUtils.defineLazyGetter(this, "BookmarkSyncLog", () => {
return Log.repository.getLogger("BookmarkSyncUtils");
});
-function validateSyncBookmarkObject(input, behavior) {
- return PlacesUtils.validateItemProperties(
+function validateSyncBookmarkObject(name, input, behavior) {
+ return PlacesUtils.validateItemProperties(name,
PlacesUtils.SYNC_BOOKMARK_VALIDATORS, input, behavior);
}
// Validates a sync change record as returned by `pullChanges` and passed to
// `pushChanges`.
-function validateChangeRecord(changeRecord, behavior) {
- return PlacesUtils.validateItemProperties(
+function validateChangeRecord(name, changeRecord, behavior) {
+ return PlacesUtils.validateItemProperties(name,
PlacesUtils.SYNC_CHANGE_RECORD_VALIDATORS, changeRecord, behavior);
}
// Similar to the private `fetchBookmarksByParent` implementation in
// `Bookmarks.jsm`.
var fetchChildGuids = async function(db, parentGuid) {
let rows = await db.executeCached(`
SELECT guid
@@ -1227,17 +1229,18 @@ var updateSyncBookmark = async function(
updateInfo.syncId} livemarks have different URLs`);
}
}
if (shouldReinsert) {
if (!updateInfo.hasOwnProperty("dateAdded")) {
updateInfo.dateAdded = oldBookmarkItem.dateAdded.getTime();
}
- let newInfo = validateNewBookmark(updateInfo);
+ let newInfo = validateNewBookmark("BookmarkSyncUtils: reinsert",
+ updateInfo);
await PlacesUtils.bookmarks.remove({
guid,
source: SOURCE_SYNC,
});
// A reinsertion likely indicates a confused client, since there aren't
// public APIs for changing livemark URLs or an item's kind (e.g., turning
// a folder into a separator while preserving its annos and position).
// This might be a good case to repair later; for now, we assume Sync has
@@ -1358,18 +1361,18 @@ var updateBookmarkMetadata = async funct
PlacesUtils.annotations.EXPIRE_NEVER,
SOURCE_SYNC);
newItem.query = updateInfo.query;
}
return newItem;
};
-function validateNewBookmark(info) {
- let insertInfo = validateSyncBookmarkObject(info,
+function validateNewBookmark(name, info) {
+ let insertInfo = validateSyncBookmarkObject(name, info,
{ kind: { required: true },
syncId: { required: true },
url: { requiredIf: b => [ BookmarkSyncUtils.KINDS.BOOKMARK,
BookmarkSyncUtils.KINDS.QUERY ].includes(b.kind),
validIf: b => [ BookmarkSyncUtils.KINDS.BOOKMARK,
BookmarkSyncUtils.KINDS.QUERY ].includes(b.kind) },
parentSyncId: { required: true },
title: { validIf: b => [ BookmarkSyncUtils.KINDS.BOOKMARK,
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -517,16 +517,19 @@ this.PlacesUtils = {
node = node.parent;
}
},
/**
* Checks validity of an object, filling up default values for optional
* properties.
*
+ * @param {string} name
+ * The operation name. This is included in the error message if
+ * validation fails.
* @param validators (object)
* An object containing input validators. Keys should be field names;
* values should be validation functions.
* @param props (object)
* The object to validate.
* @param behavior (object) [optional]
* Object defining special behavior for some of the properties.
* The following behaviors may be optionally set:
@@ -538,34 +541,34 @@ this.PlacesUtils = {
* - validIf: if the provided condition is not satisfied, then this
* property is invalid.
* - defaultValue: an undefined property should default to this value.
*
* @return a validated and normalized item.
* @throws if the object contains invalid data.
* @note any unknown properties are pass-through.
*/
- validateItemProperties(validators, props, behavior = {}) {
+ validateItemProperties(name, validators, props, behavior = {}) {
if (!props)
- throw new Error("Input should be a valid object");
+ throw new Error(`${name}: Input should be a valid object`);
// Make a shallow copy of `props` to avoid mutating the original object
// when filling in defaults.
let input = Object.assign({}, props);
let normalizedInput = {};
let required = new Set();
for (let prop in behavior) {
if (behavior[prop].hasOwnProperty("required") && behavior[prop].required) {
required.add(prop);
}
if (behavior[prop].hasOwnProperty("requiredIf") && behavior[prop].requiredIf(input)) {
required.add(prop);
}
if (behavior[prop].hasOwnProperty("validIf") && input[prop] !== undefined &&
!behavior[prop].validIf(input)) {
- throw new Error(`Invalid value for property '${prop}': ${JSON.stringify(input[prop])}`);
+ throw new Error(`${name}: Invalid value for property '${prop}': ${JSON.stringify(input[prop])}`);
}
if (behavior[prop].hasOwnProperty("defaultValue") && input[prop] === undefined) {
input[prop] = behavior[prop].defaultValue;
}
if (behavior[prop].hasOwnProperty("replaceWith")) {
input[prop] = behavior[prop].replaceWith;
}
}
@@ -576,22 +579,22 @@ this.PlacesUtils = {
} else if (input[prop] === undefined) {
// Skip undefined properties that are not required.
continue;
}
if (validators.hasOwnProperty(prop)) {
try {
normalizedInput[prop] = validators[prop](input[prop], input);
} catch (ex) {
- throw new Error(`Invalid value for property '${prop}': ${input[prop]}`);
+ throw new Error(`${name}: Invalid value for property '${prop}': ${JSON.stringify(input[prop])}`);
}
}
}
if (required.size > 0)
- throw new Error(`The following properties were expected: ${[...required].join(", ")}`);
+ throw new Error(`${name}: The following properties were expected: ${[...required].join(", ")}`);
return normalizedInput;
},
BOOKMARK_VALIDATORS,
SYNC_BOOKMARK_VALIDATORS,
SYNC_CHANGE_RECORD_VALIDATORS,
QueryInterface: XPCOMUtils.generateQI([