-
Notifications
You must be signed in to change notification settings - Fork 209
Fix "I'm feeling lucky" search to include all crates #1251
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
|
||
use crate::{ | ||
build_queue::QueuedCrate, | ||
db::Pool, | ||
db::{Pool, PoolClient}, | ||
impl_webpage, | ||
web::{error::Nope, match_version, page::WebPage, redirect_base}, | ||
BuildQueue, | ||
|
@@ -497,6 +497,73 @@ impl Default for Search { | |
} | ||
} | ||
|
||
fn redirect_to_random_crate(req: &Request, conn: &mut PoolClient) -> IronResult<Response> { | ||
// since there is a chance of this query returning an empty result, | ||
// we retry a certain amount of times, and log a warning. | ||
for i in 0..20 { | ||
let rows = ctry!( | ||
req, | ||
conn.query( | ||
"WITH params AS ( | ||
-- get maximum possible id-value in crates-table | ||
SELECT last_value AS max_id FROM crates_id_seq | ||
) | ||
SELECT | ||
crates.name, | ||
releases.version, | ||
releases.target_name | ||
FROM ( | ||
-- generate 500 random numbers in the ID-range. | ||
-- this might have to be increased when we have to repeat | ||
-- this query too often. | ||
-- it depends on the percentage of crates with > 100 stars | ||
SELECT DISTINCT 1 + trunc(random() * params.max_id)::INTEGER AS id | ||
FROM params, generate_series(1, 500) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hard-coding 500 makes me a little nervous. Could we generate this live based on production info? Or does that slow down the query too much? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. generating that value live slows down the query too much, I valued speed higher. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the number depends on the ratio of crates >100 stars There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. while thinking about this, I have another idea (but not likely it's faster) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me know if it works out :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. didn't help 😢 was too slow :) |
||
) AS r | ||
INNER JOIN crates ON r.id = crates.id | ||
INNER JOIN releases ON crates.latest_version_id = releases.id | ||
INNER JOIN github_repos ON releases.github_repo = github_repos.id | ||
WHERE | ||
releases.rustdoc_status = TRUE AND | ||
github_repos.stars >= 100 | ||
LIMIT 1 | ||
", | ||
&[] | ||
), | ||
); | ||
|
||
if let Some(row) = rows.into_iter().next() { | ||
let name: String = row.get("name"); | ||
let version: String = row.get("version"); | ||
let target_name: String = row.get("target_name"); | ||
let url = ctry!( | ||
req, | ||
Url::parse(&format!( | ||
"{}/{}/{}/{}", | ||
redirect_base(req), | ||
name, | ||
version, | ||
target_name | ||
)), | ||
); | ||
|
||
let mut resp = Response::with((status::Found, Redirect(url))); | ||
resp.headers.set(Expires(HttpDate(time::now()))); | ||
|
||
let metrics = extension!(req, crate::Metrics).clone(); | ||
metrics.im_feeling_lucky_searches.inc(); | ||
|
||
log::debug!("finished random crate search on iteration {}", i); | ||
return Ok(resp); | ||
syphar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
log::warn!("retrying random crate search"); | ||
} | ||
} | ||
log::error!("found no result in random crate search"); | ||
|
||
Err(Nope::NoResults.into()) | ||
} | ||
|
||
impl_webpage! { | ||
Search = "releases/releases.html", | ||
status = |search| search.status, | ||
|
@@ -511,53 +578,14 @@ pub fn search_handler(req: &mut Request) -> IronResult<Response> { | |
if let Some((_, query)) = query { | ||
// check if I am feeling lucky button pressed and redirect user to crate page | ||
// if there is a match | ||
// TODO: Redirecting to latest doc might be more useful | ||
// NOTE: calls `query_pairs()` again because iterators are lazy and only yield items once | ||
if url | ||
.query_pairs() | ||
.any(|(key, _)| key == "i-am-feeling-lucky") | ||
{ | ||
// redirect to a random crate if query is empty | ||
if query.is_empty() { | ||
// FIXME: This is a fast query but using a constant | ||
// There are currently 280 crates with docs and 100+ | ||
// starts. This should be fine for a while. | ||
let rows = ctry!( | ||
req, | ||
conn.query( | ||
"SELECT crates.name, | ||
releases.version, | ||
releases.target_name | ||
FROM crates | ||
INNER JOIN releases | ||
ON crates.latest_version_id = releases.id | ||
LEFT JOIN github_repos | ||
ON releases.github_repo = github_repos.id | ||
WHERE github_repos.stars >= 100 AND rustdoc_status = true | ||
OFFSET FLOOR(RANDOM() * 280) LIMIT 1", | ||
&[] | ||
), | ||
); | ||
let row = rows.into_iter().next().unwrap(); | ||
|
||
let name: String = row.get("name"); | ||
let version: String = row.get("version"); | ||
let target_name: String = row.get("target_name"); | ||
let url = ctry!( | ||
req, | ||
Url::parse(&format!( | ||
"{}/{}/{}/{}", | ||
redirect_base(req), | ||
name, | ||
version, | ||
target_name | ||
)), | ||
); | ||
|
||
let mut resp = Response::with((status::Found, Redirect(url))); | ||
resp.headers.set(Expires(HttpDate(time::now()))); | ||
|
||
return Ok(resp); | ||
return redirect_to_random_crate(req, &mut conn); | ||
} | ||
|
||
// since we never pass a version into `match_version` here, we'll never get | ||
|
@@ -1064,6 +1092,18 @@ mod tests { | |
}) | ||
} | ||
|
||
#[test] | ||
fn im_feeling_lucky_with_stars() { | ||
wrapper(|env| { | ||
let web = env.frontend(); | ||
env.fake_release() | ||
.github_stats("some/repo", 333, 22, 11) | ||
.name("some_random_crate") | ||
.create()?; | ||
assert_success("/releases/search?query=&i-am-feeling-lucky=1", web) | ||
}) | ||
} | ||
|
||
#[test] | ||
fn search() { | ||
wrapper(|env| { | ||
|
Uh oh!
There was an error while loading. Please reload this page.