From 2b34d8fa57a652eb40dd141303cabc15d8a9d5bc Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Fri, 28 Apr 2023 16:51:17 -0700 Subject: [PATCH 1/4] Add required-without-page_token information to pagination extension --- dropshot/src/api_description.rs | 10 +++--- dropshot/src/extractor/metadata.rs | 29 +++++++++++------ dropshot/src/lib.rs | 28 +++++++++++++--- dropshot/src/pagination.rs | 38 ++++++++++++++++++---- dropshot/src/server.rs | 2 +- dropshot/src/test_util.rs | 6 ++-- dropshot/tests/test_openapi.json | 13 +++++++- dropshot/tests/test_openapi.rs | 3 +- dropshot/tests/test_openapi_fuller.json | 13 +++++++- dropshot/tests/test_pagination_schema.json | 6 +++- dropshot_endpoint/src/lib.rs | 2 +- 11 files changed, 115 insertions(+), 35 deletions(-) diff --git a/dropshot/src/api_description.rs b/dropshot/src/api_description.rs index c371b3aa6..01965cb80 100644 --- a/dropshot/src/api_description.rs +++ b/dropshot/src/api_description.rs @@ -670,12 +670,12 @@ impl ApiDescription { }) .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, } diff --git a/dropshot/src/extractor/metadata.rs b/dropshot/src/extractor/metadata.rs index 2f640d96d..def2aba62 100644 --- a/dropshot/src/extractor/metadata.rs +++ b/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( +pub(crate) fn get_metadata( 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, + ); } } diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index c19d8744c..40e58e72f 100644 --- a/dropshot/src/lib.rs +++ b/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,7 +507,27 @@ //! 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 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-paginated` +//! 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, @@ -515,7 +535,7 @@ //! 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 diff --git a/dropshot/src/pagination.rs b/dropshot/src/pagination.rs index 76e4f6dae..13c763e56 100644 --- a/dropshot/src/pagination.rs +++ b/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, +} + impl JsonSchema for PaginationParams 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::::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" + ] + }) ); } } diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index f2326e2ee..275a3e8b4 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -540,7 +540,7 @@ impl Service<&TlsConn> for ServerConnectionHandler { type SharedBoxFuture = Shared + Send>>>; -/// Future returned by [`Server::wait_for_shutdown()`]. +/// Future returned by [`HttpServer::wait_for_shutdown()`]. pub struct ShutdownWaitFuture(SharedBoxFuture>); impl Future for ShutdownWaitFuture { diff --git a/dropshot/src/test_util.rs b/dropshot/src/test_util.rs index 35fe2604a..ee11f7347 100644 --- a/dropshot/src/test_util.rs +++ b/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, >( diff --git a/dropshot/tests/test_openapi.json b/dropshot/tests/test_openapi.json index 510b99610..ddec202ab 100644 --- a/dropshot/tests/test_openapi.json +++ b/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": { diff --git a/dropshot/tests/test_openapi.rs b/dropshot/tests/test_openapi.rs index 748df3fb2..ade9d168a 100644 --- a/dropshot/tests/test_openapi.rs +++ b/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)] diff --git a/dropshot/tests/test_openapi_fuller.json b/dropshot/tests/test_openapi_fuller.json index e9aaef1bb..8239cb666 100644 --- a/dropshot/tests/test_openapi_fuller.json +++ b/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": { diff --git a/dropshot/tests/test_pagination_schema.json b/dropshot/tests/test_pagination_schema.json index 22cab8bc7..f57ee7438 100644 --- a/dropshot/tests/test_pagination_schema.json +++ b/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" + ] + } } } }, diff --git a/dropshot_endpoint/src/lib.rs b/dropshot_endpoint/src/lib.rs index d0f0fb260..95d505abc 100644 --- a/dropshot_endpoint/src/lib.rs +++ b/dropshot_endpoint/src/lib.rs @@ -134,7 +134,7 @@ 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`). From 5afeda01662689fac50f059b8835079e3eef4ce0 Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Fri, 28 Apr 2023 21:21:07 -0700 Subject: [PATCH 2/4] fix doc lints --- dropshot_endpoint/src/lib.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/dropshot_endpoint/src/lib.rs b/dropshot_endpoint/src/lib.rs index 95d505abc..6c0856c46 100644 --- a/dropshot_endpoint/src/lib.rs +++ b/dropshot_endpoint/src/lib.rs @@ -142,11 +142,13 @@ fn do_endpoint( /// 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>`). -/// 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>`). Returned error values will be written to the RequestContext's +/// log. /// /// ```ignore /// #[dropshot::channel { protocol = WEBSOCKETS, path = "/my/ws/channel/{id}" }] From 8de35e55e4bc83ab3d36903ab7c69e32cad590cd Mon Sep 17 00:00:00 2001 From: Adam Leventhal Date: Mon, 1 May 2023 20:18:56 -0700 Subject: [PATCH 3/4] Update dropshot/src/lib.rs Co-authored-by: David Pacheco --- dropshot/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 40e58e72f..b36002464 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -508,7 +508,7 @@ //! which is really serde#1183. //! //! Note that any parameters defined by `MyScanParams` are effectively encoded -//! into the page token and need not be supplied invocations when `page_token` +//! 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. //! From a50dca2a92c7452d8bbc26aeaabc6a7d0b0fc821 Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Mon, 1 May 2023 20:23:22 -0700 Subject: [PATCH 4/4] nit and update changelog --- CHANGELOG.adoc | 6 +++++- dropshot/src/lib.rs | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index d2952e842..82d3ed096 100644 --- a/CHANGELOG.adoc +++ b/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>` implementation; instead use `RequestInfo::new(&hyper::Request, 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>` implementation; instead use `RequestInfo::new(&hyper::Request, 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) diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 40e58e72f..9e7e6d3ba 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -514,7 +514,7 @@ //! //! ### OpenAPI extension //! -//! In generated OpenAPI documents, Dropshot adds the `x-dropshot-paginated` +//! In generated OpenAPI documents, Dropshot adds the `x-dropshot-pagination` //! extension to paginated operations. The value is currently a structure //! with this format: //! ```json