Skip to content

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

Merged
merged 3 commits into from
Jan 21, 2021
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: 3 additions & 0 deletions src/metrics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ metrics! {

/// The number of attempted files that failed due to a memory limit
pub(crate) html_rewrite_ooms: IntCounter,

/// the number of "I'm feeling lucky" searches for crates
pub(crate) im_feeling_lucky_searches: IntCounter,
}

// The Rust prometheus library treats the namespace as the "prefix" of the metric name: a
Expand Down
122 changes: 81 additions & 41 deletions src/web/releases.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

the number depends on the ratio of crates >100 stars

Copy link
Member Author

Choose a reason for hiding this comment

The 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)

Copy link
Member

Choose a reason for hiding this comment

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

Let me know if it works out :)

Copy link
Member Author

Choose a reason for hiding this comment

The 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);
} 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,
Expand All @@ -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
Expand Down Expand Up @@ -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| {
Expand Down