application-services icon indicating copy to clipboard operation
application-services copied to clipboard

Bug 1882381 - Switch to using the data path for the suggest DB

Open bendk opened this issue 1 year ago • 3 comments

  • Use the data_path rather than cache_path for the suggest DB. This is prep for for storing the suggestion dismissal data in the DB, which should not be reset on schema upgrades.
  • Updated the schema code so that it's split into temporary tables vs permanent tables. All the current tables are temporary, the new dismissed_suggestion table is the only permanent one. Temporary tables are always thrown away and recreated when there's a schema update.
  • Other than adding the dismissed_suggestion table, this doesn't implement any of the suggestion dismissal functionality.

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • [x] This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • [x] Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • [x] Tests: This PR includes thorough tests or an explanation of why it does not
  • [x] Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • [x] Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

bendk avatar Feb 27 '24 19:02 bendk

Putting this up slightly early for feedback, but don't merge until Android and iOS is using the SuggestStoreBuilder (iOS PR, Android PR)

bendk avatar Feb 27 '24 19:02 bendk

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.08%. Comparing base (f44e9d0) to head (b112ff7).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6146   +/-   ##
=======================================
  Coverage   84.08%   84.08%           
=======================================
  Files         117      117           
  Lines       15629    15629           
=======================================
  Hits        13141    13141           
  Misses       2488     2488           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Feb 27 '24 19:02 codecov-commenter

See #6147 for how this will be used.

bendk avatar Feb 27 '24 21:02 bendk

Pushing this one out again because:

  • The Android and iOS PRs are both merged! Nothing is blocking this anymore
  • I realized that my scheme to automatically delete some of the tables but keep the rest was really fragile. I don't think it would work correctly if we ever added or removed tables from the temp tables list. Instead, I'm hoping we can use the code from SuggestDao.clear to do what we want. Does that make sense?

bendk avatar Mar 06 '24 19:03 bendk