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 6 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
8 changes: 7 additions & 1 deletion src/controllers/krate/search.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ pub fn search(req: &mut dyn Request) -> CargoResult<Response> {
let conn = req.db_conn()?;
let (offset, limit) = req.pagination(10, 100)?;
let params = req.query();
//extract the search param for loose searching
let search_q = if let Some(q) = params.get("q") {
format!("%{}%", q)
} else {
String::new()
};
let sort = params
.get("sort")
.map(|s| &**s)
Expand All @@ -57,7 +63,7 @@ pub fn search(req: &mut dyn Request) -> CargoResult<Response> {
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(&search_q)),
);

query = query.select((
Expand Down
20 changes: 20 additions & 0 deletions src/models/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ 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<'a> = diesel::pg::expression::helper_types::ILike<
CanonCrateName<crates::name>,
CanonCrateName<&'a str>,
>;
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 @@ -235,6 +240,21 @@ impl<'a> NewCrate<'a> {
}

impl Crate {
/// SQL filter with the `like` binary operator
/// ```sql
/// SELECT *
/// FROM crates
/// WHERE name like $1
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the SQL examples here?

Copy link
Author

Choose a reason for hiding this comment

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

They can be removed.

/// ```
pub fn like_name(name: &str) -> LikeName<'_> {
canon_crate_name(crates::name).ilike(canon_crate_name(name))
Copy link
Contributor

Choose a reason for hiding this comment

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

Just like is enough here, canon_crate_name already normalizes case

Copy link
Author

Choose a reason for hiding this comment

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

I can change this back, the only reason I used ilike was from your previous comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about that :(

}
/// SQL filter with the = binary operator
/// ```sql
/// SELECT *
/// FROM crates
/// WHERE name = $1
/// ```
pub fn with_name(name: &str) -> WithName<'_> {
canon_crate_name(crates::name).eq(canon_crate_name(name))
}
Expand Down
63 changes: 63 additions & 0 deletions src/tests/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,69 @@ 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);
// file shouldn't match at all
CrateBuilder::new("file", user.id)
.readme("readme")
.description("description")
.keyword("kw1")
.expect_build(conn);
// temp_udp should match second
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(
r#"$ echo 'println!("Hello World!")' | evalrs
Compiling evalrs_temp v0.0.0 (file:///tmp/evalrs_temp.daiPxHtjV2VR)
Finished debug [unoptimized + debuginfo] target(s) in 0.51 secs
Running `target\debug\evalrs_temp.exe`
Hello World!"#,
)
.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);
// mkstemp should appear 5th
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put a bit more context in here? Why should this be 5th instead of tempfile?

Copy link
Author

Choose a reason for hiding this comment

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

The only reason that is the case is because it was inserted later. I can remove that one.

let five = CrateBuilder::new("mkstemp", user.id)
.readme("readme")
.description("description")
.keyword("kw1")
.expect_build(conn);
vec![one, two, three, four, five]
});
let search_temp = anon.search("q=temp");
assert_eq!(search_temp.meta.total, 5);
assert_eq!(search_temp.crates.len(), 5);
for (lhs, rhs) in search_temp.crates.iter().zip(ordered) {
assert_eq!(lhs.name, rhs.name);
}
let search_file = anon.search("q=file");
assert_eq!(search_file.meta.total, 2);
assert_eq!(search_file.crates.len(), 2);
assert_eq!(&search_file.crates[0].name, "file");
assert_eq!(&search_file.crates[1].name, "tempfile");
}

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