Bug 1451537 - Simplify tag query rewriting in the bookmarks mirror. r?markh,mak draft
authorKit Cambridge <kit@yakshaving.ninja>
Thu, 12 Apr 2018 18:38:34 -0700
changeset 784788 4a4cfcba293c9678c3c6acde96d64b2e038adbdb
parent 784732 a0c804993efc599a95e97bea39fa1528fd0195d8
child 784789 3fc18e6f5e9b21ac49616c0341fd9d400edecb7e
push id107029
push userbmo:kit@mozilla.com
push dateThu, 19 Apr 2018 00:57:52 +0000
reviewersmarkh, mak
bugs1451537
milestone61.0a1
Bug 1451537 - Simplify tag query rewriting in the bookmarks mirror. r?markh,mak MozReview-Commit-ID: 920003s7Jvm
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/test_bookmark_kinds.js
--- 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({