Skip to content

Commit

Permalink
Always nicely show errors from crates.io if possible
Browse files Browse the repository at this point in the history
Currently if Cargo ever gets a non-200 response, it will either not show
the error at all (if it was a 403 or 404), or spit out the entire
response body. Historically crates.io has served a 200 for most errors
to work around this, but we've stopped doing this as it causes problems
for other clients.

Additionally, we're starting to server more errors that have semantic
meaning (429 for rate limiting, 503 when we're in read only mode). If
the request specifies "Accept: application/json", we should ideally
return the errors formatted nicely. This isn't always true, but it's
what we'd like to do going forward.

While the output that Cargo puts out at least contains the actual
message, it's buried under a ton of useless info. This changes the
behavior so that if the response was valid JSON in the format that Cargo
expects, it just shows that (along with a description of the response
status), and only falls back to spitting out everything if it can't
parse the response body.

I'd love to add some more tests for this, but I've had trouble finding
anywhere in the test suite that exercises these paths.
  • Loading branch information
sgrif committed Mar 21, 2019
1 parent bd60ac8 commit 9759340
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 17 deletions.
1 change: 1 addition & 0 deletions src/crates-io/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ path = "lib.rs"
[dependencies]
curl = "0.4"
failure = "0.1.1"
http = "0.1"
serde = { version = "1.0", features = ['derive'] }
serde_derive = "1.0"
serde_json = "1.0"
Expand Down
36 changes: 19 additions & 17 deletions src/crates-io/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use std::io::Cursor;

use curl::easy::{Easy, List};
use failure::bail;
use http::status::StatusCode;
use serde::{Deserialize, Serialize};
use serde_json;
use url::percent_encoding::{percent_encode, QUERY_ENCODE_SET};
Expand Down Expand Up @@ -323,30 +324,31 @@ fn handle(handle: &mut Easy, read: &mut dyn FnMut(&mut [u8]) -> usize) -> Result
handle.perform()?;
}

match handle.response_code()? {
0 => {} // file upload url sometimes
200 => {}
403 => bail!("received 403 unauthorized response code"),
404 => bail!("received 404 not found response code"),
code => bail!(
let body = match String::from_utf8(body) {
Ok(body) => body,
Err(..) => bail!("response body was not valid utf-8"),
};
let errors = serde_json::from_str::<ApiErrorList>(&body).ok().map(|s| {
s.errors.into_iter().map(|s| s.detail).collect::<Vec<_>>()
});

match (handle.response_code()?, errors) {
(0, None) | (200, None) => {},
(code, Some(errors)) => {
let code = StatusCode::from_u16(code as _)?;
bail!("api errors (status {}): {}", code, errors.join(", "))
}
(code, None) => bail!(
"failed to get a 200 OK response, got {}\n\
headers:\n\
\t{}\n\
body:\n\
{}",
code,
headers.join("\n\t"),
String::from_utf8_lossy(&body)
code,
headers.join("\n\t"),
body,
),
}

let body = match String::from_utf8(body) {
Ok(body) => body,
Err(..) => bail!("response body was not valid utf-8"),
};
if let Ok(errors) = serde_json::from_str::<ApiErrorList>(&body) {
let errors = errors.errors.into_iter().map(|s| s.detail);
bail!("api errors: {}", errors.collect::<Vec<_>>().join(", "));
}
Ok(body)
}

0 comments on commit 9759340

Please sign in to comment.