diff --git a/components/merino/src/suggest/error.rs b/components/merino/src/suggest/error.rs index 3647efaf22..b01901667c 100644 --- a/components/merino/src/suggest/error.rs +++ b/components/merino/src/suggest/error.rs @@ -7,10 +7,10 @@ use error_support::{ErrorHandling, GetErrorHandling}; pub type Result = std::result::Result; -pub type ApiResult = std::result::Result; +pub type ApiResult = std::result::Result; #[derive(Debug, thiserror::Error, uniffi::Error)] -pub enum SuggestApiError { +pub enum MerinoSuggestApiError { /// A network-level failure. #[error("Suggest network error: {reason}")] Network { reason: String }, @@ -52,11 +52,11 @@ pub enum Error { } impl GetErrorHandling for Error { - type ExternalError = SuggestApiError; + type ExternalError = MerinoSuggestApiError; fn get_error_handling(&self) -> ErrorHandling { match self { - Self::Request { .. } => ErrorHandling::convert(SuggestApiError::Network { + Self::Request { .. } => ErrorHandling::convert(MerinoSuggestApiError::Network { reason: self.to_string(), }) .log_warning(), @@ -65,13 +65,15 @@ impl GetErrorHandling for Error { | Self::Server { code, .. } | Self::Unexpected { code, .. } | Self::BadRequest { code, .. } - | Self::NoContent { code, .. } => ErrorHandling::convert(SuggestApiError::Other { - code: Some(*code), - reason: self.to_string(), - }) - .report_error("merino-http-error"), - - Self::UrlParse(_) => ErrorHandling::convert(SuggestApiError::Other { + | Self::NoContent { code, .. } => { + ErrorHandling::convert(MerinoSuggestApiError::Other { + code: Some(*code), + reason: self.to_string(), + }) + .report_error("merino-http-error") + } + + Self::UrlParse(_) => ErrorHandling::convert(MerinoSuggestApiError::Other { code: None, reason: self.to_string(), }) diff --git a/components/merino/src/suggest/mod.rs b/components/merino/src/suggest/mod.rs index d17fd138ca..680c892b2d 100644 --- a/components/merino/src/suggest/mod.rs +++ b/components/merino/src/suggest/mod.rs @@ -11,7 +11,7 @@ mod tests; use error_support::handle_error; use url::Url; -pub use error::{ApiResult, Error, Result, SuggestApiError}; +pub use error::{ApiResult, Error, MerinoSuggestApiError, Result}; pub use schema::{SuggestConfig, SuggestOptions}; const DEFAULT_BASE_HOST: &str = "https://merino.services.mozilla.com"; @@ -103,15 +103,23 @@ impl SuggestClientInner { options: SuggestOptions, endpoint_url: &Url, ) -> Result { + let providers = options + .providers + .filter(|v| !v.is_empty()) + .map(|v| v.join(",")); + let client_variants = options + .client_variants + .filter(|v| !v.is_empty()) + .map(|v| v.join(",")); self.http_client.make_suggest_request( query, http::SuggestQueryParams { - providers: options.providers.as_deref(), + providers: providers.as_deref(), source: options.source.as_deref(), country: options.country.as_deref(), region: options.region.as_deref(), city: options.city.as_deref(), - client_variants: options.client_variants.as_deref(), + client_variants: client_variants.as_deref(), request_type: options.request_type.as_deref(), accept_language: options.accept_language.as_deref(), }, diff --git a/components/merino/src/suggest/schema.rs b/components/merino/src/suggest/schema.rs index 77831d5f6c..be2963d3d9 100644 --- a/components/merino/src/suggest/schema.rs +++ b/components/merino/src/suggest/schema.rs @@ -11,8 +11,8 @@ pub struct SuggestConfig { /// All fields are optional — omitted fields are not sent to merino. #[derive(Clone, Debug, Record)] pub struct SuggestOptions { - /// Comma-separated list of suggestion providers to query (e.g. `"wikipedia,adm"`). - pub providers: Option, + /// List of suggestion providers to query (e.g. `["wikipedia", "adm"]`). + pub providers: Option>, /// Identifier of which part of firefox the request comes from (e.g. `"urlbar"`, `"newtab"`). pub source: Option, /// ISO 3166-1 country code (e.g. `"US"`). @@ -21,9 +21,9 @@ pub struct SuggestOptions { pub region: Option, /// City name (e.g. `"San Francisco"`). pub city: Option, - /// A comma-separated list of any experiments or rollouts that are affecting the client's Suggest experience. + /// List of any experiments or rollouts that are affecting the client's Suggest experience. /// If Merino recognizes any of them it will modify its behavior accordingly. - pub client_variants: Option, + pub client_variants: Option>, /// For AccuWeather provider, the request type should be either a "location" or "weather" string. For "location" it will get location completion suggestion. For "weather" it will return weather suggestions. /// If omitted, it defaults to weather suggestions. pub request_type: Option, diff --git a/components/merino/src/suggest/tests.rs b/components/merino/src/suggest/tests.rs index 65ded6a78b..cce9d0316a 100644 --- a/components/merino/src/suggest/tests.rs +++ b/components/merino/src/suggest/tests.rs @@ -126,6 +126,32 @@ impl http::HttpClientTrait for FakeCapturingClient { } } +struct FakeCapturingClientWithParams { + captured_url: std::sync::Arc>>, +} + +impl http::HttpClientTrait for FakeCapturingClientWithParams { + fn make_suggest_request( + &self, + query: &str, + options: http::SuggestQueryParams<'_>, + mut endpoint_url: Url, + ) -> Result { + { + let mut params = endpoint_url.query_pairs_mut(); + params.append_pair("q", query); + if let Some(v) = options.providers { + params.append_pair("providers", v); + } + if let Some(v) = options.client_variants { + params.append_pair("client_variants", v); + } + } + *self.captured_url.lock().unwrap() = Some(endpoint_url); + Ok(make_response(200, "{}")) + } +} + #[test] fn test_get_suggestions_success() { let client_inner = SuggestClientInner::new_with_client(FakeHttpClientSuccess); @@ -233,6 +259,58 @@ fn test_builder_uses_custom_base_host() { ); } +#[test] +fn test_providers_and_client_variants_joined_as_comma_separated() { + let captured_url = std::sync::Arc::new(std::sync::Mutex::new(None)); + let client_inner = SuggestClientInner::new_with_client(FakeCapturingClientWithParams { + captured_url: captured_url.clone(), + }); + + let options = SuggestOptions { + providers: Some(vec![ + "wikipedia".to_string(), + "adm".to_string(), + "accuweather".to_string(), + ]), + client_variants: Some(vec!["control".to_string(), "treatment".to_string()]), + ..default_options() + }; + + let endpoint = Url::parse("https://merino.services.mozilla.com/api/v1/suggest").unwrap(); + let _ = client_inner.get_suggestions("apple", options, &endpoint); + + let captured = captured_url.lock().unwrap(); + let url = captured.as_ref().unwrap(); + let params: std::collections::HashMap<_, _> = url.query_pairs().into_owned().collect(); + + assert_eq!(params["providers"], "wikipedia,adm,accuweather"); + assert_eq!(params["client_variants"], "control,treatment"); +} + +#[test] +fn test_empty_providers_and_client_variants_omitted() { + let captured_url = std::sync::Arc::new(std::sync::Mutex::new(None)); + let client_inner = SuggestClientInner::new_with_client(FakeCapturingClientWithParams { + captured_url: captured_url.clone(), + }); + + let options = SuggestOptions { + providers: Some(vec![]), + client_variants: Some(vec![]), + ..default_options() + }; + + let endpoint = Url::parse("https://merino.services.mozilla.com/api/v1/suggest").unwrap(); + let _ = client_inner.get_suggestions("apple", options, &endpoint); + + let captured = captured_url.lock().unwrap(); + let url = captured.as_ref().unwrap(); + let params: std::collections::HashMap<_, _> = url.query_pairs().into_owned().collect(); + + assert!(!params.contains_key("providers")); + assert!(!params.contains_key("client_variants")); +} + #[test] fn test_builder_fails_with_invalid_base_host() { let result = SuggestClientBuilder::new() diff --git a/examples/merino-cli/src/main.rs b/examples/merino-cli/src/main.rs index f312ba78fe..7d53f9015c 100644 --- a/examples/merino-cli/src/main.rs +++ b/examples/merino-cli/src/main.rs @@ -53,9 +53,9 @@ enum Commands { #[arg(long)] query: String, - /// Comma-separated list of providers (e.g. "wikipedia,adm") + /// List of providers (e.g. --providers wikipedia --providers adm) #[arg(long)] - providers: Option, + providers: Option>, /// Source identifier sent to Merino (e.g urlbar, new tab. defaults to unknown) #[arg(long)] @@ -73,9 +73,9 @@ enum Commands { #[arg(long)] city: Option, - /// Client variants + /// Client variants (e.g. --client-variants control --client-variants treatment) #[arg(long)] - client_variants: Option, + client_variants: Option>, /// Request type (e.g location | weather) #[arg(long)]