From 638cdb1cabca489113f3a1267d1238083e4c7d8b Mon Sep 17 00:00:00 2001 From: Paul Woolcock Date: Sun, 28 Oct 2018 09:40:49 -0400 Subject: [PATCH 1/5] Allow multiple keywords in crate search This commit allows multiple `keyword=` parameters in the querystring to enable searching for all crates under the specified keywords Towards #1461 --- src/controllers/krate/search.rs | 13 ++++++++++++- src/tests/krate.rs | 1 + 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index d2c100e451b..f438929221d 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -95,7 +95,18 @@ pub fn search(req: &mut dyn Request) -> CargoResult { ); } - if let Some(kw) = params.get("keyword") { + if let Some(kws) = params.get("all_keywords") { + let names: Vec<_> = kws.split_whitespace().map(|name| name.to_lowercase()).collect(); + + query = query.filter( + crates::id.eq_any( + crates_keywords::table + .select(crates_keywords::crate_id) + .inner_join(keywords::table) + .filter(crate::lower(keywords::keyword).eq(any(names))), + ), + ); + } else if let Some(kw) = params.get("keyword") { query = query.filter( crates::id.eq_any( crates_keywords::table diff --git a/src/tests/krate.rs b/src/tests/krate.rs index accaa6579c2..b406306fce8 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -139,6 +139,7 @@ fn index_queries() { assert_eq!(anon.search("keyword=kw1").crates.len(), 2); assert_eq!(anon.search("keyword=KW1").crates.len(), 2); assert_eq!(anon.search("keyword=kw2").crates.len(), 0); + assert_eq!(anon.search("keyword=kw1&keyword=kw3").crates.len(), 3); assert_eq!(anon.search("q=foo&keyword=kw1").crates.len(), 1); assert_eq!(anon.search("q=foo2&keyword=kw1").crates.len(), 0); From 97d15eaa2b1d42b1f4a83cd129c182640447ab21 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Fri, 22 Feb 2019 10:29:28 -0500 Subject: [PATCH 2/5] Add a failing test for the ALL functionality --- src/tests/krate.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/tests/krate.rs b/src/tests/krate.rs index b406306fce8..7a7563b441f 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -117,6 +117,11 @@ fn index_queries() { CrateBuilder::new("foo", user.id) .keyword("kw3") .expect_build(conn); + + CrateBuilder::new("two-keywords", user.id) + .keyword("kw1") + .keyword("kw3") + .expect_build(conn); (krate, krate2) }); @@ -124,11 +129,11 @@ fn index_queries() { // All of these fields should be indexed/searched by the queries assert_eq!(anon.search("q=foo").meta.total, 2); - assert_eq!(anon.search("q=kw1").meta.total, 2); + assert_eq!(anon.search("q=kw1").meta.total, 3); assert_eq!(anon.search("q=readme").meta.total, 1); assert_eq!(anon.search("q=description").meta.total, 1); - assert_eq!(anon.search_by_user_id(user.id).crates.len(), 3); + assert_eq!(anon.search_by_user_id(user.id).crates.len(), 4); assert_eq!(anon.search_by_user_id(0).crates.len(), 0); assert_eq!(anon.search("letter=F").crates.len(), 2); @@ -136,10 +141,10 @@ fn index_queries() { assert_eq!(anon.search("letter=b").crates.len(), 1); assert_eq!(anon.search("letter=c").crates.len(), 0); - assert_eq!(anon.search("keyword=kw1").crates.len(), 2); - assert_eq!(anon.search("keyword=KW1").crates.len(), 2); + assert_eq!(anon.search("keyword=kw1").crates.len(), 3); + assert_eq!(anon.search("keyword=KW1").crates.len(), 3); assert_eq!(anon.search("keyword=kw2").crates.len(), 0); - assert_eq!(anon.search("keyword=kw1&keyword=kw3").crates.len(), 3); + assert_eq!(anon.search("all_keywords=kw1 kw3").crates.len(), 1); assert_eq!(anon.search("q=foo&keyword=kw1").crates.len(), 1); assert_eq!(anon.search("q=foo2&keyword=kw1").crates.len(), 0); From 8f5366607a11a55d55896710c3b94019597741bb Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Fri, 14 Jun 2019 12:00:51 -0600 Subject: [PATCH 3/5] Add a working "matches all keywords" query. Diesel has a bug where array expression methods aren't implemented on nullable expressions, so we can't just call Diesel's `.contains` method until the fix lands. I've added a workaround for the time being. This results in a runtime error since `keyword` was defined as `varchar`, which coerces to `text`, but arrays do not. I've changed the column type to address this. The keywords table is small enough and read infrequently enough that I'm not concerned about the exclusive lock in this migration. --- .../down.sql | 1 + .../up.sql | 1 + src/controllers/krate/search.rs | 15 ++++++++++++--- 3 files changed, 14 insertions(+), 3 deletions(-) create mode 100644 migrations/2019-06-14-175833_change_keywords_to_text/down.sql create mode 100644 migrations/2019-06-14-175833_change_keywords_to_text/up.sql diff --git a/migrations/2019-06-14-175833_change_keywords_to_text/down.sql b/migrations/2019-06-14-175833_change_keywords_to_text/down.sql new file mode 100644 index 00000000000..2acad5c25ab --- /dev/null +++ b/migrations/2019-06-14-175833_change_keywords_to_text/down.sql @@ -0,0 +1 @@ +ALTER TABLE keywords ALTER COLUMN keyword TYPE varchar; diff --git a/migrations/2019-06-14-175833_change_keywords_to_text/up.sql b/migrations/2019-06-14-175833_change_keywords_to_text/up.sql new file mode 100644 index 00000000000..b21d7a2e467 --- /dev/null +++ b/migrations/2019-06-14-175833_change_keywords_to_text/up.sql @@ -0,0 +1 @@ +ALTER TABLE keywords ALTER COLUMN keyword TYPE text; diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index f438929221d..4262d6f0345 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -96,14 +96,21 @@ pub fn search(req: &mut dyn Request) -> CargoResult { } if let Some(kws) = params.get("all_keywords") { + use diesel::sql_types::Array; + sql_function!(#[aggregate] fn array_agg(x: T) -> Array); + let names: Vec<_> = kws.split_whitespace().map(|name| name.to_lowercase()).collect(); query = query.filter( - crates::id.eq_any( + // FIXME: Just use `.contains` in Diesel 2.0 + // https://github.com/diesel-rs/diesel/issues/2066 + Contains::new( crates_keywords::table - .select(crates_keywords::crate_id) .inner_join(keywords::table) - .filter(crate::lower(keywords::keyword).eq(any(names))), + .filter(crates_keywords::crate_id.eq(crates::id)) + .select(array_agg(keywords::keyword)) + .single_value(), + names.into_sql::>(), ), ); } else if let Some(kw) = params.get("keyword") { @@ -241,3 +248,5 @@ pub fn search(req: &mut dyn Request) -> CargoResult { }, })) } + +diesel_infix_operator!(Contains, "@>"); From 547339ae690978569cc44a64ab72808f85668372 Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 21 Oct 2019 16:46:32 -0400 Subject: [PATCH 4/5] Commit changes to schema made by migrations --- src/schema.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/schema.rs b/src/schema.rs index 6b692eeada9..e1f3b593b72 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -544,10 +544,10 @@ table! { id -> Int4, /// The `keyword` column of the `keywords` table. /// - /// Its SQL type is `Varchar`. + /// Its SQL type is `Text`. /// /// (Automatically generated by Diesel.) - keyword -> Varchar, + keyword -> Text, /// The `crates_cnt` column of the `keywords` table. /// /// Its SQL type is `Int4`. From 9a3485bfda774ba4d3ee94ce3a8f1ddf34dab34f Mon Sep 17 00:00:00 2001 From: "Carol (Nichols || Goulding)" Date: Mon, 21 Oct 2019 16:50:52 -0400 Subject: [PATCH 5/5] Add an all_keywords parameter to the search route Pass along any specified value for the all_keywords parameter to the API request to search crates. This lets us manually construct URLs in the browser like: - /search?all_keywords=foo+bar - /search?all_keywords=foo+bar&q=test but there is no form field in the search form as yet; eventually, we'll need an "advanced search" form or similar. --- app/controllers/application.js | 1 + app/controllers/search.js | 2 +- app/routes/search.js | 3 ++- src/controllers/krate/search.rs | 5 ++++- 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/controllers/application.js b/app/controllers/application.js index 8d8216d7424..b4646c22bf0 100644 --- a/app/controllers/application.js +++ b/app/controllers/application.js @@ -28,6 +28,7 @@ export default Controller.extend(EKMixin, { queryParams: { q: this.searchQuery, page: 1, + all_keywords: null, }, }); }, diff --git a/app/controllers/search.js b/app/controllers/search.js index 4c08b873454..a9be8a51dea 100644 --- a/app/controllers/search.js +++ b/app/controllers/search.js @@ -9,7 +9,7 @@ import PaginationMixin from '../mixins/pagination'; export default Controller.extend(PaginationMixin, { search: service(), - queryParams: ['q', 'page', 'per_page', 'sort'], + queryParams: ['all_keywords', 'page', 'per_page', 'q', 'sort'], q: alias('search.q'), page: '1', per_page: 10, diff --git a/app/routes/search.js b/app/routes/search.js index 62a8325e11d..cbd74a0da6d 100644 --- a/app/routes/search.js +++ b/app/routes/search.js @@ -2,8 +2,9 @@ import Route from '@ember/routing/route'; export default Route.extend({ queryParams: { - q: { refreshModel: true }, + all_keywords: { refreshModel: true }, page: { refreshModel: true }, + q: { refreshModel: true }, sort: { refreshModel: true }, }, diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 4262d6f0345..2b71702e880 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -99,7 +99,10 @@ pub fn search(req: &mut dyn Request) -> CargoResult { use diesel::sql_types::Array; sql_function!(#[aggregate] fn array_agg(x: T) -> Array); - let names: Vec<_> = kws.split_whitespace().map(|name| name.to_lowercase()).collect(); + let names: Vec<_> = kws + .split_whitespace() + .map(|name| name.to_lowercase()) + .collect(); query = query.filter( // FIXME: Just use `.contains` in Diesel 2.0