Skip to content

Switch most of badges and keywords to use diesel #723

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 13, 2017
Merged

Switch most of badges and keywords to use diesel #723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d6e2658
Convert badges to use diesel everywhere
carols10cents May 12, 2017
170ec38
Include the `lower` sql fn at the top level
carols10cents May 12, 2017
defd894
Update keyword to use diesel
carols10cents May 13, 2017
4c2bd42
Lowercase keywords before inserting
carols10cents May 13, 2017
a68154d
Remove Keyword functions that are no longer used
carols10cents May 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 0 additions & 47 deletions src/badge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
use Model;
use krate::Crate;
use schema::badges;
use util::CargoResult;

use diesel::pg::{Pg, PgConnection};
use diesel::prelude::*;
use pg::GenericConnection;
use pg::rows::Row;
use serde_json;
use std::collections::HashMap;

Expand Down Expand Up @@ -43,17 +40,6 @@ impl Queryable<badges::SqlType, Pg> for Badge {
}
}

impl Model for Badge {
fn from_row(row: &Row) -> Badge {
let badge_type: String = row.get("badge_type");
let attributes: serde_json::Value = row.get("attributes");
let json = json!({"badge_type": badge_type, "attributes": attributes});
serde_json::from_value(json)
.expect("Invalid CI badge in the database")
}
fn table_name(_: Option<Badge>) -> &'static str { "badges" }
}

impl Badge {
pub fn encodable(self) -> EncodableBadge {
serde_json::from_value(serde_json::to_value(self).unwrap()).unwrap()
Expand Down Expand Up @@ -107,37 +93,4 @@ impl Badge {
Ok(invalid_badges)
})
}

pub fn update_crate_old(conn: &GenericConnection,
krate: &Crate,
badges: HashMap<String, HashMap<String, String>>)
-> CargoResult<Vec<String>> {

let mut invalid_badges = vec![];

let badges: Vec<Badge> = badges.into_iter().filter_map(|(k, v)| {
let json = json!({"badge_type": k, "attributes": v});
serde_json::from_value(json)
.map_err(|_| invalid_badges.push(k))
.ok()
}).collect();

conn.execute("\
DELETE FROM badges \
WHERE crate_id = $1;",
&[&krate.id]
)?;

for badge in badges {
let json = serde_json::to_value(badge)?;
conn.execute("\
INSERT INTO badges (crate_id, badge_type, attributes) \
VALUES ($1, $2, $3) \
ON CONFLICT (crate_id, badge_type) DO UPDATE \
SET attributes = EXCLUDED.attributes;",
&[&krate.id, &json["badge_type"].as_str(), &json["attributes"]]
)?;
}
Ok(invalid_badges)
}
}
144 changes: 41 additions & 103 deletions src/keyword.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,17 @@
use std::ascii::AsciiExt;
use std::collections::HashMap;
use time::Timespec;

use conduit::{Request, Response};
use conduit_router::RequestParams;
use diesel::pg::PgConnection;
use diesel::prelude::*;
use diesel;
use pg::GenericConnection;
use pg::rows::Row;

use {Model, Crate};
use db::RequestTransaction;
use schema::*;
use util::{RequestUtils, CargoResult, ChainError, internal};
use util::errors::NotFound;
use util::{RequestUtils, CargoResult};

#[derive(Clone, Identifiable, Queryable)]
pub struct Keyword {
Expand Down Expand Up @@ -43,12 +40,11 @@ pub struct EncodableKeyword {
}

impl Keyword {
pub fn find_by_keyword(conn: &GenericConnection, name: &str)
-> CargoResult<Option<Keyword>> {
let stmt = conn.prepare("SELECT * FROM keywords \
WHERE keyword = LOWER($1)")?;
let rows = stmt.query(&[&name])?;
Ok(rows.iter().next().map(|r| Model::from_row(&r)))
pub fn find_by_keyword(conn: &PgConnection, name: &str)
-> QueryResult<Keyword> {
keywords::table
.filter(keywords::keyword.eq(::lower(name)))
.first(&*conn)
}

pub fn find_or_create_all(conn: &PgConnection, names: &[&str]) -> QueryResult<Vec<Keyword>> {
Expand All @@ -60,58 +56,26 @@ impl Keyword {
struct NewKeyword<'a> {
keyword: &'a str,
}
sql_function!(lower, lower_t, (x: ::diesel::types::Text) -> ::diesel::types::Text);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to keep these definitions close to where they're used for functions which can have varying signatures, but I don't feel strongly about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don't think lower is going to have a varying signature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It varies due to nullability (which is why I don't expose it in Diesel proper)


let (lowercase_names, new_keywords): (Vec<_>, Vec<_>) = names.iter()
.map(|s| (s.to_lowercase(), NewKeyword { keyword: *s }))
.unzip();
let lowercase_names: Vec<_> = names.iter()
.map(|s| s.to_lowercase())
.collect();

let new_keywords: Vec<_> = lowercase_names.iter()
.map(|s| NewKeyword { keyword: s })
.collect();

// https://github.com/diesel-rs/diesel/issues/797
if !new_keywords.is_empty() {
diesel::insert(&new_keywords.on_conflict_do_nothing()).into(keywords::table)
diesel::insert(&new_keywords.on_conflict_do_nothing())
.into(keywords::table)
.execute(conn)?;
}
keywords::table.filter(lower(keywords::keyword).eq(any(lowercase_names)))
keywords::table
.filter(::lower(keywords::keyword).eq(any(&lowercase_names)))
.load(conn)
}

pub fn find_or_insert(conn: &GenericConnection, name: &str)
-> CargoResult<Keyword> {
// TODO: racy (the select then insert is not atomic)
let stmt = conn.prepare("SELECT * FROM keywords
WHERE keyword = LOWER($1)")?;
for row in stmt.query(&[&name])?.iter() {
return Ok(Model::from_row(&row))
}

let stmt = conn.prepare("INSERT INTO keywords (keyword) VALUES (LOWER($1))
RETURNING *")?;
let rows = stmt.query(&[&name])?;
Ok(Model::from_row(&rows.iter().next().chain_error(|| {
internal("no version returned")
})?))
}

pub fn all(conn: &GenericConnection, sort: &str, limit: i64, offset: i64)
-> CargoResult<Vec<Keyword>> {

let sort_sql = match sort {
"crates" => "ORDER BY crates_cnt DESC",
_ => "ORDER BY keyword ASC",
};

let stmt = conn.prepare(&format!("SELECT * FROM keywords {}
LIMIT $1 OFFSET $2",
sort_sql))?;

let keywords: Vec<_> = stmt.query(&[&limit, &offset])?
.iter()
.map(|row| Model::from_row(&row))
.collect();

Ok(keywords)
}

pub fn valid_name(name: &str) -> bool {
if name.is_empty() { return false }
name.chars().next().unwrap().is_alphanumeric() &&
Expand Down Expand Up @@ -144,47 +108,6 @@ impl Keyword {
Ok(())
})
}

pub fn update_crate_old(conn: &GenericConnection,
krate: &Crate,
keywords: &[String]) -> CargoResult<()> {
let old_kws = krate.keywords(conn)?;
let old_kws = old_kws.iter().map(|kw| {
(&kw.keyword[..], kw)
}).collect::<HashMap<_, _>>();
let new_kws = keywords.iter().map(|k| {
let kw = Keyword::find_or_insert(conn, k)?;
Ok((k.as_str(), kw))
}).collect::<CargoResult<HashMap<_, _>>>()?;

let to_rm = old_kws.iter().filter(|&(kw, _)| {
!new_kws.contains_key(kw)
}).map(|(_, v)| v.id).collect::<Vec<_>>();
let to_add = new_kws.iter().filter(|&(kw, _)| {
!old_kws.contains_key(kw)
}).map(|(_, v)| v.id).collect::<Vec<_>>();

if !to_rm.is_empty() {
conn.execute("DELETE FROM crates_keywords
WHERE keyword_id = ANY($1)
AND crate_id = $2",
&[&to_rm, &krate.id])?;
}

if !to_add.is_empty() {
let insert = to_add.iter().map(|id| {
let crate_id: i32 = krate.id;
let id: i32 = *id;
format!("({}, {})", crate_id, id)
}).collect::<Vec<_>>().join(", ");
conn.execute(&format!("INSERT INTO crates_keywords
(crate_id, keyword_id) VALUES {}",
insert),
&[])?;
}

Ok(())
}
}

impl Model for Keyword {
Expand All @@ -201,34 +124,49 @@ impl Model for Keyword {

/// Handles the `GET /keywords` route.
pub fn index(req: &mut Request) -> CargoResult<Response> {
let conn = req.tx()?;
use diesel::expression::dsl::sql;
use diesel::types::BigInt;
use schema::keywords;

let conn = req.db_conn()?;
let (offset, limit) = req.pagination(10, 100)?;
let query = req.query();
let sort = query.get("sort").map(|s| &s[..]).unwrap_or("alpha");

let keywords = Keyword::all(conn, sort, limit, offset)?;
let keywords = keywords.into_iter().map(Keyword::encodable).collect();
let mut query = keywords::table
.select((keywords::all_columns, sql::<BigInt>("COUNT(*) OVER ()")))
.limit(limit)
.offset(offset)
.into_boxed();

// Query for the total count of keywords
let total = Keyword::count(conn)?;
if sort == "crates" {
query = query.order(keywords::crates_cnt.desc());
} else {
query = query.order(keywords::keyword.asc());
}

let data = query.load::<(Keyword, i64)>(&*conn)?;
let total = data.get(0).map(|&(_, t)| t).unwrap_or(0);
let kws = data.into_iter()
.map(|(k, _)| k.encodable()).collect::<Vec<_>>();

#[derive(RustcEncodable)]
struct R { keywords: Vec<EncodableKeyword>, meta: Meta }
#[derive(RustcEncodable)]
struct Meta { total: i64 }

Ok(req.json(&R {
keywords: keywords,
keywords: kws,
meta: Meta { total: total },
}))
}

/// Handles the `GET /keywords/:keyword_id` route.
pub fn show(req: &mut Request) -> CargoResult<Response> {
let name = &req.params()["keyword_id"];
let conn = req.tx()?;
let kw = Keyword::find_by_keyword(conn, name)?;
let kw = kw.chain_error(|| NotFound)?;
let conn = req.db_conn()?;

let kw = Keyword::find_by_keyword(&conn, name)?;

#[derive(RustcEncodable)]
struct R { keyword: EncodableKeyword }
Expand Down
10 changes: 3 additions & 7 deletions src/krate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,11 +609,8 @@ impl Crate {
Ok(rows.iter().map(|r| Model::from_row(&r)).collect())
}

pub fn badges(&self, conn: &GenericConnection) -> CargoResult<Vec<Badge>> {
let stmt = conn.prepare("SELECT badges.* from badges \
WHERE badges.crate_id = $1")?;
let rows = stmt.query(&[&self.id])?;
Ok(rows.iter().map(|r| Model::from_row(&r)).collect())
pub fn badges(&self, conn: &PgConnection) -> QueryResult<Vec<Badge>> {
badges::table.filter(badges::crate_id.eq(self.id)).load(conn)
}

/// Returns (dependency, dependent crate name, dependent crate downloads)
Expand Down Expand Up @@ -709,7 +706,7 @@ pub fn index(req: &mut Request) -> CargoResult<Response> {
query = query.filter(crates::id.eq_any(
crates_keywords::table.select(crates_keywords::crate_id)
.inner_join(keywords::table)
.filter(lower(keywords::keyword).eq(lower(kw)))
.filter(::lower(keywords::keyword).eq(::lower(kw)))
));
} else if let Some(letter) = params.get("letter") {
let pattern = format!("{}%", letter.chars().next().unwrap()
Expand Down Expand Up @@ -1303,4 +1300,3 @@ pub fn reverse_dependencies(req: &mut Request) -> CargoResult<Response> {

use diesel::types::Text;
sql_function!(canon_crate_name, canon_crate_name_t, (x: Text) -> Text);
sql_function!(lower, lower_t, (x: Text) -> Text);
2 changes: 2 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,3 +227,5 @@ pub fn env(s: &str) -> String {
panic!("must have `{}` defined", s)
})
}

sql_function!(lower, lower_t, (x: ::diesel::types::Text) -> ::diesel::types::Text);
6 changes: 1 addition & 5 deletions src/tests/all.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ use cargo_registry::krate::NewCrate;
use cargo_registry::upload as u;
use cargo_registry::user::NewUser;
use cargo_registry::version::NewVersion;
use cargo_registry::{User, Crate, Version, Keyword, Dependency, Category, Model, Replica};
use cargo_registry::{User, Crate, Version, Dependency, Category, Model, Replica};
use conduit::{Request, Method};
use conduit_test::MockRequest;
use diesel::pg::PgConnection;
Expand Down Expand Up @@ -296,10 +296,6 @@ fn mock_dep(req: &mut Request, version: &Version, krate: &Crate,
&target.map(|s| s.to_string())).unwrap()
}

fn mock_keyword(req: &mut Request, name: &str) -> Keyword {
Keyword::find_or_insert(req.tx().unwrap(), name).unwrap()
}

fn new_category<'a>(category: &'a str, slug: &'a str) -> NewCategory<'a> {
NewCategory { category: category, slug: slug, ..NewCategory::default() }
}
Expand Down
Loading