Introduced Nope::VersionNotFound error

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();
 
@@ -100,11 +113,37 @@ mod tests {
     use kuchiki::traits::TendrilSink;
 
     #[test]
-    fn check_404_page_content() {
+    fn check_404_page_content_crate() {
+        wrapper(|env| {
+            let page = kuchiki::parse_html().one(
+                env.frontend()
+                    .get("/crate-which-doesnt-exist")
+                    .send()?
+                    .text()?,
+            );
+            assert_eq!(page.select("#crate-title").unwrap().count(), 1);
+            assert_eq!(
+                page.select("#crate-title")
+                    .unwrap()
+                    .next()
+                    .unwrap()
+                    .text_contents(),
+                "The requested crate does not exist",
+            );
+
+            Ok(())
+        });
+    }
+
+    #[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()
-                    .get("/page-which-doesnt-exist")
+                    .get("/resource-which-doesnt-exist.js")
                     .send()?
                     .text()?,
             );
@@ -121,4 +160,66 @@ mod tests {
             Ok(())
         });
     }
+
+    #[test]
+    fn check_404_page_content_not_semver_version() {
+        wrapper(|env| {
+            env.fake_release().name("dummy").create()?;
+            let page =
+                kuchiki::parse_html().one(env.frontend().get("/dummy/not-semver").send()?.text()?);
+            assert_eq!(page.select("#crate-title").unwrap().count(), 1);
+            assert_eq!(
+                page.select("#crate-title")
+                    .unwrap()
+                    .next()
+                    .unwrap()
+                    .text_contents(),
+                "The requested version does not exist",
+            );
+
+            Ok(())
+        });
+    }
+
+    #[test]
+    fn check_404_page_content_nonexistent_version() {
+        wrapper(|env| {
+            env.fake_release().name("dummy").version("1.0.0").create()?;
+            let page = kuchiki::parse_html().one(env.frontend().get("/dummy/2.0").send()?.text()?);
+            assert_eq!(page.select("#crate-title").unwrap().count(), 1);
+            assert_eq!(
+                page.select("#crate-title")
+                    .unwrap()
+                    .next()
+                    .unwrap()
+                    .text_contents(),
+                "The requested version does not exist",
+            );
+
+            Ok(())
+        });
+    }
+
+    #[test]
+    fn check_404_page_content_any_version_all_yanked() {
+        wrapper(|env| {
+            env.fake_release()
+                .name("dummy")
+                .version("1.0.0")
+                .yanked(true)
+                .create()?;
+            let page = kuchiki::parse_html().one(env.frontend().get("/dummy/*").send()?.text()?);
+            assert_eq!(page.select("#crate-title").unwrap().count(), 1);
+            assert_eq!(
+                page.select("#crate-title")
+                    .unwrap()
+                    .next()
+                    .unwrap()
+                    .text_contents(),
+                "The requested version does not exist",
+            );
+
+            Ok(())
+        });
+    }
 }

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> {
+    /// `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,37 @@ 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::VersionNotFound)?;
 
     // 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));
+            // in theory a crate must always have a semver compatible version,
+            // but check result just in case
+            let version_sem = Version::parse(&version.0).map_err(|_| {
+                log::error!(
+                    "invalid semver in database for crate {}: {}",
+                    name,
+                    version.0
+                );
+                Nope::InternalServerError
+            })?;
+            versions_sem.push((version_sem, version.1));
         }
 
         versions_sem.sort();
@@ -349,22 +368,27 @@ 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)),
         });
     }
 
-    // semver is acting weird for '*' (any) range if a crate only have pre-release versions
+    // semver is acting weird for '*' (any) range if a crate only has 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::VersionNotFound);
     }
 
-    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
@@ -580,8 +604,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))
+        let version = match_version(&mut db.conn(), "foo", v)
+            .ok()?
+            .assume_exact()
+            .ok()?
+            .into_parts()
+            .0;
+        Some(version)
     }
 
     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());