Skip to content

Crate with all versions yanked display reduced info #5314

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

Closed
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
17 changes: 15 additions & 2 deletions src/controllers/krate/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ pub fn summary(req: &mut dyn RequestExt) -> EndpointResult {
let krates = data.into_iter().map(|(c, _)| c).collect::<Vec<_>>();

let versions: Vec<Version> = krates.versions().load(&*conn)?;
let all_versions_yanked = versions.is_empty();
versions
.grouped_by(&krates)
.into_iter()
Expand All @@ -53,6 +54,7 @@ pub fn summary(req: &mut dyn RequestExt) -> EndpointResult {
None,
false,
recent_downloads,
all_versions_yanked,
))
})
.collect()
Expand Down Expand Up @@ -125,6 +127,8 @@ pub fn summary(req: &mut dyn RequestExt) -> EndpointResult {

/// Handles the `GET /crates/:crate_id` route.
pub fn show(req: &mut dyn RequestExt) -> EndpointResult {
use diesel::dsl::exists;

let name = &req.params()["crate_id"];
let include = req
.query()
Expand All @@ -136,6 +140,14 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult {
let conn = req.db_read()?;
let krate: Crate = Crate::by_name(name).first(&*conn)?;

let all_versions_yanked = if include.versions {
// this query checks if there any non-yanked versions for a crate.
// then inverts the result.
!diesel::select(exists(krate.versions())).get_result::<bool>(&*conn)?
} else {
false
};

let versions_publishers_and_audit_actions = if include.versions {
let mut versions_and_publishers: Vec<(Version, Option<User>)> = krate
.all_versions()
Expand Down Expand Up @@ -193,7 +205,7 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult {
None
};

let badges = if include.badges {
let badges = if include.badges && !all_versions_yanked {
Copy link
Author

Choose a reason for hiding this comment

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

This is not required, but it avoids the database to do run unnecessary queries. It could be premature optimisation, let me know if you would like it removed.

Some(
badges::table
.filter(badges::crate_id.eq(krate.id))
Expand All @@ -202,7 +214,7 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult {
} else {
None
};
let top_versions = if include.versions {
let top_versions = if include.versions && !all_versions_yanked {
Copy link
Author

Choose a reason for hiding this comment

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

Same as above.

Some(krate.top_versions(&conn)?)
} else {
None
Expand All @@ -217,6 +229,7 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult {
badges,
false,
recent_downloads,
all_versions_yanked,
);
let encodable_versions = versions_publishers_and_audit_actions.map(|vpa| {
vpa.into_iter()
Expand Down
9 changes: 8 additions & 1 deletion src/controllers/krate/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,14 @@ pub fn publish(req: &mut dyn RequestExt) -> EndpointResult {
};

Ok(req.json(&GoodCrate {
krate: EncodableCrate::from_minimal(krate, Some(&top_versions), None, false, None),
krate: EncodableCrate::from_minimal(
krate,
Some(&top_versions),
None,
false,
None,
false,
Copy link
Author

Choose a reason for hiding this comment

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

Assuming there is no way to publish an yanked crate so setting up this to false.

),
warnings,
}))
})
Expand Down
13 changes: 12 additions & 1 deletion src/controllers/krate/search.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,12 @@ pub fn search(req: &mut dyn RequestExt) -> EndpointResult {
let crates = data.into_iter().map(|(c, _, _)| c).collect::<Vec<_>>();

let versions: Vec<Version> = crates.versions().load(&*conn)?;
let yanked_crates = versions
.clone()
.grouped_by(&crates)
.into_iter()
.map(|v| v.is_empty());

let versions = versions
.grouped_by(&crates)
.into_iter()
Expand All @@ -331,14 +337,19 @@ pub fn search(req: &mut dyn RequestExt) -> EndpointResult {
.zip(perfect_matches)
.zip(recent_downloads)
.zip(badges)
.zip(yanked_crates)
.map(
|((((max_version, krate), perfect_match), recent_downloads), badges)| {
|(
((((max_version, krate), perfect_match), recent_downloads), badges),
all_versions_yanked,
)| {
EncodableCrate::from_minimal(
krate,
Some(&max_version),
Some(badges),
perfect_match,
Some(recent_downloads),
all_versions_yanked,
)
},
)
Expand Down
10 changes: 10 additions & 0 deletions src/models/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,16 @@ impl<'a> NewCrate<'a> {
}

impl Crate {
pub fn from_yanked(krate: Crate) -> Crate {
Crate {
description: None,
homepage: None,
documentation: None,
repository: None,
..krate
}
}

/// SQL filter based on whether the crate's name loosely matches the given
/// string.
///
Expand Down
51 changes: 51 additions & 0 deletions src/tests/krate/search.rs
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,57 @@ fn index_include_yanked() {
assert_eq!(json.crates[2].name, "unyanked");
}

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

app.db(|conn| {
//TODO Add badge test case here.
CrateBuilder::new("test_all_yanked", user.id)
.version(VersionBuilder::new("1.0.0").yanked(true))
.version(VersionBuilder::new("2.0.0").yanked(true))
.description("yanked crate")
.homepage("https://github.com/test/yanked-crate")
.documentation("https://yanked-crate.github.io")
.expect_build(conn);

CrateBuilder::new("test_unyanked", user.id)
.version(VersionBuilder::new("1.0.0"))
.version(VersionBuilder::new("2.0.0"))
.description("unyanked crate")
.homepage("https://github.com/test/unyanked-crate")
.documentation("https://unyanked-crate.github.io")
.expect_build(conn);
});

let json = anon.search("q=test");
assert_eq!(json.meta.total, 2);

assert_eq!(json.crates[0].name, "test_all_yanked");
assert_eq!(json.crates[0].max_version, "0.0.0");
assert_eq!(json.crates[0].badges, None);
assert_eq!(json.crates[0].documentation, None);
assert_eq!(json.crates[0].homepage, None);
assert_eq!(json.crates[0].description, None);

assert_eq!(json.crates[1].name, "test_unyanked");
assert_eq!(json.crates[1].max_version, "2.0.0");
assert_eq!(json.crates[1].badges, Some(vec![]));
assert_eq!(
json.crates[1].documentation,
Some("https://unyanked-crate.github.io".to_string())
);
assert_eq!(
json.crates[1].homepage,
Some("https://github.com/test/unyanked-crate".to_string())
);
assert_eq!(
json.crates[1].description,
Some("unyanked crate".to_string())
);
}

#[test]
fn yanked_versions_are_not_considered_for_max_version() {
let (app, anon, user) = TestApp::init().with_user();
Expand Down
21 changes: 19 additions & 2 deletions src/views.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ impl EncodableCrate {
badges: Option<Vec<Badge>>,
exact_match: bool,
recent_downloads: Option<i64>,
all_versions_yanked: bool,
) -> Self {
let Crate {
name,
Expand All @@ -256,16 +257,30 @@ impl EncodableCrate {
documentation,
repository,
..
} = krate;
} = if all_versions_yanked {
Crate::from_yanked(krate)
} else {
krate
};
let versions_link = match versions {
Some(..) => None,
None => Some(format!("/api/v1/crates/{name}/versions")),
};
let keyword_ids = keywords.map(|kws| kws.iter().map(|kw| kw.keyword.clone()).collect());
let category_ids = categories.map(|cats| cats.iter().map(|cat| cat.slug.clone()).collect());
let badges = badges.map(|_| vec![]);
let documentation = Self::remove_blocked_documentation_urls(documentation);

let badges = if all_versions_yanked {
None
} else {
badges.map(|_| vec![])
};
let top_versions = if all_versions_yanked {
None
} else {
top_versions
};

let max_version = top_versions
.and_then(|v| v.highest.as_ref())
.map(|v| v.to_string())
Expand Down Expand Up @@ -316,6 +331,7 @@ impl EncodableCrate {
badges: Option<Vec<Badge>>,
exact_match: bool,
recent_downloads: Option<i64>,
all_versions_yanked: bool,
) -> Self {
Self::from(
krate,
Expand All @@ -326,6 +342,7 @@ impl EncodableCrate {
badges,
exact_match,
recent_downloads,
all_versions_yanked,
)
}

Expand Down