--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -643,38 +643,53 @@ class SyncedBookmarksMirror {
let url = validateURL(record.bmkUri);
if (!url) {
MirrorLog.warn("Ignoring query ${guid} with invalid URL ${url}",
{ guid, url: record.bmkUri });
ignoreCounts.query.url++;
return;
}
+ // Legacy tag queries may use a `place:` URL with a `folder` param that
+ // points to the tag folder ID. We need to rewrite these queries to
+ // directly reference the tag.
+ let params = new URLSearchParams(url.pathname);
+ let type = +params.get("type");
+ if (type == Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_CONTENTS) {
+ let tagFolderName = validateTag(record.folderName);
+ if (!tagFolderName) {
+ MirrorLog.warn("Ignoring tag query ${guid} with invalid tag name " +
+ "${tagFolderName}", { guid, tagFolderName });
+ ignoreCounts.query.url++;
+ return;
+ }
+ url = new URL(`place:tag=${tagFolderName}`);
+ }
+
await this.maybeStoreRemoteURL(url);
let serverModified = determineServerModified(record);
let dateAdded = determineDateAdded(record);
let title = validateTitle(record.title);
- let tagFolderName = validateTag(record.folderName);
let description = validateDescription(record.description);
let smartBookmarkName = typeof record.queryId == "string" ?
record.queryId : null;
await this.db.executeCached(`
REPLACE INTO items(guid, serverModified, needsMerge, kind,
- dateAdded, title, tagFolderName,
- urlId, description, smartBookmarkName)
+ dateAdded, title, urlId, description,
+ smartBookmarkName)
VALUES(:guid, :serverModified, :needsMerge, :kind,
- :dateAdded, NULLIF(:title, ""), :tagFolderName,
+ :dateAdded, NULLIF(:title, ""),
(SELECT id FROM urls
WHERE hash = hash(:url) AND
url = :url),
:description, :smartBookmarkName)`,
{ guid, serverModified, needsMerge,
- kind: SyncedBookmarksMirror.KIND.QUERY, dateAdded, title, tagFolderName,
+ kind: SyncedBookmarksMirror.KIND.QUERY, dateAdded, title,
url: url.href, description, smartBookmarkName });
}
async storeRemoteFolder(record, ignoreCounts, { needsMerge }) {
let guid = validateGuid(record.id);
if (!guid) {
MirrorLog.warn("Ignoring folder with invalid ID", record.id);
ignoreCounts.folder.id++;
@@ -1208,17 +1223,17 @@ class SyncedBookmarksMirror {
{ time: String(elapsedTime),
count: String(rows.length) });
return newLocalContents;
}
/**
* Builds a temporary table with the merge states of all nodes in the merged
- * tree, rewrites tag queries, and updates Places to match the merged tree.
+ * tree and updates Places to match the merged tree.
*
* Conceptually, we examine the merge state of each item, and either keep the
* complete local state, take the complete remote state, or apply a new
* structure state and flag the item for reupload.
*
* Note that we update Places and flag items *before* upload, while iOS
* updates the mirror *after* a successful upload. This simplifies our
* implementation, though we lose idempotent merges. If upload is interrupted,
@@ -1242,19 +1257,16 @@ class SyncedBookmarksMirror {
await this.db.execute(`
INSERT INTO mergeStates(localGuid, mergedGuid, parentGuid, level,
position, valueState, structureState)
VALUES(IFNULL(:localGuid, :mergedGuid), :mergedGuid, :parentGuid,
:level, :position, :valueState, :structureState)`,
mergeStatesParams);
}
- MirrorLog.debug("Rewriting tag queries in mirror");
- await this.rewriteRemoteTagQueries();
-
MirrorLog.debug("Inserting new URLs into Places");
await this.db.execute(`
INSERT OR IGNORE INTO moz_places(url, url_hash, rev_host, hidden,
frecency, guid)
SELECT u.url, u.hash, u.revHost, 0,
(CASE v.kind WHEN :queryKind THEN 0 ELSE -1 END),
IFNULL((SELECT h.guid FROM moz_places h
WHERE h.url_hash = u.hash AND
@@ -1346,65 +1358,16 @@ class SyncedBookmarksMirror {
needsMerge = 0
WHERE needsMerge AND
guid IN (${new Array(chunk.length).fill("?").join(",")})`,
chunk);
}
}
/**
- * Rewrites tag query URLs in the mirror to point to the tag.
- *
- * For example, an incoming tag query of, say, "place:type=7&folder=3" means
- * it is a query for whatever tag was defined by the local record with id=3
- * (and the incoming record has the actual tag in the folderName field).
- */
- async rewriteRemoteTagQueries() {
- let queryRows = await this.db.execute(`
- SELECT u.id AS urlId, u.url, v.tagFolderName
- FROM urls u
- JOIN items v ON v.urlId = u.id
- JOIN mergeStates r ON r.mergedGuid = v.guid
- WHERE r.valueState = :valueState AND
- v.kind = :queryKind AND
- v.tagFolderName NOT NULL AND
- CAST(get_query_param(substr(u.url, 7), "type") AS INT) = :tagContentsType
- `, { valueState: BookmarkMergeState.TYPE.REMOTE,
- queryKind: SyncedBookmarksMirror.KIND.QUERY,
- tagContentsType: Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_CONTENTS });
-
- let urlsParams = [];
- for (let row of queryRows) {
- let url = new URL(row.getResultByName("url"));
- let tagQueryParams = new URLSearchParams(url.pathname);
-
- // Rewrite the query to directly reference the tag.
- tagQueryParams.delete("queryType");
- tagQueryParams.delete("type");
- tagQueryParams.delete("folder");
- tagQueryParams.set("tag", row.getResultByName("tagFolderName"));
-
- let newURLHref = url.protocol + tagQueryParams;
- urlsParams.push({
- urlId: row.getResultByName("urlId"),
- url: newURLHref,
- });
- }
-
- if (urlsParams.length) {
- await this.db.execute(`
- UPDATE urls SET
- url = :url,
- hash = hash(:url)
- WHERE id = :urlId`,
- urlsParams);
- }
- }
-
- /**
* Records Places observer notifications for removed, added, moved, and
* changed items.
*
* @param {BookmarkObserverRecorder} observersToNotify
*/
async noteObserverChanges(observersToNotify) {
MirrorLog.debug("Recording observer notifications for removed items");
// `ORDER BY v.level DESC` sorts deleted children before parents, to ensure
@@ -1970,17 +1933,16 @@ async function initializeMirrorDatabase(
isDeleted BOOLEAN NOT NULL DEFAULT 0,
kind INTEGER NOT NULL DEFAULT -1,
/* The creation date, in milliseconds. */
dateAdded INTEGER NOT NULL DEFAULT 0,
title TEXT,
urlId INTEGER REFERENCES urls(id)
ON DELETE SET NULL,
keyword TEXT,
- tagFolderName TEXT,
description TEXT,
loadInSidebar BOOLEAN,
smartBookmarkName TEXT,
feedURL TEXT,
siteURL TEXT,
/* Only bookmarks and queries must have URLs. */
CHECK(CASE WHEN kind IN (${[
SyncedBookmarksMirror.KIND.BOOKMARK,
@@ -2435,18 +2397,18 @@ async function initializeTempMirrorEntit
WHERE id = OLD.localId;
END`);
// A view of local bookmark tags. Tags, like keywords, are associated with
// URLs, so two bookmarks with the same URL should have the same tags. Unlike
// keywords, one tag may be associated with many different URLs. Tags are also
// different because they're implemented as bookmarks under the hood. Each tag
// is stored as a folder under the tags root, and tagged URLs are stored as
- // untitled bookmarks under these folders. This complexity, along with tag
- // query rewriting, can be removed once bug 424160 lands.
+ // untitled bookmarks under these folders. This complexity can be removed once
+ // bug 424160 lands.
await db.execute(`
CREATE TEMP VIEW localTags(tagEntryId, tagEntryGuid, tagFolderId,
tagFolderGuid, tagEntryPosition, tagEntryType,
tag, placeId) AS
SELECT b.id, b.guid, p.id, p.guid, b.position, b.type, p.title, b.fk
FROM moz_bookmarks b
JOIN moz_bookmarks p ON p.id = b.parent
JOIN moz_bookmarks r ON r.id = p.parent
--- a/toolkit/components/places/tests/sync/test_bookmark_kinds.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_kinds.js
@@ -253,74 +253,127 @@ add_task(async function test_livemarks()
await stopServer();
}
await PlacesUtils.bookmarks.eraseEverything();
await PlacesSyncUtils.bookmarks.reset();
});
add_task(async function test_queries() {
- try {
- let buf = await openMirror("queries");
+ let buf = await openMirror("queries");
+
+ info("Set up places");
- info("Set up places");
+ // create a tag and grab the local folder ID.
+ let tag = await PlacesUtils.bookmarks.insert({
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ parentGuid: PlacesUtils.bookmarks.tagsGuid,
+ title: "a-tag",
+ });
+
+ await PlacesTestUtils.markBookmarksAsSynced();
- // create a tag and grab the local folder ID.
- let tag = await PlacesUtils.bookmarks.insert({
- type: PlacesUtils.bookmarks.TYPE_FOLDER,
- parentGuid: PlacesUtils.bookmarks.tagsGuid,
- title: "a-tag",
- });
-
- await PlacesTestUtils.markBookmarksAsSynced();
+ await PlacesUtils.bookmarks.insertTree({
+ guid: PlacesUtils.bookmarks.menuGuid,
+ children: [
+ {
+ // this entry has a tag= query param for a tag that exists.
+ guid: "queryAAAAAAA",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ title: "TAG_QUERY query",
+ url: `place:tag=a-tag&&sort=14&maxResults=10`,
+ },
+ {
+ // this entry has a tag= query param for a tag that doesn't exist.
+ guid: "queryBBBBBBB",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ title: "TAG_QUERY query but invalid folder id",
+ url: `place:tag=b-tag&sort=14&maxResults=10`,
+ },
+ {
+ // this entry has no tag= query param.
+ guid: "queryCCCCCCC",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ title: "TAG_QUERY without a folder at all",
+ url: "place:sort=14&maxResults=10",
+ },
+ {
+ // this entry has only a tag= query.
+ guid: "queryDDDDDDD",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ title: "TAG_QUERY without a folder at all",
+ url: "place:tag=a-tag",
+ },
+ ],
+ });
- await PlacesUtils.bookmarks.insertTree({
- guid: PlacesUtils.bookmarks.menuGuid,
- children: [
- {
- // this entry has a tag= query param for a tag that exists.
- guid: "queryAAAAAAA",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- title: "TAG_QUERY query",
- url: `place:tag=a-tag&&sort=14&maxResults=10`,
- },
- {
- // this entry has a tag= query param for a tag that doesn't exist.
- guid: "queryBBBBBBB",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- title: "TAG_QUERY query but invalid folder id",
- url: `place:tag=b-tag&sort=14&maxResults=10`,
- },
- {
- // this entry has no tag= query param.
- guid: "queryCCCCCCC",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- title: "TAG_QUERY without a folder at all",
- url: "place:sort=14&maxResults=10",
- },
- {
- // this entry has only a tag= query.
- guid: "queryDDDDDDD",
- type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- title: "TAG_QUERY without a folder at all",
- url: "place:tag=a-tag",
- },
- ],
- });
+ info("Make remote changes");
+ await storeRecords(buf, shuffle([{
+ id: "toolbar",
+ type: "folder",
+ children: ["queryEEEEEEE", "queryFFFFFFF", "queryGGGGGGG"],
+ }, {
+ // Legacy tag query.
+ id: "queryEEEEEEE",
+ type: "query",
+ title: "E",
+ bmkUri: "place:type=7&folder=999",
+ folderName: "taggy",
+ }, {
+ // New tag query.
+ id: "queryFFFFFFF",
+ type: "query",
+ title: "F",
+ bmkUri: "place:tag=a-tag",
+ folderName: "a-tag",
+ }, {
+ // Legacy tag query referencing the same tag as the new query.
+ id: "queryGGGGGGG",
+ type: "query",
+ title: "G",
+ bmkUri: "place:type=7&folder=111&something=else",
+ folderName: "a-tag",
+ }]));
- info("Create records to upload");
- let changes = await buf.apply();
- Assert.strictEqual(changes.queryAAAAAAA.cleartext.folderName, tag.title);
- Assert.strictEqual(changes.queryBBBBBBB.cleartext.folderName, "b-tag");
- Assert.strictEqual(changes.queryCCCCCCC.cleartext.folderName, undefined);
- Assert.strictEqual(changes.queryDDDDDDD.cleartext.folderName, tag.title);
- } finally {
- await PlacesUtils.bookmarks.eraseEverything();
- await PlacesSyncUtils.bookmarks.reset();
- }
+ info("Create records to upload");
+ let changes = await buf.apply();
+ Assert.strictEqual(changes.queryAAAAAAA.cleartext.folderName, tag.title);
+ Assert.strictEqual(changes.queryBBBBBBB.cleartext.folderName, "b-tag");
+ Assert.strictEqual(changes.queryCCCCCCC.cleartext.folderName, undefined);
+ Assert.strictEqual(changes.queryDDDDDDD.cleartext.folderName, tag.title);
+
+ await assertLocalTree(PlacesUtils.bookmarks.toolbarGuid, {
+ guid: PlacesUtils.bookmarks.toolbarGuid,
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ index: 1,
+ title: BookmarksToolbarTitle,
+ children: [{
+ guid: "queryEEEEEEE",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 0,
+ title: "E",
+ url: "place:tag=taggy",
+ }, {
+ guid: "queryFFFFFFF",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 1,
+ title: "F",
+ url: "place:tag=a-tag",
+ }, {
+ guid: "queryGGGGGGG",
+ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ index: 2,
+ title: "G",
+ url: "place:tag=a-tag",
+ }],
+ }, "Should rewrite legacy remote tag queries");
+
+ await buf.finalize();
+ await PlacesUtils.bookmarks.eraseEverything();
+ await PlacesSyncUtils.bookmarks.reset();
});
// Bug 632287.
add_task(async function test_mismatched_but_compatible_folder_types() {
let buf = await openMirror("mismatched_types");
info("Set up mirror");
await PlacesUtils.bookmarks.insertTree({