Skip to content

Commit

Permalink
Fix crate pages handling target-redirect
Browse files Browse the repository at this point in the history
  • Loading branch information
GuillaumeGomez committed Sep 7, 2023
1 parent 0d52ec8 commit 42321b6
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 16 deletions.
86 changes: 73 additions & 13 deletions src/web/crate_details.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ use crate::{
use anyhow::{Context, Result};
use axum::{
extract::{Extension, Path},
http::Uri,
response::{IntoResponse, Response as AxumResponse},
};
use chrono::{DateTime, Utc};
Expand Down Expand Up @@ -500,12 +499,11 @@ impl_axum_webpage! {
}

#[tracing::instrument]
pub(crate) async fn get_all_platforms(
pub(crate) async fn get_all_platforms_inner(
Path(params): Path<RustdocHtmlParams>,
Extension(pool): Extension<Pool>,
uri: Uri,
is_crate_root: bool,
) -> AxumResult<AxumResponse> {
let is_crate_root = uri.path().starts_with("/-/menus/platforms/crate/");
let req_path: String = params.path.unwrap_or_default();
let req_path: Vec<&str> = req_path.split('/').collect();

Expand Down Expand Up @@ -599,16 +597,23 @@ pub(crate) async fn get_all_platforms(
.unwrap_or(&releases[0]);

// The path within this crate version's rustdoc output
let inner;
let (target, inner_path) = {
let mut inner_path = req_path.clone();

let target = if inner_path.len() > 1 && doc_targets.iter().any(|s| s == inner_path[0]) {
inner_path.remove(0)
let target = if inner_path.len() > 1
&& doc_targets
.iter()
.any(|s| Some(s) == params.target.as_ref())
{
inner_path.remove(0);
params.target.as_ref().unwrap()
} else {
""
};

(target, inner_path.join("/"))
inner = inner_path.join("/");
(target, inner.trim_end_matches('/'))
};
let inner_path = if inner_path.is_empty() {
format!("{name}/index.html")
Expand Down Expand Up @@ -639,6 +644,21 @@ pub(crate) async fn get_all_platforms(
Ok(res.into_response())
}

pub(crate) async fn get_all_platforms_root(
Path(mut params): Path<RustdocHtmlParams>,
pool: Extension<Pool>,
) -> AxumResult<AxumResponse> {
params.path = None;
get_all_platforms_inner(Path(params), pool, true).await
}

pub(crate) async fn get_all_platforms(
params: Path<RustdocHtmlParams>,
pool: Extension<Pool>,
) -> AxumResult<AxumResponse> {
get_all_platforms_inner(params, pool, false).await
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -1251,22 +1271,26 @@ mod tests {

#[test]
fn platform_links_are_direct_and_without_nofollow() {
fn check_links(response_text: String, ajax: bool, should_contain_redirect: bool) {
let platform_links: Vec<(String, String)> = kuchikiki::parse_html()
fn check_links(
response_text: String,
ajax: bool,
should_contain_redirect: bool,
) -> Vec<(String, String, String)> {
let platform_links: Vec<(String, String, String)> = kuchikiki::parse_html()
.one(response_text)
.select(&format!(r#"{}li a"#, if ajax { "" } else { "#platforms " }))
.expect("invalid selector")
.map(|el| {
let attributes = el.attributes.borrow();
let url = attributes.get("href").expect("href").to_string();
let rel = attributes.get("rel").unwrap_or("").to_string();
(url, rel)
(el.text_contents(), url, rel)
})
.collect();

assert_eq!(platform_links.len(), 2);

for (url, rel) in platform_links {
for (_, url, rel) in &platform_links {
assert_eq!(
url.contains("/target-redirect/"),
should_contain_redirect,
Expand All @@ -1278,25 +1302,53 @@ mod tests {
assert_eq!(rel, "nofollow");
}
}
platform_links
}

fn run_check_links(
env: &TestEnvironment,
url: &str,
extra: &str,
should_contain_redirect: bool,
) {
run_check_links_redir(
env,
url,
extra,
should_contain_redirect,
should_contain_redirect,
)
}

fn run_check_links_redir(
env: &TestEnvironment,
url: &str,
extra: &str,
should_contain_redirect: bool,
ajax_should_contain_redirect: bool,
) {
let response = env.frontend().get(url).send().unwrap();
assert!(response.status().is_success());
check_links(response.text().unwrap(), false, should_contain_redirect);
let list1 = check_links(response.text().unwrap(), false, should_contain_redirect);
// Same test with AJAX endpoint.
let response = env
.frontend()
.get(&format!("/-/menus/platforms{url}{extra}"))
.send()
.unwrap();
assert!(response.status().is_success());
check_links(response.text().unwrap(), true, should_contain_redirect);
let list2 = check_links(response.text().unwrap(), true, ajax_should_contain_redirect);
if should_contain_redirect == ajax_should_contain_redirect {
assert_eq!(list1, list2);
} else {
// If redirects differ, we only check platform names.
assert!(
list1.iter().zip(&list2).all(|(a, b)| a.0 == b.0),
"{:?} != {:?}",
list1,
list2,
);
}
}

wrapper(|env| {
Expand All @@ -1308,8 +1360,16 @@ mod tests {
.rustdoc_file("x86_64-pc-windows-msvc/dummy/struct.A.html")
.default_target("x86_64-unknown-linux-gnu")
.add_target("x86_64-pc-windows-msvc")
.source_file("README.md", b"storage readme")
.create()?;

// FIXME: For some reason, there are target-redirects on non-AJAX lists on docs.rs
// crate pages other than the "default" one.
run_check_links_redir(env, "/crate/dummy/0.4.0/features", "", true, false);
run_check_links_redir(env, "/crate/dummy/0.4.0/builds", "", true, false);
run_check_links_redir(env, "/crate/dummy/0.4.0/source/", "", true, false);
run_check_links_redir(env, "/crate/dummy/0.4.0/source/README.md", "", true, false);

run_check_links(env, "/crate/dummy/0.4.0", "", false);
run_check_links(env, "/dummy/latest/dummy", "/", true);
run_check_links(env, "/dummy/0.4.0/x86_64-pc-windows-msvc/dummy", "/", true);
Expand Down
22 changes: 21 additions & 1 deletion src/web/routes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,27 @@ pub(super) fn build_axum_routes() -> AxumRouter {
)
.route(
"/-/menus/platforms/crate/:name/:version",
get_internal(super::crate_details::get_all_platforms),
get_internal(super::crate_details::get_all_platforms_root),
)
.route(
"/-/menus/platforms/crate/:name/:version/features",
get_internal(super::crate_details::get_all_platforms_root),
)
.route(
"/-/menus/platforms/crate/:name/:version/builds",
get_internal(super::crate_details::get_all_platforms_root),
)
.route(
"/-/menus/platforms/crate/:name/:version/builds/*path",
get_internal(super::crate_details::get_all_platforms_root),
)
.route(
"/-/menus/platforms/crate/:name/:version/source/",
get_internal(super::crate_details::get_all_platforms_root),
)
.route(
"/-/menus/platforms/crate/:name/:version/source/*path",
get_internal(super::crate_details::get_all_platforms_root),
)
.route(
"/-/menus/platforms/crate/:name/:version/:target",
Expand Down
2 changes: 0 additions & 2 deletions src/web/rustdoc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,9 +357,7 @@ pub(crate) struct RustdocHtmlParams {
// both target and path are only used for matching the route.
// The actual path is read from the request `Uri` because
// we have some static filenames directly in the routes.
#[allow(dead_code)]
pub(crate) target: Option<String>,
#[allow(dead_code)]
pub(crate) path: Option<String>,
}

Expand Down

0 comments on commit 42321b6

Please sign in to comment.