From 9759340078b98f6e49f28f1a09827196d87d1e85 Mon Sep 17 00:00:00 2001
From: Sean Griffin <sean@seantheprogrammer.com>
Date: Thu, 21 Mar 2019 12:10:43 -0600
Subject: [PATCH] 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 |  1 +
 src/crates-io/lib.rs     | 36 +++++++++++++++++++-----------------
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/src/crates-io/Cargo.toml b/src/crates-io/Cargo.toml
index 4d5cb9429b2..ca60d9036d9 100644
--- a/src/crates-io/Cargo.toml
+++ b/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"
diff --git a/src/crates-io/lib.rs b/src/crates-io/lib.rs
index 342442519a5..1b4d860d692 100644
--- a/src/crates-io/lib.rs
+++ b/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)
 }