get rid of empty url parameter section in some redirects

Author: syphar

src/web/mod.rs

@@ -107,6 +107,7 @@ use postgres::Client;
 use router::{NoRoute, TrailingSlash};
 use semver::{Version, VersionReq};
 use serde::Serialize;
+use std::borrow::Borrow;
 use std::{borrow::Cow, net::SocketAddr, sync::Arc};
 
 /// Duration of static files for staticfile and DatabaseFileHandler (in seconds)
@@ -507,6 +508,25 @@ fn redirect_base(req: &Request) -> String {
     }
 }
 
+/// Parse and URL into a iron::Url struct.
+/// When `queries` are given these are added to the URL,
+/// with empty `queries` the `?` will be omitted.
+pub(crate) fn parse_url_with_params<I, K, V>(url: &str, queries: I) -> Result<Url, Error>
+where
+    I: IntoIterator,
+    I::Item: Borrow<(K, V)>,
+    K: AsRef<str>,
+    V: AsRef<str>,
+{
+    let mut queries = queries.into_iter().peekable();
+    if queries.peek().is_some() {
+        Url::from_generic_url(iron::url::Url::parse_with_params(url, queries)?)
+            .map_err(|msg| anyhow!(msg))
+    } else {
+        Url::parse(url).map_err(|msg| anyhow!(msg))
+    }
+}
+
 /// MetaData used in header
 #[derive(Debug, Clone, PartialEq, Eq, Serialize)]
 pub(crate) struct MetaData {
@@ -728,24 +748,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)?;

src/web/releases.rs

@@ -5,7 +5,7 @@ use crate::{
     db::{Pool, PoolClient},
     impl_webpage,
     utils::report_error,
-    web::{error::Nope, match_version, page::WebPage, redirect_base},
+    web::{error::Nope, match_version, page::WebPage, parse_url_with_params, redirect_base},
     BuildQueue, Config,
 };
 use anyhow::{anyhow, Result};
@@ -20,7 +20,7 @@ use log::{debug, warn};
 use postgres::Client;
 use router::Router;
 use serde::{Deserialize, Serialize};
-use std::collections::HashMap;
+use std::collections::{BTreeMap, HashMap};
 use std::str;
 use url::form_urlencoded;
 
@@ -584,11 +584,11 @@ pub fn search_handler(req: &mut Request) -> IronResult<Response> {
             return redirect_to_random_crate(req, &mut conn);
         }
 
-        let mut queries = std::collections::BTreeMap::new();
+        let mut queries = BTreeMap::new();
 
         let krate = match query.split_once("::") {
             Some((krate, query)) => {
-                queries.insert("search", query);
+                queries.insert("search".into(), query.into());
                 krate.to_string()
             }
             None => query.clone(),
@@ -599,21 +599,15 @@ pub fn search_handler(req: &mut Request) -> IronResult<Response> {
         // 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())));
+            queries.extend(params);
             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;
                 let path = format!("{base}/{krate}/{version}/{target_name}/");
-                ctry!(
-                    req,
-                    Url::from_generic_url(ctry!(
-                        req,
-                        iron::url::Url::parse_with_params(&path, queries)
-                    ))
-                )
+                ctry!(req, parse_url_with_params(&path, queries))
             } else {
                 ctry!(req, Url::parse(&format!("{base}/crate/{krate}/{version}")))
             };
@@ -861,7 +855,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(())

src/web/rustdoc.rs

@@ -6,7 +6,8 @@ use crate::{
     utils,
     web::{
         crate_details::CrateDetails, csp::Csp, error::Nope, file::File, match_version,
-        metrics::RenderingTimesRecorder, redirect_base, MatchSemver, MetaData,
+        metrics::RenderingTimesRecorder, parse_url_with_params, redirect_base, MatchSemver,
+        MetaData,
     },
     Config, Metrics, Storage,
 };
@@ -22,7 +23,11 @@ use lol_html::errors::RewritingError;
 use once_cell::sync::Lazy;
 use router::Router;
 use serde::Serialize;
-use std::{collections::HashMap, fmt::Write, path::Path};
+use std::{
+    collections::{BTreeMap, HashMap},
+    fmt::Write,
+    path::Path,
+};
 
 static DOC_RUST_LANG_ORG_REDIRECTS: Lazy<HashMap<&str, &str>> = Lazy::new(|| {
     HashMap::from([
@@ -46,21 +51,12 @@ pub fn rustdoc_redirector_handler(req: &mut Request) -> IronResult<Response> {
         permanent: bool,
         path_in_crate: Option<&str>,
     ) -> IronResult<Response> {
-        let mut queries: std::collections::BTreeMap<
-            std::borrow::Cow<'_, str>,
-            std::borrow::Cow<'_, str>,
-        > = std::collections::BTreeMap::new();
+        let mut queries = BTreeMap::new();
         if let Some(path) = path_in_crate {
             queries.insert("search".into(), path.into());
         }
         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 url = ctry!(req, parse_url_with_params(&url_str, queries));
         let (status_code, max_age) = if permanent {
             (status::MovedPermanently, 86400)
         } else {
@@ -865,8 +861,8 @@ mod test {
                 "/dummy/0.3.0/all.html",
                 web,
             )?;
-            assert_redirect("/dummy/0.3.0/", &format!("{}?", base), web)?;
-            assert_redirect("/dummy/0.3.0/index.html", &format!("{}?", base), web)?;
+            assert_redirect("/dummy/0.3.0/", base, web)?;
+            assert_redirect("/dummy/0.3.0/index.html", base, web)?;
             Ok(())
         });
     }
@@ -1183,7 +1179,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(())
         });
@@ -1213,37 +1209,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,
             )?;