From be662ea7738befd876b21912b2c2f80171179d7c Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Tue, 18 Feb 2025 23:07:35 -0800 Subject: [PATCH] [DISCO-3211] Suggest: move to new remote settings API --- components/remote_settings/src/lib.rs | 5 + components/suggest/src/benchmarks/client.rs | 8 +- components/suggest/src/benchmarks/mod.rs | 22 +++- components/suggest/src/db.rs | 49 +------- components/suggest/src/rs.rs | 117 ++++++++++---------- components/suggest/src/store.rs | 41 ++----- components/suggest/src/testing/client.rs | 3 +- examples/suggest-cli/src/main.rs | 12 +- 8 files changed, 107 insertions(+), 150 deletions(-) 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/components/suggest/src/benchmarks/client.rs b/components/suggest/src/benchmarks/client.rs index 8c36f49116..8b52a6a227 100644 --- a/components/suggest/src/benchmarks/client.rs +++ b/components/suggest/src/benchmarks/client.rs @@ -4,7 +4,7 @@ use std::collections::HashMap; -use crate::{db::SuggestDao, error::Error, rs, Result}; +use crate::{error::Error, rs, Result}; /// Remotes settings client for benchmarking /// @@ -87,11 +87,7 @@ impl RemoteSettingsBenchmarkClient { } impl rs::Client for RemoteSettingsBenchmarkClient { - fn get_records( - &self, - collection: rs::Collection, - _db: &mut SuggestDao, - ) -> Result> { + fn get_records(&self, collection: rs::Collection) -> Result> { Ok(self .records .iter() diff --git a/components/suggest/src/benchmarks/mod.rs b/components/suggest/src/benchmarks/mod.rs index 7a31534866..be2cfe3792 100644 --- a/components/suggest/src/benchmarks/mod.rs +++ b/components/suggest/src/benchmarks/mod.rs @@ -20,6 +20,9 @@ use std::{ use tempfile::TempDir; use crate::{SuggestIngestionConstraints, SuggestStore}; +use remote_settings::RemoteSettingsConfig2; +use remote_settings::RemoteSettingsService; +use std::sync::Arc; pub mod client; pub mod geoname; @@ -76,8 +79,14 @@ fn new_store() -> SuggestStore { let (starter_dir, starter_db_path) = starter.get_or_insert_with(|| { let temp_dir = tempfile::tempdir().unwrap(); let db_path = temp_dir.path().join(unique_db_filename()); - let store = - SuggestStore::new(&db_path.to_string_lossy(), None).expect("Error building store"); + let rs_config = RemoteSettingsConfig2 { + bucket_name: None, + server: None, + }; + let remote_settings_service = + Arc::new(RemoteSettingsService::new("".to_string(), rs_config).unwrap()); + let store = SuggestStore::new(&db_path.to_string_lossy(), remote_settings_service) + .expect("Error building store"); store .ingest(SuggestIngestionConstraints::all_providers()) .expect("Error during ingestion"); @@ -86,8 +95,15 @@ fn new_store() -> SuggestStore { }); let db_path = starter_dir.path().join(unique_db_filename()); + let rs_config = RemoteSettingsConfig2 { + bucket_name: None, + server: None, + }; + let remote_settings_service = + Arc::new(RemoteSettingsService::new("".to_string(), rs_config).unwrap()); std::fs::copy(starter_db_path, &db_path).expect("Error copying starter DB file"); - SuggestStore::new(&db_path.to_string_lossy(), None).expect("Error building store") + SuggestStore::new(&db_path.to_string_lossy(), remote_settings_service) + .expect("Error building store") } /// Cleanup the temp directory created for SuggestStore instances used in the benchmarks. diff --git a/components/suggest/src/db.rs b/components/suggest/src/db.rs index a80de8013c..51d0619c9b 100644 --- a/components/suggest/src/db.rs +++ b/components/suggest/src/db.rs @@ -7,11 +7,10 @@ use std::{cell::OnceCell, path::Path, sync::Arc}; use interrupt_support::{SqlInterruptHandle, SqlInterruptScope}; use parking_lot::{Mutex, MutexGuard}; -use remote_settings::RemoteSettingsResponse; use rusqlite::{ named_params, types::{FromSql, ToSql}, - Connection, OptionalExtension, + Connection, }; use sql_support::{open_database, repeat_sql_vars, ConnExt}; @@ -202,52 +201,6 @@ impl<'a> SuggestDao<'a> { // // These methods implement CRUD operations - pub fn read_cached_rs_data(&self, collection: &str) -> Option { - match self.try_read_cached_rs_data(collection) { - Ok(result) => result, - Err(e) => { - // Return None on failure . If the cached data is corrupted, maybe because the - // RemoteSettingsResponse schema changed, then we want to just continue on. This also matches - // the proposed API from #6328, so it should be easier to adapt this code once - // that's merged. - error_support::report_error!("suggest-rs-cache-read", "{e}"); - None - } - } - } - - pub fn write_cached_rs_data(&mut self, collection: &str, data: &RemoteSettingsResponse) { - if let Err(e) = self.try_write_cached_rs_data(collection, data) { - // Return None on failure for the same reason as in [Self::read_cached_rs_data] - error_support::report_error!("suggest-rs-cache-write", "{e}"); - } - } - - fn try_read_cached_rs_data(&self, collection: &str) -> Result> { - let mut stmt = self - .conn - .prepare_cached("SELECT data FROM rs_cache WHERE collection = ?")?; - let data = stmt - .query_row((collection,), |row| row.get::<_, Vec>(0)) - .optional()?; - match data { - Some(data) => Ok(Some(rmp_serde::decode::from_slice(data.as_slice())?)), - None => Ok(None), - } - } - - fn try_write_cached_rs_data( - &mut self, - collection: &str, - data: &RemoteSettingsResponse, - ) -> Result<()> { - let mut stmt = self - .conn - .prepare_cached("INSERT OR REPLACE INTO rs_cache(collection, data) VALUES(?, ?)")?; - stmt.execute((collection, rmp_serde::encode::to_vec(data)?))?; - Ok(()) - } - pub fn get_ingested_records(&self) -> Result> { let mut stmt = self .conn diff --git a/components/suggest/src/rs.rs b/components/suggest/src/rs.rs index 809c12d1ba..d06552345d 100644 --- a/components/suggest/src/rs.rs +++ b/components/suggest/src/rs.rs @@ -31,14 +31,17 @@ //! the new suggestion in their results, and return `Suggestion::T` variants //! as needed. -use std::fmt; +use std::{fmt, sync::Arc}; -use remote_settings::{Attachment, RemoteSettingsRecord}; -use serde::{Deserialize, Deserializer}; +use remote_settings::{ + Attachment, RemoteSettingsClient, RemoteSettingsError, RemoteSettingsRecord, + RemoteSettingsService, +}; +use serde::{Deserialize, Deserializer, Serialize}; +use serde_json::{Map, Value}; use crate::{ - db::SuggestDao, error::Error, provider::SuggestionProvider, - query::full_keywords_to_fts_content, Result, + error::Error, provider::SuggestionProvider, query::full_keywords_to_fts_content, Result, }; #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] @@ -69,45 +72,28 @@ pub(crate) trait Client { /// client-side filtering. /// /// Records that can't be parsed as [SuggestRecord] are ignored. - fn get_records(&self, collection: Collection, dao: &mut SuggestDao) -> Result>; + fn get_records(&self, collection: Collection) -> Result>; fn download_attachment(&self, record: &Record) -> Result>; } /// Implements the [Client] trait using a real remote settings client -pub struct RemoteSettingsClient { +pub struct SuggestRemoteSettingsClient { // Create a separate client for each collection name - quicksuggest_client: remote_settings::RemoteSettings, - fakespot_client: remote_settings::RemoteSettings, + quicksuggest_client: Arc, + fakespot_client: Arc, } -impl RemoteSettingsClient { - pub fn new( - server: Option, - bucket_name: Option, - server_url: Option, - ) -> Result { +impl SuggestRemoteSettingsClient { + pub fn new(rs_service: &RemoteSettingsService, ) -> Result { Ok(Self { - quicksuggest_client: remote_settings::RemoteSettings::new( - remote_settings::RemoteSettingsConfig { - server: server.clone(), - bucket_name: bucket_name.clone(), - collection_name: "quicksuggest".to_owned(), - server_url: server_url.clone(), - }, - )?, - fakespot_client: remote_settings::RemoteSettings::new( - remote_settings::RemoteSettingsConfig { - server, - bucket_name, - collection_name: "fakespot-suggest-products".to_owned(), - server_url, - }, - )?, + quicksuggest_client: rs_service.make_client("quicksuggest".to_owned())?, + fakespot_client: rs_service + .make_client("fakespot-suggest-products".to_owned())?, }) } - fn client_for_collection(&self, collection: Collection) -> &remote_settings::RemoteSettings { + fn client_for_collection(&self, collection: Collection) -> &RemoteSettingsClient { match collection { Collection::Fakespot => &self.fakespot_client, Collection::Quicksuggest => &self.quicksuggest_client, @@ -115,39 +101,29 @@ impl RemoteSettingsClient { } } -impl Client for RemoteSettingsClient { - fn get_records(&self, collection: Collection, dao: &mut SuggestDao) -> Result> { +impl Client for SuggestRemoteSettingsClient { + fn get_records(&self, collection: Collection) -> Result> { // For now, handle the cache manually. Once 6328 is merged, we should be able to delegate // this to remote_settings. let client = self.client_for_collection(collection); - let cache = dao.read_cached_rs_data(collection.name()); - let last_modified = match &cache { - Some(response) => response.last_modified, - None => 0, - }; - let response = match cache { - None => client.get_records()?, - Some(cache) => remote_settings::cache::merge_cache_and_response( - cache, - client.get_records_since(last_modified)?, - ), - }; - if last_modified != response.last_modified { - dao.write_cached_rs_data(collection.name(), &response); + client.sync()?; + let response = client.get_records(false); + match response { + Some(r) => Ok(r + .into_iter() + .filter_map(|r| Record::new(r, collection).ok()) + .collect()), + None => Err(Error::RemoteSettings(RemoteSettingsError::Other { + reason: "Unable to get records".to_owned(), + })), } - - Ok(response - .records - .into_iter() - .filter_map(|r| Record::new(r, collection).ok()) - .collect()) } fn download_attachment(&self, record: &Record) -> Result> { match &record.attachment { - Some(a) => Ok(self + Some(_a) => Ok(self .client_for_collection(record.collection) - .get_attachment(&a.location)?), + .get_attachment(record.clone().into())?), None => Err(Error::MissingAttachment(record.id.to_string())), } } @@ -181,11 +157,23 @@ impl Record { } } +impl From for RemoteSettingsRecord { + fn from(record: Record) -> Self { + RemoteSettingsRecord { + id: record.id.to_string(), + last_modified: record.last_modified, + deleted: false, + attachment: record.attachment, + fields: record.payload.to_json_map(), + } + } +} + /// A record in the Suggest Remote Settings collection. /// /// Most Suggest records don't carry inline fields except for `type`. /// Suggestions themselves are typically stored in each record's attachment. -#[derive(Clone, Debug, Deserialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] #[serde(tag = "type")] pub(crate) enum SuggestRecord { #[serde(rename = "icon")] @@ -214,6 +202,15 @@ pub(crate) enum SuggestRecord { Geonames, } +impl SuggestRecord { + fn to_json_map(&self) -> Map { + match serde_json::to_value(self) { + Ok(Value::Object(map)) => map, + _ => unreachable!(), + } + } +} + /// Enum for the different record types that can be consumed. /// Extracting this from the serialization enum so that we can /// extend it to get type metadata. @@ -564,7 +561,7 @@ pub(crate) struct DownloadedFakespotSuggestion { } /// An exposure suggestion record's inline data -#[derive(Clone, Debug, Deserialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] pub(crate) struct DownloadedExposureRecord { pub suggestion_type: String, } @@ -644,11 +641,11 @@ impl FullOrPrefixKeywords { } /// Global Suggest configuration data to ingest from a configuration record -#[derive(Clone, Debug, Deserialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] pub(crate) struct DownloadedGlobalConfig { pub configuration: DownloadedGlobalConfigInner, } -#[derive(Clone, Debug, Deserialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] pub(crate) struct DownloadedGlobalConfigInner { /// The maximum number of times the user can click "Show less frequently" /// for a suggestion in the UI. diff --git a/components/suggest/src/store.rs b/components/suggest/src/store.rs index 7a5b916433..87dde6aa3b 100644 --- a/components/suggest/src/store.rs +++ b/components/suggest/src/store.rs @@ -12,7 +12,7 @@ use std::{ use error_support::{breadcrumb, handle_error}; use once_cell::sync::OnceCell; use parking_lot::Mutex; -use remote_settings::{self, RemoteSettingsConfig, RemoteSettingsServer, RemoteSettingsService}; +use remote_settings::{self, RemoteSettingsServer, RemoteSettingsService}; use serde::de::DeserializeOwned; @@ -24,8 +24,8 @@ use crate::{ metrics::{MetricsContext, SuggestIngestionMetrics, SuggestQueryMetrics}, provider::{SuggestionProvider, SuggestionProviderConstraints, DEFAULT_INGEST_PROVIDERS}, rs::{ - Client, Collection, DownloadedExposureRecord, Record, RemoteSettingsClient, - SuggestAttachment, SuggestRecord, SuggestRecordId, SuggestRecordType, + Client, Collection, DownloadedExposureRecord, Record, SuggestAttachment, SuggestRecord, + SuggestRecordId, SuggestRecordType, SuggestRemoteSettingsClient, }, suggestion::AmpSuggestionType, QueryWithMetricsResult, Result, SuggestApiResult, Suggestion, SuggestionQuery, @@ -107,19 +107,14 @@ impl SuggestStoreBuilder { } #[handle_error(Error)] - pub fn build(&self) -> SuggestApiResult> { + pub fn build(&self, rs_service: &RemoteSettingsService) -> SuggestApiResult> { let inner = self.0.lock(); let extensions_to_load = inner.extensions_to_load.clone(); let data_path = inner .data_path .clone() .ok_or_else(|| Error::SuggestStoreBuilder("data_path not specified".to_owned()))?; - - let client = RemoteSettingsClient::new( - inner.remote_settings_server.clone(), - inner.remote_settings_bucket_name.clone(), - None, - )?; + let client = SuggestRemoteSettingsClient::new(rs_service)?; Ok(Arc::new(SuggestStore { inner: SuggestStoreInner::new(data_path, extensions_to_load, client), @@ -169,30 +164,19 @@ pub enum InterruptKind { /// on the first launch. #[derive(uniffi::Object)] pub struct SuggestStore { - inner: SuggestStoreInner, + inner: SuggestStoreInner, } #[uniffi::export] impl SuggestStore { /// Creates a Suggest store. #[handle_error(Error)] - #[uniffi::constructor(default(settings_config = None))] + #[uniffi::constructor()] pub fn new( path: &str, - settings_config: Option, + remote_settings_service: Arc, ) -> SuggestApiResult { - let client = match settings_config { - Some(settings_config) => RemoteSettingsClient::new( - settings_config.server, - settings_config.bucket_name, - settings_config.server_url, - // Note: collection name is ignored, since we fetch from multiple collections - // (fakespot-suggest-products and quicksuggest). No consumer sets it to a - // non-default value anyways. - )?, - None => RemoteSettingsClient::new(None, None, None)?, - }; - + let client = SuggestRemoteSettingsClient::new(&remote_settings_service)?; Ok(Self { inner: SuggestStoreInner::new(path.to_owned(), vec![], client), }) @@ -578,8 +562,7 @@ where // For each collection, fetch all records for (collection, record_types) in record_types_by_collection { breadcrumb!("Ingesting collection {}", collection.name()); - let records = - write_scope.write(|dao| self.settings_client.get_records(collection, dao))?; + let records = write_scope.write(|_| self.settings_client.get_records(collection))?; // For each record type in that collection, calculate the changes and pass them to // [Self::ingest_records] @@ -852,9 +835,9 @@ where let mut context = MetricsContext::default(); let ingested_records = writer.read(|dao| dao.get_ingested_records()).unwrap(); let records = writer - .write(|dao| { + .write(|_| { self.settings_client - .get_records(ingest_record_type.collection(), dao) + .get_records(ingest_record_type.collection()) }) .unwrap(); diff --git a/components/suggest/src/testing/client.rs b/components/suggest/src/testing/client.rs index 2d8034d283..7124eb070e 100644 --- a/components/suggest/src/testing/client.rs +++ b/components/suggest/src/testing/client.rs @@ -9,7 +9,6 @@ use serde_json::json; use serde_json::Value as JsonValue; use crate::{ - db::SuggestDao, error::Error, rs::{Client, Collection, Record, SuggestRecordId, SuggestRecordType}, testing::JsonExt, @@ -264,7 +263,7 @@ pub struct MockIcon { } impl Client for MockRemoteSettingsClient { - fn get_records(&self, collection: Collection, _db: &mut SuggestDao) -> Result> { + fn get_records(&self, collection: Collection) -> Result> { Ok(self .records .iter() diff --git a/examples/suggest-cli/src/main.rs b/examples/suggest-cli/src/main.rs index 0dcc9ae446..8bc19fa3c4 100644 --- a/examples/suggest-cli/src/main.rs +++ b/examples/suggest-cli/src/main.rs @@ -7,7 +7,7 @@ use std::{collections::HashMap, sync::Arc}; use anyhow::Result; use clap::{Parser, Subcommand, ValueEnum}; -use remote_settings::RemoteSettingsServer; +use remote_settings::{RemoteSettingsConfig2, RemoteSettingsServer, RemoteSettingsService}; use suggest::{ AmpMatchingStrategy, SuggestIngestionConstraints, SuggestStore, SuggestStoreBuilder, SuggestionProvider, SuggestionProviderConstraints, SuggestionQuery, @@ -145,6 +145,14 @@ fn main() -> Result<()> { } fn build_store(cli: &Cli) -> Arc { + let remote_settings_service = RemoteSettingsService::new( + cli_support::cli_data_path(DB_FILENAME), + RemoteSettingsConfig2 { + server: None, + bucket_name: None, + }, + ) + .unwrap(); Arc::new(SuggestStoreBuilder::default()) .data_path(cli_support::cli_data_path(DB_FILENAME)) .remote_settings_server(match cli.remote_settings_server { @@ -158,7 +166,7 @@ fn build_store(cli: &Cli) -> Arc { .clone() .unwrap_or_else(|| "main".to_owned()), ) - .build() + .build(&remote_settings_service) .unwrap_or_else(|e| panic!("Error building store: {e}")) }