Skip to content

Commit

Permalink
Bug 1876208 - API for dismissing suggestions
Browse files Browse the repository at this point in the history 
Dismissed suggestion Url hashes are stored in the database and the
suggestions are not returned again in subsequent queries.

This currently works with most providers, but not Yelp or Weather.
  • Loading branch information
@bendk
bendk committed Mar 11, 2024
1 parent 63242e6 commit fc521b1
Show file tree
Hide file tree
Showing 6 changed files with 317 additions and 2 deletions.
21 changes: 21 additions & 0 deletions components/suggest/src/db.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ impl<'a> SuggestDao<'a> {
WHERE
s.provider = :provider
AND k.keyword = :keyword
AND NOT EXISTS (SELECT 1 FROM dismissed_suggestions WHERE url_hash=hash(s.url))
"#,
named_params! {
":keyword": keyword_lowercased,
Expand Down Expand Up @@ -348,6 +349,7 @@ impl<'a> SuggestDao<'a> {
WHERE
s.provider = :provider
AND k.keyword = :keyword
AND NOT EXISTS (SELECT 1 FROM dismissed_suggestions WHERE url_hash=hash(s.url))
"#,
named_params! {
":keyword": keyword_lowercased,
Expand Down Expand Up @@ -427,6 +429,7 @@ impl<'a> SuggestDao<'a> {
k.keyword_prefix = :keyword_prefix
AND (k.keyword_suffix BETWEEN :keyword_suffix AND :keyword_suffix || x'FFFF')
AND s.provider = :provider
AND NOT EXISTS (SELECT 1 FROM dismissed_suggestions WHERE url_hash=hash(s.url))
GROUP BY
s.id
ORDER BY
Expand Down Expand Up @@ -516,6 +519,7 @@ impl<'a> SuggestDao<'a> {
k.keyword_prefix = :keyword_prefix
AND (k.keyword_suffix BETWEEN :keyword_suffix AND :keyword_suffix || x'FFFF')
AND s.provider = :provider
AND NOT EXISTS (SELECT 1 FROM dismissed_suggestions WHERE url_hash=hash(s.url))
GROUP BY
s.id,
k.confidence
Expand Down Expand Up @@ -588,6 +592,7 @@ impl<'a> SuggestDao<'a> {
k.keyword_prefix = :keyword_prefix
AND (k.keyword_suffix BETWEEN :keyword_suffix AND :keyword_suffix || x'FFFF')
AND s.provider = :provider
AND NOT EXISTS (SELECT 1 FROM dismissed_suggestions WHERE url_hash=hash(s.url))
GROUP BY
s.id
ORDER BY
Expand Down Expand Up @@ -1213,6 +1218,22 @@ impl<'a> SuggestDao<'a> {
Ok(())
}

pub fn insert_dismissal(&self, url: &str) -> Result<()> {
self.conn.execute(
"INSERT OR REPLACE INTO dismissed_suggestions(url_hash)
VALUES(hash(:url))",
named_params! {
":url": url,
},
)?;
Ok(())
}

pub fn clear_dismissals(&self) -> Result<()> {
self.conn.execute("DELETE FROM dismissed_suggestions", ())?;
Ok(())
}

/// Deletes all suggestions associated with a Remote Settings record from
/// the database.
pub fn drop_suggestions(&mut self, record_id: &SuggestRecordId) -> Result<()> {
Expand Down
2 changes: 1 addition & 1 deletion components/suggest/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub(crate) type Result<T> = std::result::Result<T, error::Error>;
pub type SuggestApiResult<T> = std::result::Result<T, error::SuggestApiError>;

/// A query for suggestions to show in the address bar.
#[derive(Debug, Default)]
#[derive(Clone, Debug, Default)]
pub struct SuggestionQuery {
pub keyword: String,
pub providers: Vec<SuggestionProvider>,
Expand Down
50 changes: 49 additions & 1 deletion components/suggest/src/schema.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/

use rusqlite::{Connection, Transaction};
use rusqlite::{functions::FunctionFlags, Connection, Transaction};
use sql_support::open_database::{self, ConnectionInitializer};

/// The current database schema version.
Expand Down Expand Up @@ -148,6 +148,7 @@ impl ConnectionInitializer for SuggestConnectionInitializer {
PRAGMA foreign_keys = ON;
";
conn.execute_batch(initial_pragmas)?;
define_functions(conn)?;
sql_support::debug_tools::define_debug_functions(conn)?;

Ok(())
Expand Down Expand Up @@ -189,6 +190,53 @@ pub fn clear_database(db: &Connection) -> rusqlite::Result<()> {
)
}

fn define_functions(c: &Connection) -> rusqlite::Result<()> {
c.create_scalar_function(
"hash",
1,
FunctionFlags::SQLITE_UTF8 | FunctionFlags::SQLITE_DETERMINISTIC,
sql_fns::hash,
)?;
Ok(())
}

mod sql_fns {
use rusqlite::{functions::Context, types::ValueRef, Error, Result};
use std::{collections::hash_map::DefaultHasher, hash::Hasher};

#[inline(never)]
pub fn hash(ctx: &Context<'_>) -> Result<Option<i64>> {
match ctx.len() {
1 => {
let raw = ctx.get_raw(0);
// This is a deterministic function, which means sqlite
// does certain optimizations which means hash() may be called
// with a null value even though the query prevents the null
// value from actually being used. As a special case, we return
// null when the input is NULL. We return NULL instead of zero
// because the hash columns are NOT NULL, so attempting to
// actually use the null should fail.
if raw == ValueRef::Null {
return Ok(None);
}
match raw.as_str() {
Ok(s) => {
let mut hasher = DefaultHasher::new();
hasher.write(s.as_bytes());
Ok(Some(hasher.finish() as i64))
}
Err(e) => Err(Error::UserFunctionError(
format!("Bad arg to 'hash': {e}").into(),
)),
}
}
n => Err(Error::UserFunctionError(
format!("`hash` expects 1 argument, got {}.", n).into(),
)),
}
}
}

#[cfg(test)]
mod test {
use super::*;
Expand Down
226 changes: 226 additions & 0 deletions components/suggest/src/store.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,20 @@ impl SuggestStore {
self.inner.query(query)
}

/// Dismiss a suggestion
///
/// Dismissed suggestions will not be returned again
#[handle_error(Error)]
pub fn dismiss_suggestion(&self, suggestion: Suggestion) -> SuggestApiResult<()> {
self.inner.dismiss_suggestion(suggestion)
}

/// Clear dismissed suggestions
#[handle_error(Error)]
pub fn clear_dismissed_suggestions(&self) -> SuggestApiResult<()> {
self.inner.clear_dismissed_suggestions()
}

/// Interrupts any ongoing queries.
///
/// This should be called when the user types new input into the address
Expand Down Expand Up @@ -275,6 +289,18 @@ impl<S> SuggestStoreInner<S> {
self.dbs()?.reader.read(|dao| dao.fetch_suggestions(&query))
}

fn dismiss_suggestion(&self, suggestion: Suggestion) -> Result<()> {
if let Some(url) = suggestion.url() {
self.dbs()?.writer.write(|dao| dao.insert_dismissal(url))?;
}
Ok(())
}

fn clear_dismissed_suggestions(&self) -> Result<()> {
self.dbs()?.writer.write(|dao| dao.clear_dismissals())?;
Ok(())
}

fn interrupt(&self) {
if let Some(dbs) = self.dbs.get() {
// Only interrupt if the databases are already open.
Expand Down Expand Up @@ -5956,4 +5982,204 @@ mod tests {

Ok(())
}

#[test]
fn remove_dismissed_suggestions() -> anyhow::Result<()> {
before_each();

let snapshot = Snapshot::with_records(json!([{
"id": "data-1",
"type": "data",
"last_modified": 15,
"attachment": {
"filename": "data-1.json",
"mimetype": "application/json",
"location": "data-1.json",
"hash": "",
"size": 0,
},

}, {
"id": "data-2",
"type": "amo-suggestions",
"last_modified": 15,
"attachment": {
"filename": "data-2.json",
"mimetype": "application/json",
"location": "data-2.json",
"hash": "",
"size": 0,
},
}, {
"id": "data-3",
"type": "pocket-suggestions",
"last_modified": 15,
"attachment": {
"filename": "data-3.json",
"mimetype": "application/json",
"location": "data-3.json",
"hash": "",
"size": 0,
},
}, {
"id": "data-5",
"type": "mdn-suggestions",
"last_modified": 15,
"attachment": {
"filename": "data-5.json",
"mimetype": "application/json",
"location": "data-5.json",
"hash": "",
"size": 0,
},
}, {
"id": "data-6",
"type": "amp-mobile-suggestions",
"last_modified": 15,
"attachment": {
"filename": "data-6.json",
"mimetype": "application/json",
"location": "data-6.json",
"hash": "",
"size": 0,
},
}, {
"id": "icon-2",
"type": "icon",
"last_modified": 20,
"attachment": {
"filename": "icon-2.png",
"mimetype": "image/png",
"location": "icon-2.png",
"hash": "",
"size": 0,
},
}, {
"id": "icon-3",
"type": "icon",
"last_modified": 25,
"attachment": {
"filename": "icon-3.png",
"mimetype": "image/png",
"location": "icon-3.png",
"hash": "",
"size": 0,
},
}]))?
.with_data(
"data-1.json",
json!([{
"id": 0,
"advertiser": "Good Place Eats",
"iab_category": "8 - Food & Drink",
"keywords": ["cats"],
"title": "Lasagna Come Out Tomorrow",
"url": "https://www.lasagna.restaurant",
"icon": "2",
"impression_url": "https://example.com/impression_url",
"click_url": "https://example.com/click_url",
"score": 0.31
}, {
"id": 0,
"advertiser": "Wikipedia",
"iab_category": "5 - Education",
"keywords": ["cats"],
"title": "California",
"url": "https://wikipedia.org/California",
"icon": "3"
}]),
)?
.with_data(
"data-2.json",
json!([
{
"description": "amo suggestion",
"url": "https://addons.mozilla.org/en-US/firefox/addon/example",
"guid": "{b9db16a4-6edc-47ec-a1f4-b86292ed211d}",
"keywords": ["cats"],
"title": "Firefox Relay",
"icon": "https://addons.mozilla.org/user-media/addon_icons/2633/2633704-64.png?modified=2c11a80b",
"rating": "4.9",
"number_of_ratings": 888,
"score": 0.32
},
]),
)?
.with_data(
"data-3.json",
json!([
{
"description": "pocket suggestion",
"url": "https://getpocket.com/collections/its-not-just-burnout-how-grind-culture-failed-women",
"lowConfidenceKeywords": [],
"highConfidenceKeywords": ["cats"],
"title": "‘It’s Not Just Burnout:’ How Grind Culture Fails Women",
"score": 0.33
},
]),
)?
.with_data(
"data-5.json",
json!([
{
"description": "Javascript Array",
"url": "https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array",
"keywords": ["cats"],
"title": "Array",
"score": 0.24
},
]),
)?
.with_data(
"data-6.json",
json!([
{
"id": 0,
"advertiser": "Good Place Eats",
"iab_category": "8 - Food & Drink",
"keywords": ["cats"],
"title": "Mobile - Lasagna Come Out Tomorrow",
"url": "https://www.lasagna.restaurant",
"icon": "3",
"impression_url": "https://example.com/impression_url",
"click_url": "https://example.com/click_url",
"score": 0.26
}
]),
)?
.with_icon("icon-2.png", "i-am-an-icon".as_bytes().into())
.with_icon("icon-3.png", "also-an-icon".as_bytes().into());

let store = unique_test_store(SnapshotSettingsClient::with_snapshot(snapshot));
store.ingest(SuggestIngestionConstraints::default())?;

// A query for cats should return all suggestions
let query = SuggestionQuery {
keyword: "cats".into(),
providers: vec![
SuggestionProvider::Amp,
SuggestionProvider::Wikipedia,
SuggestionProvider::Amo,
SuggestionProvider::Pocket,
SuggestionProvider::Mdn,
SuggestionProvider::AmpMobile,
],
limit: None,
};
let results = store.query(query.clone())?;
assert_eq!(results.len(), 6);

for result in results {
store.dismiss_suggestion(result)?;
}

// After dismissing the suggestions, the next query shouldn't return them
assert_eq!(store.query(query.clone())?.len(), 0);

// Clearing the dismissals should cause them to be returned again
store.clear_dismissed_suggestions()?;
assert_eq!(store.query(query.clone())?.len(), 6);

Ok(())
}
}
Loading

0 comments on commit fc521b1

Please sign in to comment.