Auto merge of #1763 - sgrif:sg-include-page-links, r=jtgeibel

Include `next_page` and `prev_page` metadata on search

As discussed in the May 30th team meeting
(https://discordapp.com/channels/442252698964721669/448525639469891595/583749739166695444),
this is the first step in deprecating/removing numbered pagination for
the "all crates" endpoint. The remaining steps are:

- Open issues/PRs on bots which have set a user agent that lets us
  identify them, have them start following this link
- 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
- Switch the links over to cursor based pagination when applicable
- Error when given pagination parameters that would cause very high
  offsets

Since the problem we're addressing only occurs when the offset is
greater than ~18k, the only place that we currently need to worry about
this is "all crates". There isn't currently any filter you can apply
that will return enough rows for this to be a problem (`?letter=` is
the most likely, but honestly that is probably the least useful
page/endpoint we have and it might just be worth removing it. Either way
AFAIK it's not hit by crawlers and humans aren't visiting page 900 in
their browsers, so even if we do end up with enough crates for the
offset to possibly be too high, it's unlikely anyone will ever hit it
there).

I'm not entirely sure how I want to structure the final logic for "give
me the next/previous page". I've just stuck it at the bottom of the
search function for now. We'll want something more structured in the
long term, but I'd like to see what this looks like with the cursor
based option before pulling it out into a more general abstraction.

Author: bors

Cargo.lock

@@ -82,6 +82,7 @@ futures = "0.1"
 tokio = "0.1"
 hyper = "0.12"
 ctrlc = { version = "3.0", features = ["termination"] }
+indexmap = "1.0.2"
 
 [dev-dependencies]
 conduit-test = "0.8"

Cargo.toml

@@ -14,14 +14,15 @@ mod prelude {
     use std::collections::HashMap;
     use std::io;
 
+    use indexmap::IndexMap;
     use serde::Serialize;
     use url;
 
     pub trait RequestUtils {
         fn redirect(&self, url: String) -> Response;
 
         fn json<T: Serialize>(&self, t: &T) -> Response;
-        fn query(&self) -> HashMap<String, String>;
+        fn query(&self) -> IndexMap<String, String>;
         fn wants_json(&self) -> bool;
         fn pagination(&self, default: usize, max: usize) -> CargoResult<(i64, i64)>;
     }
@@ -31,7 +32,7 @@ mod prelude {
             crate::util::json_response(t)
         }
 
-        fn query(&self) -> HashMap<String, String> {
+        fn query(&self) -> IndexMap<String, String> {
             url::form_urlencoded::parse(self.query_string().unwrap_or("").as_bytes())
                 .map(|(a, b)| (a.into_owned(), b.into_owned()))
                 .collect()

src/controllers.rs

@@ -235,6 +235,27 @@ pub fn search(req: &mut dyn Request) -> CargoResult<Response> {
         )
         .collect();
 
+    let mut next_page = None;
+    let mut prev_page = None;
+    let page_num = params.get("page").map(|s| s.parse()).unwrap_or(Ok(1))?;
+
+    let url_for_page = |num: i64| {
+        let mut params = req.query();
+        params.insert("page".into(), num.to_string());
+        let query_string = url::form_urlencoded::Serializer::new(String::new())
+            .clear()
+            .extend_pairs(params)
+            .finish();
+        Some(format!("?{}", query_string))
+    };
+
+    if offset + limit < total {
+        next_page = url_for_page(page_num + 1);
+    }
+    if page_num != 1 {
+        prev_page = url_for_page(page_num - 1);
+    };
+
     #[derive(Serialize)]
     struct R {
         crates: Vec<EncodableCrate>,
@@ -243,10 +264,16 @@ pub fn search(req: &mut dyn Request) -> CargoResult<Response> {
     #[derive(Serialize)]
     struct Meta {
         total: i64,
+        next_page: Option<String>,
+        prev_page: Option<String>,
     }
 
     Ok(req.json(&R {
         crates,
-        meta: Meta { total },
+        meta: Meta {
+            total,
+            next_page,
+            prev_page,
+        },
     }))
 }

src/controllers/krate/search.rs

@@ -74,6 +74,8 @@ pub struct CrateList {
 #[derive(Deserialize)]
 struct CrateMeta {
     total: i32,
+    next_page: Option<String>,
+    prev_page: Option<String>,
 }
 #[derive(Deserialize)]
 pub struct CrateResponse {

src/tests/all.rs

@@ -2215,3 +2215,26 @@ fn publish_rate_limit_doesnt_affect_existing_crates() {
     token.enqueue_publish(new_version).good();
     app.run_pending_background_jobs();
 }
+
+#[test]
+fn pagination_links_included_if_applicable() {
+    let (app, anon, user) = TestApp::init().with_user();
+    let user = user.as_model();
+
+    app.db(|conn| {
+        CrateBuilder::new("pagination_links_1", user.id).expect_build(conn);
+        CrateBuilder::new("pagination_links_2", user.id).expect_build(conn);
+        CrateBuilder::new("pagination_links_3", user.id).expect_build(conn);
+    });
+
+    let page1 = anon.search("per_page=1");
+    let page2 = anon.search("page=2&per_page=1");
+    let page3 = anon.search("page=3&per_page=1");
+
+    assert_eq!(Some("?per_page=1&page=2".to_string()), page1.meta.next_page);
+    assert_eq!(None, page1.meta.prev_page);
+    assert_eq!(Some("?page=3&per_page=1".to_string()), page2.meta.next_page);
+    assert_eq!(Some("?page=1&per_page=1".to_string()), page2.meta.prev_page);
+    assert_eq!(None, page3.meta.next_page);
+    assert_eq!(Some("?page=2&per_page=1".to_string()), page3.meta.prev_page);
+}