Auto merge of #9213 - ehuss:registry-api-errors, r=alexcrichton

bors · bors · commit ccf781ab902d · 2021-03-01T15:13:36.000Z
Minor update to registry API error messages.

This is a minor update to the registry API errors, trying to make them a little clearer and more helpful. My concerns were:

* `api errors (status 200 OK): some error message` — why was there both an error and 200 OK?
* `api errors` — What is an "api error" anyways?  The user is probably not familiar with the "registry API".
* Adds the URL of the server it is actually talking to, in case you may be confused when using a registry other than crates.io. This also tries to make it clearer that the message is coming from the remote server, and not cargo itself.
diff --git a/crates/crates-io/lib.rs b/crates/crates-io/lib.rs
@@ -155,13 +155,13 @@ impl fmt::Display for ResponseError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
             ResponseError::Curl(e) => write!(f, "{}", e),
-            ResponseError::Api { code, errors } => write!(
-                f,
-                "api errors (status {} {}): {}",
-                code,
-                reason(*code),
-                errors.join(", ")
-            ),
+            ResponseError::Api { code, errors } => {
+                f.write_str("the remote server responded with an error")?;
+                if *code != 200 {
+                    write!(f, " (status {} {})", code, reason(*code))?;
+                };
+                write!(f, ": {}", errors.join(", "))
+            }
             ResponseError::Code {
                 code,
                 headers,
diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs
@@ -286,65 +286,62 @@ fn transmit(
         None => BTreeMap::new(),
     };
 
-    let publish = registry.publish(
-        &NewCrate {
-            name: pkg.name().to_string(),
-            vers: pkg.version().to_string(),
-            deps,
-            features: string_features,
-            authors: authors.clone(),
-            description: description.clone(),
-            homepage: homepage.clone(),
-            documentation: documentation.clone(),
-            keywords: keywords.clone(),
-            categories: categories.clone(),
-            readme: readme_content,
-            readme_file: readme.clone(),
-            repository: repository.clone(),
-            license: license.clone(),
-            license_file: license_file.clone(),
-            badges: badges.clone(),
-            links: links.clone(),
-            v: None,
-        },
-        tarball,
-    );
-
-    match publish {
-        Ok(warnings) => {
-            if !warnings.invalid_categories.is_empty() {
-                let msg = format!(
-                    "the following are not valid category slugs and were \
-                     ignored: {}. Please see https://crates.io/category_slugs \
-                     for the list of all category slugs. \
-                     ",
-                    warnings.invalid_categories.join(", ")
-                );
-                config.shell().warn(&msg)?;
-            }
-
-            if !warnings.invalid_badges.is_empty() {
-                let msg = format!(
-                    "the following are not valid badges and were ignored: {}. \
-                     Either the badge type specified is unknown or a required \
-                     attribute is missing. Please see \
-                     https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata \
-                     for valid badge types and their required attributes.",
-                    warnings.invalid_badges.join(", ")
-                );
-                config.shell().warn(&msg)?;
-            }
+    let warnings = registry
+        .publish(
+            &NewCrate {
+                name: pkg.name().to_string(),
+                vers: pkg.version().to_string(),
+                deps,
+                features: string_features,
+                authors: authors.clone(),
+                description: description.clone(),
+                homepage: homepage.clone(),
+                documentation: documentation.clone(),
+                keywords: keywords.clone(),
+                categories: categories.clone(),
+                readme: readme_content,
+                readme_file: readme.clone(),
+                repository: repository.clone(),
+                license: license.clone(),
+                license_file: license_file.clone(),
+                badges: badges.clone(),
+                links: links.clone(),
+                v: None,
+            },
+            tarball,
+        )
+        .chain_err(|| format!("failed to publish to registry at {}", registry.host()))?;
+
+    if !warnings.invalid_categories.is_empty() {
+        let msg = format!(
+            "the following are not valid category slugs and were \
+             ignored: {}. Please see https://crates.io/category_slugs \
+             for the list of all category slugs. \
+             ",
+            warnings.invalid_categories.join(", ")
+        );
+        config.shell().warn(&msg)?;
+    }
 
-            if !warnings.other.is_empty() {
-                for msg in warnings.other {
-                    config.shell().warn(&msg)?;
-                }
-            }
+    if !warnings.invalid_badges.is_empty() {
+        let msg = format!(
+            "the following are not valid badges and were ignored: {}. \
+             Either the badge type specified is unknown or a required \
+             attribute is missing. Please see \
+             https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata \
+             for valid badge types and their required attributes.",
+            warnings.invalid_badges.join(", ")
+        );
+        config.shell().warn(&msg)?;
+    }
 
-            Ok(())
+    if !warnings.other.is_empty() {
+        for msg in warnings.other {
+            config.shell().warn(&msg)?;
         }
-        Err(e) => Err(e),
     }
+
+    Ok(())
 }
 
 /// Returns the index and token from the config file for the given registry.
@@ -731,9 +728,9 @@ pub fn registry_login(
             input
                 .lock()
                 .read_line(&mut line)
-                .chain_err(|| "failed to read stdin")
-                .map_err(anyhow::Error::from)?;
-            // Automatically remove `cargo login` from an inputted token to allow direct pastes from `registry.host()`/me.
+                .chain_err(|| "failed to read stdin")?;
+            // Automatically remove `cargo login` from an inputted token to
+            // allow direct pastes from `registry.host()`/me.
             line.replace("cargo login", "").trim().to_string()
         }
     };
@@ -820,9 +817,13 @@ pub fn modify_owners(config: &Config, opts: &OwnersOptions) -> CargoResult<()> {
 
     if let Some(ref v) = opts.to_add {
         let v = v.iter().map(|s| &s[..]).collect::<Vec<_>>();
-        let msg = registry
-            .add_owners(&name, &v)
-            .map_err(|e| format_err!("failed to invite owners to crate {}: {}", name, e))?;
+        let msg = registry.add_owners(&name, &v).chain_err(|| {
+            format!(
+                "failed to invite owners to crate `{}` on registry at {}",
+                name,
+                registry.host()
+            )
+        })?;
 
         config.shell().status("Owner", msg)?;
     }
@@ -832,15 +833,23 @@ pub fn modify_owners(config: &Config, opts: &OwnersOptions) -> CargoResult<()> {
         config
             .shell()
             .status("Owner", format!("removing {:?} from crate {}", v, name))?;
-        registry
-            .remove_owners(&name, &v)
-            .chain_err(|| format!("failed to remove owners from crate {}", name))?;
+        registry.remove_owners(&name, &v).chain_err(|| {
+            format!(
+                "failed to remove owners from crate `{}` on registry at {}",
+                name,
+                registry.host()
+            )
+        })?;
     }
 
     if opts.list {
-        let owners = registry
-            .list_owners(&name)
-            .chain_err(|| format!("failed to list owners of crate {}", name))?;
+        let owners = registry.list_owners(&name).chain_err(|| {
+            format!(
+                "failed to list owners of crate `{}` on registry at {}",
+                name,
+                registry.host()
+            )
+        })?;
         for owner in owners.iter() {
             drop_print!(config, "{}", owner.login);
             match (owner.name.as_ref(), owner.email.as_ref()) {
@@ -882,16 +891,19 @@ pub fn yank(
         config
             .shell()
             .status("Unyank", format!("{}:{}", name, version))?;
-        registry
-            .unyank(&name, &version)
-            .chain_err(|| "failed to undo a yank")?;
+        registry.unyank(&name, &version).chain_err(|| {
+            format!(
+                "failed to undo a yank from the registry at {}",
+                registry.host()
+            )
+        })?;
     } else {
         config
             .shell()
             .status("Yank", format!("{}:{}", name, version))?;
         registry
             .yank(&name, &version)
-            .chain_err(|| "failed to yank")?;
+            .chain_err(|| format!("failed to yank from the registry at {}", registry.host()))?;
     }
 
     Ok(())
@@ -937,9 +949,12 @@ pub fn search(
     }
 
     let (mut registry, _, source_id) = registry(config, None, index, reg, false, false)?;
-    let (crates, total_crates) = registry
-        .search(query, limit)
-        .chain_err(|| "failed to retrieve search results from the registry")?;
+    let (crates, total_crates) = registry.search(query, limit).chain_err(|| {
+        format!(
+            "failed to retrieve search results from the registry at {}",
+            registry.host()
+        )
+    })?;
 
     let names = crates
         .iter()
diff --git a/tests/testsuite/owner.rs b/tests/testsuite/owner.rs
@@ -81,7 +81,10 @@ fn simple_add() {
         .with_status(101)
         .with_stderr(
             "    Updating `[..]` index
-error: failed to invite owners to crate foo: EOF while parsing a value at line 1 column 0",
+error: failed to invite owners to crate `foo` on registry at file://[..]
+
+Caused by:
+  EOF while parsing a value at line 1 column 0",
         )
         .run();
 }
@@ -111,7 +114,7 @@ fn simple_remove() {
         .with_stderr(
             "    Updating `[..]` index
        Owner removing [\"username\"] from crate foo
-error: failed to remove owners from crate foo
+error: failed to remove owners from crate `foo` on registry at file://[..]
 
 Caused by:
   EOF while parsing a value at line 1 column 0",
diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs
@@ -1490,7 +1490,56 @@ fn api_error_json() {
 [UPDATING] [..]
 [PACKAGING] foo v0.0.1 [..]
 [UPLOADING] foo v0.0.1 [..]
-[ERROR] api errors (status 403 Forbidden): you must be logged in
+[ERROR] failed to publish to registry at http://127.0.0.1:[..]/
+
+Caused by:
+  the remote server responded with an error (status 403 Forbidden): you must be logged in
+",
+        )
+        .run();
+
+    t.join().unwrap();
+}
+
+#[cargo_test]
+fn api_error_200() {
+    // Registry returns an API error with a 200 status code.
+    let t = registry::RegistryBuilder::new().build_api_server(&|_headers| {
+        (
+            200,
+            &r#"{"errors": [{"detail": "max upload size is 123"}]}"#,
+        )
+    });
+
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [project]
+                name = "foo"
+                version = "0.0.1"
+                authors = []
+                license = "MIT"
+                description = "foo"
+                documentation = "foo"
+                homepage = "foo"
+                repository = "foo"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("publish --no-verify --registry alternative")
+        .with_status(101)
+        .with_stderr(
+            "\
+[UPDATING] [..]
+[PACKAGING] foo v0.0.1 [..]
+[UPLOADING] foo v0.0.1 [..]
+[ERROR] failed to publish to registry at http://127.0.0.1:[..]/
+
+Caused by:
+  the remote server responded with an error: max upload size is 123
 ",
         )
         .run();
@@ -1528,13 +1577,16 @@ fn api_error_code() {
 [UPDATING] [..]
 [PACKAGING] foo v0.0.1 [..]
 [UPLOADING] foo v0.0.1 [..]
-[ERROR] failed to get a 200 OK response, got 400
-headers:
-<tab>HTTP/1.1 400
-<tab>Content-Length: 7
-<tab>
-body:
-go away
+[ERROR] failed to publish to registry at http://127.0.0.1:[..]/
+
+Caused by:
+  failed to get a 200 OK response, got 400
+  headers:
+  <tab>HTTP/1.1 400
+  <tab>Content-Length: 7
+  <tab>
+  body:
+  go away
 ",
         )
         .run();
@@ -1577,7 +1629,10 @@ fn api_curl_error() {
 [UPDATING] [..]
 [PACKAGING] foo v0.0.1 [..]
 [UPLOADING] foo v0.0.1 [..]
-[ERROR] [52] [..]
+[ERROR] failed to publish to registry at http://127.0.0.1:[..]/
+
+Caused by:
+  [52] [..]
 ",
         )
         .run();
@@ -1616,7 +1671,10 @@ fn api_other_error() {
 [UPDATING] [..]
 [PACKAGING] foo v0.0.1 [..]
 [UPLOADING] foo v0.0.1 [..]
-[ERROR] invalid response from server
+[ERROR] failed to publish to registry at http://127.0.0.1:[..]/
+
+Caused by:
+  invalid response from server
 
 Caused by:
   response body was not valid utf-8
diff --git a/tests/testsuite/yank.rs b/tests/testsuite/yank.rs
@@ -39,7 +39,7 @@ fn simple() {
         .with_stderr(
             "    Updating `[..]` index
       Unyank foo:0.0.1
-error: failed to undo a yank
+error: failed to undo a yank from the registry at file:///[..]
 
 Caused by:
   EOF while parsing a value at line 1 column 0",
