implement support for query parameters that allow multiple values

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 }

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())

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<Context: ServerContext> ApiDescription<Context> {
     }
 
     /// 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<Context>,
@@ -613,7 +614,7 @@ impl<Context: ServerContext> ApiDescription<Context> {
                             )?;
                         }
                         Some(SegmentOrWildcard::Wildcard) => {
-                            type_is_string_enum(
+                            type_is_string_seq(
                                 &e.operation_id,
                                 name,
                                 schema,
@@ -633,12 +634,28 @@ impl<Context: ServerContext> ApiDescription<Context> {
                             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
+                        ));
+                    }
                 }
                 _ => (),
             }

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,

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
 //!

dropshot/src/pagination.rs

@@ -650,7 +650,7 @@ mod test {
             querystring: &str,
         ) -> (T, Option<NonZeroU32>) {
             let pagparams: PaginationParams<T, MyPageSelector> =
-                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<MyScanParams, MyPageSelector>,
             >(querystring)
-            .unwrap_err()
+            .unwrap_err();
         }
 
         // missing required field ("the_field")
@@ -733,7 +733,7 @@ mod test {
             querystring: &str,
         ) -> (MyPageSelector, Option<NonZeroU32>) {
             let pagparams: PaginationParams<MyScanParams, MyPageSelector> =
-                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<SketchyScanParams, MyPageSelector> =
-            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(..) => {

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
+                )),
             }
         }
         _ => {

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<u32>,
+    #[serde(default)]
+    pub test3: Vec<String>,
 }
 #[endpoint {
     method = GET,
@@ -1194,6 +1239,8 @@ async fn demo_handler_args_2query(
 pub struct DemoJsonBody {
     pub test1: String,
     pub test2: Option<u32>,
+    #[serde(default, skip_serializing_if = "Vec::is_empty")]
+    pub test3: Vec<String>,
 }
 #[endpoint {
     method = GET,