diff --git a/components/suggest/src/db.rs b/components/suggest/src/db.rs index d697a4da1a..9ba2db18e2 100644 --- a/components/suggest/src/db.rs +++ b/components/suggest/src/db.rs @@ -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, @@ -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, @@ -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 @@ -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 @@ -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 @@ -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<()> { diff --git a/components/suggest/src/lib.rs b/components/suggest/src/lib.rs index 23775b7dec..54979b0eb9 100644 --- a/components/suggest/src/lib.rs +++ b/components/suggest/src/lib.rs @@ -26,7 +26,7 @@ pub(crate) type Result = std::result::Result; pub type SuggestApiResult = std::result::Result; /// 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, diff --git a/components/suggest/src/schema.rs b/components/suggest/src/schema.rs index 5a6d400fe9..94277ecf9c 100644 --- a/components/suggest/src/schema.rs +++ b/components/suggest/src/schema.rs @@ -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. @@ -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(()) @@ -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> { + 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::*; diff --git a/components/suggest/src/store.rs b/components/suggest/src/store.rs index 4c925e5482..58239a12c3 100644 --- a/components/suggest/src/store.rs +++ b/components/suggest/src/store.rs @@ -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 @@ -275,6 +289,18 @@ impl SuggestStoreInner { 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. @@ -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(()) + } } diff --git a/components/suggest/src/suggest.udl b/components/suggest/src/suggest.udl index 133cc56f14..1ed8898eb9 100644 --- a/components/suggest/src/suggest.udl +++ b/components/suggest/src/suggest.udl @@ -123,6 +123,12 @@ interface SuggestStore { [Throws=SuggestApiError] sequence query(SuggestionQuery query); + [Throws=SuggestApiError] + void dismiss_suggestion(Suggestion suggestion); + + [Throws=SuggestApiError] + void clear_dismissed_suggestions(); + void interrupt(); [Throws=SuggestApiError] diff --git a/components/suggest/src/suggestion.rs b/components/suggest/src/suggestion.rs index 02a169df15..0f335bf4c8 100644 --- a/components/suggest/src/suggestion.rs +++ b/components/suggest/src/suggestion.rs @@ -109,6 +109,20 @@ impl Ord for Suggestion { } } +impl Suggestion { + pub fn url(&self) -> Option<&str> { + match self { + Self::Amp { url, .. } + | Self::Pocket { url, .. } + | Self::Wikipedia { url, .. } + | Self::Amo { url, .. } + | Self::Yelp { url, .. } + | Self::Mdn { url, .. } => Some(url), + _ => None, + } + } +} + impl Eq for Suggestion {} /// Replaces all template parameters in a "raw" sponsored suggestion URL, /// producing a "cooked" URL with real values.