diff --git a/crates/bin/docs_rs_web/src/error.rs b/crates/bin/docs_rs_web/src/error.rs index 080bb3e5e..bf67bf714 100644 --- a/crates/bin/docs_rs_web/src/error.rs +++ b/crates/bin/docs_rs_web/src/error.rs @@ -17,6 +17,15 @@ use docs_rs_uri::EscapedURI; use std::borrow::Cow; use tracing::error; +/// A single navigation link offered on an error page to help the user recover. +#[derive(Debug, Clone, PartialEq)] +pub(crate) struct RecoveryLink { + /// The user-visible link text. + pub label: Cow<'static, str>, + /// The link target. + pub href: EscapedURI, +} + #[derive(Template)] #[template(path = "error.html")] #[derive(Debug, Clone, PartialEq)] @@ -26,6 +35,8 @@ pub(crate) struct AxumErrorPage { /// The error message, displayed as a description pub message: Cow<'static, str>, pub status: StatusCode, + /// Optional navigation links to help the user recover. Empty for most errors. + pub recovery: Vec, } impl_axum_webpage! { @@ -38,6 +49,20 @@ impl_axum_webpage! { pub enum AxumNope { #[error("Requested resource not found")] ResourceNotFound, + /// A specific doc page/file was not found, but its crate and version *do* + /// exist and have docs. Renders a 404 that acknowledges the crate/version + /// and offers recovery links (issue #2568). + #[error("Requested resource not found in an existing crate version")] + ResourceNotFoundInVersion { + name: String, + version: String, + /// Whether the request used the `/latest/` alias (vs. a pinned version). + is_latest_url: bool, + /// Root of the docs for the requested version. + version_root_url: EscapedURI, + /// The crate details page for the requested version. + crate_details_url: EscapedURI, + }, #[error("Requested build not found")] BuildNotFound, #[error("Requested crate not found")] @@ -76,6 +101,32 @@ impl AxumNope { status: StatusCode::NOT_FOUND, } } + AxumNope::ResourceNotFoundInVersion { + name, + version, + is_latest_url, + .. + } => { + // The crate & version exist and have docs, but this specific + // page within them does not (e.g. a module removed/renamed in a + // new release reached via `/latest/`). Acknowledge the version + // and offer recovery links instead of a bare 404 (issue #2568). + // The recovery links themselves are built in `recovery_links`. + let existing = if is_latest_url { + format!("The latest version of `{name}` ({version})") + } else { + format!("Version {version} of `{name}`") + }; + ErrorInfo { + title: "This page does not exist", + message: format!( + "{existing} exists, but this page inside it could not be found. \ + It may have been moved or removed." + ) + .into(), + status: StatusCode::NOT_FOUND, + } + } AxumNope::BuildNotFound => ErrorInfo { title: "The requested build does not exist", message: "no such build".into(), @@ -137,6 +188,29 @@ impl AxumNope { AxumNope::Redirect(_target, _cache_policy) => unreachable!(), } } + + /// Navigation links offered on the error page to help the user recover. + /// Empty for every error except the contextual missing-page 404 (#2568). + fn recovery_links(&self) -> Vec { + match self { + AxumNope::ResourceNotFoundInVersion { + name, + version_root_url, + crate_details_url, + .. + } => vec![ + RecoveryLink { + label: "Documentation home for this version".into(), + href: version_root_url.clone(), + }, + RecoveryLink { + label: format!("All versions of {name}").into(), + href: crate_details_url.clone(), + }, + ], + _ => Vec::new(), + } + } } struct ErrorInfo { @@ -169,6 +243,7 @@ impl IntoResponse for AxumNope { } AxumNope::Redirect(target, cache_policy) => redirect_with_policy(target, cache_policy), _ => { + let recovery = self.recovery_links(); let ErrorInfo { title, message, @@ -178,6 +253,7 @@ impl IntoResponse for AxumNope { title, message, status, + recovery, } .into_response() } @@ -192,25 +268,38 @@ impl IntoResponse for JsonAxumNope { fn into_response(self) -> AxumResponse { match self.0 { AxumNope::NoResults => { - // user did a search with no search terms; invalid, - // return 404 + // User searched without providing search terms. StatusCode::NOT_FOUND.into_response() } AxumNope::Redirect(target, cache_policy) => redirect_with_policy(target, cache_policy), _ => { + let recovery = self.0.recovery_links(); let ErrorInfo { title, message, status, } = self.0.into_error_info(); - ( - status, - Json(serde_json::json!({ - "title": title, - "message": message, - })), - ) - .into_response() + + let mut body = serde_json::json!({ + "title": title, + "message": message, + }); + + if !recovery.is_empty() { + let links: Vec<_> = recovery + .iter() + .map(|link| { + serde_json::json!({ + "label": link.label, + "href": link.href, + }) + }) + .collect(); + + body["links"] = serde_json::json!(links); + } + + (status, Json(body)).into_response() } } } @@ -245,7 +334,7 @@ pub(crate) type JsonAxumResult = Result; #[cfg(test)] mod tests { - use super::{AxumNope, EscapedURI, IntoResponse}; + use super::{AxumNope, EscapedURI, IntoResponse, JsonAxumNope}; use crate::cache::CachePolicy; use crate::testing::{ AxumResponseTestExt, AxumRouterTestExt, TestEnvironmentExt as _, async_wrapper, @@ -387,4 +476,132 @@ mod tests { Ok(()) }); } + + /// Helper: hrefs of the recovery links rendered on an error page. + fn recovery_hrefs(html: &str) -> Vec { + kuchikiki::parse_html() + .one(html) + .select("#recovery-links a") + .unwrap() + .map(|a| { + a.attributes + .borrow() + .get("href") + .unwrap_or_default() + .to_string() + }) + .collect() + } + + #[test] + fn check_404_missing_child_latest_offers_recovery_links() { + async_wrapper(|env| async move { + env.fake_release() + .await + .name("dummy") + .version("0.1.0") + .rustdoc_file("dummy/index.html") + .create() + .await?; + + let response = env + .web_app() + .await + .get("/dummy/latest/dummy/removed_module/index.html") + .await?; + assert_eq!(response.status(), 404); + + let body = response.text().await?; + let page = kuchikiki::parse_html().one(body.as_str()); + assert_eq!( + page.select("#crate-title") + .unwrap() + .next() + .unwrap() + .text_contents(), + "This page does not exist", + ); + + // Two recovery links, preserving the `/latest/` alias. + let hrefs = recovery_hrefs(&body); + assert_eq!(hrefs.len(), 2); + assert!(hrefs.iter().any(|h| h == "/dummy/latest/dummy/")); + assert!(hrefs.iter().any(|h| h == "/crate/dummy/latest")); + + Ok(()) + }); + } + + #[test] + fn check_404_missing_child_pinned_keeps_pinned_version() { + async_wrapper(|env| async move { + env.fake_release() + .await + .name("dummy") + .version("0.1.0") + .rustdoc_file("dummy/index.html") + .create() + .await?; + + let response = env + .web_app() + .await + .get("/dummy/0.1.0/dummy/removed_module/index.html") + .await?; + assert_eq!(response.status(), 404); + + let body = response.text().await?; + let hrefs = recovery_hrefs(&body); + assert_eq!(hrefs.len(), 2); + assert!(hrefs.iter().any(|h| h == "/dummy/0.1.0/dummy/")); + assert!(hrefs.iter().any(|h| h == "/crate/dummy/0.1.0")); + // Pinned requests must not be rewritten to `latest`. + assert!(hrefs.iter().all(|h| !h.contains("latest"))); + + Ok(()) + }); + } + + #[test] + fn check_404_generic_resource_has_no_recovery_links() { + async_wrapper(|env| async move { + // A resource outside any existing crate keeps the plain 404 with no + // recovery links (regression: other error types are unaffected). + let body = env + .web_app() + .await + .get("/resource-which-doesnt-exist.js") + .await? + .text() + .await?; + let page = kuchikiki::parse_html().one(body.as_str()); + assert_eq!(page.select("#recovery-links").unwrap().count(), 0); + + Ok(()) + }); + } + + #[test] + fn json_error_body_includes_recovery_links() { + async_wrapper(|_env| async move { + let response = JsonAxumNope(AxumNope::ResourceNotFoundInVersion { + name: "dummy".into(), + version: "0.1.0".into(), + is_latest_url: true, + version_root_url: EscapedURI::from_path("/dummy/latest/dummy/"), + crate_details_url: EscapedURI::from_path("/crate/dummy/latest"), + }) + .into_response(); + + assert_eq!(response.status(), 404); + + let body: serde_json::Value = response.json().await?; + let links = body["links"].as_array().unwrap(); + assert_eq!(links.len(), 2); + assert_eq!(links[0]["href"], "/dummy/latest/dummy/"); + assert_eq!(links[1]["href"], "/crate/dummy/latest"); + + Ok(()) + }); + } } diff --git a/crates/bin/docs_rs_web/src/handlers/about.rs b/crates/bin/docs_rs_web/src/handlers/about.rs index 928d2de60..5c90e62ea 100644 --- a/crates/bin/docs_rs_web/src/handlers/about.rs +++ b/crates/bin/docs_rs_web/src/handlers/about.rs @@ -82,6 +82,7 @@ pub(crate) async fn about_handler(subpage: Option>) -> AxumResult +

{{ title }}

{{ message }}
+ {%- if !recovery.is_empty() -%} + + {%- endif -%} {%- endblock header -%} {%- block topbar -%} diff --git a/crates/bin/docs_rs_web/templates/style/style.scss b/crates/bin/docs_rs_web/templates/style/style.scss index 036de8b82..531946e21 100644 --- a/crates/bin/docs_rs_web/templates/style/style.scss +++ b/crates/bin/docs_rs_web/templates/style/style.scss @@ -704,6 +704,12 @@ div.docsrs-package-container { border-bottom: 1px solid var(--color-border); margin-bottom: 20px; + // On the error page the body text below is centered against the full page + // width, so the title bar has to be centered too or the two disagree. + &.error-header .container { + margin: 0 auto; + } + .container { display: flex; align-items: center; @@ -884,6 +890,18 @@ div.search-page-search-form { display: inline-block; } +#recovery-links { + display: flex; + flex-wrap: wrap; + justify-content: center; + gap: 10px; + padding: 10px 14px 14px; + + .pure-button { + text-decoration: none; + } +} + #clipboard { cursor: pointer; } diff --git a/gui-tests/404.goml b/gui-tests/404.goml index 546e09a80..4a941e364 100644 --- a/gui-tests/404.goml +++ b/gui-tests/404.goml @@ -1,3 +1,13 @@ // Checks the content of the 404 page. go-to: |DOC_PATH| + "/non-existing-crate" assert-text: ("#crate-title", "The requested crate does not exist") + +// Checks the 404 page of a page missing inside an existing crate version. +go-to: |DOC_PATH| + "/sysinfo/latest/sysinfo/removed_module/index.html" +assert-text: ("#crate-title", "This page does not exist") +assert-text: (".description", "The latest version of `sysinfo` (0.23.5) exists", CONTAINS) +assert-count: ("#recovery-links a", 2) +assert-text: ("#recovery-links a:nth-of-type(1)", "Documentation home for this version") +assert-attribute: ("#recovery-links a:nth-of-type(1)", {"href": "/sysinfo/latest/sysinfo/"}) +assert-text: ("#recovery-links a:nth-of-type(2)", "All versions of sysinfo") +assert-attribute: ("#recovery-links a:nth-of-type(2)", {"href": "/crate/sysinfo/latest"})