From dcfd5d9d6d5954fd5a3f054dbf4b41a9666a1d89 Mon Sep 17 00:00:00 2001 From: Denis Cornehl Date: Wed, 15 Apr 2026 17:31:41 +0200 Subject: [PATCH] unify find-latest-version --- .../docs_rs_web/src/handlers/crate_details.rs | 216 ++---------------- .../bin/docs_rs_web/src/handlers/releases.rs | 9 +- .../bin/docs_rs_web/src/handlers/rustdoc.rs | 52 ++++- crates/bin/docs_rs_web/src/match_release.rs | 62 ++--- .../lib/docs_rs_database/src/crate_details.rs | 150 +++++++++++- gui-tests/yanked.goml | 3 - 6 files changed, 245 insertions(+), 247 deletions(-) delete mode 100644 gui-tests/yanked.goml diff --git a/crates/bin/docs_rs_web/src/handlers/crate_details.rs b/crates/bin/docs_rs_web/src/handlers/crate_details.rs index 060642dbb..4659000b4 100644 --- a/crates/bin/docs_rs_web/src/handlers/crate_details.rs +++ b/crates/bin/docs_rs_web/src/handlers/crate_details.rs @@ -11,7 +11,7 @@ use crate::{ page::templates::{RenderBrands, RenderRegular, RenderSolid, filters}, utils::{get_correct_docsrs_style_file, licenses}, }; -use anyhow::{Context, Result, anyhow}; +use anyhow::{Context, Result}; use askama::Template; use axum::{ extract::Extension, @@ -19,7 +19,7 @@ use axum::{ }; use chrono::{DateTime, Utc}; use docs_rs_cargo_metadata::{Dependency, ReleaseDependencyList}; -use docs_rs_database::crate_details::{Release, latest_release, parse_doc_targets}; +use docs_rs_database::crate_details::{Release, parse_doc_targets}; use docs_rs_headers::CanonicalUrl; use docs_rs_registry_api::OwnerKind; use docs_rs_storage::{AsyncStorage, PathNotFoundError}; @@ -338,9 +338,9 @@ impl CrateDetails { } /// Returns the latest non-yanked, non-prerelease release of this crate (or latest - /// yanked/prereleased if that is all that exist). - pub fn latest_release(&self) -> Result<&Release> { - latest_release(&self.releases).ok_or_else(|| anyhow!("crate without releases")) + /// prereleased if that is all that exist). Returns `None` if every release is yanked. + pub fn latest_release(&self) -> Option<&Release> { + docs_rs_database::crate_details::latest_release(&self.releases) } } @@ -660,10 +660,13 @@ pub(crate) async fn get_all_platforms_inner( .into_response()); } - let latest_release = latest_release(&matched_release.all_releases) - .expect("we couldn't end up here without releases"); + let latest_release = + docs_rs_database::crate_details::latest_release(&matched_release.all_releases); - let current_target = if latest_release.build_status.is_success() { + let current_target = if latest_release + .map(|r| r.build_status.is_success()) + .unwrap_or(false) + { params .doc_target_or_default() .unwrap_or_default() @@ -1174,192 +1177,6 @@ mod tests { }) } - #[test] - fn test_latest_version() { - async_wrapper(|env| async move { - let db = env.pool()?; - - env.fake_release() - .await - .name("foo") - .version("0.0.1") - .create() - .await?; - env.fake_release() - .await - .name("foo") - .version("0.0.3") - .create() - .await?; - env.fake_release() - .await - .name("foo") - .version("0.0.2") - .create() - .await?; - - let mut conn = db.get_async().await?; - for version in &["0.0.1", "0.0.2", "0.0.3"] { - let details = crate_details(&mut conn, "foo", *version, None).await; - assert_eq!( - details.latest_release().unwrap().version, - Version::parse("0.0.3")? - ); - } - - Ok(()) - }) - } - - #[test] - fn test_latest_version_ignores_prerelease() { - async_wrapper(|env| async move { - let db = env.pool()?; - - env.fake_release() - .await - .name("foo") - .version("0.0.1") - .create() - .await?; - env.fake_release() - .await - .name("foo") - .version("0.0.3-pre.1") - .create() - .await?; - env.fake_release() - .await - .name("foo") - .version("0.0.2") - .create() - .await?; - - let mut conn = db.get_async().await?; - for &version in &["0.0.1", "0.0.2", "0.0.3-pre.1"] { - let details = crate_details(&mut conn, "foo", version, None).await; - assert_eq!( - details.latest_release().unwrap().version, - Version::parse("0.0.2")? - ); - } - - Ok(()) - }) - } - - #[test] - fn test_latest_version_ignores_yanked() { - async_wrapper(|env| async move { - let db = env.pool()?; - - env.fake_release() - .await - .name("foo") - .version("0.0.1") - .create() - .await?; - env.fake_release() - .await - .name("foo") - .version("0.0.3") - .yanked(true) - .create() - .await?; - env.fake_release() - .await - .name("foo") - .version("0.0.2") - .create() - .await?; - - let mut conn = db.get_async().await?; - for &version in &["0.0.1", "0.0.2", "0.0.3"] { - let details = crate_details(&mut conn, "foo", version, None).await; - assert_eq!( - details.latest_release().unwrap().version, - Version::parse("0.0.2")? - ); - } - - Ok(()) - }) - } - - #[test] - fn test_latest_version_only_yanked() { - async_wrapper(|env| async move { - let db = env.pool()?; - - env.fake_release() - .await - .name("foo") - .version("0.0.1") - .yanked(true) - .create() - .await?; - env.fake_release() - .await - .name("foo") - .version("0.0.3") - .yanked(true) - .create() - .await?; - env.fake_release() - .await - .name("foo") - .version("0.0.2") - .yanked(true) - .create() - .await?; - - let mut conn = db.get_async().await?; - for &version in &["0.0.1", "0.0.2", "0.0.3"] { - let details = crate_details(&mut conn, "foo", version, None).await; - assert_eq!( - details.latest_release().unwrap().version, - Version::parse("0.0.3")? - ); - } - - Ok(()) - }) - } - - #[test] - fn test_latest_version_in_progress() { - async_wrapper(|env| async move { - let db = env.pool()?; - - env.fake_release() - .await - .name("foo") - .version("0.0.1") - .create() - .await?; - env.fake_release() - .await - .name("foo") - .version("0.0.2") - .builds(vec![ - FakeBuild::default().build_status(BuildStatus::InProgress), - ]) - .create() - .await?; - - let mut conn = db.get_async().await?; - for &version in &["0.0.1", "0.0.2"] { - let details = crate_details(&mut conn, "foo", version, None).await; - assert_eq!( - details.latest_release().unwrap().version, - Version::parse("0.0.1")? - ); - } - - Ok(()) - }) - } - #[test] fn releases_dropdowns_show_binary_warning() { async_wrapper(|env| async move { @@ -1411,11 +1228,18 @@ mod tests { .create() .await?; - let response = env.web_app().await.get("/crate/foo/latest").await?; + let response = env + .web_app() + .await + .assert_success("/crate/foo/0.1.0") + .await?; let page = kuchikiki::parse_html().one(response.text().await?); + // multiple `a.pure-menu-link` elements share this href (the topbar's + // `crate-name` link and the navigation "Crate" tab); the version dropdown's + // entry is the only one rendered with `rel="nofollow"` by the macro. let link = page - .select_first("a.pure-menu-link[href='/crate/foo/0.1.0']") + .select_first("a.pure-menu-link[href='/crate/foo/0.1.0'][rel='nofollow']") .unwrap(); assert_eq!( diff --git a/crates/bin/docs_rs_web/src/handlers/releases.rs b/crates/bin/docs_rs_web/src/handlers/releases.rs index 826b07907..53d6860ce 100644 --- a/crates/bin/docs_rs_web/src/handlers/releases.rs +++ b/crates/bin/docs_rs_web/src/handlers/releases.rs @@ -1490,16 +1490,17 @@ mod tests { let links = get_release_links("/releases/search?query=some_random_crate", &web).await?; - // `some_other_crate` won't be shown since we don't have it yet - assert_eq!(links.len(), 4); + // `some_other_crate` won't be shown since we don't have it yet. + // `yet_another_crate` only has a yanked release, so it has no canonical "latest" + // and renders as "Documentation not available on docs.rs" (no anchor). + assert_eq!(links.len(), 3); // * `max_version` from the crates.io search result will be ignored since we // might not have it yet, or the doc-build might be in progress. // * ranking/order from crates.io result is preserved // * version used is the highest semver following our own "latest version" logic assert_eq!(links[0], "/some_random_crate/latest/some_random_crate/"); assert_eq!(links[1], "/and_another_one/latest/and_another_one/"); - assert_eq!(links[2], "/yet_another_crate/0.1.0/yet_another_crate/"); - assert_eq!(links[3], "/crate/failed_hard/0.1.0"); + assert_eq!(links[2], "/crate/failed_hard/0.1.0"); Ok(()) } diff --git a/crates/bin/docs_rs_web/src/handlers/rustdoc.rs b/crates/bin/docs_rs_web/src/handlers/rustdoc.rs index d5262f055..34a221fe8 100644 --- a/crates/bin/docs_rs_web/src/handlers/rustdoc.rs +++ b/crates/bin/docs_rs_web/src/handlers/rustdoc.rs @@ -730,10 +730,12 @@ pub(crate) async fn rustdoc_html_server_handler( ); } - let latest_release = krate.latest_release()?; + let latest_release = krate.latest_release(); // Get the latest version of the crate - let latest_version = latest_release.version.clone(); + let latest_version = latest_release + .map(|release| release.version.clone()) + .unwrap_or_else(|| krate.version.clone()); let is_latest_version = latest_version == krate.version; let is_prerelease = !(krate.version.pre.is_empty()); @@ -744,7 +746,7 @@ pub(crate) async fn rustdoc_html_server_handler( .rustdoc_url() .append_raw_query(original_query.as_deref()); - let latest_path = if latest_release.build_status.is_success() { + let latest_path = if latest_release.is_some_and(|release| release.build_status.is_success()) { params .clone() .with_req_version(&ReqVersion::Latest) @@ -1623,6 +1625,50 @@ mod test { }) } + #[test_case(true)] + #[test_case(false)] + fn no_latest_stable_button_when_latest_stable_is_yanked(archive_storage: bool) { + async fn has_latest_redirect_button( + path: &str, + web: &axum::Router, + ) -> Result { + web.assert_success(path).await?; + let data = web.get(path).await?.text().await?; + Ok(kuchikiki::parse_html() + .one(data) + .select("form > ul > li > a.warn") + .expect("invalid selector") + .next() + .is_some()) + } + + async_wrapper(|env| async move { + env.fake_release() + .await + .name("dummy") + .version("0.2.0-pre.1") + .archive_storage(archive_storage) + .rustdoc_file("dummy/index.html") + .create() + .await?; + env.fake_release() + .await + .name("dummy") + .version("0.2.0") + .archive_storage(archive_storage) + .rustdoc_file("dummy/index.html") + .yanked(true) + .create() + .await?; + + let web = env.web_app().await; + assert!(!has_latest_redirect_button("/dummy/latest/dummy/", &web).await?); + assert!(!has_latest_redirect_button("/dummy/0.2.0-pre.1/dummy/", &web).await?); + + Ok(()) + }) + } + #[test_case(true)] #[test_case(false)] fn yanked_release_shows_warning_in_nav(archive_storage: bool) { diff --git a/crates/bin/docs_rs_web/src/match_release.rs b/crates/bin/docs_rs_web/src/match_release.rs index a28e8445f..60d8ac23b 100644 --- a/crates/bin/docs_rs_web/src/match_release.rs +++ b/crates/bin/docs_rs_web/src/match_release.rs @@ -118,22 +118,37 @@ fn semver_match<'a, F: Fn(&Release) -> bool>( req: &VersionReq, filter: F, ) -> Option<&'a Release> { - // first try standard semver match using `VersionReq::match`, should handle most cases. - if let Some(release) = releases + releases .iter() .filter(|release| filter(release)) .find(|release| req.matches(&release.version)) - { - Some(release) - } else if req == &VersionReq::STAR { - // semver `*` does not match pre-releases. - // So when we only have pre-releases, `VersionReq::STAR` would lead to an - // empty result. - // In this case we just return the latest prerelease instead of nothing. - releases.iter().find(|release| filter(release)) - } else { - None +} + +fn not_yanked(release: &Release) -> bool { + release.yanked.is_none() || release.yanked == Some(false) +} + +fn best_release_for_req<'a>( + releases: &'a [Release], + req_semver: &VersionReq, +) -> Option<&'a Release> { + // `VersionReq::STAR` ("*", "latest", "newest") is the "give me the latest" query; + // delegate to the canonical picker so `/latest/`, `crates.latest_version_id`, and the + // rustdoc topbar all agree on what "latest" means. Note: `latest_release` deliberately + // excludes in-progress builds — there's no canonical latest until the build finishes — + // so STAR returns `None` (and `/latest/` 404s) for a crate whose only non-yanked + // releases are still building. The non-STAR branch below keeps an in-progress fallback + // because there the user asked for a specific range and serving the matching + // in-progress release is more useful than 404. + if req_semver == &VersionReq::STAR { + return docs_rs_database::crate_details::latest_release(releases); } + // For other semver requirements: prefer non-yanked + non-in-progress, fall back to + // allowing in-progress builds if nothing else matches. + semver_match(releases, req_semver, |release: &Release| { + release.build_status != BuildStatus::InProgress && not_yanked(release) + }) + .or_else(|| semver_match(releases, req_semver, not_yanked)) } /// Checks the database for crate releases that match the given name and version. @@ -210,25 +225,10 @@ pub(crate) async fn match_version( ReqVersion::Semver(version_req) => version_req.clone(), }; - // when matching semver requirements, - // we generally only want to look at non-yanked releases, - // excluding releases which just contain in-progress builds - if let Some(release) = semver_match(&releases, &req_semver, |r: &Release| { - r.build_status != BuildStatus::InProgress && (r.yanked.is_none() || r.yanked == Some(false)) - }) { - return Ok(MatchedRelease { - name: name.to_owned(), - corrected_name, - req_version: input_version.clone(), - release: release.clone(), - all_releases: releases, - }); - } - - // when we don't find any match with "normal" releases, we also look into in-progress releases - if let Some(release) = semver_match(&releases, &req_semver, |r: &Release| { - r.yanked.is_none() || r.yanked == Some(false) - }) { + // when matching semver requirements, we generally only want to look at non-yanked + // releases, excluding releases which just contain in-progress builds. If there are no + // matching non-in-progress releases, we also look into in-progress releases. + if let Some(release) = best_release_for_req(&releases, &req_semver) { return Ok(MatchedRelease { name: name.to_owned(), corrected_name, diff --git a/crates/lib/docs_rs_database/src/crate_details.rs b/crates/lib/docs_rs_database/src/crate_details.rs index e591ddf7d..e5c83f300 100644 --- a/crates/lib/docs_rs_database/src/crate_details.rs +++ b/crates/lib/docs_rs_database/src/crate_details.rs @@ -82,18 +82,23 @@ pub async fn releases_for_crate( Ok(releases) } +/// Pick the "latest" release worth pointing users at: non-yanked, build is no longer +/// in-progress, preferring stable over prerelease. Returns `None` when no release meets +/// this bar — either every release is yanked, or the only non-yanked releases are still +/// building. Callers should treat `None` as "no canonical latest" (e.g. `/latest/` 404s, +/// `latest_version_id` is NULL, topbar hides the "Go to latest" button). pub fn latest_release(releases: &[Release]) -> Option<&Release> { - if let Some(release) = releases.iter().find(|release| { - release.version.pre.is_empty() - && release.yanked == Some(false) - && release.build_status != BuildStatus::InProgress - }) { - Some(release) - } else { - releases - .iter() - .find(|release| release.build_status != BuildStatus::InProgress) + fn eligible(release: &Release) -> bool { + let not_yanked = release.yanked.is_none() || release.yanked == Some(false); + not_yanked && release.build_status != BuildStatus::InProgress } + + // releases are sorted by version desc, so `find` returns the highest match. + // Prefer non-prerelease, then fall back to prerelease if nothing else qualifies. + releases + .iter() + .find(|r| eligible(r) && r.version.pre.is_empty()) + .or_else(|| releases.iter().find(|r| eligible(r))) } pub async fn update_latest_version_id( @@ -114,3 +119,128 @@ pub async fn update_latest_version_id( Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + fn release(version: &str, yanked: bool, build_status: BuildStatus) -> Release { + Release { + id: ReleaseId(0), + version: Version::parse(version).unwrap(), + build_status, + yanked: Some(yanked), + is_library: Some(true), + rustdoc_status: Some(true), + target_name: None, + default_target: None, + doc_targets: None, + release_time: None, + } + } + + /// Helper mirroring how `releases_for_crate` returns the slice: sorted by version desc. + fn sorted(mut releases: Vec) -> Vec { + releases.sort_by(|a, b| b.version.cmp(&a.version)); + releases + } + + #[test] + fn picks_highest_when_all_eligible() { + let releases = sorted(vec![ + release("0.0.1", false, BuildStatus::Success), + release("0.0.3", false, BuildStatus::Success), + release("0.0.2", false, BuildStatus::Success), + ]); + assert_eq!( + latest_release(&releases).unwrap().version, + Version::parse("0.0.3").unwrap() + ); + } + + #[test] + fn prefers_stable_over_prerelease() { + let releases = sorted(vec![ + release("0.0.1", false, BuildStatus::Success), + release("0.0.3-pre.1", false, BuildStatus::Success), + release("0.0.2", false, BuildStatus::Success), + ]); + assert_eq!( + latest_release(&releases).unwrap().version, + Version::parse("0.0.2").unwrap() + ); + } + + #[test] + fn picks_highest_prerelease_when_no_stable_exists() { + let releases = sorted(vec![ + release("0.0.3-pre.1", false, BuildStatus::Success), + release("0.0.2-pre.1", false, BuildStatus::Success), + ]); + assert_eq!( + latest_release(&releases).unwrap().version, + Version::parse("0.0.3-pre.1").unwrap() + ); + } + + #[test] + fn skips_yanked_releases() { + let releases = sorted(vec![ + release("0.0.1", false, BuildStatus::Success), + release("0.0.3", true, BuildStatus::Success), + release("0.0.2", false, BuildStatus::Success), + ]); + assert_eq!( + latest_release(&releases).unwrap().version, + Version::parse("0.0.2").unwrap() + ); + } + + #[test] + fn returns_none_when_all_releases_are_yanked() { + let releases = sorted(vec![ + release("0.0.1", true, BuildStatus::Success), + release("0.0.3", true, BuildStatus::Success), + release("0.0.2", true, BuildStatus::Success), + ]); + assert!(latest_release(&releases).is_none()); + } + + #[test] + fn prefers_non_in_progress() { + let releases = sorted(vec![ + release("0.0.1", false, BuildStatus::Success), + release("0.0.2", false, BuildStatus::InProgress), + ]); + assert_eq!( + latest_release(&releases).unwrap().version, + Version::parse("0.0.1").unwrap() + ); + } + + #[test] + fn returns_none_when_only_in_progress_exists() { + // An in-progress build isn't a canonical "latest" — no docs to show yet. + let releases = sorted(vec![release("0.0.1", false, BuildStatus::InProgress)]); + assert!(latest_release(&releases).is_none()); + } + + #[test] + fn yanked_latest_with_unyanked_prerelease_picks_prerelease() { + // Regression for the topbar bug: latest stable is yanked, a non-yanked prerelease + // exists. We must not return the yanked stable. + let releases = sorted(vec![ + release("0.2.0", true, BuildStatus::Success), + release("0.2.0-pre.1", false, BuildStatus::Success), + ]); + assert_eq!( + latest_release(&releases).unwrap().version, + Version::parse("0.2.0-pre.1").unwrap() + ); + } + + #[test] + fn empty_releases_returns_none() { + assert!(latest_release(&[]).is_none()); + } +} diff --git a/gui-tests/yanked.goml b/gui-tests/yanked.goml deleted file mode 100644 index 1a8cc3107..000000000 --- a/gui-tests/yanked.goml +++ /dev/null @@ -1,3 +0,0 @@ -go-to: |DOC_PATH| + "/releases/search?query=libtest" - -assert-text: ("//*[contains(@class, 'release')]//*[contains(@class, 'name')][contains(text(), 'libtest-0.0.1')]", "Yanked", CONTAINS)