diff --git a/app/adapters/crate.js b/app/adapters/crate.js index bac2eaac894..e07647fa156 100644 --- a/app/adapters/crate.js +++ b/app/adapters/crate.js @@ -9,6 +9,15 @@ export default class CrateAdapter extends ApplicationAdapter { return super.findRecord(store, type, id, setDefaultInclude(snapshot)); } + findHasMany(store, snapshot, url, relationship) { + if (relationship.key === 'version_downloads') { + // This ensures that related versions are included so we don't have to wait for versions + // request to finish for `belongsTo` relationships. + url += '?include=versions'; + } + return super.findHasMany(store, snapshot, url, relationship); + } + queryRecord(store, type, query, adapterOptions) { return super.queryRecord(store, type, setDefaultInclude(query), adapterOptions); } diff --git a/app/routes/crate/version.js b/app/routes/crate/version.js index a18450c7039..6c932f3ee66 100644 --- a/app/routes/crate/version.js +++ b/app/routes/crate/version.js @@ -15,17 +15,6 @@ export default class VersionRoute extends Route { async model(params, transition) { let crate = this.modelFor('crate'); - // TODO: Resolved version without waiting for versions to be resolved - // The main blocker for this right now is that we have a "belongsTo" relationship between - // `version-download` and `version` in sync mode. This requires us to wait for `versions` to - // exist before `version-download` can be created. - try { - await crate.loadVersionsTask.perform(); - } catch (error) { - let title = `${crate.name}: Failed to load version data`; - return this.router.replaceWith('catch-all', { transition, error, title, tryAgain: true }); - } - let version; let requestedVersion = params.version_num; let num = requestedVersion || crate.default_version; diff --git a/e2e/acceptance/crate.spec.ts b/e2e/acceptance/crate.spec.ts index 99b412d020b..ab605c978b5 100644 --- a/e2e/acceptance/crate.spec.ts +++ b/e2e/acceptance/crate.spec.ts @@ -111,22 +111,6 @@ test.describe('Acceptance | crate page', { tag: '@acceptance' }, () => { await expect(page.locator('[data-test-try-again]')).toHaveCount(0); }); - test('other versions loading error shows an error message', async ({ page, msw }) => { - let crate = msw.db.crate.create({ name: 'nanomsg' }); - msw.db.version.create({ crate, num: '0.6.0' }); - msw.db.version.create({ crate, num: '0.6.1' }); - - await msw.worker.use(http.get('/api/v1/crates/:crate_name/versions', () => HttpResponse.json({}, { status: 500 }))); - - await page.goto('/'); - await page.click('[data-test-just-updated] [data-test-crate-link="0"]'); - await expect(page).toHaveURL('/crates/nanomsg/0.6.0'); - await expect(page.locator('[data-test-404-page]')).toBeVisible(); - await expect(page.locator('[data-test-title]')).toHaveText('nanomsg: Failed to load version data'); - await expect(page.locator('[data-test-go-back]')).toHaveCount(0); - await expect(page.locator('[data-test-try-again]')).toBeVisible(); - }); - test('works for non-canonical names', async ({ page, msw }) => { let crate = msw.db.crate.create({ name: 'foo-bar' }); msw.db.version.create({ crate }); diff --git a/packages/crates-io-msw/handlers/crates/downloads.js b/packages/crates-io-msw/handlers/crates/downloads.js index 21239edd403..c4fd5b4372b 100644 --- a/packages/crates-io-msw/handlers/crates/downloads.js +++ b/packages/crates-io-msw/handlers/crates/downloads.js @@ -1,20 +1,29 @@ import { http, HttpResponse } from 'msw'; import { db } from '../../index.js'; +import { serializeVersion } from '../../serializers/version.js'; import { notFound } from '../../utils/handlers.js'; -export default http.get('/api/v1/crates/:name/downloads', async ({ params }) => { +export default http.get('/api/v1/crates/:name/downloads', async ({ request, params }) => { let crate = db.crate.findFirst({ where: { name: { equals: params.name } } }); if (!crate) return notFound(); let downloads = db.versionDownload.findMany({ where: { version: { crate: { id: { equals: crate.id } } } } }); - - return HttpResponse.json({ + let resp = { version_downloads: downloads.map(download => ({ date: download.date, downloads: download.downloads, version: download.version.id, })), meta: { extra_downloads: crate._extra_downloads }, - }); + }; + + let url = new URL(request.url); + let include = url.searchParams.get('include') ?? ''; + let includes = include ? include.split(',') : []; + if (includes.includes('versions')) { + let versions = [...new Set(downloads.map(it => it.version))]; + resp.versions = versions.map(it => serializeVersion(it)); + } + return HttpResponse.json(resp); }); diff --git a/packages/crates-io-msw/handlers/crates/downloads.test.js b/packages/crates-io-msw/handlers/crates/downloads.test.js index 4ce9c8f52a2..d1c3d9a3aca 100644 --- a/packages/crates-io-msw/handlers/crates/downloads.test.js +++ b/packages/crates-io-msw/handlers/crates/downloads.test.js @@ -53,3 +53,80 @@ test('returns a list of version downloads belonging to the specified crate versi }, }); }); + +test('includes related versions', async function () { + let crate = db.crate.create({ name: 'rand' }); + let versions = Array.from({ length: 2 }, () => db.version.create({ crate })); + db.versionDownload.create({ version: versions[0], date: '2020-01-13' }); + db.versionDownload.create({ version: versions[1], date: '2020-01-14' }); + db.versionDownload.create({ version: versions[1], date: '2020-01-15' }); + + let response = await fetch('/api/v1/crates/rand/downloads?include=versions'); + assert.strictEqual(response.status, 200); + assert.deepEqual(await response.json(), { + version_downloads: [ + { + date: '2020-01-13', + downloads: 7035, + version: 1, + }, + { + date: '2020-01-14', + downloads: 14_070, + version: 2, + }, + { + date: '2020-01-15', + downloads: 21_105, + version: 2, + }, + ], + versions: [ + { + crate: 'rand', + crate_size: 162_963, + created_at: '2010-06-16T21:30:45Z', + dl_path: '/api/v1/crates/rand/1.0.0/download', + downloads: 3702, + features: {}, + id: 1, + license: 'MIT', + links: { + dependencies: '/api/v1/crates/rand/1.0.0/dependencies', + version_downloads: '/api/v1/crates/rand/1.0.0/downloads', + }, + num: '1.0.0', + published_by: null, + readme_path: '/api/v1/crates/rand/1.0.0/readme', + rust_version: null, + updated_at: '2017-02-24T12:34:56Z', + yank_message: null, + yanked: false, + }, + { + crate: 'rand', + crate_size: 325_926, + created_at: '2010-06-16T21:30:45Z', + dl_path: '/api/v1/crates/rand/1.0.1/download', + downloads: 7404, + features: {}, + id: 2, + license: 'Apache-2.0', + links: { + dependencies: '/api/v1/crates/rand/1.0.1/dependencies', + version_downloads: '/api/v1/crates/rand/1.0.1/downloads', + }, + num: '1.0.1', + published_by: null, + readme_path: '/api/v1/crates/rand/1.0.1/readme', + rust_version: null, + updated_at: '2017-02-24T12:34:56Z', + yank_message: null, + yanked: false, + }, + ], + meta: { + extra_downloads: [], + }, + }); +}); diff --git a/src/controllers/krate/downloads.rs b/src/controllers/krate/downloads.rs index b3596a3fdbc..87ed619c6d0 100644 --- a/src/controllers/krate/downloads.rs +++ b/src/controllers/krate/downloads.rs @@ -6,17 +6,36 @@ use crate::app::AppState; use crate::controllers::krate::CratePath; use crate::models::download::Version; -use crate::models::VersionDownload; -use crate::schema::{version_downloads, versions}; -use crate::util::errors::AppResult; -use crate::views::EncodableVersionDownload; +use crate::models::{User, Version as FullVersion, VersionDownload, VersionOwnerAction}; +use crate::schema::{version_downloads, version_owner_actions, versions}; +use crate::util::errors::{bad_request, AppResult, BoxedAppError}; +use crate::views::{EncodableVersion, EncodableVersionDownload}; +use axum::extract::FromRequestParts; +use axum_extra::extract::Query; use axum_extra::json; use axum_extra::response::ErasedJson; +use crates_io_database::schema::users; use crates_io_diesel_helpers::to_char; use diesel::prelude::*; -use diesel_async::RunQueryDsl; +use diesel_async::{AsyncPgConnection, RunQueryDsl}; +use futures_util::future::BoxFuture; use futures_util::FutureExt; use std::cmp; +use std::str::FromStr; + +#[derive(Debug, Deserialize, FromRequestParts, utoipa::IntoParams)] +#[from_request(via(Query))] +#[into_params(parameter_in = Query)] +pub struct DownloadsQueryParams { + /// Additional data to include in the response. + /// + /// Valid values: `versions`. + /// + /// Defaults to no additional data. + /// + /// This parameter expects a comma-separated list of values. + include: Option, +} /// Get the download counts for a crate. /// @@ -25,12 +44,16 @@ use std::cmp; #[utoipa::path( get, path = "/api/v1/crates/{name}/downloads", - params(CratePath), + params(CratePath, DownloadsQueryParams), tag = "crates", responses((status = 200, description = "Successful Response")), )] -pub async fn get_crate_downloads(state: AppState, path: CratePath) -> AppResult { +pub async fn get_crate_downloads( + state: AppState, + path: CratePath, + params: DownloadsQueryParams, +) -> AppResult { let mut conn = state.db_read().await?; use diesel::dsl::*; @@ -47,8 +70,15 @@ pub async fn get_crate_downloads(state: AppState, path: CratePath) -> AppResult< versions.sort_unstable_by(|a, b| b.num.cmp(&a.num)); let (latest_five, rest) = versions.split_at(cmp::min(5, versions.len())); + let include = params + .include + .as_ref() + .map(|mode| ShowIncludeMode::from_str(mode)) + .transpose()? + .unwrap_or_default(); + let sum_downloads = sql::("SUM(version_downloads.downloads)"); - let (downloads, extra) = tokio::try_join!( + let (downloads, extra, versions_and_publishers, actions) = tokio::try_join!( VersionDownload::belonging_to(latest_five) .filter(version_downloads::date.gt(date(now - 90.days()))) .order(( @@ -67,6 +97,8 @@ pub async fn get_crate_downloads(state: AppState, path: CratePath) -> AppResult< .order(version_downloads::date.asc()) .load::(&mut conn) .boxed(), + load_versions_and_publishers(&mut conn, latest_five, include.versions), + load_actions(&mut conn, latest_five, include.versions), )?; let downloads = downloads @@ -80,6 +112,28 @@ pub async fn get_crate_downloads(state: AppState, path: CratePath) -> AppResult< downloads: i64, } + if include.versions { + let versions_and_publishers = versions_and_publishers.grouped_by(latest_five); + let actions = actions.grouped_by(latest_five); + let versions = versions_and_publishers + .into_iter() + .zip(actions) + .filter_map(|(vp, actions)| { + vp.into_iter().next().map(|(version, publisher)| { + EncodableVersion::from(version, &path.name, publisher, actions) + }) + }) + .collect::>(); + + return Ok(json!({ + "version_downloads": downloads, + "versions": versions, + "meta": { + "extra_downloads": extra, + }, + })); + } + Ok(json!({ "version_downloads": downloads, "meta": { @@ -87,3 +141,59 @@ pub async fn get_crate_downloads(state: AppState, path: CratePath) -> AppResult< }, })) } + +type VersionsAndPublishers = (FullVersion, Option); +fn load_versions_and_publishers<'a>( + conn: &mut AsyncPgConnection, + versions: &'a [Version], + includes: bool, +) -> BoxFuture<'a, QueryResult>> { + if !includes { + return futures_util::future::always_ready(|| Ok(vec![])).boxed(); + } + FullVersion::belonging_to(versions) + .left_outer_join(users::table) + .select(VersionsAndPublishers::as_select()) + .load(conn) + .boxed() +} + +fn load_actions<'a>( + conn: &mut AsyncPgConnection, + versions: &'a [Version], + includes: bool, +) -> BoxFuture<'a, QueryResult>> { + if !includes { + return futures_util::future::always_ready(|| Ok(vec![])).boxed(); + } + VersionOwnerAction::belonging_to(versions) + .inner_join(users::table) + .order(version_owner_actions::id) + .load(conn) + .boxed() +} + +#[derive(Debug, Default)] +struct ShowIncludeMode { + versions: bool, +} + +impl ShowIncludeMode { + const INVALID_COMPONENT: &'static str = "invalid component for ?include= (expected 'versions')"; +} + +impl FromStr for ShowIncludeMode { + type Err = BoxedAppError; + + fn from_str(s: &str) -> Result { + let mut mode = Self { versions: false }; + for component in s.split(',') { + match component { + "" => {} + "versions" => mode.versions = true, + _ => return Err(bad_request(Self::INVALID_COMPONENT)), + } + } + Ok(mode) + } +} diff --git a/src/models/action.rs b/src/models/action.rs index d54f3230ee5..cd7921dd0ec 100644 --- a/src/models/action.rs +++ b/src/models/action.rs @@ -41,6 +41,7 @@ impl From for String { belongs_to(Version), belongs_to(User, foreign_key = user_id), belongs_to(ApiToken, foreign_key = api_token_id), + belongs_to(crate::models::download::Version, foreign_key = version_id), )] pub struct VersionOwnerAction { pub id: i32, diff --git a/src/models/version.rs b/src/models/version.rs index f8fdf4ee27c..4baff8d6a8b 100644 --- a/src/models/version.rs +++ b/src/models/version.rs @@ -13,7 +13,7 @@ use crate::schema::*; // Queryable has a custom implementation below #[derive(Clone, Identifiable, Associations, Debug, Queryable, Selectable)] -#[diesel(belongs_to(Crate))] +#[diesel(belongs_to(Crate), belongs_to(crate::models::download::Version, foreign_key=id))] pub struct Version { pub id: i32, pub crate_id: i32, diff --git a/src/snapshots/crates_io__openapi__tests__openapi_snapshot.snap b/src/snapshots/crates_io__openapi__tests__openapi_snapshot.snap index 47a8d349e0f..33af3856b16 100644 --- a/src/snapshots/crates_io__openapi__tests__openapi_snapshot.snap +++ b/src/snapshots/crates_io__openapi__tests__openapi_snapshot.snap @@ -564,6 +564,15 @@ expression: response.json() "schema": { "type": "string" } + }, + { + "description": "Additional data to include in the response.\n\nValid values: `versions`.\n\nDefaults to no additional data.\n\nThis parameter expects a comma-separated list of values.", + "in": "query", + "name": "include", + "required": false, + "schema": { + "type": "string" + } } ], "responses": { diff --git a/tests/acceptance/crate-test.js b/tests/acceptance/crate-test.js index b7937482432..01aa740bc72 100644 --- a/tests/acceptance/crate-test.js +++ b/tests/acceptance/crate-test.js @@ -117,22 +117,6 @@ module('Acceptance | crate page', function (hooks) { assert.dom('[data-test-try-again]').doesNotExist(); }); - test('other versions loading error shows an error message', async function (assert) { - let crate = this.db.crate.create({ name: 'nanomsg' }); - this.db.version.create({ crate, num: '0.6.0' }); - this.db.version.create({ crate, num: '0.6.1' }); - - this.worker.use(http.get('/api/v1/crates/:crate_name/versions', () => HttpResponse.json({}, { status: 500 }))); - - await visit('/'); - await click('[data-test-just-updated] [data-test-crate-link="0"]'); - assert.strictEqual(currentURL(), '/crates/nanomsg/0.6.0'); - assert.dom('[data-test-404-page]').exists(); - assert.dom('[data-test-title]').hasText('nanomsg: Failed to load version data'); - assert.dom('[data-test-go-back]').doesNotExist(); - assert.dom('[data-test-try-again]').exists(); - }); - test('works for non-canonical names', async function (assert) { let crate = this.db.crate.create({ name: 'foo-bar' }); this.db.version.create({ crate });