Skip to content

Assert that redirects go directly to their target location #1814

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

Merged
merged 1 commit into from
Sep 1, 2022
Merged

Assert that redirects go directly to their target location #1814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ kuchiki = "0.8"
rand = "0.8"
mockito = "0.31.0"
test-case = "2.0.0"
fn-error-context = "0.2.0"

[build-dependencies]
time = "0.3"
Expand Down
122 changes: 78 additions & 44 deletions src/test/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,16 @@ use crate::repositories::RepositoryStatsUpdater;
use crate::storage::{Storage, StorageKind};
use crate::web::Server;
use crate::{BuildQueue, Config, Context, Index, Metrics};
use anyhow::Context as _;
use fn_error_context::context;
use log::error;
use once_cell::unsync::OnceCell;
use postgres::Client as Connection;
use reqwest::{
blocking::{Client, RequestBuilder},
blocking::{Client, ClientBuilder, RequestBuilder},
Method,
};
use std::fs;
use std::net::SocketAddr;
use std::{panic, sync::Arc};
use std::{fs, net::SocketAddr, panic, sync::Arc, time::Duration};

pub(crate) fn wrapper(f: impl FnOnce(&TestEnvironment) -> Result<()>) {
let _ = dotenv::dotenv();
Expand Down Expand Up @@ -56,45 +56,57 @@ pub(crate) fn assert_not_found(path: &str, web: &TestFrontend) -> Result<()> {
Ok(())
}

/// Make sure that a URL redirects to a specific page
pub(crate) fn assert_redirect(path: &str, expected_target: &str, web: &TestFrontend) -> Result<()> {
// Reqwest follows redirects automatically
let response = web.get(path).send()?;
fn assert_redirect_common(path: &str, expected_target: &str, web: &TestFrontend) -> Result<()> {
let response = web.get_no_redirect(path).send()?;
let status = response.status();
if !status.is_redirection() {
anyhow::bail!("non-redirect from GET {path}: {status}");
}

let mut redirect_target = response
.headers()
.get("Location")
.context("missing 'Location' header")?
.to_str()
.context("non-ASCII redirect")?;

if !expected_target.starts_with("http") {
// TODO: Should be able to use Url::make_relative,
// but https://github.com/servo/rust-url/issues/766
let base = format!("http://{}", web.server_addr());
redirect_target = redirect_target
.strip_prefix(&base)
.unwrap_or(redirect_target);
}

let mut tmp;
let redirect_target = if expected_target.starts_with("https://") {
response.url().as_str()
} else {
tmp = String::from(response.url().path());
if let Some(query) = response.url().query() {
tmp.push('?');
tmp.push_str(query);
}
&tmp
};
// Either we followed a redirect to the wrong place, or there was no redirect
if redirect_target != expected_target {
// wrong place
if redirect_target != path {
panic!(
"{}: expected redirect to {}, got redirect to {}",
path, expected_target, redirect_target
);
} else {
// no redirect
panic!(
"{}: expected redirect to {}, got {}",
path, expected_target, status
);
}
anyhow::bail!("got redirect to {redirect_target}");
}
assert!(
status.is_success(),
"failed to GET {}: {}",
expected_target,
status
);

Ok(())
}

/// Makes sure that a URL redirects to a specific page, but doesn't check that the target exists
#[context("expected redirect from {path} to {expected_target}")]
pub(crate) fn assert_redirect_unchecked(
path: &str,
expected_target: &str,
web: &TestFrontend,
) -> Result<()> {
assert_redirect_common(path, expected_target, web)
}

/// Make sure that a URL redirects to a specific page, and that the target exists and is not another redirect
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Make sure that a URL redirects to a specific page, and that the target exists and is not another redirect
/// Make sure that a URL redirects to a specific page, that the target exists and is not another redirect

(nit)

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 think the double and is more correct, even if a little weird

Make sure (that a URL redirects to a specific page), and (that the target (exists) and (is not another redirect)).

vs

Make sure (that a URL redirects to a specific page), (that the target exists) and (is not another redirect).

#[context("expected redirect from {path} to {expected_target}")]
pub(crate) fn assert_redirect(path: &str, expected_target: &str, web: &TestFrontend) -> Result<()> {
assert_redirect_common(path, expected_target, web)?;

let response = web.get_no_redirect(expected_target).send()?;
let status = response.status();
if !status.is_success() {
anyhow::bail!("failed to GET {expected_target}: {status}");
}

Ok(())
}

Expand All @@ -113,6 +125,7 @@ pub(crate) fn init_logger() {
// initializing rustwide logging also sets the global logger
rustwide::logging::init_with(
env_logger::Builder::from_env(env_logger::Env::default().filter("DOCSRS_LOG"))
.format_timestamp_millis()
.is_test(true)
.build(),
);
Expand Down Expand Up @@ -372,29 +385,50 @@ impl Drop for TestDatabase {
pub(crate) struct TestFrontend {
server: Server,
pub(crate) client: Client,
pub(crate) client_no_redirect: Client,
}

impl TestFrontend {
fn new(context: &dyn Context) -> Self {
fn build(f: impl FnOnce(ClientBuilder) -> ClientBuilder) -> Client {
let base = Client::builder()
.connect_timeout(Duration::from_millis(2000))
.timeout(Duration::from_millis(2000))
// The test server only supports a single connection, so having two clients with
// idle connections deadlocks the tests
.pool_max_idle_per_host(0);
f(base).build().unwrap()
}

Self {
server: Server::start(Some("127.0.0.1:0"), context)
.expect("failed to start the web server"),
client: Client::new(),
client: build(|b| b),
client_no_redirect: build(|b| b.redirect(reqwest::redirect::Policy::none())),
}
}

fn build_request(&self, method: Method, mut url: String) -> RequestBuilder {
fn build_url(&self, url: &str) -> String {
if url.is_empty() || url.starts_with('/') {
url = format!("http://{}{}", self.server.addr(), url);
format!("http://{}{}", self.server.addr(), url)
} else {
url.to_owned()
}
self.client.request(method, url)
}

pub(crate) fn server_addr(&self) -> SocketAddr {
self.server.addr()
}

pub(crate) fn get(&self, url: &str) -> RequestBuilder {
self.build_request(Method::GET, url.to_string())
let url = self.build_url(url);
log::debug!("getting {url}");
self.client.request(Method::GET, url)
}

pub(crate) fn get_no_redirect(&self, url: &str) -> RequestBuilder {
let url = self.build_url(url);
log::debug!("getting {url} (no redirects)");
self.client_no_redirect.request(Method::GET, url)
}
}
19 changes: 15 additions & 4 deletions src/web/crate_details.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ pub struct Release {
pub yanked: bool,
pub is_library: bool,
pub rustdoc_status: bool,
pub target_name: String,
}

impl CrateDetails {
Expand Down Expand Up @@ -246,15 +247,16 @@ pub(crate) fn releases_for_crate(
) -> Result<Vec<Release>, anyhow::Error> {
let mut releases: Vec<Release> = conn
.query(
"SELECT
id,
"SELECT
id,
version,
build_status,
yanked,
is_library,
rustdoc_status
rustdoc_status,
target_name
FROM releases
WHERE
WHERE
releases.crate_id = $1",
&[&crate_id],
)?
Expand All @@ -269,6 +271,7 @@ pub(crate) fn releases_for_crate(
yanked: row.get("yanked"),
is_library: row.get("is_library"),
rustdoc_status: row.get("rustdoc_status"),
target_name: row.get("target_name"),
}),
Err(err) => {
report_error(&anyhow!(err).context(format!(
Expand Down Expand Up @@ -505,6 +508,7 @@ mod tests {
is_library: true,
rustdoc_status: true,
id: details.releases[0].id,
target_name: "foo".to_owned(),
},
Release {
version: semver::Version::parse("0.12.0")?,
Expand All @@ -513,6 +517,7 @@ mod tests {
is_library: true,
rustdoc_status: true,
id: details.releases[1].id,
target_name: "foo".to_owned(),
},
Release {
version: semver::Version::parse("0.3.0")?,
Expand All @@ -521,6 +526,7 @@ mod tests {
is_library: true,
rustdoc_status: false,
id: details.releases[2].id,
target_name: "foo".to_owned(),
},
Release {
version: semver::Version::parse("0.2.0")?,
Expand All @@ -529,6 +535,7 @@ mod tests {
is_library: true,
rustdoc_status: true,
id: details.releases[3].id,
target_name: "foo".to_owned(),
},
Release {
version: semver::Version::parse("0.2.0-alpha")?,
Expand All @@ -537,6 +544,7 @@ mod tests {
is_library: true,
rustdoc_status: true,
id: details.releases[4].id,
target_name: "foo".to_owned(),
},
Release {
version: semver::Version::parse("0.1.1")?,
Expand All @@ -545,6 +553,7 @@ mod tests {
is_library: true,
rustdoc_status: true,
id: details.releases[5].id,
target_name: "foo".to_owned(),
},
Release {
version: semver::Version::parse("0.1.0")?,
Expand All @@ -553,6 +562,7 @@ mod tests {
is_library: true,
rustdoc_status: true,
id: details.releases[6].id,
target_name: "foo".to_owned(),
},
Release {
version: semver::Version::parse("0.0.1")?,
Expand All @@ -561,6 +571,7 @@ mod tests {
is_library: false,
rustdoc_status: false,
id: details.releases[7].id,
target_name: "foo".to_owned(),
},
]
);
Expand Down
6 changes: 6 additions & 0 deletions src/web/metrics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ impl<'a> RenderingTimesRecorder<'a> {

fn record_current(&mut self) {
if let Some(current) = self.current.take() {
#[cfg(test)]
log::debug!(
"rendering time - {}: {:?}",
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 added this while trying to figure out why I was getting timeouts from the two reqwest clients deadlocking, but I thought it could be useful to leave in.

current.step,
current.start.elapsed()
);
self.metric
.with_label_values(&[current.step])
.observe(duration_to_seconds(current.start.elapsed()));
Expand Down
6 changes: 5 additions & 1 deletion src/web/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ struct MatchVersion {
pub corrected_name: Option<String>,
pub version: MatchSemver,
pub rustdoc_status: bool,
pub target_name: String,
}

impl MatchVersion {
Expand Down Expand Up @@ -324,6 +325,7 @@ fn match_version(
corrected_name,
version: MatchSemver::Exact((release.version.to_string(), release.id)),
rustdoc_status: release.rustdoc_status,
target_name: release.target_name.clone(),
});
}
}
Expand Down Expand Up @@ -358,6 +360,7 @@ fn match_version(
MatchSemver::Semver((release.version.to_string(), release.id))
},
rustdoc_status: release.rustdoc_status,
target_name: release.target_name.clone(),
});
}

Expand All @@ -371,6 +374,7 @@ fn match_version(
corrected_name: corrected_name.clone(),
version: MatchSemver::Semver((release.version.to_string(), release.id)),
rustdoc_status: release.rustdoc_status,
target_name: release.target_name.clone(),
})
.ok_or(Nope::VersionNotFound);
}
Expand Down Expand Up @@ -759,7 +763,7 @@ mod test {
.create()
.unwrap();
let web = env.frontend();
assert_redirect("/bat//", "/bat/latest/bat/", web)?;
assert_redirect_unchecked("/bat//", "/bat/", web)?;
Copy link
Member Author

Choose a reason for hiding this comment

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

This redirect is handled by iron, so there's no easy way to make it go directly to the final target.

Ok(())
})
}
Expand Down
Loading