Categorize crates.io API errors #2852
Conversation
github-actions
bot
added
the
S-waiting-on-review
|
Looks good to me, thanks! I'll let @syphar take a look as well. |
|
Thank you for the review @GuillaumeGomez. Let's see if @syphar wants anything changed. |
| //Error message to gracefully display | ||
| fn create_search_error_response(query: String, sort_by: String, error_message: String) -> Search { | ||
| Search { | ||
| title: format!("Search service is not currently available: {error_message}"), |
There was a problem hiding this comment.
please add a test that tests this new behaviour.
| @@ -233,6 +253,35 @@ async fn get_search_results( | |||
| }) | |||
| } | |||
|
|
|||
| // Categorize errors from registry | |||
| fn handle_registry_error(err: anyhow::Error) -> Result<SearchResult, SearchError> { | |||
There was a problem hiding this comment.
| fn handle_registry_error(err: anyhow::Error) -> Result<SearchResult, SearchError> { | |
| fn handle_registry_error(err: anyhow::Error) -> SearchError { |
the Ok variant will never be returned, right?
There was a problem hiding this comment.
also, we could put this into from impl From<anyhow::Error> for SearchError above?
There was a problem hiding this comment.
Note: The OK variant will never return.
Also, I don't feel comfortable with returning SearchError directly. While the suggestion makes it simpler, it could lead to issues down the line when the function would fail silently. I'd prefer this to stay explicit.
| && let Some(status) = registry_request_error.status() | ||
| && (status.is_client_error() || status.is_server_error()) | ||
| { | ||
| return Err(SearchError::CratesIo(format!( |
There was a problem hiding this comment.
looking at RegistryApi::search, I can see other errors using bail! that might also be interesting for the user.
what do you think? should we change RegistryApi::search to return SearchError too?
There was a problem hiding this comment.
Great suggestion. However, I have great reserve about this because RegistryApi might introduce new problems such as limited reusability, and how we might want to handle errors in different part of the codebase.
byfnoel
force-pushed
the
categorize-errors
branch
from
c697b6b to
5f81817
Compare
Co-authored-by: Denis Cornehl <denis@cornehl.org>
This PR improves error handling for queries to the
crates.ioAPI by categorizing error responses and updating how they are surfaced to users. Instead of logging 4xx or 5xx errors and reporting them to sentry, now we simply show them directly to the user.Previously, all errors from the crates.io API were logged and reported to Sentry. By surfacing these errors to users, we get to reduce noise in the tracking system and provide more descriptive responses.
Closes #2480