diff --git a/Cargo.lock b/Cargo.lock index 384c1f43..df968e6f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -559,6 +559,7 @@ dependencies = [ "serde", "serde_json", "serde_path_to_error", + "serde_qs", "serde_urlencoded", "sha1 0.11.0", "simple-mermaid", @@ -1285,9 +1286,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 = "jni" @@ -2052,9 +2053,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 = "same-file" @@ -2244,6 +2245,18 @@ dependencies = [ "serde_core", ] +[[package]] +name = "serde_qs" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c2316d01592c3382277c5062105510e35e0a6bfb2851e30028485f7af8cf1240" +dependencies = [ + "itoa", + "percent-encoding", + "ryu", + "serde", +] + [[package]] name = "serde_spanned" version = "1.1.1" diff --git a/dropshot/Cargo.toml b/dropshot/Cargo.toml index 15576849..99458d11 100644 --- a/dropshot/Cargo.toml +++ b/dropshot/Cargo.toml @@ -37,6 +37,7 @@ scopeguard = "1.2.0" semver = "1.0.28" serde_json = "1.0.149" serde_path_to_error = "0.1.20" +serde_qs = "1.1.1" serde_urlencoded = "0.7.1" sha1 = "0.11.0" 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 08903e63..fdb53fcc 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 cbbe0163..9cdbcc61 100644 --- a/dropshot/src/api_description.rs +++ b/dropshot/src/api_description.rs @@ -19,7 +19,7 @@ use crate::router::route_path_to_segments; 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 http::Method; use http::StatusCode; @@ -561,8 +561,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, @@ -614,7 +615,7 @@ impl ApiDescription { )?; } Some(SegmentOrWildcard::Wildcard) => { - type_is_string_enum( + type_is_string_seq( &e.operation_id, name, schema, @@ -634,12 +635,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 c9d93e57..6c246e8c 100644 --- a/dropshot/src/extractor/query.rs +++ b/dropshot/src/extractor/query.rs @@ -69,9 +69,13 @@ fn http_request_load_query( where QueryType: DeserializeOwned + JsonSchema + Send + Sync, { + // We do allow duplicate keys for sequence-type parameters (e.g. a Vec), + // but don't want duplicate keys ignored for scalar parameters. + const QS_CONFIG: serde_qs::Config = serde_qs::Config::new() + .duplicate_key_behavior(serde_qs::DuplicateKeyBehavior::Error); + 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 QS_CONFIG.deserialize_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 2cee83d0..e4769f1d 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 4f6f8797..26c4af0f 100644 --- a/dropshot/src/pagination.rs +++ b/dropshot/src/pagination.rs @@ -651,7 +651,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"), @@ -710,11 +710,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") @@ -734,7 +734,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, @@ -780,8 +780,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 8ef47d05..c7e2863e 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 98ae7ec5..a18fb6eb 100644 --- a/dropshot/tests/integration-tests/demo.rs +++ b/dropshot/tests/integration-tests/demo.rs @@ -197,7 +197,8 @@ 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 @@ -213,9 +214,24 @@ async fn test_demo2query() { .expect_err("expected failure"); assert_eq!( error.message, - "unable to parse query string: duplicate field `test1`" + "unable to parse query string: multiple values provided for non-sequence field" ); + // 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 +244,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 +264,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 +365,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 +385,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 +405,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 +477,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 +498,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 +1223,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 +1241,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 170297f5..0ee0f095 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 7114c6af..b636fb1d 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 bf49feec..baa3fce5 100644 --- a/dropshot/tests/test_openapi.json +++ b/dropshot/tests/test_openapi.json @@ -935,6 +935,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 ffc25577..8e3eddc8 100644 --- a/dropshot/tests/test_openapi_fuller.json +++ b/dropshot/tests/test_openapi_fuller.json @@ -943,6 +943,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": [