From 2ea6b739bc9706ec958f369906d82ed6e10fc442 Mon Sep 17 00:00:00 2001 From: Kit Cambridge Date: Thu, 12 Apr 2018 18:38:34 -0700 Subject: [PATCH] Bug 1451537 - Simplify tag query rewriting in the bookmarks mirror. r=mak,markh MozReview-Commit-ID: 920003s7Jvm --HG-- extra : rebase_source : 7f62f4f283e6d1c065651adfebe97038108504ac --- .../places/SyncedBookmarksMirror.jsm | 84 +++------ .../places/tests/sync/test_bookmark_kinds.js | 159 ++++++++++++------ 2 files changed, 129 insertions(+), 114 deletions(-) diff --git a/toolkit/components/places/SyncedBookmarksMirror.jsm b/toolkit/components/places/SyncedBookmarksMirror.jsm index 78069e4a3681..1de3cf5baebb 100644 --- a/toolkit/components/places/SyncedBookmarksMirror.jsm +++ b/toolkit/components/places/SyncedBookmarksMirror.jsm @@ -648,28 +648,43 @@ class SyncedBookmarksMirror { 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 }); } @@ -1213,7 +1228,7 @@ class SyncedBookmarksMirror { /** * 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 @@ -1247,9 +1262,6 @@ class SyncedBookmarksMirror { 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, @@ -1350,55 +1362,6 @@ class SyncedBookmarksMirror { } } - /** - * 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. @@ -1975,7 +1938,6 @@ async function initializeMirrorDatabase(db) { urlId INTEGER REFERENCES urls(id) ON DELETE SET NULL, keyword TEXT, - tagFolderName TEXT, description TEXT, loadInSidebar BOOLEAN, smartBookmarkName TEXT, @@ -2440,8 +2402,8 @@ async function initializeTempMirrorEntities(db) { // 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, diff --git a/toolkit/components/places/tests/sync/test_bookmark_kinds.js b/toolkit/components/places/tests/sync/test_bookmark_kinds.js index 69aa4ffea1b6..cce04acd9fe5 100644 --- a/toolkit/components/places/tests/sync/test_bookmark_kinds.js +++ b/toolkit/components/places/tests/sync/test_bookmark_kinds.js @@ -258,64 +258,117 @@ add_task(async function test_livemarks() { }); 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", - }); + // 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 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("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("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); + + 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.