From 508776800085ab3bb3737a6d3fc12a6db2506d59 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Fri, 6 Dec 2019 20:49:37 -0500 Subject: [PATCH 1/3] Fix redirects if latest version failed to build Closes https://github.com/rust-lang/docs.rs/issues/502 The redirector behaved differently for `/:crate/:version/:target` than for `/:crate/:version/:target/`. This changes the latest version link to use the former, since it properly accounts for failed builds. Additionally, this changes the `/:target` redirect to keep the query parameter so that `?search=` URLs will be kept. --- src/web/rustdoc.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index ca99171bd..2b16565c1 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -98,11 +98,13 @@ pub fn rustdoc_redirector_handler(req: &mut Request) -> IronResult { vers: &str, target_name: &str) -> IronResult { - let url = ctry!(Url::parse(&format!("{}/{}/{}/{}/", + let url = ctry!(Url::parse(&format!("{}/{}/{}/{}/?{}", redirect_base(req), name, vers, - target_name)[..])); + target_name, + req.url.query().unwrap_or_default() + )[..])); let mut resp = Response::with((status::Found, Redirect(url))); resp.headers.set(Expires(HttpDate(time::now()))); @@ -343,7 +345,7 @@ fn path_for_version(req_path: &[&str], target_name: &str, conn: &Connection) -> } else { req_path[3] }; - format!("{}/?search={}", crate_root, search_item) + format!("{}?search={}", crate_root, search_item) } pub fn badge_handler(req: &mut Request) -> IronResult { From 095e24f1be987756567bf6322a4479297a1ca590 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Thu, 12 Dec 2019 17:18:34 -0500 Subject: [PATCH 2/3] don't include '?' if no query already exists --- src/web/rustdoc.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index 2b16565c1..b9ac8c570 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -98,13 +98,18 @@ pub fn rustdoc_redirector_handler(req: &mut Request) -> IronResult { vers: &str, target_name: &str) -> IronResult { - let url = ctry!(Url::parse(&format!("{}/{}/{}/{}/?{}", - redirect_base(req), - name, - vers, - target_name, - req.url.query().unwrap_or_default() - )[..])); + let mut url_str = format!( + "{}/{}/{}/{}/", + redirect_base(req), + name, + vers, + target_name, + ); + if let Some(query) = req.url.query() { + url_str.push('?'); + url_str.push_str(query); + } + let url = ctry!(Url::parse(&url_str[..])); let mut resp = Response::with((status::Found, Redirect(url))); resp.headers.set(Expires(HttpDate(time::now()))); From 1934869813aca2fdada59ef5e86751eb88e7bb13 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 1 Feb 2020 15:17:59 -0500 Subject: [PATCH 3/3] add unit tests - add query parameter if it exists - don't call `env.frontend()` a dozen times - add tests --- src/test/mod.rs | 8 +++++++- src/web/rustdoc.rs | 30 +++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/test/mod.rs b/src/test/mod.rs index fbb61e7ac..2944e1c08 100644 --- a/src/test/mod.rs +++ b/src/test/mod.rs @@ -43,10 +43,16 @@ pub(crate) fn assert_redirect(path: &str, expected_target: &str, web: &TestFront let response = web.get(path).send()?; let status = response.status(); + let mut tmp; let redirect_target = if expected_target.starts_with("https://") { response.url().as_str() } else { - response.url().path() + 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 { diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index b9ac8c570..e14e2ff20 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -545,19 +545,22 @@ mod test { .rustdoc_file("dummy/blah/blah.html", b"lah") .create()?; + let web = env.frontend(); + // check it works at all - let redirect = latest_version_redirect("/dummy/0.1.0/dummy/", &env.frontend())?; + let redirect = latest_version_redirect("/dummy/0.1.0/dummy/", &web)?; assert_eq!(redirect, "/dummy/0.2.0/dummy/index.html"); // check it keeps the subpage - let redirect = latest_version_redirect("/dummy/0.1.0/dummy/blah/", &env.frontend())?; + let redirect = latest_version_redirect("/dummy/0.1.0/dummy/blah/", &web)?; assert_eq!(redirect, "/dummy/0.2.0/dummy/blah/index.html"); - let redirect = latest_version_redirect("/dummy/0.1.0/dummy/blah/blah.html", &env.frontend())?; + let redirect = latest_version_redirect("/dummy/0.1.0/dummy/blah/blah.html", &web)?; assert_eq!(redirect, "/dummy/0.2.0/dummy/blah/blah.html"); // check it searches for removed pages - let redirect = latest_version_redirect("/dummy/0.1.0/dummy/struct.will-be-deleted.html", &env.frontend())?; - assert_eq!(redirect, "/dummy/0.2.0/dummy/?search=will-be-deleted"); + let redirect = latest_version_redirect("/dummy/0.1.0/dummy/struct.will-be-deleted.html", &web)?; + assert_eq!(redirect, "/dummy/0.2.0/dummy?search=will-be-deleted"); + assert_redirect("/dummy/0.2.0/dummy?search=will-be-deleted", "/dummy/0.2.0/dummy/?search=will-be-deleted", &web).unwrap(); Ok(()) }) @@ -582,6 +585,23 @@ mod test { let redirect = latest_version_redirect("/dummy/0.1.0/x86_64-pc-windows-msvc/dummy/", web)?; assert_eq!(redirect, "/dummy/0.2.0/x86_64-pc-windows-msvc/dummy/index.html"); + Ok(()) + }) + } + #[test] + fn redirect_latest_goes_to_crate_if_build_failed() { + wrapper(|env| { + let db = env.db(); + db.fake_release().name("dummy").version("0.1.0") + .rustdoc_file("dummy/index.html", b"lah") + .create().unwrap(); + db.fake_release().name("dummy").version("0.2.0") + .build_result_successful(false).create().unwrap(); + + let web = env.frontend(); + let redirect = latest_version_redirect("/dummy/0.1.0/dummy/", web)?; + assert_eq!(redirect, "/crate/dummy/0.2.0"); + Ok(()) }) }