From d333ab50302686da8a7c179c754cbbc0c7e0aca0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emil=20Gardstr=C3=B6m?= Date: Tue, 13 Sep 2022 16:35:37 +0200 Subject: [PATCH 1/4] keep queries when doing lucky, `::` or crate-name searches enables `docs.rs/releases/search?query=tokio::spawn&i-am-feeling-lucky=1&go_to_first=true` and `docs.rs/tokio::spawn?go_to_first=true` to go to first result after redirect --- src/web/releases.rs | 31 ++++++++++++++++++++++++++++++- src/web/rustdoc.rs | 20 ++++++++++++++++---- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/src/web/releases.rs b/src/web/releases.rs index c74652169..72c13fe8c 100644 --- a/src/web/releases.rs +++ b/src/web/releases.rs @@ -561,11 +561,25 @@ pub fn search_handler(req: &mut Request) -> IronResult { return redirect_to_random_crate(req, &mut conn); } - let (krate, query) = match query.split_once("::") { + let (krate, mut query) = match query.split_once("::") { Some((krate, query)) => (krate.to_string(), format!("?search={query}")), None => (query.clone(), "".to_string()), }; + for (k, v) in params + .iter() + .filter(|(k, _)| !matches!(k.as_ref(), "i-am-feeling-lucky" | "query")) + { + if query.is_empty() { + query.push('?'); + } else { + query.push('&') + } + query.push_str(k); + query.push('='); + query.push_str(v); + } + // since we never pass a version into `match_version` here, we'll never get // `MatchVersion::Exact`, so the distinction between `Exact` and `Semver` doesn't // matter @@ -884,6 +898,21 @@ mod tests { }) } + #[test] + fn search_coloncolon_path_redirects_to_crate_docs_and_keeps_query() { + wrapper(|env| { + let web = env.frontend(); + env.fake_release().name("some_random_crate").create()?; + + assert_redirect( + "/releases/search?query=some_random_crate::somepath&go_to_first=true", + "/some_random_crate/1.0.0/some_random_crate/?search=somepath&go_to_first=true", + web, + )?; + Ok(()) + }) + } + #[test] fn search_result_passes_cratesio_pagination_links() { wrapper(|env| { diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index 96985c9ee..a99219dfe 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -46,12 +46,19 @@ pub fn rustdoc_redirector_handler(req: &mut Request) -> IronResult { permanent: bool, path_in_crate: Option<&str>, ) -> IronResult { - if let Some(query) = req.url.query() { - url_str.push('?'); - url_str.push_str(query); - } else if let Some(path) = path_in_crate { + let mut question_mark = false; + if let Some(path) = path_in_crate { url_str.push_str("?search="); url_str.push_str(path); + question_mark = true; + } + if let Some(query) = req.url.query() { + if !question_mark { + url_str.push('?'); + } else { + url_str.push('&'); + } + url_str.push_str(query); } let url = ctry!(req, Url::parse(&url_str)); let (status_code, max_age) = if permanent { @@ -1776,6 +1783,11 @@ mod test { "/some_random_crate/latest/some_random_crate/?search=some::path", web, )?; + assert_redirect( + "/some_random_crate::some::path?go_to_first=true", + "/some_random_crate/latest/some_random_crate/?search=some::path&go_to_first=true", + web, + )?; assert_redirect( "/std::some::path", From 383009b8ee7771d7a6a8538adaa4b0fdfeefa45e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emil=20Gardstr=C3=B6m?= Date: Sat, 17 Sep 2022 21:20:50 +0200 Subject: [PATCH 2/4] code review --- src/web/releases.rs | 50 +++++++++++++++++++++++++-------------------- src/web/rustdoc.rs | 35 ++++++++++++++++--------------- 2 files changed, 46 insertions(+), 39 deletions(-) diff --git a/src/web/releases.rs b/src/web/releases.rs index 72c13fe8c..13698755a 100644 --- a/src/web/releases.rs +++ b/src/web/releases.rs @@ -561,24 +561,22 @@ pub fn search_handler(req: &mut Request) -> IronResult { return redirect_to_random_crate(req, &mut conn); } - let (krate, mut query) = match query.split_once("::") { - Some((krate, query)) => (krate.to_string(), format!("?search={query}")), - None => (query.clone(), "".to_string()), - }; + let mut queries = std::collections::BTreeMap::new(); - for (k, v) in params - .iter() - .filter(|(k, _)| !matches!(k.as_ref(), "i-am-feeling-lucky" | "query")) - { - if query.is_empty() { - query.push('?'); - } else { - query.push('&') + let krate = match query.split_once("::") { + Some((krate, query)) => { + queries.insert("search", query); + krate.to_string() } - query.push_str(k); - query.push('='); - query.push_str(v); - } + None => query.clone(), + }; + + queries.extend( + params + .iter() + .filter(|(k, _)| !matches!(k.as_ref(), "i-am-feeling-lucky" | "query")) + .map(|(k, v)| (k.as_ref(), v.as_ref())), + ); // since we never pass a version into `match_version` here, we'll never get // `MatchVersion::Exact`, so the distinction between `Exact` and `Semver` doesn't @@ -590,10 +588,18 @@ pub fn search_handler(req: &mut Request) -> IronResult { let base = redirect_base(req); let url = if matchver.rustdoc_status { let target_name = matchver.target_name; - ctry!( - req, - Url::parse(&format!("{base}/{krate}/{version}/{target_name}/{query}")) - ) + let path = format!("{base}/{krate}/{version}/{target_name}/"); + if queries.is_empty() { + ctry!(req, Url::parse(&path)) + } else { + ctry!( + req, + Url::from_generic_url(ctry!( + req, + iron::url::Url::parse_with_params(&path, queries) + )) + ) + } } else { ctry!(req, Url::parse(&format!("{base}/crate/{krate}/{version}"))) }; @@ -891,7 +897,7 @@ mod tests { )?; assert_redirect( "/releases/search?query=some_random_crate::some::path", - "/some_random_crate/1.0.0/some_random_crate/?search=some::path", + "/some_random_crate/1.0.0/some_random_crate/?search=some%3A%3Apath", web, )?; Ok(()) @@ -906,7 +912,7 @@ mod tests { assert_redirect( "/releases/search?query=some_random_crate::somepath&go_to_first=true", - "/some_random_crate/1.0.0/some_random_crate/?search=somepath&go_to_first=true", + "/some_random_crate/1.0.0/some_random_crate/?go_to_first=true&search=somepath", web, )?; Ok(()) diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index a99219dfe..c26a32faa 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -42,30 +42,31 @@ static DOC_RUST_LANG_ORG_REDIRECTS: Lazy> = Lazy::new(|| { pub fn rustdoc_redirector_handler(req: &mut Request) -> IronResult { fn redirect_to_doc( req: &Request, - mut url_str: String, + url_str: String, permanent: bool, path_in_crate: Option<&str>, ) -> IronResult { - let mut question_mark = false; + let mut queries: std::collections::BTreeMap< + std::borrow::Cow<'_, str>, + std::borrow::Cow<'_, str>, + > = std::collections::BTreeMap::new(); if let Some(path) = path_in_crate { - url_str.push_str("?search="); - url_str.push_str(path); - question_mark = true; + queries.insert("search".into(), path.into()); } - if let Some(query) = req.url.query() { - if !question_mark { - url_str.push('?'); - } else { - url_str.push('&'); - } - url_str.push_str(query); - } - let url = ctry!(req, Url::parse(&url_str)); + let url: iron::url::Url = req.url.clone().into(); + let query_pairs = url.query_pairs(); + queries.extend(query_pairs); + let url = if queries.is_empty() { + ctry!(req, iron::url::Url::parse(&url_str)) + } else { + ctry!(req, iron::url::Url::parse_with_params(&url_str, queries)) + }; let (status_code, max_age) = if permanent { (status::MovedPermanently, 86400) } else { (status::Found, 0) }; + let url = ctry!(req, Url::from_generic_url(url)); let mut resp = Response::with((status_code, Redirect(url))); resp.headers .set(CacheControl(vec![CacheDirective::MaxAge(max_age)])); @@ -1780,18 +1781,18 @@ mod test { )?; assert_redirect( "/some_random_crate::some::path", - "/some_random_crate/latest/some_random_crate/?search=some::path", + "/some_random_crate/latest/some_random_crate/?search=some%3A%3Apath", web, )?; assert_redirect( "/some_random_crate::some::path?go_to_first=true", - "/some_random_crate/latest/some_random_crate/?search=some::path&go_to_first=true", + "/some_random_crate/latest/some_random_crate/?go_to_first=true&search=some%3A%3Apath", web, )?; assert_redirect( "/std::some::path", - "https://doc.rust-lang.org/stable/std/?search=some::path", + "https://doc.rust-lang.org/stable/std/?search=some%3A%3Apath", web, )?; From de6fc1bec99415fe537faf47ba60e578c9e9fd01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emil=20Gardstr=C3=B6m?= Date: Sat, 24 Sep 2022 13:04:38 +0200 Subject: [PATCH 3/4] simplify query building --- src/web/releases.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/web/releases.rs b/src/web/releases.rs index 13698755a..e8a717078 100644 --- a/src/web/releases.rs +++ b/src/web/releases.rs @@ -545,7 +545,7 @@ impl_webpage! { pub fn search_handler(req: &mut Request) -> IronResult { let url = req.url.as_ref(); - let params: HashMap<_, _> = url.query_pairs().collect(); + let mut params: HashMap<_, _> = url.query_pairs().collect(); let query = params .get("query") .map(|q| q.to_string()) @@ -555,7 +555,7 @@ pub fn search_handler(req: &mut Request) -> IronResult { // check if I am feeling lucky button pressed and redirect user to crate page // if there is a match. Also check for paths to items within crates. - if params.contains_key("i-am-feeling-lucky") || query.contains("::") { + if params.remove("i-am-feeling-lucky").is_some() || query.contains("::") { // redirect to a random crate if query is empty if query.is_empty() { return redirect_to_random_crate(req, &mut conn); @@ -571,17 +571,12 @@ pub fn search_handler(req: &mut Request) -> IronResult { None => query.clone(), }; - queries.extend( - params - .iter() - .filter(|(k, _)| !matches!(k.as_ref(), "i-am-feeling-lucky" | "query")) - .map(|(k, v)| (k.as_ref(), v.as_ref())), - ); - // since we never pass a version into `match_version` here, we'll never get // `MatchVersion::Exact`, so the distinction between `Exact` and `Semver` doesn't // matter if let Ok(matchver) = match_version(&mut conn, &krate, None) { + params.remove("query"); + queries.extend(params.iter().map(|(k, v)| (k.as_ref(), v.as_ref()))); let (version, _) = matchver.version.into_parts(); let krate = matchver.corrected_name.unwrap_or(krate); From 709839103407c820a2d25d0c33aab1d6bb393b28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emil=20Gardstr=C3=B6m?= Date: Sat, 24 Sep 2022 13:23:18 +0200 Subject: [PATCH 4/4] always parse with params --- src/web/mod.rs | 8 ++++---- src/web/releases.rs | 18 +++++++----------- src/web/rustdoc.rs | 41 ++++++++++++++++++++--------------------- 3 files changed, 31 insertions(+), 36 deletions(-) diff --git a/src/web/mod.rs b/src/web/mod.rs index 34683c328..5e6ab9f74 100644 --- a/src/web/mod.rs +++ b/src/web/mod.rs @@ -728,24 +728,24 @@ mod test { wrapper(|env| { let web = env.frontend(); for krate in &["std", "alloc", "core", "proc_macro", "test"] { - let target = format!("https://doc.rust-lang.org/stable/{}/", krate); + let target = format!("https://doc.rust-lang.org/stable/{}/?", krate); // with or without slash assert_redirect(&format!("/{}", krate), &target, web)?; assert_redirect(&format!("/{}/", krate), &target, web)?; } - let target = "https://doc.rust-lang.org/stable/proc_macro/"; + let target = "https://doc.rust-lang.org/stable/proc_macro/?"; // with or without slash assert_redirect("/proc-macro", target, web)?; assert_redirect("/proc-macro/", target, web)?; - let target = "https://doc.rust-lang.org/nightly/nightly-rustc/"; + let target = "https://doc.rust-lang.org/nightly/nightly-rustc/?"; // with or without slash assert_redirect("/rustc", target, web)?; assert_redirect("/rustc/", target, web)?; - let target = "https://doc.rust-lang.org/nightly/nightly-rustc/rustdoc/"; + let target = "https://doc.rust-lang.org/nightly/nightly-rustc/rustdoc/?"; // with or without slash assert_redirect("/rustdoc", target, web)?; assert_redirect("/rustdoc/", target, web)?; diff --git a/src/web/releases.rs b/src/web/releases.rs index e8a717078..2677586e7 100644 --- a/src/web/releases.rs +++ b/src/web/releases.rs @@ -584,17 +584,13 @@ pub fn search_handler(req: &mut Request) -> IronResult { let url = if matchver.rustdoc_status { let target_name = matchver.target_name; let path = format!("{base}/{krate}/{version}/{target_name}/"); - if queries.is_empty() { - ctry!(req, Url::parse(&path)) - } else { - ctry!( + ctry!( + req, + Url::from_generic_url(ctry!( req, - Url::from_generic_url(ctry!( - req, - iron::url::Url::parse_with_params(&path, queries) - )) - ) - } + iron::url::Url::parse_with_params(&path, queries) + )) + ) } else { ctry!(req, Url::parse(&format!("{base}/crate/{krate}/{version}"))) }; @@ -842,7 +838,7 @@ mod tests { assert_redirect( "/releases/search?query=some_random_crate&i-am-feeling-lucky=1", - "/some_random_crate/1.0.0/some_random_crate/", + "/some_random_crate/1.0.0/some_random_crate/?", web, )?; Ok(()) diff --git a/src/web/rustdoc.rs b/src/web/rustdoc.rs index c26a32faa..a0eabfbd0 100644 --- a/src/web/rustdoc.rs +++ b/src/web/rustdoc.rs @@ -53,20 +53,19 @@ pub fn rustdoc_redirector_handler(req: &mut Request) -> IronResult { if let Some(path) = path_in_crate { queries.insert("search".into(), path.into()); } - let url: iron::url::Url = req.url.clone().into(); - let query_pairs = url.query_pairs(); - queries.extend(query_pairs); - let url = if queries.is_empty() { - ctry!(req, iron::url::Url::parse(&url_str)) - } else { - ctry!(req, iron::url::Url::parse_with_params(&url_str, queries)) - }; + queries.extend(req.url.as_ref().query_pairs()); + let url = ctry!( + req, + Url::from_generic_url(ctry!( + req, + iron::url::Url::parse_with_params(&url_str, queries) + )) + ); let (status_code, max_age) = if permanent { (status::MovedPermanently, 86400) } else { (status::Found, 0) }; - let url = ctry!(req, Url::from_generic_url(url)); let mut resp = Response::with((status_code, Redirect(url))); resp.headers .set(CacheControl(vec![CacheDirective::MaxAge(max_age)])); @@ -866,8 +865,8 @@ mod test { "/dummy/0.3.0/all.html", web, )?; - assert_redirect("/dummy/0.3.0/", base, web)?; - assert_redirect("/dummy/0.3.0/index.html", base, web)?; + assert_redirect("/dummy/0.3.0/", &format!("{}?", base), web)?; + assert_redirect("/dummy/0.3.0/index.html", &format!("{}?", base), web)?; Ok(()) }); } @@ -1184,7 +1183,7 @@ mod test { .create()?; let web = env.frontend(); - assert_redirect("/fake%2Dcrate", "/fake-crate/latest/fake_crate/", web)?; + assert_redirect("/fake%2Dcrate", "/fake-crate/latest/fake_crate/?", web)?; Ok(()) }); @@ -1214,37 +1213,37 @@ mod test { let web = env.frontend(); - assert_redirect("/dummy_dash", "/dummy-dash/latest/dummy_dash/", web)?; - assert_redirect("/dummy_dash/*", "/dummy-dash/0.2.0/dummy_dash/", web)?; - assert_redirect("/dummy_dash/0.1.0", "/dummy-dash/0.1.0/dummy_dash/", web)?; + assert_redirect("/dummy_dash", "/dummy-dash/latest/dummy_dash/?", web)?; + assert_redirect("/dummy_dash/*", "/dummy-dash/0.2.0/dummy_dash/?", web)?; + assert_redirect("/dummy_dash/0.1.0", "/dummy-dash/0.1.0/dummy_dash/?", web)?; assert_redirect( "/dummy-underscore", - "/dummy_underscore/latest/dummy_underscore/", + "/dummy_underscore/latest/dummy_underscore/?", web, )?; assert_redirect( "/dummy-underscore/*", - "/dummy_underscore/0.2.0/dummy_underscore/", + "/dummy_underscore/0.2.0/dummy_underscore/?", web, )?; assert_redirect( "/dummy-underscore/0.1.0", - "/dummy_underscore/0.1.0/dummy_underscore/", + "/dummy_underscore/0.1.0/dummy_underscore/?", web, )?; assert_redirect( "/dummy-mixed_separators", - "/dummy_mixed-separators/latest/dummy_mixed_separators/", + "/dummy_mixed-separators/latest/dummy_mixed_separators/?", web, )?; assert_redirect( "/dummy_mixed_separators/*", - "/dummy_mixed-separators/0.2.0/dummy_mixed_separators/", + "/dummy_mixed-separators/0.2.0/dummy_mixed_separators/?", web, )?; assert_redirect( "/dummy-mixed-separators/0.1.0", - "/dummy_mixed-separators/0.1.0/dummy_mixed_separators/", + "/dummy_mixed-separators/0.1.0/dummy_mixed_separators/?", web, )?;