diff --git a/Cargo.lock b/Cargo.lock index ba15b2994..3fe9dc280 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -999,6 +999,7 @@ dependencies = [ "dotenv", "env_logger", "failure", + "fn-error-context", "font-awesome-as-a-crate", "futures-util", "getrandom 0.2.7", @@ -1207,6 +1208,17 @@ dependencies = [ "miniz_oxide", ] +[[package]] +name = "fn-error-context" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "236b4e4ae2b8be5f7a5652f6108c4a0f2627c569db4e7923333d31c7dbfed0fb" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "fnv" version = "1.0.7" diff --git a/Cargo.toml b/Cargo.toml index 43071d5ef..c725b10ec 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -113,6 +113,7 @@ kuchiki = "0.8" rand = "0.8" mockito = "0.31.0" test-case = "2.0.0" +fn-error-context = "0.2.0" [build-dependencies] time = "0.3" diff --git a/src/test/mod.rs b/src/test/mod.rs index 0b51f8787..1e67ce5de 100644 --- a/src/test/mod.rs +++ b/src/test/mod.rs @@ -7,16 +7,16 @@ use crate::repositories::RepositoryStatsUpdater; use crate::storage::{Storage, StorageKind}; use crate::web::Server; use crate::{BuildQueue, Config, Context, Index, Metrics}; +use anyhow::Context as _; +use fn_error_context::context; use log::error; use once_cell::unsync::OnceCell; use postgres::Client as Connection; use reqwest::{ - blocking::{Client, RequestBuilder}, + blocking::{Client, ClientBuilder, RequestBuilder}, Method, }; -use std::fs; -use std::net::SocketAddr; -use std::{panic, sync::Arc}; +use std::{fs, net::SocketAddr, panic, sync::Arc, time::Duration}; pub(crate) fn wrapper(f: impl FnOnce(&TestEnvironment) -> Result<()>) { let _ = dotenv::dotenv(); @@ -56,45 +56,57 @@ pub(crate) fn assert_not_found(path: &str, web: &TestFrontend) -> Result<()> { Ok(()) } -/// Make sure that a URL redirects to a specific page -pub(crate) fn assert_redirect(path: &str, expected_target: &str, web: &TestFrontend) -> Result<()> { - // Reqwest follows redirects automatically - let response = web.get(path).send()?; +fn assert_redirect_common(path: &str, expected_target: &str, web: &TestFrontend) -> Result<()> { + let response = web.get_no_redirect(path).send()?; let status = response.status(); + if !status.is_redirection() { + anyhow::bail!("non-redirect from GET {path}: {status}"); + } + + let mut redirect_target = response + .headers() + .get("Location") + .context("missing 'Location' header")? + .to_str() + .context("non-ASCII redirect")?; + + if !expected_target.starts_with("http") { + // TODO: Should be able to use Url::make_relative, + // but https://github.com/servo/rust-url/issues/766 + let base = format!("http://{}", web.server_addr()); + redirect_target = redirect_target + .strip_prefix(&base) + .unwrap_or(redirect_target); + } - let mut tmp; - let redirect_target = if expected_target.starts_with("https://") { - response.url().as_str() - } else { - tmp = String::from(response.url().path()); - if let Some(query) = response.url().query() { - tmp.push('?'); - tmp.push_str(query); - } - &tmp - }; - // Either we followed a redirect to the wrong place, or there was no redirect if redirect_target != expected_target { - // wrong place - if redirect_target != path { - panic!( - "{}: expected redirect to {}, got redirect to {}", - path, expected_target, redirect_target - ); - } else { - // no redirect - panic!( - "{}: expected redirect to {}, got {}", - path, expected_target, status - ); - } + anyhow::bail!("got redirect to {redirect_target}"); } - assert!( - status.is_success(), - "failed to GET {}: {}", - expected_target, - status - ); + + Ok(()) +} + +/// Makes sure that a URL redirects to a specific page, but doesn't check that the target exists +#[context("expected redirect from {path} to {expected_target}")] +pub(crate) fn assert_redirect_unchecked( + path: &str, + expected_target: &str, + web: &TestFrontend, +) -> Result<()> { + assert_redirect_common(path, expected_target, web) +} + +/// Make sure that a URL redirects to a specific page, and that the target exists and is not another redirect +#[context("expected redirect from {path} to {expected_target}")] +pub(crate) fn assert_redirect(path: &str, expected_target: &str, web: &TestFrontend) -> Result<()> { + assert_redirect_common(path, expected_target, web)?; + + let response = web.get_no_redirect(expected_target).send()?; + let status = response.status(); + if !status.is_success() { + anyhow::bail!("failed to GET {expected_target}: {status}"); + } + Ok(()) } @@ -113,6 +125,7 @@ pub(crate) fn init_logger() { // initializing rustwide logging also sets the global logger rustwide::logging::init_with( env_logger::Builder::from_env(env_logger::Env::default().filter("DOCSRS_LOG")) + .format_timestamp_millis() .is_test(true) .build(), ); @@ -372,22 +385,35 @@ impl Drop for TestDatabase { pub(crate) struct TestFrontend { server: Server, pub(crate) client: Client, + pub(crate) client_no_redirect: Client, } impl TestFrontend { fn new(context: &dyn Context) -> Self { + fn build(f: impl FnOnce(ClientBuilder) -> ClientBuilder) -> Client { + let base = Client::builder() + .connect_timeout(Duration::from_millis(2000)) + .timeout(Duration::from_millis(2000)) + // The test server only supports a single connection, so having two clients with + // idle connections deadlocks the tests + .pool_max_idle_per_host(0); + f(base).build().unwrap() + } + Self { server: Server::start(Some("127.0.0.1:0"), context) .expect("failed to start the web server"), - client: Client::new(), + client: build(|b| b), + client_no_redirect: build(|b| b.redirect(reqwest::redirect::Policy::none())), } } - fn build_request(&self, method: Method, mut url: String) -> RequestBuilder { + fn build_url(&self, url: &str) -> String { if url.is_empty() || url.starts_with('/') { - url = format!("http://{}{}", self.server.addr(), url); + format!("http://{}{}", self.server.addr(), url) + } else { + url.to_owned() } - self.client.request(method, url) } pub(crate) fn server_addr(&self) -> SocketAddr { @@ -395,6 +421,14 @@ impl TestFrontend { } pub(crate) fn get(&self, url: &str) -> RequestBuilder { - self.build_request(Method::GET, url.to_string()) + let url = self.build_url(url); + log::debug!("getting {url}"); + self.client.request(Method::GET, url) + } + + pub(crate) fn get_no_redirect(&self, url: &str) -> RequestBuilder { + let url = self.build_url(url); + log::debug!("getting {url} (no redirects)"); + self.client_no_redirect.request(Method::GET, url) } } diff --git a/src/web/crate_details.rs b/src/web/crate_details.rs index 890b7d080..2afe1c468 100644 --- a/src/web/crate_details.rs +++ b/src/web/crate_details.rs @@ -76,6 +76,7 @@ pub struct Release { pub yanked: bool, pub is_library: bool, pub rustdoc_status: bool, + pub target_name: String, } impl CrateDetails { @@ -246,15 +247,16 @@ pub(crate) fn releases_for_crate( ) -> Result, anyhow::Error> { let mut releases: Vec = conn .query( - "SELECT - id, + "SELECT + id, version, build_status, yanked, is_library, - rustdoc_status + rustdoc_status, + target_name FROM releases - WHERE + WHERE releases.crate_id = $1", &[&crate_id], )? @@ -269,6 +271,7 @@ pub(crate) fn releases_for_crate( yanked: row.get("yanked"), is_library: row.get("is_library"), rustdoc_status: row.get("rustdoc_status"), + target_name: row.get("target_name"), }), Err(err) => { report_error(&anyhow!(err).context(format!( @@ -505,6 +508,7 @@ mod tests { is_library: true, rustdoc_status: true, id: details.releases[0].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.12.0")?, @@ -513,6 +517,7 @@ mod tests { is_library: true, rustdoc_status: true, id: details.releases[1].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.3.0")?, @@ -521,6 +526,7 @@ mod tests { is_library: true, rustdoc_status: false, id: details.releases[2].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.2.0")?, @@ -529,6 +535,7 @@ mod tests { is_library: true, rustdoc_status: true, id: details.releases[3].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.2.0-alpha")?, @@ -537,6 +544,7 @@ mod tests { is_library: true, rustdoc_status: true, id: details.releases[4].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.1.1")?, @@ -545,6 +553,7 @@ mod tests { is_library: true, rustdoc_status: true, id: details.releases[5].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.1.0")?, @@ -553,6 +562,7 @@ mod tests { is_library: true, rustdoc_status: true, id: details.releases[6].id, + target_name: "foo".to_owned(), }, Release { version: semver::Version::parse("0.0.1")?, @@ -561,6 +571,7 @@ mod tests { is_library: false, rustdoc_status: false, id: details.releases[7].id, + target_name: "foo".to_owned(), }, ] ); diff --git a/src/web/metrics.rs b/src/web/metrics.rs index fc3d677da..7ba1794d6 100644 --- a/src/web/metrics.rs +++ b/src/web/metrics.rs @@ -92,6 +92,12 @@ impl<'a> RenderingTimesRecorder<'a> { fn record_current(&mut self) { if let Some(current) = self.current.take() { + #[cfg(test)] + log::debug!( + "rendering time - {}: {:?}", + current.step, + current.start.elapsed() + ); self.metric .with_label_values(&[current.step]) .observe(duration_to_seconds(current.start.elapsed())); diff --git a/src/web/mod.rs b/src/web/mod.rs index ff9cf0cd1..b9027f7de 100644 --- a/src/web/mod.rs +++ b/src/web/mod.rs @@ -223,6 +223,7 @@ struct MatchVersion { pub corrected_name: Option, pub version: MatchSemver, pub rustdoc_status: bool, + pub target_name: String, } impl MatchVersion { @@ -324,6 +325,7 @@ fn match_version( corrected_name, version: MatchSemver::Exact((release.version.to_string(), release.id)), rustdoc_status: release.rustdoc_status, + target_name: release.target_name.clone(), }); } } @@ -358,6 +360,7 @@ fn match_version( MatchSemver::Semver((release.version.to_string(), release.id)) }, rustdoc_status: release.rustdoc_status, + target_name: release.target_name.clone(), }); } @@ -371,6 +374,7 @@ fn match_version( corrected_name: corrected_name.clone(), version: MatchSemver::Semver((release.version.to_string(), release.id)), rustdoc_status: release.rustdoc_status, + target_name: release.target_name.clone(), }) .ok_or(Nope::VersionNotFound); } @@ -759,7 +763,7 @@ mod test { .create() .unwrap(); let web = env.frontend(); - assert_redirect("/bat//", "/bat/latest/bat/", web)?; + assert_redirect_unchecked("/bat//", "/bat/", web)?; Ok(()) }) } diff --git a/src/web/releases.rs b/src/web/releases.rs index a5a734df5..c74652169 100644 --- a/src/web/releases.rs +++ b/src/web/releases.rs @@ -79,7 +79,7 @@ pub(crate) fn get_releases(conn: &mut Client, page: i64, limit: i64, order: Orde INNER JOIN builds ON releases.id = builds.rid LEFT JOIN repositories ON releases.repository_id = repositories.id WHERE - ((NOT $3) OR (releases.build_status = FALSE AND releases.is_library = TRUE)) + ((NOT $3) OR (releases.build_status = FALSE AND releases.is_library = TRUE)) AND {0} IS NOT NULL ORDER BY {0} DESC @@ -246,7 +246,7 @@ fn get_search_results( releases.rustdoc_status, repositories.stars - FROM crates + FROM crates INNER JOIN releases ON crates.latest_version_id = releases.id INNER JOIN builds ON releases.id = builds.rid LEFT JOIN repositories ON releases.repository_id = repositories.id @@ -498,7 +498,7 @@ fn redirect_to_random_crate(req: &Request, conn: &mut PoolClient) -> IronResult< releases.version, releases.target_name FROM ( - -- generate random numbers in the ID-range. + -- generate random numbers in the ID-range. SELECT DISTINCT 1 + trunc(random() * params.max_id)::INTEGER AS id FROM params, generate_series(1, $1) ) AS r @@ -573,27 +573,15 @@ pub fn search_handler(req: &mut Request) -> IronResult { let (version, _) = matchver.version.into_parts(); let krate = matchver.corrected_name.unwrap_or(krate); + let base = redirect_base(req); let url = if matchver.rustdoc_status { + let target_name = matchver.target_name; ctry!( req, - Url::parse(&format!( - "{}/{}/{}/{}", - redirect_base(req), - krate, - version, - query - )), + Url::parse(&format!("{base}/{krate}/{version}/{target_name}/{query}")) ) } else { - ctry!( - req, - Url::parse(&format!( - "{}/crate/{}/{}", - redirect_base(req), - krate, - version, - )), - ) + ctry!(req, Url::parse(&format!("{base}/crate/{krate}/{version}"))) }; let mut resp = Response::with((status::Found, Redirect(url))); @@ -688,12 +676,12 @@ pub fn activity_handler(req: &mut Request) -> IronResult { " WITH dates AS ( -- we need this series so that days in the statistic that don't have any releases are included - SELECT generate_series( + SELECT generate_series( CURRENT_DATE - INTERVAL '30 days', CURRENT_DATE - INTERVAL '1 day', '1 day'::interval )::date AS date_ - ), + ), release_stats AS ( SELECT release_time::date AS date_, @@ -706,16 +694,16 @@ pub fn activity_handler(req: &mut Request) -> IronResult { release_time < CURRENT_DATE GROUP BY release_time::date - ) - SELECT + ) + SELECT dates.date_ AS date, COALESCE(rs.counts, 0) AS counts, - COALESCE(rs.failures, 0) AS failures + COALESCE(rs.failures, 0) AS failures FROM - dates + dates LEFT OUTER JOIN Release_stats AS rs ON dates.date_ = rs.date_ - ORDER BY + ORDER BY dates.date_ ", &[], diff --git a/src/web/routes.rs b/src/web/routes.rs index 5c666f8e8..5d1f74126 100644 --- a/src/web/routes.rs +++ b/src/web/routes.rs @@ -173,22 +173,38 @@ pub(super) fn build_routes() -> Routes { &format!("/{}", redirect), super::rustdoc::RustLangRedirector::new("stable", redirect), ); + routes.internal_page( + &format!("/{}/", redirect), + super::rustdoc::RustLangRedirector::new("stable", redirect), + ); } // redirect proc-macro to proc_macro routes.internal_page( "/proc-macro", super::rustdoc::RustLangRedirector::new("stable", "proc_macro"), ); + routes.internal_page( + "/proc-macro/", + super::rustdoc::RustLangRedirector::new("stable", "proc_macro"), + ); // redirect rustc to nightly rustc docs routes.internal_page( "/rustc", super::rustdoc::RustLangRedirector::new("nightly", "nightly-rustc"), ); + routes.internal_page( + "/rustc/", + super::rustdoc::RustLangRedirector::new("nightly", "nightly-rustc"), + ); // redirect rustdoc to nightly rustdoc docs routes.internal_page( "/rustdoc", super::rustdoc::RustLangRedirector::new("nightly", "nightly-rustc/rustdoc"), ); + routes.internal_page( + "/rustdoc/", + super::rustdoc::RustLangRedirector::new("nightly", "nightly-rustc/rustdoc"), + ); routes } diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index b9e8bd594..53b873393 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -20,7 +20,7 @@ use iron::{ use lol_html::errors::RewritingError; use router::Router; use serde::Serialize; -use std::path::Path; +use std::{fmt::Write, path::Path}; #[derive(Clone)] pub struct RustLangRedirector { @@ -29,9 +29,8 @@ pub struct RustLangRedirector { impl RustLangRedirector { pub fn new(version: &str, target: &str) -> Self { - let url = - iron::url::Url::parse(&format!("https://doc.rust-lang.org/{}/{}", version, target)) - .expect("failed to parse rust-lang.org doc URL"); + let url = iron::url::Url::parse(&format!("https://doc.rust-lang.org/{version}/{target}/")) + .expect("failed to parse rust-lang.org doc URL"); let url = Url::from_generic_url(url).expect("failed to convert url::Url to iron::Url"); Self { url } @@ -547,8 +546,8 @@ pub fn rustdoc_html_server_handler(req: &mut Request) -> IronResult { /// Note that path is overloaded in this context to mean both the path of a URL /// and the file path of a static file in the DB. /// -/// `req_path` is assumed to have the following format: -/// `rustdoc/crate/version[/platform]/module/[kind.name.html|index.html]` +/// `file_path` is assumed to have the following format: +/// `[/platform]/module/[kind.name.html|index.html]` /// /// Returns a path that can be appended to `/crate/version/` to create a complete URL. fn path_for_version(file_path: &[&str], crate_details: &CrateDetails) -> String { @@ -589,11 +588,16 @@ fn path_for_version(file_path: &[&str], crate_details: &CrateDetails) -> String // else, don't try searching at all, we don't know how to find it last_component.strip_suffix(".rs.html") }; - if let Some(search) = search_item { - format!("{}?search={}", platform, search) + let target_name = &crate_details.target_name; + let mut result = if platform.is_empty() { + format!("{target_name}/") } else { - platform.to_owned() + format!("{platform}/{target_name}/") + }; + if let Some(search) = search_item { + write!(result, "?search={search}").unwrap(); } + result } pub fn target_redirect_handler(req: &mut Request) -> IronResult { @@ -663,13 +667,7 @@ pub fn target_redirect_handler(req: &mut Request) -> IronResult { path_for_version(&file_path, &crate_details) }; - let url = format!( - "{base}/{name}/{version_or_latest}/{path}", - base = base, - name = name, - version_or_latest = version_or_latest, - path = path - ); + let url = format!("{base}/{name}/{version_or_latest}/{path}"); let url = ctry!(req, Url::parse(&url)); let mut resp = Response::with((status::Found, Redirect(url)));