Auto merge of #6771 - sgrif:sg-always-show-errors-nicely, r=alexcrichton

Always nicely show errors from crates.io if possible

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.

src/crates-io/Cargo.toml

@@ -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"

src/crates-io/lib.rs

@@ -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};
@@ -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)
 }