Introduced Nope::VersionNotFound error

Trying to reproduce the issue rust-lang#55 revealed another bug: the order in which the different handlers were called in CratesfyiHandler::handle resulted in the more specific errors from the router_handler being dropped, due to them being 404 and the last handler being the static_handler which would just return a ResourceNotFound error.
It should be noted that the different execution order might affect the performance in production.

Author: robinhundt

src/web/crate_details.rs

@@ -1,4 +1,3 @@
-use super::error::Nope;
 use super::{match_version, redirect_base, render_markdown, MatchSemver, MetaData};
 use crate::{db::Pool, impl_webpage, web::page::WebPage};
 use chrono::{DateTime, NaiveDateTime, Utc};
@@ -297,13 +296,13 @@ pub fn crate_details_handler(req: &mut Request) -> IronResult<Response> {
     let mut conn = extension!(req, Pool).get()?;
 
     match match_version(&mut conn, &name, req_version).and_then(|m| m.assume_exact()) {
-        Some(MatchSemver::Exact((version, _))) => {
+        Ok(MatchSemver::Exact((version, _))) => {
             let details = cexpect!(req, CrateDetails::new(&mut conn, &name, &version));
 
             CrateDetailsPage { details }.into_response(req)
         }
 
-        Some(MatchSemver::Semver((version, _))) => {
+        Ok(MatchSemver::Semver((version, _))) => {
             let url = ctry!(
                 req,
                 Url::parse(&format!(
@@ -317,7 +316,7 @@ pub fn crate_details_handler(req: &mut Request) -> IronResult<Response> {
             Ok(super::redirect(url))
         }
 
-        None => Err(IronError::new(Nope::CrateNotFound, status::NotFound)),
+        Err(err) => Err(IronError::new(err, status::NotFound)),
     }
 }
 

src/web/error.rs

@@ -10,6 +10,7 @@ use std::{error::Error, fmt};
 pub enum Nope {
     ResourceNotFound,
     CrateNotFound,
+    VersionNotFound,
     NoResults,
     InternalServerError,
 }
@@ -19,6 +20,7 @@ impl fmt::Display for Nope {
         f.write_str(match *self {
             Nope::ResourceNotFound => "Requested resource not found",
             Nope::CrateNotFound => "Requested crate not found",
+            Nope::VersionNotFound => "Requested crate does not have specified version",
             Nope::NoResults => "Search yielded no results",
             Nope::InternalServerError => "Internal server error",
         })
@@ -52,6 +54,17 @@ impl Handler for Nope {
                 .into_response(req)
             }
 
+            Nope::VersionNotFound => {
+                // user tried to navigate to a crate with a version that does not exist
+                // TODO: Display the attempted crate and version
+                ErrorPage {
+                    title: "The requested version does not exist",
+                    message: Some("no such version for this crate".into()),
+                    status: Status::NotFound,
+                }
+                .into_response(req)
+            }
+
             Nope::NoResults => {
                 let mut params = req.url.as_ref().query_pairs();
 

src/web/mod.rs

@@ -91,6 +91,7 @@ mod statics;
 
 use crate::{impl_webpage, Context};
 use chrono::{DateTime, Utc};
+use error::Nope;
 use extensions::InjectExtensions;
 use failure::Error;
 use iron::{
@@ -177,13 +178,14 @@ impl Handler for CratesfyiHandler {
             }
         };
 
-        // try serving shared rustdoc resources first, then router, then db/static file handler
-        // return 404 if none of them return Ok
+        // try serving shared rustdoc resources first, then db/static file handler and last router
+        // return 404 if none of them return Ok. It is important that the router comes last,
+        // because it gives the most specific errors, e.g. CrateNotFound or VersionNotFound
         self.shared_resource_handler
             .handle(req)
-            .or_else(|e| if_404(e, || self.router_handler.handle(req)))
             .or_else(|e| if_404(e, || self.database_file_handler.handle(req)))
             .or_else(|e| if_404(e, || self.static_handler.handle(req)))
+            .or_else(|e| if_404(e, || self.router_handler.handle(req)))
             .or_else(|e| {
                 let err = if let Some(err) = e.error.downcast::<error::Nope>() {
                     *err
@@ -245,12 +247,12 @@ struct MatchVersion {
 impl MatchVersion {
     /// If the matched version was an exact match to the requested crate name, returns the
     /// `MatchSemver` for the query. If the lookup required a dash/underscore conversion, returns
-    /// `None`.
-    fn assume_exact(self) -> Option<MatchSemver> {
+    /// `Nope::CrateNotFound`.
+    fn assume_exact(self) -> Result<MatchSemver, Nope> {
         if self.corrected_name.is_none() {
-            Some(self.version)
+            Ok(self.version)
         } else {
-            None
+            Err(Nope::CrateNotFound)
         }
     }
 }
@@ -284,7 +286,11 @@ impl MatchSemver {
 /// This function will also check for crates where dashes in the name (`-`) have been replaced with
 /// underscores (`_`) and vice-versa. The return value will indicate whether the crate name has
 /// been matched exactly, or if there has been a "correction" in the name that matched instead.
-fn match_version(conn: &mut Client, name: &str, version: Option<&str>) -> Option<MatchVersion> {
+fn match_version(
+    conn: &mut Client,
+    name: &str,
+    version: Option<&str>,
+) -> Result<MatchVersion, Nope> {
     // version is an Option<&str> from router::Router::get, need to decode first
     use iron::url::percent_encoding::percent_decode;
 
@@ -320,24 +326,31 @@ fn match_version(conn: &mut Client, name: &str, version: Option<&str>) -> Option
             .collect()
     };
 
+    if versions.is_empty() {
+        return Err(Nope::CrateNotFound);
+    }
+
     // first check for exact match, we can't expect users to use semver in query
     if let Some((version, id, _)) = versions.iter().find(|(vers, _, _)| vers == &req_version) {
-        return Some(MatchVersion {
+        return Ok(MatchVersion {
             corrected_name,
             version: MatchSemver::Exact((version.to_owned(), *id)),
         });
     }
 
     // Now try to match with semver
-    let req_sem_ver = VersionReq::parse(&req_version).ok()?;
+    let req_sem_ver = VersionReq::parse(&req_version).map_err(|_| Nope::CrateNotFound)?;
 
     // we need to sort versions first
     let versions_sem = {
         let mut versions_sem: Vec<(Version, i32)> = Vec::with_capacity(versions.len());
 
         for version in versions.iter().filter(|(_, _, yanked)| !yanked) {
             // in theory a crate must always have a semver compatible version, but check result just in case
-            versions_sem.push((Version::parse(&version.0).ok()?, version.1));
+            versions_sem.push((
+                Version::parse(&version.0).map_err(|_| Nope::InternalServerError)?,
+                version.1,
+            ));
         }
 
         versions_sem.sort();
@@ -349,7 +362,7 @@ fn match_version(conn: &mut Client, name: &str, version: Option<&str>) -> Option
         .iter()
         .find(|(vers, _)| req_sem_ver.matches(vers))
     {
-        return Some(MatchVersion {
+        return Ok(MatchVersion {
             corrected_name,
             version: MatchSemver::Semver((version.to_string(), *id)),
         });
@@ -358,13 +371,18 @@ fn match_version(conn: &mut Client, name: &str, version: Option<&str>) -> Option
     // semver is acting weird for '*' (any) range if a crate only have pre-release versions
     // return first non-yanked version if requested version is '*'
     if req_version == "*" {
-        return versions_sem.first().map(|v| MatchVersion {
-            corrected_name,
-            version: MatchSemver::Semver((v.0.to_string(), v.1)),
-        });
+        return versions_sem
+            .first()
+            .map(|v| MatchVersion {
+                corrected_name,
+                version: MatchSemver::Semver((v.0.to_string(), v.1)),
+            })
+            .ok_or(Nope::CrateNotFound);
     }
 
-    None
+    // Since we return with a CrateNotFound earlier if the db reply is empty,
+    // we know that versions were returned but none satisfied the version requirement
+    Err(Nope::VersionNotFound)
 }
 
 /// Wrapper around the Markdown parser and renderer to render markdown
@@ -581,7 +599,13 @@ mod test {
 
     fn version(v: Option<&str>, db: &TestDatabase) -> Option<String> {
         match_version(&mut db.conn(), "foo", v)
-            .and_then(|version| version.assume_exact().map(|semver| semver.into_parts().0))
+            .ok()
+            .and_then(|version| {
+                version
+                    .assume_exact()
+                    .ok()
+                    .map(|semver| semver.into_parts().0)
+            })
     }
 
     fn semver(version: &'static str) -> Option<String> {

src/web/releases.rs

@@ -557,7 +557,7 @@ pub fn search_handler(req: &mut Request) -> IronResult<Response> {
             // since we never pass a version into `match_version` here, we'll never get
             // `MatchVersion::Exact`, so the distinction between `Exact` and `Semver` doesn't
             // matter
-            if let Some(matchver) = match_version(&mut conn, &query, None) {
+            if let Ok(matchver) = match_version(&mut conn, &query, None) {
                 let (version, id) = matchver.version.into_parts();
                 let query = matchver.corrected_name.unwrap_or_else(|| query.to_string());
 

src/web/rustdoc.rs

@@ -143,16 +143,16 @@ pub fn rustdoc_redirector_handler(req: &mut Request) -> IronResult<Response> {
     // it doesn't matter if the version that was given was exact or not, since we're redirecting
     // anyway
     let (version, id) = match match_version(&mut conn, &crate_name, req_version) {
-        Some(v) => {
+        Ok(v) => {
             if let Some(new_name) = v.corrected_name {
                 // `match_version` checked against -/_ typos, so if we have a name here we should
                 // use that instead
                 crate_name = new_name;
             }
             v.version.into_parts()
         }
-        None => {
-            return Err(IronError::new(Nope::CrateNotFound, status::NotFound));
+        Err(err) => {
+            return Err(IronError::new(err, status::NotFound));
         }
     };
 
@@ -286,28 +286,29 @@ pub fn rustdoc_html_server_handler(req: &mut Request) -> IronResult<Response> {
     // * If there is an exact match, but the requested crate name was corrected (dashes vs. underscores), redirect to the corrected name.
     // * If there is a semver (but not exact) match, redirect to the exact version.
     // * Otherwise, return a 404.
-    let version = if let Some(match_vers) = match_version(&mut conn, &name, url_version) {
-        match match_vers.version {
-            MatchSemver::Exact((version, _)) => {
-                // Redirect when the requested crate name isn't correct
-                if let Some(name) = match_vers.corrected_name {
-                    return redirect(&name, &version, &req_path);
-                }
+    let version = match match_version(&mut conn, &name, url_version) {
+        Ok(match_vers) => {
+            match match_vers.version {
+                MatchSemver::Exact((version, _)) => {
+                    // Redirect when the requested crate name isn't correct
+                    if let Some(name) = match_vers.corrected_name {
+                        return redirect(&name, &version, &req_path);
+                    }
 
-                version
-            }
+                    version
+                }
 
-            // Redirect when the requested version isn't correct
-            MatchSemver::Semver((v, _)) => {
-                // to prevent cloudfront caching the wrong artifacts on URLs with loose semver
-                // versions, redirect the browser to the returned version instead of loading it
-                // immediately
-                return redirect(&name, &v, &req_path);
+                // Redirect when the requested version isn't correct
+                MatchSemver::Semver((v, _)) => {
+                    // to prevent cloudfront caching the wrong artifacts on URLs with loose semver
+                    // versions, redirect the browser to the returned version instead of loading it
+                    // immediately
+                    return redirect(&name, &v, &req_path);
+                }
             }
         }
-    } else {
-        // Return a 404, as a crate by that name and version doesn't exist
-        return Err(IronError::new(Nope::ResourceNotFound, status::NotFound));
+        // Return a 404, as a crate by that name and version requirement doesn't exist
+        Err(err) => return Err(IronError::new(err, status::NotFound)),
     };
 
     rendering_time.step("crate details");
@@ -527,7 +528,7 @@ pub fn badge_handler(req: &mut Request) -> IronResult<Response> {
 
     let options =
         match match_version(&mut conn, &name, Some(&version)).and_then(|m| m.assume_exact()) {
-            Some(MatchSemver::Exact((version, id))) => {
+            Ok(MatchSemver::Exact((version, id))) => {
                 let rows = ctry!(
                     req,
                     conn.query(
@@ -552,7 +553,7 @@ pub fn badge_handler(req: &mut Request) -> IronResult<Response> {
                 }
             }
 
-            Some(MatchSemver::Semver((version, _))) => {
+            Ok(MatchSemver::Semver((version, _))) => {
                 let base_url = format!("{}/{}/badge.svg", redirect_base(req), name);
                 let url = ctry!(
                     req,
@@ -562,7 +563,7 @@ pub fn badge_handler(req: &mut Request) -> IronResult<Response> {
                 return Ok(super::redirect(iron_url));
             }
 
-            None => BadgeOptions {
+            Err(_) => BadgeOptions {
                 subject: "docs".to_owned(),
                 status: "no builds".to_owned(),
                 color: "#e05d44".to_owned(),