support /latest/ and others in build-details handler for consistency

Author: syphar

.sqlx/query-61e8789f7d2ddcfa951dc86c5f455cf4451f8f3b683c6bd1033e8ebaab97692e.json

@@ -11,8 +11,11 @@ sqlx-prepare ADDITIONAL_ARGS="":
 sqlx-check:
   just sqlx-prepare "--check"
 
-lint: 
-  cargo clippy --all-features --all-targets --workspace --locked -- -D warnings
+lint *args: 
+  cargo clippy --all-features --all-targets --workspace --locked {{ args }} -- -D warnings
+
+lint-fix:
+  just lint --fix --allow-dirty --allow-staged
 
 lint-js *args:
   deno run -A npm:eslint@9 static templates gui-tests eslint.config.js {{ args }}

Justfile

@@ -1,16 +1,14 @@
 use crate::{
     AsyncStorage, Config,
-    db::{
-        BuildId,
-        types::{BuildStatus, version::Version},
-    },
+    db::{BuildId, types::BuildStatus},
     impl_axum_webpage,
     web::{
         MetaData,
+        cache::CachePolicy,
         error::{AxumNope, AxumResult},
         extractors::{DbConnection, Path, rustdoc::RustdocParams},
         file::File,
-        filters,
+        filters, match_version,
         page::templates::{RenderBrands, RenderRegular, RenderSolid},
     },
 };
@@ -55,24 +53,37 @@ impl BuildDetailsPage {
 
 #[derive(Clone, Deserialize, Debug)]
 pub(crate) struct BuildDetailsParams {
-    pub(crate) name: String,
-    pub(crate) version: Version,
     pub(crate) id: String,
     pub(crate) filename: Option<String>,
 }
 
 pub(crate) async fn build_details_handler(
-    Path(params): Path<BuildDetailsParams>,
+    params: RustdocParams,
+    Path(build_params): Path<BuildDetailsParams>,
     mut conn: DbConnection,
     Extension(config): Extension<Arc<Config>>,
     Extension(storage): Extension<Arc<AsyncStorage>>,
 ) -> AxumResult<impl IntoResponse> {
-    let id = params
+    let id = build_params
         .id
         .parse()
         .map(BuildId)
         .map_err(|_| AxumNope::BuildNotFound)?;
 
+    let version = match_version(&mut conn, params.name(), params.req_version())
+        .await?
+        .assume_exact_name()?
+        .into_canonical_req_version_or_else(|version| {
+            AxumNope::Redirect(
+                params
+                    .clone()
+                    .with_req_version(version)
+                    .build_details_url(id, build_params.filename.as_deref()),
+                CachePolicy::ForeverInCdn,
+            )
+        })?
+        .into_version();
+
     let row = sqlx::query!(
         r#"SELECT
              builds.rustc_version,
@@ -87,8 +98,8 @@ pub(crate) async fn build_details_handler(
          INNER JOIN crates ON releases.crate_id = crates.id
          WHERE builds.id = $1 AND crates.name = $2 AND releases.version = $3"#,
         id.0,
-        params.name,
-        params.version.to_string(),
+        params.name(),
+        version as _
     )
     .fetch_optional(&mut *conn)
     .await?
@@ -114,7 +125,7 @@ pub(crate) async fn build_details_handler(
             .try_collect()
             .await?;
 
-        let current_filename = if let Some(filename) = params.filename {
+        let current_filename = if let Some(filename) = build_params.filename {
             // if we have a given filename in the URL, we use that one.
             Some(filename)
         } else if let Some(default_target) = row.default_target {
@@ -144,8 +155,14 @@ pub(crate) async fn build_details_handler(
         (file_content, all_log_filenames, current_filename)
     };
 
-    let metadata = MetaData::from_crate(&mut conn, &params.name, &params.version, None).await?;
-    let params = RustdocParams::from_metadata(&metadata);
+    let metadata = MetaData::from_crate(
+        &mut conn,
+        params.name(),
+        &version,
+        Some(params.req_version().clone()),
+    )
+    .await?;
+    let params = params.apply_metadata(&metadata);
 
     Ok(BuildDetailsPage {
         metadata,
@@ -167,9 +184,12 @@ pub(crate) async fn build_details_handler(
 
 #[cfg(test)]
 mod tests {
-    use crate::test::{
-        AxumResponseTestExt, AxumRouterTestExt, FakeBuild, async_wrapper,
-        fake_release_that_failed_before_build,
+    use crate::{
+        db::types::{BuildId, ReleaseId},
+        test::{
+            AxumResponseTestExt, AxumRouterTestExt, FakeBuild, TestEnvironment, V0_1,
+            async_wrapper, fake_release_that_failed_before_build,
+        },
     };
     use kuchikiki::traits::TendrilSink;
     use test_case::test_case;
@@ -187,6 +207,22 @@ mod tests {
             .collect()
     }
 
+    async fn build_ids_for_release(
+        conn: &mut sqlx::PgConnection,
+        release_id: ReleaseId,
+    ) -> Vec<BuildId> {
+        sqlx::query!(
+            "SELECT id FROM builds WHERE rid = $1 ORDER BY id ASC",
+            release_id as _
+        )
+        .fetch_all(conn)
+        .await
+        .unwrap()
+        .into_iter()
+        .map(|row| BuildId(row.id))
+        .collect()
+    }
+
     #[test]
     fn test_partial_build_result() {
         async_wrapper(|env| async move {
@@ -475,4 +511,30 @@ mod tests {
             Ok(())
         });
     }
+
+    #[tokio::test(flavor = "multi_thread")]
+    async fn build_detail_via_latest() -> anyhow::Result<()> {
+        let env = TestEnvironment::new().await?;
+        let rid = env
+            .fake_release()
+            .await
+            .name("foo")
+            .version(V0_1)
+            .create()
+            .await?;
+
+        let mut conn = env.async_db().async_conn().await;
+        let build_id = {
+            let ids = build_ids_for_release(&mut conn, rid).await;
+            assert_eq!(ids.len(), 1);
+            ids[0]
+        };
+
+        env.web_app()
+            .await
+            .assert_success(&format!("/crate/foo/latest/builds/{build_id}"))
+            .await?;
+
+        Ok(())
+    }
 }

src/web/build_details.rs

@@ -20,6 +20,13 @@ use std::borrow::Cow;
 const INDEX_HTML: &str = "index.html";
 const FOLDER_AND_INDEX_HTML: &str = "/index.html";
 
+pub(crate) const ROOT_RUSTDOC_HTML_FILES: &[&str] = &[
+    "all.html",
+    "help.html",
+    "settings.html",
+    "scrape-examples-help.html",
+];
+
 #[derive(Clone, Debug, PartialEq)]
 pub(crate) enum PageKind {
     Rustdoc,
@@ -723,27 +730,38 @@ fn generate_rustdoc_path_for_url(
         && !path.is_empty()
         && path != INDEX_HTML
     {
-        if let Some(target_name) = target_name
-            && path == target_name
-        {
-            // Sometimes people add a path that is just the target name.
-            // This matches our `rustdoc_redirector_handler` route,
-            // without trailing slash (`/{name}/{version}/{target}`).
-            //
-            // Even though the rustdoc html handler would also be capable
-            // handling this input, we want to redirect to the trailing
-            // slash version (`/{name}/{version}/{target}/`),
-            // so they are separate.
-            format!("{}/", target_name)
-        } else {
+        // for none-elements paths we have to guarantee that we have a
+        // trailing slash, otherwise the rustdoc-url won't hit the html-handler and
+        // lead to redirect loops.
+        if path.contains('/') {
             // just use the given inner to start, if:
             // * it's not empty
             // * it's not just "index.html"
+            // * we have a slash in the path.
             path.to_string()
+        } else if ROOT_RUSTDOC_HTML_FILES.iter().any(|&f| f == path) {
+            // special case: some files are at the root of the rustdoc output,
+            // without a trailing slash, and the routes are fine with that.
+            // e.g. `/help.html`, `/settings.html`, `/all.html`, ...
+            path.to_string()
+        } else if let Some(target_name) = target_name {
+            if target_name == path {
+                // when we have the target name as path, without a trailing slash,
+                // just add the slash.
+                format!("{}/", path)
+            } else {
+                // when someone just attaches some path to the URL, like
+                // `/{krate}/{version}/somefile.html`, we assume they meant
+                // `/{krate}/{version}/{target_name}/somefile.html`.
+                format!("{}/{}", target_name, path)
+            }
+        } else {
+            // fallback: just attach a slash and redirect.
+            format!("{}/", path)
         }
     } else if let Some(target_name) = target_name {
         // after having no usable given path, we generate one with the
-        // target name, if we have one.
+        // target name, if we have one/.
         format!("{}/", target_name)
     } else {
         // no usable given path:
@@ -1639,23 +1657,26 @@ mod tests {
         assert_eq!(params.rustdoc_url(), "/krate/latest/krate/");
     }
 
-    #[test_case(Some(PageKind::Rustdoc))]
-    #[test_case(None)]
-    fn test_only_target_name_without_slash(page_kind: Option<PageKind>) {
+    #[test_case("other_path.html", "/krate/latest/krate/other_path.html")]
+    #[test_case("other_path", "/krate/latest/krate/other_path"; "without .html")]
+    #[test_case("other_path.html", "/krate/latest/krate/other_path.html"; "with .html")]
+    #[test_case("settings.html", "/krate/latest/settings.html"; "static routes")]
+    #[test_case(KRATE, "/krate/latest/krate/"; "same as target name, without slash")]
+    fn test_redirect_some_odd_paths_we_saw(inner_path: &str, expected_url: &str) {
         // test for https://github.com/rust-lang/docs.rs/issues/2989
         let params = RustdocParams::new(KRATE)
-            .with_maybe_page_kind(page_kind)
-            .try_with_original_uri("/dummy/latest/dummy")
+            .with_page_kind(PageKind::Rustdoc)
+            .try_with_original_uri(format!("/{KRATE}/latest/{inner_path}"))
             .unwrap()
             .with_req_version(ReqVersion::Latest)
             .with_maybe_doc_target(None::<String>)
-            .with_inner_path(KRATE)
+            .with_inner_path(inner_path)
             .with_default_target(DEFAULT_TARGET)
             .with_target_name(KRATE)
             .with_doc_targets(TARGETS.iter().cloned());
 
         dbg!(&params);
 
-        assert_eq!(params.rustdoc_url(), "/krate/latest/krate/");
+        assert_eq!(params.rustdoc_url(), expected_url);
     }
 }

src/web/extractors/rustdoc.rs

@@ -219,6 +219,7 @@ pub(crate) async fn rustdoc_redirector_handler(
     let params = params.with_page_kind(PageKind::Rustdoc);
 
     fn redirect_to_doc(
+        original_uri: Option<&Uri>,
         url: EscapedURI,
         cache_policy: CachePolicy,
         path_in_crate: Option<&str>,
@@ -229,6 +230,17 @@ pub(crate) async fn rustdoc_redirector_handler(
             url
         };
 
+        if let Some(original_uri) = original_uri
+            && original_uri.path() == original_uri.path()
+        {
+            return Err(anyhow!(
+                "infinite redirect detected, \noriginal_uri = {}, redirect_url = {}",
+                original_uri,
+                url
+            )
+            .into());
+        }
+
         trace!(%url, ?cache_policy, path_in_crate, "redirect to doc");
         Ok(axum_cached_redirect(url, cache_policy)?)
     }
@@ -266,6 +278,7 @@ pub(crate) async fn rustdoc_redirector_handler(
         let target_uri =
             EscapedURI::from_uri(description.href.clone()).append_raw_query(original_query);
         return redirect_to_doc(
+            params.original_uri(),
             target_uri,
             CachePolicy::ForeverInCdnAndStaleInBrowser,
             path_in_crate.as_deref(),
@@ -329,6 +342,7 @@ pub(crate) async fn rustdoc_redirector_handler(
 
     if matched_release.rustdoc_status() {
         Ok(redirect_to_doc(
+            params.original_uri(),
             params.rustdoc_url().append_raw_query(original_query),
             if matched_release.is_latest_url() {
                 CachePolicy::ForeverInCdn
@@ -3345,7 +3359,9 @@ mod test {
     #[tokio::test(flavor = "multi_thread")]
     #[test_case("/dummy/"; "only krate")]
     #[test_case("/dummy/latest/"; "with version")]
-    #[test_case("/dummy/latest/dummy"; "full without trailing slash")]
+    #[test_case("/dummy/latest/dummy"; "target-name as path, without trailing slash")]
+    #[test_case("/dummy/latest/other_path"; "other path, without trailing slash")]
+    #[test_case("/dummy/latest/other_path.html"; "other html path, without trailing slash")]
     #[test_case("/dummy/latest/dummy/"; "final target")]
     async fn test_full_latest_url_without_trailing_slash(path: &str) -> Result<()> {
         // test for https://github.com/rust-lang/docs.rs/issues/2989