From ed5b96246537bcae8b1075961e2a592984216348 Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Mon, 23 Feb 2026 14:26:46 -0800 Subject: [PATCH] implement support for query parameters that allow multiple values This switches our querystring parsing to use serde_qs, which seems more specifically focused on that, from serde_urlencoded. One drawback is that serde_qs permits a query parameter for which a single value is permitted to appear multiple times within the query string without reporting an error. Instead it silently accepts the last one. If we believe that this is an obstacle, I believe we could augment the `Config` object in serde_qs to optionally turn this modality into an error. --- Cargo.lock | 21 ++++- dropshot/Cargo.toml | 1 + .../examples/pagination-multiple-sorts.rs | 2 +- dropshot/src/api_description.rs | 29 +++++-- dropshot/src/extractor/query.rs | 3 +- dropshot/src/lib.rs | 14 +++- dropshot/src/pagination.rs | 13 ++- dropshot/src/type_util.rs | 10 ++- dropshot/tests/integration-tests/demo.rs | 81 +++++++++++++++---- dropshot/tests/integration-tests/openapi.rs | 19 +++++ .../tests/integration-tests/pagination.rs | 16 ++-- dropshot/tests/test_openapi.json | 32 ++++++++ dropshot/tests/test_openapi_fuller.json | 32 ++++++++ 13 files changed, 220 insertions(+), 53 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 79824e62f..68137383b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -403,6 +403,7 @@ dependencies = [ "serde", "serde_json", "serde_path_to_error", + "serde_qs", "serde_urlencoded", "sha1", "simple-mermaid", @@ -1152,9 +1153,9 @@ dependencies = [ [[package]] name = "itoa" -version = "1.0.1" +version = "1.0.17" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1aab8fc367588b89dcee83ab0fd66b72b50b72fa1904d7095045ace2b0c81c35" +checksum = "92ecc6618181def0457392ccd0ee51198e065e016d1d527a7ac1b6dc7c1f09d2" [[package]] name = "js-sys" @@ -1909,9 +1910,9 @@ checksum = "b39cdef0fa800fc44525c84ccb54a029961a8215f9619753635a9c0d2538d46d" [[package]] name = "ryu" -version = "1.0.5" +version = "1.0.23" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "71d301d4193d031abdd79ff7e3dd721168a9572ef3fe51a1517aba235bd8f86e" +checksum = "9774ba4a74de5f7b1c1451ed6cd5285a32eddb5cccb8cc655a4e50009e06477f" [[package]] name = "schannel" @@ -2092,6 +2093,18 @@ dependencies = [ "serde_core", ] +[[package]] +name = "serde_qs" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ac22439301a0b6f45a037681518e3169e8db1db76080e2e9600a08d1027df037" +dependencies = [ + "itoa", + "percent-encoding", + "ryu", + "serde", +] + [[package]] name = "serde_spanned" version = "1.0.4" diff --git a/dropshot/Cargo.toml b/dropshot/Cargo.toml index b90c72bfe..9c6449b70 100644 --- a/dropshot/Cargo.toml +++ b/dropshot/Cargo.toml @@ -35,6 +35,7 @@ scopeguard = "1.2.0" semver = "1.0.27" serde_json = "1.0.149" serde_path_to_error = "0.1.20" +serde_qs = "1.0.0" serde_urlencoded = "0.7.1" sha1 = "0.10.6" simple-mermaid = { version = "0.2.0", optional = true } diff --git a/dropshot/examples/pagination-multiple-sorts.rs b/dropshot/examples/pagination-multiple-sorts.rs index 335294b6b..7f5af162e 100644 --- a/dropshot/examples/pagination-multiple-sorts.rs +++ b/dropshot/examples/pagination-multiple-sorts.rs @@ -325,7 +325,7 @@ fn print_example_requests(log: slog::Logger, addr: &SocketAddr) { ]; for mode in all_modes { let to_print = ProjectScanParams { sort: mode }; - let query_string = serde_urlencoded::to_string(to_print).unwrap(); + let query_string = serde_qs::to_string(&to_print).unwrap(); let uri = Uri::builder() .scheme("http") .authority(addr.to_string().as_str()) diff --git a/dropshot/src/api_description.rs b/dropshot/src/api_description.rs index 23fd0de17..d7e6dd8e4 100644 --- a/dropshot/src/api_description.rs +++ b/dropshot/src/api_description.rs @@ -15,7 +15,7 @@ use crate::router::PathSegment; use crate::schema_util::j2oas_schema; use crate::server::ServerContext; use crate::type_util::type_is_scalar; -use crate::type_util::type_is_string_enum; +use crate::type_util::type_is_string_seq; use crate::CONTENT_TYPE_JSON; use crate::CONTENT_TYPE_MULTIPART_FORM_DATA; use crate::CONTENT_TYPE_OCTET_STREAM; @@ -560,8 +560,9 @@ impl ApiDescription { } /// Validate that named parameters have appropriate types and there are no - /// duplicates. Parameters must have scalar types except in the case of the - /// received for a wildcard path which must be an array of String. + /// duplicates. Path parameters must have scalar types except in the case + /// of the receiver for a wildcard path which must be an array of String. + /// Query parameters may be either scalars or a sequence of scalars. fn validate_named_parameters( &self, e: &ApiEndpoint, @@ -613,7 +614,7 @@ impl ApiDescription { )?; } Some(SegmentOrWildcard::Wildcard) => { - type_is_string_enum( + type_is_string_seq( &e.operation_id, name, schema, @@ -633,12 +634,28 @@ impl ApiDescription { name )); } - type_is_scalar( + if type_is_scalar( &e.operation_id, name, schema, dependencies, - )?; + ) + .or_else(|_| { + type_is_string_seq( + &e.operation_id, + name, + schema, + dependencies, + ) + }) + .is_err() + { + return Err(format!( + "the parameter '{}' must be either a scalar or an \ + array of scalars", + name + )); + } } _ => (), } diff --git a/dropshot/src/extractor/query.rs b/dropshot/src/extractor/query.rs index fb340479d..efd8e3c6d 100644 --- a/dropshot/src/extractor/query.rs +++ b/dropshot/src/extractor/query.rs @@ -70,8 +70,7 @@ where QueryType: DeserializeOwned + JsonSchema + Send + Sync, { let raw_query_string = request.uri().query().unwrap_or(""); - // TODO-correctness: are query strings defined to be urlencoded in this way? - match serde_urlencoded::from_str(raw_query_string) { + match serde_qs::from_str(raw_query_string) { Ok(q) => Ok(Query { inner: q }), Err(e) => Err(HttpError::for_bad_request( None, diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 01c6c10a6..15f1614b9 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -677,14 +677,20 @@ //! ``` //! //! You might expect that instead of doing this, you could define your own -//! structure that includes a `PaginationParams` using `#[serde(flatten)]`, and -//! this ought to work, but it currently doesn't due to serde_urlencoded#33, -//! which is really serde#1183. +//! structure that includes a `PaginationParams` using `#[serde(flatten)]`, +//! however--due to design limitation of `serde`--this does not work. +//! (Proximately, this is because the `limit` parameter is a number, but a +//! deserializer for the "outer" type lacks that type information so it is +//! stored in an intermediate structure as a string; when then deserializing +//! into `PaginationParams` there is a type mismatch. See serde#1183 for more +//! details.) //! //! Note that any parameters defined by `MyScanParams` are effectively encoded //! into the page token and need not be supplied with invocations when `page_token` //! is specified. That is not the case for required parameters defined by -//! `MyExtraQueryParams`--those must be supplied on each invocation. +//! `MyExtraQueryParams`--those must be supplied on each invocation. Therefore, +//! you should carefully consider how implied versus required parameters +//! interact. //! //! ### OpenAPI extension //! diff --git a/dropshot/src/pagination.rs b/dropshot/src/pagination.rs index df8128611..d96452ca5 100644 --- a/dropshot/src/pagination.rs +++ b/dropshot/src/pagination.rs @@ -650,7 +650,7 @@ mod test { querystring: &str, ) -> (T, Option) { let pagparams: PaginationParams = - serde_urlencoded::from_str(querystring).unwrap(); + serde_qs::from_str(querystring).unwrap(); let limit = pagparams.limit; let scan_params = match pagparams.page { WhichPage::Next(..) => panic!("expected first page"), @@ -709,11 +709,11 @@ mod test { // TODO-polish The actual error messages for the following cases are // pretty poor, so we don't test them here, but we should clean these // up. - fn parse_as_error(querystring: &str) -> serde_urlencoded::de::Error { - serde_urlencoded::from_str::< + fn parse_as_error(querystring: &str) { + serde_qs::from_str::< PaginationParams, >(querystring) - .unwrap_err() + .unwrap_err(); } // missing required field ("the_field") @@ -733,7 +733,7 @@ mod test { querystring: &str, ) -> (MyPageSelector, Option) { let pagparams: PaginationParams = - serde_urlencoded::from_str(querystring).unwrap(); + serde_qs::from_str(querystring).unwrap(); let limit = pagparams.limit; let page_selector = match pagparams.page { WhichPage::Next(x) => x, @@ -779,8 +779,7 @@ mod test { } let pagparams: PaginationParams = - serde_urlencoded::from_str(&format!("page_token={}", token)) - .unwrap(); + serde_qs::from_str(&format!("page_token={}", token)).unwrap(); assert_eq!(pagparams.limit, None); match &pagparams.page { WhichPage::First(..) => { diff --git a/dropshot/src/type_util.rs b/dropshot/src/type_util.rs index b76b864d7..c4ab24009 100644 --- a/dropshot/src/type_util.rs +++ b/dropshot/src/type_util.rs @@ -166,7 +166,7 @@ fn type_is_scalar_subschemas( } } -pub fn type_is_string_enum( +pub fn type_is_string_seq( operation_id: &str, name: &str, schema: &Schema, @@ -209,9 +209,11 @@ pub fn type_is_string_enum( name ) }), - _ => { - panic!("the parameter '{}' has an invalid array type", name) - } + + _ => Err(format!( + "the parameter '{}' has an invalid array type", + name + )), } } _ => { diff --git a/dropshot/tests/integration-tests/demo.rs b/dropshot/tests/integration-tests/demo.rs index 4e3bebd58..7adfe7792 100644 --- a/dropshot/tests/integration-tests/demo.rs +++ b/dropshot/tests/integration-tests/demo.rs @@ -197,24 +197,38 @@ async fn test_demo2query() { .expect_err("expected failure"); assert_eq!( error.message, - "unable to parse query string: invalid digit found in string" + "unable to parse query string: invalid type: \ + string \"bar\", expected u32" ); - // Test case: duplicated field name - let error = testctx + // Test case: duplicated field name (we take the last one) + let mut response = testctx .client_testctx .make_request( Method::GET, - "/testing/demo2query?test1=foo&test1=bar", + "/testing/demo2query?test1=overwritten&test1=foo", None as Option<()>, - StatusCode::BAD_REQUEST, + StatusCode::OK, ) .await - .expect_err("expected failure"); - assert_eq!( - error.message, - "unable to parse query string: duplicate field `test1`" - ); + .expect("expected success"); + let json: DemoJsonBody = read_json(&mut response).await; + assert_eq!(json.test1, "foo"); + + // Test case: multiple value for a query parameter sequence + let mut response = testctx + .client_testctx + .make_request( + Method::GET, + "/testing/demo2query?test1=foo&test3=bar&test3=baz", + None as Option<()>, + StatusCode::OK, + ) + .await + .expect("expected success"); + let json: DemoJsonBody = read_json(&mut response).await; + assert_eq!(json.test1, "foo"); + assert_eq!(json.test3, vec!["bar".to_string(), "baz".to_string()]); testctx.teardown().await; } @@ -228,7 +242,11 @@ async fn test_demo2json() { let testctx = common::test_setup("demo2json", api); // Test case: optional field - let input = DemoJsonBody { test1: "bar".to_string(), test2: None }; + let input = DemoJsonBody { + test1: "bar".to_string(), + test2: None, + test3: Default::default(), + }; let mut response = testctx .client_testctx .make_request( @@ -244,7 +262,11 @@ async fn test_demo2json() { assert_eq!(json.test2, None); // Test case: both fields populated - let input = DemoJsonBody { test1: "bar".to_string(), test2: Some(15) }; + let input = DemoJsonBody { + test1: "bar".to_string(), + test2: Some(15), + test3: Default::default(), + }; let mut response = testctx .client_testctx .make_request( @@ -341,7 +363,11 @@ async fn test_demo2urlencoded() { let testctx = common::test_setup("demo2urlencoded", api); // Test case: optional field - let input = DemoJsonBody { test1: "bar".to_string(), test2: None }; + let input = DemoJsonBody { + test1: "bar".to_string(), + test2: None, + test3: Default::default(), + }; let mut response = testctx .client_testctx .make_request_url_encoded( @@ -357,7 +383,11 @@ async fn test_demo2urlencoded() { assert_eq!(json.test2, None); // Test case: both fields populated - let input = DemoJsonBody { test1: "baz".to_string(), test2: Some(20) }; + let input = DemoJsonBody { + test1: "baz".to_string(), + test2: Some(20), + test3: Default::default(), + }; let mut response = testctx .client_testctx .make_request_url_encoded( @@ -373,7 +403,11 @@ async fn test_demo2urlencoded() { assert_eq!(json.test2, Some(20)); // Error case: wrong content type for endpoint - let input = DemoJsonBody { test1: "qux".to_string(), test2: Some(30) }; + let input = DemoJsonBody { + test1: "qux".to_string(), + test2: Some(30), + test3: Default::default(), + }; let error = testctx .client_testctx .make_request( @@ -441,7 +475,11 @@ async fn test_demo3json() { let testctx = common::test_setup("demo3json", api); // Test case: everything filled in. - let json_input = DemoJsonBody { test1: "bart".to_string(), test2: Some(0) }; + let json_input = DemoJsonBody { + test1: "bart".to_string(), + test2: Some(0), + test3: vec!["milhouse".to_string()], + }; let mut response = testctx .client_testctx @@ -458,9 +496,14 @@ async fn test_demo3json() { assert_eq!(json.json.test2.unwrap(), 0); assert_eq!(json.query.test1, "martin"); assert_eq!(json.query.test2.unwrap(), 2); + assert_eq!(json.json.test3, vec!["milhouse".to_string()]); // Test case: error parsing query - let json_input = DemoJsonBody { test1: "bart".to_string(), test2: Some(0) }; + let json_input = DemoJsonBody { + test1: "bart".to_string(), + test2: Some(0), + test3: Default::default(), + }; let error = testctx .client_testctx .make_request( @@ -1178,6 +1221,8 @@ async fn demo_handler_args_1( pub struct DemoQueryArgs { pub test1: String, pub test2: Option, + #[serde(default)] + pub test3: Vec, } #[endpoint { method = GET, @@ -1194,6 +1239,8 @@ async fn demo_handler_args_2query( pub struct DemoJsonBody { pub test1: String, pub test2: Option, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub test3: Vec, } #[endpoint { method = GET, diff --git a/dropshot/tests/integration-tests/openapi.rs b/dropshot/tests/integration-tests/openapi.rs index 399b2cb1c..0cd25f6f6 100644 --- a/dropshot/tests/integration-tests/openapi.rs +++ b/dropshot/tests/integration-tests/openapi.rs @@ -688,6 +688,24 @@ async fn handler32( todo!(); } +#[derive(Deserialize, JsonSchema)] +struct Query33 { + #[allow(unused)] + multi: Vec, +} + +#[endpoint { + method = GET, + path = "/testing33", + tags = ["it"] +}] +async fn handler33( + _: RequestContext<()>, + _: Query, +) -> Result { + todo!(); +} + fn make_api( maybe_tag_config: Option, ) -> Result, ApiDescriptionRegisterError> { @@ -729,6 +747,7 @@ fn make_api( api.register(handler30)?; api.register(handler31)?; api.register(handler32)?; + api.register(handler33)?; Ok(api) } diff --git a/dropshot/tests/integration-tests/pagination.rs b/dropshot/tests/integration-tests/pagination.rs index 3e443fa70..c204a45c0 100644 --- a/dropshot/tests/integration-tests/pagination.rs +++ b/dropshot/tests/integration-tests/pagination.rs @@ -202,18 +202,18 @@ async fn test_paginate_errors() { }, ErrorTestCase { path: "/intapi?limit=-3".to_string(), - message: "unable to parse query string: invalid digit found in \ - string", + message: "unable to parse query string: invalid type: \ + string \"-3\", expected a nonzero u32", }, ErrorTestCase { path: "/intapi?limit=seven".to_string(), - message: "unable to parse query string: invalid digit found in \ - string", + message: "unable to parse query string: invalid type: \ + string \"seven\", expected a nonzero u32", }, ErrorTestCase { path: format!("/intapi?limit={}", u128::from(std::u64::MAX) + 1), - message: "unable to parse query string: number too large to fit \ - in target type", + message: "unable to parse query string: invalid type: \ + string \"18446744073709551616\", expected a nonzero u32", }, ErrorTestCase { path: "/intapi?page_token=q".to_string(), @@ -377,8 +377,8 @@ async fn test_paginate_empty() { assert_error( client, "/empty?limit=0", - "unable to parse query string: invalid value: integer `0`, \ - expected a nonzero u32", + "unable to parse query string: invalid value: \ + integer `0`, expected a nonzero u32", ) .await; diff --git a/dropshot/tests/test_openapi.json b/dropshot/tests/test_openapi.json index 9dfaeb021..f1c499384 100644 --- a/dropshot/tests/test_openapi.json +++ b/dropshot/tests/test_openapi.json @@ -934,6 +934,38 @@ } } }, + "/testing33": { + "get": { + "tags": [ + "it" + ], + "operationId": "handler33", + "parameters": [ + { + "in": "query", + "name": "multi", + "required": true, + "schema": { + "type": "array", + "items": { + "type": "string" + } + } + } + ], + "responses": { + "204": { + "description": "resource updated" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/thing_with_headers": { "get": { "tags": [ diff --git a/dropshot/tests/test_openapi_fuller.json b/dropshot/tests/test_openapi_fuller.json index bfc94458b..7d58938c1 100644 --- a/dropshot/tests/test_openapi_fuller.json +++ b/dropshot/tests/test_openapi_fuller.json @@ -942,6 +942,38 @@ } } }, + "/testing33": { + "get": { + "tags": [ + "it" + ], + "operationId": "handler33", + "parameters": [ + { + "in": "query", + "name": "multi", + "required": true, + "schema": { + "type": "array", + "items": { + "type": "string" + } + } + } + ], + "responses": { + "204": { + "description": "resource updated" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/thing_with_headers": { "get": { "tags": [