Bug 1983587 - Namespace dismissals of dynamic suggestions by suggestion_type (#6902)

* Bug 1983587 - Namespace dismissals of dynamic suggestions by suggestion_type

* address comments

Author: moritzbeier

CHANGELOG.md

@@ -25,6 +25,10 @@
 - Added `SearchEngineUrl::exclude_partner_code_from_telemetry` ([bug 1980474](https://bugzilla.mozilla.org/show_bug.cgi?id=1980474))
 - Added `SearchEngineUrl::accepted_content_types`
 
+### Suggest
+
+- Added namespacing by `suggestion_type` to dismissals of dynamic suggestions ([bug 1983587](https://bugzilla.mozilla.org/show_bug.cgi?id=1983587))
+
 ### RC Crypto
 - Fix NSS bindings for key management
 

components/suggest/src/db.rs

@@ -517,7 +517,7 @@ impl<'a> SuggestDao<'a> {
 
     fn fetch_categories_for_suggestion(&self, suggestion_id: i64) -> Result<Vec<i32>> {
         let mut category_stmt = self.conn.prepare(
-            r#" 
+            r#"
             SELECT category FROM serp_categories WHERE suggestion_id = :suggestion_id
             "#,
         )?;
@@ -905,7 +905,10 @@ impl<'a> SuggestDao<'a> {
                   s.provider = ?
                   AND k.keyword = ?
                   AND d.suggestion_type IN ({})
-                  AND NOT EXISTS (SELECT 1 FROM dismissed_suggestions WHERE url = s.url)
+                  AND NOT EXISTS (
+                    SELECT 1 FROM dismissed_dynamic_suggestions 
+                    WHERE dismissal_key = s.url AND suggestion_type = d.suggestion_type
+                  )
                 ORDER BY
                   s.score ASC, d.suggestion_type ASC, s.id ASC
                 "#,
@@ -1205,8 +1208,23 @@ impl<'a> SuggestDao<'a> {
         Ok(())
     }
 
+    pub fn insert_dynamic_dismissal(&self, suggestion_type: &str, key: &str) -> Result<()> {
+        self.conn.execute(
+            "INSERT OR IGNORE INTO dismissed_dynamic_suggestions(suggestion_type, dismissal_key)
+             VALUES(:suggestion_type, :dismissal_key)",
+            named_params! {
+                ":suggestion_type": suggestion_type,
+                ":dismissal_key": key,
+            },
+        )?;
+        Ok(())
+    }
+
     pub fn clear_dismissals(&self) -> Result<()> {
-        self.conn.execute("DELETE FROM dismissed_suggestions", ())?;
+        self.conn.execute_batch(
+            "DELETE FROM dismissed_suggestions;
+             DELETE FROM dismissed_dynamic_suggestions;",
+        )?;
         Ok(())
     }
 
@@ -1219,10 +1237,27 @@ impl<'a> SuggestDao<'a> {
         )?)
     }
 
+    pub fn has_dynamic_dismissal(&self, suggestion_type: &str, key: &str) -> Result<bool> {
+        Ok(self.conn.exists(
+            "SELECT 1
+             FROM dismissed_dynamic_suggestions
+             WHERE suggestion_type = :suggestion_type AND dismissal_key = :dismissal_key",
+            named_params! {
+                ":suggestion_type": suggestion_type,
+                ":dismissal_key": key,
+            },
+        )?)
+    }
+
     pub fn any_dismissals(&self) -> Result<bool> {
-        Ok(self
-            .conn
-            .exists("SELECT 1 FROM dismissed_suggestions LIMIT 1", ())?)
+        Ok(self.conn.query_row(
+            "SELECT
+             EXISTS(SELECT 1 FROM dismissed_suggestions)
+               OR
+             EXISTS(SELECT 1 FROM dismissed_dynamic_suggestions)",
+            (),
+            |row| row.get(0),
+        )?)
     }
 
     /// Deletes all suggestions associated with a Remote Settings record from

components/suggest/src/schema.rs

@@ -23,7 +23,7 @@ use sql_support::{
 ///     `clear_database()` by adding their names to `conditional_tables`, unless
 ///     they are cleared via a deletion trigger or there's some other good
 ///     reason not to do so.
-pub const VERSION: u32 = 43;
+pub const VERSION: u32 = 44;
 
 /// The current Suggest database schema.
 pub const SQL: &str = "
@@ -260,6 +260,12 @@ CREATE TABLE geonames_metrics(
 CREATE TABLE dismissed_suggestions (
     url TEXT PRIMARY KEY
 ) WITHOUT ROWID;
+
+CREATE TABLE dismissed_dynamic_suggestions (
+    suggestion_type TEXT,
+    dismissal_key TEXT NOT NULL,
+    PRIMARY KEY(suggestion_type, dismissal_key)
+) WITHOUT ROWID;
 ";
 
 /// Initializes an SQLite connection to the Suggest database, performing
@@ -825,6 +831,22 @@ impl ConnectionInitializer for SuggestConnectionInitializer<'_> {
                 )?;
                 Ok(())
             }
+            43 => {
+                clear_database(tx)?;
+                tx.execute_batch(
+                    r#"
+                    CREATE TABLE dismissed_dynamic_suggestions (
+                        suggestion_type TEXT,
+                        dismissal_key TEXT NOT NULL,
+                        PRIMARY KEY(suggestion_type, dismissal_key)
+                    ) WITHOUT ROWID;
+
+                    INSERT INTO dismissed_dynamic_suggestions
+                    SELECT "vpn", url FROM dismissed_suggestions WHERE url = "vpn-suggestions";
+                    "#,
+                )?;
+                Ok(())
+            }
 
             _ => Err(open_database::Error::IncompatibleVersion(version)),
         }
@@ -1053,4 +1075,39 @@ PRAGMA user_version=16;
 
         Ok(())
     }
+
+    /// Test if dynamic vpn suggestions are migrated into the
+    /// dismissed_dynamic_suggestions table during the migration to schema 44.
+    #[test]
+    fn test_migrate_vpn_dismissal() -> anyhow::Result<()> {
+        let db_file =
+            MigratedDatabaseFile::new(SuggestConnectionInitializer::default(), V16_SCHEMA);
+
+        // Upgrade to v43 (just before the migration) and insert the suggestion that should be migrated.
+        db_file.upgrade_to(43);
+        let conn = db_file.open();
+        conn.execute(
+            "INSERT INTO dismissed_suggestions VALUES (?)",
+            ("vpn-suggestions",),
+        )?;
+        conn.close().expect("Connection should be closed");
+        // Finish upgrading to the current version.
+        db_file.upgrade_to(VERSION);
+        db_file.assert_schema_matches_new_database();
+
+        // Check if the vpn suggestion was migrated.
+        let conn = db_file.open();
+        let mut stmt = conn.prepare("SELECT * FROM dismissed_dynamic_suggestions")?;
+        let mut rows = stmt.query([])?;
+        let row = rows.next()?.expect("Should have one row");
+        let suggestion_type: String = row.get(0)?;
+        assert_eq!(suggestion_type, "vpn", "Has correct suggestion_type");
+        let dismissal_key: String = row.get(1)?;
+        assert_eq!(
+            dismissal_key, "vpn-suggestions",
+            "Has correct suggestion_type"
+        );
+        assert!(rows.next()?.is_none(), "Should not have other rows");
+        Ok(())
+    }
 }

components/suggest/src/store.rs

@@ -474,7 +474,15 @@ impl<S> SuggestStoreInner<S> {
 
     fn dismiss_by_suggestion(&self, suggestion: &Suggestion) -> Result<()> {
         if let Some(key) = suggestion.dismissal_key() {
-            self.dismiss_by_key(key)?;
+            match suggestion {
+                Suggestion::Dynamic {
+                    suggestion_type, ..
+                } => self
+                    .dbs()?
+                    .writer
+                    .write(|dao| dao.insert_dynamic_dismissal(suggestion_type, key))?,
+                _ => self.dismiss_by_key(key)?,
+            }
         }
         Ok(())
     }
@@ -496,7 +504,15 @@ impl<S> SuggestStoreInner<S> {
 
     fn is_dismissed_by_suggestion(&self, suggestion: &Suggestion) -> Result<bool> {
         if let Some(key) = suggestion.dismissal_key() {
-            self.dbs()?.reader.read(|dao| dao.has_dismissal(key))
+            match suggestion {
+                Suggestion::Dynamic {
+                    suggestion_type, ..
+                } => self
+                    .dbs()?
+                    .reader
+                    .read(|dao| dao.has_dynamic_dismissal(suggestion_type, key)),
+                _ => self.dbs()?.reader.read(|dao| dao.has_dismissal(key)),
+            }
         } else {
             Ok(false)
         }
@@ -3826,40 +3842,58 @@ pub(crate) mod tests {
     fn dynamic_dismissal() -> anyhow::Result<()> {
         before_each();
 
-        let store = TestStore::new(MockRemoteSettingsClient::default().with_record(
-            SuggestionProvider::Dynamic.full_record(
-                "dynamic-0",
-                Some(json!({
-                    "suggestion_type": "aaa",
-                })),
-                Some(MockAttachment::Json(json!([
-                    {
-                        "keywords": ["aaa"],
-                        "dismissal_key": "dk0",
-                    },
-                    {
-                        "keywords": ["aaa"],
-                        "dismissal_key": "dk1",
-                    },
-                    {
-                        "keywords": ["aaa"],
-                    },
-                ]))),
-            ),
-        ));
+        let store = TestStore::new(
+            MockRemoteSettingsClient::default()
+                .with_record(SuggestionProvider::Dynamic.full_record(
+                    "dynamic-0",
+                    Some(json!({
+                        "suggestion_type": "aaa",
+                    })),
+                    Some(MockAttachment::Json(json!([
+                        {
+                            "keywords": ["aaa"],
+                            "dismissal_key": "dk0",
+                        },
+                        {
+                            "keywords": ["aaa"],
+                            "dismissal_key": "dk1",
+                        },
+                        {
+                            "keywords": ["aaa"],
+                        },
+                    ]))),
+                ))
+                .with_record(SuggestionProvider::Dynamic.full_record(
+                    "dynamic-1",
+                    Some(json!({
+                        "suggestion_type": "bbb",
+                    })),
+                    Some(MockAttachment::Json(json!([
+                        {
+                            "keywords": ["bbb"],
+                            "dismissal_key": "dk0",
+                        },
+                    ]))),
+                )),
+        );
+
         store.ingest(SuggestIngestionConstraints {
             providers: Some(vec![SuggestionProvider::Dynamic]),
             provider_constraints: Some(SuggestionProviderConstraints {
-                dynamic_suggestion_types: Some(vec!["aaa".to_string()]),
+                dynamic_suggestion_types: Some(vec!["aaa".to_string(), "bbb".to_string()]),
                 ..SuggestionProviderConstraints::default()
             }),
             ..SuggestIngestionConstraints::all_providers()
         });
 
         // Make sure the suggestions are initially fetchable.
-        let suggestions = store.fetch_suggestions(SuggestionQuery::dynamic("aaa", &["aaa"]));
+        assert!(!store.inner.any_dismissed_suggestions()?);
+        let suggestions_0: Vec<Suggestion> =
+            store.fetch_suggestions(SuggestionQuery::dynamic("aaa", &["aaa"]));
+        let suggestions_1: Vec<Suggestion> =
+            store.fetch_suggestions(SuggestionQuery::dynamic("bbb", &["bbb"]));
         assert_eq!(
-            suggestions,
+            suggestions_0,
             vec![
                 Suggestion::Dynamic {
                     suggestion_type: "aaa".to_string(),
@@ -3883,8 +3917,11 @@ pub(crate) mod tests {
         );
 
         // Dismiss the first suggestion.
-        assert_eq!(suggestions[0].dismissal_key(), Some("dk0"));
-        store.inner.dismiss_by_suggestion(&suggestions[0])?;
+        assert_eq!(suggestions_0[0].dismissal_key(), Some("dk0"));
+        store.inner.dismiss_by_suggestion(&suggestions_0[0])?;
+
+        assert!(store.inner.any_dismissed_suggestions()?);
+        assert!(store.inner.is_dismissed_by_suggestion(&suggestions_0[0])?);
         assert_eq!(
             store.fetch_suggestions(SuggestionQuery::dynamic("aaa", &["aaa"])),
             vec![
@@ -3904,8 +3941,10 @@ pub(crate) mod tests {
         );
 
         // Dismiss the second suggestion.
-        assert_eq!(suggestions[1].dismissal_key(), Some("dk1"));
-        store.inner.dismiss_by_suggestion(&suggestions[1])?;
+        assert_eq!(suggestions_0[1].dismissal_key(), Some("dk1"));
+        store.inner.dismiss_by_suggestion(&suggestions_0[1])?;
+
+        assert!(store.inner.is_dismissed_by_suggestion(&suggestions_0[1])?);
         assert_eq!(
             store.fetch_suggestions(SuggestionQuery::dynamic("aaa", &["aaa"])),
             vec![Suggestion::Dynamic {
@@ -3916,6 +3955,23 @@ pub(crate) mod tests {
             },],
         );
 
+        // Make sure the bbb suggestion hasn't been dismissed even though it
+        // has the same key as the first aaa suggestion.
+        assert_eq!(
+            suggestions_1[0].dismissal_key(),
+            suggestions_0[0].dismissal_key()
+        );
+        assert!(!store.inner.is_dismissed_by_suggestion(&suggestions_1[0])?);
+        assert_eq!(
+            store.fetch_suggestions(SuggestionQuery::dynamic("bbb", &["bbb"])),
+            vec![Suggestion::Dynamic {
+                suggestion_type: "bbb".to_string(),
+                data: None,
+                dismissal_key: Some("dk0".to_string()),
+                score: DEFAULT_SUGGESTION_SCORE,
+            },],
+        );
+
         // Clear dismissals. All suggestions should be fetchable again.
         store.inner.clear_dismissed_suggestions()?;
         assert_eq!(