Follow the Diesel

Get it? Because this ports the following endpoints. To Diesel.
...Anyone?

This ports 4 endpoints over to Diesel. The 3 endpoints which manipulate
the `following` endpoint, as well as the `/me/updates` endpoint since it
is only hit by the tests for the following endpoints.

I ended up changing the updates endpoint quite a bit. I wanted to
eliminate the N+1 queries on the max version, and was wondering why we
needed the max version at all here. I went to go look at it in the UI,
and it turns out that the dashboard page which displayed it is actually
broken as well. After fixing it, I noticed that it doesn't need the
crates at all, just the name (which we tell Ember is the id).

I couldn't actually find a good way in Ember to reference the ID of an
association without loading the whole thing. If anybody knows a better
way to do it than what I'm doing here, please let me know.

Since we don't need the crates, I've just opted not to include that data
in the response body (note that just not including the max version is a
bad idea, since ember caches stuff and it could result in a page that
does need the max version displaying wrong later).

While I was touching these endpoints, I also went ahead and reduced them
all to a single query.

Fixes #438.

Author: sgrif

app/controllers/dashboard.js

@@ -38,9 +38,6 @@ export default Ember.Controller.extend({
             var page = (this.get('myFeed').length / 10) + 1;
 
             ajax(`/me/updates?page=${page}`).then((data) => {
-                data.crates.forEach(crate =>
-                    this.store.push(this.store.normalize('crate', crate)));
-
                 var versions = data.versions.map(version =>
                     this.store.push(this.store.normalize('version', version)));
 

app/models/version.js

@@ -1,4 +1,5 @@
 import DS from 'ember-data';
+import Ember from 'ember';
 
 export default DS.Model.extend({
     num: DS.attr('string'),
@@ -15,6 +16,10 @@ export default DS.Model.extend({
     dependencies: DS.hasMany('dependency', { async: true }),
     version_downloads: DS.hasMany('version-download', { async: true }),
 
+    crateName: Ember.computed('crate', function() {
+        return this.belongsTo('crate').id();
+    }),
+
     getDownloadUrl() {
         return this.store.adapterFor('version').getDownloadUrl(this.get('dl_path'));
     },

app/templates/dashboard.hbs

@@ -49,10 +49,12 @@
             {{#each myFeed as |version|}}
                 <div class='row'>
                     <div class='info'>
-                        {{link-to version.crate.name 'crate.version' version.num}}
+                        {{#link-to 'crate.version' version.crateName version.num}}
+                        {{ version.crateName }}
                         <span class='small'>{{ version.num }}</span>
+                        {{/link-to}}
                         <span class='date small'>
-                            {{from-now version.created_at}}
+                            {{moment-from-now version.created_at}}
                         </span>
                     </div>
                 </div>

src/krate.rs

@@ -4,9 +4,11 @@ use std::collections::HashMap;
 
 use conduit::{Request, Response};
 use conduit_router::RequestParams;
-use diesel::pg::PgConnection;
+use diesel::associations::Identifiable;
 use diesel::pg::upsert::*;
+use diesel::pg::{Pg, PgConnection};
 use diesel::prelude::*;
+use diesel;
 use diesel_full_text_search::*;
 use license_exprs;
 use pg::GenericConnection;
@@ -35,7 +37,8 @@ use util::{RequestUtils, CargoResult, internal, ChainError, human};
 use version::EncodableVersion;
 use {Model, User, Keyword, Version, Category, Badge, Replica};
 
-#[derive(Clone, Queryable, Identifiable, AsChangeset)]
+#[derive(Clone, Queryable, Identifiable, AsChangeset, Associations)]
+#[has_many(versions)]
 pub struct Crate {
     pub id: i32,
     pub name: String,
@@ -64,6 +67,8 @@ pub const ALL_COLUMNS: AllColumns = (crates::id, crates::name,
     crates::readme, crates::license, crates::repository,
     crates::max_upload_size);
 
+type CrateQuery<'a> = crates::BoxedQuery<'a, Pg, <AllColumns as Expression>::SqlType>;
+
 #[derive(RustcEncodable, RustcDecodable)]
 pub struct EncodableCrate {
     pub id: String,
@@ -224,6 +229,15 @@ impl<'a> NewCrate<'a> {
 }
 
 impl Crate {
+    pub fn by_name(name: &str) -> CrateQuery {
+        crates::table
+            .select(ALL_COLUMNS)
+            .filter(
+                canon_crate_name(crates::name).eq(
+                    canon_crate_name(name))
+            ).into_boxed()
+    }
+
     pub fn find_by_name(conn: &GenericConnection,
                         name: &str) -> CargoResult<Crate> {
         let stmt = conn.prepare("SELECT * FROM crates \
@@ -1093,44 +1107,61 @@ fn user_and_crate(req: &mut Request) -> CargoResult<(User, Crate)> {
     Ok((user.clone(), krate))
 }
 
+#[derive(Insertable, Queryable, Identifiable, Associations)]
+#[belongs_to(User)]
+#[primary_key(user_id, crate_id)]
+#[table_name="follows"]
+pub struct Follow {
+    user_id: i32,
+    crate_id: i32,
+}
+
+fn follow_target(req: &mut Request) -> CargoResult<Follow> {
+    let user = req.user()?;
+    let conn = req.db_conn()?;
+    let crate_name = &req.params()["crate_id"];
+    let crate_id = Crate::by_name(crate_name)
+        .select(crates::id)
+        .first(conn)?;
+    Ok(Follow {
+        user_id: user.id,
+        crate_id: crate_id,
+    })
+}
+
 /// Handles the `PUT /crates/:crate_id/follow` route.
 pub fn follow(req: &mut Request) -> CargoResult<Response> {
-    let (user, krate) = user_and_crate(req)?;
-    let tx = req.tx()?;
-    let stmt = tx.prepare("SELECT 1 FROM follows
-                                WHERE user_id = $1 AND crate_id = $2")?;
-    let rows = stmt.query(&[&user.id, &krate.id])?;
-    if !rows.iter().next().is_some() {
-        tx.execute("INSERT INTO follows (user_id, crate_id)
-                         VALUES ($1, $2)", &[&user.id, &krate.id])?;
-    }
+    let follow = follow_target(req)?;
+    let conn = req.db_conn()?;
+    diesel::insert(&follow.on_conflict_do_nothing())
+        .into(follows::table)
+        .execute(conn)?;
     #[derive(RustcEncodable)]
     struct R { ok: bool }
     Ok(req.json(&R { ok: true }))
 }
 
 /// Handles the `DELETE /crates/:crate_id/follow` route.
 pub fn unfollow(req: &mut Request) -> CargoResult<Response> {
-    let (user, krate) = user_and_crate(req)?;
-    let tx = req.tx()?;
-    tx.execute("DELETE FROM follows
-                     WHERE user_id = $1 AND crate_id = $2",
-               &[&user.id, &krate.id])?;
+    let follow = follow_target(req)?;
+    let conn = req.db_conn()?;
+    diesel::delete(&follow).execute(conn)?;
     #[derive(RustcEncodable)]
     struct R { ok: bool }
     Ok(req.json(&R { ok: true }))
 }
 
 /// Handles the `GET /crates/:crate_id/following` route.
 pub fn following(req: &mut Request) -> CargoResult<Response> {
-    let (user, krate) = user_and_crate(req)?;
-    let tx = req.tx()?;
-    let stmt = tx.prepare("SELECT 1 FROM follows
-                                WHERE user_id = $1 AND crate_id = $2")?;
-    let rows = stmt.query(&[&user.id, &krate.id])?;
+    use diesel::expression::dsl::exists;
+
+    let follow = follow_target(req)?;
+    let conn = req.db_conn()?;
+    let following = diesel::select(exists(follows::table.find(follow.id())))
+        .get_result(conn)?;
     #[derive(RustcEncodable)]
     struct R { following: bool }
-    Ok(req.json(&R { following: rows.iter().next().is_some() }))
+    Ok(req.json(&R { following: following }))
 }
 
 /// Handles the `GET /crates/:crate_id/versions` route.

src/tests/all.rs

@@ -1,12 +1,13 @@
 #![deny(warnings)]
 
+#[macro_use] extern crate diesel;
+#[macro_use] extern crate diesel_codegen;
 extern crate bufstream;
 extern crate cargo_registry;
 extern crate conduit;
 extern crate conduit_middleware;
 extern crate conduit_test;
 extern crate curl;
-extern crate diesel;
 extern crate dotenv;
 extern crate git2;
 extern crate postgres;

src/tests/krate.rs

@@ -769,63 +769,53 @@ fn dependencies() {
 
 #[test]
 fn following() {
-    // #[derive(RustcDecodable)] struct F { following: bool }
-    // #[derive(RustcDecodable)] struct O { ok: bool }
+    #[derive(RustcDecodable)] struct F { following: bool }
+    #[derive(RustcDecodable)] struct O { ok: bool }
 
     let (_b, app, middle) = ::app();
     let mut req = ::req(app.clone(), Method::Get, "/api/v1/crates/foo_following/following");
 
     let user;
-    let krate;
     {
         let conn = app.diesel_database.get().unwrap();
         user = ::new_user("foo").create_or_update(&conn).unwrap();
         ::sign_in_as(&mut req, &user);
-        krate = ::new_crate("foo_following").create_or_update(&conn, None, user.id).unwrap();
-
-        // FIXME: Go back to hitting the actual endpoint once it's using Diesel
-        conn
-            .execute(&format!("INSERT INTO follows (user_id, crate_id) VALUES ({}, {})",
-                              user.id, krate.id))
-            .unwrap();
+        ::new_crate("foo_following").create_or_update(&conn, None, user.id).unwrap();
     }
 
-    // let mut response = ok_resp!(middle.call(&mut req));
-    // assert!(!::json::<F>(&mut response).following);
+    let mut response = ok_resp!(middle.call(&mut req));
+    assert!(!::json::<F>(&mut response).following);
 
-    // req.with_path("/api/v1/crates/foo_following/follow")
-    //    .with_method(Method::Put);
-    // let mut response = ok_resp!(middle.call(&mut req));
-    // assert!(::json::<O>(&mut response).ok);
-    // let mut response = ok_resp!(middle.call(&mut req));
-    // assert!(::json::<O>(&mut response).ok);
+    req.with_path("/api/v1/crates/foo_following/follow")
+       .with_method(Method::Put);
+    let mut response = ok_resp!(middle.call(&mut req));
+    assert!(::json::<O>(&mut response).ok);
+    let mut response = ok_resp!(middle.call(&mut req));
+    assert!(::json::<O>(&mut response).ok);
 
-    // req.with_path("/api/v1/crates/foo_following/following")
-    //    .with_method(Method::Get);
-    // let mut response = ok_resp!(middle.call(&mut req));
-    // assert!(::json::<F>(&mut response).following);
+    req.with_path("/api/v1/crates/foo_following/following")
+       .with_method(Method::Get);
+    let mut response = ok_resp!(middle.call(&mut req));
+    assert!(::json::<F>(&mut response).following);
 
     req.with_path("/api/v1/crates")
-       .with_query("following=1");
+        .with_method(Method::Get)
+        .with_query("following=1");
     let mut response = ok_resp!(middle.call(&mut req));
     let l = ::json::<CrateList>(&mut response);
     assert_eq!(l.crates.len(), 1);
 
-    // FIXME: Go back to hitting the actual endpoint once it's using Diesel
-    req.db_conn().unwrap()
-        .execute("TRUNCATE TABLE follows")
-        .unwrap();
-    // req.with_path("/api/v1/crates/foo_following/follow")
-    //    .with_method(Method::Delete);
-    // let mut response = ok_resp!(middle.call(&mut req));
-    // assert!(::json::<O>(&mut response).ok);
-    // let mut response = ok_resp!(middle.call(&mut req));
-    // assert!(::json::<O>(&mut response).ok);
-
-    // req.with_path("/api/v1/crates/foo_following/following")
-    //    .with_method(Method::Get);
-    // let mut response = ok_resp!(middle.call(&mut req));
-    // assert!(!::json::<F>(&mut response).following);
+    req.with_path("/api/v1/crates/foo_following/follow")
+       .with_method(Method::Delete);
+    let mut response = ok_resp!(middle.call(&mut req));
+    assert!(::json::<O>(&mut response).ok);
+    let mut response = ok_resp!(middle.call(&mut req));
+    assert!(::json::<O>(&mut response).ok);
+
+    req.with_path("/api/v1/crates/foo_following/following")
+       .with_method(Method::Get);
+    let mut response = ok_resp!(middle.call(&mut req));
+    assert!(!::json::<F>(&mut response).following);
 
     req.with_path("/api/v1/crates")
        .with_query("following=1")

src/tests/user.rs

@@ -1,9 +1,12 @@
 use conduit::{Handler, Method};
+use diesel::prelude::*;
+use diesel::insert;
 
 use cargo_registry::Model;
+use cargo_registry::db::RequestTransaction;
 use cargo_registry::krate::EncodableCrate;
+use cargo_registry::schema::versions;
 use cargo_registry::user::{User, NewUser, EncodableUser};
-use cargo_registry::db::RequestTransaction;
 use cargo_registry::version::EncodableVersion;
 
 #[derive(RustcDecodable)]
@@ -139,10 +142,27 @@ fn following() {
     #[derive(RustcDecodable)] struct Meta { more: bool }
 
     let (_b, app, middle) = ::app();
-    let mut req = ::req(app, Method::Get, "/");
-    ::mock_user(&mut req, ::user("foo"));
-    ::mock_crate(&mut req, ::krate("foo_fighters"));
-    ::mock_crate(&mut req, ::krate("bar_fighters"));
+    let mut req = ::req(app.clone(), Method::Get, "/");
+    {
+        let conn = app.diesel_database.get().unwrap();
+        let user = ::new_user("foo").create_or_update(&conn).unwrap();
+        ::sign_in_as(&mut req, &user);
+        #[derive(Insertable)]
+        #[table_name="versions"]
+        struct NewVersion<'a> {
+            crate_id: i32,
+            num: &'a str,
+        }
+        let id1 = ::new_crate("foo_fighters").create_or_update(&conn, None, user.id)
+            .unwrap().id;
+        let id2 = ::new_crate("bar_fighters").create_or_update(&conn, None, user.id)
+            .unwrap().id;
+        let new_versions = vec![
+            NewVersion { crate_id: id1, num: "1.0.0" },
+            NewVersion { crate_id: id2, num: "1.0.0" },
+        ];
+        insert(&new_versions).into(versions::table).execute(&*conn).unwrap();
+    }
 
     let mut response = ok_resp!(middle.call(req.with_path("/me/updates")
                                                .with_method(Method::Get)));