diff --git a/src/controllers/helpers/pagination.rs b/src/controllers/helpers/pagination.rs index 2565aa88211..3100e121684 100644 --- a/src/controllers/helpers/pagination.rs +++ b/src/controllers/helpers/pagination.rs @@ -4,7 +4,8 @@ use crate::middleware::log_request::RequestLogExt; use crate::middleware::real_ip::RealIp; use crate::models::helpers::with_count::*; use crate::util::errors::{bad_request, AppResult}; -use crate::util::{HeaderMapExt, RequestUtils}; +use crate::util::HeaderMapExt; +use std::num::NonZeroU32; use base64::{engine::general_purpose, Engine}; use diesel::pg::Pg; @@ -77,28 +78,30 @@ impl PaginationOptionsBuilder { } pub(crate) fn gather(self, parts: &Parts) -> AppResult { - let params = parts.query(); - let page_param = params.get("page"); - let seek_param = params.get("seek"); + use axum::extract::Query; - if seek_param.is_some() && page_param.is_some() { + #[derive(Debug, Deserialize)] + struct QueryParams { + page: Option, + per_page: Option, + seek: Option, + } + + let Query(params) = Query::::try_from_uri(&parts.uri) + .map_err(|err| bad_request(err.body_text()))?; + + if params.seek.is_some() && params.page.is_some() { return Err(bad_request( "providing both ?page= and ?seek= is unsupported", )); } - let page = if let Some(s) = page_param { + let page = if let Some(s) = params.page { if !self.enable_pages { return Err(bad_request("?page= is not supported for this request")); } - let numeric_page = s.parse().map_err(bad_request)?; - if numeric_page < 1 { - return Err(bad_request(format_args!( - "page indexing starts from 1, page {numeric_page} is invalid", - ))); - } - + let numeric_page = s.get(); if numeric_page > MAX_PAGE_BEFORE_SUSPECTED_BOT { parts.request_log().add("bot", "suspected"); } @@ -119,28 +122,22 @@ impl PaginationOptionsBuilder { } Page::Numeric(numeric_page) - } else if let Some(s) = seek_param { + } else if let Some(s) = params.seek { if !self.enable_seek { return Err(bad_request("?seek= is not supported for this request")); } - Page::Seek(RawSeekPayload(s.clone())) + Page::Seek(RawSeekPayload(s)) } else { Page::Unspecified }; - let per_page = params - .get("per_page") - .map(|s| s.parse().map_err(bad_request)) - .unwrap_or(Ok(DEFAULT_PER_PAGE))?; + let per_page = params.per_page.map(|p| p.get() as i64); + let per_page = per_page.unwrap_or(DEFAULT_PER_PAGE); if per_page > MAX_PER_PAGE { return Err(bad_request(format_args!( "cannot request more than {MAX_PER_PAGE} items", ))); - } else if per_page < 1 { - return Err(bad_request(format_args!( - "cannot request less than 1 item, per_page {per_page} is invalid", - ))); } Ok(PaginationOptions { page, per_page }) @@ -529,6 +526,7 @@ pub(crate) use seek; mod tests { use super::*; use http::{Method, Request, StatusCode}; + use insta::assert_snapshot; #[test] fn no_pagination_param() { @@ -539,13 +537,12 @@ mod tests { #[test] fn page_param_parsing() { - let assert_error = - |query, msg| assert_pagination_error(PaginationOptions::builder(), query, msg); + let error = |query| pagination_error(PaginationOptions::builder(), query); - assert_error("page=", "cannot parse integer from empty string"); - assert_error("page=not_a_number", "invalid digit found in string"); - assert_error("page=1.0", "invalid digit found in string"); - assert_error("page=0", "page indexing starts from 1, page 0 is invalid"); + assert_snapshot!(error("page="), @"Failed to deserialize query string: cannot parse integer from empty string"); + assert_snapshot!(error("page=not_a_number"), @"Failed to deserialize query string: invalid digit found in string"); + assert_snapshot!(error("page=1.0"), @"Failed to deserialize query string: invalid digit found in string"); + assert_snapshot!(error("page=0"), @"Failed to deserialize query string: invalid value: integer `0`, expected a nonzero u32"); let pagination = PaginationOptions::builder() .gather(&mock("page=5")) @@ -555,17 +552,13 @@ mod tests { #[test] fn per_page_param_parsing() { - let assert_error = - |query, msg| assert_pagination_error(PaginationOptions::builder(), query, msg); - - assert_error("per_page=", "cannot parse integer from empty string"); - assert_error("per_page=not_a_number", "invalid digit found in string"); - assert_error("per_page=1.0", "invalid digit found in string"); - assert_error("per_page=101", "cannot request more than 100 items"); - assert_error( - "per_page=0", - "cannot request less than 1 item, per_page 0 is invalid", - ); + let error = |query| pagination_error(PaginationOptions::builder(), query); + + assert_snapshot!(error("per_page="), @"Failed to deserialize query string: cannot parse integer from empty string"); + assert_snapshot!(error("per_page=not_a_number"), @"Failed to deserialize query string: invalid digit found in string"); + assert_snapshot!(error("per_page=1.0"), @"Failed to deserialize query string: invalid digit found in string"); + assert_snapshot!(error("per_page=101"), @"cannot request more than 100 items"); + assert_snapshot!(error("per_page=0"), @"Failed to deserialize query string: invalid value: integer `0`, expected a nonzero u32"); let pagination = PaginationOptions::builder() .gather(&mock("per_page=5")) @@ -575,11 +568,8 @@ mod tests { #[test] fn seek_param_parsing() { - assert_pagination_error( - PaginationOptions::builder(), - "seek=OTg", - "?seek= is not supported for this request", - ); + let error = pagination_error(PaginationOptions::builder(), "seek=OTg"); + assert_snapshot!(error, @"?seek= is not supported for this request"); let pagination = PaginationOptions::builder() .enable_seek(true) @@ -598,25 +588,20 @@ mod tests { #[test] fn both_page_and_seek() { - assert_pagination_error( - PaginationOptions::builder(), - "page=1&seek=OTg", - "providing both ?page= and ?seek= is unsupported", - ); - assert_pagination_error( + let error = pagination_error(PaginationOptions::builder(), "page=1&seek=OTg"); + assert_snapshot!(error, @"providing both ?page= and ?seek= is unsupported"); + + let error = pagination_error( PaginationOptions::builder().enable_seek(true), "page=1&seek=OTg", - "providing both ?page= and ?seek= is unsupported", ); + assert_snapshot!(error, @"providing both ?page= and ?seek= is unsupported"); } #[test] fn disabled_pages() { - assert_pagination_error( - PaginationOptions::builder().enable_pages(false), - "page=1", - "?page= is not supported for this request", - ); + let error = pagination_error(PaginationOptions::builder().enable_pages(false), "page=1"); + assert_snapshot!(error, @"?page= is not supported for this request"); } #[test] @@ -749,11 +734,12 @@ mod tests { .0 } - fn assert_pagination_error(options: PaginationOptionsBuilder, query: &str, message: &str) { + fn pagination_error(options: PaginationOptionsBuilder, query: &str) -> String { let error = options.gather(&mock(query)).unwrap_err(); - assert_eq!(error.to_string(), message); let response = error.response(); assert_eq!(response.status(), StatusCode::BAD_REQUEST); + + error.to_string() } } diff --git a/src/tests/routes/crates/list.rs b/src/tests/routes/crates/list.rs index 9e586f7e3f7..1ea73b4a194 100644 --- a/src/tests/routes/crates/list.rs +++ b/src/tests/routes/crates/list.rs @@ -1172,13 +1172,13 @@ async fn pagination_parameters_only_accept_integers() { .get_with_query::<()>("/api/v1/crates", "page=1&per_page=100%22%EF%BC%8Cexception") .await; assert_eq!(response.status(), StatusCode::BAD_REQUEST); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"invalid digit found in string"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"Failed to deserialize query string: invalid digit found in string"}]}"#); let response = anon .get_with_query::<()>("/api/v1/crates", "page=100%22%EF%BC%8Cexception&per_page=1") .await; assert_eq!(response.status(), StatusCode::BAD_REQUEST); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"invalid digit found in string"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"Failed to deserialize query string: invalid digit found in string"}]}"#); } #[tokio::test(flavor = "multi_thread")] diff --git a/src/tests/routes/me/updates.rs b/src/tests/routes/me/updates.rs index 53ba84f7d44..c9936dbcefc 100644 --- a/src/tests/routes/me/updates.rs +++ b/src/tests/routes/me/updates.rs @@ -103,5 +103,5 @@ async fn following() { .get_with_query::<()>("/api/v1/me/updates", "page=0") .await; assert_eq!(response.status(), StatusCode::BAD_REQUEST); - assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"page indexing starts from 1, page 0 is invalid"}]}"#); + assert_snapshot!(response.text(), @r#"{"errors":[{"detail":"Failed to deserialize query string: invalid value: integer `0`, expected a nonzero u32"}]}"#); }