[DISCO-3212] Switch Relevancy to new remote settings API

A side effect is the relevancy constructor is now infallible.

Author: bendk

CHANGELOG.md

@@ -39,6 +39,7 @@ text, the list view has been slimmed down and a detail view has been added.
 
 ### Remote Settings
 - Several methods `RemoteSettingsService` are now infallible, which is a breaking change for Swift code.
+    - `RelevancyStore` constructor
     - `RemoteSettingsService` constructor
     - `RemoteSettingsService::make_client()`
     - `SearchEngineSelector::use_remote_settings_server()`

Cargo.lock

@@ -42,11 +42,8 @@ fn fetch_interest_data_inner<C: RelevancyRemoteSettingsClient>(
     let remote_settings_response = client.get_records()?;
     let mut result = vec![];
 
-    for record in remote_settings_response.records {
-        let attachment_data = match &record.attachment {
-            None => return Err(Error::FetchInterestDataError),
-            Some(a) => client.get_attachment(&a.location)?,
-        };
+    for record in remote_settings_response {
+        let attachment_data = client.get_attachment(&record)?;
         let interest = get_interest(&record)?;
         let urls = get_hash_urls(attachment_data)?;
         result.extend(std::iter::repeat(interest).zip(urls));
@@ -98,7 +95,6 @@ mod test {
     use std::{cell::RefCell, collections::HashMap};
 
     use anyhow::Context;
-    use remote_settings::RemoteSettingsResponse;
     use serde_json::json;
 
     use super::*;
@@ -160,20 +156,12 @@ mod test {
     }
 
     impl RelevancyRemoteSettingsClient for SnapshotSettingsClient {
-        fn get_records(&self) -> Result<RemoteSettingsResponse> {
-            let records = self.snapshot.borrow().records.clone();
-            let last_modified = records
-                .iter()
-                .map(|record: &RemoteSettingsRecord| record.last_modified)
-                .max()
-                .unwrap_or(0);
-            Ok(RemoteSettingsResponse {
-                records,
-                last_modified,
-            })
+        fn get_records(&self) -> Result<Vec<RemoteSettingsRecord>> {
+            Ok(self.snapshot.borrow().records.clone())
         }
 
-        fn get_attachment(&self, location: &str) -> Result<Vec<u8>> {
+        fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result<Vec<u8>> {
+            let location = record.attachment.as_ref().unwrap().location.as_str();
             Ok(self
                 .snapshot
                 .borrow()

components/relevancy/src/ingest.rs

@@ -20,22 +20,25 @@ pub mod url_hash;
 
 use rand_distr::{Beta, Distribution};
 
+use std::{collections::HashMap, sync::Arc};
+
+use parking_lot::Mutex;
+use remote_settings::{RemoteSettingsClient, RemoteSettingsService};
+
 pub use db::RelevancyDb;
 pub use error::{ApiResult, Error, RelevancyApiError, Result};
 pub use interest::{Interest, InterestVector};
-use parking_lot::Mutex;
 pub use ranker::score;
 
 use error_support::handle_error;
 
 use db::BanditData;
-use std::collections::HashMap;
 
 uniffi::setup_scaffolding!();
 
 #[derive(uniffi::Object)]
 pub struct RelevancyStore {
-    inner: RelevancyStoreInner<remote_settings::RemoteSettings>,
+    inner: RelevancyStoreInner<Arc<RemoteSettingsClient>>,
 }
 
 /// Top-level API for the Relevancy component
@@ -46,11 +49,13 @@ impl RelevancyStore {
     ///
     /// This is non-blocking since databases and other resources are lazily opened.
     #[uniffi::constructor]
-    #[handle_error(Error)]
-    pub fn new(db_path: String) -> ApiResult<Self> {
-        Ok(Self {
-            inner: RelevancyStoreInner::new(db_path, rs::create_client()?),
-        })
+    pub fn new(db_path: String, remote_settings: Arc<RemoteSettingsService>) -> Self {
+        Self {
+            inner: RelevancyStoreInner::new(
+                db_path,
+                remote_settings.make_client(rs::REMOTE_SETTINGS_COLLECTION.to_string()),
+            ),
+        }
     }
 
     /// Close any open resources (for example databases)

components/relevancy/src/lib.rs

@@ -4,9 +4,7 @@
  */
 
 use crate::{Error, Result};
-use remote_settings::{
-    RemoteSettings, RemoteSettingsConfig, RemoteSettingsResponse, RemoteSettingsServer,
-};
+use remote_settings::{RemoteSettingsClient, RemoteSettingsRecord};
 use serde::Deserialize;
 /// The Remote Settings collection name.
 pub(crate) const REMOTE_SETTINGS_COLLECTION: &str = "content-relevance";
@@ -16,45 +14,46 @@ pub(crate) const REMOTE_SETTINGS_COLLECTION: &str = "content-relevance";
 /// This trait lets tests use a mock client.
 pub(crate) trait RelevancyRemoteSettingsClient {
     /// Fetches records from the Suggest Remote Settings collection.
-    fn get_records(&self) -> Result<RemoteSettingsResponse>;
+    fn get_records(&self) -> Result<Vec<RemoteSettingsRecord>>;
 
     /// Fetches a record's attachment from the Suggest Remote Settings
     /// collection.
-    fn get_attachment(&self, location: &str) -> Result<Vec<u8>>;
+    fn get_attachment(&self, location: &RemoteSettingsRecord) -> Result<Vec<u8>>;
 }
 
-impl RelevancyRemoteSettingsClient for remote_settings::RemoteSettings {
-    fn get_records(&self) -> Result<RemoteSettingsResponse> {
-        Ok(remote_settings::RemoteSettings::get_records(self)?)
+impl RelevancyRemoteSettingsClient for RemoteSettingsClient {
+    fn get_records(&self) -> Result<Vec<RemoteSettingsRecord>> {
+        self.sync()?;
+        Ok(self
+            .get_records(false)
+            .expect("RemoteSettingsClient::get_records() returned None after `sync()` called"))
     }
 
-    fn get_attachment(&self, location: &str) -> Result<Vec<u8>> {
-        Ok(remote_settings::RemoteSettings::get_attachment(
-            self, location,
-        )?)
+    fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result<Vec<u8>> {
+        Ok(self.get_attachment(record)?)
     }
 }
 
 impl<T: RelevancyRemoteSettingsClient> RelevancyRemoteSettingsClient for &T {
-    fn get_records(&self) -> Result<RemoteSettingsResponse> {
+    fn get_records(&self) -> Result<Vec<RemoteSettingsRecord>> {
         (*self).get_records()
     }
 
-    fn get_attachment(&self, location: &str) -> Result<Vec<u8>> {
-        (*self).get_attachment(location)
+    fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result<Vec<u8>> {
+        (*self).get_attachment(record)
     }
 }
 
-pub fn create_client() -> Result<RemoteSettings> {
-    Ok(RemoteSettings::new(RemoteSettingsConfig {
-        collection_name: REMOTE_SETTINGS_COLLECTION.to_string(),
-        server: Some(RemoteSettingsServer::Prod),
-        server_url: None,
-        bucket_name: None,
-    })?)
+impl<T: RelevancyRemoteSettingsClient> RelevancyRemoteSettingsClient for std::sync::Arc<T> {
+    fn get_records(&self) -> Result<Vec<RemoteSettingsRecord>> {
+        (**self).get_records()
+    }
+
+    fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result<Vec<u8>> {
+        (**self).get_attachment(record)
+    }
 }
 
-/// A record in the Relevancy Remote Settings collection.
 #[derive(Clone, Debug, Deserialize)]
 pub struct RelevancyRecord {
     #[allow(dead_code)]
@@ -115,11 +114,11 @@ pub mod test {
     pub struct NullRelavancyRemoteSettingsClient;
 
     impl RelevancyRemoteSettingsClient for NullRelavancyRemoteSettingsClient {
-        fn get_records(&self) -> Result<RemoteSettingsResponse> {
+        fn get_records(&self) -> Result<Vec<RemoteSettingsRecord>> {
             panic!("NullRelavancyRemoteSettingsClient::get_records was called")
         }
 
-        fn get_attachment(&self, _location: &str) -> Result<Vec<u8>> {
+        fn get_attachment(&self, _record: &RemoteSettingsRecord) -> Result<Vec<u8>> {
             panic!("NullRelavancyRemoteSettingsClient::get_records was called")
         }
     }

components/relevancy/src/rs.rs

@@ -17,7 +17,7 @@ use crate::{ApiResult, Error, RemoteSettingsContext, Result};
 /// This is the version used in the new API, hence the `2` at the end.  The plan is to move
 /// consumers to the new API, remove the RemoteSettingsConfig struct, then remove the `2` from this
 /// name.
-#[derive(Debug, Clone, uniffi::Record)]
+#[derive(Debug, Default, Clone, uniffi::Record)]
 pub struct RemoteSettingsConfig2 {
     /// The Remote Settings server to use. Defaults to [RemoteSettingsServer::Prod],
     #[uniffi(default = None)]

components/remote_settings/src/config.rs

@@ -8,6 +8,7 @@ license = "MPL-2.0"
 [dependencies]
 anyhow = "1.0"
 fxa-client = { path = "../../components/fxa-client" }
+remote_settings = { path = "../../components/remote_settings" }
 sync_manager = { path = "../../components/sync_manager" }
 log = "0.4"
 sync15 = { path = "../../components/sync15", features=["sync-client"] }

examples/cli-support/Cargo.toml

@@ -5,7 +5,12 @@
 #![allow(unknown_lints)]
 #![warn(rust_2018_idioms)]
 
-use std::path::{Path, PathBuf};
+use std::{
+    path::{Path, PathBuf},
+    sync::Arc,
+};
+
+use remote_settings::{RemoteSettingsConfig2, RemoteSettingsService};
 
 pub mod fxa_creds;
 pub mod prompt;
@@ -68,3 +73,12 @@ pub fn workspace_root_dir() -> PathBuf {
     let cargo_toml_path = Path::new(std::str::from_utf8(&cargo_output).unwrap().trim());
     cargo_toml_path.parent().unwrap().to_path_buf()
 }
+
+pub fn remote_settings_service() -> Arc<RemoteSettingsService> {
+    Arc::new(RemoteSettingsService::new(
+        data_path(Some("remote-settings"))
+            .to_string_lossy()
+            .to_string(),
+        RemoteSettingsConfig2::default(),
+    ))
+}

examples/cli-support/src/lib.rs

@@ -5,7 +5,10 @@
 use std::sync::Arc;
 
 use clap::Parser;
-use cli_support::fxa_creds::{get_cli_fxa, get_default_fxa_config, SYNC_SCOPE};
+use cli_support::{
+    fxa_creds::{get_cli_fxa, get_default_fxa_config, SYNC_SCOPE},
+    remote_settings_service,
+};
 use env_logger::Builder;
 use interrupt_support::NeverInterrupts;
 use places::{ConnectionType, PlacesApi};
@@ -44,8 +47,10 @@ fn main() -> Result<()> {
     }
     builder.init();
     println!("================== Initializing Relevancy ===================");
-    let relevancy_store =
-        RelevancyStore::new("file:relevancy-cli-relevancy?mode=memory&cache=shared".to_owned())?;
+    let relevancy_store = RelevancyStore::new(
+        "file:relevancy-cli-relevancy?mode=memory&cache=shared".to_owned(),
+        remote_settings_service(),
+    );
     relevancy_store.ensure_interest_data_populated()?;
 
     println!("==================== Downloading History ====================");