Skip to content

Commit 69070c1

Browse files
Nemo157Nemo157
committed
Assert that redirects go directly to their target location
Previously the assertion allowed multiple redirect steps as long as it eventually got to the target. Now it's checked that the very first response directly returns a `Location` header to the final target.
1 parent 94ba4fa commit 69070c1

File tree

9 files changed

+161
-91
lines changed

9 files changed

+161
-91
lines changed

‎Cargo.lock

Lines changed: 12 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ kuchiki = "0.8"
113113
rand = "0.8"
114114
mockito = "0.31.0"
115115
test-case = "2.0.0"
116+
fn-error-context = "0.2.0"
116117

117118
[build-dependencies]
118119
time = "0.3"

‎src/test/mod.rs

Lines changed: 78 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,16 @@ use crate::repositories::RepositoryStatsUpdater;
77
use crate::storage::{Storage, StorageKind};
88
use crate::web::Server;
99
use crate::{BuildQueue, Config, Context, Index, Metrics};
10+
use anyhow::Context as _;
11+
use fn_error_context::context;
1012
use log::error;
1113
use once_cell::unsync::OnceCell;
1214
use postgres::Client as Connection;
1315
use reqwest::{
14-
blocking::{Client, RequestBuilder},
16+
blocking::{Client, ClientBuilder, RequestBuilder},
1517
Method,
1618
};
17-
use std::fs;
18-
use std::net::SocketAddr;
19-
use std::{panic, sync::Arc};
19+
use std::{fs, net::SocketAddr, panic, sync::Arc, time::Duration};
2020

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

59-
/// Make sure that a URL redirects to a specific page
60-
pub(crate) fn assert_redirect(path: &str, expected_target: &str, web: &TestFrontend) -> Result<()> {
61-
// Reqwest follows redirects automatically
62-
let response = web.get(path).send()?;
59+
fn assert_redirect_common(path: &str, expected_target: &str, web: &TestFrontend) -> Result<()> {
60+
let response = web.get_no_redirect(path).send()?;
6361
let status = response.status();
62+
if !status.is_redirection() {
63+
anyhow::bail!("non-redirect from GET {path}: {status}");
64+
}
65+
66+
let mut redirect_target = response
67+
.headers()
68+
.get("Location")
69+
.context("missing 'Location' header")?
70+
.to_str()
71+
.context("non-ASCII redirect")?;
72+
73+
if !expected_target.starts_with("http") {
74+
// TODO: Should be able to use Url::make_relative,
75+
// but https://github.com/servo/rust-url/issues/766
76+
let base = format!("http://{}", web.server_addr());
77+
redirect_target = redirect_target
78+
.strip_prefix(&base)
79+
.unwrap_or(redirect_target);
80+
}
6481

65-
let mut tmp;
66-
let redirect_target = if expected_target.starts_with("https://") {
67-
response.url().as_str()
68-
} else {
69-
tmp = String::from(response.url().path());
70-
if let Some(query) = response.url().query() {
71-
tmp.push('?');
72-
tmp.push_str(query);
73-
}
74-
&tmp
75-
};
76-
// Either we followed a redirect to the wrong place, or there was no redirect
7782
if redirect_target != expected_target {
78-
// wrong place
79-
if redirect_target != path {
80-
panic!(
81-
"{}: expected redirect to {}, got redirect to {}",
82-
path, expected_target, redirect_target
83-
);
84-
} else {
85-
// no redirect
86-
panic!(
87-
"{}: expected redirect to {}, got {}",
88-
path, expected_target, status
89-
);
90-
}
83+
anyhow::bail!("got redirect to {redirect_target}");
9184
}
92-
assert!(
93-
status.is_success(),
94-
"failed to GET {}: {}",
95-
expected_target,
96-
status
97-
);
85+
86+
Ok(())
87+
}
88+
89+
/// Makes sure that a URL redirects to a specific page, but doesn't check that the target exists
90+
#[context("expected redirect from {path} to {expected_target}")]
91+
pub(crate) fn assert_redirect_unchecked(
92+
path: &str,
93+
expected_target: &str,
94+
web: &TestFrontend,
95+
) -> Result<()> {
96+
assert_redirect_common(path, expected_target, web)
97+
}
98+
99+
/// Make sure that a URL redirects to a specific page, and that the target exists and is not another redirect
100+
#[context("expected redirect from {path} to {expected_target}")]
101+
pub(crate) fn assert_redirect(path: &str, expected_target: &str, web: &TestFrontend) -> Result<()> {
102+
assert_redirect_common(path, expected_target, web)?;
103+
104+
let response = web.get_no_redirect(expected_target).send()?;
105+
let status = response.status();
106+
if !status.is_success() {
107+
anyhow::bail!("failed to GET {expected_target}: {status}");
108+
}
109+
98110
Ok(())
99111
}
100112

@@ -113,6 +125,7 @@ pub(crate) fn init_logger() {
113125
// initializing rustwide logging also sets the global logger
114126
rustwide::logging::init_with(
115127
env_logger::Builder::from_env(env_logger::Env::default().filter("DOCSRS_LOG"))
128+
.format_timestamp_millis()
116129
.is_test(true)
117130
.build(),
118131
);
@@ -372,29 +385,50 @@ impl Drop for TestDatabase {
372385
pub(crate) struct TestFrontend {
373386
server: Server,
374387
pub(crate) client: Client,
388+
pub(crate) client_no_redirect: Client,
375389
}
376390

377391
impl TestFrontend {
378392
fn new(context: &dyn Context) -> Self {
393+
fn build(f: impl FnOnce(ClientBuilder) -> ClientBuilder) -> Client {
394+
let base = Client::builder()
395+
.connect_timeout(Duration::from_millis(2000))
396+
.timeout(Duration::from_millis(2000))
397+
// The test server only supports a single connection, so having two clients with
398+
// idle connections deadlocks the tests
399+
.pool_max_idle_per_host(0);
400+
f(base).build().unwrap()
401+
}
402+
379403
Self {
380404
server: Server::start(Some("127.0.0.1:0"), context)
381405
.expect("failed to start the web server"),
382-
client: Client::new(),
406+
client: build(|b| b),
407+
client_no_redirect: build(|b| b.redirect(reqwest::redirect::Policy::none())),
383408
}
384409
}
385410

386-
fn build_request(&self, method: Method, mut url: String) -> RequestBuilder {
411+
fn build_url(&self, url: &str) -> String {
387412
if url.is_empty() || url.starts_with('/') {
388-
url = format!("http://{}{}", self.server.addr(), url);
413+
format!("http://{}{}", self.server.addr(), url)
414+
} else {
415+
url.to_owned()
389416
}
390-
self.client.request(method, url)
391417
}
392418

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

397423
pub(crate) fn get(&self, url: &str) -> RequestBuilder {
398-
self.build_request(Method::GET, url.to_string())
424+
let url = self.build_url(url);
425+
log::debug!("getting {url}");
426+
self.client.request(Method::GET, url)
427+
}
428+
429+
pub(crate) fn get_no_redirect(&self, url: &str) -> RequestBuilder {
430+
let url = self.build_url(url);
431+
log::debug!("getting {url} (no redirects)");
432+
self.client_no_redirect.request(Method::GET, url)
399433
}
400434
}

‎src/web/crate_details.rs

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ pub struct Release {
7676
pub yanked: bool,
7777
pub is_library: bool,
7878
pub rustdoc_status: bool,
79+
pub target_name: String,
7980
}
8081

8182
impl CrateDetails {
@@ -241,15 +242,16 @@ pub(crate) fn releases_for_crate(
241242
) -> Result<Vec<Release>, anyhow::Error> {
242243
let mut releases: Vec<Release> = conn
243244
.query(
244-
"SELECT
245-
id,
245+
"SELECT
246+
id,
246247
version,
247248
build_status,
248249
yanked,
249250
is_library,
250-
rustdoc_status
251+
rustdoc_status,
252+
target_name
251253
FROM releases
252-
WHERE
254+
WHERE
253255
releases.crate_id = $1",
254256
&[&crate_id],
255257
)?
@@ -264,6 +266,7 @@ pub(crate) fn releases_for_crate(
264266
yanked: row.get("yanked"),
265267
is_library: row.get("is_library"),
266268
rustdoc_status: row.get("rustdoc_status"),
269+
target_name: row.get("target_name"),
267270
}),
268271
Err(err) => {
269272
report_error(&anyhow!(err).context(format!(
@@ -500,6 +503,7 @@ mod tests {
500503
is_library: true,
501504
rustdoc_status: true,
502505
id: details.releases[0].id,
506+
target_name: "foo".to_owned(),
503507
},
504508
Release {
505509
version: semver::Version::parse("0.12.0")?,
@@ -508,6 +512,7 @@ mod tests {
508512
is_library: true,
509513
rustdoc_status: true,
510514
id: details.releases[1].id,
515+
target_name: "foo".to_owned(),
511516
},
512517
Release {
513518
version: semver::Version::parse("0.3.0")?,
@@ -516,6 +521,7 @@ mod tests {
516521
is_library: true,
517522
rustdoc_status: false,
518523
id: details.releases[2].id,
524+
target_name: "foo".to_owned(),
519525
},
520526
Release {
521527
version: semver::Version::parse("0.2.0")?,
@@ -524,6 +530,7 @@ mod tests {
524530
is_library: true,
525531
rustdoc_status: true,
526532
id: details.releases[3].id,
533+
target_name: "foo".to_owned(),
527534
},
528535
Release {
529536
version: semver::Version::parse("0.2.0-alpha")?,
@@ -532,6 +539,7 @@ mod tests {
532539
is_library: true,
533540
rustdoc_status: true,
534541
id: details.releases[4].id,
542+
target_name: "foo".to_owned(),
535543
},
536544
Release {
537545
version: semver::Version::parse("0.1.1")?,
@@ -540,6 +548,7 @@ mod tests {
540548
is_library: true,
541549
rustdoc_status: true,
542550
id: details.releases[5].id,
551+
target_name: "foo".to_owned(),
543552
},
544553
Release {
545554
version: semver::Version::parse("0.1.0")?,
@@ -548,6 +557,7 @@ mod tests {
548557
is_library: true,
549558
rustdoc_status: true,
550559
id: details.releases[6].id,
560+
target_name: "foo".to_owned(),
551561
},
552562
Release {
553563
version: semver::Version::parse("0.0.1")?,
@@ -556,6 +566,7 @@ mod tests {
556566
is_library: false,
557567
rustdoc_status: false,
558568
id: details.releases[7].id,
569+
target_name: "foo".to_owned(),
559570
},
560571
]
561572
);

‎src/web/metrics.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,12 @@ impl<'a> RenderingTimesRecorder<'a> {
9292

9393
fn record_current(&mut self) {
9494
if let Some(current) = self.current.take() {
95+
#[cfg(test)]
96+
log::debug!(
97+
"rendering time - {}: {:?}",
98+
current.step,
99+
current.start.elapsed()
100+
);
95101
self.metric
96102
.with_label_values(&[current.step])
97103
.observe(duration_to_seconds(current.start.elapsed()));

‎src/web/mod.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,7 @@ struct MatchVersion {
223223
pub corrected_name: Option<String>,
224224
pub version: MatchSemver,
225225
pub rustdoc_status: bool,
226+
pub target_name: String,
226227
}
227228

228229
impl MatchVersion {
@@ -324,6 +325,7 @@ fn match_version(
324325
corrected_name,
325326
version: MatchSemver::Exact((release.version.to_string(), release.id)),
326327
rustdoc_status: release.rustdoc_status,
328+
target_name: release.target_name.clone(),
327329
});
328330
}
329331
}
@@ -358,6 +360,7 @@ fn match_version(
358360
MatchSemver::Semver((release.version.to_string(), release.id))
359361
},
360362
rustdoc_status: release.rustdoc_status,
363+
target_name: release.target_name.clone(),
361364
});
362365
}
363366

@@ -371,6 +374,7 @@ fn match_version(
371374
corrected_name: corrected_name.clone(),
372375
version: MatchSemver::Semver((release.version.to_string(), release.id)),
373376
rustdoc_status: release.rustdoc_status,
377+
target_name: release.target_name.clone(),
374378
})
375379
.ok_or(Nope::VersionNotFound);
376380
}
@@ -759,7 +763,7 @@ mod test {
759763
.create()
760764
.unwrap();
761765
let web = env.frontend();
762-
assert_redirect("/bat//", "/bat/latest/bat/", web)?;
766+
assert_redirect_unchecked("/bat//", "/bat/", web)?;
763767
Ok(())
764768
})
765769
}

0 commit comments

Comments
 (0)