Skip to content

Keep arguments in redirections #2800

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Keep arguments in redirections #2800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
681c3ed
Keep arguments in redirections
GuillaumeGomez Apr 11, 2025
e78f44e
Fix clippy lints
GuillaumeGomez Apr 11, 2025
f085c5c
Remove `get_query_and_fragment` function and only get `query`
GuillaumeGomez Apr 12, 2025
39430b6
Add missing redirection in `rustdoc_redirector_handler`
GuillaumeGomez Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/web/builds.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::{
impl_axum_webpage,
web::{
MetaData, ReqVersion,
error::AxumResult,
error::{AxumResult, EscapedURI},
extractors::{DbConnection, Path},
filters, match_version,
page::templates::{RenderRegular, RenderSolid},
Expand Down Expand Up @@ -69,7 +69,7 @@ pub(crate) async fn build_list_handler(
.assume_exact_name()?
.into_canonical_req_version_or_else(|version| {
AxumNope::Redirect(
format!("/crate/{name}/{version}/builds"),
EscapedURI::new(&format!("/crate/{name}/{version}/builds"), None),
CachePolicy::ForeverInCdn,
)
})?
Expand All @@ -93,7 +93,7 @@ pub(crate) async fn build_list_json_handler(
.assume_exact_name()?
.into_canonical_req_version_or_else(|version| {
AxumNope::Redirect(
format!("/crate/{name}/{version}/builds.json"),
EscapedURI::new(&format!("/crate/{name}/{version}/builds.json"), None),
CachePolicy::ForeverInCdn,
)
})?
Expand Down
40 changes: 24 additions & 16 deletions src/web/crate_details.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ use crate::{
web::{
MatchedRelease, ReqVersion,
cache::CachePolicy,
encode_url_path,
error::{AxumNope, AxumResult},
error::{AxumNope, AxumResult, EscapedURI},
extractors::{DbConnection, Path},
page::templates::{RenderRegular, RenderSolid, filters},
rustdoc::RustdocHtmlParams,
Expand Down Expand Up @@ -475,7 +474,10 @@ pub(crate) async fn crate_details_handler(
) -> AxumResult<AxumResponse> {
let req_version = params.version.ok_or_else(|| {
AxumNope::Redirect(
format!("/crate/{}/{}", &params.name, ReqVersion::Latest),
EscapedURI::new(
&format!("/crate/{}/{}", &params.name, ReqVersion::Latest),
None,
),
CachePolicy::ForeverInCdn,
)
})?;
Expand All @@ -485,7 +487,7 @@ pub(crate) async fn crate_details_handler(
.assume_exact_name()?
.into_canonical_req_version_or_else(|version| {
AxumNope::Redirect(
format!("/crate/{}/{}", &params.name, version),
EscapedURI::new(&format!("/crate/{}/{}", &params.name, version), None),
CachePolicy::ForeverInCdn,
)
})?;
Expand Down Expand Up @@ -694,23 +696,29 @@ pub(crate) async fn get_all_platforms_inner(
.await?
.into_exactly_named_or_else(|corrected_name, req_version| {
AxumNope::Redirect(
encode_url_path(&format!(
"/platforms/{}/{}/{}",
corrected_name,
req_version,
req_path.join("/")
)),
EscapedURI::new(
&format!(
"/platforms/{}/{}/{}",
corrected_name,
req_version,
req_path.join("/")
),
None,
),
CachePolicy::NoCaching,
)
})?
.into_canonical_req_version_or_else(|version| {
AxumNope::Redirect(
encode_url_path(&format!(
"/platforms/{}/{}/{}",
&params.name,
version,
req_path.join("/")
)),
EscapedURI::new(
&format!(
"/platforms/{}/{}/{}",
&params.name,
version,
req_path.join("/")
),
None,
),
CachePolicy::ForeverInCdn,
)
})?;
Expand Down
34 changes: 27 additions & 7 deletions src/web/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,24 @@ use tracing::error;

use super::AxumErrorPage;

#[derive(Debug)]
pub struct EscapedURI(String);

impl EscapedURI {
pub fn new(path: &str, query: Option<&str>) -> Self {
let mut path = encode_url_path(path);
if let Some(query) = query {
path.push('?');
path.push_str(query);
}
Self(path)
}

pub fn as_str(&self) -> &str {
self.0.as_str()
}
}

#[derive(Debug, thiserror::Error)]
pub enum AxumNope {
#[error("Requested resource not found")]
Expand All @@ -35,7 +53,7 @@ pub enum AxumNope {
#[error("bad request")]
BadRequest(anyhow::Error),
#[error("redirect")]
Redirect(String, CachePolicy),
Redirect(EscapedURI, CachePolicy),
}

// FUTURE: Ideally, the split between the 3 kinds of responses would
Expand Down Expand Up @@ -118,8 +136,8 @@ struct ErrorInfo {
pub status: StatusCode,
}

fn redirect_with_policy(target: String, cache_policy: CachePolicy) -> AxumResponse {
match super::axum_cached_redirect(encode_url_path(&target), cache_policy) {
fn redirect_with_policy(target: EscapedURI, cache_policy: CachePolicy) -> AxumResponse {
match super::axum_cached_redirect(target.0, cache_policy) {
Ok(response) => response.into_response(),
Err(err) => AxumNope::InternalError(err).into_response(),
}
Expand Down Expand Up @@ -215,16 +233,18 @@ pub(crate) type JsonAxumResult<T> = Result<T, JsonAxumNope>;

#[cfg(test)]
mod tests {
use super::{AxumNope, IntoResponse};
use super::{AxumNope, EscapedURI, IntoResponse};
use crate::test::{AxumResponseTestExt, AxumRouterTestExt, async_wrapper};
use crate::web::cache::CachePolicy;
use kuchikiki::traits::TendrilSink;

#[test]
fn test_redirect_error_encodes_url_path() {
let response =
AxumNope::Redirect("/something>".into(), CachePolicy::ForeverInCdnAndBrowser)
.into_response();
let response = AxumNope::Redirect(
EscapedURI::new("/something>", None),
CachePolicy::ForeverInCdnAndBrowser,
)
.into_response();

assert_eq!(response.status(), 302);
assert_eq!(response.headers().get("Location").unwrap(), "/something%3E");
Expand Down
4 changes: 2 additions & 2 deletions src/web/features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
web::{
MetaData, ReqVersion,
cache::CachePolicy,
error::{AxumNope, AxumResult},
error::{AxumNope, AxumResult, EscapedURI},
extractors::{DbConnection, Path},
filters,
headers::CanonicalUrl,
Expand Down Expand Up @@ -127,7 +127,7 @@ pub(crate) async fn build_features_handler(
.assume_exact_name()?
.into_canonical_req_version_or_else(|version| {
AxumNope::Redirect(
format!("/crate/{}/{}/features", &name, version),
EscapedURI::new(&format!("/crate/{}/{}/features", &name, version), None),
CachePolicy::ForeverInCdn,
)
})?
Expand Down
44 changes: 28 additions & 16 deletions src/web/rustdoc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::{
crate_details::CrateDetails,
csp::Csp,
encode_url_path,
error::{AxumNope, AxumResult},
error::{AxumNope, AxumResult, EscapedURI},
extractors::{DbConnection, Path},
file::File,
match_version,
Expand Down Expand Up @@ -253,7 +253,11 @@ pub(crate) async fn rustdoc_redirector_handler(
.into_response())
} else {
Ok(axum_cached_redirect(
format!("/crate/{crate_name}/{}", matched_release.req_version),
EscapedURI::new(
&format!("/crate/{crate_name}/{}", matched_release.req_version),
uri.query(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm missing a test for this updated behaviour here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't figure out what's the URL for it. I tried /crate/sysinfo/0.29.0?search=q and /crate/sysinfo/0.40.0?search=q and both have the query arguments even before this change. Any idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the redirector is behind URLs like /sysinfo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah wait, you're searching for an example for this specific else part

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you will only reach it when rustdoc_status is false.

That being said, should we update the query_pairs in the redirect_to_doc function in here too to your new EscapedURI?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure it's worth it considering we need to overload the search query if it exists and then we make use of axum_parse_uri_with_params.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you will only reach it when rustdoc_status is false.

Can't find a way to write a test with this case. Do we have a test with this situation by any chance?

)
.as_str(),
CachePolicy::ForeverInCdn,
)?
.into_response())
Expand Down Expand Up @@ -369,11 +373,15 @@ pub(crate) async fn rustdoc_html_server_handler(
vers: &Version,
path: &[&str],
cache_policy: CachePolicy,
uri: &Uri,
) -> AxumResult<AxumResponse> {
trace!("redirect");
// Format and parse the redirect url
Ok(axum_cached_redirect(
encode_url_path(&format!("/{}/{}/{}", name, vers, path.join("/"))),
EscapedURI::new(
&format!("/{}/{}/{}", name, vers, path.join("/")),
uri.query(),
)
.as_str(),
cache_policy,
)?
.into_response())
Expand All @@ -392,23 +400,19 @@ pub(crate) async fn rustdoc_html_server_handler(
.await?
.into_exactly_named_or_else(|corrected_name, req_version| {
AxumNope::Redirect(
encode_url_path(&format!(
"/{}/{}/{}",
corrected_name,
req_version,
req_path.join("/")
)),
EscapedURI::new(
&format!("/{}/{}/{}", corrected_name, req_version, req_path.join("/")),
uri.query(),
),
CachePolicy::NoCaching,
)
})?
.into_canonical_req_version_or_else(|version| {
AxumNope::Redirect(
encode_url_path(&format!(
"/{}/{}/{}",
&params.name,
version,
req_path.join("/")
)),
EscapedURI::new(
&format!("/{}/{}/{}", &params.name, version, req_path.join("/")),
None,
),
CachePolicy::ForeverInCdn,
)
})?;
Expand Down Expand Up @@ -439,6 +443,7 @@ pub(crate) async fn rustdoc_html_server_handler(
&krate.version,
&req_path[1..],
CachePolicy::ForeverInCdn,
&uri,
);
}

Expand Down Expand Up @@ -494,6 +499,7 @@ pub(crate) async fn rustdoc_html_server_handler(
&krate.version,
&req_path,
CachePolicy::ForeverInCdn,
&uri,
);
}
}
Expand Down Expand Up @@ -2004,6 +2010,12 @@ mod test {
"/foo-ab/0.0.1/foo_ab/",
)
.await?;
// `-` becomes `_` but we keep the query arguments.
web.assert_redirect_unchecked(
"/foo-ab/0.0.1/foo_ab/?search=a",
"/foo_ab/0.0.1/foo_ab/?search=a",
)
.await?;
Ok(())
})
}
Expand Down
16 changes: 11 additions & 5 deletions src/web/source.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
web::{
MetaData, ReqVersion,
cache::CachePolicy,
error::AxumNope,
error::{AxumNope, EscapedURI},
extractors::Path,
file::File as DbFile,
headers::CanonicalUrl,
Expand Down Expand Up @@ -203,16 +203,22 @@ pub(crate) async fn source_browser_handler(
.await?
.into_exactly_named_or_else(|corrected_name, req_version| {
AxumNope::Redirect(
format!(
"/crate/{corrected_name}/{req_version}/source/{}",
params.path
EscapedURI::new(
&format!(
"/crate/{corrected_name}/{req_version}/source/{}",
params.path
),
None,
),
CachePolicy::NoCaching,
)
})?
.into_canonical_req_version_or_else(|version| {
AxumNope::Redirect(
format!("/crate/{}/{version}/source/{}", params.name, params.path),
EscapedURI::new(
&format!("/crate/{}/{version}/source/{}", params.name, params.path),
None,
),
CachePolicy::ForeverInCdn,
)
})?
Expand Down
4 changes: 2 additions & 2 deletions src/web/status.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use super::{cache::CachePolicy, error::AxumNope};
use crate::web::{
ReqVersion,
error::AxumResult,
error::{AxumResult, EscapedURI},
extractors::{DbConnection, Path},
match_version,
};
Expand All @@ -28,7 +28,7 @@ pub(crate) async fn status_handler(
let version = matched_release
.into_canonical_req_version_or_else(|version| {
AxumNope::Redirect(
format!("/crate/{name}/{version}/status.json"),
EscapedURI::new(&format!("/crate/{name}/{version}/status.json"), None),
CachePolicy::NoCaching,
)
})?
Expand Down
Loading