diff --git a/components/merino/src/suggest/error.rs b/components/merino/src/suggest/error.rs index b01901667c..a4135b3493 100644 --- a/components/merino/src/suggest/error.rs +++ b/components/merino/src/suggest/error.rs @@ -30,10 +30,6 @@ pub enum Error { #[error("Error sending request: {0}")] Request(#[from] viaduct::ViaductError), - /// The server returned no content (HTTP 204), meaning no suggestions were available. - #[error("No content ({code}): {message}")] - NoContent { code: u16, message: String }, - /// The server rejected the request due to malformed syntax (HTTP 400). #[error("Bad request ({code}): {message}")] BadRequest { code: u16, message: String }, @@ -64,8 +60,7 @@ impl GetErrorHandling for Error { Self::Validation { code, .. } | Self::Server { code, .. } | Self::Unexpected { code, .. } - | Self::BadRequest { code, .. } - | Self::NoContent { code, .. } => { + | Self::BadRequest { code, .. } => { ErrorHandling::convert(MerinoSuggestApiError::Other { code: Some(*code), reason: self.to_string(), diff --git a/components/merino/src/suggest/http.rs b/components/merino/src/suggest/http.rs index 9779604a93..e24971485e 100644 --- a/components/merino/src/suggest/http.rs +++ b/components/merino/src/suggest/http.rs @@ -27,7 +27,7 @@ pub trait HttpClientTrait { query: &str, options: SuggestQueryParams<'_>, endpoint_url: Url, - ) -> Result; + ) -> Result>; } fn build_suggest_url(query: &str, options: &SuggestQueryParams<'_>, endpoint_url: Url) -> Url { @@ -67,7 +67,7 @@ impl HttpClientTrait for HttpClient { query: &str, options: SuggestQueryParams<'_>, endpoint_url: Url, - ) -> Result { + ) -> Result> { let url = build_suggest_url(query, &options, endpoint_url); let client = Client::with_ohttp_channel("merino", ClientSettings::default())?; @@ -81,28 +81,24 @@ impl HttpClientTrait for HttpClient { let response = client.send_sync(request)?; let status = response.status; - let message = response.text().to_string(); match status { - 200 => Ok(response), - 204 => Err(Error::NoContent { - code: status, - message, - }), + 200 => Ok(Some(response)), + 204 => Ok(None), 400 => Err(Error::BadRequest { code: status, - message, + message: response.text().to_string(), }), 422 => Err(Error::Validation { code: status, - message, + message: response.text().to_string(), }), 500..=599 => Err(Error::Server { code: status, - message, + message: response.text().to_string(), }), _ => Err(Error::Unexpected { code: status, - message, + message: response.text().to_string(), }), } } diff --git a/components/merino/src/suggest/mod.rs b/components/merino/src/suggest/mod.rs index 680c892b2d..f06ee0e015 100644 --- a/components/merino/src/suggest/mod.rs +++ b/components/merino/src/suggest/mod.rs @@ -77,14 +77,19 @@ impl SuggestClient { /// Fetches suggestions from the merino suggest endpoint for the given query. /// - /// Returns the raw JSON response body as a string. + /// Returns the raw JSON response body as a string, or `None` if the server + /// returned HTTP 204 (no suggestions available for weather). #[handle_error(Error)] - pub fn get_suggestions(&self, query: String, options: SuggestOptions) -> ApiResult { + pub fn get_suggestions( + &self, + query: String, + options: SuggestOptions, + ) -> ApiResult> { let response = self .inner .get_suggestions(query.as_str(), options, &self.endpoint_url)?; - Ok(response.text().to_string()) + Ok(response.map(|r| r.text().to_string())) } } @@ -102,7 +107,7 @@ impl SuggestClientInner { query: &str, options: SuggestOptions, endpoint_url: &Url, - ) -> Result { + ) -> Result> { let providers = options .providers .filter(|v| !v.is_empty()) diff --git a/components/merino/src/suggest/tests.rs b/components/merino/src/suggest/tests.rs index cce9d0316a..ddc8782b08 100644 --- a/components/merino/src/suggest/tests.rs +++ b/components/merino/src/suggest/tests.rs @@ -57,8 +57,8 @@ impl http::HttpClientTrait for FakeHttpClientSuccess { _query: &str, _options: http::SuggestQueryParams<'_>, _endpoint_url: Url, - ) -> Result { - Ok(make_response(200, SAMPLE_RESPONSE)) + ) -> Result> { + Ok(Some(make_response(200, SAMPLE_RESPONSE))) } } @@ -70,7 +70,7 @@ impl http::HttpClientTrait for FakeHttpClientValidationError { _query: &str, _options: http::SuggestQueryParams<'_>, _endpoint_url: Url, - ) -> Result { + ) -> Result> { Err(Error::Validation { code: 422, message: "Invalid input".to_string(), @@ -86,7 +86,7 @@ impl http::HttpClientTrait for FakeHttpClientServerError { _query: &str, _options: http::SuggestQueryParams<'_>, _endpoint_url: Url, - ) -> Result { + ) -> Result> { Err(Error::Server { code: 500, message: "Internal server error".to_string(), @@ -102,7 +102,7 @@ impl http::HttpClientTrait for FakeHttpClientBadRequestError { _query: &str, _options: http::SuggestQueryParams<'_>, _endpoint_url: Url, - ) -> Result { + ) -> Result> { Err(Error::BadRequest { code: 400, message: "Bad request".to_string(), @@ -110,6 +110,19 @@ impl http::HttpClientTrait for FakeHttpClientBadRequestError { } } +struct FakeHttpClientNoContent; + +impl http::HttpClientTrait for FakeHttpClientNoContent { + fn make_suggest_request( + &self, + _query: &str, + _options: http::SuggestQueryParams<'_>, + _endpoint_url: Url, + ) -> Result> { + Ok(None) + } +} + struct FakeCapturingClient { captured_url: std::sync::Arc>>, } @@ -120,9 +133,9 @@ impl http::HttpClientTrait for FakeCapturingClient { _query: &str, _options: http::SuggestQueryParams<'_>, endpoint_url: Url, - ) -> Result { + ) -> Result> { *self.captured_url.lock().unwrap() = Some(endpoint_url); - Ok(make_response(200, "{}")) + Ok(Some(make_response(200, "{}"))) } } @@ -136,7 +149,7 @@ impl http::HttpClientTrait for FakeCapturingClientWithParams { query: &str, options: http::SuggestQueryParams<'_>, mut endpoint_url: Url, - ) -> Result { + ) -> Result> { { let mut params = endpoint_url.query_pairs_mut(); params.append_pair("q", query); @@ -148,7 +161,7 @@ impl http::HttpClientTrait for FakeCapturingClientWithParams { } } *self.captured_url.lock().unwrap() = Some(endpoint_url); - Ok(make_response(200, "{}")) + Ok(Some(make_response(200, "{}"))) } } @@ -162,7 +175,20 @@ fn test_get_suggestions_success() { ); assert!(result.is_ok()); - assert_eq!(result.unwrap().text(), SAMPLE_RESPONSE); + assert_eq!(result.unwrap().unwrap().text(), SAMPLE_RESPONSE); +} + +#[test] +fn test_get_suggestions_no_content() { + let client_inner = SuggestClientInner::new_with_client(FakeHttpClientNoContent); + let result = client_inner.get_suggestions( + "apple", + default_options(), + &Url::parse("https://merino.services.mozilla.com/api/v1/suggest").unwrap(), + ); + + assert!(result.is_ok()); + assert!(result.unwrap().is_none()); } #[test] diff --git a/examples/merino-cli/src/main.rs b/examples/merino-cli/src/main.rs index 7d53f9015c..19e0d53366 100644 --- a/examples/merino-cli/src/main.rs +++ b/examples/merino-cli/src/main.rs @@ -146,9 +146,13 @@ fn main() -> Result<()> { request_type, accept_language, }; - let response = client.get_suggestions(query, options)?; - let json: serde_json::Value = serde_json::from_str(&response)?; - println!("{}", serde_json::to_string_pretty(&json)?); + match client.get_suggestions(query, options)? { + Some(response) => { + let json: serde_json::Value = serde_json::from_str(&response)?; + println!("{}", serde_json::to_string_pretty(&json)?); + } + None => println!("No suggestions available (204 No Content)"), + } } }