code review

Author: Emilgardis

src/web/releases.rs

@@ -584,24 +584,22 @@ pub fn search_handler(req: &mut Request) -> IronResult<Response> {
             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
@@ -613,10 +611,18 @@ pub fn search_handler(req: &mut Request) -> IronResult<Response> {
             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}")))
             };
@@ -914,7 +920,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(())
@@ -929,7 +935,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(())

src/web/rustdoc.rs

@@ -42,30 +42,31 @@ static DOC_RUST_LANG_ORG_REDIRECTS: Lazy<HashMap<&str, &str>> = Lazy::new(|| {
 pub fn rustdoc_redirector_handler(req: &mut Request) -> IronResult<Response> {
     fn redirect_to_doc(
         req: &Request,
-        mut url_str: String,
+        url_str: String,
         permanent: bool,
         path_in_crate: Option<&str>,
     ) -> IronResult<Response> {
-        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,
             )?;