Bug 1451537 - Simplify tag query rewriting in the bookmarks mirror. r=mak,markh
authorKit Cambridge <kit@yakshaving.ninja>
Thu, 12 Apr 2018 18:38:34 -0700
changeset 414434 1fa6af3138d54a43e41f668e62ecc8cd22d34308
parent 414433 b3a530c29fbe1a471fded9632d9affcca79d58c7
child 414435 8dbdf2617258f1806e9277bb4750f263cae6f121
push id102329
push usernerli@mozilla.com
push dateThu, 19 Apr 2018 09:59:48 +0000
treeherdermozilla-inbound@f4f07a24b951 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, markh
bugs1451537
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1451537 - Simplify tag query rewriting in the bookmarks mirror. r=mak,markh 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({