Bug 1876208 - API for dismissing suggestions

[bendk]

(This is on top of #5546, let's merge that one first)

Dismissed suggestion Urls are stored in the database and not returned again in subsequent queries.

This currently works with most providers, but not Yelp or Weather.

Pull Request checklist

• Breaking changes: This PR follows our breaking change policy
  • [ ] 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
• [ ] 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.
• [ ] Tests: This PR includes thorough tests or an explanation of why it does not
• [ ] 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
• [ ] 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.

[codecov-commenter]

Codecov Report

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

Project coverage is 84.09%. Comparing base (48798d2) to head (b934153).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6147   +/-   ##
=======================================
  Coverage   84.09%   84.09%           
=======================================
  Files         117      117           
  Lines       15627    15627           
=======================================
  Hits        13141    13141           
  Misses       2486     2486           

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

[bendk]

We've decided to stop storing unparsable records and simply always drop and recreate the suggestions DB whenever the schema is updated.

[linabutler]

@bendk Hmmm, was that comment and close meant for #6107? 😅

[bendk]

Yes it was, I was wondering where it went and why I had to close that issue twice :rofl:

[bendk]

Updated this PR to not hash the URLs for now at least. AFAICT, these are the remaining open questions:

• Is the external API okay (passing a Suggestion object into dismiss_suggestion)?
• pub vs pub(crate)
• hashing only 63-bits to avoid signed integer issues (not currently relevant for this feature, but I figure we might as well check in the hash function in case we want to use it in the future).

@0c0w3 @linabutler @ncloudioj what do you think about the remaining questions?

[bendk]

Per Lina's comment, we'd like to avoid those legacy insecure hashers (e.g. MD5) in Places, shall we follow that here as well?

Good point. I just went ahead and removed that code, since it seems like we don't know exactly how we want to handle hashing in the future.

[linabutler]

Thank you so much for working through all this, @bendk!

I did think of one downside for passing the full Suggestion to dismiss_suggestion that I should've thought of earlier, but this review thread is getting pretty long, and the rest of the plumbing looks good. We can always land this PR as-is, and iterate on the API in a follow-up.

[bendk]

Would it be a bad idea to have a single check-for-dismissals step at the end of fetch_suggestions()? It seems a little onerous to have to add a subquery (or whatever logic) per suggestion type, especially as we start to scale up new types. Doing one more query at the end doesn't seem too bad especially since dismissed_suggestions is keyed on the URL.

This seems like a good idea to me, but do we need to worry about the limits? I'm slightly paranoid that the dismissed suggestions will push non-dismissed suggestions down the list and cause them to be filtered out.

[bendk]

AFAICT, there's no major blockers left. How do others feel about merging this one, then following up on:

• Simplifying the code by removing the sub-queries on each suggestion
• Better system to identify suggestions

Are there any other remaining questions that I'm missing?

[ncloudioj]

Sounds good to me. Go ahead! :shipit:

[bendk]

Follow ups:

• https://bugzilla.mozilla.org/show_bug.cgi?id=1886477
• https://bugzilla.mozilla.org/show_bug.cgi?id=1886484