From 43c5a4168d6afed2e92c98555e025b2b95012758 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Mon, 10 Jun 2019 15:53:48 -0600 Subject: [PATCH 1/2] 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. --- Cargo.lock | 1 + Cargo.toml | 1 + src/controllers.rs | 5 +++-- src/controllers/krate/search.rs | 27 ++++++++++++++++++++++++++- src/tests/all.rs | 2 ++ src/tests/krate.rs | 26 ++++++++++++++++++++++++++ 6 files changed, 59 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d4858a82fcd..968842d8d33 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -200,6 +200,7 @@ dependencies = [ "htmlescape 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "hyper 0.12.25 (registry+https://github.com/rust-lang/crates.io-index)", "hyper-tls 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", + "indexmap 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)", "jemalloc-ctl 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", "jemallocator 0.1.9 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/Cargo.toml b/Cargo.toml index ca9b3454102..3332014e8d9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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" diff --git a/src/controllers.rs b/src/controllers.rs index a8fe4eab65a..e8f9d68b847 100644 --- a/src/controllers.rs +++ b/src/controllers.rs @@ -15,13 +15,14 @@ mod prelude { use std::io; use serde::Serialize; + use indexmap::IndexMap; use url; pub trait RequestUtils { fn redirect(&self, url: String) -> Response; fn json(&self, t: &T) -> Response; - fn query(&self) -> HashMap; + fn query(&self) -> IndexMap; 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 { + fn query(&self) -> IndexMap { url::form_urlencoded::parse(self.query_string().unwrap_or("").as_bytes()) .map(|(a, b)| (a.into_owned(), b.into_owned())) .collect() diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 8120b3e860b..894309fff4e 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -222,6 +222,29 @@ pub fn search(req: &mut dyn Request) -> CargoResult { ) .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, @@ -230,10 +253,12 @@ pub fn search(req: &mut dyn Request) -> CargoResult { #[derive(Serialize)] struct Meta { total: i64, + next_page: Option, + prev_page: Option, } Ok(req.json(&R { crates, - meta: Meta { total }, + meta: Meta { total, next_page, prev_page }, })) } diff --git a/src/tests/all.rs b/src/tests/all.rs index c535e4cc384..00176c56b31 100644 --- a/src/tests/all.rs +++ b/src/tests/all.rs @@ -93,6 +93,8 @@ pub struct CrateList { #[derive(Deserialize)] struct CrateMeta { total: i32, + next_page: Option, + prev_page: Option, } #[derive(Deserialize)] pub struct CrateResponse { diff --git a/src/tests/krate.rs b/src/tests/krate.rs index 0ad371f5408..c84a1d18ff7 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -2172,3 +2172,29 @@ 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); +} From cf5c284d4a70ea5ff2aaac767968fde7e382bc63 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Tue, 11 Jun 2019 17:28:06 -0600 Subject: [PATCH 2/2] rustfmt --- src/controllers.rs | 2 +- src/controllers/krate/search.rs | 10 ++++++---- src/tests/krate.rs | 9 +++------ 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/controllers.rs b/src/controllers.rs index e8f9d68b847..06cbafa0600 100644 --- a/src/controllers.rs +++ b/src/controllers.rs @@ -14,8 +14,8 @@ mod prelude { use std::collections::HashMap; use std::io; - use serde::Serialize; use indexmap::IndexMap; + use serde::Serialize; use url; pub trait RequestUtils { diff --git a/src/controllers/krate/search.rs b/src/controllers/krate/search.rs index 894309fff4e..ac8684a7175 100644 --- a/src/controllers/krate/search.rs +++ b/src/controllers/krate/search.rs @@ -224,9 +224,7 @@ pub fn search(req: &mut dyn Request) -> CargoResult { 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 page_num = params.get("page").map(|s| s.parse()).unwrap_or(Ok(1))?; let url_for_page = |num: i64| { let mut params = req.query(); @@ -259,6 +257,10 @@ pub fn search(req: &mut dyn Request) -> CargoResult { Ok(req.json(&R { crates, - meta: Meta { total, next_page, prev_page }, + meta: Meta { + total, + next_page, + prev_page, + }, })) } diff --git a/src/tests/krate.rs b/src/tests/krate.rs index c84a1d18ff7..e6ac93e9248 100644 --- a/src/tests/krate.rs +++ b/src/tests/krate.rs @@ -2179,12 +2179,9 @@ fn pagination_links_included_if_applicable() { 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); + 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");