From 57ff6aaa2d377a7ebb9f4b6205ffcb6e1c9539a8 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Fri, 28 Feb 2020 23:38:42 -0500 Subject: [PATCH 1/6] Bump to `conduit` alpha series to switch to `http` types --- Cargo.lock | 58 ++++------ Cargo.toml | 20 ++-- src/controllers.rs | 35 +++--- src/controllers/category.rs | 6 +- src/controllers/crate_owner_invitation.rs | 14 +-- src/controllers/helpers.rs | 5 +- src/controllers/helpers/pagination.rs | 6 +- src/controllers/keyword.rs | 4 +- src/controllers/krate/downloads.rs | 2 +- src/controllers/krate/follow.rs | 8 +- src/controllers/krate/metadata.rs | 10 +- src/controllers/krate/owners.rs | 14 +-- src/controllers/krate/publish.rs | 4 +- src/controllers/krate/search.rs | 2 +- src/controllers/site_metadata.rs | 2 +- src/controllers/team.rs | 2 +- src/controllers/token.rs | 6 +- src/controllers/user/me.rs | 12 +- src/controllers/user/other.rs | 4 +- src/controllers/user/session.rs | 6 +- src/controllers/util.rs | 6 +- src/controllers/version.rs | 6 +- src/controllers/version/deprecated.rs | 4 +- src/controllers/version/downloads.rs | 6 +- src/controllers/version/metadata.rs | 6 +- src/controllers/version/yank.rs | 6 +- src/db.rs | 4 +- src/middleware.rs | 10 +- src/middleware/app.rs | 6 +- src/middleware/block_traffic.rs | 25 ++-- src/middleware/current_user.rs | 4 +- src/middleware/debug.rs | 14 +-- src/middleware/ember_index_rewrite.rs | 8 +- src/middleware/ensure_well_formed_500.rs | 16 +-- src/middleware/head.rs | 18 ++- src/middleware/log_connection_pool_status.rs | 6 +- src/middleware/log_request.rs | 26 ++--- src/middleware/require_user_agent.rs | 20 ++-- src/middleware/static_or_continue.rs | 4 +- src/router.rs | 34 +++--- src/tests/all.rs | 14 +-- src/tests/authentication.rs | 20 ++-- src/tests/krate.rs | 113 ++++++++++++++----- src/tests/owners.rs | 9 +- src/tests/read_only_mode.rs | 9 +- src/tests/server.rs | 42 ++++--- src/tests/team.rs | 21 ++-- src/tests/token.rs | 20 +++- src/tests/user.rs | 14 +-- src/tests/util.rs | 50 ++++---- src/uploaders.rs | 4 +- src/util.rs | 25 ++-- src/util/errors.rs | 46 ++++---- src/util/errors/http.rs | 17 +-- src/util/request_helpers.rs | 21 ++-- src/util/request_proxy.rs | 18 ++- 56 files changed, 467 insertions(+), 425 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d26abd5ce7e..57a4e0c9028 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -228,7 +228,7 @@ dependencies = [ "rand 0.6.5", "reqwest", "scheduled-thread-pool", - "semver 0.9.0 (git+https://github.com/steveklabnik/semver.git)", + "semver", "serde", "serde_json", "swirl", @@ -277,14 +277,13 @@ dependencies = [ [[package]] name = "civet" -version = "0.12.0-alpha.1" +version = "0.12.0-alpha.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5f5ae04519752da172ab2e82540b28244da241064aa50e64de83645d0ae8d952" +checksum = "b6ae786c5519f3740ffe88639d6d404c102c7af4acf206a94567c68f93d25b38" dependencies = [ "civet-sys", "conduit", "libc", - "semver 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -320,18 +319,18 @@ dependencies = [ [[package]] name = "conduit" -version = "0.9.0-alpha.0" +version = "0.9.0-alpha.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "37ae2b5937afd049324f246e5ecd1d628bd23ba221879b9f56cd45674e9194bf" +checksum = "2ca703f41f1b6a01299404ea407c2ae5b1cffc09a3d77af0e760130621cb3795" dependencies = [ - "semver 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", + "http", ] [[package]] name = "conduit-conditional-get" -version = "0.9.0-alpha.0" +version = "0.9.0-alpha.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2b1e0b06d987366f9fc85a76260bc4057bab5dcc73f85b23fb633a49d8130136" +checksum = "9a686f3233875b24b228b3362738de7cd16c74a8c81d8acfc0617edf427a2741" dependencies = [ "conduit", "conduit-middleware", @@ -340,9 +339,9 @@ dependencies = [ [[package]] name = "conduit-cookie" -version = "0.9.0-alpha.0" +version = "0.9.0-alpha.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8cdba83653b08d6b2f082433e678ece2039f25f554fb68daf2a39f8c9ad351af" +checksum = "7645e2fa9b3fdbae7ce77b29a7e353af2e616c936dae950e927c6bb5209c265a" dependencies = [ "base64 0.11.0", "conduit", @@ -352,9 +351,9 @@ dependencies = [ [[package]] name = "conduit-git-http-backend" -version = "0.9.0-alpha.0" +version = "0.9.0-alpha.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4cf9cbbd931a7f6ecc159a70325e3e487f5f96a7310d02abbf62846e12ce9eab" +checksum = "7a5962ccb612bd8e435978519a0acc13879cb86c087902fc8f8c3ab7bf2a7d4a" dependencies = [ "conduit", "flate2", @@ -362,15 +361,14 @@ dependencies = [ [[package]] name = "conduit-hyper" -version = "0.3.0-alpha.0" +version = "0.3.0-alpha.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8802341cb1b01e4fc706804edde75789927fc3b05f9a0235ef59507dac5b0acf" +checksum = "c305c4a09c27fe168fad4539eef738b0898063b7ca8c26a7434605c9aa954be6" dependencies = [ "conduit", "futures", "http", "hyper", - "semver 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", "tokio", "tower-service", "tracing", @@ -378,12 +376,11 @@ dependencies = [ [[package]] name = "conduit-middleware" -version = "0.9.0-alpha.0" +version = "0.9.0-alpha.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "83cc6359459dc275cd9715aa84954ed861d0f68bde9e1f53edf494113a9ea005" +checksum = "800bd3641b7b986cf1305f18243d50fe668f8b4d8bb543856119f55d84622223" dependencies = [ "conduit", - "semver 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -397,20 +394,19 @@ dependencies = [ [[package]] name = "conduit-router" -version = "0.9.0-alpha.0" +version = "0.9.0-alpha.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7cbe1cb2c36d3ec99795c6c2fe6fc54199513240166a16f7cf471d8b8a1d5287" +checksum = "79f5fd8e955ceb288e230572935c323cd84f1042a2804ef5a8b41b1c67fd85c8" dependencies = [ "conduit", "route-recognizer", - "semver 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] name = "conduit-static" -version = "0.9.0-alpha.0" +version = "0.9.0-alpha.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9937e95a4192c239ed588a1d8190126441435de5bebcc7d0db34474c21c9558c" +checksum = "b5b0cec0864a37f86320e0a0781d173b8c8fe7d878b16421c477a345f14bece8" dependencies = [ "conduit", "conduit-mime-types", @@ -420,12 +416,11 @@ dependencies = [ [[package]] name = "conduit-test" -version = "0.9.0-alpha.0" +version = "0.9.0-alpha.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d81b2a279a7630087a56e8772ddef85670ae4d53dd2c9dbe361866ec0be9842f" +checksum = "a29e3574fbffef97d38eabe60757ca26a44d1f5aec8ea549543291cda91c73d9" dependencies = [ "conduit", - "semver 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -2207,15 +2202,6 @@ dependencies = [ "serde", ] -[[package]] -name = "semver" -version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1d7eb9ef2c18661902cc47e535f9bc51b78acd254da71d375c2f6720d9a40403" -dependencies = [ - "semver-parser", -] - [[package]] name = "semver-parser" version = "0.7.0" diff --git a/Cargo.toml b/Cargo.toml index c5ed3c13321..64d62055bd2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -66,16 +66,16 @@ lettre = "0.9" lettre_email = "0.9" failure = "0.1.1" -conduit = "0.9.0-alpha.0" -conduit-conditional-get = "0.9.0-alpha.0" -conduit-cookie = "0.9.0-alpha.0" +conduit = "0.9.0-alpha.1" +conduit-conditional-get = "0.9.0-alpha.1" +conduit-cookie = "0.9.0-alpha.1" cookie = { version = "0.12", features = ["secure"] } -conduit-middleware = "0.9.0-alpha.0" -conduit-router = "0.9.0-alpha.0" -conduit-static = "0.9.0-alpha.0" -conduit-git-http-backend = "0.9.0-alpha.0" -civet = "0.12.0-alpha.1" -conduit-hyper = "0.3.0-alpha.0" +conduit-middleware = "0.9.0-alpha.1" +conduit-router = "0.9.0-alpha.1" +conduit-static = "0.9.0-alpha.1" +conduit-git-http-backend = "0.9.0-alpha.1" +civet = "0.12.0-alpha.2" +conduit-hyper = "0.3.0-alpha.1" futures-util = "0.3" futures-channel = { version = "0.3.1", default-features = false } @@ -86,7 +86,7 @@ indexmap = "1.0.2" handlebars = "3.0.1" [dev-dependencies] -conduit-test = "0.9.0-alpha.0" +conduit-test = "0.9.0-alpha.1" hyper-tls = "0.4" lazy_static = "1.0" diesel_migrations = { version = "1.3.0", features = ["postgres"] } diff --git a/src/controllers.rs b/src/controllers.rs index 82742c5831b..78080a972f0 100644 --- a/src/controllers.rs +++ b/src/controllers.rs @@ -14,15 +14,14 @@ mod prelude { pub use super::helpers::ok_true; pub use diesel::prelude::*; - pub use conduit::{Request, Response}; + pub use conduit::{header, RequestExt, StatusCode}; pub use conduit_router::RequestParams; pub use crate::db::RequestTransaction; - pub use crate::util::errors::{cargo_err, AppError, AppResult, ChainError}; // TODO: Remove cargo_err from here - pub use crate::middleware::app::RequestApp; + pub use crate::util::errors::{cargo_err, AppError, AppResult, ChainError}; // TODO: Remove cargo_err from here + pub use crate::util::{AppResponse, EndpointResult}; - use std::collections::HashMap; use std::io; use indexmap::IndexMap; @@ -33,9 +32,9 @@ mod prelude { } pub trait RequestUtils { - fn redirect(&self, url: String) -> Response; + fn redirect(&self, url: String) -> AppResponse; - fn json(&self, t: &T) -> Response; + fn json(&self, t: &T) -> AppResponse; fn query(&self) -> IndexMap; fn wants_json(&self) -> bool; fn query_with_params(&self, params: IndexMap) -> String; @@ -43,8 +42,8 @@ mod prelude { fn log_metadata(&mut self, key: &'static str, value: V); } - impl<'a> RequestUtils for dyn Request + 'a { - fn json(&self, t: &T) -> Response { + impl<'a> RequestUtils for dyn RequestExt + 'a { + fn json(&self, t: &T) -> AppResponse { crate::util::json_response(t) } @@ -54,21 +53,19 @@ mod prelude { .collect() } - fn redirect(&self, url: String) -> Response { - let mut headers = HashMap::new(); - headers.insert("Location".to_string(), vec![url]); - Response { - status: (302, "Found"), - headers, - body: Box::new(io::empty()), - } + fn redirect(&self, url: String) -> AppResponse { + conduit::Response::builder() + .status(StatusCode::FOUND) + .header(header::LOCATION, url) + .body(Box::new(io::empty()) as conduit::Body) + .unwrap() // Should not panic unless url contains "\r\n" } fn wants_json(&self) -> bool { self.headers() - .find("Accept") - .map(|accept| accept.iter().any(|s| s.contains("json"))) - .unwrap_or(false) + .get_all(header::ACCEPT) + .iter() + .any(|val| val.to_str().unwrap_or_default().contains("json")) } fn query_with_params(&self, new_params: IndexMap) -> String { diff --git a/src/controllers/category.rs b/src/controllers/category.rs index 693ab288ea6..731b2abe88d 100644 --- a/src/controllers/category.rs +++ b/src/controllers/category.rs @@ -6,7 +6,7 @@ use crate::schema::categories; use crate::views::{EncodableCategory, EncodableCategoryWithSubcategories}; /// Handles the `GET /categories` route. -pub fn index(req: &mut dyn Request) -> AppResult { +pub fn index(req: &mut dyn RequestExt) -> EndpointResult { let conn = req.db_conn()?; let query = req.query(); // FIXME: There are 69 categories, 47 top level. This isn't going to @@ -40,7 +40,7 @@ pub fn index(req: &mut dyn Request) -> AppResult { } /// Handles the `GET /categories/:category_id` route. -pub fn show(req: &mut dyn Request) -> AppResult { +pub fn show(req: &mut dyn RequestExt) -> EndpointResult { let slug = &req.params()["category_id"]; let conn = req.db_conn()?; let cat = Category::by_slug(slug).first::(&*conn)?; @@ -77,7 +77,7 @@ pub fn show(req: &mut dyn Request) -> AppResult { } /// Handles the `GET /category_slugs` route. -pub fn slugs(req: &mut dyn Request) -> AppResult { +pub fn slugs(req: &mut dyn RequestExt) -> EndpointResult { let conn = req.db_conn()?; let slugs = categories::table .select((categories::slug, categories::slug, categories::description)) diff --git a/src/controllers/crate_owner_invitation.rs b/src/controllers/crate_owner_invitation.rs index ddf0146756c..8ebc503e6a5 100644 --- a/src/controllers/crate_owner_invitation.rs +++ b/src/controllers/crate_owner_invitation.rs @@ -5,7 +5,7 @@ use crate::schema::{crate_owner_invitations, crate_owners}; use crate::views::{EncodableCrateOwnerInvitation, InvitationResponse}; /// Handles the `GET /me/crate_owner_invitations` route. -pub fn list(req: &mut dyn Request) -> AppResult { +pub fn list(req: &mut dyn RequestExt) -> EndpointResult { let conn = &*req.db_conn()?; let user_id = req.authenticate(conn)?.user_id(); @@ -31,7 +31,7 @@ struct OwnerInvitation { } /// Handles the `PUT /me/crate_owner_invitations/:crate_id` route. -pub fn handle_invite(req: &mut dyn Request) -> AppResult { +pub fn handle_invite(req: &mut dyn RequestExt) -> EndpointResult { let mut body = String::new(); req.body().read_to_string(&mut body)?; @@ -51,7 +51,7 @@ pub fn handle_invite(req: &mut dyn Request) -> AppResult { } /// Handles the `PUT /me/crate_owner_invitations/accept/:token` route. -pub fn handle_invite_with_token(req: &mut dyn Request) -> AppResult { +pub fn handle_invite_with_token(req: &mut dyn RequestExt) -> EndpointResult { let conn = req.db_conn()?; let req_token = &req.params()["token"]; @@ -72,11 +72,11 @@ pub fn handle_invite_with_token(req: &mut dyn Request) -> AppResult { } fn accept_invite( - req: &dyn Request, + req: &dyn RequestExt, conn: &PgConnection, crate_invite: InvitationResponse, user_id: i32, -) -> AppResult { +) -> EndpointResult { use diesel::{delete, insert_into}; conn.transaction(|| { @@ -110,10 +110,10 @@ fn accept_invite( } fn decline_invite( - req: &dyn Request, + req: &dyn RequestExt, conn: &PgConnection, crate_invite: InvitationResponse, -) -> AppResult { +) -> EndpointResult { use diesel::delete; let user_id = req.authenticate(conn)?.user_id(); diff --git a/src/controllers/helpers.rs b/src/controllers/helpers.rs index 4f7053eaf08..281e8436b62 100644 --- a/src/controllers/helpers.rs +++ b/src/controllers/helpers.rs @@ -1,11 +1,10 @@ -use crate::util::{errors::AppResult, json_response}; -use conduit::Response; +use crate::util::{json_response, EndpointResult}; pub(crate) mod pagination; pub(crate) use self::pagination::Paginate; -pub fn ok_true() -> AppResult { +pub fn ok_true() -> EndpointResult { #[derive(Serialize)] struct R { ok: bool, diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index 536ecdf0f66..ce9e2e28d6a 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -186,6 +186,8 @@ where #[cfg(test)] mod tests { use super::{Page, PaginationOptions}; + + use conduit::StatusCode; use indexmap::IndexMap; #[test] @@ -194,7 +196,7 @@ mod tests { params.insert(String::from("page"), String::from("not a number")); let page_error = Page::new(¶ms).unwrap_err().response().unwrap(); - assert_eq!(page_error.status, (400, "Bad Request")); + assert_eq!(page_error.status(), StatusCode::BAD_REQUEST); } #[test] @@ -206,6 +208,6 @@ mod tests { .response() .unwrap(); - assert_eq!(per_page_error.status, (400, "Bad Request")); + assert_eq!(per_page_error.status(), StatusCode::BAD_REQUEST); } } diff --git a/src/controllers/keyword.rs b/src/controllers/keyword.rs index 1176a82044f..f3c60c111a4 100644 --- a/src/controllers/keyword.rs +++ b/src/controllers/keyword.rs @@ -5,7 +5,7 @@ use crate::models::Keyword; use crate::views::EncodableKeyword; /// Handles the `GET /keywords` route. -pub fn index(req: &mut dyn Request) -> AppResult { +pub fn index(req: &mut dyn RequestExt) -> EndpointResult { use crate::schema::keywords; let conn = req.db_conn()?; @@ -41,7 +41,7 @@ pub fn index(req: &mut dyn Request) -> AppResult { } /// Handles the `GET /keywords/:keyword_id` route. -pub fn show(req: &mut dyn Request) -> AppResult { +pub fn show(req: &mut dyn RequestExt) -> EndpointResult { let name = &req.params()["keyword_id"]; let conn = req.db_conn()?; diff --git a/src/controllers/krate/downloads.rs b/src/controllers/krate/downloads.rs index 775053248cc..030a5c432bf 100644 --- a/src/controllers/krate/downloads.rs +++ b/src/controllers/krate/downloads.rs @@ -14,7 +14,7 @@ use crate::views::EncodableVersionDownload; use crate::models::krate::to_char; /// Handles the `GET /crates/:crate_id/downloads` route. -pub fn downloads(req: &mut dyn Request) -> AppResult { +pub fn downloads(req: &mut dyn RequestExt) -> EndpointResult { use diesel::dsl::*; use diesel::sql_types::BigInt; diff --git a/src/controllers/krate/follow.rs b/src/controllers/krate/follow.rs index f0be406b840..4f80d56e70e 100644 --- a/src/controllers/krate/follow.rs +++ b/src/controllers/krate/follow.rs @@ -7,7 +7,7 @@ use crate::db::DieselPooledConn; use crate::models::{Crate, Follow}; use crate::schema::*; -fn follow_target(req: &dyn Request, conn: &DieselPooledConn<'_>) -> AppResult { +fn follow_target(req: &dyn RequestExt, conn: &DieselPooledConn<'_>) -> AppResult { let user_id = req.authenticate(conn)?.user_id(); let crate_name = &req.params()["crate_id"]; let crate_id = Crate::by_name(crate_name) @@ -17,7 +17,7 @@ fn follow_target(req: &dyn Request, conn: &DieselPooledConn<'_>) -> AppResult AppResult { +pub fn follow(req: &mut dyn RequestExt) -> EndpointResult { let conn = req.db_conn()?; let follow = follow_target(req, &conn)?; diesel::insert_into(follows::table) @@ -29,7 +29,7 @@ pub fn follow(req: &mut dyn Request) -> AppResult { } /// Handles the `DELETE /crates/:crate_id/follow` route. -pub fn unfollow(req: &mut dyn Request) -> AppResult { +pub fn unfollow(req: &mut dyn RequestExt) -> EndpointResult { let conn = req.db_conn()?; let follow = follow_target(req, &conn)?; diesel::delete(&follow).execute(&*conn)?; @@ -38,7 +38,7 @@ pub fn unfollow(req: &mut dyn Request) -> AppResult { } /// Handles the `GET /crates/:crate_id/following` route. -pub fn following(req: &mut dyn Request) -> AppResult { +pub fn following(req: &mut dyn RequestExt) -> EndpointResult { use diesel::dsl::exists; let conn = req.db_conn()?; diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 213b6f85ff1..6db5f12ddd2 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -18,7 +18,7 @@ use crate::views::{ use crate::models::krate::ALL_COLUMNS; /// Handles the `GET /summary` route. -pub fn summary(req: &mut dyn Request) -> AppResult { +pub fn summary(req: &mut dyn RequestExt) -> EndpointResult { use crate::schema::crates::dsl::*; let conn = req.db_read_only()?; @@ -101,7 +101,7 @@ pub fn summary(req: &mut dyn Request) -> AppResult { } /// Handles the `GET /crates/:crate_id` route. -pub fn show(req: &mut dyn Request) -> AppResult { +pub fn show(req: &mut dyn RequestExt) -> EndpointResult { let name = &req.params()["crate_id"]; let conn = req.db_read_only()?; let krate = Crate::by_name(name).first::(&*conn)?; @@ -173,7 +173,7 @@ pub fn show(req: &mut dyn Request) -> AppResult { } /// Handles the `GET /crates/:crate_id/:version/readme` route. -pub fn readme(req: &mut dyn Request) -> AppResult { +pub fn readme(req: &mut dyn RequestExt) -> EndpointResult { let crate_name = &req.params()["crate_id"]; let version = &req.params()["version"]; @@ -197,7 +197,7 @@ pub fn readme(req: &mut dyn Request) -> AppResult { /// Handles the `GET /crates/:crate_id/versions` route. // FIXME: Not sure why this is necessary since /crates/:crate_id returns // this information already, but ember is definitely requesting it -pub fn versions(req: &mut dyn Request) -> AppResult { +pub fn versions(req: &mut dyn RequestExt) -> EndpointResult { let crate_name = &req.params()["crate_id"]; let conn = req.db_read_only()?; let krate = Crate::by_name(crate_name).first::(&*conn)?; @@ -226,7 +226,7 @@ pub fn versions(req: &mut dyn Request) -> AppResult { } /// Handles the `GET /crates/:crate_id/reverse_dependencies` route. -pub fn reverse_dependencies(req: &mut dyn Request) -> AppResult { +pub fn reverse_dependencies(req: &mut dyn RequestExt) -> EndpointResult { use diesel::dsl::any; let name = &req.params()["crate_id"]; diff --git a/src/controllers/krate/owners.rs b/src/controllers/krate/owners.rs index 04329118134..103afa8e6d4 100644 --- a/src/controllers/krate/owners.rs +++ b/src/controllers/krate/owners.rs @@ -5,7 +5,7 @@ use crate::models::{Crate, Owner, Rights, Team, User}; use crate::views::EncodableOwner; /// Handles the `GET /crates/:crate_id/owners` route. -pub fn owners(req: &mut dyn Request) -> AppResult { +pub fn owners(req: &mut dyn RequestExt) -> EndpointResult { let crate_name = &req.params()["crate_id"]; let conn = req.db_conn()?; let krate = Crate::by_name(crate_name).first::(&*conn)?; @@ -23,7 +23,7 @@ pub fn owners(req: &mut dyn Request) -> AppResult { } /// Handles the `GET /crates/:crate_id/owner_team` route. -pub fn owner_team(req: &mut dyn Request) -> AppResult { +pub fn owner_team(req: &mut dyn RequestExt) -> EndpointResult { let crate_name = &req.params()["crate_id"]; let conn = req.db_conn()?; let krate = Crate::by_name(crate_name).first::(&*conn)?; @@ -40,7 +40,7 @@ pub fn owner_team(req: &mut dyn Request) -> AppResult { } /// Handles the `GET /crates/:crate_id/owner_user` route. -pub fn owner_user(req: &mut dyn Request) -> AppResult { +pub fn owner_user(req: &mut dyn RequestExt) -> EndpointResult { let crate_name = &req.params()["crate_id"]; let conn = req.db_conn()?; let krate = Crate::by_name(crate_name).first::(&*conn)?; @@ -57,12 +57,12 @@ pub fn owner_user(req: &mut dyn Request) -> AppResult { } /// Handles the `PUT /crates/:crate_id/owners` route. -pub fn add_owners(req: &mut dyn Request) -> AppResult { +pub fn add_owners(req: &mut dyn RequestExt) -> EndpointResult { modify_owners(req, true) } /// Handles the `DELETE /crates/:crate_id/owners` route. -pub fn remove_owners(req: &mut dyn Request) -> AppResult { +pub fn remove_owners(req: &mut dyn RequestExt) -> EndpointResult { modify_owners(req, false) } @@ -70,7 +70,7 @@ pub fn remove_owners(req: &mut dyn Request) -> AppResult { /// The format is /// /// {"owners": ["username", "github:org:team", ...]} -fn parse_owners_request(req: &mut dyn Request) -> AppResult> { +fn parse_owners_request(req: &mut dyn RequestExt) -> AppResult> { let mut body = String::new(); req.body().read_to_string(&mut body)?; #[derive(Deserialize)] @@ -87,7 +87,7 @@ fn parse_owners_request(req: &mut dyn Request) -> AppResult> { .ok_or_else(|| cargo_err("invalid json request")) } -fn modify_owners(req: &mut dyn Request, add: bool) -> AppResult { +fn modify_owners(req: &mut dyn RequestExt, add: bool) -> EndpointResult { let logins = parse_owners_request(req)?; let app = req.app(); let crate_name = &req.params()["crate_id"]; diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 93f3e1d5a50..beffa88baa7 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -23,7 +23,7 @@ use crate::views::{EncodableCrateUpload, GoodCrate, PublishWarnings}; /// Currently blocks the HTTP thread, perhaps some function calls can spawn new /// threads and return completion or error through other methods a `cargo publish /// --status` command, via crates.io's front end, or email. -pub fn publish(req: &mut dyn Request) -> AppResult { +pub fn publish(req: &mut dyn RequestExt) -> EndpointResult { let app = Arc::clone(req.app()); // The format of the req.body() of a publish request is as follows: @@ -228,7 +228,7 @@ pub fn publish(req: &mut dyn Request) -> AppResult { /// /// This function parses the JSON headers to interpret the data and validates /// the data during and after the parsing. Returns crate metadata. -fn parse_new_headers(req: &mut dyn Request) -> AppResult { +fn parse_new_headers(req: &mut dyn RequestExt) -> AppResult { // Read the json upload request let metadata_length = u64::from(read_le_u32(req.body())?); req.log_metadata("metadata_length", metadata_length); diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 1c338afbffd..e956ccdd193 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -33,7 +33,7 @@ use crate::models::krate::{canon_crate_name, ALL_COLUMNS}; /// caused the break. In the future, we should look at splitting this /// function out to cover the different use cases, and create unit tests /// for them. -pub fn search(req: &mut dyn Request) -> AppResult { +pub fn search(req: &mut dyn RequestExt) -> EndpointResult { use diesel::sql_types::{Bool, Text}; let conn = req.db_read_only()?; diff --git a/src/controllers/site_metadata.rs b/src/controllers/site_metadata.rs index bc9ea754f72..0feca662f5f 100644 --- a/src/controllers/site_metadata.rs +++ b/src/controllers/site_metadata.rs @@ -4,7 +4,7 @@ use super::prelude::*; /// /// The sha is contained within the `HEROKU_SLUG_COMMIT` environment variable. /// If `HEROKU_SLUG_COMMIT` is not set, returns `"unknown"`. -pub fn show_deployed_sha(req: &mut dyn Request) -> AppResult { +pub fn show_deployed_sha(req: &mut dyn RequestExt) -> EndpointResult { let deployed_sha = dotenv::var("HEROKU_SLUG_COMMIT").unwrap_or_else(|_| String::from("unknown")); diff --git a/src/controllers/team.rs b/src/controllers/team.rs index a0a98b4a224..5e67a0cdf72 100644 --- a/src/controllers/team.rs +++ b/src/controllers/team.rs @@ -5,7 +5,7 @@ use crate::schema::teams; use crate::views::EncodableTeam; /// Handles the `GET /teams/:team_id` route. -pub fn show_team(req: &mut dyn Request) -> AppResult { +pub fn show_team(req: &mut dyn RequestExt) -> EndpointResult { use self::teams::dsl::{login, teams}; let name = &req.params()["team_id"]; diff --git a/src/controllers/token.rs b/src/controllers/token.rs index c426528c250..28d2ae5f86f 100644 --- a/src/controllers/token.rs +++ b/src/controllers/token.rs @@ -8,7 +8,7 @@ use crate::views::EncodableApiTokenWithToken; use serde_json as json; /// Handles the `GET /me/tokens` route. -pub fn list(req: &mut dyn Request) -> AppResult { +pub fn list(req: &mut dyn RequestExt) -> EndpointResult { let conn = req.db_conn()?; let user = req.authenticate(&conn)?.find_user(&conn)?; @@ -24,7 +24,7 @@ pub fn list(req: &mut dyn Request) -> AppResult { } /// Handles the `PUT /me/tokens` route. -pub fn new(req: &mut dyn Request) -> AppResult { +pub fn new(req: &mut dyn RequestExt) -> EndpointResult { /// The incoming serialization format for the `ApiToken` model. #[derive(Deserialize, Serialize)] struct NewApiToken { @@ -93,7 +93,7 @@ pub fn new(req: &mut dyn Request) -> AppResult { } /// Handles the `DELETE /me/tokens/:id` route. -pub fn revoke(req: &mut dyn Request) -> AppResult { +pub fn revoke(req: &mut dyn RequestExt) -> EndpointResult { let id = req.params()["id"] .parse::() .map_err(|e| bad_request(&format!("invalid token id: {:?}", e)))?; diff --git a/src/controllers/user/me.rs b/src/controllers/user/me.rs index dfd1fc1c6b6..fea34b89ad1 100644 --- a/src/controllers/user/me.rs +++ b/src/controllers/user/me.rs @@ -12,7 +12,7 @@ use crate::schema::{crate_owners, crates, emails, follows, users, versions}; use crate::views::{EncodableMe, EncodableVersion, OwnedCrate}; /// Handles the `GET /me` route. -pub fn me(req: &mut dyn Request) -> AppResult { +pub fn me(req: &mut dyn RequestExt) -> EndpointResult { let conn = req.db_conn()?; let user_id = req.authenticate(&conn)?.user_id(); @@ -50,7 +50,7 @@ pub fn me(req: &mut dyn Request) -> AppResult { } /// Handles the `GET /me/updates` route. -pub fn updates(req: &mut dyn Request) -> AppResult { +pub fn updates(req: &mut dyn RequestExt) -> EndpointResult { use diesel::dsl::any; let conn = req.db_conn()?; @@ -100,7 +100,7 @@ pub fn updates(req: &mut dyn Request) -> AppResult { } /// Handles the `PUT /user/:user_id` route. -pub fn update_user(req: &mut dyn Request) -> AppResult { +pub fn update_user(req: &mut dyn RequestExt) -> EndpointResult { use self::emails::user_id; use diesel::insert_into; @@ -162,7 +162,7 @@ pub fn update_user(req: &mut dyn Request) -> AppResult { } /// Handles the `PUT /confirm/:email_token` route -pub fn confirm_user_email(req: &mut dyn Request) -> AppResult { +pub fn confirm_user_email(req: &mut dyn RequestExt) -> EndpointResult { use diesel::update; let conn = req.db_conn()?; @@ -180,7 +180,7 @@ pub fn confirm_user_email(req: &mut dyn Request) -> AppResult { } /// Handles `PUT /user/:user_id/resend` route -pub fn regenerate_token_and_send(req: &mut dyn Request) -> AppResult { +pub fn regenerate_token_and_send(req: &mut dyn RequestExt) -> EndpointResult { use diesel::dsl::sql; use diesel::update; @@ -209,7 +209,7 @@ pub fn regenerate_token_and_send(req: &mut dyn Request) -> AppResult { } /// Handles `PUT /me/email_notifications` route -pub fn update_email_notifications(req: &mut dyn Request) -> AppResult { +pub fn update_email_notifications(req: &mut dyn RequestExt) -> EndpointResult { use self::crate_owners::dsl::*; use diesel::pg::upsert::excluded; diff --git a/src/controllers/user/other.rs b/src/controllers/user/other.rs index 7ee1a94215f..1f0b7952212 100644 --- a/src/controllers/user/other.rs +++ b/src/controllers/user/other.rs @@ -6,7 +6,7 @@ use crate::util::errors::ChainError; use crate::views::EncodablePublicUser; /// Handles the `GET /users/:user_id` route. -pub fn show(req: &mut dyn Request) -> AppResult { +pub fn show(req: &mut dyn RequestExt) -> EndpointResult { use self::users::dsl::{gh_login, id, users}; let name = &req.params()["user_id"].to_lowercase(); @@ -26,7 +26,7 @@ pub fn show(req: &mut dyn Request) -> AppResult { } /// Handles the `GET /users/:user_id/stats` route. -pub fn stats(req: &mut dyn Request) -> AppResult { +pub fn stats(req: &mut dyn RequestExt) -> EndpointResult { use diesel::dsl::sum; let user_id = &req.params()["user_id"] diff --git a/src/controllers/user/session.rs b/src/controllers/user/session.rs index 87f86fdce21..9e0a2914576 100644 --- a/src/controllers/user/session.rs +++ b/src/controllers/user/session.rs @@ -25,7 +25,7 @@ use crate::util::errors::ReadOnlyMode; /// "url": "https://github.com/login/oauth/authorize?client_id=...&state=...&scope=read%3Aorg" /// } /// ``` -pub fn begin(req: &mut dyn Request) -> AppResult { +pub fn begin(req: &mut dyn RequestExt) -> EndpointResult { let (url, state) = req .app() .github @@ -73,7 +73,7 @@ pub fn begin(req: &mut dyn Request) -> AppResult { /// } /// } /// ``` -pub fn authorize(req: &mut dyn Request) -> AppResult { +pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult { // Parse the url query let mut query = req.query(); let code = query.remove("code").unwrap_or_default(); @@ -148,7 +148,7 @@ impl GithubUser { } /// Handles the `DELETE /api/private/session` route. -pub fn logout(req: &mut dyn Request) -> AppResult { +pub fn logout(req: &mut dyn RequestExt) -> EndpointResult { req.session().remove(&"user_id".to_string()); Ok(req.json(&true)) } diff --git a/src/controllers/util.rs b/src/controllers/util.rs index 7e5171dfd0d..c659432cc58 100644 --- a/src/controllers/util.rs +++ b/src/controllers/util.rs @@ -25,7 +25,7 @@ impl AuthenticatedUser { } } -impl<'a> UserAuthenticationExt for dyn Request + 'a { +impl<'a> UserAuthenticationExt for dyn RequestExt + 'a { /// Obtain `AuthenticatedUser` for the request or return an `Unauthorized` error fn authenticate(&self, conn: &PgConnection) -> AppResult { if let Some(id) = self.extensions().find::() { @@ -36,8 +36,8 @@ impl<'a> UserAuthenticationExt for dyn Request + 'a { }) } else { // Otherwise, look for an `Authorization` header on the request - if let Some(headers) = self.headers().find("Authorization") { - ApiToken::find_by_api_token(conn, headers[0]) + if let Some(headers) = self.headers().get(header::AUTHORIZATION) { + ApiToken::find_by_api_token(conn, headers.to_str().unwrap_or_default()) .map(|token| AuthenticatedUser { user_id: token.user_id, token_id: Some(token.id), diff --git a/src/controllers/version.rs b/src/controllers/version.rs index 948c95a81e6..c2722e29afb 100644 --- a/src/controllers/version.rs +++ b/src/controllers/version.rs @@ -8,7 +8,7 @@ use super::prelude::*; use crate::db::DieselPooledConn; use crate::models::{Crate, Version}; -fn version_and_crate(req: &dyn Request) -> AppResult<(DieselPooledConn<'_>, Version, Crate)> { +fn version_and_crate(req: &dyn RequestExt) -> AppResult<(DieselPooledConn<'_>, Version, Crate)> { let crate_name = extract_crate_name(req); let semver = extract_semver(req)?; @@ -19,11 +19,11 @@ fn version_and_crate(req: &dyn Request) -> AppResult<(DieselPooledConn<'_>, Vers Ok((conn, version, krate)) } -fn extract_crate_name(req: &dyn Request) -> &str { +fn extract_crate_name(req: &dyn RequestExt) -> &str { &req.params()["crate_id"] } -fn extract_semver(req: &dyn Request) -> AppResult<&str> { +fn extract_semver(req: &dyn RequestExt) -> AppResult<&str> { let semver = &req.params()["version"]; if semver::Version::parse(semver).is_err() { return Err(cargo_err(&format_args!("invalid semver: {}", semver))); diff --git a/src/controllers/version/deprecated.rs b/src/controllers/version/deprecated.rs index 271e8b87ea3..0dcd1d95f98 100644 --- a/src/controllers/version/deprecated.rs +++ b/src/controllers/version/deprecated.rs @@ -12,7 +12,7 @@ use crate::schema::*; use crate::views::EncodableVersion; /// Handles the `GET /versions` route. -pub fn index(req: &mut dyn Request) -> AppResult { +pub fn index(req: &mut dyn RequestExt) -> EndpointResult { use diesel::dsl::any; let conn = req.db_conn()?; @@ -55,7 +55,7 @@ pub fn index(req: &mut dyn Request) -> AppResult { /// Handles the `GET /versions/:version_id` route. /// The frontend doesn't appear to hit this endpoint. Instead, the version information appears to /// be returned by `krate::show`. -pub fn show_by_id(req: &mut dyn Request) -> AppResult { +pub fn show_by_id(req: &mut dyn RequestExt) -> EndpointResult { let id = &req.params()["version_id"]; let id = id.parse().unwrap_or(0); let conn = req.db_conn()?; diff --git a/src/controllers/version/downloads.rs b/src/controllers/version/downloads.rs index a71d076a5e3..33cd4d2c932 100644 --- a/src/controllers/version/downloads.rs +++ b/src/controllers/version/downloads.rs @@ -14,7 +14,7 @@ use super::{extract_crate_name, extract_semver}; /// Handles the `GET /crates/:crate_id/:version/download` route. /// This returns a URL to the location where the crate is stored. -pub fn download(req: &mut dyn Request) -> AppResult { +pub fn download(req: &mut dyn RequestExt) -> EndpointResult { let crate_name = &req.params()["crate_id"]; let version = &req.params()["version"]; @@ -47,7 +47,7 @@ pub fn download(req: &mut dyn Request) -> AppResult { /// Even if failure occurs for unexpected reasons, we would rather have `cargo /// build` succeed and not count the download than break people's builds. fn increment_download_counts( - req: &dyn Request, + req: &dyn RequestExt, crate_name: &str, version: &str, ) -> AppResult { @@ -68,7 +68,7 @@ fn increment_download_counts( } /// Handles the `GET /crates/:crate_id/:version/downloads` route. -pub fn downloads(req: &mut dyn Request) -> AppResult { +pub fn downloads(req: &mut dyn RequestExt) -> EndpointResult { let crate_name = extract_crate_name(req); let semver = extract_semver(req)?; diff --git a/src/controllers/version/metadata.rs b/src/controllers/version/metadata.rs index 0f36ab001c6..96ca488ff7e 100644 --- a/src/controllers/version/metadata.rs +++ b/src/controllers/version/metadata.rs @@ -19,7 +19,7 @@ use super::version_and_crate; /// In addition to returning cached data from the index, this returns /// fields for `id`, `version_id`, and `downloads` (which appears to always /// be 0) -pub fn dependencies(req: &mut dyn Request) -> AppResult { +pub fn dependencies(req: &mut dyn RequestExt) -> EndpointResult { let (conn, version, _) = version_and_crate(req)?; let deps = version.dependencies(&*conn)?; let deps = deps @@ -35,7 +35,7 @@ pub fn dependencies(req: &mut dyn Request) -> AppResult { } /// Handles the `GET /crates/:crate_id/:version/authors` route. -pub fn authors(req: &mut dyn Request) -> AppResult { +pub fn authors(req: &mut dyn RequestExt) -> EndpointResult { let (conn, version, _) = version_and_crate(req)?; let names = version_authors::table .filter(version_authors::version_id.eq(version.id)) @@ -65,7 +65,7 @@ pub fn authors(req: &mut dyn Request) -> AppResult { /// /// The frontend doesn't appear to hit this endpoint, but our tests do, and it seems to be a useful /// API route to have. -pub fn show(req: &mut dyn Request) -> AppResult { +pub fn show(req: &mut dyn RequestExt) -> EndpointResult { let (conn, version, krate) = version_and_crate(req)?; let published_by = version.published_by(&conn); let actions = VersionOwnerAction::by_version(&conn, &version)?; diff --git a/src/controllers/version/yank.rs b/src/controllers/version/yank.rs index 597d8d53415..77791bc297e 100644 --- a/src/controllers/version/yank.rs +++ b/src/controllers/version/yank.rs @@ -17,17 +17,17 @@ use crate::models::{insert_version_owner_action, VersionAction}; /// Crate deletion is not implemented to avoid breaking builds, /// and the goal of yanking a crate is to prevent crates /// beginning to depend on the yanked crate version. -pub fn yank(req: &mut dyn Request) -> AppResult { +pub fn yank(req: &mut dyn RequestExt) -> EndpointResult { modify_yank(req, true) } /// Handles the `PUT /crates/:crate_id/:version/unyank` route. -pub fn unyank(req: &mut dyn Request) -> AppResult { +pub fn unyank(req: &mut dyn RequestExt) -> EndpointResult { modify_yank(req, false) } /// Changes `yanked` flag on a crate version record -fn modify_yank(req: &mut dyn Request, yanked: bool) -> AppResult { +fn modify_yank(req: &mut dyn RequestExt, yanked: bool) -> EndpointResult { let (conn, version, krate) = version_and_crate(req)?; let ids = req.authenticate(&conn)?; let user = ids.find_user(&conn)?; diff --git a/src/db.rs b/src/db.rs index 50954f53bcf..0345e196659 100644 --- a/src/db.rs +++ b/src/db.rs @@ -1,4 +1,4 @@ -use conduit::Request; +use conduit::RequestExt; use diesel::prelude::*; use diesel::r2d2::{self, ConnectionManager, CustomizeConnection}; use parking_lot::{ReentrantMutex, ReentrantMutexGuard}; @@ -93,7 +93,7 @@ pub trait RequestTransaction { fn db_read_only(&self) -> Result, r2d2::PoolError>; } -impl RequestTransaction for T { +impl RequestTransaction for T { fn db_conn(&self) -> Result, r2d2::PoolError> { self.app().primary_database.get().map_err(Into::into) } diff --git a/src/middleware.rs b/src/middleware.rs index 6546d4c0c1e..cea40125834 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -1,14 +1,8 @@ mod prelude { - pub use conduit::{Handler, Request, Response}; - pub use conduit_middleware::{AroundMiddleware, Middleware}; - - use std::error::Error; - pub type BoxError = Box; - pub type Result = std::result::Result; + pub use conduit::{box_error, header, Body, Handler, RequestExt, Response, StatusCode}; + pub use conduit_middleware::{AfterResult, AroundMiddleware, BeforeResult, Middleware}; } -pub use prelude::Result; - use self::app::AppMiddleware; use self::current_user::CaptureUserIdFromCookie; use self::debug::*; diff --git a/src/middleware/app.rs b/src/middleware/app.rs index cf18cc56f8e..42f2817e278 100644 --- a/src/middleware/app.rs +++ b/src/middleware/app.rs @@ -17,12 +17,12 @@ impl AppMiddleware { } impl Middleware for AppMiddleware { - fn before(&self, req: &mut dyn Request) -> Result<()> { + fn before(&self, req: &mut dyn RequestExt) -> BeforeResult { req.mut_extensions().insert(Arc::clone(&self.app)); Ok(()) } - fn after(&self, req: &mut dyn Request, res: Result) -> Result { + fn after(&self, req: &mut dyn RequestExt, res: AfterResult) -> AfterResult { req.mut_extensions().pop::>().unwrap(); res } @@ -33,7 +33,7 @@ pub trait RequestApp { fn app(&self) -> &Arc; } -impl RequestApp for T { +impl RequestApp for T { fn app(&self) -> &Arc { self.extensions().find::>().expect("Missing app") } diff --git a/src/middleware/block_traffic.rs b/src/middleware/block_traffic.rs index 9f1696577df..7e279ce3f38 100644 --- a/src/middleware/block_traffic.rs +++ b/src/middleware/block_traffic.rs @@ -10,7 +10,6 @@ use super::prelude::*; -use std::collections::HashMap; use std::io::Cursor; // Can't derive debug because of Handler. @@ -39,12 +38,12 @@ impl AroundMiddleware for BlockTraffic { } impl Handler for BlockTraffic { - fn call(&self, req: &mut dyn Request) -> Result { + fn call(&self, req: &mut dyn RequestExt) -> AfterResult { let has_blocked_value = req .headers() - .find(&self.header_name) - .unwrap_or_default() + .get_all(&self.header_name) .iter() + .map(|val| val.to_str().unwrap_or_default()) .any(|value| self.blocked_values.iter().any(|v| v == value)); if has_blocked_value { let cause = format!("blocked due to contents of header {}", self.header_name); @@ -57,15 +56,17 @@ impl Handler for BlockTraffic { or email help@crates.io \ and provide the request id {}", // Heroku should always set this header - req.headers().find("X-Request-Id").unwrap()[0] + req.headers() + .get("x-request-id") + .map(|val| val.to_str().unwrap_or_default()) + .unwrap_or_default() ); - let mut headers = HashMap::new(); - headers.insert("Content-Length".to_string(), vec![body.len().to_string()]); - Ok(Response { - status: (403, "Forbidden"), - headers, - body: Box::new(Cursor::new(body.into_bytes())), - }) + + Response::builder() + .status(StatusCode::FORBIDDEN) + .header(header::CONTENT_LENGTH, body.len()) + .body(Box::new(Cursor::new(body.into_bytes())) as Body) + .map_err(box_error) } else { self.handler.as_ref().unwrap().call(req) } diff --git a/src/middleware/current_user.rs b/src/middleware/current_user.rs index fc3a6d02ede..2c95c83b155 100644 --- a/src/middleware/current_user.rs +++ b/src/middleware/current_user.rs @@ -10,7 +10,7 @@ //! This is particularly problematic for the user authentication code. When an API token is used //! for authentication, the datbase must be queried to obtain the `user_id`, so endpoint code must //! obtain and pass in a database connection. Because of that connection, it is no longer possible -//! to use or pass around the `&mut dyn Request` that it was derived from and it is not possible +//! to use or pass around the `&mut dyn RequestExt` that it was derived from and it is not possible //! to access the session cookie. In order to support authentication via session cookies and API //! tokens via the same code path, the `user_id` is extracted from the session cookie and stored in //! a `TrustedUserId` that can be read from while a connection reference is live. @@ -27,7 +27,7 @@ pub struct TrustedUserId(pub i32); pub(super) struct CaptureUserIdFromCookie; impl Middleware for CaptureUserIdFromCookie { - fn before(&self, req: &mut dyn Request) -> Result<()> { + fn before(&self, req: &mut dyn RequestExt) -> BeforeResult { if let Some(id) = req.session().get("user_id").and_then(|s| s.parse().ok()) { req.mut_extensions().insert(TrustedUserId(id)); } diff --git a/src/middleware/debug.rs b/src/middleware/debug.rs index a99432fde2b..6ea439c9a28 100644 --- a/src/middleware/debug.rs +++ b/src/middleware/debug.rs @@ -6,14 +6,14 @@ use super::prelude::*; pub struct Debug; impl Middleware for Debug { - fn before(&self, req: &mut dyn Request) -> Result<()> { + fn before(&self, req: &mut dyn RequestExt) -> BeforeResult { DebugRequest.before(req) } - fn after(&self, _req: &mut dyn Request, res: Result) -> Result { + fn after(&self, _req: &mut dyn RequestExt, res: AfterResult) -> AfterResult { res.map(|res| { - println!(" <- {:?}", res.status); - for (k, v) in &res.headers { + println!(" <- {:?}", res.status()); + for (k, v) in res.headers().iter() { println!(" <- {} {:?}", k, v); } res @@ -25,15 +25,15 @@ impl Middleware for Debug { pub struct DebugRequest; impl Middleware for DebugRequest { - fn before(&self, req: &mut dyn Request) -> Result<()> { - println!(" version: {}", req.http_version()); + fn before(&self, req: &mut dyn RequestExt) -> BeforeResult { + println!(" version: {:?}", req.http_version()); println!(" method: {:?}", req.method()); println!(" scheme: {:?}", req.scheme()); println!(" host: {:?}", req.host()); println!(" path: {}", req.path()); println!(" query_string: {:?}", req.query_string()); println!(" remote_addr: {:?}", req.remote_addr()); - for &(k, ref v) in &req.headers().all() { + for (k, ref v) in req.headers().iter() { println!(" hdr: {}={:?}", k, v); } Ok(()) diff --git a/src/middleware/ember_index_rewrite.rs b/src/middleware/ember_index_rewrite.rs index db7868d6328..fbcc011a8d1 100644 --- a/src/middleware/ember_index_rewrite.rs +++ b/src/middleware/ember_index_rewrite.rs @@ -24,14 +24,14 @@ impl AroundMiddleware for EmberIndexRewrite { } impl Handler for EmberIndexRewrite { - fn call(&self, req: &mut dyn Request) -> Result { + fn call(&self, req: &mut dyn RequestExt) -> AfterResult { // If the client is requesting html, then we've only got one page so // rewrite the request. let wants_html = req .headers() - .find("Accept") - .map(|accept| accept.iter().any(|s| s.contains("html"))) - .unwrap_or(false); + .get_all(header::ACCEPT) + .iter() + .any(|val| val.to_str().unwrap_or_default().contains("html")); // If the route starts with /api, just assume they want the API // response and fall through. let is_api_path = req.path().starts_with("/api"); diff --git a/src/middleware/ensure_well_formed_500.rs b/src/middleware/ensure_well_formed_500.rs index 87b94345dbe..df6d104ccc2 100644 --- a/src/middleware/ensure_well_formed_500.rs +++ b/src/middleware/ensure_well_formed_500.rs @@ -2,24 +2,20 @@ use super::prelude::*; -use std::collections::HashMap; - // Can't derive debug because of Handler. #[allow(missing_debug_implementations)] #[derive(Default)] pub struct EnsureWellFormed500; impl Middleware for EnsureWellFormed500 { - fn after(&self, _: &mut dyn Request, res: Result) -> Result { + fn after(&self, _: &mut dyn RequestExt, res: AfterResult) -> AfterResult { res.or_else(|_| { let body = "Internal Server Error"; - let mut headers = HashMap::new(); - headers.insert("Content-Length".to_string(), vec![body.len().to_string()]); - Ok(Response { - status: (500, "Internal Server Error"), - headers, - body: Box::new(body.as_bytes()), - }) + Response::builder() + .status(StatusCode::INTERNAL_SERVER_ERROR) + .header(header::CONTENT_LENGTH, body.len()) + .body(Box::new(body.as_bytes()) as Body) + .map_err(box_error) }) } } diff --git a/src/middleware/head.rs b/src/middleware/head.rs index 063f52239f3..73a1f1c6dd9 100644 --- a/src/middleware/head.rs +++ b/src/middleware/head.rs @@ -20,17 +20,13 @@ impl AroundMiddleware for Head { } impl Handler for Head { - fn call(&self, req: &mut dyn Request) -> Result { - if req.method() == Method::Head { - let mut req = RequestProxy::rewrite_method(req, Method::Get); - self.handler - .as_ref() - .unwrap() - .call(&mut req) - .map(|r| Response { - body: Box::new(io::empty()), - ..r - }) + fn call(&self, req: &mut dyn RequestExt) -> AfterResult { + if req.method() == Method::HEAD { + let mut req = RequestProxy::rewrite_method(req, Method::GET); + self.handler.as_ref().unwrap().call(&mut req).map(|mut r| { + *r.body_mut() = Box::new(io::empty()) as Body; + r + }) } else { self.handler.as_ref().unwrap().call(req) } diff --git a/src/middleware/log_connection_pool_status.rs b/src/middleware/log_connection_pool_status.rs index 657a1e58c73..ace85d9d730 100644 --- a/src/middleware/log_connection_pool_status.rs +++ b/src/middleware/log_connection_pool_status.rs @@ -3,7 +3,7 @@ use super::prelude::*; use crate::app::App; -use conduit::Request; +use conduit::RequestExt; use std::sync::{ atomic::{AtomicUsize, Ordering}, Arc, Mutex, PoisonError, @@ -28,7 +28,7 @@ impl LogConnectionPoolStatus { } impl Middleware for LogConnectionPoolStatus { - fn before(&self, _: &mut dyn Request) -> Result<()> { + fn before(&self, _: &mut dyn RequestExt) -> BeforeResult { let mut last_log_time = self .last_log_time .lock() @@ -49,7 +49,7 @@ impl Middleware for LogConnectionPoolStatus { Ok(()) } - fn after(&self, _: &mut dyn Request, res: Result) -> Result { + fn after(&self, _: &mut dyn RequestExt, res: AfterResult) -> AfterResult { self.in_flight_requests.fetch_sub(1, Ordering::SeqCst); res } diff --git a/src/middleware/log_request.rs b/src/middleware/log_request.rs index cfaccd384c1..7ff5b205089 100644 --- a/src/middleware/log_request.rs +++ b/src/middleware/log_request.rs @@ -3,7 +3,7 @@ use super::prelude::*; use crate::util::request_header; -use conduit::Request; +use conduit::{header, RequestExt, StatusCode}; use std::fmt::{self, Display, Formatter}; use std::time::Instant; @@ -15,12 +15,12 @@ pub(super) struct LogRequests(); struct RequestStart(Instant); impl Middleware for LogRequests { - fn before(&self, req: &mut dyn Request) -> Result<()> { + fn before(&self, req: &mut dyn RequestExt) -> BeforeResult { req.mut_extensions().insert(RequestStart(Instant::now())); Ok(()) } - fn after(&self, req: &mut dyn Request, res: Result) -> Result { + fn after(&self, req: &mut dyn RequestExt, res: AfterResult) -> AfterResult { // Unwrap shouldn't panic as no other code has access to the private struct to remove it let request_start = req.extensions().find::().unwrap().0; @@ -45,7 +45,7 @@ struct CustomMetadata { entries: Vec<(&'static str, String)>, } -pub fn add_custom_metadata(req: &mut dyn Request, key: &'static str, value: V) { +pub fn add_custom_metadata(req: &mut dyn RequestExt, key: &'static str, value: V) { if let Some(metadata) = req.mut_extensions().find_mut::() { metadata.entries.push((key, value.to_string())); } else { @@ -58,7 +58,7 @@ pub fn add_custom_metadata(req: &mut dyn Request, key: &'static str, } #[cfg(test)] -pub(crate) fn get_log_message(req: &dyn Request, key: &'static str) -> String { +pub(crate) fn get_log_message(req: &dyn RequestExt, key: &'static str) -> String { for (k, v) in &req.extensions().find::().unwrap().entries { if key == *k { return v.clone(); @@ -68,8 +68,8 @@ pub(crate) fn get_log_message(req: &dyn Request, key: &'static str) -> String { } struct RequestLine<'r> { - req: &'r dyn Request, - res: &'r Result, + req: &'r dyn RequestExt, + res: &'r AfterResult, response_time: u64, } @@ -78,18 +78,18 @@ impl Display for RequestLine<'_> { let mut line = LogLine::new(f); let (at, status) = match self.res { - Ok(resp) => ("info", resp.status.0), - Err(_) => ("error", 500), + Ok(resp) => ("info", resp.status()), + Err(_) => ("error", StatusCode::INTERNAL_SERVER_ERROR), }; line.add_field("at", at)?; line.add_field("method", self.req.method())?; line.add_quoted_field("path", FullPath(self.req))?; - line.add_field("request_id", request_header(self.req, "X-Request-Id"))?; - line.add_quoted_field("fwd", request_header(self.req, "X-Real-Ip"))?; + line.add_field("request_id", request_header(self.req, "x-request-id"))?; + line.add_quoted_field("fwd", request_header(self.req, "x-real-ip"))?; line.add_field("service", TimeMs(self.response_time))?; line.add_field("status", status)?; - line.add_quoted_field("user_agent", request_header(self.req, "User-Agent"))?; + line.add_quoted_field("user_agent", request_header(self.req, header::USER_AGENT))?; if let Some(metadata) = self.req.extensions().find::() { for (key, value) in &metadata.entries { @@ -109,7 +109,7 @@ impl Display for RequestLine<'_> { } } -struct FullPath<'a>(&'a dyn Request); +struct FullPath<'a>(&'a dyn RequestExt); impl<'a> Display for FullPath<'a> { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { diff --git a/src/middleware/require_user_agent.rs b/src/middleware/require_user_agent.rs index ded8f8ae038..f2d9145b4ab 100644 --- a/src/middleware/require_user_agent.rs +++ b/src/middleware/require_user_agent.rs @@ -3,7 +3,6 @@ use super::prelude::*; use crate::util::request_header; -use std::collections::HashMap; use std::io::Cursor; // Can't derive debug because of Handler. @@ -20,22 +19,21 @@ impl AroundMiddleware for RequireUserAgent { } impl Handler for RequireUserAgent { - fn call(&self, req: &mut dyn Request) -> Result { - let has_user_agent = request_header(req, "User-Agent") != ""; + fn call(&self, req: &mut dyn RequestExt) -> AfterResult { + let has_user_agent = request_header(req, header::USER_AGENT) != ""; let is_download = req.path().ends_with("download"); if !has_user_agent && !is_download { super::log_request::add_custom_metadata(req, "cause", "no user agent"); let body = format!( include_str!("no_user_agent_message.txt"), - request_header(req, "X-Request-Id"), + request_header(req, "x-request-id"), ); - let mut headers = HashMap::new(); - headers.insert("Content-Length".to_string(), vec![body.len().to_string()]); - Ok(Response { - status: (403, "Forbidden"), - headers, - body: Box::new(Cursor::new(body.into_bytes())), - }) + + Response::builder() + .status(StatusCode::FORBIDDEN) + .header(header::CONTENT_LENGTH, body.len()) + .body(Box::new(Cursor::new(body.into_bytes())) as Body) + .map_err(box_error) } else { self.handler.as_ref().unwrap().call(req) } diff --git a/src/middleware/static_or_continue.rs b/src/middleware/static_or_continue.rs index 478b7f6af85..5d54302fe8c 100644 --- a/src/middleware/static_or_continue.rs +++ b/src/middleware/static_or_continue.rs @@ -27,9 +27,9 @@ impl AroundMiddleware for StaticOrContinue { } impl Handler for StaticOrContinue { - fn call(&self, req: &mut dyn Request) -> Result { + fn call(&self, req: &mut dyn RequestExt) -> AfterResult { match self.static_handler.call(req) { - Ok(ref resp) if resp.status.0 == 404 => {} + Ok(ref resp) if resp.status() == StatusCode::NOT_FOUND => {} ret => return ret, } diff --git a/src/router.rs b/src/router.rs index 69a366f3fe8..2dd99581ca2 100644 --- a/src/router.rs +++ b/src/router.rs @@ -1,12 +1,12 @@ use std::sync::Arc; -use conduit::{Handler, Request, Response}; +use conduit::{Handler, HandlerResult, RequestExt}; use conduit_router::{RequestParams, RouteBuilder}; use crate::controllers::*; -use crate::util::errors::{std_error, AppError, AppResult, NotFound}; -use crate::util::RequestProxy; -use crate::{middleware, App, Env}; +use crate::util::errors::{std_error, AppError, NotFound}; +use crate::util::{EndpointResult, RequestProxy}; +use crate::{App, Env}; pub fn build_router(app: &App) -> R404 { let mut api_router = RouteBuilder::new(); @@ -136,10 +136,10 @@ pub fn build_router(app: &App) -> R404 { R404(router) } -struct C(pub fn(&mut dyn Request) -> AppResult); +struct C(pub fn(&mut dyn RequestExt) -> EndpointResult); impl Handler for C { - fn call(&self, req: &mut dyn Request) -> middleware::Result { + fn call(&self, req: &mut dyn RequestExt) -> HandlerResult { let C(f) = *self; match f(req) { Ok(resp) => Ok(resp), @@ -159,7 +159,7 @@ impl Handler for C { struct R(pub Arc); impl Handler for R { - fn call(&self, req: &mut dyn Request) -> middleware::Result { + fn call(&self, req: &mut dyn RequestExt) -> HandlerResult { let path = req.params()["path"].to_string(); let R(ref sub_router) = *self; sub_router.call(&mut RequestProxy::rewrite_path(req, &path)) @@ -171,7 +171,7 @@ impl Handler for R { pub struct R404(pub RouteBuilder); impl Handler for R404 { - fn call(&self, req: &mut dyn Request) -> middleware::Result { + fn call(&self, req: &mut dyn RequestExt) -> HandlerResult { let R404(ref router) = *self; match router.recognize(&req.method(), req.path()) { Ok(m) => { @@ -189,40 +189,40 @@ mod tests { use crate::util::errors::{ bad_request, cargo_err, internal, AppError, ChainError, NotFound, Unauthorized, }; + use crate::util::EndpointResult; use conduit_test::MockRequest; use diesel::result::Error as DieselError; - fn err(err: E) -> AppResult { + fn err(err: E) -> EndpointResult { Err(Box::new(err)) } #[test] fn http_error_responses() { - let mut req = MockRequest::new(::conduit::Method::Get, "/"); + let mut req = MockRequest::new(::conduit::Method::GET, "/"); // Types for handling common error status codes assert_eq!( - C(|_| Err(bad_request(""))).call(&mut req).unwrap().status.0, + C(|_| Err(bad_request(""))).call(&mut req).unwrap().status(), 400 ); assert_eq!( - C(|_| err(Unauthorized)).call(&mut req).unwrap().status.0, + C(|_| err(Unauthorized)).call(&mut req).unwrap().status(), 403 ); assert_eq!( C(|_| Err(DieselError::NotFound.into())) .call(&mut req) .unwrap() - .status - .0, + .status(), 404 ); - assert_eq!(C(|_| err(NotFound)).call(&mut req).unwrap().status.0, 404); + assert_eq!(C(|_| err(NotFound)).call(&mut req).unwrap().status(), 404); // cargo_err errors are returned as 200 so that cargo displays this nicely on the command line assert_eq!( - C(|_| Err(cargo_err(""))).call(&mut req).unwrap().status.0, + C(|_| Err(cargo_err(""))).call(&mut req).unwrap().status(), 200 ); @@ -236,7 +236,7 @@ mod tests { }) .call(&mut req) .unwrap(); - assert_eq!(response.status.0, 400); + assert_eq!(response.status(), 400); assert_eq!( crate::middleware::log_request::get_log_message(&req, "cause"), "middle error caused by invalid digit found in string" diff --git a/src/tests/all.rs b/src/tests/all.rs index 4275785f2fd..2b61849b2bc 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -13,6 +13,7 @@ use crate::util::{Bad, RequestHelper, TestApp}; use cargo_registry::{ models::{Crate, CrateOwner, Dependency, NewCategory, NewTeam, NewUser, Team, User, Version}, schema::crate_owners, + util::AppResponse, views::{ EncodableCategory, EncodableCategoryWithSubcategories, EncodableCrate, EncodableKeyword, EncodableOwner, EncodableVersion, GoodCrate, @@ -27,6 +28,7 @@ use std::{ }, }; +use conduit::header; use conduit_test::MockRequest; use diesel::prelude::*; use reqwest::{blocking::Client, Proxy}; @@ -180,15 +182,11 @@ fn env(var: &str) -> String { fn req(method: conduit::Method, path: &str) -> MockRequest { let mut request = MockRequest::new(method, path); - request.header("User-Agent", "conduit-test"); + request.header(header::USER_AGENT, "conduit-test"); request } -fn ok_resp(r: &conduit::Response) -> bool { - r.status.0 == 200 -} - -fn bad_resp(r: &mut conduit::Response) -> Option { +fn bad_resp(r: &mut AppResponse) -> Option { let bad = json::(r); if bad.errors.is_empty() { return None; @@ -196,12 +194,12 @@ fn bad_resp(r: &mut conduit::Response) -> Option { Some(bad) } -fn json(r: &mut conduit::Response) -> T +fn json(r: &mut AppResponse) -> T where for<'de> T: serde::Deserialize<'de>, { let mut data = Vec::new(); - r.body.write_body(&mut data).unwrap(); + r.body_mut().write_body(&mut data).unwrap(); let s = std::str::from_utf8(&data).unwrap(); match serde_json::from_str(s) { Ok(t) => t, diff --git a/src/tests/authentication.rs b/src/tests/authentication.rs index ebf615c1841..9ff5d59e085 100644 --- a/src/tests/authentication.rs +++ b/src/tests/authentication.rs @@ -2,32 +2,30 @@ use crate::{util::RequestHelper, TestApp}; use cargo_registry::middleware::current_user::TrustedUserId; -use conduit::{Handler, Method, Request, Response}; +use conduit::{header, Handler, HandlerResult, Method, RequestExt, StatusCode}; use conduit_test::MockRequest; -type ResponseResult = Result>; - static URL: &str = "/api/v1/me/updates"; static MUST_LOGIN: &[u8] = b"{\"errors\":[{\"detail\":\"must be logged in to perform that action\"}]}"; static INTERNAL_ERROR_NO_USER: &str = "user_id from cookie or token not found in database caused by NotFound"; -fn call(app: &TestApp, mut request: MockRequest) -> ResponseResult { +fn call(app: &TestApp, mut request: MockRequest) -> HandlerResult { app.as_middleware().call(&mut request) } -fn into_parts(response: ResponseResult) -> (u32, Vec) { +fn into_parts(response: HandlerResult) -> (StatusCode, Vec) { let mut response = response.unwrap(); let mut body = Vec::new(); - response.body.write_body(&mut body).unwrap(); - (response.status.0, body) + response.body_mut().write_body(&mut body).unwrap(); + (response.status(), body) } #[test] fn anonymous_user_unauthorized() { let (app, anon) = TestApp::init().empty(); - let request = anon.request_builder(Method::Get, URL); + let request = anon.request_builder(Method::GET, URL); let (status, body) = into_parts(call(&app, request)); assert_eq!(status, 403); @@ -37,8 +35,8 @@ fn anonymous_user_unauthorized() { #[test] fn token_auth_cannot_find_token() { let (app, anon) = TestApp::init().empty(); - let mut request = anon.request_builder(Method::Get, URL); - request.header("Authorization", "fake-token"); + let mut request = anon.request_builder(Method::GET, URL); + request.header(header::AUTHORIZATION, "fake-token"); let (status, body) = into_parts(call(&app, request)); assert_eq!(status, 403); @@ -51,7 +49,7 @@ fn token_auth_cannot_find_token() { #[test] fn cookie_auth_cannot_find_user() { let (app, anon) = TestApp::init().empty(); - let mut request = anon.request_builder(Method::Get, URL); + let mut request = anon.request_builder(Method::GET, URL); request.mut_extensions().insert(TrustedUserId(-1)); let response = call(&app, request); diff --git a/src/tests/krate.rs b/src/tests/krate.rs index f041364272d..c49a564b677 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -19,6 +19,7 @@ use std::{ }; use chrono::Utc; +use conduit::StatusCode; use diesel::{dsl::*, prelude::*, update}; use flate2::{write::GzEncoder, Compression}; @@ -710,7 +711,9 @@ fn new_wrong_token() { // Try to publish without a token let crate_to_publish = PublishBuilder::new("foo"); - let json = anon.enqueue_publish(crate_to_publish).bad_with_status(403); + let json = anon + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::FORBIDDEN); assert!( json.errors[0] .detail @@ -728,7 +731,9 @@ fn new_wrong_token() { }); let crate_to_publish = PublishBuilder::new("foo"); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(403); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::FORBIDDEN); assert!( json.errors[0] .detail @@ -744,7 +749,9 @@ fn invalid_names() { let bad_name = |name: &str, error_message: &str| { let crate_to_publish = PublishBuilder::new(name).version("1.0.0"); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0].detail.contains(error_message,), @@ -861,7 +868,9 @@ fn reject_new_krate_with_non_exact_dependency() { let crate_to_publish = PublishBuilder::new("new_dep") .version("1.0.0") .dependency(dependency); - token.enqueue_publish(crate_to_publish).bad_with_status(200); + token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); } #[test] @@ -885,7 +894,9 @@ fn reject_new_crate_with_alternative_registry_dependency() { DependencyBuilder::new("dep").registry("https://server.example/path/to/registry"); let crate_to_publish = PublishBuilder::new("depends-on-alt-registry").dependency(dependency); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0] .detail @@ -910,7 +921,9 @@ fn new_krate_with_wildcard_dependency() { .version("1.0.0") .dependency(dependency); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0].detail.contains("dependency constraints"), "{:?}", @@ -951,7 +964,7 @@ fn new_krate_wrong_user() { let json = another_user .enqueue_publish(crate_to_publish) - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!( json.errors[0] .detail @@ -979,7 +992,9 @@ fn new_krate_too_big() { let files = [("foo_big-1.0.0/big", &[b'a'; 2000] as &[_])]; let builder = PublishBuilder::new("foo_big").files(&files); - let json = user.enqueue_publish(builder).bad_with_status(200); + let json = user + .enqueue_publish(builder) + .bad_with_status(StatusCode::OK); assert!( json.errors[0] .detail @@ -1014,7 +1029,9 @@ fn new_krate_wrong_files() { let files = [("foo-1.0.0/a", data), ("bar-1.0.0/a", data)]; let builder = PublishBuilder::new("foo").files(&files); - let json = user.enqueue_publish(builder).bad_with_status(200); + let json = user + .enqueue_publish(builder) + .bad_with_status(StatusCode::OK); assert!( json.errors[0].detail.contains("invalid tarball uploaded"), "{:?}", @@ -1033,7 +1050,9 @@ fn new_krate_gzip_bomb() { .version("1.1.0") .files_with_io(&mut [("foo-1.1.0/a", &mut body, len)]); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0] @@ -1056,7 +1075,9 @@ fn new_krate_duplicate_version() { }); let crate_to_publish = PublishBuilder::new("foo_dupe").version("1.0.0"); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0].detail.contains("already uploaded"), @@ -1076,7 +1097,9 @@ fn new_crate_similar_name() { }); let crate_to_publish = PublishBuilder::new("foo_similar").version("1.1.0"); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0].detail.contains("previously named"), @@ -1096,7 +1119,9 @@ fn new_crate_similar_name_hyphen() { }); let crate_to_publish = PublishBuilder::new("foo-bar-hyphen").version("1.1.0"); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0].detail.contains("previously named"), @@ -1116,7 +1141,9 @@ fn new_crate_similar_name_underscore() { }); let crate_to_publish = PublishBuilder::new("foo_bar_underscore").version("1.1.0"); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0].detail.contains("previously named"), @@ -1190,7 +1217,9 @@ fn new_krate_dependency_missing() { let dependency = DependencyBuilder::new("bar_missing"); let crate_to_publish = PublishBuilder::new("foo_missing").dependency(dependency); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0] .detail @@ -1221,7 +1250,9 @@ fn new_krate_without_any_email_fails() { let crate_to_publish = PublishBuilder::new("foo_no_email"); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0] @@ -1245,7 +1276,9 @@ fn new_krate_with_unverified_email_fails() { let crate_to_publish = PublishBuilder::new("foo_unverified_email"); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0] @@ -1402,7 +1435,7 @@ fn download() { let download = |name_and_version: &str| { let url = format!("/api/v1/crates/{}/download", name_and_version); - anon.get::<()>(&url).assert_status(302); + anon.get::<()>(&url).assert_status(StatusCode::FOUND); // TODO: test the with_json code path }; @@ -1474,7 +1507,7 @@ fn dependencies() { assert_eq!(deps.dependencies[0].crate_id, "bar_deps"); anon.get::<()>("/api/v1/crates/foo_deps/1.0.2/dependencies") - .bad_with_status(200); + .bad_with_status(StatusCode::OK); } #[test] @@ -1583,7 +1616,9 @@ fn yank_by_a_non_owner_fails() { .expect_build(conn); }); - let json = token.yank("foo_not", "1.0.0").bad_with_status(200); + let json = token + .yank("foo_not", "1.0.0") + .bad_with_status(StatusCode::OK); assert_eq!( json.errors[0].detail, "must already be an owner to yank or unyank" @@ -1784,13 +1819,19 @@ fn bad_keywords() { let (_, _, _, token) = TestApp::init().with_token(); let crate_to_publish = PublishBuilder::new("foo_bad_key").keyword("super-long-keyword-name-oh-no"); - token.enqueue_publish(crate_to_publish).bad_with_status(200); + token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); let crate_to_publish = PublishBuilder::new("foo_bad_key").keyword("?@?%"); - token.enqueue_publish(crate_to_publish).bad_with_status(200); + token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); let crate_to_publish = PublishBuilder::new("foo_bad_key").keyword("áccênts"); - token.enqueue_publish(crate_to_publish).bad_with_status(200); + token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); } #[test] @@ -2090,7 +2131,9 @@ fn author_license_and_description_required() { .unset_description() .unset_authors(); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0].detail.contains("author") && json.errors[0].detail.contains("description") @@ -2105,7 +2148,9 @@ fn author_license_and_description_required() { .unset_authors() .author(""); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0].detail.contains("author") && json.errors[0].detail.contains("description") @@ -2120,7 +2165,9 @@ fn author_license_and_description_required() { .license_file("foo") .unset_description(); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( !json.errors[0].detail.contains("author") && json.errors[0].detail.contains("description") @@ -2339,7 +2386,9 @@ fn new_krate_tarball_with_hard_links() { let crate_to_publish = PublishBuilder::new("foo").version("1.1.0").tarball(tarball); - let json = token.enqueue_publish(crate_to_publish).bad_with_status(200); + let json = token + .enqueue_publish(crate_to_publish) + .bad_with_status(StatusCode::OK); assert!( json.errors[0] @@ -2362,11 +2411,13 @@ fn publish_new_crate_rate_limited() { // Uploading a second crate is limited let crate_to_publish = PublishBuilder::new("rate_limited2"); - token.enqueue_publish(crate_to_publish).assert_status(429); + token + .enqueue_publish(crate_to_publish) + .assert_status(StatusCode::TOO_MANY_REQUESTS); app.run_pending_background_jobs(); anon.get::<()>("/api/v1/crates/rate_limited2") - .assert_status(404); + .assert_status(StatusCode::NOT_FOUND); // Wait for the limit to be up thread::sleep(Duration::from_millis(500)); @@ -2430,7 +2481,7 @@ fn pagination_parameters_only_accept_integers() { let invalid_per_page_json = anon .get_with_query::<()>("/api/v1/crates", "page=1&per_page=100%22%EF%BC%8Cexception") - .bad_with_status(400); + .bad_with_status(StatusCode::BAD_REQUEST); assert_eq!( invalid_per_page_json.errors[0].detail, "invalid digit found in string" @@ -2438,7 +2489,7 @@ fn pagination_parameters_only_accept_integers() { let invalid_page_json = anon .get_with_query::<()>("/api/v1/crates", "page=100%22%EF%BC%8Cexception&per_page=1") - .bad_with_status(400); + .bad_with_status(StatusCode::BAD_REQUEST); assert_eq!( invalid_page_json.errors[0].detail, "invalid digit found in string" diff --git a/src/tests/owners.rs b/src/tests/owners.rs index c37a48b2020..bcfa19f0ede 100644 --- a/src/tests/owners.rs +++ b/src/tests/owners.rs @@ -10,6 +10,7 @@ use cargo_registry::{ views::{EncodableCrateOwnerInvitation, EncodableOwner, InvitationResponse}, }; +use conduit::StatusCode; use diesel::prelude::*; #[derive(Deserialize)] @@ -136,7 +137,7 @@ fn owners_can_remove_self() { // Deleting yourself when you're the only owner isn't allowed. let json = token .remove_named_owner("owners_selfremove", username) - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!(json.errors[0] .detail .contains("cannot remove all individual owners of a crate")); @@ -152,7 +153,7 @@ fn owners_can_remove_self() { // After you delete yourself, you no longer have permisions to manage the crate. let json = token .remove_named_owner("owners_selfremove", username) - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!(json.errors[0] .detail .contains("only owners have permission to modify owners",)); @@ -173,7 +174,7 @@ fn modify_multiple_owners() { // Deleting all owners is not allowed. let json = token .remove_named_owners("owners_multiple", &[username, "user2", "user3"]) - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!(&json.errors[0] .detail .contains("cannot remove all individual owners of a crate")); @@ -189,7 +190,7 @@ fn modify_multiple_owners() { // Adding multiple users fails if one of them already is an owner. let json = token .add_named_owners("owners_multiple", &["user2", username]) - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!(&json.errors[0].detail.contains("is already an owner")); assert_eq!(app.db(|conn| krate.owners(&conn).unwrap()).len(), 1); diff --git a/src/tests/read_only_mode.rs b/src/tests/read_only_mode.rs index 3da27d815a0..875f3a8cd83 100644 --- a/src/tests/read_only_mode.rs +++ b/src/tests/read_only_mode.rs @@ -1,12 +1,15 @@ use crate::builders::CrateBuilder; use crate::{RequestHelper, TestApp}; + +use conduit::StatusCode; use diesel::prelude::*; #[test] fn can_hit_read_only_endpoints_in_read_only_mode() { let (app, anon) = TestApp::init().empty(); app.db(set_read_only).unwrap(); - anon.get::<()>("/api/v1/crates").assert_status(200); + anon.get::<()>("/api/v1/crates") + .assert_status(StatusCode::OK); } #[test] @@ -20,7 +23,7 @@ fn cannot_hit_endpoint_which_writes_db_in_read_only_mode() { }); token .delete::<()>("/api/v1/crates/foo_yank_read_only/1.0.0/yank") - .assert_status(503); + .assert_status(StatusCode::SERVICE_UNAVAILABLE); // Restore the transaction so `TestApp::drop` can still access the transaction app.db(|conn| { @@ -42,7 +45,7 @@ fn can_download_crate_in_read_only_mode() { }); anon.get::<()>("/api/v1/crates/foo_download_read_only/1.0.0/download") - .assert_status(302); + .assert_status(StatusCode::FOUND); // We're in read only mode so the download should not have been counted app.db(|conn| { diff --git a/src/tests/server.rs b/src/tests/server.rs index 6a07041c078..8e941e05b9c 100644 --- a/src/tests/server.rs +++ b/src/tests/server.rs @@ -1,4 +1,4 @@ -use conduit::Method; +use conduit::{header, Method}; use crate::builders::*; use crate::util::*; @@ -7,10 +7,10 @@ use crate::util::*; fn user_agent_is_required() { let (_app, anon) = TestApp::init().empty(); - let mut req = anon.request_builder(Method::Get, "/api/v1/crates"); - req.header("User-Agent", ""); + let mut req = anon.request_builder(Method::GET, "/api/v1/crates"); + req.header(header::USER_AGENT, ""); let resp = anon.run::<()>(req); - resp.assert_status(403); + resp.assert_status(StatusCode::FORBIDDEN); } #[test] @@ -21,10 +21,10 @@ fn user_agent_is_not_required_for_download() { CrateBuilder::new("dl_no_ua", user.as_model().id).expect_build(conn); }); - let mut req = anon.request_builder(Method::Get, "/api/v1/crates/dl_no_ua/0.99.0/download"); - req.header("User-Agent", ""); + let mut req = anon.request_builder(Method::GET, "/api/v1/crates/dl_no_ua/0.99.0/download"); + req.header(header::USER_AGENT, ""); let resp = anon.run::<()>(req); - resp.assert_status(302); + resp.assert_status(StatusCode::FOUND); } #[test] @@ -39,10 +39,10 @@ fn blocked_traffic_doesnt_panic_if_checked_header_is_not_present() { CrateBuilder::new("dl_no_ua", user.as_model().id).expect_build(conn); }); - let mut req = anon.request_builder(Method::Get, "/api/v1/crates/dl_no_ua/0.99.0/download"); - req.header("User-Agent", ""); + let mut req = anon.request_builder(Method::GET, "/api/v1/crates/dl_no_ua/0.99.0/download"); + req.header(header::USER_AGENT, ""); let resp = anon.run::<()>(req); - resp.assert_status(302); + resp.assert_status(StatusCode::FOUND); } #[test] @@ -57,17 +57,25 @@ fn block_traffic_via_arbitrary_header_and_value() { CrateBuilder::new("dl_no_ua", user.as_model().id).expect_build(conn); }); - let mut req = anon.request_builder(Method::Get, "/api/v1/crates/dl_no_ua/0.99.0/download"); + let mut req = anon.request_builder(Method::GET, "/api/v1/crates/dl_no_ua/0.99.0/download"); // A request with a header value we want to block isn't allowed - req.header("User-Agent", "1"); - req.header("X-Request-Id", "abcd"); // Needed for the error message we generate + req.header(header::USER_AGENT, "1"); + req.header( + // Needed for the error message we generate + // FIXME: Simplify once we hit `conduit-test 0.9.0-alpha.2` + header::HeaderName::from_lowercase(b"x-request-id").unwrap(), + "abcd", + ); let resp = anon.run::<()>(req); - resp.assert_status(403); + resp.assert_status(StatusCode::FORBIDDEN); - let mut req = anon.request_builder(Method::Get, "/api/v1/crates/dl_no_ua/0.99.0/download"); + let mut req = anon.request_builder(Method::GET, "/api/v1/crates/dl_no_ua/0.99.0/download"); // A request with a header value we don't want to block is allowed, even though there might // be a substring match - req.header("User-Agent", "1value-must-match-exactly-this-is-allowed"); + req.header( + header::USER_AGENT, + "1value-must-match-exactly-this-is-allowed", + ); let resp = anon.run::<()>(req); - resp.assert_status(302); + resp.assert_status(StatusCode::FOUND); } diff --git a/src/tests/team.rs b/src/tests/team.rs index 3f54b5a9dc7..f7ce76a419e 100644 --- a/src/tests/team.rs +++ b/src/tests/team.rs @@ -8,6 +8,7 @@ use crate::{ use cargo_registry::models::{Crate, NewUser}; use std::sync::Once; +use conduit::StatusCode; use diesel::*; impl crate::util::MockAnonymousUser { @@ -50,7 +51,7 @@ fn not_github() { let json = token .add_named_owner("foo_not_github", "dropbox:foo:foo") - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!( json.errors[0].detail.contains("unknown organization"), @@ -69,7 +70,7 @@ fn weird_name() { let json = token .add_named_owner("foo_weird_name", "github:foo/../bar:wut") - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!( json.errors[0] @@ -91,7 +92,7 @@ fn one_colon() { let json = token .add_named_owner("foo_one_colon", "github:foo") - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!( json.errors[0].detail.contains("missing github team"), @@ -113,7 +114,7 @@ fn nonexistent_team() { "foo_nonexistent", "github:crates-test-org:this-does-not-exist", ) - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!( json.errors[0] @@ -170,7 +171,7 @@ fn add_team_as_non_member() { "foo_team_non_member", "github:crates-test-org:just-for-crates-2", ) - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!( json.errors[0] @@ -200,7 +201,7 @@ fn remove_team_as_named_owner() { // have permission to manage ownership let json = token_on_both_teams .remove_named_owner("foo_remove_team", username) - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!(json.errors[0] .detail .contains("cannot remove all individual owners of a crate")); @@ -213,7 +214,7 @@ fn remove_team_as_named_owner() { let crate_to_publish = PublishBuilder::new("foo_remove_team").version("2.0.0"); let json = user_on_one_team .enqueue_publish(crate_to_publish) - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!( json.errors[0] @@ -244,7 +245,7 @@ fn remove_team_as_team_owner() { let json = token_on_one_team .remove_named_owner("foo_remove_team_owner", "github:crates-test-org:core") - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!( json.errors[0] @@ -275,7 +276,7 @@ fn publish_not_owned() { let crate_to_publish = PublishBuilder::new("foo_not_owned").version("2.0.0"); let json = user_on_one_team .enqueue_publish(crate_to_publish) - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!( json.errors[0] @@ -327,7 +328,7 @@ fn add_owners_as_team_owner() { let json = token_on_one_team .add_named_owner("foo_add_owner", "arbitrary_username") - .bad_with_status(200); + .bad_with_status(StatusCode::OK); assert!( json.errors[0] diff --git a/src/tests/token.rs b/src/tests/token.rs index 0cc92314332..a2efac4d8f3 100644 --- a/src/tests/token.rs +++ b/src/tests/token.rs @@ -1,4 +1,4 @@ -use crate::{user::UserShowPrivateResponse, RequestHelper, TestApp}; +use crate::{user::UserShowPrivateResponse, util::StatusCode, RequestHelper, TestApp}; use cargo_registry::{ models::ApiToken, schema::api_tokens, @@ -111,7 +111,9 @@ fn create_token_logged_out() { fn create_token_invalid_request() { let (_, _, user) = TestApp::init().with_user(); let invalid = br#"{ "name": "" }"#; - let json = user.put::<()>(URL, invalid).bad_with_status(400); + let json = user + .put::<()>(URL, invalid) + .bad_with_status(StatusCode::BAD_REQUEST); assert_contains!(json.errors[0].detail, "invalid new token request"); } @@ -120,7 +122,9 @@ fn create_token_invalid_request() { fn create_token_no_name() { let (_, _, user) = TestApp::init().with_user(); let empty_name = br#"{ "api_token": { "name": "" } }"#; - let json = user.put::<()>(URL, empty_name).bad_with_status(400); + let json = user + .put::<()>(URL, empty_name) + .bad_with_status(StatusCode::BAD_REQUEST); assert_eq!(json.errors[0].detail, "name must have a value"); } @@ -129,7 +133,9 @@ fn create_token_no_name() { fn create_token_long_body() { let (_, _, user) = TestApp::init().with_user(); let too_big = &[5; 5192]; // Send a request with a 5kB body of 5's - let json = user.put::<()>(URL, too_big).bad_with_status(400); + let json = user + .put::<()>(URL, too_big) + .bad_with_status(StatusCode::BAD_REQUEST); assert_contains!(json.errors[0].detail, "max content length"); } @@ -143,7 +149,9 @@ fn create_token_exceeded_tokens_per_user() { t!(ApiToken::insert(conn, id, &format!("token {}", i))); } }); - let json = user.put::<()>(URL, NEW_BAR).bad_with_status(400); + let json = user + .put::<()>(URL, NEW_BAR) + .bad_with_status(StatusCode::BAD_REQUEST); assert_contains!(json.errors[0].detail, "maximum tokens per user"); } @@ -192,7 +200,7 @@ fn cannot_create_token_with_token() { "/api/v1/me/tokens", br#"{ "api_token": { "name": "baz" } }"#, ) - .bad_with_status(400); + .bad_with_status(StatusCode::BAD_REQUEST); assert_contains!( json.errors[0].detail, diff --git a/src/tests/user.rs b/src/tests/user.rs index ee1e77bc7b4..91d6c1572c9 100644 --- a/src/tests/user.rs +++ b/src/tests/user.rs @@ -1,7 +1,7 @@ use crate::{ builders::{CrateBuilder, VersionBuilder}, new_user, - util::{MockCookieUser, RequestHelper, Response}, + util::{MockCookieUser, RequestHelper, Response, StatusCode}, OkBool, TestApp, }; use cargo_registry::{ @@ -115,7 +115,7 @@ fn access_token_needs_data() { let (_, anon) = TestApp::init().empty(); let json = anon .get::<()>("/api/private/session/authorize") - .bad_with_status(400); + .bad_with_status(StatusCode::BAD_REQUEST); assert!(json.errors[0].detail.contains("invalid state")); } @@ -296,7 +296,7 @@ fn following() { assert_eq!(r.meta.more, false); user.get_with_query::<()>("/api/v1/me/updates", "page=0") - .bad_with_status(400); + .bad_with_status(StatusCode::BAD_REQUEST); } #[test] @@ -486,7 +486,7 @@ fn test_empty_email_not_added() { let json = user .update_email_more_control(model.id, Some("")) - .bad_with_status(400); + .bad_with_status(StatusCode::BAD_REQUEST); assert!( json.errors[0].detail.contains("empty email rejected"), "{:?}", @@ -495,7 +495,7 @@ fn test_empty_email_not_added() { let json = user .update_email_more_control(model.id, None) - .bad_with_status(400); + .bad_with_status(StatusCode::BAD_REQUEST); assert!( json.errors[0].detail.contains("empty email rejected"), @@ -521,7 +521,7 @@ fn test_other_users_cannot_change_my_email() { another_user_model.id, Some("pineapple@pineapples.pineapple"), ) - .bad_with_status(400); + .bad_with_status(StatusCode::BAD_REQUEST); assert!( json.errors[0] .detail @@ -534,7 +534,7 @@ fn test_other_users_cannot_change_my_email() { another_user_model.id, Some("pineapple@pineapples.pineapple"), ) - .bad_with_status(403); + .bad_with_status(StatusCode::FORBIDDEN); } /* Given a new user, test that their email can be added diff --git a/src/tests/util.rs b/src/tests/util.rs index 87759ec09c0..b7acc08f723 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -29,13 +29,14 @@ use cargo_registry::{ git::{Credentials, RepositoryConfig}, middleware::current_user::TrustedUserId, models::{ApiToken, User}, + util::AppResponse, App, Config, }; use diesel::PgConnection; use std::{rc::Rc, sync::Arc, time::Duration}; use swirl::Runner; -use conduit::{Handler, Method, Request}; +use conduit::{Handler, HandlerResult, Method, RequestExt}; use conduit_test::MockRequest; use cargo_registry::git::Repository as WorkerRepository; @@ -43,6 +44,8 @@ use git2::Repository as UpstreamRepository; use url::Url; +pub use conduit::{header, StatusCode}; + struct TestAppInner { app: Arc, // The bomb (if created) needs to be held in scope until the end of the test. @@ -330,7 +333,7 @@ pub trait RequestHelper { where for<'de> T: serde::Deserialize<'de>, { - let request = self.request_builder(Method::Get, path); + let request = self.request_builder(Method::GET, path); self.run(request) } @@ -339,7 +342,7 @@ pub trait RequestHelper { where for<'de> T: serde::Deserialize<'de>, { - let mut request = self.request_builder(Method::Get, path); + let mut request = self.request_builder(Method::GET, path); request.with_query(query); self.run(request) } @@ -349,7 +352,7 @@ pub trait RequestHelper { where for<'de> T: serde::Deserialize<'de>, { - let mut request = self.request_builder(Method::Put, path); + let mut request = self.request_builder(Method::PUT, path); request.with_body(body); self.run(request) } @@ -359,7 +362,7 @@ pub trait RequestHelper { where for<'de> T: serde::Deserialize<'de>, { - let request = self.request_builder(Method::Delete, path); + let request = self.request_builder(Method::DELETE, path); self.run(request) } @@ -368,7 +371,7 @@ pub trait RequestHelper { where for<'de> T: serde::Deserialize<'de>, { - let mut request = self.request_builder(Method::Delete, path); + let mut request = self.request_builder(Method::DELETE, path); request.with_body(body); self.run(request) } @@ -500,7 +503,7 @@ pub struct MockTokenUser { impl RequestHelper for MockTokenUser { fn request_builder(&self, method: Method, path: &str) -> MockRequest { let mut request = crate::req(method, path); - request.header("Authorization", &self.token.token); + request.header(header::AUTHORIZATION, &self.token.token); request } @@ -560,12 +563,10 @@ pub struct Bad { pub errors: Vec, } -type ResponseResult = Result>; - /// A type providing helper methods for working with responses #[must_use] pub struct Response { - response: conduit::Response, + response: AppResponse, callback_on_good: Option>, } @@ -573,7 +574,7 @@ impl Response where for<'de> T: serde::Deserialize<'de>, { - fn new(response: ResponseResult) -> Self { + fn new(response: HandlerResult) -> Self { Self { response: t!(response), callback_on_good: None, @@ -589,8 +590,8 @@ where /// Assert that the response is good and deserialize the message pub fn good(mut self) -> T { - if !crate::ok_resp(&self.response) { - panic!("bad response: {:?}", self.response.status); + if !self.response.status().is_success() { + panic!("bad response: {:?}", self.response.status()); } let good = crate::json(&mut self.response); if let Some(callback) = self.callback_on_good { @@ -602,21 +603,28 @@ where /// Assert the response status code and deserialze into a list of errors /// /// Cargo endpoints return a status 200 on error instead of 400. - pub fn bad_with_status(&mut self, code: u32) -> Bad { - assert_eq!(self.response.status.0, code); + pub fn bad_with_status(&mut self, expected: StatusCode) -> Bad { + assert_eq!(self.response.status(), expected); match crate::bad_resp(&mut self.response) { - None => panic!("ok response: {:?}", self.response.status), + None => panic!("ok response: {:?}", self.response.status()), Some(b) => b, } } - pub fn assert_status(&self, status: u32) -> &Self { - assert_eq!(status, self.response.status.0); + pub fn assert_status(&self, status: StatusCode) -> &Self { + assert_eq!(status, self.response.status()); self } pub fn assert_redirect_ends_with(&self, target: &str) -> &Self { - assert!(self.response.headers["Location"][0].ends_with(target)); + assert!(self + .response + .headers() + .get(header::LOCATION) + .unwrap() + .to_str() + .unwrap() + .ends_with(target)); self } } @@ -624,11 +632,11 @@ where impl Response<()> { /// Assert that the status code is 404 pub fn assert_not_found(&self) { - assert_eq!((404, "Not Found"), self.response.status); + assert_eq!(StatusCode::NOT_FOUND, self.response.status()); } /// Assert that the status code is 403 pub fn assert_forbidden(&self) { - assert_eq!((403, "Forbidden"), self.response.status); + assert_eq!(StatusCode::FORBIDDEN, self.response.status()); } } diff --git a/src/uploaders.rs b/src/uploaders.rs index 682029505db..46a7c3acace 100644 --- a/src/uploaders.rs +++ b/src/uploaders.rs @@ -1,4 +1,4 @@ -use conduit::Request; +use conduit::RequestExt; use flate2::read::GzDecoder; use openssl::error::ErrorStack; use openssl::hash::{Hasher, MessageDigest}; @@ -129,7 +129,7 @@ impl Uploader { /// Uploads a crate and returns the checksum of the uploaded crate file. pub fn upload_crate( &self, - req: &mut dyn Request, + req: &mut dyn RequestExt, krate: &Crate, maximums: Maximums, vers: &semver::Version, diff --git a/src/util.rs b/src/util.rs index 5bdbdb74d7b..9f8d20c963d 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1,8 +1,6 @@ use std::cmp; -use std::collections::HashMap; -use std::io::Cursor; -use conduit::Response; +use conduit::{header, vec_to_body, Response}; use serde::Serialize; pub use self::errors::concrete::Error; @@ -16,6 +14,9 @@ mod request_helpers; mod request_proxy; pub mod rfc3339; +pub type AppResponse = Response; +pub type EndpointResult = Result>; + /// Serialize a value to JSON and build a status 200 Response /// /// This helper sets appropriate values for `Content-Type` and `Content-Length`. @@ -23,19 +24,13 @@ pub mod rfc3339; /// # Panics /// /// This function will panic if serialization fails. -pub fn json_response(t: &T) -> Response { +pub fn json_response(t: &T) -> AppResponse { let json = serde_json::to_string(t).unwrap(); - let mut headers = HashMap::new(); - headers.insert( - "Content-Type".to_string(), - vec!["application/json; charset=utf-8".to_string()], - ); - headers.insert("Content-Length".to_string(), vec![json.len().to_string()]); - Response { - status: (200, "OK"), - headers, - body: Box::new(Cursor::new(json.into_bytes())), - } + Response::builder() + .header(header::CONTENT_TYPE, "application/json; charset=utf-8") + .header(header::CONTENT_LENGTH, json.len()) + .body(vec_to_body(json.into_bytes())) + .unwrap() // Header values are well formed, so should not panic } #[derive(Debug, Copy, Clone)] diff --git a/src/util/errors.rs b/src/util/errors.rs index 9c7eafc851d..727cbb99409 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -17,10 +17,10 @@ use std::error::Error; use std::fmt; use chrono::NaiveDateTime; -use conduit::Response; +use conduit::{header, StatusCode}; use diesel::result::Error as DieselError; -use crate::util::json_response; +use crate::util::{json_response, AppResponse}; pub(super) mod concrete; mod http; @@ -59,11 +59,11 @@ struct Bad<'a> { } /// Generates a response with the provided status and description as JSON -fn json_error(detail: &str, status: (u32, &'static str)) -> Response { +fn json_error(detail: &str, status: StatusCode) -> AppResponse { let mut response = json_response(&Bad { errors: vec![StringError { detail }], }); - response.status = status; + *response.status_mut() = status; response } @@ -75,7 +75,7 @@ pub trait AppError: Send + fmt::Display + fmt::Debug + 'static { /// /// If none is returned, the error will bubble up the middleware stack /// where it is eventually logged and turned into a status 500 response. - fn response(&self) -> Option; + fn response(&self) -> Option; /// The cause of an error response /// @@ -115,7 +115,7 @@ impl dyn AppError { } impl AppError for Box { - fn response(&self) -> Option { + fn response(&self) -> Option { (**self).response() } @@ -171,7 +171,7 @@ impl ChainError for Option { } impl AppError for ChainedError { - fn response(&self) -> Option { + fn response(&self) -> Option { self.error.response() } @@ -190,7 +190,7 @@ impl fmt::Display for ChainedError { // Error impls impl AppError for E { - fn response(&self) -> Option { + fn response(&self) -> Option { None } } @@ -216,7 +216,7 @@ impl fmt::Display for InternalAppError { } impl AppError for InternalAppError { - fn response(&self) -> Option { + fn response(&self) -> Option { None } } @@ -227,8 +227,8 @@ impl AppError for InternalAppError { pub struct NotFound; impl AppError for NotFound { - fn response(&self) -> Option { - Some(json_error("Not Found", (404, "Not Found"))) + fn response(&self) -> Option { + Some(json_error("Not Found", StatusCode::NOT_FOUND)) } } @@ -242,9 +242,9 @@ impl fmt::Display for NotFound { pub struct Unauthorized; impl AppError for Unauthorized { - fn response(&self) -> Option { + fn response(&self) -> Option { let detail = "must be logged in to perform that action"; - Some(json_error(detail, (403, "Forbidden"))) + Some(json_error(detail, StatusCode::FORBIDDEN)) } } @@ -279,10 +279,10 @@ pub(crate) fn std_error(e: Box) -> Box { pub struct ReadOnlyMode; impl AppError for ReadOnlyMode { - fn response(&self) -> Option { + fn response(&self) -> Option { let detail = "Crates.io is currently in read-only mode for maintenance. \ Please try again later."; - Some(json_error(detail, (503, "Service Unavailable"))) + Some(json_error(detail, StatusCode::SERVICE_UNAVAILABLE)) } } @@ -298,7 +298,9 @@ pub struct TooManyRequests { } impl AppError for TooManyRequests { - fn response(&self) -> Option { + fn response(&self) -> Option { + use std::convert::TryInto; + const HTTP_DATE_FORMAT: &str = "%a, %d %b %Y %H:%M:%S GMT"; let retry_after = self.retry_after.format(HTTP_DATE_FORMAT); @@ -308,10 +310,14 @@ impl AppError for TooManyRequests { help@crates.io to have your limit increased.", retry_after ); - let mut response = json_error(&detail, (429, "TOO MANY REQUESTS")); - response - .headers - .insert("Retry-After".into(), vec![retry_after.to_string()]); + let mut response = json_error(&detail, StatusCode::TOO_MANY_REQUESTS); + response.headers_mut().insert( + header::RETRY_AFTER, + retry_after + .to_string() + .try_into() + .expect("HTTP_DATE_FORMAT contains invalid char"), + ); Some(response) } } diff --git a/src/util/errors/http.rs b/src/util/errors/http.rs index b7c7063ea57..25ef65035c8 100644 --- a/src/util/errors/http.rs +++ b/src/util/errors/http.rs @@ -1,8 +1,9 @@ use std::fmt; -use conduit::Response; - use super::{json_error, AppError}; +use crate::util::AppResponse; + +use conduit::StatusCode; #[derive(Debug)] pub(super) struct Ok(pub(super) String); @@ -12,8 +13,8 @@ pub(super) struct BadRequest(pub(super) String); pub(super) struct ServerError(pub(super) String); impl AppError for Ok { - fn response(&self) -> Option { - Some(json_error(&self.0, (200, "OK"))) + fn response(&self) -> Option { + Some(json_error(&self.0, StatusCode::OK)) } } @@ -24,8 +25,8 @@ impl fmt::Display for Ok { } impl AppError for BadRequest { - fn response(&self) -> Option { - Some(json_error(&self.0, (400, "Bad Request"))) + fn response(&self) -> Option { + Some(json_error(&self.0, StatusCode::BAD_REQUEST)) } } @@ -36,8 +37,8 @@ impl fmt::Display for BadRequest { } impl AppError for ServerError { - fn response(&self) -> Option { - Some(json_error(&self.0, (500, "Internal Server Error"))) + fn response(&self) -> Option { + Some(json_error(&self.0, StatusCode::INTERNAL_SERVER_ERROR)) } } diff --git a/src/util/request_helpers.rs b/src/util/request_helpers.rs index fc98603983b..cb514607d4d 100644 --- a/src/util/request_helpers.rs +++ b/src/util/request_helpers.rs @@ -1,18 +1,17 @@ -use conduit::Request; +use conduit::{header::AsHeaderName, RequestExt}; -/// Returns the value of the request header, or an empty string if it is not +/// Returns the value of the request header, or an empty slice if it is not /// present. /// -/// The implementation should return only the first line if it's a multiline -/// header, but rust-civet is just wrapping the whole thing in a `vec!` anyway, -/// and I have no clue what the C code it's wrapping does. +/// If a header appears multiple times, this will return only one of them. /// -/// The C library rust-civet is wrapping does not document whether headers are -/// case sensitive or not, so I have no clue if this follows the HTTP spec in -/// that regard. -pub fn request_header<'a>(req: &'a dyn Request, header_name: &str) -> &'a str { +/// If the header value is invalid utf8, an empty slice will be returned. +pub fn request_header<'a, K>(req: &'a dyn RequestExt, key: K) -> &'a str +where + K: AsHeaderName, +{ req.headers() - .find(header_name) - .and_then(|x| x.first().cloned()) + .get(key) + .map(|value| value.to_str().unwrap_or_default()) .unwrap_or_default() } diff --git a/src/util/request_proxy.rs b/src/util/request_proxy.rs index a1064b92557..ea3299fc5cc 100644 --- a/src/util/request_proxy.rs +++ b/src/util/request_proxy.rs @@ -2,10 +2,9 @@ use std::{io::Read, net::SocketAddr}; -use conduit::{Method, Request}; -use conduit_hyper::semver; +use conduit::{Method, RequestExt}; -type RequestMutRef<'a> = &'a mut (dyn Request + 'a); +type RequestMutRef<'a> = &'a mut (dyn RequestExt + 'a); // Can't derive Debug because of Request. #[allow(missing_debug_implementations)] @@ -35,10 +34,10 @@ impl<'a> RequestProxy<'a> { } } -impl<'a> Request for RequestProxy<'a> { +impl<'a> RequestExt for RequestProxy<'a> { // Use local value if available, defer to the original request - fn method(&self) -> conduit::Method { - self.method.clone().unwrap_or_else(|| self.other.method()) + fn method(&self) -> &conduit::Method { + self.method.as_ref().unwrap_or_else(|| &self.other.method()) } fn path(&self) -> &str { @@ -46,12 +45,9 @@ impl<'a> Request for RequestProxy<'a> { } // Pass-through - fn http_version(&self) -> semver::Version { + fn http_version(&self) -> conduit::Version { self.other.http_version() } - fn conduit_version(&self) -> semver::Version { - self.other.conduit_version() - } fn scheme(&self) -> conduit::Scheme { self.other.scheme() } @@ -70,7 +66,7 @@ impl<'a> Request for RequestProxy<'a> { fn content_length(&self) -> Option { self.other.content_length() } - fn headers(&self) -> &dyn conduit::Headers { + fn headers(&self) -> &conduit::HeaderMap { self.other.headers() } fn body(&mut self) -> &mut dyn Read { From 036dfdfaeb66bbc15d75670cfb857ba72e6735fc Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Sun, 1 Mar 2020 00:41:48 -0500 Subject: [PATCH 2/6] New conduit::Body API --- Cargo.lock | 41 ++++++++++++------------ Cargo.toml | 21 ++++++------ src/controllers.rs | 4 +-- src/middleware/block_traffic.rs | 4 +-- src/middleware/ensure_well_formed_500.rs | 2 +- src/middleware/head.rs | 3 +- src/middleware/require_user_agent.rs | 3 +- src/tests/all.rs | 16 ++++++--- src/tests/authentication.rs | 10 +++--- src/util.rs | 4 +-- 10 files changed, 56 insertions(+), 52 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 57a4e0c9028..6c740ad42b5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -213,6 +213,7 @@ dependencies = [ "handlebars", "hex", "htmlescape", + "http", "hyper", "hyper-tls", "indexmap", @@ -277,9 +278,9 @@ dependencies = [ [[package]] name = "civet" -version = "0.12.0-alpha.2" +version = "0.12.0-alpha.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b6ae786c5519f3740ffe88639d6d404c102c7af4acf206a94567c68f93d25b38" +checksum = "f360e1a48dd03961dfac6120d9d90a189e2274a09b00c9f536cce844d462239d" dependencies = [ "civet-sys", "conduit", @@ -319,18 +320,18 @@ dependencies = [ [[package]] name = "conduit" -version = "0.9.0-alpha.1" +version = "0.9.0-alpha.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2ca703f41f1b6a01299404ea407c2ae5b1cffc09a3d77af0e760130621cb3795" +checksum = "47432edd46e337326eed753ec7c9a9163a5e297f82a299d24f106221a2dee412" dependencies = [ "http", ] [[package]] name = "conduit-conditional-get" -version = "0.9.0-alpha.1" +version = "0.9.0-alpha.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9a686f3233875b24b228b3362738de7cd16c74a8c81d8acfc0617edf427a2741" +checksum = "769c01b79af748e1be176bea12603a2f7087608b84c1857a31ddd861d34de5a8" dependencies = [ "conduit", "conduit-middleware", @@ -339,9 +340,9 @@ dependencies = [ [[package]] name = "conduit-cookie" -version = "0.9.0-alpha.1" +version = "0.9.0-alpha.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7645e2fa9b3fdbae7ce77b29a7e353af2e616c936dae950e927c6bb5209c265a" +checksum = "fb6928c01306c79f16ec946b12846ae5e90bb0f4d8700a5d90373d72cb27711d" dependencies = [ "base64 0.11.0", "conduit", @@ -351,9 +352,9 @@ dependencies = [ [[package]] name = "conduit-git-http-backend" -version = "0.9.0-alpha.1" +version = "0.9.0-alpha.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7a5962ccb612bd8e435978519a0acc13879cb86c087902fc8f8c3ab7bf2a7d4a" +checksum = "2a41aef2b54111fa1703d7b033b518d279e5938b83e7fe94aa51bd9f3671c383" dependencies = [ "conduit", "flate2", @@ -361,9 +362,9 @@ dependencies = [ [[package]] name = "conduit-hyper" -version = "0.3.0-alpha.1" +version = "0.3.0-alpha.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c305c4a09c27fe168fad4539eef738b0898063b7ca8c26a7434605c9aa954be6" +checksum = "8c5bdc8858250eafc0d006e43002caa122b306d3571d0242e68e7770240a424e" dependencies = [ "conduit", "futures", @@ -376,9 +377,9 @@ dependencies = [ [[package]] name = "conduit-middleware" -version = "0.9.0-alpha.1" +version = "0.9.0-alpha.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "800bd3641b7b986cf1305f18243d50fe668f8b4d8bb543856119f55d84622223" +checksum = "bda9153060cd9c67b2f13e04c93f2f0d4f3c49c22e35763d7a700ce2a26afb04" dependencies = [ "conduit", ] @@ -394,9 +395,9 @@ dependencies = [ [[package]] name = "conduit-router" -version = "0.9.0-alpha.1" +version = "0.9.0-alpha.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "79f5fd8e955ceb288e230572935c323cd84f1042a2804ef5a8b41b1c67fd85c8" +checksum = "6eaa68ea0121fc61e2bf515f6dce485fa7674e57d17a2201762bfbc9380f65ec" dependencies = [ "conduit", "route-recognizer", @@ -404,9 +405,9 @@ dependencies = [ [[package]] name = "conduit-static" -version = "0.9.0-alpha.1" +version = "0.9.0-alpha.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b5b0cec0864a37f86320e0a0781d173b8c8fe7d878b16421c477a345f14bece8" +checksum = "e89d5ef0f88a5cbf0ba01e35ec2114d87527090508de47c3a1de7f5740ab69a5" dependencies = [ "conduit", "conduit-mime-types", @@ -416,9 +417,9 @@ dependencies = [ [[package]] name = "conduit-test" -version = "0.9.0-alpha.1" +version = "0.9.0-alpha.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a29e3574fbffef97d38eabe60757ca26a44d1f5aec8ea549543291cda91c73d9" +checksum = "19f72c6a9f57a45242f3df7a46907d1f1983d1ee279e8d9b8c355679732122dc" dependencies = [ "conduit", ] diff --git a/Cargo.toml b/Cargo.toml index 64d62055bd2..a884aac9ed6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -66,16 +66,17 @@ lettre = "0.9" lettre_email = "0.9" failure = "0.1.1" -conduit = "0.9.0-alpha.1" -conduit-conditional-get = "0.9.0-alpha.1" -conduit-cookie = "0.9.0-alpha.1" +conduit = "0.9.0-alpha.2" +conduit-conditional-get = "0.9.0-alpha.2" +conduit-cookie = "0.9.0-alpha.2" cookie = { version = "0.12", features = ["secure"] } -conduit-middleware = "0.9.0-alpha.1" -conduit-router = "0.9.0-alpha.1" -conduit-static = "0.9.0-alpha.1" -conduit-git-http-backend = "0.9.0-alpha.1" -civet = "0.12.0-alpha.2" -conduit-hyper = "0.3.0-alpha.1" +conduit-middleware = "0.9.0-alpha.2" +conduit-router = "0.9.0-alpha.2" +conduit-static = "0.9.0-alpha.2" +conduit-git-http-backend = "0.9.0-alpha.2" +civet = "0.12.0-alpha.3" +conduit-hyper = "0.3.0-alpha.2" +http = "0.2" futures-util = "0.3" futures-channel = { version = "0.3.1", default-features = false } @@ -86,7 +87,7 @@ indexmap = "1.0.2" handlebars = "3.0.1" [dev-dependencies] -conduit-test = "0.9.0-alpha.1" +conduit-test = "0.9.0-alpha.2" hyper-tls = "0.4" lazy_static = "1.0" diesel_migrations = { version = "1.3.0", features = ["postgres"] } diff --git a/src/controllers.rs b/src/controllers.rs index 78080a972f0..5cc6e2e3877 100644 --- a/src/controllers.rs +++ b/src/controllers.rs @@ -22,8 +22,6 @@ mod prelude { pub use crate::util::errors::{cargo_err, AppError, AppResult, ChainError}; // TODO: Remove cargo_err from here pub use crate::util::{AppResponse, EndpointResult}; - use std::io; - use indexmap::IndexMap; use serde::Serialize; @@ -57,7 +55,7 @@ mod prelude { conduit::Response::builder() .status(StatusCode::FOUND) .header(header::LOCATION, url) - .body(Box::new(io::empty()) as conduit::Body) + .body(conduit::Body::empty()) .unwrap() // Should not panic unless url contains "\r\n" } diff --git a/src/middleware/block_traffic.rs b/src/middleware/block_traffic.rs index 7e279ce3f38..64dd32878a8 100644 --- a/src/middleware/block_traffic.rs +++ b/src/middleware/block_traffic.rs @@ -10,8 +10,6 @@ use super::prelude::*; -use std::io::Cursor; - // Can't derive debug because of Handler. #[allow(missing_debug_implementations)] #[derive(Default)] @@ -65,7 +63,7 @@ impl Handler for BlockTraffic { Response::builder() .status(StatusCode::FORBIDDEN) .header(header::CONTENT_LENGTH, body.len()) - .body(Box::new(Cursor::new(body.into_bytes())) as Body) + .body(Body::from_vec(body.into_bytes()) as Body) .map_err(box_error) } else { self.handler.as_ref().unwrap().call(req) diff --git a/src/middleware/ensure_well_formed_500.rs b/src/middleware/ensure_well_formed_500.rs index df6d104ccc2..c096dca8a9d 100644 --- a/src/middleware/ensure_well_formed_500.rs +++ b/src/middleware/ensure_well_formed_500.rs @@ -14,7 +14,7 @@ impl Middleware for EnsureWellFormed500 { Response::builder() .status(StatusCode::INTERNAL_SERVER_ERROR) .header(header::CONTENT_LENGTH, body.len()) - .body(Box::new(body.as_bytes()) as Body) + .body(Body::from_static(body.as_bytes())) .map_err(box_error) }) } diff --git a/src/middleware/head.rs b/src/middleware/head.rs index 73a1f1c6dd9..98b4dcc4ba7 100644 --- a/src/middleware/head.rs +++ b/src/middleware/head.rs @@ -4,7 +4,6 @@ use super::prelude::*; use crate::util::RequestProxy; use conduit::Method; -use std::io; // Can't derive debug because of Handler. #[allow(missing_debug_implementations)] @@ -24,7 +23,7 @@ impl Handler for Head { if req.method() == Method::HEAD { let mut req = RequestProxy::rewrite_method(req, Method::GET); self.handler.as_ref().unwrap().call(&mut req).map(|mut r| { - *r.body_mut() = Box::new(io::empty()) as Body; + *r.body_mut() = Body::empty(); r }) } else { diff --git a/src/middleware/require_user_agent.rs b/src/middleware/require_user_agent.rs index f2d9145b4ab..f9945c9d294 100644 --- a/src/middleware/require_user_agent.rs +++ b/src/middleware/require_user_agent.rs @@ -3,7 +3,6 @@ use super::prelude::*; use crate::util::request_header; -use std::io::Cursor; // Can't derive debug because of Handler. #[allow(missing_debug_implementations)] @@ -32,7 +31,7 @@ impl Handler for RequireUserAgent { Response::builder() .status(StatusCode::FORBIDDEN) .header(header::CONTENT_LENGTH, body.len()) - .body(Box::new(Cursor::new(body.into_bytes())) as Body) + .body(Body::from_vec(body.into_bytes())) .map_err(box_error) } else { self.handler.as_ref().unwrap().call(req) diff --git a/src/tests/all.rs b/src/tests/all.rs index 2b61849b2bc..213972f99ec 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -28,7 +28,7 @@ use std::{ }, }; -use conduit::header; +use conduit::{header, Body}; use conduit_test::MockRequest; use diesel::prelude::*; use reqwest::{blocking::Client, Proxy}; @@ -198,9 +198,17 @@ fn json(r: &mut AppResponse) -> T where for<'de> T: serde::Deserialize<'de>, { - let mut data = Vec::new(); - r.body_mut().write_body(&mut data).unwrap(); - let s = std::str::from_utf8(&data).unwrap(); + use conduit::Body::*; + + let mut body = Body::empty(); + std::mem::swap(r.body_mut(), &mut body); + let body: std::borrow::Cow<'static, [u8]> = match body { + Static(slice) => slice.into(), + Owned(vec) => vec.into(), + File(_) => unimplemented!(), + }; + + let s = std::str::from_utf8(&body).unwrap(); match serde_json::from_str(s) { Ok(t) => t, Err(e) => panic!("failed to decode: {:?}\n{}", e, s), diff --git a/src/tests/authentication.rs b/src/tests/authentication.rs index 9ff5d59e085..8c88145ab54 100644 --- a/src/tests/authentication.rs +++ b/src/tests/authentication.rs @@ -15,11 +15,11 @@ fn call(app: &TestApp, mut request: MockRequest) -> HandlerResult { app.as_middleware().call(&mut request) } -fn into_parts(response: HandlerResult) -> (StatusCode, Vec) { - let mut response = response.unwrap(); - let mut body = Vec::new(); - response.body_mut().write_body(&mut body).unwrap(); - (response.status(), body) +fn into_parts(response: HandlerResult) -> (StatusCode, std::borrow::Cow<'static, [u8]>) { + use conduit_test::ResponseExt; + + let response = response.unwrap(); + (response.status(), response.into_cow()) } #[test] diff --git a/src/util.rs b/src/util.rs index 9f8d20c963d..9c2d59f96c2 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1,6 +1,6 @@ use std::cmp; -use conduit::{header, vec_to_body, Response}; +use conduit::{header, Body, Response}; use serde::Serialize; pub use self::errors::concrete::Error; @@ -29,7 +29,7 @@ pub fn json_response(t: &T) -> AppResponse { Response::builder() .header(header::CONTENT_TYPE, "application/json; charset=utf-8") .header(header::CONTENT_LENGTH, json.len()) - .body(vec_to_body(json.into_bytes())) + .body(Body::from_vec(json.into_bytes())) .unwrap() // Header values are well formed, so should not panic } From f345573e9488ae714c2ef5f0885fd491e4b9fa3e Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Tue, 3 Mar 2020 21:18:59 -0500 Subject: [PATCH 3/6] Output the status code as a static string slice --- src/middleware/log_request.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/log_request.rs b/src/middleware/log_request.rs index 7ff5b205089..9523ddd06b2 100644 --- a/src/middleware/log_request.rs +++ b/src/middleware/log_request.rs @@ -88,7 +88,7 @@ impl Display for RequestLine<'_> { line.add_field("request_id", request_header(self.req, "x-request-id"))?; line.add_quoted_field("fwd", request_header(self.req, "x-real-ip"))?; line.add_field("service", TimeMs(self.response_time))?; - line.add_field("status", status)?; + line.add_field("status", status.as_str())?; line.add_quoted_field("user_agent", request_header(self.req, header::USER_AGENT))?; if let Some(metadata) = self.req.extensions().find::() { From 5d0906e65663c1560cdf5bc7fa4e9c973e481d40 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Sun, 5 Apr 2020 13:46:03 -0400 Subject: [PATCH 4/6] Convert a few remaining integers to StatusCode --- src/router.rs | 16 ++++++++++------ src/tests/authentication.rs | 4 ++-- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/router.rs b/src/router.rs index 2dd99581ca2..73dddba95b8 100644 --- a/src/router.rs +++ b/src/router.rs @@ -191,6 +191,7 @@ mod tests { }; use crate::util::EndpointResult; + use conduit::StatusCode; use conduit_test::MockRequest; use diesel::result::Error as DieselError; @@ -205,25 +206,28 @@ mod tests { // Types for handling common error status codes assert_eq!( C(|_| Err(bad_request(""))).call(&mut req).unwrap().status(), - 400 + StatusCode::BAD_REQUEST ); assert_eq!( C(|_| err(Unauthorized)).call(&mut req).unwrap().status(), - 403 + StatusCode::FORBIDDEN ); assert_eq!( C(|_| Err(DieselError::NotFound.into())) .call(&mut req) .unwrap() .status(), - 404 + StatusCode::NOT_FOUND + ); + assert_eq!( + C(|_| err(NotFound)).call(&mut req).unwrap().status(), + StatusCode::NOT_FOUND ); - assert_eq!(C(|_| err(NotFound)).call(&mut req).unwrap().status(), 404); // cargo_err errors are returned as 200 so that cargo displays this nicely on the command line assert_eq!( C(|_| Err(cargo_err(""))).call(&mut req).unwrap().status(), - 200 + StatusCode::OK ); // Inner errors are captured for logging when wrapped by a user facing error @@ -236,7 +240,7 @@ mod tests { }) .call(&mut req) .unwrap(); - assert_eq!(response.status(), 400); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); assert_eq!( crate::middleware::log_request::get_log_message(&req, "cause"), "middle error caused by invalid digit found in string" diff --git a/src/tests/authentication.rs b/src/tests/authentication.rs index 8c88145ab54..68948d6eda7 100644 --- a/src/tests/authentication.rs +++ b/src/tests/authentication.rs @@ -28,7 +28,7 @@ fn anonymous_user_unauthorized() { let request = anon.request_builder(Method::GET, URL); let (status, body) = into_parts(call(&app, request)); - assert_eq!(status, 403); + assert_eq!(status, StatusCode::FORBIDDEN); assert_eq!(body, MUST_LOGIN); } @@ -39,7 +39,7 @@ fn token_auth_cannot_find_token() { request.header(header::AUTHORIZATION, "fake-token"); let (status, body) = into_parts(call(&app, request)); - assert_eq!(status, 403); + assert_eq!(status, StatusCode::FORBIDDEN); assert_eq!(body, MUST_LOGIN); } From 858f5efd3acaffd5b26ca4a9701597fd27d85fe2 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Sun, 5 Apr 2020 14:21:21 -0400 Subject: [PATCH 5/6] Make clippy happy --- src/util/request_helpers.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/request_helpers.rs b/src/util/request_helpers.rs index cb514607d4d..88a1ed22cec 100644 --- a/src/util/request_helpers.rs +++ b/src/util/request_helpers.rs @@ -6,7 +6,7 @@ use conduit::{header::AsHeaderName, RequestExt}; /// If a header appears multiple times, this will return only one of them. /// /// If the header value is invalid utf8, an empty slice will be returned. -pub fn request_header<'a, K>(req: &'a dyn RequestExt, key: K) -> &'a str +pub fn request_header(req: &dyn RequestExt, key: K) -> &str where K: AsHeaderName, { From 6985639ebbc0772f758ecc1e006be21c841804f9 Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Sun, 12 Apr 2020 10:22:50 -0400 Subject: [PATCH 6/6] Drop a cast that isn't necesary --- src/middleware/block_traffic.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/block_traffic.rs b/src/middleware/block_traffic.rs index 64dd32878a8..48283b301d0 100644 --- a/src/middleware/block_traffic.rs +++ b/src/middleware/block_traffic.rs @@ -63,7 +63,7 @@ impl Handler for BlockTraffic { Response::builder() .status(StatusCode::FORBIDDEN) .header(header::CONTENT_LENGTH, body.len()) - .body(Body::from_vec(body.into_bytes()) as Body) + .body(Body::from_vec(body.into_bytes())) .map_err(box_error) } else { self.handler.as_ref().unwrap().call(req)