Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persist keywords in moz_keywords instead of using the Sync tables #2501

Merged
merged 1 commit into from Mar 11, 2020

Conversation

linabutler
Copy link
Contributor

@linabutler linabutler commented Jan 21, 2020

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 Add and expose a getBookmarkURLForKeyword API. #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.

Pull Request checklist

  • Quality: This PR builds and tests run cleanly
    • cargo test --all produces no test failures
    • cargo clippy --all --all-targets --all-features runs without emitting any warnings
    • cargo fmt does not produce any changes to the code
    • ./gradlew ktlint detekt runs without emitting any warnings
    • swiftformat --swiftversion 4 megazords components/*/ios && swiftlint runs without emitting any warnings or producing changes
    • Note: For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

@linabutler
Copy link
Contributor Author

This needs more tests, but I thought I'd get the initial version up for feedback. Does all that make sense?

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I agree it needs tests, and should it have a migration step to grab the keywords out of the _synced table? (Even if it should theoretically all come out in the wash, there seems a risk that the engine will end up reset before it syncs? But I guess that's the current status-quo...)

components/places/src/bookmark_sync/store.rs Outdated Show resolved Hide resolved
@linabutler
Copy link
Contributor Author

I think this is ready for re-review! ✨ Also tagging @lougeniaC64, since she just reviewed tags and this is pretty similar. 😁

Copy link
Contributor

@lougeniaC64 lougeniaC64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admittedly it took me some research to understand what was happening (I haven't used keywords) but the explanation and the comments in the code were extremely helpful. This makes sense to me.

Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

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.
@linabutler linabutler merged commit b26cd02 into master Mar 11, 2020
@linabutler linabutler deleted the keywords branch March 11, 2020 01:16
linabutler added a commit that referenced this pull request Mar 11, 2020
If a Place has keywords, we should delete them from `moz_keywords`
first, before deleting the Place from `moz_places`. This is to catch
coding errors where we accidentally ignore the `foreign_count` and
delete the Place, losing its keywords in the process.

Since we never shipped the schema in #2501 in a release, this doesn't
need a migration.
linabutler added a commit that referenced this pull request Mar 11, 2020
If a Place has keywords, we should delete them from `moz_keywords`
first, before deleting the Place from `moz_places`. This is to catch
coding errors where we accidentally ignore the `foreign_count` and
delete the Place, losing its keywords in the process.

Since we never shipped the schema in #2501 in a release, this doesn't
need a migration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants