Auto merge of #1614 - integer32llc:split-version-routes, r=jtgeibel

Split the two uses of version::deprecated::show into separate fns

Discovered that /crates/:crate_id/:version is used in the show_version
test RequestHelper function, so it probably shouldn't be marked as
deprecated even though the frontend isn't using it.

Takes care of the FIXME comment.

Author: bors

src/controllers/version/deprecated.rs

@@ -7,12 +7,10 @@
 
 use crate::controllers::prelude::*;
 
-use crate::models::Version;
+use crate::models::{Crate, Version};
 use crate::schema::*;
 use crate::views::EncodableVersion;
 
-use super::version_and_crate;
-
 /// Handles the `GET /versions` route.
 pub fn index(req: &mut dyn Request) -> CargoResult<Response> {
     use diesel::dsl::any;
@@ -40,28 +38,18 @@ pub fn index(req: &mut dyn Request) -> CargoResult<Response> {
     Ok(req.json(&R { versions }))
 }
 
-/// Handles the `GET /versions/:version_id` and
-/// `GET /crates/:crate_id/:version` routes.
-///
-/// The frontend doesn't appear to hit either of these endpoints. Instead the
-/// version information appears to be returned by `krate::show`.
-///
-/// FIXME: These two routes have very different semantics and should be split into
-/// a separate function for each endpoint.
-pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
-    let (version, krate) = match req.params().find("crate_id") {
-        Some(..) => version_and_crate(req)?,
-        None => {
-            let id = &req.params()["version_id"];
-            let id = id.parse().unwrap_or(0);
-            let conn = req.db_conn()?;
-            versions::table
-                .find(id)
-                .inner_join(crates::table)
-                .select((versions::all_columns, crate::models::krate::ALL_COLUMNS))
-                .first(&*conn)?
-        }
-    };
+/// Handles the `GET /versions/:version_id` route.
+/// The frontend doesn't appear to hit this endpoint. Instead, the version information appears to
+/// be returned by `krate::show`.
+pub fn show_by_id(req: &mut dyn Request) -> CargoResult<Response> {
+    let id = &req.params()["version_id"];
+    let id = id.parse().unwrap_or(0);
+    let conn = req.db_conn()?;
+    let (version, krate): (Version, Crate) = versions::table
+        .find(id)
+        .inner_join(crates::table)
+        .select((versions::all_columns, crate::models::krate::ALL_COLUMNS))
+        .first(&*conn)?;
 
     #[derive(Serialize)]
     struct R {

src/controllers/version/metadata.rs

@@ -7,7 +7,7 @@
 use crate::controllers::prelude::*;
 
 use crate::schema::*;
-use crate::views::{EncodableDependency, EncodablePublicUser};
+use crate::views::{EncodableDependency, EncodablePublicUser, EncodableVersion};
 
 use super::version_and_crate;
 
@@ -61,3 +61,19 @@ pub fn authors(req: &mut dyn Request) -> CargoResult<Response> {
         meta: Meta { names },
     }))
 }
+
+/// Handles the `GET /crates/:crate/:version` route.
+///
+/// The frontend doesn't appear to hit this endpoint, but our tests do, and it seems to be a useful
+/// API route to have.
+pub fn show(req: &mut dyn Request) -> CargoResult<Response> {
+    let (version, krate) = version_and_crate(req)?;
+
+    #[derive(Serialize)]
+    struct R {
+        version: EncodableVersion,
+    }
+    Ok(req.json(&R {
+        version: version.encodable(&krate.name),
+    }))
+}

src/router.rs

@@ -32,11 +32,11 @@ pub fn build_router(app: &App) -> R404 {
 
     // Routes that appear to be unused
     api_router.get("/versions", C(version::deprecated::index));
-    api_router.get("/versions/:version_id", C(version::deprecated::show));
+    api_router.get("/versions/:version_id", C(version::deprecated::show_by_id));
 
     // Routes used by the frontend
     api_router.get("/crates/:crate_id", C(krate::metadata::show));
-    api_router.get("/crates/:crate_id/:version", C(version::deprecated::show));
+    api_router.get("/crates/:crate_id/:version", C(version::metadata::show));
     api_router.get(
         "/crates/:crate_id/:version/readme",
         C(krate::metadata::readme),