Skip to content

Commit f973a15

Browse files
sgrifsgrif
committed
Remove mostly unused keywords column from crates table
This column was present both in the table and in the Rust model, but was only ever used to update the search index on update. The actual source of keyword info is the crates_keywords table, and it's actually reasonably easy for this column to get out of sync. Instead of relying on things calling the `update_crate` function every time the keywords column is modified, we can instead populate the column with a subselect which grabs the keywords associated. This also means we need an additional trigger to touch the crate whenever the crates_keywords table is modified.
1 parent 3f2a813 commit f973a15

File tree

6 files changed

+121
-55
lines changed

6 files changed

+121
-55
lines changed

‎src/bin/migrate.rs

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -602,7 +602,7 @@ fn migrations() -> Vec<Migration> {
602602
try!(tx.batch_execute("
603603
DROP TRIGGER trigger_crates_set_updated_at ON crates;
604604
DROP TRIGGER trigger_versions_set_updated_at ON versions;
605-
DROP FUNCTION set_updated_at_ignore_downloads;
605+
DROP FUNCTION set_updated_at_ignore_downloads();
606606
CREATE TRIGGER trigger_crates_set_updated_at BEFORE UPDATE
607607
ON crates
608608
FOR EACH ROW EXECUTE PROCEDURE set_updated_at();
@@ -613,6 +613,75 @@ fn migrations() -> Vec<Migration> {
613613
"));
614614
Ok(())
615615
}),
616+
Migration::new(20160219125609, |tx| {
617+
tx.batch_execute("
618+
ALTER TABLE crates DROP COLUMN keywords;
619+
CREATE OR REPLACE FUNCTION trigger_crates_name_search() RETURNS trigger AS $$
620+
DECLARE kws TEXT;
621+
begin
622+
SELECT array_to_string(array_agg(keyword), ',') INTO kws
623+
FROM keywords INNER JOIN crates_keywords
624+
ON keywords.id = crates_keywords.keyword_id
625+
WHERE crates_keywords.crate_id = new.id;
626+
new.textsearchable_index_col :=
627+
setweight(to_tsvector('pg_catalog.english',
628+
coalesce(new.name, '')), 'A') ||
629+
setweight(to_tsvector('pg_catalog.english',
630+
coalesce(kws, '')), 'B') ||
631+
setweight(to_tsvector('pg_catalog.english',
632+
coalesce(new.description, '')), 'C') ||
633+
setweight(to_tsvector('pg_catalog.english',
634+
coalesce(new.readme, '')), 'D');
635+
return new;
636+
end
637+
$$ LANGUAGE plpgsql;
638+
CREATE OR REPLACE FUNCTION touch_crate() RETURNS trigger AS $$
639+
BEGIN
640+
IF TG_OP = 'DELETE' THEN
641+
UPDATE crates SET updated_at = CURRENT_TIMESTAMP WHERE
642+
id = OLD.crate_id;
643+
RETURN OLD;
644+
ELSE
645+
UPDATE crates SET updated_at = CURRENT_TIMESTAMP WHERE
646+
id = NEW.crate_id;
647+
RETURN NEW;
648+
END IF;
649+
END
650+
$$ LANGUAGE plpgsql;
651+
CREATE TRIGGER touch_crate_on_modify_keywords
652+
AFTER INSERT OR DELETE ON crates_keywords
653+
FOR EACH ROW
654+
EXECUTE PROCEDURE touch_crate();
655+
")
656+
}, |tx| {
657+
tx.batch_execute("
658+
ALTER TABLE crates ADD COLUMN keywords VARCHAR;
659+
CREATE OR REPLACE FUNCTION trigger_crates_name_search() RETURNS trigger AS $$
660+
begin
661+
new.textsearchable_index_col :=
662+
setweight(to_tsvector('pg_catalog.english',
663+
coalesce(new.name, '')), 'A') ||
664+
setweight(to_tsvector('pg_catalog.english',
665+
coalesce(new.keywords, '')), 'B') ||
666+
setweight(to_tsvector('pg_catalog.english',
667+
coalesce(new.description, '')), 'C') ||
668+
setweight(to_tsvector('pg_catalog.english',
669+
coalesce(new.readme, '')), 'D');
670+
return new;
671+
end
672+
$$ LANGUAGE plpgsql;
673+
674+
UPDATE crates SET keywords = (
675+
SELECT array_to_string(array_agg(keyword), ',')
676+
FROM keywords INNER JOIN crates_keywords
677+
ON keywords.id = crates_keywords.keyword_id
678+
WHERE crates_keywords.crate_id = crates.id
679+
);
680+
681+
DROP TRIGGER touch_crate_on_modify_keywords ON crates_keywords;
682+
DROP FUNCTION touch_crate();
683+
")
684+
}),
616685
];
617686
// NOTE: Generate a new id via `date +"%Y%m%d%H%M%S"`
618687

‎src/bin/update-downloads.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ mod test {
167167
let tx = conn.transaction().unwrap();
168168
let user = user(&tx);
169169
let krate = Crate::find_or_insert(&tx, "foo", user.id, &None, &None,
170-
&None, &None, &[], &None, &None,
170+
&None, &None, &None, &None,
171171
&None, None).unwrap();
172172
let version = Version::insert(&tx, krate.id,
173173
&semver::Version::parse("1.0.0").unwrap(),
@@ -194,7 +194,7 @@ mod test {
194194
let tx = conn.transaction().unwrap();
195195
let user = user(&tx);
196196
let krate = Crate::find_or_insert(&tx, "foo", user.id, &None,
197-
&None, &None, &None, &[], &None,
197+
&None, &None, &None, &None,
198198
&None, &None, None).unwrap();
199199
let version = Version::insert(&tx, krate.id,
200200
&semver::Version::parse("1.0.0").unwrap(),
@@ -217,7 +217,7 @@ mod test {
217217
let tx = conn.transaction().unwrap();
218218
let user = user(&tx);
219219
let krate = Crate::find_or_insert(&tx, "foo", user.id, &None,
220-
&None, &None, &None, &[], &None,
220+
&None, &None, &None, &None,
221221
&None, &None, None).unwrap();
222222
let version = Version::insert(&tx, krate.id,
223223
&semver::Version::parse("1.0.0").unwrap(),
@@ -241,7 +241,7 @@ mod test {
241241
let tx = conn.transaction().unwrap();
242242
let user = user(&tx);
243243
let krate = Crate::find_or_insert(&tx, "foo", user.id, &None,
244-
&None, &None, &None, &[], &None,
244+
&None, &None, &None, &None,
245245
&None, &None, None).unwrap();
246246
let version = Version::insert(&tx, krate.id,
247247
&semver::Version::parse("1.0.0").unwrap(),
@@ -281,7 +281,7 @@ mod test {
281281
let tx = conn.transaction().unwrap();
282282
let user = user(&tx);
283283
let krate = Crate::find_or_insert(&tx, "foo", user.id, &None,
284-
&None, &None, &None, &[], &None,
284+
&None, &None, &None, &None,
285285
&None, &None, None).unwrap();
286286
let version = Version::insert(&tx, krate.id,
287287
&semver::Version::parse("1.0.0").unwrap(),

‎src/krate.rs

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@ pub struct Crate {
4848
pub homepage: Option<String>,
4949
pub documentation: Option<String>,
5050
pub readme: Option<String>,
51-
pub keywords: Vec<String>,
5251
pub license: Option<String>,
5352
pub repository: Option<String>,
5453
pub max_upload_size: Option<i32>,
@@ -66,7 +65,6 @@ pub struct EncodableCrate {
6665
pub description: Option<String>,
6766
pub homepage: Option<String>,
6867
pub documentation: Option<String>,
69-
pub keywords: Vec<String>,
7068
pub license: Option<String>,
7169
pub repository: Option<String>,
7270
pub links: CrateLinks,
@@ -99,7 +97,6 @@ impl Crate {
9997
homepage: &Option<String>,
10098
documentation: &Option<String>,
10199
readme: &Option<String>,
102-
keywords: &[String],
103100
repository: &Option<String>,
104101
license: &Option<String>,
105102
license_file: &Option<String>,
@@ -112,7 +109,6 @@ impl Crate {
112109
let repository = repository.as_ref().map(|s| &s[..]);
113110
let mut license = license.as_ref().map(|s| &s[..]);
114111
let license_file = license_file.as_ref().map(|s| &s[..]);
115-
let keywords = keywords.join(",");
116112
try!(validate_url(homepage, "homepage"));
117113
try!(validate_url(documentation, "documentation"));
118114
try!(validate_url(repository, "repository"));
@@ -138,14 +134,13 @@ impl Crate {
138134
homepage = $2,
139135
description = $3,
140136
readme = $4,
141-
keywords = $5,
142-
license = $6,
143-
repository = $7
137+
license = $5,
138+
repository = $6
144139
WHERE canon_crate_name(name) =
145-
canon_crate_name($8)
140+
canon_crate_name($7)
146141
RETURNING *"));
147142
let rows = try!(stmt.query(&[&documentation, &homepage,
148-
&description, &readme, &keywords,
143+
&description, &readme,
149144
&license, &repository,
150145
&name]));
151146
match rows.iter().next() {
@@ -162,13 +157,13 @@ impl Crate {
162157

163158
let stmt = try!(conn.prepare("INSERT INTO crates
164159
(name, user_id, description, homepage,
165-
documentation, readme, keywords,
160+
documentation, readme,
166161
repository, license, max_upload_size)
167-
VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10)
162+
VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)
168163
RETURNING *"));
169164
let rows = try!(stmt.query(&[&name, &user_id,
170165
&description, &homepage,
171-
&documentation, &readme, &keywords,
166+
&documentation, &readme,
172167
&repository, &license, &max_upload_size]));
173168
let ret: Crate = Model::from_row(&try!(rows.iter().next().chain_error(|| {
174169
internal("no crate returned")
@@ -239,7 +234,7 @@ impl Crate {
239234
pub fn encodable(self, versions: Option<Vec<i32>>) -> EncodableCrate {
240235
let Crate {
241236
name, created_at, updated_at, downloads, max_version, description,
242-
homepage, documentation, keywords, license, repository,
237+
homepage, documentation, license, repository,
243238
readme: _, id: _, user_id: _, max_upload_size: _,
244239
} = self;
245240
let versions_link = match versions {
@@ -257,7 +252,6 @@ impl Crate {
257252
documentation: documentation,
258253
homepage: homepage,
259254
description: description,
260-
keywords: keywords,
261255
license: license,
262256
repository: repository,
263257
links: CrateLinks {
@@ -431,7 +425,6 @@ impl Crate {
431425
impl Model for Crate {
432426
fn from_row(row: &Row) -> Crate {
433427
let max: String = row.get("max_version");
434-
let kws: Option<String> = row.get("keywords");
435428
Crate {
436429
id: row.get("id"),
437430
name: row.get("name"),
@@ -444,9 +437,6 @@ impl Model for Crate {
444437
homepage: row.get("homepage"),
445438
readme: row.get("readme"),
446439
max_version: semver::Version::parse(&max).unwrap(),
447-
keywords: kws.unwrap_or(String::new()).split(',')
448-
.filter(|s| !s.is_empty())
449-
.map(|s| s.to_string()).collect(),
450440
license: row.get("license"),
451441
repository: row.get("repository"),
452442
max_upload_size: row.get("max_upload_size"),
@@ -666,7 +656,6 @@ pub fn new(req: &mut Request) -> CargoResult<Response> {
666656
&new_crate.homepage,
667657
&new_crate.documentation,
668658
&new_crate.readme,
669-
&keywords,
670659
&new_crate.repository,
671660
&new_crate.license,
672661
&new_crate.license_file,

‎src/tests/all.rs

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,6 @@ fn krate(name: &str) -> Crate {
191191
homepage: None,
192192
description: None,
193193
readme: None,
194-
keywords: Vec::new(),
195194
license: None,
196195
repository: None,
197196
max_upload_size: None,
@@ -222,13 +221,10 @@ fn mock_crate_vers(req: &mut Request, krate: Crate, v: &semver::Version)
222221
&krate.homepage,
223222
&krate.documentation,
224223
&krate.readme,
225-
&krate.keywords,
226224
&krate.repository,
227225
&krate.license,
228226
&None,
229227
krate.max_upload_size).unwrap();
230-
Keyword::update_crate(req.tx().unwrap(), &krate,
231-
&krate.keywords).unwrap();
232228
let v = krate.add_version(req.tx().unwrap(), v, &HashMap::new(), &[]);
233229
(krate, v.unwrap())
234230
}
@@ -260,13 +256,20 @@ fn new_req(app: Arc<App>, krate: &str, version: &str) -> MockRequest {
260256
fn new_req_full(app: Arc<App>, krate: Crate, version: &str,
261257
deps: Vec<u::CrateDependency>) -> MockRequest {
262258
let mut req = ::req(app, Method::Put, "/api/v1/crates/new");
263-
req.with_body(&new_req_body(krate, version, deps));
259+
req.with_body(&new_req_body(krate, version, deps, Vec::new()));
264260
return req;
265261
}
266262

267-
fn new_req_body(krate: Crate, version: &str, deps: Vec<u::CrateDependency>)
268-
-> Vec<u8> {
269-
let kws = krate.keywords.into_iter().map(u::Keyword).collect();
263+
fn new_req_with_keywords(app: Arc<App>, krate: Crate, version: &str,
264+
kws: Vec<String>) -> MockRequest {
265+
let mut req = ::req(app, Method::Put, "/api/v1/crates/new");
266+
req.with_body(&new_req_body(krate, version, Vec::new(), kws));
267+
return req;
268+
}
269+
270+
fn new_req_body(krate: Crate, version: &str, deps: Vec<u::CrateDependency>,
271+
kws: Vec<String>) -> Vec<u8> {
272+
let kws = kws.into_iter().map(u::Keyword).collect();
270273
new_crate_to_body(&u::NewCrate {
271274
name: u::CrateName(krate.name),
272275
vers: u::CrateVersion(semver::Version::parse(version).unwrap()),

‎src/tests/krate.rs

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,14 @@ use std::fs::{self, File};
44

55
use conduit::{Handler, Request, Method};
66
use git2;
7+
use postgres::GenericConnection;
78
use rustc_serialize::{json, Decoder};
89
use semver;
910

11+
use cargo_registry::db::RequestTransaction;
1012
use cargo_registry::dependency::EncodableDependency;
1113
use cargo_registry::download::EncodableVersionDownload;
14+
use cargo_registry::keyword::Keyword;
1215
use cargo_registry::krate::{Crate, EncodableCrate};
1316
use cargo_registry::upload as u;
1417
use cargo_registry::user::EncodableUser;
@@ -71,20 +74,22 @@ fn index() {
7174
assert_eq!(json.crates[0].id, krate.name);
7275
}
7376

77+
fn tx(req: &Request) -> &GenericConnection { req.tx().unwrap() }
78+
7479
#[test]
7580
fn index_queries() {
7681
let (_b, app, middle) = ::app();
7782

7883
let mut req = ::req(app, Method::Get, "/api/v1/crates");
7984
let u = ::mock_user(&mut req, ::user("foo"));
8085
let mut krate = ::krate("foo");
81-
krate.keywords.push("kw1".to_string());
8286
krate.readme = Some("readme".to_string());
8387
krate.description = Some("description".to_string());
84-
::mock_crate(&mut req, krate);
85-
let mut krate2 = ::krate("BAR");
86-
krate2.keywords.push("KW1".to_string());
87-
::mock_crate(&mut req, krate2);
88+
let (krate, _) = ::mock_crate(&mut req, krate.clone());
89+
let krate2 = ::krate("BAR");
90+
let (krate2, _) = ::mock_crate(&mut req, krate2.clone());
91+
Keyword::update_crate(tx(&req), &krate, &["kw1".into()]).unwrap();
92+
Keyword::update_crate(tx(&req), &krate2, &["KW1".into()]).unwrap();
8893

8994
let mut response = ok_resp!(middle.call(req.with_query("q=baz")));
9095
assert_eq!(::json::<CrateList>(&mut response).meta.total, 0);
@@ -345,7 +350,7 @@ fn new_crate_owner() {
345350
assert_eq!(::json::<CrateList>(&mut response).crates.len(), 1);
346351

347352
// And upload a new crate as the first user
348-
let body = ::new_req_body(::krate("foo"), "2.0.0", Vec::new());
353+
let body = ::new_req_body(::krate("foo"), "2.0.0", Vec::new(), Vec::new());
349354
req.mut_extensions().insert(u2);
350355
let mut response = ok_resp!(middle.call(req.with_path("/api/v1/crates/new")
351356
.with_method(Method::Put)
@@ -734,33 +739,33 @@ fn yank_not_owner() {
734739
fn bad_keywords() {
735740
let (_b, app, middle) = ::app();
736741
{
737-
let mut krate = ::krate("foo");
738-
krate.keywords.push("super-long-keyword-name-oh-no".to_string());
739-
let mut req = ::new_req_full(app.clone(), krate, "1.0.0", Vec::new());
742+
let krate = ::krate("foo");
743+
let kws = vec!["super-long-keyword-name-oh-no".into()];
744+
let mut req = ::new_req_with_keywords(app.clone(), krate, "1.0.0", kws);
740745
::mock_user(&mut req, ::user("foo"));
741746
let mut response = ok_resp!(middle.call(&mut req));
742747
::json::<::Bad>(&mut response);
743748
}
744749
{
745-
let mut krate = ::krate("foo");
746-
krate.keywords.push("?@?%".to_string());
747-
let mut req = ::new_req_full(app.clone(), krate, "1.0.0", Vec::new());
750+
let krate = ::krate("foo");
751+
let kws = vec!["?@?%".into()];
752+
let mut req = ::new_req_with_keywords(app.clone(), krate, "1.0.0", kws);
748753
::mock_user(&mut req, ::user("foo"));
749754
let mut response = ok_resp!(middle.call(&mut req));
750755
::json::<::Bad>(&mut response);
751756
}
752757
{
753-
let mut krate = ::krate("foo");
754-
krate.keywords.push("?@?%".to_string());
755-
let mut req = ::new_req_full(app.clone(), krate, "1.0.0", Vec::new());
758+
let krate = ::krate("foo");
759+
let kws = vec!["?@?%".into()];
760+
let mut req = ::new_req_with_keywords(app.clone(), krate, "1.0.0", kws);
756761
::mock_user(&mut req, ::user("foo"));
757762
let mut response = ok_resp!(middle.call(&mut req));
758763
::json::<::Bad>(&mut response);
759764
}
760765
{
761-
let mut krate = ::krate("foo");
762-
krate.keywords.push("áccênts".to_string());
763-
let mut req = ::new_req_full(app.clone(), krate, "1.0.0", Vec::new());
766+
let krate = ::krate("foo");
767+
let kws = vec!["áccênts".into()];
768+
let mut req = ::new_req_with_keywords(app.clone(), krate, "1.0.0", kws);
764769
::mock_user(&mut req, ::user("foo"));
765770
let mut response = ok_resp!(middle.call(&mut req));
766771
::json::<::Bad>(&mut response);

0 commit comments

Comments
 (0)