Skip to content

Commit

Permalink
Remove database_file_handler; give precendence to router error mess…
Browse files Browse the repository at this point in the history
…ages

The database handler was unused; the router already serves the favicon and it
doesn't do anything else. Note that 'database handler' is different
from 'files served from storage', the router already handles those.

This also fixes the long standing bug that all 404 errors are either
'crate not found' or 'resource not found'.
  • Loading branch information
jyn514 committed Mar 21, 2021
1 parent 214710f commit 228ee9f
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 20 deletions.
11 changes: 4 additions & 7 deletions src/web/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,9 +163,6 @@ mod tests {

#[test]
fn check_404_page_content_resource() {
// Resources with a `.js` and `.ico` extension are special cased in the
// routes_handler which is currently run last. This means that `get("resource.exe")` will
// fail with a `no so such crate` instead of 'no such resource'
wrapper(|env| {
let page = kuchiki::parse_html().one(
env.frontend()
Expand All @@ -180,7 +177,7 @@ mod tests {
.next()
.unwrap()
.text_contents(),
"The requested crate does not exist",
"The requested resource does not exist",
);

Ok(())
Expand All @@ -200,7 +197,7 @@ mod tests {
.next()
.unwrap()
.text_contents(),
"The requested crate does not exist",
"The requested version does not exist",
);

Ok(())
Expand All @@ -219,7 +216,7 @@ mod tests {
.next()
.unwrap()
.text_contents(),
"The requested crate does not exist",
"The requested version does not exist",
);

Ok(())
Expand All @@ -242,7 +239,7 @@ mod tests {
.next()
.unwrap()
.text_contents(),
"The requested crate does not exist",
"The requested version does not exist",
);

Ok(())
Expand Down
16 changes: 4 additions & 12 deletions src/web/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ use iron::{
status::Status,
Chain, Handler, Iron, IronError, IronResult, Listening, Request, Response, Url,
};
use metrics::RequestRecorder;
use page::TemplateData;
use postgres::Client;
use router::NoRoute;
Expand All @@ -121,7 +120,6 @@ const DEFAULT_BIND: &str = "0.0.0.0:3000";
struct CratesfyiHandler {
shared_resource_handler: Box<dyn Handler>,
router_handler: Box<dyn Handler>,
database_file_handler: Box<dyn Handler>,
inject_extensions: InjectExtensions,
}

Expand All @@ -140,19 +138,13 @@ impl CratesfyiHandler {
let inject_extensions = InjectExtensions::new(context, template_data)?;

let routes = routes::build_routes();
let blacklisted_prefixes = routes.page_prefixes();

let shared_resources =
Self::chain(inject_extensions.clone(), rustdoc::SharedResourceHandler);
let router_chain = Self::chain(inject_extensions.clone(), routes.iron_router());

Ok(CratesfyiHandler {
shared_resource_handler: Box::new(shared_resources),
router_handler: Box::new(router_chain),
database_file_handler: Box::new(routes::BlockBlacklistedPrefixes::new(
blacklisted_prefixes,
Box::new(RequestRecorder::new(file::DatabaseFileHandler, "database")),
)),
inject_extensions,
})
}
Expand All @@ -165,7 +157,9 @@ impl Handler for CratesfyiHandler {
handle: impl FnOnce() -> IronResult<Response>,
) -> IronResult<Response> {
if e.response.status == Some(status::NotFound) {
handle()
// the routes are ordered from most specific to least; give precedence to the
// original error message.
handle().or(Err(e))
} else {
Err(e)
}
Expand All @@ -174,8 +168,7 @@ impl Handler for CratesfyiHandler {
// This is kind of a mess.
//
// Almost all files should be served through the `router_handler`; eventually
// `shared_resource_handler` should go through the router too, and `database_file_handler`
// could be removed altogether.
// `shared_resource_handler` should go through the router too.
//
// Unfortunately, combining `shared_resource_handler` with the `router_handler` breaks
// things, because right now `shared_resource_handler` allows requesting files from *any*
Expand All @@ -188,7 +181,6 @@ impl Handler for CratesfyiHandler {
self.router_handler
.handle(req)
.or_else(|e| if_404(e, || self.shared_resource_handler.handle(req)))
.or_else(|e| if_404(e, || self.database_file_handler.handle(req)))
.or_else(|e| {
let err = if let Some(err) = e.error.downcast_ref::<error::Nope>() {
*err
Expand Down
2 changes: 1 addition & 1 deletion src/web/rustdoc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ pub fn target_redirect_handler(req: &mut Request) -> IronResult<Response> {

let crate_details = match CrateDetails::new(&mut conn, &name, &version) {
Some(krate) => krate,
None => return Err(Nope::ResourceNotFound.into()),
None => return Err(Nope::VersionNotFound.into()),
};

// [crate, :name, :version, target-redirect, :target, *path]
Expand Down

0 comments on commit 228ee9f

Please sign in to comment.