Skip to content
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

Hides crates on index calls unless they have at least one available v… #985

Closed
wants to merge 1 commit into from
Closed
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
11 changes: 11 additions & 0 deletions src/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -696,6 +696,17 @@ pub fn index(req: &mut Request) -> CargoResult<Response> {
));
}

if vec![params.get("user_id"), params.get("team_id")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this is worth the complexity of sticking it in a vec and iterating over it, vs params.get("user_id").is_some() || params.get("team_id").is_some()?

.iter()
.any(|s| s.is_some())
{
let not_yanked_versions = sql::<Bool>(
"crates.id = ANY (SELECT vs.crate_id FROM versions vs WHERE vs.crate_id = crates.id AND vs.yanked IS FALSE)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just be EXISTS (SELECT 1 FROM versions WHERE versions.crate_id = crates.id AND versions.yanked IS FALSE)?

);

query = query.filter(not_yanked_versions.clone())
}

// The database query returns a tuple within a tuple , with the root
// tuple containing 3 items.
let data = query
Expand Down
113 changes: 113 additions & 0 deletions src/tests/krate.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
extern crate diesel;
extern crate conduit;

use std::collections::HashMap;
use std::io::prelude::*;
Expand Down Expand Up @@ -1853,6 +1854,7 @@ fn yanked_versions_not_included_in_reverse_dependencies() {
assert_eq!(deps.dependencies[0].crate_id, "c1");

// TODO: have this test call `version.yank()` once the yank method is converted to diesel

diesel::update(versions::table.filter(versions::num.eq("2.0.0")))
.set(versions::yanked.eq(true))
.execute(&*app.diesel_database.get().unwrap())
Expand Down Expand Up @@ -2146,6 +2148,117 @@ fn test_default_sort_recent() {
assert_eq!(json.crates[1].downloads, 20);
}

/* Given two crates, one with all versions yanked and another
with a good version only the one that doesn't have all versions
yanked is returned if a user_id or team_id is provided.
*/
#[test]
fn test_hides_yanked_crate() {
let (_b, app, middle) = ::app();

let (user, team) = {
let conn = app.diesel_database.get().unwrap();
let user = ::new_user("Oskar").create_or_update(&conn).unwrap();
let team = ::new_team("github:test_org:team_sloth")
.create_or_update(&conn)
.unwrap();

let green_ball = ::CrateBuilder::new("green_ball", user.id)
.description("For fetching")
.downloads(0)
.recent_downloads(0)
.expect_build(&conn);

::add_team_to_crate(&team, &green_ball, &user, &conn).unwrap();

let yanked_crate = ::CrateBuilder::new("fully_yanked", user.id)
.description("Not here anymore")
.expect_build(&conn);

::add_team_to_crate(&team, &yanked_crate, &user, &conn).unwrap();

diesel::update(versions::table.filter(
versions::crate_id.eq(yanked_crate.id),
)).set(versions::yanked.eq(true))
.execute(&*conn)
.unwrap();

(user, team)
};

{
let mut req = ::req(app.clone(), Method::Get, "/api/v1/crates");
let mut response = ok_resp!(middle.call(req.with_query(
format!("sort=recent-downloads&user_id={}", user.id).as_str(),
)));
assert_crate_yanked(&mut response);
}

{
let mut req = ::req(app.clone(), Method::Get, "/api/v1/crates");
let mut response = ok_resp!(middle.call(req.with_query(
format!("sort=recent-downloads&team_id={}", team.id).as_str(),
)));
assert_crate_yanked(&mut response);
}
}

fn assert_crate_yanked(response: &mut conduit::Response) {
let json: CrateList = ::json(response);

assert_eq!(json.meta.total, 1);

assert_eq!(json.crates[0].name, "green_ball");
assert_eq!(json.crates[0].recent_downloads, Some(0));
assert_eq!(json.crates[0].downloads, 0);
}

/* Given two crates, one with all versions yanked and another
with a good version, both crates are returned if there is
no user_id or team_id provided.
*/
#[test]
fn test_shows_yanked_crate_without_user_or_team() {
let (_b, app, middle) = ::app();

{
let conn = app.diesel_database.get().unwrap();
let user = ::new_user("Oskar").create_or_update(&conn).unwrap();

::CrateBuilder::new("green_ball", user.id)
.description("For fetching")
.downloads(0)
.recent_downloads(0)
.expect_build(&conn);

let yanked_crate = ::CrateBuilder::new("fully_yanked", user.id)
.description("Not here anymore")
.expect_build(&conn);

diesel::update(versions::table.filter(
versions::crate_id.eq(yanked_crate.id),
)).set(versions::yanked.eq(true))
.execute(&*conn)
.unwrap();
}


let mut req = ::req(app.clone(), Method::Get, "/api/v1/crates");
let mut response = ok_resp!(middle.call(req.with_query("sort=recent-downloads")));

let json: CrateList = ::json(&mut response);

assert_eq!(json.meta.total, 2);

assert_eq!(json.crates[0].name, "green_ball");
assert_eq!(json.crates[0].recent_downloads, Some(0));
assert_eq!(json.crates[0].downloads, 0);

assert_eq!(json.crates[1].name, "fully_yanked");
assert_eq!(json.crates[1].recent_downloads, Some(0));
assert_eq!(json.crates[1].downloads, 0);
}

#[test]
fn block_blacklisted_documentation_url() {
let (_b, app, middle) = ::app();
Expand Down