diff --git a/CHANGELOG.md b/CHANGELOG.md index 177e54c19b2..d65a1a46c2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ ### Glean - Updated to v70.0.0 ([#7598](https://github.com/mozilla/application-services/pull/7598)) +### Search + +- Added in-development v3 APIs to `SearchEngineSelector` for a new `search-config-v3` collection. + The APIs are currently the same as v2 but do not support configuration overrides. + # v157.0 (_2026-09-10_) ## ✨ What's Changed ✨ diff --git a/components/search/src/configuration_types.rs b/components/search/src/configuration_types.rs index 3c564de0078..b66442e6131 100644 --- a/components/search/src/configuration_types.rs +++ b/components/search/src/configuration_types.rs @@ -126,6 +126,47 @@ pub(crate) struct JSONEngineBase { pub urls: JSONEngineUrls, } +/// Represents the engine base section of the configuration for v3. +#[derive(Debug, Default, Deserialize, Clone)] +#[serde(rename_all = "camelCase")] +pub(crate) struct JSONEngineBaseV3 { + /// A list of aliases for this engine. + pub aliases: Option>, + + /// The character set this engine uses for queries. Defaults to 'UTF=8' if not set. + pub charset: Option, + + /// The classification of search engine according to the main search types + /// (e.g. general, shopping, travel, dictionary). Currently, only marking as + /// a general search engine is supported. + #[serde(default)] + pub classification: SearchEngineClassification, + + /// The user visible name for the search engine. + pub name: String, + + /// The partner code for the engine. This will be inserted into parameters + /// which include `{partnerCode}`. + pub partner_code: Option, + + /// The URLs associated with the search engine. + pub urls: JSONEngineUrls, +} + +/// Temporary helper to reduce work for handling the original and v3 types. +impl From for JSONEngineBase { + fn from(base: JSONEngineBaseV3) -> Self { + Self { + aliases: base.aliases, + charset: base.charset, + classification: base.classification, + name: base.name, + partner_code: base.partner_code, + urls: base.urls, + } + } +} + /// Specifies details of possible user environments that the engine or variant /// applies to. #[derive(Debug, Deserialize, Clone, Default)] @@ -246,6 +287,23 @@ pub(crate) struct JSONEngineRecord { pub variants: Vec, } +/// Represents an individual engine record in the v3 configuration. +#[derive(Debug, Deserialize, Clone)] +#[serde(rename_all = "camelCase")] +pub(crate) struct JSONEngineRecordV3 { + /// The identiifer for the search engine. + pub identifier: String, + + /// The base information of the search engine, may be extended by the + /// variants. + pub base: JSONEngineBaseV3, + + /// Describes variations of this search engine that may occur depending on + /// the user's environment. The last variant that matches the user's + /// environment will be applied to the engine, subvariants may also be applied. + pub variants: Vec, +} + #[derive(Debug, Deserialize, Clone)] #[serde(rename_all = "camelCase")] pub(crate) struct JSONSpecificDefaultRecord { @@ -332,8 +390,29 @@ pub(crate) enum JSONSearchConfigurationRecords { Unknown, } +/// Represents an individual record in the raw search configuration v3. +#[derive(Debug, Deserialize, Clone)] +#[serde(tag = "recordType", rename_all = "camelCase")] +pub(crate) enum JSONSearchConfigurationRecordsV3 { + DefaultEngines(JSONDefaultEnginesRecord), + Engine(Box), + EngineOrders(JSONEngineOrdersRecord), + AvailableLocales(JSONAvailableLocalesRecord), + // Include some flexibilty if we choose to add new record types in future. + // Current versions of the application receiving the configuration will + // ignore the new record types. + #[serde(other)] + Unknown, +} + /// Represents the search configuration as received from remote settings. #[derive(Debug, Deserialize)] pub(crate) struct JSONSearchConfiguration { pub data: Vec, } + +/// Represents the search configuration v3 as received from remote settings. +#[derive(Debug, Deserialize)] +pub(crate) struct JSONSearchConfigurationV3 { + pub data: Vec, +} diff --git a/components/search/src/error.rs b/components/search/src/error.rs index 7973b783dcd..ce7d134968c 100644 --- a/components/search/src/error.rs +++ b/components/search/src/error.rs @@ -19,7 +19,7 @@ pub enum Error { SearchConfigNotSpecified, #[error("Search configuration overrides not specified")] SearchConfigOverridesNotSpecified, - #[error("No search config v2 records received from remote settings")] + #[error("No search config records received from remote settings")] SearchConfigNoRecords, #[error("No search config overrides v2 records received from remote settings")] SearchConfigOverridesNoRecords, diff --git a/components/search/src/filter.rs b/components/search/src/filter.rs index aa55ce4c846..cb256e609ac 100644 --- a/components/search/src/filter.rs +++ b/components/search/src/filter.rs @@ -8,11 +8,12 @@ use crate::configuration_overrides_types::JSONOverridesRecord; use crate::environment_matching::matches_user_environment; use crate::{ error::Error, JSONDefaultEnginesRecord, JSONEngineBase, JSONEngineMethod, JSONEngineRecord, - JSONEngineUrl, JSONEngineUrls, JSONEngineVariant, JSONSearchConfigurationRecords, - RefinedSearchConfig, SearchEngineDefinition, SearchEngineUrl, SearchEngineUrls, - SearchUserEnvironment, + JSONEngineRecordV3, JSONEngineUrl, JSONEngineUrls, JSONEngineVariant, + JSONSearchConfigurationRecords, JSONSearchConfigurationRecordsV3, RefinedSearchConfig, + RefinedSearchConfigV3, SearchEngineDefinition, SearchEngineDefinitionV3, SearchEngineUrl, + SearchEngineUrls, SearchUserEnvironment, }; -use crate::{sort_helpers, JSONAvailableLocalesRecord, JSONEngineOrdersRecord}; +use crate::{sort_helpers, JSONAvailableLocalesRecord, JSONEngineBaseV3, JSONEngineOrdersRecord}; use remote_settings::RemoteSettingsRecord; use std::collections::HashSet; @@ -89,6 +90,49 @@ impl SearchEngineUrls { } } +pub(crate) trait EngineDefinition: Clone { + fn identifier(&self) -> &str; + fn name(&self) -> &str; + fn order_hint(&self) -> Option; + fn set_order_hint(&mut self, order_hint: Option); +} + +impl EngineDefinition for SearchEngineDefinition { + fn identifier(&self) -> &str { + &self.identifier + } + + fn name(&self) -> &str { + &self.name + } + + fn order_hint(&self) -> Option { + self.order_hint + } + + fn set_order_hint(&mut self, order_hint: Option) { + self.order_hint = order_hint; + } +} + +impl EngineDefinition for SearchEngineDefinitionV3 { + fn identifier(&self) -> &str { + &self.identifier + } + + fn name(&self) -> &str { + &self.name + } + + fn order_hint(&self) -> Option { + self.order_hint + } + + fn set_order_hint(&mut self, order_hint: Option) { + self.order_hint = order_hint; + } +} + impl SearchEngineDefinition { fn merge_variant( &mut self, @@ -158,18 +202,43 @@ impl SearchEngineDefinition { } } -pub(crate) struct FilterRecordsResult { - engines: Vec, +impl SearchEngineDefinitionV3 { + pub(crate) fn from_configuration_details( + user_environment: &SearchUserEnvironment, + identifier: &str, + base: JSONEngineBaseV3, + variant: &JSONEngineVariant, + sub_variant: &Option, + ) -> SearchEngineDefinitionV3 { + // v3 engines are currently built identically to v2 engines, so this + // delegates to the v2 builder. As search-config-v3 diverges from v2, + // we'll need to replace the delegation piecewise (e.g. set v3-only + // fields on the result, or fork the builder entirely). + SearchEngineDefinition::from_configuration_details( + user_environment, + identifier, + base.into(), + variant, + sub_variant, + ) + .into() + } +} + +pub(crate) struct FilterRecordsResult { + engines: Vec, default_engines_record: Option, engine_orders_record: Option, } pub(crate) trait Filter { + type Engine: EngineDefinition; + fn filter_records( &self, user_environment: &mut SearchUserEnvironment, overrides: Option>, - ) -> Result; + ) -> Result, Error>; } fn apply_overrides( @@ -210,11 +279,13 @@ fn negotiate_languages(user_environment: &mut SearchUserEnvironment, available_l } impl Filter for Vec { + type Engine = SearchEngineDefinition; + fn filter_records( &self, user_environment: &mut SearchUserEnvironment, overrides: Option>, - ) -> Result { + ) -> Result, Error> { let mut available_locales = Vec::new(); for record in self { if let Some(val) = record.fields.get("recordType") { @@ -275,11 +346,13 @@ impl Filter for Vec { } impl Filter for Vec { + type Engine = SearchEngineDefinition; + fn filter_records( &self, user_environment: &mut SearchUserEnvironment, overrides: Option>, - ) -> Result { + ) -> Result, Error> { let mut available_locales = Vec::new(); for record in self { if let JSONSearchConfigurationRecords::AvailableLocales(locales_record) = record { @@ -325,11 +398,68 @@ impl Filter for Vec { } } -pub(crate) fn filter_engine_configuration_impl( +impl Filter for Vec { + type Engine = SearchEngineDefinitionV3; + + fn filter_records( + &self, + user_environment: &mut SearchUserEnvironment, + _overrides: Option>, + ) -> Result, Error> { + let mut available_locales = Vec::new(); + for record in self { + if let JSONSearchConfigurationRecordsV3::AvailableLocales(locales_record) = record { + available_locales = locales_record.locales.clone(); + } + } + negotiate_languages(user_environment, &available_locales); + + let mut engines = Vec::new(); + let mut default_engines_record = None; + let mut engine_orders_record = None; + + for record in self { + match record { + JSONSearchConfigurationRecordsV3::Engine(engine) => { + let result = maybe_extract_engine_config_v3(user_environment, engine.clone()); + engines.extend(result); + } + JSONSearchConfigurationRecordsV3::DefaultEngines(default_engines) => { + default_engines_record = Some(default_engines); + } + JSONSearchConfigurationRecordsV3::EngineOrders(engine_orders) => { + engine_orders_record = Some(engine_orders) + } + JSONSearchConfigurationRecordsV3::AvailableLocales(_) => { + // Handled above + } + JSONSearchConfigurationRecordsV3::Unknown => { + // Prevents panics if a new record type is added in future. + } + } + } + + Ok(FilterRecordsResult { + engines, + default_engines_record: default_engines_record.cloned(), + engine_orders_record: engine_orders_record.cloned(), + }) + } +} + +/// The intermediate result of filtering a configuration, before it is +/// converted into the version-specific refined configuration type. +struct FilteredConfiguration { + engines: Vec, + app_default_engine_id: Option, + app_private_default_engine_id: Option, +} + +fn filter_engine_configuration_core( user_environment: SearchUserEnvironment, - configuration: &impl Filter, + configuration: &F, overrides: Option>, -) -> Result { +) -> Result, Error> { let mut user_environment = user_environment.clone(); user_environment.locale = user_environment.locale.to_lowercase(); user_environment.region = user_environment.region.to_lowercase(); @@ -363,7 +493,7 @@ pub(crate) fn filter_engine_configuration_impl( ) }); - RefinedSearchConfig { + FilteredConfiguration { engines, app_default_engine_id: default_engine_id, app_private_default_engine_id: default_private_engine_id, @@ -371,43 +501,94 @@ pub(crate) fn filter_engine_configuration_impl( }) } +pub(crate) fn filter_engine_configuration_impl( + user_environment: SearchUserEnvironment, + configuration: &impl Filter, + overrides: Option>, +) -> Result { + filter_engine_configuration_core(user_environment, configuration, overrides).map(|filtered| { + RefinedSearchConfig { + engines: filtered.engines, + app_default_engine_id: filtered.app_default_engine_id, + app_private_default_engine_id: filtered.app_private_default_engine_id, + } + }) +} + +pub(crate) fn filter_engine_configuration_v3_impl( + user_environment: SearchUserEnvironment, + configuration: &impl Filter, +) -> Result { + filter_engine_configuration_core(user_environment, configuration, None).map(|filtered| { + RefinedSearchConfigV3 { + engines: filtered.engines, + app_default_engine_id: filtered.app_default_engine_id, + app_private_default_engine_id: filtered.app_private_default_engine_id, + } + }) +} + +fn find_matching_variant( + variants: Vec, + user_environment: &SearchUserEnvironment, +) -> Option<(JSONEngineVariant, Option)> { + let matching_variant = variants + .into_iter() + .rev() + .find(|r| matches_user_environment(&r.environment, user_environment))?; + + let matching_sub_variant = matching_variant + .sub_variants + .iter() + .rev() + .find(|r| matches_user_environment(&r.environment, user_environment)) + .cloned(); + + Some((matching_variant, matching_sub_variant)) +} + fn maybe_extract_engine_config( user_environment: &SearchUserEnvironment, record: Box, ) -> Option { let JSONEngineRecord { identifier, - variants, base, + variants, } = *record; - let matching_variant = variants - .into_iter() - .rev() - .find(|r| matches_user_environment(&r.environment, user_environment)); - - let mut matching_sub_variant = None; - if let Some(variant) = &matching_variant { - matching_sub_variant = variant - .sub_variants - .iter() - .rev() - .find(|r| matches_user_environment(&r.environment, user_environment)) - .cloned(); - } - - matching_variant.map(|variant| { + find_matching_variant(variants, user_environment).map(|(variant, sub_variant)| { SearchEngineDefinition::from_configuration_details( user_environment, &identifier, base, &variant, - &matching_sub_variant, + &sub_variant, ) }) } -fn determine_default_engines( - engines: &[SearchEngineDefinition], +fn maybe_extract_engine_config_v3( + user_environment: &SearchUserEnvironment, + record: Box, +) -> Option { + let JSONEngineRecordV3 { + identifier, + base, + variants, + } = *record; + find_matching_variant(variants, user_environment).map(|(variant, sub_variant)| { + SearchEngineDefinitionV3::from_configuration_details( + user_environment, + &identifier, + base, + &variant, + &sub_variant, + ) + }) +} + +fn determine_default_engines( + engines: &[E], default_engines_record: Option, user_environment: &SearchUserEnvironment, ) -> (Option, Option) { @@ -455,18 +636,18 @@ fn determine_default_engines( } } -fn find_engine_id(engines: &[SearchEngineDefinition], engine_id: String) -> Option { +fn find_engine_id(engines: &[E], engine_id: String) -> Option { if engine_id.is_empty() { return None; } - match engines.iter().any(|e| e.identifier == engine_id) { + match engines.iter().any(|e| e.identifier() == engine_id) { true => Some(engine_id.clone()), false => None, } } -fn find_engine_id_with_match( - engines: &[SearchEngineDefinition], +fn find_engine_id_with_match( + engines: &[E], engine_id_match: String, ) -> Option { if engine_id_match.is_empty() { @@ -475,14 +656,30 @@ fn find_engine_id_with_match( if let Some(match_no_star) = engine_id_match.strip_suffix('*') { return engines .iter() - .find(|e| e.identifier.starts_with(match_no_star)) - .map(|e| e.identifier.clone()); + .find(|e| e.identifier().starts_with(match_no_star)) + .map(|e| e.identifier().to_string()); } engines .iter() - .find(|e| e.identifier == engine_id_match) - .map(|e| e.identifier.clone()) + .find(|e| e.identifier() == engine_id_match) + .map(|e| e.identifier().to_string()) +} + +/// Converts raw remote settings records into typed v3 configuration records. +pub(crate) fn parse_v3_record_fields( + records: &[RemoteSettingsRecord], +) -> Result, Error> { + // TODO: Bug 1947241 - Find a way to avoid having to serialise the records + // back to strings and then deserialise them into the records that we want. + records + .iter() + .map(|record| { + Ok(serde_json::from_str(&serde_json::to_string( + &record.fields, + )?)?) + }) + .collect() } #[cfg(test)] diff --git a/components/search/src/selector.rs b/components/search/src/selector.rs index 105b60a154b..5a970886d75 100644 --- a/components/search/src/selector.rs +++ b/components/search/src/selector.rs @@ -7,9 +7,11 @@ use crate::configuration_overrides_types::JSONOverridesRecord; use crate::configuration_overrides_types::JSONSearchConfigurationOverrides; use crate::filter::filter_engine_configuration_impl; +use crate::filter::filter_engine_configuration_v3_impl; +use crate::filter::parse_v3_record_fields; use crate::{ - error::Error, JSONSearchConfiguration, RefinedSearchConfig, SearchApiResult, - SearchUserEnvironment, + error::Error, JSONSearchConfiguration, JSONSearchConfigurationV3, RefinedSearchConfig, + RefinedSearchConfigV3, SearchApiResult, SearchUserEnvironment, }; use error_support::handle_error; use parking_lot::Mutex; @@ -19,8 +21,10 @@ use std::sync::Arc; #[derive(Default)] pub(crate) struct SearchEngineSelectorInner { configuration: Option, + configuration_v3: Option, configuration_overrides: Option, search_config_client: Option>, + search_config_v3_client: Option>, search_config_overrides_client: Option>, } @@ -60,6 +64,20 @@ impl SearchEngineSelector { } } + /// Sets the RemoteSettingsService to use. The selector will create the + /// relevant remote settings client(s) from the service. + /// + /// # Params: + /// - `service`: The remote settings service instance for the application. + /// - `options`: The remote settings options to be passed to the client(s). + /// - `apply_engine_overrides`: Whether or not to apply overrides from + /// `search-config-v2-overrides` to the selected engines. Should be false unless the + /// application supports the click URL feature. + pub fn use_remote_settings_server_v3(self: Arc, service: &Arc) { + let mut inner = self.0.lock(); + inner.search_config_v3_client = Some(service.make_client("search-config-v3".to_string())); + } + /// Sets the search configuration from the given string. If the configuration /// string is unchanged since the last update, the cached configuration is /// reused to avoid unnecessary reprocessing. This helps optimize performance, @@ -74,6 +92,20 @@ impl SearchEngineSelector { Ok(()) } + /// Sets the search configuration from the given string. If the configuration + /// string is unchanged since the last update, the cached configuration is + /// reused to avoid unnecessary reprocessing. This helps optimize performance, + /// particularly during test runs where the same configuration may be used + /// repeatedly. + #[handle_error(Error)] + pub fn set_search_config_v3(self: Arc, configuration: String) -> SearchApiResult<()> { + if configuration.is_empty() { + return Err(Error::SearchConfigNotSpecified); + } + self.0.lock().configuration_v3 = serde_json::from_str(&configuration)?; + Ok(()) + } + #[handle_error(Error)] pub fn set_config_overrides(self: Arc, overrides: String) -> SearchApiResult<()> { if overrides.is_empty() { @@ -83,11 +115,6 @@ impl SearchEngineSelector { Ok(()) } - /// Clears the search configuration from memory if it is known that it is - /// not required for a time, e.g. if the configuration will only be re-filtered - /// after an app/environment update. - pub fn clear_search_config(self: Arc) {} - /// Filters the search configuration with the user's given environment, /// and returns the set of engines and parameters that should be presented /// to the user. @@ -153,6 +180,37 @@ impl SearchEngineSelector { }; return filter_engine_configuration_impl(user_environment, &config, Some(config_overrides)); } + + /// Filters the search configuration with the user's given environment, + /// and returns the set of engines and parameters that should be presented + /// to the user. + #[handle_error(Error)] + pub fn filter_engine_configuration_v3( + self: Arc, + user_environment: SearchUserEnvironment, + ) -> SearchApiResult { + let inner = self.0.lock(); + if let Some(client) = &inner.search_config_v3_client { + // Remote settings ships dumps of the collections, so it is highly + // unlikely that we'll ever hit the case where we have no records. + // However, just in case of an issue that does causes us to receive + // no records, we will raise an error so that the application can + // handle or record it appropriately. + match client.get_records(false) { + Some(records) if !records.is_empty() => { + let parsed_records = parse_v3_record_fields(&records)?; + return filter_engine_configuration_v3_impl(user_environment, &parsed_records); + } + _ => return Err(Error::SearchConfigNoRecords), + } + } + match &inner.configuration_v3 { + None => return Err(Error::SearchConfigNotSpecified), + Some(configuration) => { + filter_engine_configuration_v3_impl(user_environment, &configuration.data.clone()) + } + } + } } #[cfg(test)] @@ -182,6 +240,24 @@ mod tests { config_result.expect("Should have set the configuration successfully"); } + #[test] + fn test_set_config_v3_should_allow_basic_config() { + let selector = Arc::new(SearchEngineSelector::new()); + + let config = json!({ + "data": [ + EngineRecord::full("test1", "Test 1").build(), + { + "recordType": "defaultEngines", + "globalDefault": "test" + } + ] + }); + + let config_result = Arc::clone(&selector).set_search_config_v3(config.to_string()); + config_result.expect("Should have set the configuration v3 successfully"); + } + #[test] fn test_set_config_should_allow_extra_fields() { let selector = Arc::new(SearchEngineSelector::new()); @@ -209,6 +285,33 @@ mod tests { config_result.expect("Should have set the configuration successfully with extra fields"); } + #[test] + fn test_set_config_v3_should_allow_extra_fields() { + let selector = Arc::new(SearchEngineSelector::new()); + + let mut engine = EngineRecord::minimal("test", "Test").build(); + engine["base"]["urls"]["search"]["extraField1"] = json!(true); + engine["base"]["extraField2"] = json!("123"); + engine["extraField3"] = json!(["foo"]); + + let config_result = Arc::clone(&selector).set_search_config_v3( + json!({ + "data": [ + engine, + { + "recordType": "defaultEngines", + "globalDefault": "test", + "extraField4": { + "subField1": true + } + } + ] + }) + .to_string(), + ); + config_result.expect("Should have set the configuration v3 successfully with extra fields"); + } + #[test] fn test_set_config_should_ignore_unknown_record_types() { let selector = Arc::new(SearchEngineSelector::new()); @@ -230,6 +333,27 @@ mod tests { .expect("Should have set the configuration successfully with unknown record types."); } + #[test] + fn test_set_config_v3_should_ignore_unknown_record_types() { + let selector = Arc::new(SearchEngineSelector::new()); + let config = json!({ + "data": [ + EngineRecord::full("test1", "Test 1").build(), + { + "recordType": "defaultEngines", + "globalDefault": "test" + }, + { + "recordType": "unknown" + } + ] + }); + let config_result = Arc::clone(&selector).set_search_config_v3(config.to_string()); + + config_result + .expect("Should have set the configuration v3 successfully with unknown record types."); + } + #[test] fn test_filter_engine_configuration_throws_without_config() { let selector = Arc::new(SearchEngineSelector::new()); @@ -248,6 +372,24 @@ mod tests { .contains("Search configuration not specified")) } + #[test] + fn test_filter_engine_configuration_v3_throws_without_config() { + let selector = Arc::new(SearchEngineSelector::new()); + + let result = selector.filter_engine_configuration_v3(SearchUserEnvironment { + ..Default::default() + }); + + assert!( + result.is_err(), + "Should throw an error when a configuration v3 has not been specified before filtering" + ); + assert!(result + .unwrap_err() + .to_string() + .contains("Search configuration not specified")) + } + #[test] fn test_filter_engine_configuration_throws_without_config_overrides() { let selector = Arc::new(SearchEngineSelector::new()); @@ -321,6 +463,97 @@ mod tests { ) } + #[test] + fn test_filter_engine_configuration_v3_returns_basic_engines() { + let selector = Arc::new(SearchEngineSelector::new()); + + let config_result = Arc::clone(&selector).set_search_config_v3( + json!({ + "data": [ + EngineRecord::full("test1", "Test 1").build(), + EngineRecord::minimal("test2", "Test 2").build(), + { + "recordType": "defaultEngines", + "globalDefault": "test1", + "globalDefaultPrivate": "test2" + } + ] + }) + .to_string(), + ); + config_result.expect("Should have set the v3 configuration successfully"); + + let result = selector.filter_engine_configuration_v3(SearchUserEnvironment { + ..Default::default() + }); + + assert!( + result.is_ok(), + "Should have filtered the v3 configuration without error. {:?}", + result + ); + assert_eq!( + result.unwrap(), + RefinedSearchConfigV3 { + engines: vec!( + ExpectedEngine::full("test1", "Test 1").build().into(), + ExpectedEngine::minimal("test2", "Test 2").build().into(), + ), + app_default_engine_id: Some("test1".to_string()), + app_private_default_engine_id: Some("test2".to_string()) + } + ) + } + + #[test] + fn test_filter_engine_configuration_v2_and_v3_are_independent() { + let selector = Arc::new(SearchEngineSelector::new()); + + Arc::clone(&selector) + .set_search_config( + json!({ + "data": [ + EngineRecord::minimal("v2-engine", "V2 Engine").build(), + { "recordType": "defaultEngines", "globalDefault": "v2-engine" } + ] + }) + .to_string(), + ) + .expect("Should have set the v2 configuration successfully"); + Arc::clone(&selector) + .set_config_overrides(json!({ "data": [test_helpers::overrides_engine()] }).to_string()) + .expect("Should have set the v2 configuration overrides successfully"); + Arc::clone(&selector) + .set_search_config_v3( + json!({ + "data": [ + EngineRecord::minimal("v3-engine", "V3 Engine").build(), + { "recordType": "defaultEngines", "globalDefault": "v3-engine" } + ] + }) + .to_string(), + ) + .expect("Should have set the v3 configuration successfully"); + + let v2_result = Arc::clone(&selector) + .filter_engine_configuration(SearchUserEnvironment::default()) + .expect("Should have filtered the v2 configuration without error"); + let v3_result = selector + .filter_engine_configuration_v3(SearchUserEnvironment::default()) + .expect("Should have filtered the v3 configuration without error"); + + assert_eq!(v2_result.engines[0].identifier, "v2-engine"); + assert_eq!( + v2_result.app_default_engine_id, + Some("v2-engine".to_string()) + ); + assert_eq!(v3_result.engines[0].identifier, "v3-engine"); + assert_eq!( + v3_result.app_default_engine_id, + Some("v3-engine".to_string()) + ); + } + #[test] fn test_filter_engine_configuration_handles_basic_variants() { let selector = Arc::new(SearchEngineSelector::new()); @@ -896,6 +1129,36 @@ mod tests { selector } + fn setup_remote_settings_test_v3(expect_sync_successful: bool) -> Arc { + error_support::init_for_tests(); + viaduct_dev::init_backend_dev(); + + let config = RemoteSettingsConfig { + server: Some(RemoteSettingsServer::Custom { + url: mockito::server_url(), + }), + bucket_name: Some(String::from("main")), + app_context: Some(RemoteSettingsContext::default()), + }; + let service = Arc::new(RemoteSettingsService::new(String::from(":memory:"), config)); + + let selector = Arc::new(SearchEngineSelector::new()); + + Arc::clone(&selector).use_remote_settings_server_v3(&service); + let sync_result = Arc::clone(&service).sync(); + assert!( + if expect_sync_successful { + sync_result.is_ok() + } else { + sync_result.is_err() + }, + "Should have completed the sync successfully. {:?}", + sync_result + ); + + selector + } + fn mock_changes_endpoint() -> mockito::Mock { mock( "GET", @@ -908,10 +1171,10 @@ mod tests { .create() } - fn response_body() -> String { + fn response_body(collection: &str) -> String { json!({ "metadata": { - "id": "search-config-v2", + "id": collection, "last_modified": 1000, "bucket": "main", "signatures": [{ @@ -1084,7 +1347,54 @@ mod tests { assert!(result .unwrap_err() .to_string() - .contains("No search config v2 records received from remote settings")); + .contains("No search config records received from remote settings")); + changes_mock.expect(1).assert(); + m.expect(1).assert(); + } + + #[test] + fn test_remote_settings_empty_search_config_records_throws_error_v3() { + let changes_mock = mock_changes_endpoint(); + let m = mock( + "GET", + "/v2/buckets/main/collections/search-config-v3/changeset?_expected=0", + ) + .with_body( + json!({ + "metadata": { + "id": "search-config-v3", + "last_modified": 1000, + "bucket": "main", + "signatures": [{ + "x5u": "fake", + "signature": "fake", + "mode": "fake", + }], + }, + "timestamp": 1000, + "changes": [ + ]}) + .to_string(), + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_header("etag", "\"1000\"") + .create(); + + let selector = setup_remote_settings_test_v3(RECORDS_PRESENT); + + let result = Arc::clone(&selector).filter_engine_configuration_v3(SearchUserEnvironment { + distribution_id: "test-distro".to_string(), + ..Default::default() + }); + assert!( + result.is_err(), + "Should throw an error when a configuration has not been specified before filtering" + ); + assert!(result + .unwrap_err() + .to_string() + .contains("No search config records received from remote settings")); changes_mock.expect(1).assert(); m.expect(1).assert(); } @@ -1096,7 +1406,7 @@ mod tests { "GET", "/v2/buckets/main/collections/search-config-v2/changeset?_expected=0", ) - .with_body(response_body()) + .with_body(response_body("search-config-v2")) .with_status(501) .with_header("content-type", "application/json") .with_header("etag", "\"1000\"") @@ -1115,7 +1425,38 @@ mod tests { assert!(result .unwrap_err() .to_string() - .contains("No search config v2 records received from remote settings")); + .contains("No search config records received from remote settings")); + changes_mock.expect(1).assert(); + m1.expect(1).assert(); + } + + #[test] + fn test_remote_settings_search_config_records_is_none_throws_error_v3() { + let changes_mock = mock_changes_endpoint(); + let m1 = mock( + "GET", + "/v2/buckets/main/collections/search-config-v3/changeset?_expected=0", + ) + .with_body(response_body("search-config-v3")) + .with_status(501) + .with_header("content-type", "application/json") + .with_header("etag", "\"1000\"") + .create(); + + let selector = setup_remote_settings_test_v3(RECORDS_MISSING); + + let result = Arc::clone(&selector).filter_engine_configuration_v3(SearchUserEnvironment { + distribution_id: "test-distro".to_string(), + ..Default::default() + }); + assert!( + result.is_err(), + "Should throw an error when a configuration has not been specified before filtering" + ); + assert!(result + .unwrap_err() + .to_string() + .contains("No search config records received from remote settings")); changes_mock.expect(1).assert(); m1.expect(1).assert(); } @@ -1127,7 +1468,7 @@ mod tests { "GET", "/v2/buckets/main/collections/search-config-v2/changeset?_expected=0", ) - .with_body(response_body()) + .with_body(response_body("search-config-v2")) .with_status(200) .with_header("content-type", "application/json") .with_header("etag", "\"1000\"") @@ -1182,7 +1523,7 @@ mod tests { "GET", "/v2/buckets/main/collections/search-config-v2/changeset?_expected=0", ) - .with_body(response_body()) + .with_body(response_body("search-config-v2")) .with_status(200) .with_header("content-type", "application/json") .with_header("etag", "\"1000\"") @@ -1224,7 +1565,7 @@ mod tests { "GET", "/v2/buckets/main/collections/search-config-v2/changeset?_expected=0", ) - .with_body(response_body()) + .with_body(response_body("search-config-v2")) .with_status(200) .with_header("content-type", "application/json") .with_header("etag", "\"1000\"") @@ -1283,7 +1624,7 @@ mod tests { "GET", "/v2/buckets/main/collections/search-config-v2/changeset?_expected=0", ) - .with_body(response_body()) + .with_body(response_body("search-config-v2")) .with_status(200) .with_header("content-type", "application/json") .with_header("etag", "\"1000\"") @@ -1347,6 +1688,78 @@ mod tests { m.expect(1).assert(); } + #[test] + fn test_filter_v3_with_remote_settings() { + let changes_mock = mock_changes_endpoint(); + + let m = mock( + "GET", + "/v2/buckets/main/collections/search-config-v3/changeset?_expected=0", + ) + .with_body(response_body("search-config-v3")) + .with_status(200) + .with_header("content-type", "application/json") + .with_header("etag", "\"1000\"") + .create(); + + let selector = setup_remote_settings_test_v3(RECORDS_PRESENT); + + let test_engine = ExpectedEngine::minimal("test", "Test").build(); + let private_default_fr_engine = + ExpectedEngine::minimal("private-default-FR", "Private default FR").build(); + let distro_default_engine = + ExpectedEngine::minimal("distro-default", "Distribution Default").build(); + + let result = Arc::clone(&selector).filter_engine_configuration_v3(SearchUserEnvironment { + distribution_id: "test-distro".to_string(), + ..Default::default() + }); + assert!( + result.is_ok(), + "Should have filtered the configuration without error. {:?}", + result + ); + assert_eq!( + result.unwrap(), + RefinedSearchConfigV3 { + engines: vec![ + distro_default_engine.clone().into(), + private_default_fr_engine.clone().into(), + test_engine.clone().into(), + ], + app_default_engine_id: Some("distro-default".to_string()), + app_private_default_engine_id: None + }, + "Should have selected the default engine for the matching specific default" + ); + + let result = Arc::clone(&selector).filter_engine_configuration_v3(SearchUserEnvironment { + region: "fr".into(), + distribution_id: String::new(), + ..Default::default() + }); + assert!( + result.is_ok(), + "Should have filtered the configuration without error. {:?}", + result + ); + assert_eq!( + result.unwrap(), + RefinedSearchConfigV3 { + engines: vec![ + test_engine.into(), + private_default_fr_engine.into(), + distro_default_engine.into(), + ], + app_default_engine_id: Some("test".to_string()), + app_private_default_engine_id: Some("private-default-FR".to_string()) + }, + "Should have selected the private default engine for the matching specific default" + ); + changes_mock.expect(1).assert(); + m.expect(1).assert(); + } + #[test] fn test_filter_with_remote_settings_negotiate_locales() { let changes_mock = mock_changes_endpoint(); @@ -1399,6 +1812,62 @@ mod tests { m.expect(1).assert(); } + #[test] + fn test_filter_with_remote_settings_negotiate_locales_v3() { + let changes_mock = mock_changes_endpoint(); + let m = mock( + "GET", + "/v2/buckets/main/collections/search-config-v3/changeset?_expected=0", + ) + .with_body(response_body_locales()) + .with_status(200) + .with_header("content-type", "application/json") + .with_header("etag", "\"1000\"") + .create(); + + let selector = setup_remote_settings_test_v3(RECORDS_PRESENT); + + let result_de = + Arc::clone(&selector).filter_engine_configuration_v3(SearchUserEnvironment { + locale: "de-AT".into(), + ..Default::default() + }); + assert!( + result_de.is_ok(), + "Should have filtered the configuration without error. {:?}", + result_de + ); + + assert_eq!( + result_de.unwrap(), + RefinedSearchConfigV3 { + engines: vec![ExpectedEngine::minimal("engine-de", "German Engine") + .build() + .into()], + app_default_engine_id: None, + app_private_default_engine_id: None, + }, + "Should have selected the de engine when given de-AT which is not an available locale" + ); + + let result_en = + Arc::clone(&selector).filter_engine_configuration_v3(SearchUserEnvironment { + locale: "en-AU".to_string(), + ..Default::default() + }); + assert_eq!( + result_en.unwrap(), + RefinedSearchConfigV3 { + engines: vec![ExpectedEngine::minimal("engine-en-us", "English US Engine").build().into(),], + app_default_engine_id: None, + app_private_default_engine_id: None, + }, + "Should have selected the en-us engine when given another english locale we don't support" + ); + changes_mock.expect(1).assert(); + m.expect(1).assert(); + } + #[test] fn test_configuration_overrides_applied() { let selector = Arc::new(SearchEngineSelector::new()); @@ -1545,4 +2014,67 @@ mod tests { "Should have selected the en-us engine when given another english locale we don't support" ); } + + #[test] + fn test_filter_engine_configuration_negotiate_locales_v3() { + let selector = Arc::new(SearchEngineSelector::new()); + let config_result = Arc::clone(&selector).set_search_config_v3( + json!({ + "data": [ + { + "recordType": "availableLocales", + "locales": ["de", "en-US"] + }, + EngineRecord::minimal("engine-de", "German Engine") + .override_variants(Variant::new() + .locales(&["de"])) + .build(), + EngineRecord::minimal("engine-en-us", "English US Engine") + .override_variants(Variant::new() + .locales(&["en-US"])) + .build(), + ] + }) + .to_string(), + ); + config_result.expect("Should have set the configuration successfully"); + + let result_de = + Arc::clone(&selector).filter_engine_configuration_v3(SearchUserEnvironment { + locale: "de-AT".into(), + ..Default::default() + }); + assert!( + result_de.is_ok(), + "Should have filtered the configuration without error. {:?}", + result_de + ); + + assert_eq!( + result_de.unwrap(), + RefinedSearchConfigV3 { + engines: vec![ExpectedEngine::minimal("engine-de", "German Engine") + .build() + .into(),], + app_default_engine_id: None, + app_private_default_engine_id: None, + }, + "Should have selected the de engine when given de-AT which is not an available locale" + ); + + let result_en = + Arc::clone(&selector).filter_engine_configuration_v3(SearchUserEnvironment { + locale: "en-AU".to_string(), + ..Default::default() + }); + assert_eq!( + result_en.unwrap(), + RefinedSearchConfigV3 { + engines: vec![ExpectedEngine::minimal("engine-en-us", "English US Engine").build().into(),], + app_default_engine_id: None, + app_private_default_engine_id: None, + }, + "Should have selected the en-us engine when given another english locale we don't support" + ); + } } diff --git a/components/search/src/sort_helpers.rs b/components/search/src/sort_helpers.rs index 7b7e93f781c..c375d35e0d8 100644 --- a/components/search/src/sort_helpers.rs +++ b/components/search/src/sort_helpers.rs @@ -5,24 +5,24 @@ //! This module defines functions for sorting search engines based on priority //! and order hints, falling back to alphabetical sorting when neither is provided. -use crate::SearchEngineDefinition; +use crate::filter::EngineDefinition; -pub(crate) fn set_engine_order(engines: &mut [SearchEngineDefinition], ordered_engines: &[String]) { +pub(crate) fn set_engine_order(engines: &mut [E], ordered_engines: &[String]) { let mut order_number = ordered_engines.len(); for engine_id in ordered_engines { if let Some(found_engine) = find_engine_with_match_mut(engines, engine_id) { - found_engine.order_hint = Some(order_number as u32); + found_engine.set_order_hint(Some(order_number as u32)); order_number -= 1; } } } -pub(crate) fn sort( +pub(crate) fn sort( default_engine_id: Option<&String>, default_private_engine_id: Option<&String>, - a: &SearchEngineDefinition, - b: &SearchEngineDefinition, + a: &E, + b: &E, ) -> std::cmp::Ordering { let b_index = get_priority(b, default_engine_id, default_private_engine_id); let a_index = get_priority(a, default_engine_id, default_private_engine_id); @@ -32,42 +32,42 @@ pub(crate) fn sort( // See Bug 1945295: https://bugzilla.mozilla.org/show_bug.cgi?id=1945295 // If order is equal and order_hint is None for both, fall back to alphabetical sorting if order == std::cmp::Ordering::Equal { - return a.name.cmp(&b.name); + return a.name().cmp(b.name()); } order } -fn find_engine_with_match_mut<'a>( - engines: &'a mut [SearchEngineDefinition], +fn find_engine_with_match_mut<'a, E: EngineDefinition>( + engines: &'a mut [E], engine_id_match: &String, -) -> Option<&'a mut SearchEngineDefinition> { +) -> Option<&'a mut E> { if engine_id_match.is_empty() { return None; } if let Some(match_no_star) = engine_id_match.strip_suffix('*') { return engines .iter_mut() - .find(|e| e.identifier.starts_with(match_no_star)); + .find(|e| e.identifier().starts_with(match_no_star)); } engines .iter_mut() - .find(|e| e.identifier == *engine_id_match) + .find(|e| e.identifier() == *engine_id_match) } -fn get_priority( - engine: &SearchEngineDefinition, +fn get_priority( + engine: &E, default_engine_id: Option<&String>, default_private_engine_id: Option<&String>, ) -> u32 { - if Some(&engine.identifier) == default_engine_id { + if Some(engine.identifier()) == default_engine_id.map(String::as_str) { return u32::MAX; } - if Some(&engine.identifier) == default_private_engine_id { + if Some(engine.identifier()) == default_private_engine_id.map(String::as_str) { return u32::MAX - 1; } - engine.order_hint.unwrap_or(0) + engine.order_hint().unwrap_or(0) } #[cfg(test)] diff --git a/components/search/src/types.rs b/components/search/src/types.rs index 233c9f8f14f..fed8e57df05 100644 --- a/components/search/src/types.rs +++ b/components/search/src/types.rs @@ -245,6 +245,79 @@ pub struct SearchEngineDefinition { pub click_url: Option, } +/// A search-config-v3 definition for an individual search engine to be +/// presented to the user. +#[derive(Debug, uniffi::Record, PartialEq, Clone, Default)] +pub struct SearchEngineDefinitionV3 { + /// A list of aliases for this engine. + pub aliases: Vec, + + /// The character set this engine uses for queries. + pub charset: String, + + /// The classification of search engine according to the main search types + /// (e.g. general, shopping, travel, dictionary). Currently, only marking as + /// a general search engine is supported. + /// On Android, only general search engines may be selected as "default" + /// search engines. + pub classification: SearchEngineClassification, + + /// The identifier of the search engine. This is used as an internal + /// identifier, e.g. for saving the user's settings for the engine. It is + /// also used to form the base telemetry id and may be extended by telemetrySuffix. + pub identifier: String, + + /// Indicates the date until which the engine variant or subvariant is considered new + /// (format: YYYY-MM-DD). + pub is_new_until: Option, + + /// The user visible name of the search engine. + pub name: String, + + /// This search engine is presented as an option that the user may enable. + /// The application should not include these in the default list of the + /// user's engines. If not supported, it should filter them out. + pub optional: bool, + + /// The partner code for the engine. This will be inserted into parameters + /// which include `{partnerCode}`. May be the empty string. + pub partner_code: String, + + /// Optional suffix that is appended to the search engine identifier + /// following a dash, i.e. `-`. If it is an empty string + /// no dash should be appended. + pub telemetry_suffix: String, + + /// The URLs associated with the search engine. + pub urls: SearchEngineUrls, + + /// A hint to the order that this engine should be in the engine list. This + /// is derived from the `engineOrders` section of the search configuration. + /// The higher the number, the nearer to the front it should be. + /// If the number is not specified, other methods of sorting may be relied + /// upon (e.g. alphabetical). + pub order_hint: Option, +} + +/// Temporary helper to reduce work for handling the original and v3 types. +impl From for SearchEngineDefinitionV3 { + fn from(engine: SearchEngineDefinition) -> Self { + Self { + aliases: engine.aliases, + charset: engine.charset, + classification: engine.classification, + identifier: engine.identifier, + is_new_until: engine.is_new_until, + name: engine.name, + optional: engine.optional, + partner_code: engine.partner_code, + telemetry_suffix: engine.telemetry_suffix, + urls: engine.urls, + order_hint: engine.order_hint, + } + } +} + /// Details of the search engines to display to the user, generated as a result /// of processing the search configuration. #[derive(Debug, uniffi::Record, PartialEq)] @@ -275,3 +348,34 @@ pub struct RefinedSearchConfig { /// Only desktop uses this currently. pub app_private_default_engine_id: Option, } + +/// Details of the search engines to display to the user, generated as a result +/// of processing the search configuration v3. +#[derive(Debug, uniffi::Record, PartialEq)] +pub struct RefinedSearchConfigV3 { + /// A sorted list of engines. Clients may use the engine in the order that + /// this list is specified, or they may implement their own order if they + /// have other requirements. + /// + /// The application default engines should not be assumed from this order in + /// case of future changes. + /// + /// The sort order is: + /// + /// * Application Default Engine + /// * Application Default Engine for Private Mode (if specified & different) + /// * Engines sorted by descending `SearchEngineDefinition.orderHint` + /// * Any other engines in alphabetical order (locale based comparison) + pub engines: Vec, + + /// The identifier of the engine that should be used for the application + /// default engine. If this is undefined, an error has occurred, and the + /// application should either default to the first engine in the engines + /// list or otherwise handle appropriately. + pub app_default_engine_id: Option, + + /// If specified, the identifier of the engine that should be used for the + /// application default engine in private browsing mode. + /// Only desktop uses this currently. + pub app_private_default_engine_id: Option, +}