From 5dba6b5fe3110250ff2f03dbda06452be2617ad9 Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Wed, 19 Feb 2025 11:13:27 -0500 Subject: [PATCH] [DISCO-3212] Switch Relevancy to new remote settings API --- Cargo.lock | 1 + components/relevancy/src/ingest.rs | 25 +++--------- components/relevancy/src/lib.rs | 15 ++++++-- components/relevancy/src/rs.rs | 49 ++++++++++++------------ components/remote_settings/src/config.rs | 2 +- components/remote_settings/src/lib.rs | 5 +++ examples/cli-support/Cargo.toml | 1 + examples/cli-support/src/lib.rs | 20 +++++++++- 8 files changed, 68 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5be5f8f0cf..22d99e6d3b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -763,6 +763,7 @@ dependencies = [ "env_logger", "fxa-client", "log", + "remote_settings", "sync15", "sync_manager", "url", diff --git a/components/relevancy/src/ingest.rs b/components/relevancy/src/ingest.rs index aaf413e29e..0cf5542630 100644 --- a/components/relevancy/src/ingest.rs +++ b/components/relevancy/src/ingest.rs @@ -39,14 +39,10 @@ pub fn ensure_interest_data_populated( fn fetch_interest_data_inner( client: C, ) -> Result> { - 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 client.get_records()? { + 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 +94,6 @@ mod test { use std::{cell::RefCell, collections::HashMap}; use anyhow::Context; - use remote_settings::RemoteSettingsResponse; use serde_json::json; use super::*; @@ -160,20 +155,12 @@ mod test { } impl RelevancyRemoteSettingsClient for SnapshotSettingsClient { - fn get_records(&self) -> Result { - 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> { + Ok(self.snapshot.borrow().records.clone()) } - fn get_attachment(&self, location: &str) -> Result> { + fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result> { + let location = record.attachment.as_ref().unwrap().location.as_str(); Ok(self .snapshot .borrow() diff --git a/components/relevancy/src/lib.rs b/components/relevancy/src/lib.rs index 75a29138ba..c07a0a85e9 100644 --- a/components/relevancy/src/lib.rs +++ b/components/relevancy/src/lib.rs @@ -20,10 +20,14 @@ pub mod url_hash; use rand_distr::{Beta, Distribution}; +use std::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; @@ -35,7 +39,7 @@ uniffi::setup_scaffolding!(); #[derive(uniffi::Object)] pub struct RelevancyStore { - inner: RelevancyStoreInner, + inner: RelevancyStoreInner>, } /// Top-level API for the Relevancy component @@ -47,9 +51,12 @@ 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 { + pub fn new(db_path: String, remote_settings: Arc) -> ApiResult { Ok(Self { - inner: RelevancyStoreInner::new(db_path, rs::create_client()?), + inner: RelevancyStoreInner::new( + db_path, + remote_settings.make_client(rs::REMOTE_SETTINGS_COLLECTION.to_string())?, + ), }) } diff --git a/components/relevancy/src/rs.rs b/components/relevancy/src/rs.rs index 1273ebd9cb..b4e4f9c530 100644 --- a/components/relevancy/src/rs.rs +++ b/components/relevancy/src/rs.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; + fn get_records(&self) -> Result>; /// Fetches a record's attachment from the Suggest Remote Settings /// collection. - fn get_attachment(&self, location: &str) -> Result>; + fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result>; } -impl RelevancyRemoteSettingsClient for remote_settings::RemoteSettings { - fn get_records(&self) -> Result { - Ok(remote_settings::RemoteSettings::get_records(self)?) +impl RelevancyRemoteSettingsClient for RemoteSettingsClient { + fn get_records(&self) -> Result> { + self.sync()?; + Ok(self + .get_records(false) + .expect("RemoteSettingsClient::get_records() returned None after `sync()` called")) } - fn get_attachment(&self, location: &str) -> Result> { - Ok(remote_settings::RemoteSettings::get_attachment( - self, location, - )?) + fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result> { + Ok(self.get_attachment(record)?) } } impl RelevancyRemoteSettingsClient for &T { - fn get_records(&self) -> Result { + fn get_records(&self) -> Result> { (*self).get_records() } - fn get_attachment(&self, location: &str) -> Result> { - (*self).get_attachment(location) + fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result> { + (*self).get_attachment(record) } } -pub fn create_client() -> Result { - Ok(RemoteSettings::new(RemoteSettingsConfig { - collection_name: REMOTE_SETTINGS_COLLECTION.to_string(), - server: Some(RemoteSettingsServer::Prod), - server_url: None, - bucket_name: None, - })?) +impl RelevancyRemoteSettingsClient for std::sync::Arc { + fn get_records(&self) -> Result> { + (**self).get_records() + } + + fn get_attachment(&self, record: &RemoteSettingsRecord) -> Result> { + (**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 { + fn get_records(&self) -> Result> { panic!("NullRelavancyRemoteSettingsClient::get_records was called") } - fn get_attachment(&self, _location: &str) -> Result> { + fn get_attachment(&self, _record: &RemoteSettingsRecord) -> Result> { panic!("NullRelavancyRemoteSettingsClient::get_records was called") } } diff --git a/components/remote_settings/src/config.rs b/components/remote_settings/src/config.rs index 9c7d9afd41..23b4499352 100644 --- a/components/remote_settings/src/config.rs +++ b/components/remote_settings/src/config.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)] diff --git a/components/remote_settings/src/lib.rs b/components/remote_settings/src/lib.rs index e80f017808..7aecfbf146 100644 --- a/components/remote_settings/src/lib.rs +++ b/components/remote_settings/src/lib.rs @@ -199,6 +199,11 @@ impl RemoteSettingsClient { pub fn get_attachment(&self, record: &RemoteSettingsRecord) -> ApiResult> { self.internal.get_attachment(record) } + + #[handle_error(Error)] + pub fn sync(&self) -> ApiResult<()> { + self.internal.sync() + } } impl RemoteSettingsClient { diff --git a/examples/cli-support/Cargo.toml b/examples/cli-support/Cargo.toml index 40c29d2c3d..b0e3c06004 100644 --- a/examples/cli-support/Cargo.toml +++ b/examples/cli-support/Cargo.toml @@ -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"] } diff --git a/examples/cli-support/src/lib.rs b/examples/cli-support/src/lib.rs index a07e724598..bb1777b2c2 100644 --- a/examples/cli-support/src/lib.rs +++ b/examples/cli-support/src/lib.rs @@ -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; @@ -63,3 +68,16 @@ 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 { + Arc::new( + RemoteSettingsService::new( + data_path(Some("remote-settings")) + .to_string_lossy() + .to_string(), + RemoteSettingsConfig2::default(), + None, + ) + .expect("Error creating RemoteSettingsService"), + ) +}