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..70183ce533 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, RemoteSettingsContext, RemoteSettingsService}; + +use std::sync::Arc; pub mod client; pub mod geoname; @@ -76,8 +79,15 @@ 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, + app_context: Some(RemoteSettingsContext::default()), + }; + 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 +96,16 @@ fn new_store() -> SuggestStore { }); let db_path = starter_dir.path().join(unique_db_filename()); + let rs_config = RemoteSettingsConfig2 { + bucket_name: None, + server: None, + app_context: Some(RemoteSettingsContext::default()), + }; + 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..0a2a66f16e 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,27 @@ 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 +100,28 @@ impl RemoteSettingsClient { } } -impl Client for RemoteSettingsClient { - fn get_records(&self, collection: Collection, dao: &mut SuggestDao) -> Result> { - // For now, handle the cache manually. Once 6328 is merged, we should be able to delegate - // this to remote_settings. +impl Client for SuggestRemoteSettingsClient { + fn get_records(&self, collection: Collection) -> Result> { 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> { + let converted_record: RemoteSettingsRecord = record.clone().into(); match &record.attachment { - Some(a) => Ok(self + Some(_) => Ok(self .client_for_collection(record.collection) - .get_attachment(&a.location)?), + .get_attachment(&converted_record)?), None => Err(Error::MissingAttachment(record.id.to_string())), } } @@ -181,11 +155,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.clone(), + 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 +200,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 +559,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 +639,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/schema.rs b/components/suggest/src/schema.rs index 82f56b87be..9c7dbd6783 100644 --- a/components/suggest/src/schema.rs +++ b/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 = 32; +pub const VERSION: u32 = 33; /// The current Suggest database schema. pub const SQL: &str = " @@ -32,11 +32,6 @@ CREATE TABLE meta( value NOT NULL ) WITHOUT ROWID; -CREATE TABLE rs_cache( - collection TEXT PRIMARY KEY, - data TEXT NOT NULL -) WITHOUT ROWID; - CREATE TABLE ingested_records( id TEXT, collection TEXT, @@ -616,6 +611,11 @@ impl ConnectionInitializer for SuggestConnectionInitializer<'_> { )?; Ok(()) } + 32 => { + // Drop rs_cache since it's no longer needed. + tx.execute_batch("DROP TABLE rs_cache;")?; + Ok(()) + } _ => Err(open_database::Error::IncompatibleVersion(version)), } } @@ -643,7 +643,6 @@ pub fn clear_database(db: &Connection) -> rusqlite::Result<()> { "geonames_metrics", "ingested_records", "keywords_metrics", - "rs_cache", ]; for t in conditional_tables { let table_exists = db.exists("SELECT 1 FROM sqlite_master WHERE name = ?", [t])?; @@ -792,38 +791,28 @@ PRAGMA user_version=16; let db_file = MigratedDatabaseFile::new(SuggestConnectionInitializer::default(), V16_SCHEMA); - // Upgrade to v25, the first version with with `ingested_records` and - // `rs_cache` tables. + // Upgrade to v25, the first version with with `ingested_records` tables. db_file.upgrade_to(25); - // Insert some ingested records and cache data. + // Insert some ingested records. let conn = db_file.open(); conn.execute( "INSERT INTO ingested_records(id, collection, type, last_modified) VALUES(?, ?, ?, ?)", ("record-id", "quicksuggest", "record-type", 1), )?; - conn.execute( - "INSERT INTO rs_cache(collection, data) VALUES(?, ?)", - ("quicksuggest", "some data"), - )?; 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(); - // `ingested_records` and `rs_cache` should be empty. + // `ingested_records` should be empty. let conn = db_file.open(); assert_eq!( conn.query_one::("SELECT count(*) FROM ingested_records")?, 0, "ingested_records should be empty" ); - assert_eq!( - conn.query_one::("SELECT count(*) FROM rs_cache")?, - 0, - "rs_cache should be empty" - ); conn.close().expect("Connection should be closed"); Ok(()) diff --git a/components/suggest/src/store.rs b/components/suggest/src/store.rs index 7a5b916433..8204c2f7fd 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, RemoteSettingsError, 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, @@ -42,6 +42,7 @@ pub struct SuggestStoreBuilder(Mutex); struct SuggestStoreBuilderInner { data_path: Option, remote_settings_server: Option, + remote_settings_service: Option>, remote_settings_bucket_name: Option, extensions_to_load: Vec, } @@ -82,10 +83,9 @@ impl SuggestStoreBuilder { pub fn remote_settings_service( self: Arc, - _rs_service: Arc, + rs_service: Arc, ) -> Arc { - // When #6607 lands, this will set the remote settings service. - // For now, it just exists so we can move consumers over to the new API ahead of time. + self.0.lock().remote_settings_service = Some(rs_service); self } @@ -114,15 +114,17 @@ impl SuggestStoreBuilder { .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 rs_service = inner.remote_settings_service.clone().ok_or_else(|| { + Error::RemoteSettings(RemoteSettingsError::Other { + reason: "remote_settings_service_not_specified".to_string(), + }) + })?; Ok(Arc::new(SuggestStore { - inner: SuggestStoreInner::new(data_path, extensions_to_load, client), + inner: SuggestStoreInner::new( + data_path, + extensions_to_load, + SuggestRemoteSettingsClient::new(&rs_service)?, + ), })) } } @@ -169,30 +171,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 +569,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 = self.settings_client.get_records(collection)?; // For each record type in that collection, calculate the changes and pass them to // [Self::ingest_records] @@ -851,11 +841,9 @@ where let writer = &self.dbs().unwrap().writer; let mut context = MetricsContext::default(); let ingested_records = writer.read(|dao| dao.get_ingested_records()).unwrap(); - let records = writer - .write(|dao| { - self.settings_client - .get_records(ingest_record_type.collection(), dao) - }) + let records = self + .settings_client + .get_records(ingest_record_type.collection()) .unwrap(); let changes = RecordChanges::new( 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()