[DISCO-4064] Resolve naming conflict and update suggest options

components/merino/src/suggest/error.rs

@@ -7,10 +7,10 @@ use error_support::{ErrorHandling, GetErrorHandling};
 
 pub type Result<T> = std::result::Result<T, Error>;
 
-pub type ApiResult<T> = std::result::Result<T, SuggestApiError>;
+pub type ApiResult<T> = std::result::Result<T, MerinoSuggestApiError>;
 
 #[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<Self::ExternalError> {
         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(),
             })

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<T: http::HttpClientTrait> SuggestClientInner<T> {
         options: SuggestOptions,
         endpoint_url: &Url,
     ) -> Result<viaduct::Response> {
+        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(),
             },

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<String>,
+    /// List of suggestion providers to query (e.g. `["wikipedia", "adm"]`).
+    pub providers: Option<Vec<String>>,
     /// Identifier of which part of firefox the request comes from (e.g. `"urlbar"`, `"newtab"`).
     pub source: Option<String>,
     /// ISO 3166-1 country code (e.g. `"US"`).
@@ -21,9 +21,9 @@ pub struct SuggestOptions {
     pub region: Option<String>,
     /// City name (e.g. `"San Francisco"`).
     pub city: Option<String>,
-    /// 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<String>,
+    pub client_variants: Option<Vec<String>>,
     /// 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<String>,

components/merino/src/suggest/tests.rs

@@ -126,6 +126,32 @@ impl http::HttpClientTrait for FakeCapturingClient {
     }
 }
 
+struct FakeCapturingClientWithParams {
+    captured_url: std::sync::Arc<std::sync::Mutex<Option<Url>>>,
+}
+
+impl http::HttpClientTrait for FakeCapturingClientWithParams {
+    fn make_suggest_request(
+        &self,
+        query: &str,
+        options: http::SuggestQueryParams<'_>,
+        mut endpoint_url: Url,
+    ) -> Result<Response> {
+        {
+            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()

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<String>,
+        providers: Option<Vec<String>>,
 
         /// 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<String>,
 
-        /// Client variants
+        /// Client variants (e.g. --client-variants control --client-variants treatment)
         #[arg(long)]
-        client_variants: Option<String>,
+        client_variants: Option<Vec<String>>,
 
         /// Request type (e.g location | weather)
         #[arg(long)]