Bug 1706933: When sync removes a bookmark with a keyword, should also remove the keyword if necessary r=mak,lina

Differential Revision: https://phabricator.services.mozilla.com/D167333

services/sync/tests/unit/test_bookmark_store.js

@@ -273,6 +273,11 @@ add_bookmark_test(async function test_deleted(engine) {
     let newrec = await store.createRecord(bmk1.guid);
     Assert.equal(null, item);
     Assert.equal(newrec.deleted, true);
+    _("Verify that the keyword has been cleared.");
+    let keyword = await PlacesUtils.keywords.fetch({
+      url: "http://getfirefox.com/",
+    });
+    Assert.equal(null, keyword);
   } finally {
     _("Clean up.");
     await store.wipe();

toolkit/components/places/SyncedBookmarksMirror.sys.mjs

@@ -1927,7 +1927,8 @@ async function initializeTempMirrorEntities(db) {
     /* We record the original level of the removed item in the tree so that we
        can notify children before parents. */
     level INTEGER NOT NULL DEFAULT -1,
-    isUntagging BOOLEAN NOT NULL DEFAULT 0
+    isUntagging BOOLEAN NOT NULL DEFAULT 0,
+    keywordRemoved BOOLEAN NOT NULL DEFAULT 0
   )`);
 
   await db.execute(
@@ -2114,7 +2115,7 @@ class BookmarkObserverRecorder {
     await this.db.execute(
       `SELECT v.itemId AS id, v.parentId, v.parentGuid, v.position, v.type,
               (SELECT h.url FROM moz_places h WHERE h.id = v.placeId) AS url,
-              v.title, v.guid, v.isUntagging
+              v.title, v.guid, v.isUntagging, v.keywordRemoved
        FROM itemsRemoved v
        ${this.orderBy("v.level", "v.parentId", "v.position")}`,
       null,
@@ -2135,6 +2136,9 @@ class BookmarkObserverRecorder {
           isUntagging: row.getResultByName("isUntagging"),
         };
         this.noteItemRemoved(info);
+        if (row.getResultByName("keywordRemoved")) {
+          this.shouldInvalidateKeywords = true;
+        }
       }
     );
     if (this.signal.aborted) {

toolkit/components/places/bookmark_sync/src/store.rs

@@ -979,9 +979,9 @@ fn remove_local_items(
         "WITH
          ops(guid, level) AS (VALUES {})
          INSERT INTO itemsRemoved(itemId, parentId, position, type, title,
-                                  placeId, guid, parentGuid, level)
+                                  placeId, guid, parentGuid, level, keywordRemoved)
          SELECT b.id, b.parent, b.position, b.type, IFNULL(b.title, \"\"), b.fk,
-                b.guid, p.guid, n.level
+                b.guid, p.guid, n.level, EXISTS(SELECT 1 FROM moz_keywords k WHERE k.place_id = b.fk)
          FROM ops n
          JOIN moz_bookmarks b ON b.guid = n.guid
          JOIN moz_bookmarks p ON p.id = b.parent",
@@ -1034,6 +1034,25 @@ fn remove_local_items(
     }
     annos_statement.execute()?;
 
+    debug!(
+        driver,
+        "Removing keywords associated with deleted bookmarks"
+    );
+    let mut keywords_statement = db.prepare(format!(
+        "DELETE FROM moz_keywords
+         WHERE place_id IN (SELECT b.fk FROM moz_bookmarks b
+            WHERE b.guid IN ({}))",
+        repeat_sql_vars(ops.len()),
+    ))?;
+    for (index, op) in ops.iter().enumerate() {
+        controller.err_if_aborted()?;
+        keywords_statement.bind_by_index(
+            index as u32,
+            nsString::from(&*op.local_node().guid.as_str()),
+        )?;
+    }
+    keywords_statement.execute()?;
+
     debug!(driver, "Removing deleted items from Places");
     let mut delete_statement = db.prepare(format!(
         "DELETE FROM moz_bookmarks

toolkit/components/places/tests/sync/test_bookmark_deletion.js

@@ -1473,3 +1473,130 @@ add_task(async function test_newer_move_to_deleted() {
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
+
+add_task(async function test_remotely_deleted_also_removes_keyword() {
+  let buf = await openMirror("remotely_deleted_removes_keyword");
+
+  info("Set up mirror");
+  await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.menuGuid,
+    children: [
+      {
+        guid: "bookmarkAAAA",
+        title: "A",
+        url: "http://example.com/a",
+        keyword: "keyworda",
+      },
+      {
+        guid: "bookmarkBBBB",
+        title: "B",
+        url: "http://example.com/b",
+        keyword: "keywordb",
+      },
+    ],
+  });
+  await storeRecords(
+    buf,
+    [
+      {
+        id: "menu",
+        parentid: "places",
+        type: "folder",
+        children: ["bookmarkAAAA", "bookmarkBBBB"],
+      },
+      {
+        id: "bookmarkAAAA",
+        parentid: "menu",
+        type: "bookmark",
+        title: "A",
+        bmkUri: "http://example.com/a",
+        keyword: "keyworda",
+      },
+      {
+        id: "bookmarkBBBB",
+        parentid: "menu",
+        type: "bookmark",
+        title: "B",
+        bmkUri: "http://example.com/b",
+        keyword: "keywordb",
+      },
+    ],
+    { needsMerge: false }
+  );
+  await PlacesTestUtils.markBookmarksAsSynced();
+
+  // Validate the keywords exists
+  let has_keyword_a = await PlacesUtils.keywords.fetch({
+    url: "http://example.com/a",
+  });
+  Assert.equal(has_keyword_a.keyword, "keyworda");
+
+  let has_keyword_b = await PlacesUtils.keywords.fetch({
+    url: "http://example.com/b",
+  });
+  Assert.equal(has_keyword_b.keyword, "keywordb");
+
+  info("Make remote changes: delete A & B");
+  await storeRecords(buf, [
+    {
+      id: "menu",
+      parentid: "places",
+      type: "folder",
+      children: [],
+    },
+    {
+      id: "bookmarkAAAA",
+      deleted: true,
+    },
+    {
+      id: "bookmarkBBBB",
+      deleted: true,
+    },
+  ]);
+
+  info("Apply remote");
+  let changesToUpload = await buf.apply();
+
+  let idsToUpload = inspectChangeRecords(changesToUpload);
+  deepEqual(
+    idsToUpload,
+    {
+      updated: [],
+      deleted: [],
+    },
+    "No local changes done"
+  );
+
+  await assertLocalTree(
+    PlacesUtils.bookmarks.menuGuid,
+    {
+      guid: PlacesUtils.bookmarks.menuGuid,
+      type: PlacesUtils.bookmarks.TYPE_FOLDER,
+      index: 0,
+      title: BookmarksMenuTitle,
+    },
+    "Should've remove A & B from menu"
+  );
+
+  // Validate the keyword no longer exists after removing the bookmark
+  let no_keyword_a = await PlacesUtils.keywords.fetch({
+    url: "http://example.com/a",
+  });
+  Assert.equal(no_keyword_a, null);
+
+  // Both keywords should've been removed after the sync
+  let no_keyword_b = await PlacesUtils.keywords.fetch({
+    url: "http://example.com/b",
+  });
+  Assert.equal(no_keyword_b, null);
+
+  let tombstones = await PlacesTestUtils.fetchSyncTombstones();
+  deepEqual(tombstones, [], "Should not store local tombstones");
+
+  await storeChangesInMirror(buf, changesToUpload);
+  deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
+
+  await buf.finalize();
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesSyncUtils.bookmarks.reset();
+});