Applied suggestions for #1043

Tests are still TODO

Author: robinhundt

src/web/mod.rs

@@ -247,7 +247,7 @@ 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
-    /// `Nope::CrateNotFound`.
+    /// `CrateNotFound`.
     fn assume_exact(self) -> Result<MatchSemver, Nope> {
         if self.corrected_name.is_none() {
             Ok(self.version)
@@ -339,7 +339,7 @@ fn match_version(
     }
 
     // Now try to match with semver
-    let req_sem_ver = VersionReq::parse(&req_version).map_err(|_| Nope::CrateNotFound)?;
+    let req_sem_ver = VersionReq::parse(&req_version).map_err(|_| Nope::VersionNotFound)?;
 
     // we need to sort versions first
     let versions_sem = {
@@ -348,7 +348,14 @@ fn match_version(
         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).map_err(|_| Nope::InternalServerError)?,
+                Version::parse(&version.0).map_err(|_| {
+                    log::error!(
+                        "invalid semver in database for crate {}: {}",
+                        name,
+                        version.0
+                    );
+                    Nope::InternalServerError
+                })?,
                 version.1,
             ));
         }
@@ -377,7 +384,7 @@ fn match_version(
                 corrected_name,
                 version: MatchSemver::Semver((v.0.to_string(), v.1)),
             })
-            .ok_or(Nope::CrateNotFound);
+            .ok_or(Nope::VersionNotFound);
     }
 
     // Since we return with a CrateNotFound earlier if the db reply is empty,
@@ -598,14 +605,13 @@ mod test {
     }
 
     fn version(v: Option<&str>, db: &TestDatabase) -> Option<String> {
-        match_version(&mut db.conn(), "foo", v)
-            .ok()
-            .and_then(|version| {
-                version
-                    .assume_exact()
-                    .ok()
-                    .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/rustdoc.rs

@@ -282,33 +282,31 @@ pub fn rustdoc_html_server_handler(req: &mut Request) -> IronResult<Response> {
     rendering_time.step("match version");
 
     // Check the database for releases with the requested version while doing the following:
+    // * If no matching releases are found, return a 404 with the underlying error
+    // Then:
     // * If both the name and the version are an exact match, return the version of the crate.
     // * 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 = 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);
-                    }
+    let release_found = match_version(&mut conn, &name, url_version)
+        .map_err(|err| IronError::new(err, status::NotFound))?;
+
+    let version = match release_found.version {
+        MatchSemver::Exact((version, _)) => {
+            // Redirect when the requested crate name isn't correct
+            if let Some(name) = release_found.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);
         }
-        // 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");
@@ -563,6 +561,12 @@ pub fn badge_handler(req: &mut Request) -> IronResult<Response> {
                 return Ok(super::redirect(iron_url));
             }
 
+            Err(Nope::VersionNotFound) => BadgeOptions {
+                subject: "docs".to_owned(),
+                status: "version not found".to_owned(),
+                color: "#e05d44".to_owned(),
+            },
+
             Err(_) => BadgeOptions {
                 subject: "docs".to_owned(),
                 status: "no builds".to_owned(),