Paginate only when per_page parameter is present

Author: Marco Napetti

src/controllers/helpers/pagination.rs

@@ -73,7 +73,7 @@ impl PaginationOptionsBuilder {
         self
     }
 
-    pub(crate) fn gather(self, req: &mut dyn RequestExt) -> AppResult<PaginationOptions> {
+    pub(crate) fn gather(self, req: &dyn RequestExt) -> AppResult<PaginationOptions> {
         let params = req.query();
         let page_param = params.get("page");
         let seek_param = params.get("seek");
@@ -315,7 +315,7 @@ mod tests {
 
     #[test]
     fn no_pagination_param() {
-        let pagination = PaginationOptions::builder().gather(&mut mock("")).unwrap();
+        let pagination = PaginationOptions::builder().gather(&mock("")).unwrap();
         assert_eq!(Page::Unspecified, pagination.page);
         assert_eq!(DEFAULT_PER_PAGE, pagination.per_page);
     }
@@ -331,7 +331,7 @@ mod tests {
         assert_error("page=0", "page indexing starts from 1, page 0 is invalid");
 
         let pagination = PaginationOptions::builder()
-            .gather(&mut mock("page=5"))
+            .gather(&mock("page=5"))
             .unwrap();
         assert_eq!(Page::Numeric(5), pagination.page);
     }
@@ -347,7 +347,7 @@ mod tests {
         assert_error("per_page=101", "cannot request more than 100 items");
 
         let pagination = PaginationOptions::builder()
-            .gather(&mut mock("per_page=5"))
+            .gather(&mock("per_page=5"))
             .unwrap();
         assert_eq!(5, pagination.per_page);
     }
@@ -362,7 +362,7 @@ mod tests {
 
         let pagination = PaginationOptions::builder()
             .enable_seek(true)
-            .gather(&mut mock("seek=OTg"))
+            .gather(&mock("seek=OTg"))
             .unwrap();
 
         if let Page::Seek(raw) = pagination.page {
@@ -420,7 +420,7 @@ mod tests {
 
     fn assert_pagination_error(options: PaginationOptionsBuilder, query: &str, message: &str) {
         let response = options
-            .gather(&mut mock(query))
+            .gather(&mock(query))
             .unwrap_err()
             .response()
             .unwrap();

src/controllers/krate/metadata.rs

@@ -326,20 +326,35 @@ pub fn readme(req: &mut dyn RequestExt) -> EndpointResult {
 // FIXME: Not sure why this is necessary since /crates/:crate_id returns
 // this information already, but ember is definitely requesting it
 pub fn versions(req: &mut dyn RequestExt) -> EndpointResult {
-    let pagination_options = PaginationOptions::builder().gather(req)?;
     let crate_name = &req.params()["crate_id"];
     let conn = req.db_read()?;
     let krate: Crate = Crate::by_name(crate_name).first(&*conn)?;
-    let data: Paginated<(Version, Option<User>)> = krate
-        .all_versions()
-        .left_outer_join(users::table)
-        .select((versions::all_columns, users::all_columns.nullable()))
-        .order(versions::created_at.desc())
-        .pages_pagination(pagination_options)
-        .load(&*conn)?;
-    let more = data.next_page_params().is_some();
 
-    let mut versions_and_publishers = data.into_iter().collect::<Vec<_>>();
+    // to keep retrocompatibility, we paginate only if per_page parameter is present
+    let (mut versions_and_publishers, meta) = if req.query().get("per_page").is_some() {
+        let pagination_options = PaginationOptions::builder().gather(req)?;
+        let data: Paginated<(Version, Option<User>)> = krate
+            .all_versions()
+            .left_outer_join(users::table)
+            .select((versions::all_columns, users::all_columns.nullable()))
+            .order(versions::created_at.desc())
+            .pages_pagination(pagination_options)
+            .load(&*conn)?;
+        let more = data.next_page_params().is_some();
+
+        (
+            data.into_iter().collect::<Vec<_>>(),
+            Some(json!({ "more": more })),
+        )
+    } else {
+        let data: Vec<(Version, Option<User>)> = krate
+            .all_versions()
+            .left_outer_join(users::table)
+            .select((versions::all_columns, users::all_columns.nullable()))
+            .load(&*conn)?;
+        (data, None)
+    };
+
     versions_and_publishers
         .sort_by_cached_key(|(version, _)| Reverse(semver::Version::parse(&version.num).ok()));
 
@@ -354,10 +369,18 @@ pub fn versions(req: &mut dyn RequestExt) -> EndpointResult {
         .map(|((v, pb), aas)| EncodableVersion::from(v, crate_name, pb, aas))
         .collect::<Vec<_>>();
 
-    Ok(req.json(&json!({
-        "versions": versions,
-        "meta": { "more": more },
-    })))
+    // probably here would be simplier with a concrete response type
+    let response = if let Some(meta) = meta {
+        json!({
+            "versions": versions,
+            "meta": meta,
+        })
+    } else {
+        json!({
+            "versions": versions,
+        })
+    };
+    Ok(req.json(&response))
 }
 
 /// Handles the `GET /crates/:crate_id/reverse_dependencies` route.