From be662ea7738befd876b21912b2c2f80171179d7c Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Tue, 18 Feb 2025 23:07:35 -0800 Subject: [PATCH 1/6] [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}")) } From f584af2abf551d4a69d9627eb5a8e1b72619680b Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Tue, 25 Feb 2025 20:48:45 -0800 Subject: [PATCH 2/6] rebase changes --- components/suggest/src/benchmarks/client.rs | 2 +- components/suggest/src/benchmarks/mod.rs | 6 ++++-- components/suggest/src/rs.rs | 14 +++++++------- components/suggest/src/store.rs | 9 +++++---- components/suggest/src/testing/client.rs | 2 +- examples/suggest-cli/src/main.rs | 5 ++++- 6 files changed, 22 insertions(+), 16 deletions(-) diff --git a/components/suggest/src/benchmarks/client.rs b/components/suggest/src/benchmarks/client.rs index 8b52a6a227..d2b6ca8a79 100644 --- a/components/suggest/src/benchmarks/client.rs +++ b/components/suggest/src/benchmarks/client.rs @@ -96,7 +96,7 @@ impl rs::Client for RemoteSettingsBenchmarkClient { .collect()) } - fn download_attachment(&self, record: &rs::Record) -> Result> { + fn download_attachment(&self, record: rs::Record) -> Result> { match &record.attachment { Some(a) => match self.attachments.get(&a.location) { Some(data) => Ok(data.clone()), diff --git a/components/suggest/src/benchmarks/mod.rs b/components/suggest/src/benchmarks/mod.rs index be2cfe3792..70183ce533 100644 --- a/components/suggest/src/benchmarks/mod.rs +++ b/components/suggest/src/benchmarks/mod.rs @@ -20,8 +20,8 @@ use std::{ use tempfile::TempDir; use crate::{SuggestIngestionConstraints, SuggestStore}; -use remote_settings::RemoteSettingsConfig2; -use remote_settings::RemoteSettingsService; +use remote_settings::{RemoteSettingsConfig2, RemoteSettingsContext, RemoteSettingsService}; + use std::sync::Arc; pub mod client; @@ -82,6 +82,7 @@ fn new_store() -> SuggestStore { 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()); @@ -98,6 +99,7 @@ fn new_store() -> SuggestStore { 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()); diff --git a/components/suggest/src/rs.rs b/components/suggest/src/rs.rs index d06552345d..fa45d84365 100644 --- a/components/suggest/src/rs.rs +++ b/components/suggest/src/rs.rs @@ -74,7 +74,7 @@ pub(crate) trait Client { /// Records that can't be parsed as [SuggestRecord] are ignored. fn get_records(&self, collection: Collection) -> Result>; - fn download_attachment(&self, record: &Record) -> Result>; + fn download_attachment(&self, record: Record) -> Result>; } /// Implements the [Client] trait using a real remote settings client @@ -85,11 +85,10 @@ pub struct SuggestRemoteSettingsClient { } impl SuggestRemoteSettingsClient { - pub fn new(rs_service: &RemoteSettingsService, ) -> Result { + pub fn new(rs_service: &RemoteSettingsService) -> Result { Ok(Self { quicksuggest_client: rs_service.make_client("quicksuggest".to_owned())?, - fakespot_client: rs_service - .make_client("fakespot-suggest-products".to_owned())?, + fakespot_client: rs_service.make_client("fakespot-suggest-products".to_owned())?, }) } @@ -119,11 +118,12 @@ impl Client for SuggestRemoteSettingsClient { } } - fn download_attachment(&self, record: &Record) -> Result> { + fn download_attachment(&self, record: Record) -> Result> { + let converted_record: RemoteSettingsRecord = record.clone().into(); match &record.attachment { Some(_a) => Ok(self .client_for_collection(record.collection) - .get_attachment(record.clone().into())?), + .get_attachment(&converted_record)?), None => Err(Error::MissingAttachment(record.id.to_string())), } } @@ -163,7 +163,7 @@ impl From for RemoteSettingsRecord { id: record.id.to_string(), last_modified: record.last_modified, deleted: false, - attachment: record.attachment, + attachment: record.attachment.clone(), fields: record.payload.to_json_map(), } } diff --git a/components/suggest/src/store.rs b/components/suggest/src/store.rs index 87dde6aa3b..e3fdab6ff4 100644 --- a/components/suggest/src/store.rs +++ b/components/suggest/src/store.rs @@ -661,8 +661,9 @@ where // malformed, so skip to the next record. return Ok(()); }; - let data = context - .measure_download(|| self.settings_client.download_attachment(record))?; + let data = context.measure_download(|| { + self.settings_client.download_attachment(record.clone()) + })?; dao.put_icon(icon_id, &data, &attachment.mimetype)?; } SuggestRecord::Amo => { @@ -732,8 +733,8 @@ where return Ok(()); }; - let attachment_data = - context.measure_download(|| self.settings_client.download_attachment(record))?; + let attachment_data = context + .measure_download(|| self.settings_client.download_attachment(record.clone()))?; match serde_json::from_slice::>(&attachment_data) { Ok(attachment) => ingestion_handler(dao, &record.id, attachment.suggestions()), // If the attachment doesn't match our expected schema, just skip it. It's possible diff --git a/components/suggest/src/testing/client.rs b/components/suggest/src/testing/client.rs index 7124eb070e..6dc37a98af 100644 --- a/components/suggest/src/testing/client.rs +++ b/components/suggest/src/testing/client.rs @@ -272,7 +272,7 @@ impl Client for MockRemoteSettingsClient { .collect()) } - fn download_attachment(&self, record: &Record) -> Result> { + fn download_attachment(&self, record: Record) -> Result> { match &record.attachment { None => Err(Error::MissingAttachment(record.id.to_string())), Some(a) => Ok(self diff --git a/examples/suggest-cli/src/main.rs b/examples/suggest-cli/src/main.rs index 8bc19fa3c4..fb6d6a691d 100644 --- a/examples/suggest-cli/src/main.rs +++ b/examples/suggest-cli/src/main.rs @@ -7,7 +7,9 @@ use std::{collections::HashMap, sync::Arc}; use anyhow::Result; use clap::{Parser, Subcommand, ValueEnum}; -use remote_settings::{RemoteSettingsConfig2, RemoteSettingsServer, RemoteSettingsService}; +use remote_settings::{ + RemoteSettingsConfig2, RemoteSettingsContext, RemoteSettingsServer, RemoteSettingsService, +}; use suggest::{ AmpMatchingStrategy, SuggestIngestionConstraints, SuggestStore, SuggestStoreBuilder, SuggestionProvider, SuggestionProviderConstraints, SuggestionQuery, @@ -150,6 +152,7 @@ fn build_store(cli: &Cli) -> Arc { RemoteSettingsConfig2 { server: None, bucket_name: None, + app_context: Some(RemoteSettingsContext::default()), }, ) .unwrap(); From 24ded475dc3b11c2e8aca60c9d9d8c3d71c01838 Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Wed, 26 Feb 2025 11:45:17 -0800 Subject: [PATCH 3/6] feedback changes --- components/suggest/src/benchmarks/client.rs | 2 +- components/suggest/src/rs.rs | 8 ++---- components/suggest/src/schema.rs | 23 +++++---------- components/suggest/src/store.rs | 31 +++++++++++++-------- components/suggest/src/testing/client.rs | 2 +- 5 files changed, 31 insertions(+), 35 deletions(-) diff --git a/components/suggest/src/benchmarks/client.rs b/components/suggest/src/benchmarks/client.rs index d2b6ca8a79..8b52a6a227 100644 --- a/components/suggest/src/benchmarks/client.rs +++ b/components/suggest/src/benchmarks/client.rs @@ -96,7 +96,7 @@ impl rs::Client for RemoteSettingsBenchmarkClient { .collect()) } - fn download_attachment(&self, record: rs::Record) -> Result> { + fn download_attachment(&self, record: &rs::Record) -> Result> { match &record.attachment { Some(a) => match self.attachments.get(&a.location) { Some(data) => Ok(data.clone()), diff --git a/components/suggest/src/rs.rs b/components/suggest/src/rs.rs index fa45d84365..0a2a66f16e 100644 --- a/components/suggest/src/rs.rs +++ b/components/suggest/src/rs.rs @@ -74,7 +74,7 @@ pub(crate) trait Client { /// Records that can't be parsed as [SuggestRecord] are ignored. fn get_records(&self, collection: Collection) -> Result>; - fn download_attachment(&self, record: Record) -> Result>; + fn download_attachment(&self, record: &Record) -> Result>; } /// Implements the [Client] trait using a real remote settings client @@ -102,8 +102,6 @@ impl SuggestRemoteSettingsClient { 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); client.sync()?; let response = client.get_records(false); @@ -118,10 +116,10 @@ impl Client for SuggestRemoteSettingsClient { } } - fn download_attachment(&self, record: Record) -> Result> { + 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(&converted_record)?), None => Err(Error::MissingAttachment(record.id.to_string())), diff --git a/components/suggest/src/schema.rs b/components/suggest/src/schema.rs index 82f56b87be..0eebfdc33c 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,12 @@ impl ConnectionInitializer for SuggestConnectionInitializer<'_> { )?; Ok(()) } + 32 => { + // Drop rs_cache since it's no longer needed. + clear_database(tx)?; + tx.execute_batch("DROP TABLE rs_cache;")?; + Ok(()) + } _ => Err(open_database::Error::IncompatibleVersion(version)), } } @@ -643,7 +644,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])?; @@ -802,10 +802,6 @@ PRAGMA user_version=16; "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. @@ -819,11 +815,6 @@ PRAGMA user_version=16; 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 e3fdab6ff4..d01445466c 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, RemoteSettingsServer, RemoteSettingsService}; +use remote_settings::{self, RemoteSettingsError, RemoteSettingsServer, RemoteSettingsService}; use serde::de::DeserializeOwned; @@ -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,11 @@ 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 } @@ -107,18 +109,24 @@ impl SuggestStoreBuilder { } #[handle_error(Error)] - pub fn build(&self, rs_service: &RemoteSettingsService) -> SuggestApiResult> { + pub fn build(&self) -> 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 = SuggestRemoteSettingsClient::new(rs_service)?; - Ok(Arc::new(SuggestStore { - inner: SuggestStoreInner::new(data_path, extensions_to_load, client), - })) + if let Some(rs_service) = &inner.remote_settings_service { + let client = SuggestRemoteSettingsClient::new(rs_service)?; + Ok(Arc::new(SuggestStore { + inner: SuggestStoreInner::new(data_path, extensions_to_load, client), + })) + } else { + Err(Error::RemoteSettings(RemoteSettingsError::Other { + reason: "Unable to create client".to_string(), + })) + } } } @@ -661,9 +669,8 @@ where // malformed, so skip to the next record. return Ok(()); }; - let data = context.measure_download(|| { - self.settings_client.download_attachment(record.clone()) - })?; + let data = context + .measure_download(|| self.settings_client.download_attachment(record))?; dao.put_icon(icon_id, &data, &attachment.mimetype)?; } SuggestRecord::Amo => { @@ -733,8 +740,8 @@ where return Ok(()); }; - let attachment_data = context - .measure_download(|| self.settings_client.download_attachment(record.clone()))?; + let attachment_data = + context.measure_download(|| self.settings_client.download_attachment(record))?; match serde_json::from_slice::>(&attachment_data) { Ok(attachment) => ingestion_handler(dao, &record.id, attachment.suggestions()), // If the attachment doesn't match our expected schema, just skip it. It's possible diff --git a/components/suggest/src/testing/client.rs b/components/suggest/src/testing/client.rs index 6dc37a98af..7124eb070e 100644 --- a/components/suggest/src/testing/client.rs +++ b/components/suggest/src/testing/client.rs @@ -272,7 +272,7 @@ impl Client for MockRemoteSettingsClient { .collect()) } - fn download_attachment(&self, record: Record) -> Result> { + fn download_attachment(&self, record: &Record) -> Result> { match &record.attachment { None => Err(Error::MissingAttachment(record.id.to_string())), Some(a) => Ok(self From 16a78e43d440081e971be859f2bcb420976e00c4 Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Wed, 26 Feb 2025 12:15:23 -0800 Subject: [PATCH 4/6] clippy fix --- examples/suggest-cli/src/main.rs | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/examples/suggest-cli/src/main.rs b/examples/suggest-cli/src/main.rs index fb6d6a691d..0dcc9ae446 100644 --- a/examples/suggest-cli/src/main.rs +++ b/examples/suggest-cli/src/main.rs @@ -7,9 +7,7 @@ use std::{collections::HashMap, sync::Arc}; use anyhow::Result; use clap::{Parser, Subcommand, ValueEnum}; -use remote_settings::{ - RemoteSettingsConfig2, RemoteSettingsContext, RemoteSettingsServer, RemoteSettingsService, -}; +use remote_settings::RemoteSettingsServer; use suggest::{ AmpMatchingStrategy, SuggestIngestionConstraints, SuggestStore, SuggestStoreBuilder, SuggestionProvider, SuggestionProviderConstraints, SuggestionQuery, @@ -147,15 +145,6 @@ 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, - app_context: Some(RemoteSettingsContext::default()), - }, - ) - .unwrap(); Arc::new(SuggestStoreBuilder::default()) .data_path(cli_support::cli_data_path(DB_FILENAME)) .remote_settings_server(match cli.remote_settings_server { @@ -169,7 +158,7 @@ fn build_store(cli: &Cli) -> Arc { .clone() .unwrap_or_else(|| "main".to_owned()), ) - .build(&remote_settings_service) + .build() .unwrap_or_else(|e| panic!("Error building store: {e}")) } From fd74e934298651bb5232dea9f0ac800cbd55f662 Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Thu, 27 Feb 2025 10:18:59 -0800 Subject: [PATCH 5/6] feedback changes --- components/suggest/src/schema.rs | 8 +++----- components/suggest/src/store.rs | 25 ++++++++++++------------- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/components/suggest/src/schema.rs b/components/suggest/src/schema.rs index 0eebfdc33c..9c7dbd6783 100644 --- a/components/suggest/src/schema.rs +++ b/components/suggest/src/schema.rs @@ -613,7 +613,6 @@ impl ConnectionInitializer for SuggestConnectionInitializer<'_> { } 32 => { // Drop rs_cache since it's no longer needed. - clear_database(tx)?; tx.execute_batch("DROP TABLE rs_cache;")?; Ok(()) } @@ -792,11 +791,10 @@ 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(?, ?, ?, ?)", @@ -808,7 +806,7 @@ PRAGMA user_version=16; 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")?, diff --git a/components/suggest/src/store.rs b/components/suggest/src/store.rs index d01445466c..8aa8357db4 100644 --- a/components/suggest/src/store.rs +++ b/components/suggest/src/store.rs @@ -85,8 +85,6 @@ impl SuggestStoreBuilder { self: 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 } @@ -116,17 +114,18 @@ impl SuggestStoreBuilder { .data_path .clone() .ok_or_else(|| Error::SuggestStoreBuilder("data_path not specified".to_owned()))?; - - if let Some(rs_service) = &inner.remote_settings_service { - let client = SuggestRemoteSettingsClient::new(rs_service)?; - Ok(Arc::new(SuggestStore { - inner: SuggestStoreInner::new(data_path, extensions_to_load, client), - })) - } else { - Err(Error::RemoteSettings(RemoteSettingsError::Other { - reason: "Unable to create client".to_string(), - })) - } + 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, + SuggestRemoteSettingsClient::new(&rs_service)?, + ), + })) } } From 11b060654b59a5ca28fd6af4c7d631eb37565c06 Mon Sep 17 00:00:00 2001 From: Tif Tran Date: Thu, 27 Feb 2025 19:48:35 -0800 Subject: [PATCH 6/6] feedback changes --- components/suggest/src/store.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/components/suggest/src/store.rs b/components/suggest/src/store.rs index 8aa8357db4..8204c2f7fd 100644 --- a/components/suggest/src/store.rs +++ b/components/suggest/src/store.rs @@ -569,7 +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(|_| self.settings_client.get_records(collection))?; + 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] @@ -841,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(|_| { - self.settings_client - .get_records(ingest_record_type.collection()) - }) + let records = self + .settings_client + .get_records(ingest_record_type.collection()) .unwrap(); let changes = RecordChanges::new(