From 52b33b6f8e5087ccb0fc13fe211bfe820a65f070 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teo=20Klestrup=20R=C3=B6ijezon?= Date: Thu, 10 Feb 2022 16:23:40 +0100 Subject: [PATCH 1/7] Add a "minimal crate metadata" API endpoint As mentioned in #4503 and https://github.com/renovatebot/renovate/issues/3486, this is intended to be a lighter-weight API than /crates/:crate_id, which can be subject to looser rate limits. --- src/controllers/krate/metadata.rs | 24 ++++++++++++++++++++++-- src/controllers/krate/publish.rs | 2 +- src/controllers/krate/search.rs | 2 +- src/router.rs | 1 + src/views.rs | 14 +++++++------- 5 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 3e3c64c308e..2d674e702a8 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -45,7 +45,7 @@ pub fn summary(req: &mut dyn RequestExt) -> EndpointResult { .map(|((top_versions, krate), recent_downloads)| { Ok(EncodableCrate::from_minimal( krate, - &top_versions, + Some(&top_versions), None, false, recent_downloads, @@ -159,7 +159,7 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult { Ok(req.json(&json!({ "crate": EncodableCrate::from( krate.clone(), - &top_versions, + Some(&top_versions), Some(ids), Some(&kws), Some(&cats), @@ -176,6 +176,26 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult { }))) } +/// Handles the `GET /crates/:crate_id/crate` route. +/// +/// A minimal version of [`show`] that only covers the crate itself, without versions or catalog information +/// (such as keywords and categories). +pub fn krate(req: &mut dyn RequestExt) -> EndpointResult { + let name = &req.params()["crate_id"]; + let conn = req.db_read_only()?; + let krate: Crate = Crate::by_name(name).first(&*conn)?; + + Ok(req.json(&json!({ + "crate": EncodableCrate::from_minimal( + krate, + None, + None, + false, + None, + ), + }))) +} + /// Handles the `GET /crates/:crate_id/:version/readme` route. pub fn readme(req: &mut dyn RequestExt) -> EndpointResult { let crate_name = &req.params()["crate_id"]; diff --git a/src/controllers/krate/publish.rs b/src/controllers/krate/publish.rs index 361d808bd17..165b9b86e89 100644 --- a/src/controllers/krate/publish.rs +++ b/src/controllers/krate/publish.rs @@ -252,7 +252,7 @@ pub fn publish(req: &mut dyn RequestExt) -> EndpointResult { }; Ok(req.json(&GoodCrate { - krate: EncodableCrate::from_minimal(krate, &top_versions, None, false, None), + krate: EncodableCrate::from_minimal(krate, Some(&top_versions), None, false, None), warnings, })) }) diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 3951776e1a0..116e8147511 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -330,7 +330,7 @@ pub fn search(req: &mut dyn RequestExt) -> EndpointResult { |((((max_version, krate), perfect_match), recent_downloads), badges)| { EncodableCrate::from_minimal( krate, - &max_version, + Some(&max_version), Some(badges), perfect_match, Some(recent_downloads), diff --git a/src/router.rs b/src/router.rs index 737d6cb9320..c99df1799aa 100644 --- a/src/router.rs +++ b/src/router.rs @@ -36,6 +36,7 @@ pub fn build_router(app: &App) -> RouteBuilder { // Routes used by the frontend api_router.get("/crates/:crate_id", C(krate::metadata::show)); + api_router.get("/crates/:crate_id/crate", C(krate::metadata::krate)); api_router.get("/crates/:crate_id/:version", C(version::metadata::show)); api_router.get( "/crates/:crate_id/:version/readme", diff --git a/src/views.rs b/src/views.rs index af6ce34733a..787fafb968a 100644 --- a/src/views.rs +++ b/src/views.rs @@ -238,7 +238,7 @@ impl EncodableCrate { #[allow(clippy::too_many_arguments)] pub fn from( krate: Crate, - top_versions: &TopVersions, + top_versions: Option<&TopVersions>, versions: Option>, keywords: Option<&[Keyword]>, categories: Option<&[Category]>, @@ -267,18 +267,18 @@ impl EncodableCrate { let documentation = Self::remove_blocked_documentation_urls(documentation); let max_version = top_versions - .highest - .as_ref() + .and_then(|v| v.highest.as_ref()) .map(|v| v.to_string()) .unwrap_or_else(|| "0.0.0".to_string()); let newest_version = top_versions - .newest - .as_ref() + .and_then(|v| v.newest.as_ref()) .map(|v| v.to_string()) .unwrap_or_else(|| "0.0.0".to_string()); - let max_stable_version = top_versions.highest_stable.as_ref().map(|v| v.to_string()); + let max_stable_version = top_versions + .and_then(|v| v.highest_stable.as_ref()) + .map(|v| v.to_string()); EncodableCrate { id: name.clone(), @@ -312,7 +312,7 @@ impl EncodableCrate { pub fn from_minimal( krate: Crate, - top_versions: &TopVersions, + top_versions: Option<&TopVersions>, badges: Option>, exact_match: bool, recent_downloads: Option, From 29c21f5206715f4ccc0253be06934eb6a2061955 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teo=20Klestrup=20R=C3=B6ijezon?= Date: Thu, 10 Feb 2022 16:59:19 +0100 Subject: [PATCH 2/7] There's not much of a point having a single-field wrapper object --- src/controllers/krate/metadata.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 2d674e702a8..8d05a8417bc 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -185,15 +185,9 @@ pub fn krate(req: &mut dyn RequestExt) -> EndpointResult { let conn = req.db_read_only()?; let krate: Crate = Crate::by_name(name).first(&*conn)?; - Ok(req.json(&json!({ - "crate": EncodableCrate::from_minimal( - krate, - None, - None, - false, - None, - ), - }))) + Ok(req.json(&EncodableCrate::from_minimal( + krate, None, None, false, None, + ))) } /// Handles the `GET /crates/:crate_id/:version/readme` route. From 2dbb3db03aa619ccbc17145df7d75b36e08440fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teo=20Klestrup=20R=C3=B6ijezon?= Date: Fri, 11 Feb 2022 15:23:32 +0100 Subject: [PATCH 3/7] Add test for minimal crate metadata endpoint --- src/tests/krate/krate.rs | 47 ++++++++++++++++++++++++++++++++++++++++ src/tests/krate/mod.rs | 1 + src/tests/util.rs | 11 +++++++++- 3 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 src/tests/krate/krate.rs diff --git a/src/tests/krate/krate.rs b/src/tests/krate/krate.rs new file mode 100644 index 00000000000..419ecd7a700 --- /dev/null +++ b/src/tests/krate/krate.rs @@ -0,0 +1,47 @@ +use crate::builders::{CrateBuilder, VersionBuilder}; +use crate::util::{RequestHelper, TestApp}; +use diesel::prelude::*; + +#[test] +fn krate() { + let (app, anon, user) = TestApp::init().with_user(); + let user = user.as_model(); + + let krate = app.db(|conn| { + use cargo_registry::schema::versions; + use diesel::{update, ExpressionMethods}; + + let krate = CrateBuilder::new("foo_show", user.id) + .description("description") + .documentation("https://example.com") + .homepage("http://example.com") + .version(VersionBuilder::new("1.0.0")) + .version(VersionBuilder::new("0.5.0")) + .version(VersionBuilder::new("0.5.1")) + .keyword("kw1") + .downloads(20) + .recent_downloads(10) + .expect_build(conn); + + // Make version 1.0.0 mimic a version published before we started recording who published + // versions + let none: Option = None; + update(versions::table) + .filter(versions::num.eq("1.0.0")) + .set(versions::published_by.eq(none)) + .execute(conn) + .unwrap(); + + krate + }); + + let json = anon.show_crate_minimal("foo_show"); + assert_eq!(json.name, krate.name); + assert_eq!(json.id, krate.name); + assert_eq!(json.description, krate.description); + assert_eq!(json.homepage, krate.homepage); + assert_eq!(json.documentation, krate.documentation); + assert_eq!(json.keywords, None); + assert_eq!(json.recent_downloads, None); + assert_eq!(json.versions, None); +} diff --git a/src/tests/krate/mod.rs b/src/tests/krate/mod.rs index 29140ce05a9..34f705c9b72 100644 --- a/src/tests/krate/mod.rs +++ b/src/tests/krate/mod.rs @@ -1,6 +1,7 @@ mod dependencies; mod downloads; mod following; +mod krate; mod owners; mod publish; mod reverse_dependencies; diff --git a/src/tests/util.rs b/src/tests/util.rs index e2f4d69f554..f7a6a9fb47f 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -23,7 +23,10 @@ use crate::{ builders::PublishBuilder, CategoryListResponse, CategoryResponse, CrateList, CrateResponse, GoodCrate, OkBool, OwnersResponse, VersionResponse, }; -use cargo_registry::models::{ApiToken, CreatedApiToken, User}; +use cargo_registry::{ + models::{ApiToken, CreatedApiToken, User}, + views::EncodableCrate, +}; use conduit::{BoxError, Handler, Method}; use conduit_cookie::SessionMiddleware; @@ -162,6 +165,12 @@ pub trait RequestHelper { self.get(&url).good() } + /// Request the JSON used for a crate's minimal page + fn show_crate_minimal(&self, krate_name: &str) -> EncodableCrate { + let url = format!("/api/v1/crates/{krate_name}/crate"); + self.get(&url).good() + } + /// Request the JSON used to list a crate's owners fn show_crate_owners(&self, krate_name: &str) -> OwnersResponse { let url = format!("/api/v1/crates/{krate_name}/owners"); From f90863b14458e93e9b733c185666a7d47b970cfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teo=20Klestrup=20R=C3=B6ijezon?= Date: Fri, 11 Feb 2022 15:37:26 +0100 Subject: [PATCH 4/7] Rename krate::metadata::krate to show_minimal --- src/controllers/krate/metadata.rs | 2 +- src/router.rs | 2 +- src/tests/krate/krate.rs | 47 ------------------------------- src/tests/krate/mod.rs | 1 - src/tests/krate/show.rs | 44 +++++++++++++++++++++++++++++ 5 files changed, 46 insertions(+), 50 deletions(-) delete mode 100644 src/tests/krate/krate.rs diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 8d05a8417bc..523960da375 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -180,7 +180,7 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult { /// /// A minimal version of [`show`] that only covers the crate itself, without versions or catalog information /// (such as keywords and categories). -pub fn krate(req: &mut dyn RequestExt) -> EndpointResult { +pub fn show_minimal(req: &mut dyn RequestExt) -> EndpointResult { let name = &req.params()["crate_id"]; let conn = req.db_read_only()?; let krate: Crate = Crate::by_name(name).first(&*conn)?; diff --git a/src/router.rs b/src/router.rs index c99df1799aa..8981cb28134 100644 --- a/src/router.rs +++ b/src/router.rs @@ -36,7 +36,7 @@ pub fn build_router(app: &App) -> RouteBuilder { // Routes used by the frontend api_router.get("/crates/:crate_id", C(krate::metadata::show)); - api_router.get("/crates/:crate_id/crate", C(krate::metadata::krate)); + api_router.get("/crates/:crate_id/crate", C(krate::metadata::show_minimal)); api_router.get("/crates/:crate_id/:version", C(version::metadata::show)); api_router.get( "/crates/:crate_id/:version/readme", diff --git a/src/tests/krate/krate.rs b/src/tests/krate/krate.rs deleted file mode 100644 index 419ecd7a700..00000000000 --- a/src/tests/krate/krate.rs +++ /dev/null @@ -1,47 +0,0 @@ -use crate::builders::{CrateBuilder, VersionBuilder}; -use crate::util::{RequestHelper, TestApp}; -use diesel::prelude::*; - -#[test] -fn krate() { - let (app, anon, user) = TestApp::init().with_user(); - let user = user.as_model(); - - let krate = app.db(|conn| { - use cargo_registry::schema::versions; - use diesel::{update, ExpressionMethods}; - - let krate = CrateBuilder::new("foo_show", user.id) - .description("description") - .documentation("https://example.com") - .homepage("http://example.com") - .version(VersionBuilder::new("1.0.0")) - .version(VersionBuilder::new("0.5.0")) - .version(VersionBuilder::new("0.5.1")) - .keyword("kw1") - .downloads(20) - .recent_downloads(10) - .expect_build(conn); - - // Make version 1.0.0 mimic a version published before we started recording who published - // versions - let none: Option = None; - update(versions::table) - .filter(versions::num.eq("1.0.0")) - .set(versions::published_by.eq(none)) - .execute(conn) - .unwrap(); - - krate - }); - - let json = anon.show_crate_minimal("foo_show"); - assert_eq!(json.name, krate.name); - assert_eq!(json.id, krate.name); - assert_eq!(json.description, krate.description); - assert_eq!(json.homepage, krate.homepage); - assert_eq!(json.documentation, krate.documentation); - assert_eq!(json.keywords, None); - assert_eq!(json.recent_downloads, None); - assert_eq!(json.versions, None); -} diff --git a/src/tests/krate/mod.rs b/src/tests/krate/mod.rs index 34f705c9b72..29140ce05a9 100644 --- a/src/tests/krate/mod.rs +++ b/src/tests/krate/mod.rs @@ -1,7 +1,6 @@ mod dependencies; mod downloads; mod following; -mod krate; mod owners; mod publish; mod reverse_dependencies; diff --git a/src/tests/krate/show.rs b/src/tests/krate/show.rs index 6177fc62185..660504b9608 100644 --- a/src/tests/krate/show.rs +++ b/src/tests/krate/show.rs @@ -68,6 +68,50 @@ fn show() { ); } +#[test] +fn show_minimal() { + let (app, anon, user) = TestApp::init().with_user(); + let user = user.as_model(); + + let krate = app.db(|conn| { + use cargo_registry::schema::versions; + use diesel::{update, ExpressionMethods}; + + let krate = CrateBuilder::new("foo_show_minimal", user.id) + .description("description") + .documentation("https://example.com") + .homepage("http://example.com") + .version(VersionBuilder::new("1.0.0")) + .version(VersionBuilder::new("0.5.0")) + .version(VersionBuilder::new("0.5.1")) + .keyword("kw1") + .downloads(20) + .recent_downloads(10) + .expect_build(conn); + + // Make version 1.0.0 mimic a version published before we started recording who published + // versions + let none: Option = None; + update(versions::table) + .filter(versions::num.eq("1.0.0")) + .set(versions::published_by.eq(none)) + .execute(conn) + .unwrap(); + + krate + }); + + let json = anon.show_crate_minimal("foo_show_minimal"); + assert_eq!(json.name, krate.name); + assert_eq!(json.id, krate.name); + assert_eq!(json.description, krate.description); + assert_eq!(json.homepage, krate.homepage); + assert_eq!(json.documentation, krate.documentation); + assert_eq!(json.keywords, None); + assert_eq!(json.recent_downloads, None); + assert_eq!(json.versions, None); +} + #[test] fn block_bad_documentation_url() { let (app, anon, user) = TestApp::init().with_user(); From b9c7fff9d2aa80eeb6aa866a3b751296e85a0100 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teo=20Klestrup=20R=C3=B6ijezon?= Date: Mon, 14 Feb 2022 11:40:00 +0100 Subject: [PATCH 5/7] Convert krate::show_minimal into a mode of krate::show --- src/controllers/krate/metadata.rs | 195 ++++++++++++++++++++---------- src/router.rs | 1 - src/tests/all.rs | 4 +- src/tests/krate/show.rs | 46 +++---- src/tests/util.rs | 19 +-- src/tests/version.rs | 4 + 6 files changed, 172 insertions(+), 97 deletions(-) diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 523960da375..945ab6c5f78 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -5,6 +5,7 @@ //! `Cargo.toml` file. use std::cmp::Reverse; +use std::str::FromStr; use crate::controllers::frontend_prelude::*; use crate::controllers::helpers::pagination::PaginationOptions; @@ -111,83 +112,145 @@ pub fn summary(req: &mut dyn RequestExt) -> EndpointResult { /// Handles the `GET /crates/:crate_id` route. pub fn show(req: &mut dyn RequestExt) -> EndpointResult { let name = &req.params()["crate_id"]; + let include = req + .query() + .get("include") + .map(|mode| ShowIncludeMode::from_str(mode)) + .transpose()? + .unwrap_or_default(); + let conn = req.db_read_only()?; let krate: Crate = Crate::by_name(name).first(&*conn)?; - let mut versions_and_publishers: Vec<(Version, Option)> = krate - .all_versions() - .left_outer_join(users::table) - .select((versions::all_columns, users::all_columns.nullable())) - .load(&*conn)?; - - versions_and_publishers - .sort_by_cached_key(|(version, _)| Reverse(semver::Version::parse(&version.num).ok())); - - let versions = versions_and_publishers - .iter() - .map(|(v, _)| v) - .cloned() - .collect::>(); - let versions_publishers_and_audit_actions = versions_and_publishers - .into_iter() - .zip(VersionOwnerAction::for_versions(&conn, &versions)?.into_iter()) - .map(|((v, pb), aas)| (v, pb, aas)) - .collect::>(); + let versions_publishers_and_audit_actions = if include.full { + let mut versions_and_publishers: Vec<(Version, Option)> = krate + .all_versions() + .left_outer_join(users::table) + .select((versions::all_columns, users::all_columns.nullable())) + .load(&*conn)?; + versions_and_publishers + .sort_by_cached_key(|(version, _)| Reverse(semver::Version::parse(&version.num).ok())); + + let versions = versions_and_publishers + .iter() + .map(|(v, _)| v) + .cloned() + .collect::>(); + Some( + versions_and_publishers + .into_iter() + .zip(VersionOwnerAction::for_versions(&conn, &versions)?.into_iter()) + .map(|((v, pb), aas)| (v, pb, aas)) + .collect::>(), + ) + } else { + None + }; let ids = versions_publishers_and_audit_actions - .iter() - .map(|v| v.0.id) - .collect(); - - let kws = CrateKeyword::belonging_to(&krate) - .inner_join(keywords::table) - .select(keywords::all_columns) - .load(&*conn)?; - let cats = CrateCategory::belonging_to(&krate) - .inner_join(categories::table) - .select(categories::all_columns) - .load(&*conn)?; - let recent_downloads = RecentCrateDownloads::belonging_to(&krate) - .select(recent_crate_downloads::downloads) - .get_result(&*conn) - .optional()?; + .as_ref() + .map(|vps| vps.iter().map(|v| v.0.id).collect()); + + let kws = if include.full { + Some( + CrateKeyword::belonging_to(&krate) + .inner_join(keywords::table) + .select(keywords::all_columns) + .load(&*conn)?, + ) + } else { + None + }; + let cats = if include.full { + Some( + CrateCategory::belonging_to(&krate) + .inner_join(categories::table) + .select(categories::all_columns) + .load(&*conn)?, + ) + } else { + None + }; + let recent_downloads = if include.full { + RecentCrateDownloads::belonging_to(&krate) + .select(recent_crate_downloads::downloads) + .get_result(&*conn) + .optional()? + } else { + None + }; - let badges = badges::table - .filter(badges::crate_id.eq(krate.id)) - .load(&*conn)?; - let top_versions = krate.top_versions(&conn)?; + let badges = if include.full { + Some( + badges::table + .filter(badges::crate_id.eq(krate.id)) + .load(&*conn)?, + ) + } else { + None + }; + let top_versions = if include.full { + Some(krate.top_versions(&conn)?) + } else { + None + }; - Ok(req.json(&json!({ - "crate": EncodableCrate::from( - krate.clone(), - Some(&top_versions), - Some(ids), - Some(&kws), - Some(&cats), - Some(badges), - false, - recent_downloads, - ), - "versions": versions_publishers_and_audit_actions - .into_iter() + let encodable_crate = EncodableCrate::from( + krate.clone(), + top_versions.as_ref(), + ids, + kws.as_deref(), + cats.as_deref(), + badges, + false, + recent_downloads, + ); + let encodable_versions = versions_publishers_and_audit_actions.map(|vpa| { + vpa.into_iter() .map(|(v, pb, aas)| EncodableVersion::from(v, &krate.name, pb, aas)) - .collect::>(), - "keywords": kws.into_iter().map(Keyword::into).collect::>(), - "categories": cats.into_iter().map(Category::into).collect::>(), + .collect::>() + }); + let encodable_keywords = kws.map(|kws| { + kws.into_iter() + .map(Keyword::into) + .collect::>() + }); + let encodable_cats = cats.map(|cats| { + cats.into_iter() + .map(Category::into) + .collect::>() + }); + Ok(req.json(&json!({ + "crate": encodable_crate, + "versions": encodable_versions, + "keywords": encodable_keywords, + "categories": encodable_cats, }))) } -/// Handles the `GET /crates/:crate_id/crate` route. -/// -/// A minimal version of [`show`] that only covers the crate itself, without versions or catalog information -/// (such as keywords and categories). -pub fn show_minimal(req: &mut dyn RequestExt) -> EndpointResult { - let name = &req.params()["crate_id"]; - let conn = req.db_read_only()?; - let krate: Crate = Crate::by_name(name).first(&*conn)?; +#[derive(Debug)] +struct ShowIncludeMode { + full: bool, +} - Ok(req.json(&EncodableCrate::from_minimal( - krate, None, None, false, None, - ))) +impl Default for ShowIncludeMode { + fn default() -> Self { + // Send everything for legacy clients that expect the full response + Self { full: true } + } +} + +impl FromStr for ShowIncludeMode { + type Err = Box; + + fn from_str(s: &str) -> Result { + match s { + "" => Ok(ShowIncludeMode { full: false }), + "full" => Ok(ShowIncludeMode { full: true }), + _ => Err(bad_request( + "invalid value for ?mode= (expected '' or 'full')", + )), + } + } } /// Handles the `GET /crates/:crate_id/:version/readme` route. diff --git a/src/router.rs b/src/router.rs index 8981cb28134..737d6cb9320 100644 --- a/src/router.rs +++ b/src/router.rs @@ -36,7 +36,6 @@ pub fn build_router(app: &App) -> RouteBuilder { // Routes used by the frontend api_router.get("/crates/:crate_id", C(krate::metadata::show)); - api_router.get("/crates/:crate_id/crate", C(krate::metadata::show_minimal)); api_router.get("/crates/:crate_id/:version", C(version::metadata::show)); api_router.get( "/crates/:crate_id/:version/readme", diff --git a/src/tests/all.rs b/src/tests/all.rs index 493a1b63a75..07badc60c81 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -69,8 +69,8 @@ struct CrateMeta { pub struct CrateResponse { #[serde(rename = "crate")] krate: EncodableCrate, - versions: Vec, - keywords: Vec, + versions: Option>, + keywords: Option>, } #[derive(Deserialize)] pub struct VersionResponse { diff --git a/src/tests/krate/show.rs b/src/tests/krate/show.rs index 660504b9608..49d89a90b10 100644 --- a/src/tests/krate/show.rs +++ b/src/tests/krate/show.rs @@ -43,27 +43,29 @@ fn show() { assert_eq!(json.krate.documentation, krate.documentation); assert_eq!(json.krate.keywords, Some(vec!["kw1".into()])); assert_eq!(json.krate.recent_downloads, Some(10)); - let versions = json.krate.versions.as_ref().unwrap(); + let crate_versions = json.krate.versions.as_ref().unwrap(); + assert_eq!(crate_versions.len(), 3); + let versions = json.versions.as_ref().unwrap(); assert_eq!(versions.len(), 3); - assert_eq!(json.versions.len(), 3); - assert_eq!(json.versions[0].id, versions[0]); - assert_eq!(json.versions[0].krate, json.krate.id); - assert_eq!(json.versions[0].num, "1.0.0"); - assert_none!(&json.versions[0].published_by); + assert_eq!(versions[0].id, crate_versions[0]); + assert_eq!(versions[0].krate, json.krate.id); + assert_eq!(versions[0].num, "1.0.0"); + assert_none!(&versions[0].published_by); let suffix = "/api/v1/crates/foo_show/1.0.0/download"; assert!( - json.versions[0].dl_path.ends_with(suffix), + versions[0].dl_path.ends_with(suffix), "bad suffix {}", - json.versions[0].dl_path + versions[0].dl_path ); - assert_eq!(1, json.keywords.len()); - assert_eq!("kw1", json.keywords[0].id); + let keywords = json.keywords.as_ref().unwrap(); + assert_eq!(1, keywords.len()); + assert_eq!("kw1", keywords[0].id); - assert_eq!(json.versions[1].num, "0.5.1"); - assert_eq!(json.versions[2].num, "0.5.0"); + assert_eq!(versions[1].num, "0.5.1"); + assert_eq!(versions[2].num, "0.5.0"); assert_eq!( - json.versions[1].published_by.as_ref().unwrap().login, + versions[1].published_by.as_ref().unwrap().login, user.gh_login ); } @@ -102,14 +104,16 @@ fn show_minimal() { }); let json = anon.show_crate_minimal("foo_show_minimal"); - assert_eq!(json.name, krate.name); - assert_eq!(json.id, krate.name); - assert_eq!(json.description, krate.description); - assert_eq!(json.homepage, krate.homepage); - assert_eq!(json.documentation, krate.documentation); - assert_eq!(json.keywords, None); - assert_eq!(json.recent_downloads, None); - assert_eq!(json.versions, None); + assert_eq!(json.krate.name, krate.name); + assert_eq!(json.krate.id, krate.name); + assert_eq!(json.krate.description, krate.description); + assert_eq!(json.krate.homepage, krate.homepage); + assert_eq!(json.krate.documentation, krate.documentation); + assert_eq!(json.krate.keywords, None); + assert_eq!(json.krate.recent_downloads, None); + assert_eq!(json.krate.versions, None); + assert!(json.versions.is_none()); + assert!(json.keywords.is_none()); } #[test] diff --git a/src/tests/util.rs b/src/tests/util.rs index f7a6a9fb47f..e96cc1ea362 100644 --- a/src/tests/util.rs +++ b/src/tests/util.rs @@ -23,10 +23,7 @@ use crate::{ builders::PublishBuilder, CategoryListResponse, CategoryResponse, CrateList, CrateResponse, GoodCrate, OkBool, OwnersResponse, VersionResponse, }; -use cargo_registry::{ - models::{ApiToken, CreatedApiToken, User}, - views::EncodableCrate, -}; +use cargo_registry::models::{ApiToken, CreatedApiToken, User}; use conduit::{BoxError, Handler, Method}; use conduit_cookie::SessionMiddleware; @@ -106,6 +103,14 @@ pub trait RequestHelper { self.run(self.get_request(path)) } + /// Issue a GET request with a query string + #[track_caller] + fn get_query(&self, path: &str, query: &str) -> Response { + let mut req = self.get_request(path); + req.with_query(query); + self.run(req) + } + /// Issue a GET request that includes query parameters #[track_caller] fn get_with_query(&self, path: &str, query: &str) -> Response { @@ -166,9 +171,9 @@ pub trait RequestHelper { } /// Request the JSON used for a crate's minimal page - fn show_crate_minimal(&self, krate_name: &str) -> EncodableCrate { - let url = format!("/api/v1/crates/{krate_name}/crate"); - self.get(&url).good() + fn show_crate_minimal(&self, krate_name: &str) -> CrateResponse { + let url = format!("/api/v1/crates/{krate_name}"); + self.get_query(&url, "include=").good() } /// Request the JSON used to list a crate's owners diff --git a/src/tests/version.rs b/src/tests/version.rs index 610c62182d1..82c4bbe567f 100644 --- a/src/tests/version.rs +++ b/src/tests/version.rs @@ -150,6 +150,8 @@ fn version_size() { let version1 = crate_json .versions + .as_ref() + .unwrap() .iter() .find(|v| v.num == "1.0.0") .expect("Could not find v1.0.0"); @@ -157,6 +159,8 @@ fn version_size() { let version2 = crate_json .versions + .as_ref() + .unwrap() .iter() .find(|v| v.num == "2.0.0") .expect("Could not find v2.0.0"); From 28ead19603c2db9e345660e74b2ee4858c646145 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teo=20Klestrup=20R=C3=B6ijezon?= Date: Mon, 14 Feb 2022 11:48:48 +0100 Subject: [PATCH 6/7] Break out include modes into specific categories --- src/controllers/krate/metadata.rs | 64 ++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 14 deletions(-) diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 945ab6c5f78..374d1e2d383 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -122,7 +122,7 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult { let conn = req.db_read_only()?; let krate: Crate = Crate::by_name(name).first(&*conn)?; - let versions_publishers_and_audit_actions = if include.full { + let versions_publishers_and_audit_actions = if include.versions { let mut versions_and_publishers: Vec<(Version, Option)> = krate .all_versions() .left_outer_join(users::table) @@ -150,7 +150,7 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult { .as_ref() .map(|vps| vps.iter().map(|v| v.0.id).collect()); - let kws = if include.full { + let kws = if include.keywords { Some( CrateKeyword::belonging_to(&krate) .inner_join(keywords::table) @@ -160,7 +160,7 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult { } else { None }; - let cats = if include.full { + let cats = if include.categories { Some( CrateCategory::belonging_to(&krate) .inner_join(categories::table) @@ -170,7 +170,7 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult { } else { None }; - let recent_downloads = if include.full { + let recent_downloads = if include.downloads { RecentCrateDownloads::belonging_to(&krate) .select(recent_crate_downloads::downloads) .get_result(&*conn) @@ -179,7 +179,7 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult { None }; - let badges = if include.full { + let badges = if include.badges { Some( badges::table .filter(badges::crate_id.eq(krate.id)) @@ -188,7 +188,7 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult { } else { None }; - let top_versions = if include.full { + let top_versions = if include.versions { Some(krate.top_versions(&conn)?) } else { None @@ -229,27 +229,63 @@ pub fn show(req: &mut dyn RequestExt) -> EndpointResult { #[derive(Debug)] struct ShowIncludeMode { - full: bool, + versions: bool, + keywords: bool, + categories: bool, + badges: bool, + downloads: bool, } impl Default for ShowIncludeMode { fn default() -> Self { // Send everything for legacy clients that expect the full response - Self { full: true } + Self { + versions: true, + keywords: true, + categories: true, + badges: true, + downloads: true, + } } } +impl ShowIncludeMode { + const INVALID_COMPONENT: &'static str = + "invalid component for ?mode= (expected 'versions', 'keywords', 'categories', 'badges', 'downloads', or 'full')"; +} + impl FromStr for ShowIncludeMode { type Err = Box; fn from_str(s: &str) -> Result { - match s { - "" => Ok(ShowIncludeMode { full: false }), - "full" => Ok(ShowIncludeMode { full: true }), - _ => Err(bad_request( - "invalid value for ?mode= (expected '' or 'full')", - )), + let mut mode = Self { + versions: false, + keywords: false, + categories: false, + badges: false, + downloads: false, + }; + for component in s.split(',') { + match component { + "" => {} + "full" => { + mode = Self { + versions: true, + keywords: true, + categories: true, + badges: true, + downloads: true, + } + } + "versions" => mode.versions = true, + "keywords" => mode.keywords = true, + "categories" => mode.categories = true, + "badges" => mode.badges = true, + "downloads" => mode.downloads = true, + _ => return Err(bad_request(Self::INVALID_COMPONENT)), + } } + Ok(mode) } } From fae83f17e68c3903331bffc1476e27a1180c1dc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Teo=20Klestrup=20R=C3=B6ijezon?= Date: Mon, 14 Mar 2022 09:09:06 +0100 Subject: [PATCH 7/7] Fix INVALID_COMPONENT referring to the old parameter name ?mode --- src/controllers/krate/metadata.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 374d1e2d383..0aac33c30ee 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -251,7 +251,7 @@ impl Default for ShowIncludeMode { impl ShowIncludeMode { const INVALID_COMPONENT: &'static str = - "invalid component for ?mode= (expected 'versions', 'keywords', 'categories', 'badges', 'downloads', or 'full')"; + "invalid component for ?include= (expected 'versions', 'keywords', 'categories', 'badges', 'downloads', or 'full')"; } impl FromStr for ShowIncludeMode {