From c10cf87be3e8084cb90fd335ddd5f89c91f890ab Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Sun, 12 Mar 2017 07:01:22 -0400 Subject: [PATCH] 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. --- app/controllers/dashboard.js | 3 -- app/models/version.js | 5 +++ app/templates/dashboard.hbs | 6 ++- src/krate.rs | 75 +++++++++++++++++++++++---------- src/tests/all.rs | 3 +- src/tests/krate.rs | 66 +++++++++++++---------------- src/tests/user.rs | 30 +++++++++++--- src/user/mod.rs | 80 ++++++++++++++---------------------- 8 files changed, 148 insertions(+), 120 deletions(-) diff --git a/app/controllers/dashboard.js b/app/controllers/dashboard.js index 3708b4c7866..0396f21c613 100644 --- a/app/controllers/dashboard.js +++ b/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))); diff --git a/app/models/version.js b/app/models/version.js index 0876d3cdf89..5f6a7343ef8 100644 --- a/app/models/version.js +++ b/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')); }, diff --git a/app/templates/dashboard.hbs b/app/templates/dashboard.hbs index f0b074e6f3d..0f0ed93f2ee 100644 --- a/app/templates/dashboard.hbs +++ b/app/templates/dashboard.hbs @@ -49,10 +49,12 @@ {{#each myFeed as |version|}}
- {{link-to version.crate.name 'crate.version' version.num}} + {{#link-to 'crate.version' version.crateName version.num}} + {{ version.crateName }} {{ version.num }} + {{/link-to}} - {{from-now version.created_at}} + {{moment-from-now version.created_at}}
diff --git a/src/krate.rs b/src/krate.rs index 20e2c959e50..7f973c790a0 100644 --- a/src/krate.rs +++ b/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, ::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 { let stmt = conn.prepare("SELECT * FROM crates \ @@ -1093,17 +1107,35 @@ 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 { + 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 { - 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 })) @@ -1111,11 +1143,9 @@ pub fn follow(req: &mut Request) -> CargoResult { /// Handles the `DELETE /crates/:crate_id/follow` route. pub fn unfollow(req: &mut Request) -> CargoResult { - 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 })) @@ -1123,14 +1153,15 @@ pub fn unfollow(req: &mut Request) -> CargoResult { /// Handles the `GET /crates/:crate_id/following` route. pub fn following(req: &mut Request) -> CargoResult { - 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. diff --git a/src/tests/all.rs b/src/tests/all.rs index 7e2375ab64e..06dedadf8f9 100755 --- a/src/tests/all.rs +++ b/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; diff --git a/src/tests/krate.rs b/src/tests/krate.rs index 83a8cb6088f..57c9e9cf719 100644 --- a/src/tests/krate.rs +++ b/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::(&mut response).following); + let mut response = ok_resp!(middle.call(&mut req)); + assert!(!::json::(&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::(&mut response).ok); - // let mut response = ok_resp!(middle.call(&mut req)); - // assert!(::json::(&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::(&mut response).ok); + let mut response = ok_resp!(middle.call(&mut req)); + assert!(::json::(&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::(&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::(&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::(&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::(&mut response).ok); - // let mut response = ok_resp!(middle.call(&mut req)); - // assert!(::json::(&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::(&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::(&mut response).ok); + let mut response = ok_resp!(middle.call(&mut req)); + assert!(::json::(&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::(&mut response).following); req.with_path("/api/v1/crates") .with_query("following=1") diff --git a/src/tests/user.rs b/src/tests/user.rs index eb75abf93dc..47a15141244 100644 --- a/src/tests/user.rs +++ b/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))); diff --git a/src/user/mod.rs b/src/user/mod.rs index 3fc9cfc19df..52c3d314395 100644 --- a/src/user/mod.rs +++ b/src/user/mod.rs @@ -1,4 +1,3 @@ -use std::collections::HashMap; use conduit::{Request, Response}; use conduit_cookie::{RequestSession}; @@ -11,19 +10,19 @@ use rand::{thread_rng, Rng}; use app::RequestApp; use db::RequestTransaction; -use {http, Model, Version}; -use krate::{Crate, EncodableCrate}; -use schema::users; +use krate::Follow; +use schema::*; use util::errors::NotFound; use util::{RequestUtils, CargoResult, internal, ChainError, human}; use version::EncodableVersion; +use {http, Model, Version}; pub use self::middleware::{Middleware, RequestUser}; pub mod middleware; /// The model representing a row in the `users` database table. -#[derive(Clone, Debug, PartialEq, Eq, Queryable)] +#[derive(Clone, Debug, PartialEq, Eq, Queryable, Identifiable)] pub struct User { pub id: i32, pub email: Option, @@ -356,62 +355,45 @@ pub fn show(req: &mut Request) -> CargoResult { /// Handles the `GET /me/updates` route. pub fn updates(req: &mut Request) -> CargoResult { + use diesel::expression::dsl::{any, sql}; + use diesel::types::BigInt; + let user = req.user()?; let (offset, limit) = req.pagination(10, 100)?; - let tx = req.tx()?; - let sql = "SELECT versions.* FROM versions - INNER JOIN follows - ON follows.user_id = $1 AND - follows.crate_id = versions.crate_id - ORDER BY versions.created_at DESC OFFSET $2 LIMIT $3"; - - // Load all versions - let stmt = tx.prepare(sql)?; - let mut versions = Vec::new(); - let mut crate_ids = Vec::new(); - for row in stmt.query(&[&user.id, &offset, &limit])?.iter() { - let version: Version = Model::from_row(&row); - crate_ids.push(version.crate_id); - versions.push(version); - } - - // Load all crates - let mut map = HashMap::new(); - let mut crates = Vec::new(); - if crate_ids.len() > 0 { - let stmt = tx.prepare("SELECT * FROM crates WHERE id = ANY($1)")?; - for row in stmt.query(&[&crate_ids])?.iter() { - let krate: Crate = Model::from_row(&row); - map.insert(krate.id, krate.name.clone()); - crates.push(krate); - } - } + let conn = req.db_conn()?; - // Encode everything! - let crates = crates.into_iter().map(|c| { - let max_version = c.max_version(tx)?; - Ok(c.minimal_encodable(max_version, None)) - }).collect::>()?; - let versions = versions.into_iter().map(|v| { - let id = v.crate_id; - v.encodable(&map[&id]) + let followed_crates = Follow::belonging_to(user) + .select(follows::crate_id); + let data = versions::table + .select(versions::id) // FIXME: Remove this line when upgraded to Diesel 0.12 + .inner_join(crates::table) + .filter(crates::id.eq(any(followed_crates))) + .order(versions::created_at.desc()) + .limit(limit) + .offset(offset) + .select(( + versions::all_columns, + crates::name, + sql::("COUNT(*) OVER ()"), + )) + .load::<(Version, String, i64)>(conn)?; + + let more = data.get(0) + .map(|&(_, _, count)| count > offset + limit) + .unwrap_or(false); + + let versions = data.into_iter().map(|(version, crate_name, _)| { + version.encodable(&crate_name) }).collect(); - // Check if we have another - let sql = format!("SELECT 1 WHERE EXISTS({})", sql); - let stmt = tx.prepare(&sql)?; - let more = stmt.query(&[&user.id, &(offset + limit), &limit])? - .iter().next().is_some(); - #[derive(RustcEncodable)] struct R { versions: Vec, - crates: Vec, meta: Meta, } #[derive(RustcEncodable)] struct Meta { more: bool } - Ok(req.json(&R{ versions: versions, crates: crates, meta: Meta { more: more } })) + Ok(req.json(&R{ versions: versions, meta: Meta { more: more } })) } #[cfg(test)]