From 340656e29d09c08d1427d03ea17294dafe43193b Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 28 Jan 2021 11:54:27 -0800 Subject: [PATCH 1/2] Add RegistryBuilder to help initializing test registries. The intent here is to make it more flexible to create different registry setups, and to reuse code a little more easily. --- crates/cargo-test-support/src/registry.rs | 242 +++++++++++++++++----- tests/testsuite/alt_registry.rs | 23 +- tests/testsuite/credential_process.rs | 79 +++---- tests/testsuite/install_upgrade.rs | 3 +- tests/testsuite/login.rs | 2 +- tests/testsuite/logout.rs | 1 + tests/testsuite/package.rs | 1 + tests/testsuite/patch.rs | 5 +- tests/testsuite/publish.rs | 4 +- tests/testsuite/rename_deps.rs | 3 +- tests/testsuite/rustdoc_extern_html.rs | 3 +- tests/testsuite/vendor.rs | 3 +- 12 files changed, 251 insertions(+), 118 deletions(-) diff --git a/crates/cargo-test-support/src/registry.rs b/crates/cargo-test-support/src/registry.rs index e2e3b03024d..dd52be0703d 100644 --- a/crates/cargo-test-support/src/registry.rs +++ b/crates/cargo-test-support/src/registry.rs @@ -5,9 +5,12 @@ use cargo::util::Sha256; use flate2::write::GzEncoder; use flate2::Compression; use std::collections::HashMap; +use std::fmt::Write as _; use std::fs::{self, File}; -use std::io::prelude::*; +use std::io::{BufRead, BufReader, Write}; +use std::net::TcpListener; use std::path::{Path, PathBuf}; +use std::thread; use tar::{Builder, Header}; use url::Url; @@ -70,6 +73,183 @@ pub fn generate_alt_dl_url(name: &str) -> String { format!("{}/{{crate}}/{{version}}/{{crate}}-{{version}}.crate", base) } +/// A builder for initializing registries. +pub struct RegistryBuilder { + /// If `true`, adds source replacement for crates.io to a registry on the filesystem. + replace_crates_io: bool, + /// If `true`, configures a registry named "alternative". + alternative: bool, + /// If set, sets the API url for the "alternative" registry. + /// This defaults to a directory on the filesystem. + alt_api_url: Option, + /// If `true`, configures `.cargo/credentials` with some tokens. + add_tokens: bool, +} + +impl RegistryBuilder { + pub fn new() -> RegistryBuilder { + RegistryBuilder { + replace_crates_io: true, + alternative: false, + alt_api_url: None, + add_tokens: true, + } + } + + /// Sets whether or not to replace crates.io with a registry on the filesystem. + /// Default is `true`. + pub fn replace_crates_io(&mut self, replace: bool) -> &mut Self { + self.replace_crates_io = replace; + self + } + + /// Sets whether or not to initialize an alternative registry named "alternative". + /// Default is `false`. + pub fn alternative(&mut self, alt: bool) -> &mut Self { + self.alternative = alt; + self + } + + /// Sets the API url for the "alternative" registry. + /// Defaults to a path on the filesystem ([`alt_api_path`]). + pub fn alternative_api_url(&mut self, url: &str) -> &mut Self { + self.alternative = true; + self.alt_api_url = Some(url.to_string()); + self + } + + /// Sets whether or not to initialize `.cargo/credentials` with some tokens. + /// Defaults to `true`. + pub fn add_tokens(&mut self, add: bool) -> &mut Self { + self.add_tokens = add; + self + } + + /// Initializes the registries. + pub fn build(&self) { + let config_path = paths::home().join(".cargo/config"); + if config_path.exists() { + panic!( + "{} already exists, the registry may only be initialized once, \ + and must be done before the config file is created", + config_path.display() + ); + } + t!(fs::create_dir_all(config_path.parent().unwrap())); + let mut config = String::new(); + if self.replace_crates_io { + write!( + &mut config, + " + [source.crates-io] + replace-with = 'dummy-registry' + + [source.dummy-registry] + registry = '{}' + ", + registry_url() + ) + .unwrap(); + } + if self.alternative { + write!( + config, + " + [registries.alternative] + index = '{}' + ", + alt_registry_url() + ) + .unwrap(); + } + t!(fs::write(&config_path, config)); + + if self.add_tokens { + let credentials = paths::home().join(".cargo/credentials"); + t!(fs::write( + &credentials, + r#" + [registry] + token = "api-token" + + [registries.alternative] + token = "api-token" + "# + )); + } + + if self.replace_crates_io { + init_registry( + registry_path(), + dl_url().into_string(), + api_url(), + api_path(), + ); + } + + if self.alternative { + init_registry( + alt_registry_path(), + alt_dl_url(), + self.alt_api_url + .as_ref() + .map_or_else(alt_api_url, |url| Url::parse(&url).expect("valid url")), + alt_api_path(), + ); + } + } + + /// Initializes the registries, and sets up an HTTP server for the + /// "alternative" registry. + /// + /// The given callback takes a `Vec` of headers when a request comes in. + /// The first entry should be the HTTP command, such as + /// `PUT /api/v1/crates/new HTTP/1.1`. + /// + /// The callback should return the HTTP code for the response, and the + /// response body. + /// + /// This method returns a `JoinHandle` which you should call + /// `.join().unwrap()` on before exiting the test. + pub fn build_api_server<'a>( + &mut self, + handler: &'static (dyn (Fn(Vec) -> (u32, &'a dyn AsRef<[u8]>)) + Sync), + ) -> thread::JoinHandle<()> { + let server = TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = server.local_addr().unwrap(); + let api_url = format!("http://{}", addr); + + self.replace_crates_io(false) + .alternative_api_url(&api_url) + .build(); + + let t = thread::spawn(move || { + let mut conn = BufReader::new(server.accept().unwrap().0); + let headers: Vec<_> = (&mut conn) + .lines() + .map(|s| s.unwrap()) + .take_while(|s| s.len() > 2) + .map(|s| s.trim().to_string()) + .collect(); + let (code, response) = handler(headers); + let response = response.as_ref(); + let stream = conn.get_mut(); + write!( + stream, + "HTTP/1.1 {}\r\n\ + Content-Length: {}\r\n\ + \r\n", + code, + response.len() + ) + .unwrap(); + stream.write_all(response).unwrap(); + }); + + t + } +} + /// A builder for creating a new package in a registry. /// /// This uses "source replacement" using an automatically generated @@ -162,70 +342,28 @@ pub struct Dependency { optional: bool, } +/// Initializes the on-disk registry and sets up the config so that crates.io +/// is replaced with the one on disk. pub fn init() { let config = paths::home().join(".cargo/config"); - t!(fs::create_dir_all(config.parent().unwrap())); if config.exists() { return; } - t!(fs::write( - &config, - format!( - r#" - [source.crates-io] - registry = 'https://wut' - replace-with = 'dummy-registry' - - [source.dummy-registry] - registry = '{reg}' - - [registries.alternative] - index = '{alt}' - "#, - reg = registry_url(), - alt = alt_registry_url() - ) - )); - let credentials = paths::home().join(".cargo/credentials"); - t!(fs::write( - &credentials, - r#" - [registry] - token = "api-token" - - [registries.alternative] - token = "api-token" - "# - )); + RegistryBuilder::new().build(); +} - // Initialize a new registry. - init_registry( - registry_path(), - dl_url().into_string(), - api_url(), - api_path(), - ); - - // Initialize an alternative registry. - init_registry( - alt_registry_path(), - alt_dl_url(), - alt_api_url(), - alt_api_path(), - ); +/// Variant of `init` that initializes the "alternative" registry. +pub fn alt_init() { + RegistryBuilder::new().alternative(true).build(); } +/// Creates a new on-disk registry. pub fn init_registry(registry_path: PathBuf, dl_url: String, api_url: Url, api_path: PathBuf) { // Initialize a new registry. repo(®istry_path) .file( "config.json", - &format!( - r#" - {{"dl":"{}","api":"{}"}} - "#, - dl_url, api_url - ), + &format!(r#"{{"dl":"{}","api":"{}"}}"#, dl_url, api_url), ) .build(); fs::create_dir_all(api_path.join("api/v1/crates")).unwrap(); diff --git a/tests/testsuite/alt_registry.rs b/tests/testsuite/alt_registry.rs index 748d5dcaaa5..336e978ffd8 100644 --- a/tests/testsuite/alt_registry.rs +++ b/tests/testsuite/alt_registry.rs @@ -8,6 +8,7 @@ use std::fs; #[cargo_test] fn depend_on_alt_registry() { + registry::alt_init(); let p = project() .file( "Cargo.toml", @@ -57,6 +58,7 @@ fn depend_on_alt_registry() { #[cargo_test] fn depend_on_alt_registry_depends_on_same_registry_no_index() { + registry::alt_init(); let p = project() .file( "Cargo.toml", @@ -99,6 +101,7 @@ fn depend_on_alt_registry_depends_on_same_registry_no_index() { #[cargo_test] fn depend_on_alt_registry_depends_on_same_registry() { + registry::alt_init(); let p = project() .file( "Cargo.toml", @@ -141,6 +144,7 @@ fn depend_on_alt_registry_depends_on_same_registry() { #[cargo_test] fn depend_on_alt_registry_depends_on_crates_io() { + registry::alt_init(); let p = project() .file( "Cargo.toml", @@ -185,7 +189,7 @@ fn depend_on_alt_registry_depends_on_crates_io() { #[cargo_test] fn registry_and_path_dep_works() { - registry::init(); + registry::alt_init(); let p = project() .file( @@ -219,7 +223,7 @@ fn registry_and_path_dep_works() { #[cargo_test] fn registry_incompatible_with_git() { - registry::init(); + registry::alt_init(); let p = project() .file( @@ -249,6 +253,7 @@ fn registry_incompatible_with_git() { #[cargo_test] fn cannot_publish_to_crates_io_with_registry_dependency() { + registry::alt_init(); let fakeio_path = paths::root().join("fake.io"); let fakeio_url = fakeio_path.into_url().unwrap(); let p = project() @@ -307,6 +312,7 @@ fn cannot_publish_to_crates_io_with_registry_dependency() { #[cargo_test] fn publish_with_registry_dependency() { + registry::alt_init(); let p = project() .file( "Cargo.toml", @@ -370,6 +376,7 @@ fn publish_with_registry_dependency() { #[cargo_test] fn alt_registry_and_crates_io_deps() { + registry::alt_init(); let p = project() .file( "Cargo.toml", @@ -415,7 +422,7 @@ fn alt_registry_and_crates_io_deps() { #[cargo_test] fn block_publish_due_to_no_token() { - registry::init(); + registry::alt_init(); let p = project().file("src/lib.rs", "").build(); fs::remove_file(paths::home().join(".cargo/credentials")).unwrap(); @@ -432,6 +439,7 @@ fn block_publish_due_to_no_token() { #[cargo_test] fn publish_to_alt_registry() { + registry::alt_init(); let p = project().file("src/main.rs", "fn main() {}").build(); // Setup the registry by publishing a package @@ -472,6 +480,7 @@ fn publish_to_alt_registry() { #[cargo_test] fn publish_with_crates_io_dep() { + registry::alt_init(); let p = project() .file( "Cargo.toml", @@ -537,7 +546,7 @@ fn publish_with_crates_io_dep() { #[cargo_test] fn passwords_in_registries_index_url_forbidden() { - registry::init(); + registry::alt_init(); let config = paths::home().join(".cargo/config"); @@ -567,6 +576,7 @@ Caused by: #[cargo_test] fn patch_alt_reg() { + registry::alt_init(); Package::new("bar", "0.1.0").publish(); let p = project() .file( @@ -656,6 +666,7 @@ Caused by: #[cargo_test] fn no_api() { + registry::alt_init(); Package::new("bar", "0.0.1").alternative(true).publish(); // Configure without `api`. let repo = git2::Repository::open(registry::alt_registry_path()).unwrap(); @@ -739,6 +750,7 @@ fn no_api() { #[cargo_test] fn alt_reg_metadata() { // Check for "registry" entries in `cargo metadata` with alternative registries. + registry::alt_init(); let p = project() .file( "Cargo.toml", @@ -1033,6 +1045,7 @@ fn alt_reg_metadata() { fn unknown_registry() { // A known registry refers to an unknown registry. // foo -> bar(crates.io) -> baz(alt) + registry::alt_init(); let p = project() .file( "Cargo.toml", @@ -1188,6 +1201,7 @@ fn unknown_registry() { #[cargo_test] fn registries_index_relative_url() { + registry::alt_init(); let config = paths::root().join(".cargo/config"); fs::create_dir_all(config.parent().unwrap()).unwrap(); fs::write( @@ -1237,6 +1251,7 @@ fn registries_index_relative_url() { #[cargo_test] fn registries_index_relative_path_not_allowed() { + registry::alt_init(); let config = paths::root().join(".cargo/config"); fs::create_dir_all(config.parent().unwrap()).unwrap(); fs::write( diff --git a/tests/testsuite/credential_process.rs b/tests/testsuite/credential_process.rs index 8360ae4c627..69d839aa85e 100644 --- a/tests/testsuite/credential_process.rs +++ b/tests/testsuite/credential_process.rs @@ -1,12 +1,8 @@ //! Tests for credential-process. -use cargo_test_support::paths::CargoPathExt; use cargo_test_support::{basic_manifest, cargo_process, paths, project, registry, Project}; use std::fs; -use std::io::{BufRead, BufReader, Write}; -use std::net::TcpListener; use std::thread; -use url::Url; fn toml_bin(proj: &Project, name: &str) -> String { proj.bin(name).display().to_string().replace('\\', "\\\\") @@ -14,9 +10,10 @@ fn toml_bin(proj: &Project, name: &str) -> String { #[cargo_test] fn gated() { - registry::init(); - - paths::home().join(".cargo/credentials").rm_rf(); + registry::RegistryBuilder::new() + .alternative(true) + .add_tokens(false) + .build(); let p = project() .file( @@ -64,8 +61,10 @@ fn gated() { #[cargo_test] fn warn_both_token_and_process() { // Specifying both credential-process and a token in config should issue a warning. - registry::init(); - paths::home().join(".cargo/credentials").rm_rf(); + registry::RegistryBuilder::new() + .alternative(true) + .add_tokens(false) + .build(); let p = project() .file( ".cargo/config", @@ -138,38 +137,16 @@ Only one of these values may be set, remove one or the other to proceed. /// Returns a thread handle for the API server, the test should join it when /// finished. Also returns the simple `foo` project to test against. fn get_token_test() -> (Project, thread::JoinHandle<()>) { - let server = TcpListener::bind("127.0.0.1:0").unwrap(); - let addr = server.local_addr().unwrap(); - let api_url = format!("http://{}", addr); - - registry::init_registry( - registry::alt_registry_path(), - registry::alt_dl_url(), - Url::parse(&api_url).unwrap(), - registry::alt_api_path(), - ); - // API server that checks that the token is included correctly. - let t = thread::spawn(move || { - let mut conn = BufReader::new(server.accept().unwrap().0); - let headers: Vec<_> = (&mut conn) - .lines() - .map(|s| s.unwrap()) - .take_while(|s| s.len() > 2) - .map(|s| s.trim().to_string()) - .collect(); - assert!(headers - .iter() - .any(|header| header == "Authorization: sekrit")); - conn.get_mut() - .write_all( - b"HTTP/1.1 200\r\n\ - Content-Length: 33\r\n\ - \r\n\ - {\"ok\": true, \"msg\": \"completed!\"}\r\n", - ) - .unwrap(); - }); + let server = registry::RegistryBuilder::new() + .add_tokens(false) + .build_api_server(&|headers| { + assert!(headers + .iter() + .any(|header| header == "Authorization: sekrit")); + + (200, &r#"{"ok": true, "msg": "completed!"}"#) + }); // The credential process to use. let cred_proj = project() @@ -206,7 +183,7 @@ fn get_token_test() -> (Project, thread::JoinHandle<()>) { ) .file("src/lib.rs", "") .build(); - (p, t) + (p, server) } #[cargo_test] @@ -231,10 +208,7 @@ fn publish() { #[cargo_test] fn basic_unsupported() { // Non-action commands don't support login/logout. - registry::init(); - // If both `credential-process` and `token` are specified, it will ignore - // `credential-process`, so remove the default tokens. - paths::home().join(".cargo/credentials").rm_rf(); + registry::RegistryBuilder::new().add_tokens(false).build(); cargo::util::paths::append( &paths::home().join(".cargo/config"), br#" @@ -327,10 +301,7 @@ fn login() { #[cargo_test] fn logout() { - registry::init(); - // If both `credential-process` and `token` are specified, it will ignore - // `credential-process`, so remove the default tokens. - paths::home().join(".cargo/credentials").rm_rf(); + registry::RegistryBuilder::new().add_tokens(false).build(); // The credential process to use. let cred_proj = project() .at("cred_proj") @@ -418,9 +389,7 @@ fn owner() { #[cargo_test] fn libexec_path() { // cargo: prefixed names use the sysroot - registry::init(); - - paths::home().join(".cargo/credentials").rm_rf(); + registry::RegistryBuilder::new().add_tokens(false).build(); cargo::util::paths::append( &paths::home().join(".cargo/config"), br#" @@ -448,8 +417,10 @@ Caused by: #[cargo_test] fn invalid_token_output() { // Error when credential process does not output the expected format for a token. - registry::init(); - paths::home().join(".cargo/credentials").rm_rf(); + registry::RegistryBuilder::new() + .alternative(true) + .add_tokens(false) + .build(); let cred_proj = project() .at("cred_proj") .file("Cargo.toml", &basic_manifest("test-cred", "1.0.0")) diff --git a/tests/testsuite/install_upgrade.rs b/tests/testsuite/install_upgrade.rs index 7d3c8beac89..4fa8c154c40 100644 --- a/tests/testsuite/install_upgrade.rs +++ b/tests/testsuite/install_upgrade.rs @@ -9,7 +9,7 @@ use std::sync::atomic::{AtomicUsize, Ordering}; use cargo_test_support::install::{cargo_home, exe}; use cargo_test_support::paths::CargoPathExt; -use cargo_test_support::registry::Package; +use cargo_test_support::registry::{self, Package}; use cargo_test_support::{ basic_manifest, cargo_process, cross_compile, execs, git, process, project, Execs, }; @@ -549,6 +549,7 @@ fn upgrade_git() { fn switch_sources() { // Installing what appears to be the same thing, but from different // sources should reinstall. + registry::alt_init(); pkg("foo", "1.0.0"); Package::new("foo", "1.0.0") .file("src/main.rs", r#"fn main() { println!("alt"); }"#) diff --git a/tests/testsuite/login.rs b/tests/testsuite/login.rs index c226c07a57c..835461af8de 100644 --- a/tests/testsuite/login.rs +++ b/tests/testsuite/login.rs @@ -145,7 +145,7 @@ fn new_credentials_is_used_instead_old() { #[cargo_test] fn registry_credentials() { - registry::init(); + registry::alt_init(); let config = paths::home().join(".cargo/config"); let mut f = OpenOptions::new().append(true).open(config).unwrap(); diff --git a/tests/testsuite/logout.rs b/tests/testsuite/logout.rs index 041d3fb5976..606a06c8484 100644 --- a/tests/testsuite/logout.rs +++ b/tests/testsuite/logout.rs @@ -78,5 +78,6 @@ fn default_registry() { #[cargo_test] fn other_registry() { + registry::alt_init(); simple_logout_test(Some("alternative"), "--registry alternative"); } diff --git a/tests/testsuite/package.rs b/tests/testsuite/package.rs index 2dac71825e9..cbe998e82fb 100644 --- a/tests/testsuite/package.rs +++ b/tests/testsuite/package.rs @@ -818,6 +818,7 @@ to proceed despite this and include the uncommitted changes, pass the `--allow-d #[cargo_test] fn generated_manifest() { + registry::alt_init(); Package::new("abc", "1.0.0").publish(); Package::new("def", "1.0.0").alternative(true).publish(); Package::new("ghi", "1.0.0").publish(); diff --git a/tests/testsuite/patch.rs b/tests/testsuite/patch.rs index 8e2a3b1a305..0890b1e8565 100644 --- a/tests/testsuite/patch.rs +++ b/tests/testsuite/patch.rs @@ -2,7 +2,7 @@ use cargo_test_support::git; use cargo_test_support::paths; -use cargo_test_support::registry::Package; +use cargo_test_support::registry::{self, Package}; use cargo_test_support::{basic_manifest, project}; use std::fs; @@ -1543,6 +1543,7 @@ fn update_unused_new_version() { #[cargo_test] fn too_many_matches() { // The patch locations has multiple versions that match. + registry::alt_init(); Package::new("bar", "0.1.0").publish(); Package::new("bar", "0.1.0").alternative(true).publish(); Package::new("bar", "0.1.1").alternative(true).publish(); @@ -1866,6 +1867,7 @@ fn patched_dep_new_version() { fn patch_update_doesnt_update_other_sources() { // Very extreme edge case, make sure a patch update doesn't update other // sources. + registry::alt_init(); Package::new("bar", "0.1.0").publish(); Package::new("bar", "0.1.0").alternative(true).publish(); @@ -1931,6 +1933,7 @@ fn patch_update_doesnt_update_other_sources() { #[cargo_test] fn can_update_with_alt_reg() { // A patch to an alt reg can update. + registry::alt_init(); Package::new("bar", "0.1.0").publish(); Package::new("bar", "0.1.0").alternative(true).publish(); Package::new("bar", "0.1.1").alternative(true).publish(); diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index 02bb318e7f8..7597384acd5 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -677,7 +677,7 @@ The registry `alternative` is not listed in the `publish` value in Cargo.toml. #[cargo_test] fn publish_allowed_registry() { - registry::init(); + registry::alt_init(); let p = project().build(); @@ -717,7 +717,7 @@ fn publish_allowed_registry() { #[cargo_test] fn publish_implicitly_to_only_allowed_registry() { - registry::init(); + registry::alt_init(); let p = project().build(); diff --git a/tests/testsuite/rename_deps.rs b/tests/testsuite/rename_deps.rs index 16d15994b2e..fc3d11d2737 100644 --- a/tests/testsuite/rename_deps.rs +++ b/tests/testsuite/rename_deps.rs @@ -2,7 +2,7 @@ use cargo_test_support::git; use cargo_test_support::paths; -use cargo_test_support::registry::Package; +use cargo_test_support::registry::{self, Package}; use cargo_test_support::{basic_manifest, project}; #[cargo_test] @@ -66,6 +66,7 @@ fn rename_with_different_names() { #[cargo_test] fn lots_of_names() { + registry::alt_init(); Package::new("foo", "0.1.0") .file("src/lib.rs", "pub fn foo1() {}") .publish(); diff --git a/tests/testsuite/rustdoc_extern_html.rs b/tests/testsuite/rustdoc_extern_html.rs index b5b970320dd..820cf7bd644 100644 --- a/tests/testsuite/rustdoc_extern_html.rs +++ b/tests/testsuite/rustdoc_extern_html.rs @@ -1,6 +1,6 @@ //! Tests for the -Zrustdoc-map feature. -use cargo_test_support::registry::Package; +use cargo_test_support::registry::{self, Package}; use cargo_test_support::{is_nightly, paths, project, Project}; fn basic_project() -> Project { @@ -215,6 +215,7 @@ fn alt_registry() { // --extern-html-root-url is unstable return; } + registry::alt_init(); Package::new("bar", "1.0.0") .alternative(true) .file( diff --git a/tests/testsuite/vendor.rs b/tests/testsuite/vendor.rs index 5d081d79902..e55fe414267 100644 --- a/tests/testsuite/vendor.rs +++ b/tests/testsuite/vendor.rs @@ -7,7 +7,7 @@ use std::fs; use cargo_test_support::git; -use cargo_test_support::registry::Package; +use cargo_test_support::registry::{self, Package}; use cargo_test_support::{basic_lib_manifest, paths, project, Project}; #[cargo_test] @@ -594,6 +594,7 @@ fn ignore_hidden() { #[cargo_test] fn config_instructions_works() { // Check that the config instructions work for all dependency kinds. + registry::alt_init(); Package::new("dep", "0.1.0").publish(); Package::new("altdep", "0.1.0").alternative(true).publish(); let git_project = git::new("gitdep", |project| { From 06b8d1cac18209c7655bb7e94bc36b59b7f269ef Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 28 Jan 2021 12:39:51 -0800 Subject: [PATCH 2/2] Add a ResponseError for registry API interaction. The intent here is to make it easier to work with API errors. This also includes some new tests for checking network errors. --- crates/crates-io/lib.rs | 138 ++++++++++++++++++++++-------- tests/testsuite/publish.rs | 169 +++++++++++++++++++++++++++++++++++++ 2 files changed, 274 insertions(+), 33 deletions(-) diff --git a/crates/crates-io/lib.rs b/crates/crates-io/lib.rs index 6f6769eaf04..3f76b4b3451 100644 --- a/crates/crates-io/lib.rs +++ b/crates/crates-io/lib.rs @@ -2,12 +2,13 @@ #![allow(clippy::identity_op)] // used for vertical alignment use std::collections::BTreeMap; +use std::fmt; use std::fs::File; use std::io::prelude::*; use std::io::{Cursor, SeekFrom}; use std::time::Instant; -use anyhow::{bail, Context, Result}; +use anyhow::{bail, format_err, Context, Result}; use curl::easy::{Easy, List}; use percent_encoding::{percent_encode, NON_ALPHANUMERIC}; use serde::{Deserialize, Serialize}; @@ -121,6 +122,70 @@ struct Crates { crates: Vec, meta: TotalCrates, } + +#[derive(Debug)] +pub enum ResponseError { + Curl(curl::Error), + Api { + code: u32, + errors: Vec, + }, + Code { + code: u32, + headers: Vec, + body: String, + }, + Other(anyhow::Error), +} + +impl std::error::Error for ResponseError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + ResponseError::Curl(..) => None, + ResponseError::Api { .. } => None, + ResponseError::Code { .. } => None, + ResponseError::Other(e) => Some(e.as_ref()), + } + } +} + +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::Code { + code, + headers, + body, + } => write!( + f, + "failed to get a 200 OK response, got {}\n\ + headers:\n\ + \t{}\n\ + body:\n\ + {}", + code, + headers.join("\n\t"), + body + ), + ResponseError::Other(..) => write!(f, "invalid response from server"), + } + } +} + +impl From for ResponseError { + fn from(error: curl::Error) -> Self { + ResponseError::Curl(error) + } +} + impl Registry { /// Creates a new `Registry`. /// @@ -214,7 +279,25 @@ impl Registry { headers.append(&format!("Authorization: {}", token))?; self.handle.http_headers(headers)?; - let body = self.handle(&mut |buf| body.read(buf).unwrap_or(0))?; + let started = Instant::now(); + let body = self + .handle(&mut |buf| body.read(buf).unwrap_or(0)) + .map_err(|e| match e { + ResponseError::Code { code, .. } + if code == 503 + && started.elapsed().as_secs() >= 29 + && self.host_is_crates_io() => + { + format_err!( + "Request timed out after 30 seconds. If you're trying to \ + upload a crate it may be too large. If the crate is under \ + 10MB in size, you can email help@crates.io for assistance.\n\ + Total size was {}.", + tarball_len + ) + } + _ => e.into(), + })?; let response = if body.is_empty() { "{}".parse()? @@ -308,15 +391,18 @@ impl Registry { self.handle.upload(true)?; self.handle.in_filesize(body.len() as u64)?; self.handle(&mut |buf| body.read(buf).unwrap_or(0)) + .map_err(|e| e.into()) } - None => self.handle(&mut |_| 0), + None => self.handle(&mut |_| 0).map_err(|e| e.into()), } } - fn handle(&mut self, read: &mut dyn FnMut(&mut [u8]) -> usize) -> Result { + fn handle( + &mut self, + read: &mut dyn FnMut(&mut [u8]) -> usize, + ) -> std::result::Result { let mut headers = Vec::new(); let mut body = Vec::new(); - let started; { let mut handle = self.handle.transfer(); handle.read_function(|buf| Ok(read(buf)))?; @@ -325,50 +411,36 @@ impl Registry { Ok(data.len()) })?; handle.header_function(|data| { - headers.push(String::from_utf8_lossy(data).into_owned()); + // Headers contain trailing \r\n, trim them to make it easier + // to work with. + let s = String::from_utf8_lossy(data).trim().to_string(); + headers.push(s); true })?; - started = Instant::now(); handle.perform()?; } let body = match String::from_utf8(body) { Ok(body) => body, - Err(..) => bail!("response body was not valid utf-8"), + Err(..) => { + return Err(ResponseError::Other(format_err!( + "response body was not valid utf-8" + ))) + } }; let errors = serde_json::from_str::(&body) .ok() .map(|s| s.errors.into_iter().map(|s| s.detail).collect::>()); match (self.handle.response_code()?, errors) { - (0, None) | (200, None) => {} - (503, None) if started.elapsed().as_secs() >= 29 && self.host_is_crates_io() => bail!( - "Request timed out after 30 seconds. If you're trying to \ - upload a crate it may be too large. If the crate is under \ - 10MB in size, you can email help@crates.io for assistance." - ), - (code, Some(errors)) => { - let reason = reason(code); - bail!( - "api errors (status {} {}): {}", - code, - reason, - errors.join(", ") - ) - } - (code, None) => bail!( - "failed to get a 200 OK response, got {}\n\ - headers:\n\ - \t{}\n\ - body:\n\ - {}", + (0, None) | (200, None) => Ok(body), + (code, Some(errors)) => Err(ResponseError::Api { code, errors }), + (code, None) => Err(ResponseError::Code { code, - headers.join("\n\t"), + headers, body, - ), + }), } - - Ok(body) } } diff --git a/tests/testsuite/publish.rs b/tests/testsuite/publish.rs index 7597384acd5..b0a0c547b2e 100644 --- a/tests/testsuite/publish.rs +++ b/tests/testsuite/publish.rs @@ -1457,3 +1457,172 @@ Caused by: )) .run(); } + +#[cargo_test] +fn api_error_json() { + // Registry returns an API error. + let t = registry::RegistryBuilder::new().build_api_server(&|_headers| { + (403, &r#"{"errors": [{"detail": "you must be logged in"}]}"#) + }); + + 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] api errors (status 403 Forbidden): you must be logged in +", + ) + .run(); + + t.join().unwrap(); +} + +#[cargo_test] +fn api_error_code() { + // Registry returns an error code without a JSON message. + let t = registry::RegistryBuilder::new().build_api_server(&|_headers| (400, &"go away")); + + 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 get a 200 OK response, got 400 +headers: +HTTP/1.1 400 +Content-Length: 7 + +body: +go away +", + ) + .run(); + + t.join().unwrap(); +} + +#[cargo_test] +fn api_curl_error() { + // Registry has a network error. + let t = registry::RegistryBuilder::new().build_api_server(&|_headers| panic!("broke!")); + + 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(); + + // This doesn't check for the exact text of the error in the remote + // possibility that cargo is linked with a weird version of libcurl, or + // curl changes the text of the message. Currently the message 52 + // (CURLE_GOT_NOTHING) is: + // Server returned nothing (no headers, no data) (Empty reply from server) + p.cargo("publish --no-verify --registry alternative") + .with_status(101) + .with_stderr( + "\ +[UPDATING] [..] +[PACKAGING] foo v0.0.1 [..] +[UPLOADING] foo v0.0.1 [..] +[ERROR] [52] [..] +", + ) + .run(); + + let e = t.join().unwrap_err(); + assert_eq!(*e.downcast::<&str>().unwrap(), "broke!"); +} + +#[cargo_test] +fn api_other_error() { + // Registry returns an invalid response. + let t = registry::RegistryBuilder::new().build_api_server(&|_headers| (200, b"\xff")); + + 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] invalid response from server + +Caused by: + response body was not valid utf-8 +", + ) + .run(); + + t.join().unwrap(); +}