Merge pull request #10529 from eth3lbert/download-include-versions

Add `?include=versions` support for `GET /api/v1/crates/{name}/downloads` endpoint

Author: Turbo87

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);
   }

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;

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 });

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);
 });

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: [],
+    },
+  });
+});

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<String>,
+}
 
 /// 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<ErasedJson> {
+pub async fn get_crate_downloads(
+    state: AppState,
+    path: CratePath,
+    params: DownloadsQueryParams,
+) -> AppResult<ErasedJson> {
     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::<BigInt>("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::<ExtraDownload>(&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,10 +112,88 @@ 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::<Vec<_>>();
+
+        return Ok(json!({
+            "version_downloads": downloads,
+            "versions": versions,
+            "meta": {
+                "extra_downloads": extra,
+            },
+        }));
+    }
+
     Ok(json!({
         "version_downloads": downloads,
         "meta": {
             "extra_downloads": extra,
         },
     }))
 }
+
+type VersionsAndPublishers = (FullVersion, Option<User>);
+fn load_versions_and_publishers<'a>(
+    conn: &mut AsyncPgConnection,
+    versions: &'a [Version],
+    includes: bool,
+) -> BoxFuture<'a, QueryResult<Vec<VersionsAndPublishers>>> {
+    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<Vec<(VersionOwnerAction, User)>>> {
+    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<Self, Self::Err> {
+        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)
+    }
+}

src/models/action.rs

@@ -41,6 +41,7 @@ impl From<VersionAction> 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,

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,

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": {