Skip to content

loosen search #1560

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.

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 11 commits into from
Apr 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion src/controllers/krate/search.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,11 @@ pub fn search(req: &mut dyn Request) -> CargoResult<Response> {
has_filter = true;
if !q_string.is_empty() {
let sort = params.get("sort").map(|s| &**s).unwrap_or("relevance");

let q = plainto_tsquery(q_string);
query = query.filter(
q.matches(crates::textsearchable_index_col)
.or(Crate::with_name(q_string)),
.or(Crate::like_name(&q_string)),
);

query = query.select((
Expand Down
9 changes: 9 additions & 0 deletions src/models/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ pub const MAX_NAME_LENGTH: usize = 64;
type CanonCrateName<T> = self::canon_crate_name::HelperType<T>;
type All = diesel::dsl::Select<crates::table, AllColumns>;
type WithName<'a> = diesel::dsl::Eq<CanonCrateName<crates::name>, CanonCrateName<&'a str>>;
/// The result of a loose search
type LikeName = diesel::dsl::Like<CanonCrateName<crates::name>, CanonCrateName<String>>;
type ByName<'a> = diesel::dsl::Filter<All, WithName<'a>>;
type ByExactName<'a> = diesel::dsl::Filter<All, diesel::dsl::Eq<crates::name, &'a str>>;

Expand Down Expand Up @@ -234,6 +236,13 @@ impl<'a> NewCrate<'a> {
}

impl Crate {
/// SQL filter with the `like` binary operator. Adds wildcards to the beginning and end to get
/// substring matches.
pub fn like_name(name: &str) -> LikeName {
let wildcard_name = format!("%{}%", name);
canon_crate_name(crates::name).like(canon_crate_name(wildcard_name))
}
/// SQL filter with the = binary operator
pub fn with_name(name: &str) -> WithName<'_> {
canon_crate_name(crates::name).eq(canon_crate_name(name))
}
Expand Down
40 changes: 40 additions & 0 deletions src/tests/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,46 @@ fn exact_match_on_queries_with_sort() {
assert_eq!(json.crates[3].name, "other_sort");
}

#[test]
fn loose_search_order() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is extremely long given how little code is being changed. Do you think this might be retesting things that are already tested elsewhere? Would it be worth trying to narrow the scope a bit to make it more clear what's being tested?

Copy link
Author

@FreeMasen FreeMasen Feb 14, 2019

Choose a reason for hiding this comment

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

General sort order is being addressed in other tests. I could reduce the number of creates to simplify the test. The basic idea is that we are now including crates that are partial matches. One of the previous comments about this PR was that the change should't impact current ordering so I tried to capture a few pieces of information that go into the current ordering like _ working like white space or description matching superseding this new search. I can remove the non-matching crate and the extra partial match to shorten things up?

let (app, anon, user) = TestApp::init().with_user();
let user = user.as_model();

let ordered = app.db(|conn| {
// exact match should be first
let one = CrateBuilder::new("temp", user.id)
.readme("readme")
.description("description")
.keyword("kw1")
.expect_build(conn);
// temp_udp should match second because of _
let two = CrateBuilder::new("temp_utp", user.id)
.readme("readme")
.description("description")
.keyword("kw1")
.expect_build(conn);
// evalrs should match 3rd because of readme
let three = CrateBuilder::new("evalrs", user.id)
.readme("evalrs_temp evalrs_temp evalrs_temp")
.description("description")
.keyword("kw1")
.expect_build(conn);
// tempfile should appear 4th
let four = CrateBuilder::new("tempfile", user.id)
.readme("readme")
.description("description")
.keyword("kw1")
.expect_build(conn);
vec![one, two, three, four]
});
let search_temp = anon.search("q=temp");
assert_eq!(search_temp.meta.total, 4);
assert_eq!(search_temp.crates.len(), 4);
for (lhs, rhs) in search_temp.crates.iter().zip(ordered) {
assert_eq!(lhs.name, rhs.name);
}
}

#[test]
fn show() {
let (app, anon, user) = TestApp::init().with_user();
Expand Down