Skip to content

Commit

Permalink
Persist keywords in moz_keywords instead of using the Sync tables.
Browse files Browse the repository at this point in the history
Before this patch, we stored keywords for incoming bookmarks in the
`moz_bookmarks_synced` table, and read them back out from that table
when we uploaded outgoing records. This was done for a few reasons:

* In the beginning, Rust Places didn't support search keywords at all.
  There are plans to unify keywords and custom search engines, since
  they solve the same problem, but it's not currently a priority.
  https://bugzilla.mozilla.org/show_bug.cgi?id=648398 has more
  context.
* However, keyword searches were a very popular feature in Firefox for
  iOS, so we added an API to read them out of `moz_bookmarks_synced`
  in #1345.
* This was still fine, since the only way to get keywords into Fenix
  and iOS was via Sync. (Though there were some gotchas; for instance,
  disconnecting Sync would lose all keywords).
* But the Fennec to Fenix migration meant that there are now two ways
  to get keywords into Fenix, since Fennec supported setting them on
  bookmarks.

To make things more interesting, keywords are handled differently
across our products. Desktop exposes keywords as bookmark properties,
but, internally, associates them with (URL, POST data) tuples.
(This is also why they're closer to custom search engines than
bookmarks). But Sync, Fennec, and the old (before a-s) Firefox for iOS
associated them with individual bookmarks.

This patch takes a middle ground. Keywords are associated with URLs,
like Desktop, and we store those associations in a separate
`moz_keywords` table. This means keywords imported from Fennec should
be migrated, and signing out of Sync in Firefox for iOS no longer
loses keywords. Like Fennec and iOS, we don't support POST data,
since we don't sync it, and there's no UI for setting it.

The keyword syncing code is the same as Desktop's: we remove all
synced keywords from all local URLs, and all local keywords from old
and new synced URLs, then apply incoming items with new keywords and
URLs. We also flag items with the same keyword and different URLs,
as well as items with different keywords and the same URL, for
reupload, to enforce the "1 keyword per URL" association.

Finally, we prepopulate the `moz_keywords` table with existing synced
keywords, so that they can be accessed via
`bookmarks_get_url_for_keyword` without triggering a full sync.
  • Loading branch information
linabutler committed Mar 10, 2020
1 parent e397dcc commit 37c8221
Show file tree
Hide file tree
Showing 8 changed files with 484 additions and 75 deletions.
3 changes: 3 additions & 0 deletions CHANGES_UNRELEASED.md
Expand Up @@ -27,3 +27,6 @@
longer cause syncs to fail ([#2750](https://github.com/mozilla/application-services/pull/2750)),
and bookmarks with duplicate or mismatched tags are reuploaded
([#2774](https://github.com/mozilla/application-services/pull/2774)).
- Improve handling of bookmark search keywords. Keywords are now imported
correctly from Fennec, and signing out of Sync in Firefox for iOS no longer
loses keywords ([#2501](https://github.com/mozilla/application-services/pull/2501)).
13 changes: 13 additions & 0 deletions components/places/sql/create_shared_schema.sql
Expand Up @@ -220,3 +220,16 @@ CREATE TABLE IF NOT EXISTS moz_bookmarks_synced_tag_relation(
ON DELETE CASCADE,
PRIMARY KEY(itemId, tagId)
) WITHOUT ROWID;

-- This table holds search keywords for URLs. Desktop would like to replace
-- these with custom search engines eventually (bug 648398); however, we
-- must still round-trip keywords imported via Sync or migrated from Fennec.
-- Since none of the `moz_bookmarks_synced_*` tables are durable, we store
-- keywords for URLs in a separate table. Unlike Desktop, we don't support
-- custom POST data, since we don't sync it (bug 1345417), and Fennec
-- doesn't write it.
CREATE TABLE IF NOT EXISTS moz_keywords(
place_id INTEGER PRIMARY KEY REFERENCES moz_places(id)
ON DELETE CASCADE,
keyword TEXT NOT NULL UNIQUE
);
32 changes: 32 additions & 0 deletions components/places/sql/create_shared_triggers.sql
Expand Up @@ -306,3 +306,35 @@ BEGIN
foreign_count = foreign_count - 1
WHERE id = OLD.place_id;
END;

-- These triggers adjust the foreign count for URLs with keywords, so that
-- they won't be expired or automatically removed.

CREATE TEMP TRIGGER moz_keywords_afterinsert_trigger
AFTER INSERT ON moz_keywords
BEGIN
UPDATE moz_places SET
foreign_count = foreign_count + 1
WHERE id = NEW.place_id;
END;

CREATE TEMP TRIGGER moz_keywords_afterupdate_trigger
AFTER UPDATE OF place_id ON moz_keywords
WHEN OLD.place_id <> NEW.place_id
BEGIN
UPDATE moz_places SET
foreign_count = foreign_count + 1
WHERE id = NEW.place_id;

UPDATE moz_places SET
foreign_count = foreign_count - 1
WHERE id = OLD.place_id;
END;

CREATE TEMP TRIGGER moz_keywords_afterdelete_trigger
AFTER DELETE ON moz_keywords
BEGIN
UPDATE moz_places SET
foreign_count = foreign_count - 1
WHERE id = OLD.place_id;
END;
5 changes: 4 additions & 1 deletion components/places/sql/create_sync_temp_tables.sql
Expand Up @@ -25,7 +25,8 @@ CREATE TEMP TABLE itemsToApply(
oldTitle TEXT,
newTitle TEXT,
oldPlaceId INTEGER,
newPlaceId INTEGER
newPlaceId INTEGER,
newKeyword TEXT
);

CREATE INDEX existingItems ON itemsToApply(localId) WHERE localId NOT NULL;
Expand All @@ -34,6 +35,8 @@ CREATE INDEX oldPlaceIds ON itemsToApply(newKind, oldPlaceId);

CREATE INDEX newPlaceIds ON itemsToApply(newKind, newPlaceId);

CREATE INDEX newKeywords ON itemsToApply(newKeyword) WHERE newKeyword NOT NULL;

CREATE TEMP TABLE applyNewLocalStructureOps(
mergedGuid TEXT PRIMARY KEY,
mergedParentGuid TEXT NOT NULL,
Expand Down
28 changes: 27 additions & 1 deletion components/places/src/bookmark_sync/incoming.rs
Expand Up @@ -64,7 +64,7 @@ impl<'a> IncomingApplicator<'a> {
let parent_record_id = unpack_optional_id("parentid", b);
let date_added = unpack_optional_i64("dateAdded", b, &mut validity);
let title = unpack_optional_str("title", b, &mut validity);
let keyword = unpack_optional_str("keyword", b, &mut validity);
let keyword = unpack_optional_keyword("keyword", b, &mut validity);

let raw_tags = &b["tags"];
let tags = if let Some(array) = raw_tags.as_array() {
Expand Down Expand Up @@ -528,6 +528,32 @@ fn unpack_optional_i64(
}
}

fn unpack_optional_keyword(
key: &str,
data: &JsonValue,
validity: &mut SyncedBookmarkValidity,
) -> Option<String> {
match &data[key] {
JsonValue::String(ref s) => {
// Like Desktop, we don't reupload if a keyword has leading or
// trailing whitespace, or isn't lowercase.
let k = s.trim();
if k.is_empty() {
None
} else {
Some(k.to_lowercase())
}
}
JsonValue::Null => None,
_ => {
// ...But we do reupload if it's not a string, since older
// clients expect that.
set_reupload(validity);
None
}
}
}

fn set_replace(validity: &mut SyncedBookmarkValidity) {
if *validity < SyncedBookmarkValidity::Replace {
*validity = SyncedBookmarkValidity::Replace;
Expand Down

0 comments on commit 37c8221

Please sign in to comment.