Bug 1907577 - Fakespot icon support

Fetch the icons using a separate query than manually joined them with
the suggestion data.  This has the advantage of not needing to update
the schema.

Also updated the suggest CLI a bit so that I could use it to test that
the code was working in practice.

CHANGELOG.md

@@ -1,5 +1,8 @@
 # v130.0 (In progress)
 
+### Suggest
+- Added support for Fakespot suggestions.
+
 [Full Changelog](In progress)
 
 # v129.0 (_2024-07-08_)

components/suggest/src/db.rs

@@ -690,7 +690,9 @@ impl<'a> SuggestDao<'a> {
                 f.fakespot_grade,
                 f.product_id,
                 f.rating,
-                f.total_reviews
+                f.total_reviews,
+                i.data,
+                i.mimetype
             FROM
                 suggestions s
             JOIN
@@ -699,6 +701,9 @@ impl<'a> SuggestDao<'a> {
             JOIN
                 fakespot_custom_details f
                 ON f.suggestion_id = s.id
+            LEFT JOIN
+                icons i
+                ON i.id = f.icon_id
             WHERE
                 fakespot_fts MATCH ?
             ORDER BY
@@ -714,6 +719,8 @@ impl<'a> SuggestDao<'a> {
                     product_id: row.get(4)?,
                     rating: row.get(5)?,
                     total_reviews: row.get(6)?,
+                    icon: row.get(7)?,
+                    icon_mimetype: row.get(8)?,
                 })
             },
         )
@@ -1385,9 +1392,10 @@ impl<'conn> FakespotInsertStatement<'conn> {
                  fakespot_grade,
                  product_id,
                  rating,
-                 total_reviews
+                 total_reviews,
+                 icon_id
              )
-             VALUES(?, ?, ?, ?, ?)
+             VALUES(?, ?, ?, ?, ?, ?)
              ",
         )?))
     }
@@ -1397,13 +1405,18 @@ impl<'conn> FakespotInsertStatement<'conn> {
         suggestion_id: i64,
         fakespot: &DownloadedFakespotSuggestion,
     ) -> Result<()> {
+        let icon_id = fakespot
+            .product_id
+            .split_once('-')
+            .map(|(vendor, _)| format!("fakespot-{vendor}"));
         self.0
             .execute((
                 suggestion_id,
                 &fakespot.fakespot_grade,
                 &fakespot.product_id,
                 fakespot.rating,
                 fakespot.total_reviews,
+                icon_id,
             ))
             .with_context("fakespot insert")?;
         Ok(())

components/suggest/src/schema.rs

@@ -19,7 +19,7 @@ use sql_support::{
 ///     [`SuggestConnectionInitializer::upgrade_from`].
 ///    a. If suggestions should be re-ingested after the migration, call `clear_database()` inside
 ///       the migration.
-pub const VERSION: u32 = 21;
+pub const VERSION: u32 = 22;
 
 /// The current Suggest database schema.
 pub const SQL: &str = "
@@ -95,6 +95,7 @@ CREATE TABLE fakespot_custom_details(
     product_id TEXT NOT NULL,
     rating REAL NOT NULL,
     total_reviews INTEGER NOT NULL,
+    icon_id TEXT,
     FOREIGN KEY(suggestion_id) REFERENCES suggestions(id) ON DELETE CASCADE
 );
 
@@ -321,6 +322,30 @@ END;
                 )?;
                 Ok(())
             }
+            21 => {
+                // Drop and re-create the fakespot_custom_details to add the icon_id column.
+                tx.execute_batch(
+                    "
+DROP TABLE fakespot_custom_details;
+CREATE TABLE fakespot_custom_details(
+    suggestion_id INTEGER PRIMARY KEY,
+    fakespot_grade TEXT NOT NULL,
+    product_id TEXT NOT NULL,
+    rating REAL NOT NULL,
+    total_reviews INTEGER NOT NULL,
+    icon_id TEXT,
+    FOREIGN KEY(suggestion_id) REFERENCES suggestions(id) ON DELETE CASCADE
+);
+CREATE TRIGGER fakespot_ai AFTER INSERT ON fakespot_custom_details BEGIN
+  INSERT INTO fakespot_fts(rowid, title)
+    SELECT id, title
+    FROM suggestions
+    WHERE id = new.suggestion_id;
+END;
+                    ",
+                )?;
+                Ok(())
+            }
             _ => Err(open_database::Error::IncompatibleVersion(version)),
         }
     }

components/suggest/src/store.rs

@@ -2129,11 +2129,15 @@ mod tests {
     fn query_fakespot() -> anyhow::Result<()> {
         before_each();
 
-        let store = TestStore::new(MockRemoteSettingsClient::default().with_record(
-            "fakespot-suggestions",
-            "fakespot-1",
-            json!([snowglobe_fakespot(), simpsons_fakespot()]),
-        ));
+        let store = TestStore::new(
+            MockRemoteSettingsClient::default()
+                .with_record(
+                    "fakespot-suggestions",
+                    "fakespot-1",
+                    json!([snowglobe_fakespot(), simpsons_fakespot()]),
+                )
+                .with_icon(fakespot_amazon_icon()),
+        );
         store.ingest(SuggestIngestionConstraints::all_providers());
         assert_eq!(
             store.fetch_suggestions(SuggestionQuery::fakespot("globe")),
@@ -2165,11 +2169,15 @@ mod tests {
     fn fakespot_prefix_matching() -> anyhow::Result<()> {
         before_each();
 
-        let store = TestStore::new(MockRemoteSettingsClient::default().with_record(
-            "fakespot-suggestions",
-            "fakespot-1",
-            json!([snowglobe_fakespot(), simpsons_fakespot()]),
-        ));
+        let store = TestStore::new(
+            MockRemoteSettingsClient::default()
+                .with_record(
+                    "fakespot-suggestions",
+                    "fakespot-1",
+                    json!([snowglobe_fakespot(), simpsons_fakespot()]),
+                )
+                .with_icon(fakespot_amazon_icon()),
+        );
         store.ingest(SuggestIngestionConstraints::all_providers());
         assert_eq!(
             store.fetch_suggestions(SuggestionQuery::fakespot("simp")),
@@ -2191,23 +2199,30 @@ mod tests {
     fn fakespot_updates_and_deletes() -> anyhow::Result<()> {
         before_each();
 
-        let mut store = TestStore::new(MockRemoteSettingsClient::default().with_record(
-            "fakespot-suggestions",
-            "fakespot-1",
-            json!([snowglobe_fakespot(), simpsons_fakespot()]),
-        ));
+        let mut store = TestStore::new(
+            MockRemoteSettingsClient::default()
+                .with_record(
+                    "fakespot-suggestions",
+                    "fakespot-1",
+                    json!([snowglobe_fakespot(), simpsons_fakespot()]),
+                )
+                .with_icon(fakespot_amazon_icon()),
+        );
         store.ingest(SuggestIngestionConstraints::all_providers());
 
         // Update the snapshot so that:
         //   - The Simpsons entry is deleted
         //   - Snow globes now use sea glass instead of glitter
-        store.replace_client(MockRemoteSettingsClient::default().with_record(
-            "fakespot-suggestions",
-            "fakespot-1",
-            json!([
-                snowglobe_fakespot().merge(json!({"title": "Make Your Own Sea Glass Snow Globes"}))
-            ]),
-        ));
+        store.replace_client(
+            MockRemoteSettingsClient::default()
+                .with_record(
+                    "fakespot-suggestions",
+                    "fakespot-1",
+                    json!([snowglobe_fakespot()
+                        .merge(json!({"title": "Make Your Own Sea Glass Snow Globes"}))]),
+                )
+                .with_icon(fakespot_amazon_icon()),
+        );
         store.ingest(SuggestIngestionConstraints::all_providers());
 
         assert_eq!(

components/suggest/src/suggest.udl

@@ -103,6 +103,8 @@ interface Suggestion {
         string title,
         i64 total_reviews,
         string url,
+        sequence<u8>? icon,
+        string? icon_mimetype,
         f64 score
     );
 };

components/suggest/src/suggestion.rs

@@ -88,6 +88,8 @@ pub enum Suggestion {
         title: String,
         total_reviews: i64,
         url: String,
+        icon: Option<Vec<u8>>,
+        icon_mimetype: Option<String>,
         score: f64,
     },
 }
@@ -127,7 +129,8 @@ impl Suggestion {
             | Self::Wikipedia { url, .. }
             | Self::Amo { url, .. }
             | Self::Yelp { url, .. }
-            | Self::Mdn { url, .. } => Some(url),
+            | Self::Mdn { url, .. }
+            | Self::Fakespot { url, .. } => Some(url),
             _ => None,
         }
     }
@@ -160,6 +163,16 @@ impl Suggestion {
             Self::Fakespot { title, .. } => title,
         }
     }
+
+    pub fn icon_data(&self) -> Option<&[u8]> {
+        match self {
+            Self::Amp { icon, .. }
+            | Self::Wikipedia { icon, .. }
+            | Self::Yelp { icon, .. }
+            | Self::Fakespot { icon, .. } => icon.as_deref(),
+            _ => None,
+        }
+    }
 }
 
 impl Eq for Suggestion {}

components/suggest/src/testing/data.rs

@@ -473,29 +473,43 @@ pub fn snowglobe_suggestion() -> Suggestion {
         total_reviews: 152,
         url: "http://amazon.com/dp/ABC".into(),
         score: 0.7,
+        icon: Some("fakespot-icon-amazon-data".as_bytes().to_vec()),
+        icon_mimetype: Some("image/png".into()),
     }
 }
 
 pub fn simpsons_fakespot() -> JsonValue {
     json!({
         "fakespot_grade": "A",
-        "product_id": "amazon-XYZ",
+        // Use a product ID that doesn't match the ingested icons to test what happens.  In this
+        // case, icon and icon_mimetype for the returned Suggestion should both be None.
+        "product_id": "vendorwithouticon-XYZ",
         "rating": 4.9,
         "score": 0.9,
         "title": "The Simpsons: Skinner's Sense of Snow (DVD)",
         "total_reviews": 14000,
-        "url": "http://amazon.com/dp/XYZ"
+        "url": "http://vendorwithouticon.com/dp/XYZ"
     })
 }
 
 pub fn simpsons_suggestion() -> Suggestion {
     Suggestion::Fakespot {
         fakespot_grade: "A".into(),
-        product_id: "amazon-XYZ".into(),
+        product_id: "vendorwithouticon-XYZ".into(),
         rating: 4.9,
         title: "The Simpsons: Skinner's Sense of Snow (DVD)".into(),
         total_reviews: 14000,
-        url: "http://amazon.com/dp/XYZ".into(),
+        url: "http://vendorwithouticon.com/dp/XYZ".into(),
         score: 0.9,
+        icon: None,
+        icon_mimetype: None,
+    }
+}
+
+pub fn fakespot_amazon_icon() -> MockIcon {
+    MockIcon {
+        id: "fakespot-amazon",
+        data: "fakespot-icon-amazon-data",
+        mimetype: "image/png",
     }
 }

examples/suggest-cli/src/main.rs

@@ -107,7 +107,7 @@ fn build_store(cli: &Cli) -> Arc<SuggestStore> {
 fn ingest(store: &SuggestStore) {
     println!("Ingesting data...");
     store
-        .ingest(SuggestIngestionConstraints::default())
+        .ingest(SuggestIngestionConstraints::all_providers())
         .unwrap_or_else(|e| panic!("Error in ingest: {e}"));
     println!("Done");
 }
@@ -127,11 +127,13 @@ fn query(store: &SuggestStore, provider: SuggestionProviderArg, input: String) {
         println!("Results:");
         for suggestion in suggestions {
             let title = suggestion.title();
-            if let Some(url) = suggestion.url() {
-                println!("{title} ({url})");
+            let url = suggestion.url().unwrap_or("[no-url]");
+            let icon = if suggestion.icon_data().is_some() {
+                "with icon"
             } else {
-                println!("{title}");
-            }
+                "no icon"
+            };
+            println!("{title} ({url}) ({icon})");
         }
     }
 }