Auto merge of #1807 - sgrif:sg-refactor-pagination, r=jtgeibel

Refactor pagination to prepare for cursor-based pagination.

This continues the changes made in #1763. Since that PR was merged, one
of the non-code steps has been taken care of -- All users hitting any
endpoint with `?page=20` (which is an arbitrary search pattern that
seemed high enough to give any crawlers going through pagination) have
been contacted about the change, with a PR opened against any that
included a repo. (Intersting aside, there are *zero* records of this for
any endpoint other than search, which perhaps implies we can get rid of
a few of these endpoints, but that's a separate discussion).

This PR does not change any functionality, but moves some code around
to better encapsulate things for upcoming changes. Specifically:

- Change our frontend to show "next/prev page" links on the all crates
  page
- Stop returning the "total" meta item when the next/prev page links
  will be cursor based (which I'd actually just like to start omitting
  in general)

The main goal of this change was to stop having any code outside of
`Paginated` (which has been renamed to `PaginatedQuery`, as there's a
new type called `Paginated`) care about how pagination occurs. This
means other code can't care about *how* pagination happens (with the
exception of `reverse_dependencies`, which uses raw SQL, and sorta has
to... That was a bit of a rabbit hole, see
diesel-rs/diesel#2150 for details. Given the
low traffic to that endpoint, I think we can safely ignore it).

The distribution of responsibilities is as follows:

- `PaginatedQuery<T>`: Given the query params, decides how to paginate
  things, generates appropriate SQL, loads a `Paginated<T>`.
- `Paginated<T>`: Handles providing an iterator to the records, getting
  the total count (to be removed in the near future), providing the
  next/prev page params
- `Request`: Takes the pagination related query params, turns that into
  an actual URL (note: Due to jankiness in our router, this will only
  ever be a query string, we have no way of getting the actual path)

The next step from here is to change our UI to stop showing page
numbers, and then remove the `total` field.

This PR will introduce a performance regression that was addressed by
 #1668. That PR was addressing "this will become a problem in a future",
not "this is about to take the site down". Given the intent to remove
the `total` field entirely, I think it is fine to cause this regression
in the short term. If we aren't confident that the changes to remove
this field will land in the near future, or want to be conservative
about this, I can add some additional complexity/git churn to retain the
previous performance characteristics

Author: bors

src/controllers.rs

@@ -24,7 +24,7 @@ mod prelude {
         fn json<T: Serialize>(&self, t: &T) -> Response;
         fn query(&self) -> IndexMap<String, String>;
         fn wants_json(&self) -> bool;
-        fn pagination(&self, default: usize, max: usize) -> CargoResult<(i64, i64)>;
+        fn query_with_params(&self, params: IndexMap<String, String>) -> String;
     }
 
     impl<'a> RequestUtils for dyn Request + 'a {
@@ -55,26 +55,13 @@ mod prelude {
                 .unwrap_or(false)
         }
 
-        fn pagination(&self, default: usize, max: usize) -> CargoResult<(i64, i64)> {
-            let query = self.query();
-            let page = query
-                .get("page")
-                .and_then(|s| s.parse::<usize>().ok())
-                .unwrap_or(1);
-            let limit = query
-                .get("per_page")
-                .and_then(|s| s.parse::<usize>().ok())
-                .unwrap_or(default);
-            if limit > max {
-                return Err(human(&format_args!(
-                    "cannot request more than {} items",
-                    max
-                )));
-            }
-            if page == 0 {
-                return Err(human("page indexing starts from 1, page 0 is invalid"));
-            }
-            Ok((((page - 1) * limit) as i64, limit as i64))
+        fn query_with_params(&self, new_params: IndexMap<String, String>) -> String {
+            let mut params = self.query().clone();
+            params.extend(new_params);
+            let query_string = url::form_urlencoded::Serializer::new(String::new())
+                .extend_pairs(params)
+                .finish();
+            format!("?{}", query_string)
         }
     }
 }

src/controllers/category.rs

@@ -1,3 +1,4 @@
+use super::helpers::pagination::*;
 use super::prelude::*;
 
 use crate::models::Category;
@@ -7,11 +8,16 @@ use crate::views::{EncodableCategory, EncodableCategoryWithSubcategories};
 /// Handles the `GET /categories` route.
 pub fn index(req: &mut dyn Request) -> CargoResult<Response> {
     let conn = req.db_conn()?;
-    let (offset, limit) = req.pagination(10, 100)?;
     let query = req.query();
+    // FIXME: There are 69 categories, 47 top level. This isn't going to
+    // grow by an OoM. We need a limit for /summary, but we don't need
+    // to paginate this.
+    let options = PaginationOptions::new(&query)?;
+    let offset = options.offset().unwrap_or_default();
     let sort = query.get("sort").map_or("alpha", String::as_str);
 
-    let categories = Category::toplevel(&conn, sort, limit, offset)?;
+    let categories =
+        Category::toplevel(&conn, sort, i64::from(options.per_page), i64::from(offset))?;
     let categories = categories.into_iter().map(Category::encodable).collect();
 
     // Query for the total count of categories

src/controllers/helpers.rs

@@ -1,9 +1,9 @@
 use crate::util::{json_response, CargoResult};
 use conduit::Response;
 
-pub mod pagination;
+pub(crate) mod pagination;
 
-pub use self::pagination::Paginate;
+pub(crate) use self::pagination::Paginate;
 
 pub fn ok_true() -> CargoResult<Response> {
     #[derive(Serialize)]

src/controllers/helpers/pagination.rs

@@ -1,44 +1,184 @@
+use crate::models::helpers::with_count::*;
+use crate::util::errors::*;
 use diesel::pg::Pg;
 use diesel::prelude::*;
 use diesel::query_builder::*;
+use diesel::query_dsl::LoadQuery;
 use diesel::sql_types::BigInt;
+use indexmap::IndexMap;
+
+#[derive(Debug, Clone, Copy)]
+pub(crate) enum Page {
+    Numeric(u32),
+    Unspecified,
+}
+
+impl Page {
+    fn new(params: &IndexMap<String, String>) -> CargoResult<Self> {
+        if let Some(s) = params.get("page") {
+            let numeric_page = s.parse()?;
+            if numeric_page < 1 {
+                return Err(human(&format_args!(
+                    "page indexing starts from 1, page {} is invalid",
+                    numeric_page,
+                )));
+            }
+
+            Ok(Page::Numeric(numeric_page))
+        } else {
+            Ok(Page::Unspecified)
+        }
+    }
+}
+
+#[derive(Debug, Clone, Copy)]
+pub(crate) struct PaginationOptions {
+    page: Page,
+    pub(crate) per_page: u32,
+}
+
+impl PaginationOptions {
+    pub(crate) fn new(params: &IndexMap<String, String>) -> CargoResult<Self> {
+        const DEFAULT_PER_PAGE: u32 = 10;
+        const MAX_PER_PAGE: u32 = 100;
+
+        let per_page = params
+            .get("per_page")
+            .map(|s| s.parse())
+            .unwrap_or(Ok(DEFAULT_PER_PAGE))?;
+
+        if per_page > MAX_PER_PAGE {
+            return Err(human(&format_args!(
+                "cannot request more than {} items",
+                MAX_PER_PAGE,
+            )));
+        }
+
+        Ok(Self {
+            page: Page::new(params)?,
+            per_page,
+        })
+    }
+
+    pub(crate) fn offset(&self) -> Option<u32> {
+        if let Page::Numeric(p) = self.page {
+            Some((p - 1) * self.per_page)
+        } else {
+            None
+        }
+    }
+}
+
+pub(crate) trait Paginate: Sized {
+    fn paginate(self, params: &IndexMap<String, String>) -> CargoResult<PaginatedQuery<Self>> {
+        Ok(PaginatedQuery {
+            query: self,
+            options: PaginationOptions::new(params)?,
+        })
+    }
+}
+
+impl<T> Paginate for T {}
 
-#[derive(Debug, QueryId)]
 pub struct Paginated<T> {
-    query: T,
-    limit: i64,
-    offset: i64,
+    records_and_total: Vec<WithCount<T>>,
+    options: PaginationOptions,
 }
 
-pub trait Paginate: AsQuery + Sized {
-    fn paginate(self, limit: i64, offset: i64) -> Paginated<Self::Query> {
-        Paginated {
-            query: self.as_query(),
-            limit,
-            offset,
+impl<T> Paginated<T> {
+    pub(crate) fn total(&self) -> Option<i64> {
+        Some(
+            self.records_and_total
+                .get(0)
+                .map(|row| row.total)
+                .unwrap_or_default(),
+        )
+    }
+
+    pub(crate) fn next_page_params(&self) -> Option<IndexMap<String, String>> {
+        if self.records_and_total.len() < self.options.per_page as usize {
+            return None;
         }
+
+        let mut opts = IndexMap::new();
+        match self.options.page {
+            Page::Numeric(n) => opts.insert("page".into(), (n + 1).to_string()),
+            Page::Unspecified => opts.insert("page".into(), 2.to_string()),
+        };
+        Some(opts)
+    }
+
+    pub(crate) fn prev_page_params(&self) -> Option<IndexMap<String, String>> {
+        if let Page::Numeric(1) | Page::Unspecified = self.options.page {
+            return None;
+        }
+
+        let mut opts = IndexMap::new();
+        match self.options.page {
+            Page::Numeric(n) => opts.insert("page".into(), (n - 1).to_string()),
+            Page::Unspecified => unreachable!(),
+        };
+        Some(opts)
+    }
+
+    pub(crate) fn iter(&self) -> impl Iterator<Item = &T> {
+        self.records_and_total.iter().map(|row| &row.record)
+    }
+}
+
+impl<T: 'static> IntoIterator for Paginated<T> {
+    type IntoIter = Box<dyn Iterator<Item = Self::Item>>;
+    type Item = T;
+
+    fn into_iter(self) -> Self::IntoIter {
+        Box::new(self.records_and_total.into_iter().map(|row| row.record))
     }
 }
 
-impl<T: AsQuery> Paginate for T {}
+#[derive(Debug)]
+pub(crate) struct PaginatedQuery<T> {
+    query: T,
+    options: PaginationOptions,
+}
 
-impl<T: Query> Query for Paginated<T> {
+impl<T> PaginatedQuery<T> {
+    pub(crate) fn load<U>(self, conn: &PgConnection) -> QueryResult<Paginated<U>>
+    where
+        Self: LoadQuery<PgConnection, WithCount<U>>,
+    {
+        let options = self.options;
+        let records_and_total = self.internal_load(conn)?;
+        Ok(Paginated {
+            records_and_total,
+            options,
+        })
+    }
+}
+
+impl<T> QueryId for PaginatedQuery<T> {
+    const HAS_STATIC_QUERY_ID: bool = false;
+    type QueryId = ();
+}
+
+impl<T: Query> Query for PaginatedQuery<T> {
     type SqlType = (T::SqlType, BigInt);
 }
 
-impl<T, DB> RunQueryDsl<DB> for Paginated<T> {}
+impl<T, DB> RunQueryDsl<DB> for PaginatedQuery<T> {}
 
-impl<T> QueryFragment<Pg> for Paginated<T>
+impl<T> QueryFragment<Pg> for PaginatedQuery<T>
 where
     T: QueryFragment<Pg>,
 {
     fn walk_ast(&self, mut out: AstPass<'_, Pg>) -> QueryResult<()> {
         out.push_sql("SELECT *, COUNT(*) OVER () FROM (");
         self.query.walk_ast(out.reborrow())?;
         out.push_sql(") t LIMIT ");
-        out.push_bind_param::<BigInt, _>(&self.limit)?;
-        out.push_sql(" OFFSET ");
-        out.push_bind_param::<BigInt, _>(&self.offset)?;
+        out.push_bind_param::<BigInt, _>(&i64::from(self.options.per_page))?;
+        if let Some(offset) = self.options.offset() {
+            out.push_sql(" OFFSET ");
+            out.push_bind_param::<BigInt, _>(&i64::from(offset))?;
+        }
         Ok(())
     }
 }

src/controllers/keyword.rs

@@ -9,7 +9,6 @@ pub fn index(req: &mut dyn Request) -> CargoResult<Response> {
     use crate::schema::keywords;
 
     let conn = req.db_conn()?;
-    let (offset, limit) = req.pagination(10, 100)?;
     let query = req.query();
     let sort = query.get("sort").map(|s| &s[..]).unwrap_or("alpha");
 
@@ -21,14 +20,9 @@ pub fn index(req: &mut dyn Request) -> CargoResult<Response> {
         query = query.order(keywords::keyword.asc());
     }
 
-    let data = query
-        .paginate(limit, offset)
-        .load::<(Keyword, i64)>(&*conn)?;
-    let total = data.get(0).map(|&(_, t)| t).unwrap_or(0);
-    let kws = data
-        .into_iter()
-        .map(|(k, _)| k.encodable())
-        .collect::<Vec<_>>();
+    let data = query.paginate(&req.query())?.load::<Keyword>(&*conn)?;
+    let total = data.total();
+    let kws = data.into_iter().map(Keyword::encodable).collect::<Vec<_>>();
 
     #[derive(Serialize)]
     struct R {
@@ -37,7 +31,7 @@ pub fn index(req: &mut dyn Request) -> CargoResult<Response> {
     }
     #[derive(Serialize)]
     struct Meta {
-        total: i64,
+        total: Option<i64>,
     }
 
     Ok(req.json(&R {

src/controllers/krate/metadata.rs

@@ -212,8 +212,7 @@ pub fn reverse_dependencies(req: &mut dyn Request) -> CargoResult<Response> {
     let name = &req.params()["crate_id"];
     let conn = req.db_conn()?;
     let krate = Crate::by_name(name).first::<Crate>(&*conn)?;
-    let (offset, limit) = req.pagination(10, 100)?;
-    let (rev_deps, total) = krate.reverse_dependencies(&*conn, offset, limit)?;
+    let (rev_deps, total) = krate.reverse_dependencies(&*conn, &req.query())?;
     let rev_deps: Vec<_> = rev_deps
         .into_iter()
         .map(|dep| dep.encodable(&krate.name))