Add required-without-page_token information to pagination extension

CHANGELOG.adoc

@@ -17,7 +17,11 @@ https://github.com/oxidecomputer/dropshot/compare/v0.9.0\...HEAD[Full list of co
 
 === Breaking Changes
 
-* https://github.com/oxidecomputer/dropshot/pull/651[#651]  The address of the remote peer is now available to request handlers via the `RequestInfo` struct. With this change we've removed the related `From<hyper::Request<B>>` implementation; instead use `RequestInfo::new<B>(&hyper::Request<B>, std::net::SocketAddr)`.
+* https://github.com/oxidecomputer/dropshot/pull/651[#651] The address of the remote peer is now available to request handlers via the `RequestInfo` struct. With this change we've removed the related `From<hyper::Request<B>>` implementation; instead use `RequestInfo::new<B>(&hyper::Request<B>, std::net::SocketAddr)`.
+
+=== Other notable Changes
+
+* https://github.com/oxidecomputer/dropshot/pull/660[#660] The `x-dropshot-pagination` extension used to be simply the value `true`. Now it is an object with a field, `required`, that is an array of parameters that are mandatory on the first invocation.
 
 == 0.9.0 (released 2023-01-20)
 

dropshot/src/api_description.rs

@@ -670,12 +670,12 @@ impl<Context: ServerContext> ApiDescription<Context> {
                 })
                 .next();
 
-            match endpoint.extension_mode {
+            match &endpoint.extension_mode {
                 ExtensionMode::None => {}
-                ExtensionMode::Paginated => {
+                ExtensionMode::Paginated(first_page_schema) => {
                     operation.extensions.insert(
                         crate::pagination::PAGINATION_EXTENSION.to_string(),
-                        serde_json::json! {true},
+                        first_page_schema.clone(),
                     );
                 }
                 ExtensionMode::Websocket => {
@@ -1105,11 +1105,11 @@ pub struct TagExternalDocs {
 }
 
 /// Dropshot/Progenitor features used by endpoints which are not a part of the base OpenAPI spec.
-#[derive(Copy, Clone, Debug, Default, Eq, PartialEq)]
+#[derive(Clone, Debug, Default, Eq, PartialEq)]
 pub enum ExtensionMode {
     #[default]
     None,
-    Paginated,
+    Paginated(serde_json::Value),
     Websocket,
 }
 

dropshot/src/extractor/metadata.rs

@@ -14,7 +14,7 @@ use schemars::JsonSchema;
 
 /// Convenience function to generate parameter metadata from types implementing
 /// `JsonSchema` for use with `Query` and `Path` `Extractors`.
-pub fn get_metadata<ParamType>(
+pub(crate) fn get_metadata<ParamType>(
     loc: &ApiEndpointParameterLocation,
 ) -> ExtractorMetadata
 where
@@ -29,16 +29,17 @@ where
 
     let extension_mode = match schema_extensions(&schema) {
         Some(extensions) => {
-            let paginated = extensions
-                .get(&PAGINATION_PARAM_SENTINEL.to_string())
-                .is_some();
+            let paginated =
+                extensions.get(&PAGINATION_PARAM_SENTINEL.to_string());
             let websocket =
-                extensions.get(&WEBSOCKET_PARAM_SENTINEL.to_string()).is_some();
+                extensions.get(&WEBSOCKET_PARAM_SENTINEL.to_string());
             match (paginated, websocket) {
-                (false, false) => ExtensionMode::None,
-                (false, true) => ExtensionMode::Websocket,
-                (true, false) => ExtensionMode::Paginated,
-                (true, true) => panic!(
+                (None, None) => ExtensionMode::None,
+                (None, Some(_)) => ExtensionMode::Websocket,
+                (Some(first_page_schema), None) => {
+                    ExtensionMode::Paginated(first_page_schema.clone())
+                }
+                (Some(_), Some(_)) => panic!(
                     "Cannot use websocket and pagination in the same endpoint!"
                 ),
             }
@@ -184,6 +185,14 @@ mod test {
             ("page_token", false),
         ];
 
-        compare(params, ExtensionMode::Paginated, expected);
+        compare(
+            params,
+            ExtensionMode::Paginated(serde_json::json!(
+                {
+                    "required": ["bar", "foo"]
+                }
+            )),
+            expected,
+        );
     }
 }

dropshot/src/lib.rs

@@ -392,7 +392,7 @@
 //!
 //! ```json
 //! {
-//!     "page_token": "abc123...",
+//!     "next_page": "abc123...",
 //!     "items": [
 //!         {
 //!             "name": "aardvark",
@@ -413,7 +413,7 @@
 //! The page token `"abc123..."` is an opaque token to the client, but typically
 //! encodes the scan parameters and the value of the last item seen
 //! (`"name=badger"`).  The client knows it has completed the scan when it
-//! receives a response with no `page_token` in it.
+//! receives a response with no `next_page` in it.
 //!
 //! Our API endpoint can also support scanning in reverse order.  In this case,
 //! when the client makes the first request, it should fetch
@@ -507,15 +507,35 @@
 //! this ought to work, but it currently doesn't due to serde_urlencoded#33,
 //! which is really serde#1183.
 //!
-//! ### DTrace probes
+//! 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.
+//!
+//! ### OpenAPI extension
+//!
+//! In generated OpenAPI documents, Dropshot adds the `x-dropshot-pagination`
+//! extension to paginated operations. The value is currently a structure
+//! with this format:
+//! ```json
+//! {
+//!     "required": [ .. ]
+//! }
+//! ```
+//!
+//! The string values in the `required` array are the names of those query
+//! parameters that are mandatory if `page_token` is not specified (when
+//! fetching the first page of data).
+//!
+//! ## DTrace probes
 //!
 //! Dropshot optionally exposes two DTrace probes, `request_start` and
 //! `request_finish`. These provide detailed information about each request,
 //! such as their ID, the local and remote IPs, and the response information.
 //! See the dropshot::dtrace::RequestInfo` and `dropshot::dtrace::ResponseInfo`
 //! types for a complete listing of what's available.
 //!
-//! These probes are implemented via the [`usdt`] crate. They may require a
+//! These probes are implemented via the `usdt` crate. They may require a
 //! nightly toolchain if built on macOS prior to Rust version 1.66. Otherwise a
 //! stable compiler >= v1.59 is required in order to present the necessary
 //! features. Given these constraints, USDT functionality is behind the feature

dropshot/src/pagination.rs

@@ -1,4 +1,4 @@
-// Copyright 2022 Oxide Computer Company
+// Copyright 2023 Oxide Computer Company
 
 //! Detailed end-user documentation for pagination lives in the Dropshot top-
 //! level block comment.  Here we discuss some of the design choices.
@@ -106,7 +106,6 @@ use serde::de::DeserializeOwned;
 use serde::Deserialize;
 use serde::Deserializer;
 use serde::Serialize;
-use serde_json::json;
 use std::collections::BTreeMap;
 use std::fmt::Debug;
 use std::num::NonZeroU32;
@@ -231,6 +230,11 @@ pub(crate) const PAGINATION_PARAM_SENTINEL: &str =
     "x-dropshot-pagination-param";
 pub(crate) const PAGINATION_EXTENSION: &str = "x-dropshot-pagination";
 
+#[derive(Serialize)]
+struct PaginationParamSentinelValue {
+    required: schemars::Set<String>,
+}
+
 impl<ScanParams, PageSelector> JsonSchema
     for PaginationParams<ScanParams, PageSelector>
 where
@@ -244,17 +248,33 @@ where
     fn json_schema(
         gen: &mut schemars::gen::SchemaGenerator,
     ) -> schemars::schema::Schema {
-        // We use `SchemaPaginationParams to generate an intuitive schema and
+        // We use `SchemaPaginationParams` to generate an intuitive schema and
         // we use the JSON schema extensions mechanism to communicate the fact
         // that this is a pagination parameter. We'll later use this to tag
         // its associated operation as paginated.
+        //
+        // Because `SchemaPaginationParams` wraps `ScanParams` in an `Option`
+        // all properties become optional. The value of our sentinel is the
+        // set of properties that normally *would* be required.
+        //
         // TODO we would ideally like to verify that both parameters *and*
         // response structure are properly configured for pagination.
         let mut schema = SchemaPaginationParams::<ScanParams>::json_schema(gen)
             .into_object();
-        schema
-            .extensions
-            .insert(PAGINATION_PARAM_SENTINEL.to_string(), json!(true));
+        let first_page_schema = ScanParams::json_schema(gen);
+        let Some(first_page_object) = first_page_schema.into_object().object
+        else {
+            panic!("ScanParams must be an object");
+        };
+
+        let value = PaginationParamSentinelValue {
+            required: first_page_object.required,
+        };
+
+        schema.extensions.insert(
+            PAGINATION_PARAM_SENTINEL.to_string(),
+            serde_json::to_value(value).unwrap(),
+        );
         schemars::schema::Schema::Object(schema)
     }
 }
@@ -793,7 +813,11 @@ mod test {
                 .extensions
                 .get(&(PAGINATION_PARAM_SENTINEL.to_string()))
                 .unwrap(),
-            json!(true)
+            json!({
+                "required": [
+                    "name"
+                ]
+            })
         );
     }
 }

dropshot/src/server.rs

@@ -540,7 +540,7 @@ impl<C: ServerContext> Service<&TlsConn> for ServerConnectionHandler<C> {
 
 type SharedBoxFuture<T> = Shared<Pin<Box<dyn Future<Output = T> + Send>>>;
 
-/// Future returned by [`Server::wait_for_shutdown()`].
+/// Future returned by [`HttpServer::wait_for_shutdown()`].
 pub struct ShutdownWaitFuture(SharedBoxFuture<Result<(), String>>);
 
 impl Future for ShutdownWaitFuture {

dropshot/src/test_util.rs

@@ -1,4 +1,4 @@
-// Copyright 2020 Oxide Computer Company
+// Copyright 2023 Oxide Computer Company
 //! Automated testing facilities.  These are intended for use both by this crate
 //! and dependents of this crate.
 
@@ -130,8 +130,8 @@ impl ClientTestContext {
     }
 
     /// Execute an HTTP request against the test server and perform basic
-    /// validation of the result like [`make_request`], but with a content
-    /// type of "application/x-www-form-urlencoded".
+    /// validation of the result like [`ClientTestContext::make_request`], but
+    /// with a content type of "application/x-www-form-urlencoded".
     pub async fn make_request_url_encoded<
         RequestBodyType: Serialize + Debug,
     >(

dropshot/tests/test_openapi.json

@@ -207,6 +207,13 @@
         ],
         "operationId": "handler6",
         "parameters": [
+          {
+            "in": "query",
+            "name": "a_mandatory_string",
+            "schema": {
+              "type": "string"
+            }
+          },
           {
             "in": "query",
             "name": "a_number",
@@ -255,7 +262,11 @@
             "$ref": "#/components/responses/Error"
           }
         },
-        "x-dropshot-pagination": true
+        "x-dropshot-pagination": {
+          "required": [
+            "a_mandatory_string"
+          ]
+        }
       }
     },
     "/playing/a/bit/nicer": {

dropshot/tests/test_openapi.rs

@@ -1,4 +1,4 @@
-// Copyright 2022 Oxide Computer Company
+// Copyright 2023 Oxide Computer Company
 
 use dropshot::{
     endpoint, http_response_found, http_response_see_other,
@@ -138,6 +138,7 @@ struct ResponseItem {
 struct ExampleScanParams {
     #[serde(default)]
     a_number: u16,
+    a_mandatory_string: String,
 }
 
 #[derive(Deserialize, JsonSchema, Serialize)]

dropshot/tests/test_openapi_fuller.json

@@ -215,6 +215,13 @@
         ],
         "operationId": "handler6",
         "parameters": [
+          {
+            "in": "query",
+            "name": "a_mandatory_string",
+            "schema": {
+              "type": "string"
+            }
+          },
           {
             "in": "query",
             "name": "a_number",
@@ -263,7 +270,11 @@
             "$ref": "#/components/responses/Error"
           }
         },
-        "x-dropshot-pagination": true
+        "x-dropshot-pagination": {
+          "required": [
+            "a_mandatory_string"
+          ]
+        }
       }
     },
     "/playing/a/bit/nicer": {

dropshot/tests/test_pagination_schema.json

@@ -63,7 +63,11 @@
             "$ref": "#/components/responses/Error"
           }
         },
-        "x-dropshot-pagination": true
+        "x-dropshot-pagination": {
+          "required": [
+            "garbage_goes_in"
+          ]
+        }
       }
     }
   },

dropshot_endpoint/src/lib.rs

@@ -134,19 +134,21 @@ fn do_endpoint(
     do_endpoint_inner(metadata, attr, item)
 }
 
-/// As with [`endpoint`], this attribute turns a handler function into a
+/// As with [`macro@endpoint`], this attribute turns a handler function into a
 /// Dropshot endpoint, but first wraps the handler function in such a way
 /// that is spawned asynchronously and given the upgraded connection of
 /// the given `protocol` (i.e. `WEBSOCKETS`).
 ///
 /// The first argument still must be a `RequestContext<_>`.
 ///
 /// The last argument passed to the handler function must be a
-/// [`dropshot::WebsocketConnection`].
+/// [`WebsocketConnection`](../dropshot/struct.WebsocketConnection.html).
 ///
-/// The function must return a [`dropshot::WebsocketChannelResult`] (which is
-/// a general-purpose `Result<(), Box<dyn Error + Send + Sync + 'static>>`).
-/// Returned error values will be written to the RequestContext's log.
+/// The function must return a
+/// [`WebsocketChannelResult`](dropshot/type.WebsocketChannelResult.html)
+/// (which is a general-purpose `Result<(), Box<dyn Error + Send + Sync +
+/// 'static>>`). Returned error values will be written to the RequestContext's
+/// log.
 ///
 /// ```ignore
 /// #[dropshot::channel { protocol = WEBSOCKETS, path = "/my/ws/channel/{id}" }]