Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions components/merino/src/suggest/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand Down Expand Up @@ -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(),
Expand Down
20 changes: 8 additions & 12 deletions components/merino/src/suggest/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pub trait HttpClientTrait {
query: &str,
options: SuggestQueryParams<'_>,
endpoint_url: Url,
) -> Result<Response>;
) -> Result<Option<Response>>;
}

fn build_suggest_url(query: &str, options: &SuggestQueryParams<'_>, endpoint_url: Url) -> Url {
Expand Down Expand Up @@ -67,7 +67,7 @@ impl HttpClientTrait for HttpClient {
query: &str,
options: SuggestQueryParams<'_>,
endpoint_url: Url,
) -> Result<Response> {
) -> Result<Option<Response>> {
let url = build_suggest_url(query, &options, endpoint_url);

let client = Client::with_ohttp_channel("merino", ClientSettings::default())?;
Expand All @@ -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)),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep the error codes on the top level, for readability. The performance cost for unwrapping the .text() is very low here, and we could even do:

 match status {
      200 => Ok(Some(response)),
      204 => Ok(None),
      400 => Err(Error::BadRequest { code: status, message: response.text().to_string() }),
      422 => Err(Error::Validation { code: status, message: response.text().to_string() }),
      500..=599 => Err(Error::Server { code: status, message: response.text().to_string() }),
      _ => Err(Error::Unexpected { code: status, message: response.text().to_string() }),
  }

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(),
}),
}
}
Expand Down
13 changes: 9 additions & 4 deletions components/merino/src/suggest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> {
pub fn get_suggestions(
&self,
query: String,
options: SuggestOptions,
) -> ApiResult<Option<String>> {
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()))
}
}

Expand All @@ -102,7 +107,7 @@ impl<T: http::HttpClientTrait> SuggestClientInner<T> {
query: &str,
options: SuggestOptions,
endpoint_url: &Url,
) -> Result<viaduct::Response> {
) -> Result<Option<viaduct::Response>> {
let providers = options
.providers
.filter(|v| !v.is_empty())
Expand Down
46 changes: 36 additions & 10 deletions components/merino/src/suggest/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ impl http::HttpClientTrait for FakeHttpClientSuccess {
_query: &str,
_options: http::SuggestQueryParams<'_>,
_endpoint_url: Url,
) -> Result<Response> {
Ok(make_response(200, SAMPLE_RESPONSE))
) -> Result<Option<Response>> {
Ok(Some(make_response(200, SAMPLE_RESPONSE)))
}
}

Expand All @@ -70,7 +70,7 @@ impl http::HttpClientTrait for FakeHttpClientValidationError {
_query: &str,
_options: http::SuggestQueryParams<'_>,
_endpoint_url: Url,
) -> Result<Response> {
) -> Result<Option<Response>> {
Err(Error::Validation {
code: 422,
message: "Invalid input".to_string(),
Expand All @@ -86,7 +86,7 @@ impl http::HttpClientTrait for FakeHttpClientServerError {
_query: &str,
_options: http::SuggestQueryParams<'_>,
_endpoint_url: Url,
) -> Result<Response> {
) -> Result<Option<Response>> {
Err(Error::Server {
code: 500,
message: "Internal server error".to_string(),
Expand All @@ -102,14 +102,27 @@ impl http::HttpClientTrait for FakeHttpClientBadRequestError {
_query: &str,
_options: http::SuggestQueryParams<'_>,
_endpoint_url: Url,
) -> Result<Response> {
) -> Result<Option<Response>> {
Err(Error::BadRequest {
code: 400,
message: "Bad request".to_string(),
})
}
}

struct FakeHttpClientNoContent;

impl http::HttpClientTrait for FakeHttpClientNoContent {
fn make_suggest_request(
&self,
_query: &str,
_options: http::SuggestQueryParams<'_>,
_endpoint_url: Url,
) -> Result<Option<Response>> {
Ok(None)
}
}

struct FakeCapturingClient {
captured_url: std::sync::Arc<std::sync::Mutex<Option<Url>>>,
}
Expand All @@ -120,9 +133,9 @@ impl http::HttpClientTrait for FakeCapturingClient {
_query: &str,
_options: http::SuggestQueryParams<'_>,
endpoint_url: Url,
) -> Result<Response> {
) -> Result<Option<Response>> {
*self.captured_url.lock().unwrap() = Some(endpoint_url);
Ok(make_response(200, "{}"))
Ok(Some(make_response(200, "{}")))
}
}

Expand All @@ -136,7 +149,7 @@ impl http::HttpClientTrait for FakeCapturingClientWithParams {
query: &str,
options: http::SuggestQueryParams<'_>,
mut endpoint_url: Url,
) -> Result<Response> {
) -> Result<Option<Response>> {
{
let mut params = endpoint_url.query_pairs_mut();
params.append_pair("q", query);
Expand All @@ -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, "{}")))
}
}

Expand All @@ -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]
Expand Down
10 changes: 7 additions & 3 deletions examples/merino-cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)"),
}
}
}

Expand Down