From a5bfb883f96a5188cfd62061908ab320a9d04c0b Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 24 Sep 2024 12:39:07 -0700 Subject: [PATCH 01/23] support for specifying version numbers in API endpoints --- Cargo.lock | 17 +- dropshot/Cargo.toml | 2 + dropshot/examples/api-trait-alternate.rs | 10 +- dropshot/examples/api-trait-websocket.rs | 11 +- dropshot/examples/api-trait.rs | 11 +- dropshot/examples/petstore.rs | 2 +- dropshot/examples/schema-with-example.rs | 2 +- dropshot/examples/versioning.rs | 157 +++++ dropshot/src/api_description.rs | 506 +++++++++++++++- dropshot/src/lib.rs | 6 +- dropshot/src/router.rs | 542 ++++++++++++++---- dropshot/src/server.rs | 113 +++- dropshot/src/websocket.rs | 3 +- dropshot/tests/fail/bad_endpoint10.stderr | 4 +- dropshot/tests/fail/bad_endpoint12.stderr | 4 +- dropshot/tests/fail/bad_endpoint15.stderr | 4 +- dropshot/tests/fail/bad_endpoint17.stderr | 4 +- dropshot/tests/fail/bad_endpoint18.stderr | 4 +- dropshot/tests/fail/bad_endpoint19.stderr | 4 +- dropshot/tests/fail/bad_endpoint3.stderr | 4 +- dropshot/tests/fail/bad_endpoint4.stderr | 4 +- dropshot/tests/fail/bad_endpoint5.stderr | 4 +- dropshot/tests/fail/bad_endpoint7.stderr | 4 +- dropshot/tests/fail/bad_endpoint9.stderr | 4 +- dropshot/tests/test_openapi.json | 2 +- dropshot/tests/test_openapi.rs | 5 +- dropshot/tests/test_openapi_fuller.json | 2 +- dropshot/tests/test_pagination_schema.json | 2 +- dropshot/tests/test_pagination_schema.rs | 2 +- dropshot_endpoint/Cargo.toml | 1 + dropshot_endpoint/src/api_trait.rs | 12 +- dropshot_endpoint/src/channel.rs | 29 + dropshot_endpoint/src/endpoint.rs | 231 ++++++++ dropshot_endpoint/src/metadata.rs | 132 ++++- .../tests/output/api_trait_basic.rs | 8 + .../output/api_trait_with_custom_params.rs | 11 +- .../output/channel_with_custom_params.rs | 1 + .../output/channel_with_unnamed_params.rs | 1 + .../tests/output/channel_with_versions.rs | 64 +++ .../tests/output/endpoint_basic.rs | 1 + .../tests/output/endpoint_content_type.rs | 1 + .../endpoint_context_fully_qualified_names.rs | 1 + .../tests/output/endpoint_pub_crate.rs | 1 + .../endpoint_weird_but_ok_arg_types_1.rs | 1 + .../endpoint_weird_but_ok_arg_types_2.rs | 1 + .../output/endpoint_with_custom_params.rs | 1 + .../tests/output/endpoint_with_doc.rs | 1 + .../endpoint_with_empty_where_clause.rs | 1 + .../tests/output/endpoint_with_query.rs | 1 + .../tests/output/endpoint_with_tags.rs | 1 + .../output/endpoint_with_unnamed_params.rs | 1 + .../output/endpoint_with_versions_all.rs | 67 +++ .../output/endpoint_with_versions_from.rs | 67 +++ .../endpoint_with_versions_from_until.rs | 71 +++ .../output/endpoint_with_versions_until.rs | 67 +++ 55 files changed, 2020 insertions(+), 193 deletions(-) create mode 100644 dropshot/examples/versioning.rs create mode 100644 dropshot_endpoint/tests/output/channel_with_versions.rs create mode 100644 dropshot_endpoint/tests/output/endpoint_with_versions_all.rs create mode 100644 dropshot_endpoint/tests/output/endpoint_with_versions_from.rs create mode 100644 dropshot_endpoint/tests/output/endpoint_with_versions_from_until.rs create mode 100644 dropshot_endpoint/tests/output/endpoint_with_versions_until.rs diff --git a/Cargo.lock b/Cargo.lock index 253ef8780..7d14b19dd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -381,6 +381,7 @@ dependencies = [ "rustls-pki-types", "schemars", "scopeguard", + "semver", "serde", "serde_json", "serde_path_to_error", @@ -394,6 +395,7 @@ dependencies = [ "slog-term", "subprocess", "tempfile", + "thiserror", "tokio", "tokio-rustls", "tokio-tungstenite", @@ -415,6 +417,7 @@ dependencies = [ "proc-macro2", "quote", "schema", + "semver", "serde", "serde_tokenstream", "syn 2.0.77", @@ -1527,6 +1530,12 @@ dependencies = [ "libc", ] +[[package]] +name = "semver" +version = "1.0.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "61697e0a1c7e512e84a621326239844a24d8207b4669b41bc18b32ea5cbf988b" + [[package]] name = "serde" version = "1.0.210" @@ -1827,18 +1836,18 @@ dependencies = [ [[package]] name = "thiserror" -version = "1.0.56" +version = "1.0.63" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d54378c645627613241d077a3a79db965db602882668f9136ac42af9ecb730ad" +checksum = "c0342370b38b6a11b6cc11d6a805569958d54cfa061a29969c3b5ce2ea405724" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "1.0.56" +version = "1.0.63" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fa0faa943b50f3db30a20aa7e265dbc66076993efed8463e8de414e5d06d3471" +checksum = "a4558b58466b9ad7ca0f102865eccc95938dca1a74a856f2b57b6629050da261" dependencies = [ "proc-macro2", "quote", diff --git a/dropshot/Cargo.toml b/dropshot/Cargo.toml index 92a132ff4..aa27be0eb 100644 --- a/dropshot/Cargo.toml +++ b/dropshot/Cargo.toml @@ -31,6 +31,7 @@ percent-encoding = "2.3.1" rustls = "0.22.4" rustls-pemfile = "2.1.3" scopeguard = "1.2.0" +semver = "1.0.23" serde_json = "1.0.128" serde_path_to_error = "0.1.16" serde_urlencoded = "0.7.1" @@ -41,6 +42,7 @@ slog-async = "2.8.0" slog-bunyan = "2.5.0" slog-json = "2.6.1" slog-term = "2.9.1" +thiserror = "1.0.63" tokio-rustls = "0.25.0" toml = "0.8.19" waitgroup = "0.1.2" diff --git a/dropshot/examples/api-trait-alternate.rs b/dropshot/examples/api-trait-alternate.rs index 733442a81..cd23ee9f3 100644 --- a/dropshot/examples/api-trait-alternate.rs +++ b/dropshot/examples/api-trait-alternate.rs @@ -143,14 +143,14 @@ mod api { pub(crate) counter: u64, } - // A simple function to generate an OpenAPI spec for the trait, without having - // a real implementation available. + // A simple function to generate an OpenAPI spec for the trait, without + // having a real implementation available. // - // If the interface and implementation (see below) are in different crates, then - // this function would live in the interface crate. + // If the interface and implementation (see below) are in different crates, + // then this function would live in the interface crate. pub(crate) fn generate_openapi_spec() -> String { let api = counter_api_mod::stub_api_description().unwrap(); - let spec = api.openapi("Counter Server", "1.0.0"); + let spec = api.openapi("Counter Server", semver::Version::new(1, 0, 0)); serde_json::to_string_pretty(&spec.json().unwrap()).unwrap() } } diff --git a/dropshot/examples/api-trait-websocket.rs b/dropshot/examples/api-trait-websocket.rs index 9f2d923d5..9ac2f486f 100644 --- a/dropshot/examples/api-trait-websocket.rs +++ b/dropshot/examples/api-trait-websocket.rs @@ -43,14 +43,15 @@ mod api { pub(crate) counter: u8, } - // A simple function to generate an OpenAPI spec for the server, without having - // a real implementation available. + // A simple function to generate an OpenAPI spec for the server, without + // having a real implementation available. // - // If the interface and implementation (see below) are in different crates, then - // this function would live in the interface crate. + // If the interface and implementation (see below) are in different crates, + // then this function would live in the interface crate. pub(crate) fn generate_openapi_spec() -> String { let my_server = counter_api_mod::stub_api_description().unwrap(); - let spec = my_server.openapi("Counter Server", "1.0.0"); + let spec = + my_server.openapi("Counter Server", semver::Version::new(1, 0, 0)); serde_json::to_string_pretty(&spec.json().unwrap()).unwrap() } } diff --git a/dropshot/examples/api-trait.rs b/dropshot/examples/api-trait.rs index e68684de9..2768e6516 100644 --- a/dropshot/examples/api-trait.rs +++ b/dropshot/examples/api-trait.rs @@ -54,14 +54,15 @@ mod api { pub(crate) counter: u64, } - // A simple function to generate an OpenAPI spec for the trait, without having - // a real implementation available. + // A simple function to generate an OpenAPI spec for the trait, without + // having a real implementation available. // - // If the interface and implementation (see below) are in different crates, then - // this function would live in the interface crate. + // If the interface and implementation (see below) are in different crates, + // then this function would live in the interface crate. pub(crate) fn generate_openapi_spec() -> String { let description = counter_api_mod::stub_api_description().unwrap(); - let spec = description.openapi("Counter Server", "1.0.0"); + let spec = description + .openapi("Counter Server", semver::Version::new(1, 0, 0)); serde_json::to_string_pretty(&spec.json().unwrap()).unwrap() } } diff --git a/dropshot/examples/petstore.rs b/dropshot/examples/petstore.rs index 9b4d7fc34..aa350761c 100644 --- a/dropshot/examples/petstore.rs +++ b/dropshot/examples/petstore.rs @@ -12,7 +12,7 @@ fn main() -> Result<(), String> { api.register(update_pet_with_form).unwrap(); api.register(find_pets_by_tags).unwrap(); - api.openapi("Pet Shop", "") + api.openapi("Pet Shop", semver::Version::new(1, 0, 0)) .write(&mut std::io::stdout()) .map_err(|e| e.to_string())?; diff --git a/dropshot/examples/schema-with-example.rs b/dropshot/examples/schema-with-example.rs index 90d2077c2..cd44fca0a 100644 --- a/dropshot/examples/schema-with-example.rs +++ b/dropshot/examples/schema-with-example.rs @@ -51,7 +51,7 @@ fn main() -> Result<(), String> { let mut api = ApiDescription::new(); api.register(get_foo).unwrap(); - api.openapi("Examples", "0.0.0") + api.openapi("Examples", semver::Version::new(0, 0, 0)) .write(&mut std::io::stdout()) .map_err(|e| e.to_string())?; diff --git a/dropshot/examples/versioning.rs b/dropshot/examples/versioning.rs new file mode 100644 index 000000000..2ed68c59e --- /dev/null +++ b/dropshot/examples/versioning.rs @@ -0,0 +1,157 @@ +// Copyright 2024 Oxide Computer Company + +//! Example using API versioning + +use dropshot::endpoint; +use dropshot::ApiDescription; +use dropshot::ConfigDropshot; +use dropshot::ConfigLogging; +use dropshot::ConfigLoggingLevel; +use dropshot::DynamicVersionPolicy; +use dropshot::HttpError; +use dropshot::HttpResponseOk; +use dropshot::HttpServerStarter; +use dropshot::RequestContext; +use dropshot::VersionPolicy; +use hyper::Body; +use hyper::Request; +use schemars::JsonSchema; +use semver::Version; +use serde::Serialize; +use slog::Logger; +use std::str::FromStr; + +#[tokio::main] +async fn main() -> Result<(), String> { + // See dropshot/examples/basic.rs for more details on these pieces. + let config_dropshot: ConfigDropshot = Default::default(); + let config_logging = + ConfigLogging::StderrTerminal { level: ConfigLoggingLevel::Debug }; + let log = config_logging + .to_logger("example-basic") + .map_err(|error| format!("failed to create logger: {}", error))?; + + let mut api = ApiDescription::new(); + api.register(v1::versioned_get).unwrap(); + api.register(v2::versioned_get).unwrap(); + + let api_context = (); + let server = HttpServerStarter::new_with_versioning( + &config_dropshot, + api, + api_context, + &log, + None, + VersionPolicy::Dynamic(Box::new(BasicVersionPolicy {})), + ) + .map_err(|error| format!("failed to create server: {}", error))? + .start(); + + server.await +} + +/// This impl of `DynamicVersionPolicy` tells Dropshot how to determine the +/// appropriate version number for a request. +#[derive(Debug)] +struct BasicVersionPolicy {} +impl DynamicVersionPolicy for BasicVersionPolicy { + fn request_extract_version( + &self, + request: &Request, + _log: &Logger, + ) -> Result { + parse_header(request.headers(), "dropshot-demo-version") + } +} + +/// Parses a required header out of a request (producing useful error messages +/// for all failure modes) +fn parse_header( + headers: &http::HeaderMap, + header_name: &str, +) -> Result +where + T: FromStr, + ::Err: std::fmt::Display, +{ + let v_value = headers.get(header_name).ok_or_else(|| { + HttpError::for_bad_request( + None, + format!("missing expected header {:?}", header_name), + ) + })?; + + let v_str = v_value.to_str().map_err(|_| { + HttpError::for_bad_request( + None, + format!( + "bad value for header {:?}: not ASCII: {:?}", + header_name, v_value + ), + ) + })?; + + v_str.parse::().map_err(|e| { + HttpError::for_bad_request( + None, + format!("bad value for header {:?}: {}: {}", header_name, e, v_str), + ) + }) +} + +// HTTP API interface +// +// This API defines several different versions: +// +// - versions 1.0.0 through 1.0.3 use `v1::versioned_get` +// - versions 1.0.5 and later use `v2::versioned_get` +// - versions prior to 1.0.0 and version 1.0.4 do not exist + +mod v1 { + // The contents of this module define endpoints and types used in v1.0.0 + // through v1.0.3. + + use super::*; + + #[derive(Serialize, JsonSchema)] + struct Value { + s: &'static str, + } + + /// Fetch the current value of the counter. + #[endpoint { + method = GET, + path = "/", + versions = "1.0.0".."1.0.3" + }] + pub async fn versioned_get( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + Ok(HttpResponseOk(Value { s: "hello from an early v1" })) + } +} + +mod v2 { + // The contents of this module define endpoints and types used in v1.0.5 and + // later. + + use super::*; + + #[derive(Serialize, JsonSchema)] + struct Value { + s: &'static str, + number: u32, + } + + /// Fetch the current value of the counter. + #[endpoint { + method = GET, + path = "/", + versions = "1.0.5".. + }] + pub async fn versioned_get( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + Ok(HttpResponseOk(Value { s: "hello from a LATE v1", number: 12 })) + } +} diff --git a/dropshot/src/api_description.rs b/dropshot/src/api_description.rs index 9758fc8d5..9f5f92b5c 100644 --- a/dropshot/src/api_description.rs +++ b/dropshot/src/api_description.rs @@ -59,6 +59,7 @@ pub struct ApiEndpoint { pub extension_mode: ExtensionMode, pub visible: bool, pub deprecated: bool, + pub versions: ApiEndpointVersions, } impl<'a, Context: ServerContext> ApiEndpoint { @@ -68,6 +69,7 @@ impl<'a, Context: ServerContext> ApiEndpoint { method: Method, content_type: &'a str, path: &'a str, + versions: ApiEndpointVersions, ) -> Self where HandlerType: HttpHandlerFunc, @@ -93,6 +95,7 @@ impl<'a, Context: ServerContext> ApiEndpoint { extension_mode: func_parameters.extension_mode, visible: true, deprecated: false, + versions, } } @@ -136,7 +139,7 @@ impl<'a> ApiEndpoint { /// type parameters. /// /// ```rust - /// use dropshot::{ApiDescription, ApiEndpoint, HttpError, HttpResponseOk, Query, StubContext}; + /// use dropshot::{ApiDescription, ApiEndpoint, ApiEndpointVersions, HttpError, HttpResponseOk, Query, StubContext}; /// use schemars::JsonSchema; /// use serde::Deserialize; /// @@ -157,6 +160,7 @@ impl<'a> ApiEndpoint { /// http::Method::GET, /// "application/json", /// "/value", + /// ApiEndpointVersions::All, /// ); /// api.register(endpoint).unwrap(); /// ``` @@ -165,6 +169,7 @@ impl<'a> ApiEndpoint { method: Method, content_type: &'a str, path: &'a str, + versions: ApiEndpointVersions, ) -> Self where FuncParams: RequestExtractor + 'static, @@ -190,6 +195,7 @@ impl<'a> ApiEndpoint { extension_mode: func_parameters.extension_mode, visible: true, deprecated: false, + versions, } } } @@ -351,9 +357,9 @@ impl std::fmt::Debug for ApiSchemaGenerator { } } -/// An ApiDescription represents the endpoints and handler functions in your API. -/// Other metadata could also be provided here. This object can be used to -/// generate an OpenAPI spec or to run an HTTP server implementing the API. +/// An ApiDescription represents the endpoints and handler functions in your +/// API. Other metadata could also be provided here. This object can be used +/// to generate an OpenAPI spec or to run an HTTP server implementing the API. pub struct ApiDescription { /// In practice, all the information we need is encoded in the router. router: HttpRouter, @@ -592,32 +598,36 @@ impl ApiDescription { /// [`OpenApiDefinition`] which can be used to specify the contents of the /// definition and select an output format. /// - /// The arguments to this function will be used for the mandatory `title` and - /// `version` properties that the `Info` object in an OpenAPI definition must - /// contain. - pub fn openapi( + /// The arguments to this function will be used for the mandatory `title` + /// and `version` properties that the `Info` object in an OpenAPI definition + /// must contain. + pub fn openapi( &self, - title: S1, - version: S2, + title: S, + version: semver::Version, ) -> OpenApiDefinition where - S1: AsRef, - S2: AsRef, + S: AsRef, { - OpenApiDefinition::new(self, title.as_ref(), version.as_ref()) + OpenApiDefinition::new(self, title.as_ref(), version) } /// Internal routine for constructing the OpenAPI definition describing this /// API in its JSON form. - fn gen_openapi(&self, info: openapiv3::Info) -> openapiv3::OpenAPI { + fn gen_openapi( + &self, + info: openapiv3::Info, + version: &semver::Version, + ) -> openapiv3::OpenAPI { let mut openapi = openapiv3::OpenAPI::default(); openapi.openapi = "3.0.3".to_string(); openapi.info = info; // Gather up the ad hoc tags from endpoints - let endpoint_tags = (&self.router) - .into_iter() + let endpoint_tags = self + .router + .endpoints(Some(version)) .flat_map(|(_, _, endpoint)| { endpoint .tags @@ -657,7 +667,7 @@ impl ApiDescription { let mut definitions = indexmap::IndexMap::::new(); - for (path, method, endpoint) in &self.router { + for (path, method, endpoint) in self.router.endpoints(Some(version)) { if !endpoint.visible { continue; } @@ -1056,6 +1066,167 @@ impl fmt::Display for ApiDescriptionRegisterError { impl std::error::Error for ApiDescriptionRegisterError {} +/// Describes which versions of the API this endpoint is defined for +#[derive(Debug, Eq, PartialEq)] +pub enum ApiEndpointVersions { + /// this endpoint covers all versions of the API + All, + /// this endpoint was introduced in a specific version and is present in all + /// subsequent versions + From(semver::Version), + /// this endpoint was introduced in a specific version and removed in a + /// subsequent version + // We use an extra level of indirection to enforce that the versions here + // are provided in order. + FromUntil(OrderedVersionPair), + /// this endpoint was present in all versions up to (and including) this + /// specific version + Until(semver::Version), +} + +#[derive(Debug, Eq, PartialEq)] +pub struct OrderedVersionPair { + earliest: semver::Version, + latest: semver::Version, +} + +impl ApiEndpointVersions { + pub fn all() -> ApiEndpointVersions { + ApiEndpointVersions::All + } + + pub fn from(v: semver::Version) -> ApiEndpointVersions { + ApiEndpointVersions::From(v) + } + + pub fn until(v: semver::Version) -> ApiEndpointVersions { + ApiEndpointVersions::Until(v) + } + + pub fn from_until( + earliest: semver::Version, + latest: semver::Version, + ) -> Result { + if latest < earliest { + return Err( + "versions in a from-until version range must be provided \ + in order", + ); + } + + Ok(ApiEndpointVersions::FromUntil(OrderedVersionPair { + earliest, + latest, + })) + } + + /// Returns whether the given `version` matches this endpoint version + /// + /// Recall that `ApiEndpointVersions` essentially defines a _range_ of + /// API versions that an API endpoint will appear in. This returns true if + /// `version` is contained in that range. This is used to determine if an + /// endpoint satisfies the requirements for an incoming request. + /// + /// If `version` is `None`, that means that the request doesn't care what + /// version it's getting. `matches()` always returns true in that case. + pub(crate) fn matches(&self, version: Option<&semver::Version>) -> bool { + let Some(version) = version else { + // If there's no version constraint at all, then all versions match. + return true; + }; + + match self { + ApiEndpointVersions::All => true, + ApiEndpointVersions::From(earliest) => version >= earliest, + ApiEndpointVersions::FromUntil(OrderedVersionPair { + earliest, + latest, + }) => version >= earliest && version <= latest, + ApiEndpointVersions::Until(latest) => version <= latest, + } + } + + /// Returns whether one version range overlaps with another + /// + /// This is used to disallow registering multiple API endpoints where, if + /// given a particular version, it would be ambiguous which endpoint to use. + pub(crate) fn overlaps_with(&self, other: &ApiEndpointVersions) -> bool { + // There must be better ways to do this. You might think: + // + // - `semver` has a `VersionReq`, which represents a range similar to + // our variants. But it does not have a way to programmatically + // construct it and it does not support an "intersection" operator. + // + // - These are basically Rust ranges, right? Yes, but Rust also doesn't + // have a range "intersection" operator. + match (self, other) { + // easy degenerate cases + (ApiEndpointVersions::All, _) => true, + (_, ApiEndpointVersions::All) => true, + (ApiEndpointVersions::From(_), ApiEndpointVersions::From(_)) => { + true + } + (ApiEndpointVersions::Until(_), ApiEndpointVersions::Until(_)) => { + true + } + + // more complicated cases + ( + ApiEndpointVersions::From(earliest), + ApiEndpointVersions::Until(latest), + ) => latest >= earliest, + ( + ApiEndpointVersions::Until(latest), + ApiEndpointVersions::From(earliest), + ) => latest >= earliest, + + ( + ApiEndpointVersions::From(earliest), + ApiEndpointVersions::FromUntil(OrderedVersionPair { + earliest: _, + latest, + }), + ) => earliest <= latest, + ( + ApiEndpointVersions::FromUntil(OrderedVersionPair { + earliest: _, + latest, + }), + ApiEndpointVersions::From(earliest), + ) => earliest <= latest, + + ( + ApiEndpointVersions::Until(latest), + ApiEndpointVersions::FromUntil(OrderedVersionPair { + earliest, + latest: _, + }), + ) => earliest <= latest, + ( + ApiEndpointVersions::FromUntil(OrderedVersionPair { + earliest, + latest: _, + }), + ApiEndpointVersions::Until(latest), + ) => earliest <= latest, + + ( + ApiEndpointVersions::FromUntil(OrderedVersionPair { + earliest: earliest1, + latest: latest1, + }), + ApiEndpointVersions::FromUntil(OrderedVersionPair { + earliest: earliest2, + latest: latest2, + }), + ) => { + (earliest1 <= earliest2 && earliest2 <= latest1) + || (earliest2 <= earliest1 && earliest1 <= latest2) + } + } + } +} + /// Returns true iff the schema represents the void schema that matches no data. fn is_empty(schema: &schemars::schema::Schema) -> bool { if let schemars::schema::Schema::Bool(false) = schema { @@ -1120,27 +1291,28 @@ fn is_empty(schema: &schemars::schema::Schema) -> bool { pub struct OpenApiDefinition<'a, Context: ServerContext> { api: &'a ApiDescription, info: openapiv3::Info, + version: semver::Version, } impl<'a, Context: ServerContext> OpenApiDefinition<'a, Context> { fn new( api: &'a ApiDescription, title: &str, - version: &str, + version: semver::Version, ) -> OpenApiDefinition<'a, Context> { let info = openapiv3::Info { title: title.to_string(), version: version.to_string(), ..Default::default() }; - OpenApiDefinition { api, info } + OpenApiDefinition { api, info, version } } /// Provide a short description of the API. CommonMark syntax may be /// used for rich text representation. /// - /// This routine will set the `description` field of the `Info` object in the - /// OpenAPI definition. + /// This routine will set the `description` field of the `Info` object in + /// the OpenAPI definition. pub fn description>(&mut self, description: S) -> &mut Self { self.info.description = Some(description.as_ref().to_string()); self @@ -1227,7 +1399,9 @@ impl<'a, Context: ServerContext> OpenApiDefinition<'a, Context> { /// Build a JSON object containing the OpenAPI definition for this API. pub fn json(&self) -> serde_json::Result { - serde_json::to_value(&self.api.gen_openapi(self.info.clone())) + serde_json::to_value( + &self.api.gen_openapi(self.info.clone(), &self.version), + ) } /// Build a JSON object containing the OpenAPI definition for this API and @@ -1238,7 +1412,7 @@ impl<'a, Context: ServerContext> OpenApiDefinition<'a, Context> { ) -> serde_json::Result<()> { serde_json::to_writer_pretty( &mut *out, - &self.api.gen_openapi(self.info.clone()), + &self.api.gen_openapi(self.info.clone(), &self.version), )?; writeln!(out).map_err(serde_json::Error::custom)?; Ok(()) @@ -1328,6 +1502,8 @@ mod test { use std::str::from_utf8; use crate as dropshot; // for "endpoint" macro + use crate::api_description::ApiEndpointVersions; + use semver::Version; #[derive(Deserialize, JsonSchema)] #[allow(dead_code)] @@ -1352,6 +1528,7 @@ mod test { Method::GET, CONTENT_TYPE_JSON, "/", + ApiEndpointVersions::All, )); let error = ret.unwrap_err(); assert_eq!( @@ -1369,6 +1546,7 @@ mod test { Method::GET, CONTENT_TYPE_JSON, "/{a}/{aa}/{b}/{bb}", + ApiEndpointVersions::All, )); let error = ret.unwrap_err(); assert_eq!(error.message(), "path parameters are not consumed (aa,bb)"); @@ -1383,6 +1561,7 @@ mod test { Method::GET, CONTENT_TYPE_JSON, "/{c}/{d}", + ApiEndpointVersions::All, )); let error = ret.unwrap_err(); assert_eq!( @@ -1455,6 +1634,7 @@ mod test { Method::GET, CONTENT_TYPE_JSON, "/{a}/{b}", + ApiEndpointVersions::All, )); let error = ret.unwrap_err(); assert_eq!(error.message(), "At least one tag is required".to_string()); @@ -1474,6 +1654,7 @@ mod test { Method::GET, CONTENT_TYPE_JSON, "/{a}/{b}", + ApiEndpointVersions::All, ) .tag("howdy") .tag("pardner"), @@ -1496,6 +1677,7 @@ mod test { Method::GET, CONTENT_TYPE_JSON, "/{a}/{b}", + ApiEndpointVersions::All, ) .tag("a-tag"), ); @@ -1525,6 +1707,7 @@ mod test { Method::GET, CONTENT_TYPE_JSON, "/xx/{a}/{b}", + ApiEndpointVersions::All, ) .tag("a-tag") .tag("z-tag"), @@ -1537,6 +1720,7 @@ mod test { Method::GET, CONTENT_TYPE_JSON, "/yy/{a}/{b}", + ApiEndpointVersions::All, ) .tag("b-tag") .tag("y-tag"), @@ -1544,7 +1728,7 @@ mod test { .unwrap(); let mut out = Vec::new(); - api.openapi("", "").write(&mut out).unwrap(); + api.openapi("", Version::new(1, 0, 0)).write(&mut out).unwrap(); let out = from_utf8(&out).unwrap(); let spec = serde_json::from_str::(out).unwrap(); @@ -1576,4 +1760,278 @@ mod test { assert_eq!(config.policy, EndpointTagPolicy::AtLeastOne); assert_eq!(config.tags.len(), 2); } + + #[test] + fn test_endpoint_versions_range() { + let error = ApiEndpointVersions::from_until( + Version::new(1, 2, 3), + Version::new(1, 2, 2), + ) + .unwrap_err(); + assert_eq!( + error, + "versions in a from-until version range must be provided in order" + ); + } + + #[test] + fn test_endpoint_versions_matches() { + let v_all = ApiEndpointVersions::all(); + let v_from = ApiEndpointVersions::from(Version::new(1, 2, 3)); + let v_until = ApiEndpointVersions::until(Version::new(4, 5, 6)); + let v_fromuntil = ApiEndpointVersions::from_until( + Version::new(1, 2, 3), + Version::new(4, 5, 6), + ) + .unwrap(); + let v_oneversion = ApiEndpointVersions::from_until( + Version::new(1, 2, 3), + Version::new(1, 2, 3), + ) + .unwrap(); + + struct TestCase<'a> { + versions: &'a ApiEndpointVersions, + check: Option, + expected: bool, + } + impl<'a> TestCase<'a> { + fn new( + versions: &'a ApiEndpointVersions, + check: Version, + expected: bool, + ) -> TestCase<'a> { + TestCase { versions, check: Some(check), expected } + } + + fn new_empty(versions: &'a ApiEndpointVersions) -> TestCase<'a> { + // Every type of ApiEndpointVersions ought to match when + // provided no version constraint. + TestCase { versions, check: None, expected: true } + } + } + + let mut nerrors = 0; + for test_case in &[ + TestCase::new_empty(&v_all), + TestCase::new_empty(&v_from), + TestCase::new_empty(&v_until), + TestCase::new_empty(&v_fromuntil), + TestCase::new_empty(&v_oneversion), + TestCase::new(&v_all, Version::new(0, 0, 0), true), + TestCase::new(&v_all, Version::new(1, 0, 0), true), + TestCase::new(&v_all, Version::new(1, 2, 3), true), + TestCase::new(&v_from, Version::new(0, 0, 0), false), + TestCase::new(&v_from, Version::new(1, 2, 2), false), + TestCase::new(&v_from, Version::new(1, 2, 3), true), + TestCase::new(&v_from, Version::new(1, 2, 4), true), + TestCase::new(&v_from, Version::new(5, 0, 0), true), + TestCase::new(&v_until, Version::new(0, 0, 0), true), + TestCase::new(&v_until, Version::new(4, 5, 5), true), + TestCase::new(&v_until, Version::new(4, 5, 6), true), + TestCase::new(&v_until, Version::new(4, 5, 7), false), + TestCase::new(&v_until, Version::new(37, 0, 0), false), + TestCase::new(&v_fromuntil, Version::new(0, 0, 0), false), + TestCase::new(&v_fromuntil, Version::new(1, 2, 2), false), + TestCase::new(&v_fromuntil, Version::new(1, 2, 3), true), + TestCase::new(&v_fromuntil, Version::new(4, 5, 5), true), + TestCase::new(&v_fromuntil, Version::new(4, 5, 6), true), + TestCase::new(&v_fromuntil, Version::new(4, 5, 7), false), + TestCase::new(&v_fromuntil, Version::new(12, 0, 0), false), + TestCase::new(&v_oneversion, Version::new(1, 2, 2), false), + TestCase::new(&v_oneversion, Version::new(1, 2, 3), true), + TestCase::new(&v_oneversion, Version::new(1, 2, 4), false), + ] { + print!( + "test case: {:?} matches {}: expected {}, got ", + test_case.versions, + match &test_case.check { + Some(x) => format!("Some({x})"), + None => String::from("None"), + }, + test_case.expected + ); + + let result = test_case.versions.matches(test_case.check.as_ref()); + if result != test_case.expected { + println!("{} (FAIL)", result); + nerrors += 1; + } else { + println!("{} (PASS)", result); + } + } + + if nerrors > 0 { + panic!("test cases failed: {}", nerrors); + } + } + + #[test] + fn test_endpoint_versions_overlaps() { + let v_all = ApiEndpointVersions::all(); + let v_from = ApiEndpointVersions::from(Version::new(1, 2, 3)); + let v_until = ApiEndpointVersions::until(Version::new(4, 5, 6)); + let v_fromuntil = ApiEndpointVersions::from_until( + Version::new(1, 2, 3), + Version::new(4, 5, 6), + ) + .unwrap(); + let v_oneversion = ApiEndpointVersions::from_until( + Version::new(1, 2, 3), + Version::new(1, 2, 3), + ) + .unwrap(); + + struct TestCase<'a> { + v1: &'a ApiEndpointVersions, + v2: &'a ApiEndpointVersions, + expected: bool, + } + + impl<'a> TestCase<'a> { + fn new( + v1: &'a ApiEndpointVersions, + v2: &'a ApiEndpointVersions, + expected: bool, + ) -> TestCase<'a> { + TestCase { v1, v2, expected } + } + } + + let mut nerrors = 0; + for test_case in &[ + // All of our canned intervals overlap with themselves. + TestCase::new(&v_all, &v_all, true), + TestCase::new(&v_from, &v_from, true), + TestCase::new(&v_until, &v_until, true), + TestCase::new(&v_fromuntil, &v_fromuntil, true), + TestCase::new(&v_oneversion, &v_oneversion, true), + // + // "all" test cases. + // + // "all" overlaps with all of our other canned intervals. + TestCase::new(&v_all, &v_from, true), + TestCase::new(&v_all, &v_until, true), + TestCase::new(&v_all, &v_fromuntil, true), + TestCase::new(&v_all, &v_oneversion, true), + // + // "from" test cases. + // + // "from" + "from" always overlap + TestCase::new( + &v_from, + &ApiEndpointVersions::from(Version::new(0, 1, 2)), + true, + ), + // "from" + "until": overlap is exactly one point + TestCase::new( + &v_from, + &ApiEndpointVersions::until(Version::new(1, 2, 3)), + true, + ), + // "from" + "until": no overlap + TestCase::new( + &v_from, + &ApiEndpointVersions::until(Version::new(1, 2, 2)), + false, + ), + // "from" from "from-until": overlap + TestCase::new(&v_from, &v_fromuntil, true), + // "from" + "from-until": no overlap + TestCase::new( + &v_from, + &ApiEndpointVersions::from_until( + Version::new(1, 2, 0), + Version::new(1, 2, 2), + ) + .unwrap(), + false, + ), + // + // "until" test cases + // + // "until" + "until" always overlap. + TestCase::new( + &v_until, + &ApiEndpointVersions::until(Version::new(2, 0, 0)), + true, + ), + // "until" plus "from-until": overlap + TestCase::new(&v_until, &v_fromuntil, true), + // "until" plus "from-until": no overlap + TestCase::new( + &v_until, + &ApiEndpointVersions::from_until( + Version::new(5, 0, 0), + Version::new(6, 0, 0), + ) + .unwrap(), + false, + ), + // + // "from-until" test cases + // + // We've tested everything except two "from-until" ranges. + // first: no overlap + TestCase::new( + &v_fromuntil, + &ApiEndpointVersions::from_until( + Version::new(0, 0, 1), + Version::new(1, 2, 2), + ) + .unwrap(), + false, + ), + // overlap at one endpoint + TestCase::new( + &v_fromuntil, + &ApiEndpointVersions::from_until( + Version::new(0, 0, 1), + Version::new(1, 2, 3), + ) + .unwrap(), + true, + ), + // overlap in the middle somewhere + TestCase::new( + &v_fromuntil, + &ApiEndpointVersions::from_until( + Version::new(0, 0, 1), + Version::new(2, 0, 0), + ) + .unwrap(), + true, + ), + // one contained entirely inside the other + TestCase::new( + &v_fromuntil, + &ApiEndpointVersions::from_until( + Version::new(0, 0, 1), + Version::new(12, 0, 0), + ) + .unwrap(), + true, + ), + ] { + print!( + "test case: {:?} overlaps {:?}: expected {}, got ", + test_case.v1, test_case.v2, test_case.expected + ); + + // Make sure to test both directions. The result should be the + // same. + let result1 = test_case.v1.overlaps_with(&test_case.v2); + let result2 = test_case.v2.overlaps_with(&test_case.v1); + if result1 != test_case.expected || result2 != test_case.expected { + println!("{} {} (FAIL)", result1, result2); + nerrors += 1; + } else { + println!("{} (PASS)", result1); + } + } + + if nerrors > 0 { + panic!("test cases failed: {}", nerrors); + } + } } diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 620f3f1c0..78049aa77 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -502,7 +502,8 @@ //! # // defining fn main puts the doctest in a module context //! # fn main() { //! let description = project_api_mod::stub_api_description().unwrap(); -//! let mut openapi = description.openapi("Project Server", "1.0.0"); +//! let mut openapi = description +//! .openapi("Project Server", semver::Version::new(1, 0, 0)); //! openapi.write(&mut std::io::stdout().lock()).unwrap(); //! # } //! ``` @@ -778,6 +779,7 @@ pub use api_description::ApiEndpointBodyContentType; pub use api_description::ApiEndpointParameter; pub use api_description::ApiEndpointParameterLocation; pub use api_description::ApiEndpointResponse; +pub use api_description::ApiEndpointVersions; pub use api_description::EndpointTagPolicy; pub use api_description::ExtensionMode; pub use api_description::OpenApiDefinition; @@ -834,8 +836,10 @@ pub use pagination::PaginationOrder; pub use pagination::PaginationParams; pub use pagination::ResultsPage; pub use pagination::WhichPage; +pub use server::DynamicVersionPolicy; pub use server::ServerContext; pub use server::ShutdownWaitFuture; +pub use server::VersionPolicy; pub use server::{HttpServer, HttpServerStarter}; pub use websocket::WebsocketChannelResult; pub use websocket::WebsocketConnection; diff --git a/dropshot/src/router.rs b/dropshot/src/router.rs index 93934f02e..c1be0fd56 100644 --- a/dropshot/src/router.rs +++ b/dropshot/src/router.rs @@ -4,9 +4,11 @@ use super::error::HttpError; use super::handler::RouteHandler; +use crate::api_description::ApiEndpointVersions; use crate::from_map::MapError; use crate::from_map::MapValue; use crate::server::ServerContext; +use crate::server::Version; use crate::ApiEndpoint; use crate::ApiEndpointBodyContentType; use http::Method; @@ -16,9 +18,9 @@ use std::collections::BTreeMap; use std::collections::BTreeSet; use std::sync::Arc; -/// `HttpRouter` is a simple data structure for routing incoming HTTP requests to -/// specific handler functions based on the request method and URI path. For -/// examples, see the basic test below. +/// `HttpRouter` is a simple data structure for routing incoming HTTP requests +/// to specific handler functions based on the request method, URI path, and +/// version. For examples, see the basic test below. /// /// Routes are registered and looked up according to a path, like `"/foo/bar"`. /// Paths are split into segments separated by one or more '/' characters. When @@ -56,7 +58,8 @@ use std::sync::Arc; /// * A given path cannot use the same variable name twice. For example, you /// can't register path `"/projects/{id}/instances/{id}"`. /// -/// * A given resource may have at most one handler for a given HTTP method. +/// * A given resource may have at most one handler for a given HTTP method and +/// version. /// /// * The expectation is that during server initialization, /// `HttpRouter::insert()` will be invoked to register a number of route @@ -66,6 +69,9 @@ use std::sync::Arc; pub struct HttpRouter { /// root of the trie root: Box>, + /// indicates whether this router contains any endpoints that are + /// constrained by version + has_versioned_routes: bool, } /// Each node in the tree represents a group of HTTP resources having the same @@ -81,7 +87,7 @@ pub struct HttpRouter { #[derive(Debug)] struct HttpRouterNode { /// Handlers, etc. for each of the HTTP methods defined for this node. - methods: BTreeMap>, + methods: BTreeMap>>, /// Edges linking to child nodes. edges: Option>, } @@ -163,7 +169,7 @@ impl PathSegment { /// Wrapper for a path that's the result of user input i.e. an HTTP query. /// We use this type to avoid confusion with paths used to define routes. -#[derive(Debug)] +#[derive(Debug, Clone, Copy)] pub struct InputPath<'a>(&'a str); impl<'a> From<&'a str> for InputPath<'a> { @@ -224,7 +230,10 @@ impl HttpRouterNode { impl HttpRouter { /// Returns a new `HttpRouter` with no routes configured. pub fn new() -> Self { - HttpRouter { root: Box::new(HttpRouterNode::new()) } + HttpRouter { + root: Box::new(HttpRouterNode::new()), + has_versioned_routes: false, + } } /// Configure a route for HTTP requests based on the HTTP `method` and @@ -386,15 +395,48 @@ impl HttpRouter { } let methodname = method.as_str().to_uppercase(); - if node.methods.contains_key(&methodname) { - panic!( - "URI path \"{}\": attempted to create duplicate route for \ - method \"{}\"", - path, method, - ); + let existing_handlers = + node.methods.entry(methodname.clone()).or_default(); + + for handler in existing_handlers.iter() { + if handler.versions.overlaps_with(&endpoint.versions) { + if handler.versions == endpoint.versions { + panic!( + "URI path \"{}\": attempted to create duplicate route \ + for method \"{}\"", + path, methodname + ); + } else { + panic!( + "URI path \"{}\": attempted to register multiple \ + handlers for method \"{}\" with overlapping version \ + ranges", + path, methodname + ); + } + } } - node.methods.insert(methodname, endpoint); + if endpoint.versions != ApiEndpointVersions::All { + self.has_versioned_routes = true; + } + + existing_handlers.push(endpoint); + } + + /// Returns whether this router contains any routes that are constrained by + /// version + pub fn has_versioned_routes(&self) -> bool { + self.has_versioned_routes + } + + #[cfg(test)] + pub fn lookup_route_unversioned( + &self, + method: &Method, + path: InputPath<'_>, + ) -> Result, HttpError> { + self.lookup_route(method, path, None) } /// Look up the route handler for an HTTP request having method `method` and @@ -407,6 +449,7 @@ impl HttpRouter { &self, method: &Method, path: InputPath<'_>, + version: Option<&Version>, ) -> Result, HttpError> { let all_segments = input_path_to_segments(&path).map_err(|_| { HttpError::for_bad_request( @@ -469,28 +512,62 @@ impl HttpRouter { _ => {} } - // As a somewhat special case, if one requests a node with no handlers - // at all, report a 404. We could probably treat this as a 405 as well. - if node.methods.is_empty() { - return Err(HttpError::for_not_found( - None, - String::from("route has no handlers"), - )); - } - + // First, look for a matching implementation. let methodname = method.as_str().to_uppercase(); - node.methods - .get(&methodname) - .map(|handler| RouterLookupResult { + if let Some(handler) = find_handler_matching_version( + node.methods.get(&methodname).map(|v| v.as_slice()).unwrap_or(&[]), + version, + ) { + return Ok(RouterLookupResult { handler: Arc::clone(&handler.handler), operation_id: handler.operation_id.clone(), variables, body_content_type: handler.body_content_type.clone(), - }) - .ok_or_else(|| { - HttpError::for_status(None, StatusCode::METHOD_NOT_ALLOWED) - }) + }); + } + + // We found no handler matching this path, method name, and version. + // We're going to report a 404 ("Not Found") or 405 ("Method Not + // Allowed"). It's a 405 if there are any handlers matching this path + // and version for a different method. It's a 404 otherwise. + if node.methods.values().any(|handlers| { + find_handler_matching_version(handlers, version).is_some() + }) { + Err(HttpError::for_status(None, StatusCode::METHOD_NOT_ALLOWED)) + } else { + Err(HttpError::for_not_found( + None, + format!( + "route has no handlers for version {}", + match version { + Some(v) => v.to_string(), + None => String::from(""), + } + ), + )) + } } + + pub fn endpoints<'a>( + &'a self, + version: Option<&'a Version>, + ) -> HttpRouterIter<'a, Context> { + HttpRouterIter::new(self, version) + } +} + +/// Given a list of handlers, return the first one matching the given semver +/// +/// If `version` is `None`, any handler will do. +fn find_handler_matching_version<'a, I, C>( + handlers: I, + version: Option<&Version>, +) -> Option<&'a ApiEndpoint> +where + I: IntoIterator>, + C: ServerContext, +{ + handlers.into_iter().find(|h| h.versions.matches(version)) } /// Insert a variable into the set after checking for duplicates. @@ -511,14 +588,6 @@ fn insert_var( varnames.insert(new_varname.clone()); } -impl<'a, Context: ServerContext> IntoIterator for &'a HttpRouter { - type Item = (String, String, &'a ApiEndpoint); - type IntoIter = HttpRouterIter<'a, Context>; - fn into_iter(self) -> Self::IntoIter { - HttpRouterIter::new(self) - } -} - /// Route Interator implementation. We perform a preorder, depth first traversal /// of the tree starting from the root node. For each node, we enumerate the /// methods and then descend into its children (or single child in the case of @@ -531,18 +600,42 @@ pub struct HttpRouterIter<'a, Context: ServerContext> { method: Box)> + 'a>, path: Vec<(PathSegment, Box>)>, + version: Option<&'a Version>, } type PathIter<'a, Context> = dyn Iterator>)> + 'a; +fn iter_handlers_from_node<'a, 'b, 'c, C: ServerContext>( + node: &'a HttpRouterNode, + version: Option<&'b Version>, +) -> Box)> + 'c> +where + 'a: 'c, + 'b: 'c, +{ + Box::new(node.methods.iter().flat_map(move |(m, handlers)| { + handlers.iter().filter_map(move |h| { + if h.versions.matches(version.clone()) { + Some((m, h)) + } else { + None + } + }) + })) +} + impl<'a, Context: ServerContext> HttpRouterIter<'a, Context> { - fn new(router: &'a HttpRouter) -> Self { + fn new( + router: &'a HttpRouter, + version: Option<&'a Version>, + ) -> Self { HttpRouterIter { - method: Box::new(router.root.methods.iter()), + method: iter_handlers_from_node(&router.root, version.clone()), path: vec![( PathSegment::Literal("".to_string()), HttpRouterIter::iter_node(&router.root), )], + version, } } @@ -606,8 +699,8 @@ impl<'a, Context: ServerContext> Iterator for HttpRouterIter<'a, Context> { match self.method.next() { Some((m, ref e)) => break Some((self.path(), m.clone(), e)), None => { - // We've iterated fully through the method in this node so it's - // time to find the next node. + // We've iterated fully through the method in this node so + // it's time to find the next node. match self.path.last_mut() { None => break None, Some((_, ref mut last)) => match last.next() { @@ -620,7 +713,10 @@ impl<'a, Context: ServerContext> Iterator for HttpRouterIter<'a, Context> { path_component, HttpRouterIter::iter_node(node), )); - self.method = Box::new(node.methods.iter()); + self.method = iter_handlers_from_node( + &node, + self.version.clone(), + ); } }, } @@ -731,6 +827,7 @@ mod test { use super::HttpRouter; use super::PathSegment; use crate::api_description::ApiEndpointBodyContentType; + use crate::api_description::ApiEndpointVersions; use crate::from_map::from_map; use crate::router::VariableValue; use crate::ApiEndpoint; @@ -739,6 +836,7 @@ mod test { use http::StatusCode; use hyper::Body; use hyper::Response; + use semver::Version; use serde::Deserialize; use std::collections::BTreeMap; use std::sync::Arc; @@ -761,6 +859,15 @@ mod test { handler: Arc>, method: Method, path: &str, + ) -> ApiEndpoint<()> { + new_endpoint_versions(handler, method, path, ApiEndpointVersions::All) + } + + fn new_endpoint_versions( + handler: Arc>, + method: Method, + path: &str, + versions: ApiEndpointVersions, ) -> ApiEndpoint<()> { ApiEndpoint { operation_id: "test_handler".to_string(), @@ -776,6 +883,7 @@ mod test { extension_mode: Default::default(), visible: true, deprecated: false, + versions, } } @@ -840,6 +948,45 @@ mod test { router.insert(new_endpoint(new_handler(), Method::GET, "//")); } + #[test] + #[should_panic(expected = "URI path \"/boo\": attempted to create \ + duplicate route for method \"GET\"")] + fn test_duplicate_route_same_version() { + let mut router = HttpRouter::new(); + router.insert(new_endpoint_versions( + new_handler(), + Method::GET, + "/boo", + ApiEndpointVersions::From(Version::new(1, 2, 3)), + )); + router.insert(new_endpoint_versions( + new_handler(), + Method::GET, + "/boo", + ApiEndpointVersions::From(Version::new(1, 2, 3)), + )); + } + + #[test] + #[should_panic(expected = "URI path \"/boo\": attempted to register \ + multiple handlers for method \"GET\" with \ + overlapping version ranges")] + fn test_duplicate_route_overlapping_version() { + let mut router = HttpRouter::new(); + router.insert(new_endpoint_versions( + new_handler(), + Method::GET, + "/boo", + ApiEndpointVersions::From(Version::new(1, 2, 3)), + )); + router.insert(new_endpoint_versions( + new_handler(), + Method::GET, + "/boo", + ApiEndpointVersions::From(Version::new(4, 5, 6)), + )); + } + #[test] #[should_panic(expected = "URI path \"/projects/{id}/insts/{id}\": \ variable name \"id\" is used more than once")] @@ -970,54 +1117,98 @@ mod test { let mut router = HttpRouter::new(); // Check a few initial conditions. - let error = router.lookup_route(&Method::GET, "/".into()).unwrap_err(); + let error = router + .lookup_route_unversioned(&Method::GET, "/".into()) + .unwrap_err(); assert_eq!(error.status_code, StatusCode::NOT_FOUND); - let error = - router.lookup_route(&Method::GET, "////".into()).unwrap_err(); + let error = router + .lookup_route_unversioned(&Method::GET, "////".into()) + .unwrap_err(); assert_eq!(error.status_code, StatusCode::NOT_FOUND); - let error = - router.lookup_route(&Method::GET, "/foo/bar".into()).unwrap_err(); + let error = router + .lookup_route_unversioned(&Method::GET, "/foo/bar".into()) + .unwrap_err(); assert_eq!(error.status_code, StatusCode::NOT_FOUND); let error = router - .lookup_route(&Method::GET, "//foo///bar".into()) + .lookup_route_unversioned(&Method::GET, "//foo///bar".into()) .unwrap_err(); assert_eq!(error.status_code, StatusCode::NOT_FOUND); // Insert a route into the middle of the tree. This will let us look at // parent nodes, sibling nodes, and child nodes. router.insert(new_endpoint(new_handler(), Method::GET, "/foo/bar")); - assert!(router.lookup_route(&Method::GET, "/foo/bar".into()).is_ok()); - assert!(router.lookup_route(&Method::GET, "/foo/bar/".into()).is_ok()); - assert!(router.lookup_route(&Method::GET, "//foo/bar".into()).is_ok()); - assert!(router.lookup_route(&Method::GET, "//foo//bar".into()).is_ok()); assert!(router - .lookup_route(&Method::GET, "//foo//bar//".into()) + .lookup_route_unversioned(&Method::GET, "/foo/bar".into()) + .is_ok()); + assert!(router + .lookup_route_unversioned(&Method::GET, "/foo/bar/".into()) .is_ok()); assert!(router - .lookup_route(&Method::GET, "///foo///bar///".into()) + .lookup_route_unversioned(&Method::GET, "//foo/bar".into()) + .is_ok()); + assert!(router + .lookup_route_unversioned(&Method::GET, "//foo//bar".into()) + .is_ok()); + assert!(router + .lookup_route_unversioned(&Method::GET, "//foo//bar//".into()) + .is_ok()); + assert!(router + .lookup_route_unversioned(&Method::GET, "///foo///bar///".into()) .is_ok()); // TODO-cleanup: consider having a "build" step that constructs a // read-only router and does validation like making sure that there's a // GET route on all nodes? - let error = router.lookup_route(&Method::GET, "/".into()).unwrap_err(); + let error = router + .lookup_route_unversioned(&Method::GET, "/".into()) + .unwrap_err(); assert_eq!(error.status_code, StatusCode::NOT_FOUND); - let error = - router.lookup_route(&Method::GET, "/foo".into()).unwrap_err(); + let error = router + .lookup_route_unversioned(&Method::GET, "/foo".into()) + .unwrap_err(); assert_eq!(error.status_code, StatusCode::NOT_FOUND); - let error = - router.lookup_route(&Method::GET, "//foo".into()).unwrap_err(); + let error = router + .lookup_route_unversioned(&Method::GET, "//foo".into()) + .unwrap_err(); assert_eq!(error.status_code, StatusCode::NOT_FOUND); let error = router - .lookup_route(&Method::GET, "/foo/bar/baz".into()) + .lookup_route_unversioned(&Method::GET, "/foo/bar/baz".into()) .unwrap_err(); assert_eq!(error.status_code, StatusCode::NOT_FOUND); - let error = - router.lookup_route(&Method::PUT, "/foo/bar".into()).unwrap_err(); + let error = router + .lookup_route_unversioned(&Method::PUT, "/foo/bar".into()) + .unwrap_err(); assert_eq!(error.status_code, StatusCode::METHOD_NOT_ALLOWED); - let error = - router.lookup_route(&Method::PUT, "/foo/bar/".into()).unwrap_err(); + let error = router + .lookup_route_unversioned(&Method::PUT, "/foo/bar/".into()) + .unwrap_err(); + assert_eq!(error.status_code, StatusCode::METHOD_NOT_ALLOWED); + + // Check error cases that are specific (or handled differently) when + // routes are versioned. + let mut router = HttpRouter::new(); + router.insert(new_endpoint_versions( + new_handler(), + Method::GET, + "/foo", + ApiEndpointVersions::from(Version::new(1, 0, 0)), + )); + let error = router + .lookup_route( + &Method::GET, + "/foo".into(), + Some(&Version::new(0, 9, 0)), + ) + .unwrap_err(); + assert_eq!(error.status_code, StatusCode::NOT_FOUND); + let error = router + .lookup_route( + &Method::PUT, + "/foo".into(), + Some(&Version::new(1, 1, 0)), + ) + .unwrap_err(); assert_eq!(error.status_code, StatusCode::METHOD_NOT_ALLOWED); } @@ -1029,15 +1220,21 @@ mod test { // back, even if we use different names that normalize to "/". // Before we start, sanity-check that there's nothing at the root // already. Other test cases examine the errors in more detail. - assert!(router.lookup_route(&Method::GET, "/".into()).is_err()); + assert!(router + .lookup_route_unversioned(&Method::GET, "/".into()) + .is_err()); router.insert(new_endpoint(new_handler_named("h1"), Method::GET, "/")); - let result = router.lookup_route(&Method::GET, "/".into()).unwrap(); + let result = + router.lookup_route_unversioned(&Method::GET, "/".into()).unwrap(); assert_eq!(result.handler.label(), "h1"); assert!(result.variables.is_empty()); - let result = router.lookup_route(&Method::GET, "//".into()).unwrap(); + let result = + router.lookup_route_unversioned(&Method::GET, "//".into()).unwrap(); assert_eq!(result.handler.label(), "h1"); assert!(result.variables.is_empty()); - let result = router.lookup_route(&Method::GET, "///".into()).unwrap(); + let result = router + .lookup_route_unversioned(&Method::GET, "///".into()) + .unwrap(); assert_eq!(result.handler.label(), "h1"); assert!(result.variables.is_empty()); @@ -1045,49 +1242,151 @@ mod test { // we get both this handler and the previous one if we ask for the // corresponding method and that we get no handler for a different, // third method. - assert!(router.lookup_route(&Method::PUT, "/".into()).is_err()); + assert!(router + .lookup_route_unversioned(&Method::PUT, "/".into()) + .is_err()); router.insert(new_endpoint(new_handler_named("h2"), Method::PUT, "/")); - let result = router.lookup_route(&Method::PUT, "/".into()).unwrap(); + let result = + router.lookup_route_unversioned(&Method::PUT, "/".into()).unwrap(); assert_eq!(result.handler.label(), "h2"); assert!(result.variables.is_empty()); - let result = router.lookup_route(&Method::GET, "/".into()).unwrap(); + let result = + router.lookup_route_unversioned(&Method::GET, "/".into()).unwrap(); assert_eq!(result.handler.label(), "h1"); - assert!(router.lookup_route(&Method::DELETE, "/".into()).is_err()); + assert!(router + .lookup_route_unversioned(&Method::DELETE, "/".into()) + .is_err()); assert!(result.variables.is_empty()); // Now insert a handler one level deeper. Verify that all the previous // handlers behave as we expect, and that we have one handler at the new // path, whichever name we use for it. - assert!(router.lookup_route(&Method::GET, "/foo".into()).is_err()); + assert!(router + .lookup_route_unversioned(&Method::GET, "/foo".into()) + .is_err()); router.insert(new_endpoint( new_handler_named("h3"), Method::GET, "/foo", )); - let result = router.lookup_route(&Method::PUT, "/".into()).unwrap(); + let result = + router.lookup_route_unversioned(&Method::PUT, "/".into()).unwrap(); assert_eq!(result.handler.label(), "h2"); assert!(result.variables.is_empty()); - let result = router.lookup_route(&Method::GET, "/".into()).unwrap(); + let result = + router.lookup_route_unversioned(&Method::GET, "/".into()).unwrap(); assert_eq!(result.handler.label(), "h1"); assert!(result.variables.is_empty()); - let result = router.lookup_route(&Method::GET, "/foo".into()).unwrap(); + let result = router + .lookup_route_unversioned(&Method::GET, "/foo".into()) + .unwrap(); assert_eq!(result.handler.label(), "h3"); assert!(result.variables.is_empty()); - let result = router.lookup_route(&Method::GET, "/foo/".into()).unwrap(); + let result = router + .lookup_route_unversioned(&Method::GET, "/foo/".into()) + .unwrap(); assert_eq!(result.handler.label(), "h3"); assert!(result.variables.is_empty()); - let result = - router.lookup_route(&Method::GET, "//foo//".into()).unwrap(); + let result = router + .lookup_route_unversioned(&Method::GET, "//foo//".into()) + .unwrap(); assert_eq!(result.handler.label(), "h3"); assert!(result.variables.is_empty()); - let result = - router.lookup_route(&Method::GET, "/foo//".into()).unwrap(); + let result = router + .lookup_route_unversioned(&Method::GET, "/foo//".into()) + .unwrap(); assert_eq!(result.handler.label(), "h3"); assert!(result.variables.is_empty()); - assert!(router.lookup_route(&Method::PUT, "/foo".into()).is_err()); - assert!(router.lookup_route(&Method::PUT, "/foo/".into()).is_err()); - assert!(router.lookup_route(&Method::PUT, "//foo//".into()).is_err()); - assert!(router.lookup_route(&Method::PUT, "/foo//".into()).is_err()); + assert!(router + .lookup_route_unversioned(&Method::PUT, "/foo".into()) + .is_err()); + assert!(router + .lookup_route_unversioned(&Method::PUT, "/foo/".into()) + .is_err()); + assert!(router + .lookup_route_unversioned(&Method::PUT, "//foo//".into()) + .is_err()); + assert!(router + .lookup_route_unversioned(&Method::PUT, "/foo//".into()) + .is_err()); + } + + #[test] + fn test_router_versioned() { + // Install handlers for a particular route for a bunch of different + // versions. + let method = Method::GET; + let path: super::InputPath<'static> = "/foo".into(); + let mut router = HttpRouter::new(); + router.insert(new_endpoint_versions( + new_handler_named("h1"), + method.clone(), + "/foo", + ApiEndpointVersions::until(Version::new(1, 2, 3)), + )); + router.insert(new_endpoint_versions( + new_handler_named("h2"), + method.clone(), + "/foo", + ApiEndpointVersions::from_until( + Version::new(2, 0, 0), + Version::new(3, 0, 0), + ) + .unwrap(), + )); + router.insert(new_endpoint_versions( + new_handler_named("h3"), + method.clone(), + "/foo", + ApiEndpointVersions::From(Version::new(5, 0, 0)), + )); + + // Check what happens for a representative range of versions. + let result = router + .lookup_route(&method, path, Some(&Version::new(1, 2, 2))) + .unwrap(); + assert_eq!(result.handler.label(), "h1"); + let result = router + .lookup_route(&method, path, Some(&Version::new(1, 2, 3))) + .unwrap(); + assert_eq!(result.handler.label(), "h1"); + + let error = router + .lookup_route(&method, path, Some(&Version::new(1, 2, 4))) + .unwrap_err(); + assert_eq!(error.status_code, StatusCode::NOT_FOUND); + + let result = router + .lookup_route(&method, path, Some(&Version::new(2, 0, 0))) + .unwrap(); + assert_eq!(result.handler.label(), "h2"); + let result = router + .lookup_route(&method, path, Some(&Version::new(2, 1, 0))) + .unwrap(); + assert_eq!(result.handler.label(), "h2"); + let result = router + .lookup_route(&method, path, Some(&Version::new(3, 0, 0))) + .unwrap(); + assert_eq!(result.handler.label(), "h2"); + + let error = router + .lookup_route(&method, path, Some(&Version::new(3, 0, 1))) + .unwrap_err(); + assert_eq!(error.status_code, StatusCode::NOT_FOUND); + + let error = router + .lookup_route(&method, path, Some(&Version::new(4, 99, 99))) + .unwrap_err(); + assert_eq!(error.status_code, StatusCode::NOT_FOUND); + + let result = router + .lookup_route(&method, path, Some(&Version::new(5, 0, 0))) + .unwrap(); + assert_eq!(result.handler.label(), "h3"); + let result = router + .lookup_route(&method, path, Some(&Version::new(128313, 0, 0))) + .unwrap(); + assert_eq!(result.handler.label(), "h3"); } #[test] @@ -1096,7 +1395,7 @@ mod test { // change the behavior, intentionally or otherwise. let mut router = HttpRouter::new(); assert!(router - .lookup_route(&Method::GET, "/not{a}variable".into()) + .lookup_route_unversioned(&Method::GET, "/not{a}variable".into()) .is_err()); router.insert(new_endpoint( new_handler_named("h4"), @@ -1104,15 +1403,15 @@ mod test { "/not{a}variable", )); let result = router - .lookup_route(&Method::GET, "/not{a}variable".into()) + .lookup_route_unversioned(&Method::GET, "/not{a}variable".into()) .unwrap(); assert_eq!(result.handler.label(), "h4"); assert!(result.variables.is_empty()); assert!(router - .lookup_route(&Method::GET, "/not{b}variable".into()) + .lookup_route_unversioned(&Method::GET, "/not{b}variable".into()) .is_err()); assert!(router - .lookup_route(&Method::GET, "/notnotavariable".into()) + .lookup_route_unversioned(&Method::GET, "/notnotavariable".into()) .is_err()); } @@ -1125,12 +1424,14 @@ mod test { Method::GET, "/projects/{project_id}", )); - assert!(router.lookup_route(&Method::GET, "/projects".into()).is_err()); assert!(router - .lookup_route(&Method::GET, "/projects/".into()) + .lookup_route_unversioned(&Method::GET, "/projects".into()) + .is_err()); + assert!(router + .lookup_route_unversioned(&Method::GET, "/projects/".into()) .is_err()); let result = router - .lookup_route(&Method::GET, "/projects/p12345".into()) + .lookup_route_unversioned(&Method::GET, "/projects/p12345".into()) .unwrap(); assert_eq!(result.handler.label(), "h5"); assert_eq!( @@ -1142,10 +1443,13 @@ mod test { VariableValue::String("p12345".to_string()) ); assert!(router - .lookup_route(&Method::GET, "/projects/p12345/child".into()) + .lookup_route_unversioned( + &Method::GET, + "/projects/p12345/child".into() + ) .is_err()); let result = router - .lookup_route(&Method::GET, "/projects/p12345/".into()) + .lookup_route_unversioned(&Method::GET, "/projects/p12345/".into()) .unwrap(); assert_eq!(result.handler.label(), "h5"); assert_eq!( @@ -1153,7 +1457,10 @@ mod test { VariableValue::String("p12345".to_string()) ); let result = router - .lookup_route(&Method::GET, "/projects///p12345//".into()) + .lookup_route_unversioned( + &Method::GET, + "/projects///p12345//".into(), + ) .unwrap(); assert_eq!(result.handler.label(), "h5"); assert_eq!( @@ -1162,7 +1469,10 @@ mod test { ); // Trick question! let result = router - .lookup_route(&Method::GET, "/projects/{project_id}".into()) + .lookup_route_unversioned( + &Method::GET, + "/projects/{project_id}".into(), + ) .unwrap(); assert_eq!(result.handler.label(), "h5"); assert_eq!( @@ -1182,7 +1492,7 @@ mod test { {fwrule_id}/info", )); let result = router - .lookup_route( + .lookup_route_unversioned( &Method::GET, "/projects/p1/instances/i2/fwrules/fw3/info".into(), ) @@ -1217,16 +1527,28 @@ mod test { "/projects/{project_id}/instances", )); assert!(router - .lookup_route(&Method::GET, "/projects/instances".into()) + .lookup_route_unversioned( + &Method::GET, + "/projects/instances".into() + ) .is_err()); assert!(router - .lookup_route(&Method::GET, "/projects//instances".into()) + .lookup_route_unversioned( + &Method::GET, + "/projects//instances".into() + ) .is_err()); assert!(router - .lookup_route(&Method::GET, "/projects///instances".into()) + .lookup_route_unversioned( + &Method::GET, + "/projects///instances".into() + ) .is_err()); let result = router - .lookup_route(&Method::GET, "/projects/foo/instances".into()) + .lookup_route_unversioned( + &Method::GET, + "/projects/foo/instances".into(), + ) .unwrap(); assert_eq!(result.handler.label(), "h7"); } @@ -1241,7 +1563,10 @@ mod test { )); let result = router - .lookup_route(&Method::OPTIONS, "/console/missiles/launch".into()) + .lookup_route_unversioned( + &Method::OPTIONS, + "/console/missiles/launch".into(), + ) .unwrap(); assert_eq!( @@ -1274,7 +1599,10 @@ mod test { )); let result = router - .lookup_route(&Method::OPTIONS, "/console/missiles/launch".into()) + .lookup_route_unversioned( + &Method::OPTIONS, + "/console/missiles/launch".into(), + ) .unwrap(); let path = @@ -1288,7 +1616,7 @@ mod test { #[test] fn test_iter_null() { let router = HttpRouter::<()>::new(); - let ret: Vec<_> = router.into_iter().map(|x| (x.0, x.1)).collect(); + let ret: Vec<_> = router.endpoints(None).map(|x| (x.0, x.1)).collect(); assert_eq!(ret, vec![]); } @@ -1305,7 +1633,7 @@ mod test { Method::GET, "/projects/{project_id}/instances", )); - let ret: Vec<_> = router.into_iter().map(|x| (x.0, x.1)).collect(); + let ret: Vec<_> = router.endpoints(None).map(|x| (x.0, x.1)).collect(); assert_eq!( ret, vec![ @@ -1331,7 +1659,7 @@ mod test { Method::POST, "/", )); - let ret: Vec<_> = router.into_iter().map(|x| (x.0, x.1)).collect(); + let ret: Vec<_> = router.endpoints(None).map(|x| (x.0, x.1)).collect(); assert_eq!( ret, vec![ diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index 7f7935005..af6f5d761 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -51,6 +51,10 @@ use slog::Logger; // TODO Replace this with something else? type GenericError = Box; +#[derive(Debug, thiserror::Error)] +#[error("unversioned servers cannot have endpoints with specific versions")] +pub struct UnversionedServerHasVersionedRoutes {} + /// Endpoint-accessible context associated with a server. /// /// Automatically implemented for all Send + Sync types. @@ -76,6 +80,7 @@ pub struct DropshotState { /// Worker for the handler_waitgroup associated with this server, allowing /// graceful shutdown to wait for all handlers to complete. pub(crate) handler_waitgroup_worker: DebugIgnore, + pub(crate) version_policy: VersionPolicy, } impl DropshotState { @@ -84,6 +89,55 @@ impl DropshotState { } } +pub type Version = semver::Version; + +#[derive(Debug)] +pub enum VersionPolicy { + Unversioned, + Dynamic(Box), +} + +impl VersionPolicy { + fn request_version( + &self, + request: &Request, + request_log: &Logger, + ) -> Result, HttpError> { + match self { + VersionPolicy::Unversioned => Ok(None), + VersionPolicy::Dynamic(vers_impl) => { + let result = + vers_impl.request_extract_version(request, request_log); + + match &result { + Ok(version) => { + debug!(request_log, "determined request API version"; + "version" => %version, + ); + } + Err(error) => { + error!( + request_log, + "failed to determine request API version"; + "error" => ?error, + ); + } + } + + result.map(Some) + } + } + } +} + +pub trait DynamicVersionPolicy: std::fmt::Debug + Send + Sync { + fn request_extract_version( + &self, + request: &Request, + log: &Logger, + ) -> Result; +} + /// Stores static configuration associated with the server /// TODO-cleanup merge with ConfigDropshot #[derive(Debug)] @@ -130,6 +184,24 @@ impl HttpServerStarter { private: C, log: &Logger, tls: Option, + ) -> Result, GenericError> { + Self::new_with_versioning( + config, + api, + private, + log, + tls, + VersionPolicy::Unversioned, + ) + } + + pub fn new_with_versioning( + config: &ConfigDropshot, + api: ApiDescription, + private: C, + log: &Logger, + tls: Option, + version_policy: VersionPolicy, ) -> Result, GenericError> { let server_config = ServerConfig { // We start aggressively to ensure test coverage. @@ -152,6 +224,7 @@ impl HttpServerStarter { log, tls, handler_waitgroup.worker(), + version_policy, )?; HttpServerStarter { app_state, @@ -169,6 +242,7 @@ impl HttpServerStarter { private, log, handler_waitgroup.worker(), + version_policy, )?; HttpServerStarter { app_state, @@ -179,10 +253,12 @@ impl HttpServerStarter { } }; - for (path, method, _) in &starter.app_state.router { + for (path, method, endpoint) in starter.app_state.router.endpoints(None) + { debug!(starter.app_state.log, "registered endpoint"; "method" => &method, - "path" => &path + "path" => &path, + "versions" => ?endpoint.versions, ); } @@ -290,18 +366,27 @@ impl InnerHttpServerStarter { private: C, log: &Logger, handler_waitgroup_worker: waitgroup::Worker, - ) -> Result, hyper::Error> { + version_policy: VersionPolicy, + ) -> Result, GenericError> { let incoming = AddrIncoming::bind(&config.bind_address)?; let local_addr = incoming.local_addr(); + let router = api.into_router(); + if let VersionPolicy::Unversioned = version_policy { + if router.has_versioned_routes() { + return Err(Box::new(UnversionedServerHasVersionedRoutes {})); + } + } + let app_state = Arc::new(DropshotState { private, config: server_config, - router: api.into_router(), + router, log: log.new(o!("local_addr" => local_addr)), local_addr, tls_acceptor: None, handler_waitgroup_worker: DebugIgnore(handler_waitgroup_worker), + version_policy, }); let make_service = ServerConnectionHandler::new(app_state.clone()); @@ -576,6 +661,7 @@ impl InnerHttpsServerStarter { log: &Logger, tls: &ConfigTls, handler_waitgroup_worker: waitgroup::Worker, + version_policy: VersionPolicy, ) -> Result, GenericError> { let acceptor = Arc::new(Mutex::new(TlsAcceptor::from(Arc::new( rustls::ServerConfig::try_from(tls)?, @@ -595,14 +681,22 @@ impl InnerHttpsServerStarter { let https_acceptor = HttpsAcceptor::new(logger.clone(), acceptor.clone(), tcp); + let router = api.into_router(); + if let VersionPolicy::Unversioned = version_policy { + if router.has_versioned_routes() { + return Err(Box::new(UnversionedServerHasVersionedRoutes {})); + } + } + let app_state = Arc::new(DropshotState { private, config: server_config, - router: api.into_router(), + router, log: logger, local_addr, tls_acceptor: Some(acceptor), handler_waitgroup_worker: DebugIgnore(handler_waitgroup_worker), + version_policy, }); let make_service = ServerConnectionHandler::new(Arc::clone(&app_state)); @@ -949,8 +1043,13 @@ async fn http_request_handle( // TODO-correctness: Do we need to dump the body on errors? let method = request.method(); let uri = request.uri(); - let lookup_result = - server.router.lookup_route(&method, uri.path().into())?; + let found_version = + server.version_policy.request_version(&request, &request_log)?; + let lookup_result = server.router.lookup_route( + &method, + uri.path().into(), + found_version.as_ref(), + )?; let rqctx = RequestContext { server: Arc::clone(&server), request: RequestInfo::new(&request, remote_addr), diff --git a/dropshot/src/websocket.rs b/dropshot/src/websocket.rs index 5311e130c..d5e101ecd 100644 --- a/dropshot/src/websocket.rs +++ b/dropshot/src/websocket.rs @@ -299,7 +299,7 @@ mod tests { use crate::server::{DropshotState, ServerConfig}; use crate::{ ExclusiveExtractor, HttpError, RequestContext, RequestInfo, - WebsocketUpgrade, + VersionPolicy, WebsocketUpgrade, }; use debug_ignore::DebugIgnore; use http::Request; @@ -342,6 +342,7 @@ mod tests { handler_waitgroup_worker: DebugIgnore( WaitGroup::new().worker(), ), + version_policy: VersionPolicy::Unversioned, }), request: RequestInfo::new(&request, remote_addr), path_variables: Default::default(), diff --git a/dropshot/tests/fail/bad_endpoint10.stderr b/dropshot/tests/fail/bad_endpoint10.stderr index 01a065e1f..7baafea37 100644 --- a/dropshot/tests/fail/bad_endpoint10.stderr +++ b/dropshot/tests/fail/bad_endpoint10.stderr @@ -13,7 +13,7 @@ note: required by a bound in `validate_result_error_type` 16 | ) -> Result, String> { | ^^^^^^ required by this bound in `validate_result_error_type` -error[E0277]: the trait bound `fn(RequestContext<()>) -> impl Future, String>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_error_type}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied +error[E0277]: the trait bound `fn(RequestContext<()>) -> impl Future, String>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_error_type}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied --> tests/fail/bad_endpoint10.rs:14:10 | 10 | / #[endpoint { @@ -22,7 +22,7 @@ error[E0277]: the trait bound `fn(RequestContext<()>) -> impl Future` is not implemented for fn item `fn(RequestContext<()>) -> impl Future, String>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_error_type}` + | ^^^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(RequestContext<()>) -> impl Future, String>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_error_type}` | note: required by a bound in `ApiEndpoint::::new` --> src/api_description.rs diff --git a/dropshot/tests/fail/bad_endpoint12.stderr b/dropshot/tests/fail/bad_endpoint12.stderr index aa8cd2285..77c3a122b 100644 --- a/dropshot/tests/fail/bad_endpoint12.stderr +++ b/dropshot/tests/fail/bad_endpoint12.stderr @@ -20,7 +20,7 @@ note: required for `Result` to implement `ResultTrait` 15 | ) -> Result { | ^^^^^^ -error[E0277]: the trait bound `fn(RequestContext<()>) -> impl Future> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_response_type}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied +error[E0277]: the trait bound `fn(RequestContext<()>) -> impl Future> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_response_type}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied --> tests/fail/bad_endpoint12.rs:13:10 | 9 | / #[endpoint { @@ -29,7 +29,7 @@ error[E0277]: the trait bound `fn(RequestContext<()>) -> impl Future` is not implemented for fn item `fn(RequestContext<()>) -> impl Future> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_response_type}` + | ^^^^^^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(RequestContext<()>) -> impl Future> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_response_type}` | note: required by a bound in `ApiEndpoint::::new` --> src/api_description.rs diff --git a/dropshot/tests/fail/bad_endpoint15.stderr b/dropshot/tests/fail/bad_endpoint15.stderr index 65d7312d5..08e8a6588 100644 --- a/dropshot/tests/fail/bad_endpoint15.stderr +++ b/dropshot/tests/fail/bad_endpoint15.stderr @@ -1,4 +1,4 @@ -error[E0277]: the trait bound `fn(RequestContext<()>) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied +error[E0277]: the trait bound `fn(RequestContext<()>) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied --> tests/fail/bad_endpoint15.rs:17:10 | 13 | / #[endpoint { @@ -7,7 +7,7 @@ error[E0277]: the trait bound `fn(RequestContext<()>) -> impl Future` is not implemented for fn item `fn(RequestContext<()>) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}` + | ^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(RequestContext<()>) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}` | note: required by a bound in `ApiEndpoint::::new` --> src/api_description.rs diff --git a/dropshot/tests/fail/bad_endpoint17.stderr b/dropshot/tests/fail/bad_endpoint17.stderr index a1e3f0d96..4c81848e6 100644 --- a/dropshot/tests/fail/bad_endpoint17.stderr +++ b/dropshot/tests/fail/bad_endpoint17.stderr @@ -20,7 +20,7 @@ note: required by a bound in `need_shared_extractor` | --------- required by a bound in this function = note: this error originates in the attribute macro `endpoint` (in Nightly builds, run with -Z macro-backtrace for more info) -error[E0277]: the trait bound `fn(RequestContext<()>, TypedBody, UntypedBody) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::two_exclusive_extractors}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied +error[E0277]: the trait bound `fn(RequestContext<()>, TypedBody, UntypedBody) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::two_exclusive_extractors}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied --> tests/fail/bad_endpoint17.rs:28:10 | 24 | / #[endpoint { @@ -29,7 +29,7 @@ error[E0277]: the trait bound `fn(RequestContext<()>, TypedBody, UntypedB 27 | | }] | |__- required by a bound introduced by this call 28 | async fn two_exclusive_extractors( - | ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(RequestContext<()>, TypedBody, UntypedBody) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::two_exclusive_extractors}` + | ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(RequestContext<()>, TypedBody, UntypedBody) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::two_exclusive_extractors}` | note: required by a bound in `ApiEndpoint::::new` --> src/api_description.rs diff --git a/dropshot/tests/fail/bad_endpoint18.stderr b/dropshot/tests/fail/bad_endpoint18.stderr index a2710abac..d9f0a2737 100644 --- a/dropshot/tests/fail/bad_endpoint18.stderr +++ b/dropshot/tests/fail/bad_endpoint18.stderr @@ -20,7 +20,7 @@ note: required by a bound in `need_shared_extractor` | --------- required by a bound in this function = note: this error originates in the attribute macro `endpoint` (in Nightly builds, run with -Z macro-backtrace for more info) -error[E0277]: the trait bound `fn(RequestContext<()>, TypedBody, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::exclusive_extractor_not_last}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied +error[E0277]: the trait bound `fn(RequestContext<()>, TypedBody, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::exclusive_extractor_not_last}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied --> tests/fail/bad_endpoint18.rs:25:10 | 21 | / #[endpoint { @@ -29,7 +29,7 @@ error[E0277]: the trait bound `fn(RequestContext<()>, TypedBody, dropshot 24 | | }] | |__- required by a bound introduced by this call 25 | async fn exclusive_extractor_not_last( - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(RequestContext<()>, TypedBody, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::exclusive_extractor_not_last}` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(RequestContext<()>, TypedBody, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::exclusive_extractor_not_last}` | note: required by a bound in `ApiEndpoint::::new` --> src/api_description.rs diff --git a/dropshot/tests/fail/bad_endpoint19.stderr b/dropshot/tests/fail/bad_endpoint19.stderr index f8bb4bb61..efaf795d9 100644 --- a/dropshot/tests/fail/bad_endpoint19.stderr +++ b/dropshot/tests/fail/bad_endpoint19.stderr @@ -20,7 +20,7 @@ note: required by a bound in `need_shared_extractor` | ------ required by a bound in this function = note: this error originates in the attribute macro `endpoint` (in Nightly builds, run with -Z macro-backtrace for more info) -error[E0277]: the trait bound `fn(RequestContext<()>, std::string::String, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::non_extractor_as_last_argument}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied +error[E0277]: the trait bound `fn(RequestContext<()>, std::string::String, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::non_extractor_as_last_argument}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied --> tests/fail/bad_endpoint19.rs:24:10 | 20 | / #[endpoint { @@ -29,7 +29,7 @@ error[E0277]: the trait bound `fn(RequestContext<()>, std::string::String, drops 23 | | }] | |__- required by a bound introduced by this call 24 | async fn non_extractor_as_last_argument( - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(RequestContext<()>, std::string::String, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::non_extractor_as_last_argument}` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(RequestContext<()>, std::string::String, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::non_extractor_as_last_argument}` | note: required by a bound in `ApiEndpoint::::new` --> src/api_description.rs diff --git a/dropshot/tests/fail/bad_endpoint3.stderr b/dropshot/tests/fail/bad_endpoint3.stderr index d4df7a6ed..2bbb04951 100644 --- a/dropshot/tests/fail/bad_endpoint3.stderr +++ b/dropshot/tests/fail/bad_endpoint3.stderr @@ -25,7 +25,7 @@ note: required by a bound in `need_exclusive_extractor` | ------ required by a bound in this function = note: this error originates in the attribute macro `endpoint` (in Nightly builds, run with -Z macro-backtrace for more info) -error[E0277]: the trait bound `fn(RequestContext<()>, String) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied +error[E0277]: the trait bound `fn(RequestContext<()>, String) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied --> tests/fail/bad_endpoint3.rs:16:10 | 12 | / #[endpoint { @@ -34,7 +34,7 @@ error[E0277]: the trait bound `fn(RequestContext<()>, String) -> impl Future` is not implemented for fn item `fn(RequestContext<()>, String) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}` + | ^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(RequestContext<()>, String) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}` | note: required by a bound in `ApiEndpoint::::new` --> src/api_description.rs diff --git a/dropshot/tests/fail/bad_endpoint4.stderr b/dropshot/tests/fail/bad_endpoint4.stderr index fe4eccc81..549f7aa9a 100644 --- a/dropshot/tests/fail/bad_endpoint4.stderr +++ b/dropshot/tests/fail/bad_endpoint4.stderr @@ -98,7 +98,7 @@ note: required by a bound in `dropshot::Query` | pub struct Query { | ^^^^^^^^^^^^^^^^ required by this bound in `Query` -error[E0277]: the trait bound `fn(RequestContext<()>, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied +error[E0277]: the trait bound `fn(RequestContext<()>, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied --> tests/fail/bad_endpoint4.rs:21:10 | 17 | / #[endpoint { @@ -107,7 +107,7 @@ error[E0277]: the trait bound `fn(RequestContext<()>, dropshot::Query` is not implemented for fn item `fn(RequestContext<()>, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}` + | ^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(RequestContext<()>, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}` | note: required by a bound in `ApiEndpoint::::new` --> src/api_description.rs diff --git a/dropshot/tests/fail/bad_endpoint5.stderr b/dropshot/tests/fail/bad_endpoint5.stderr index de826f370..d907b829d 100644 --- a/dropshot/tests/fail/bad_endpoint5.stderr +++ b/dropshot/tests/fail/bad_endpoint5.stderr @@ -51,7 +51,7 @@ note: required by a bound in `dropshot::Query` | pub struct Query { | ^^^^^^^^^^^^^^^^ required by this bound in `Query` -error[E0277]: the trait bound `fn(RequestContext<()>, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied +error[E0277]: the trait bound `fn(RequestContext<()>, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied --> tests/fail/bad_endpoint5.rs:23:10 | 19 | / #[endpoint { @@ -60,7 +60,7 @@ error[E0277]: the trait bound `fn(RequestContext<()>, dropshot::Query` is not implemented for fn item `fn(RequestContext<()>, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}` + | ^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(RequestContext<()>, dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}` | note: required by a bound in `ApiEndpoint::::new` --> src/api_description.rs diff --git a/dropshot/tests/fail/bad_endpoint7.stderr b/dropshot/tests/fail/bad_endpoint7.stderr index 8decd8673..7c5b3c57f 100644 --- a/dropshot/tests/fail/bad_endpoint7.stderr +++ b/dropshot/tests/fail/bad_endpoint7.stderr @@ -158,7 +158,7 @@ note: required by a bound in `HttpResponseOk` | pub struct HttpResponseOk( | ^^^^^^^^^^^^^^^^^^^ required by this bound in `HttpResponseOk` -error[E0277]: the trait bound `fn(RequestContext<()>) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied +error[E0277]: the trait bound `fn(RequestContext<()>) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied --> tests/fail/bad_endpoint7.rs:22:10 | 18 | / #[endpoint { @@ -167,7 +167,7 @@ error[E0277]: the trait bound `fn(RequestContext<()>) -> impl Future` is not implemented for fn item `fn(RequestContext<()>) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}` + | ^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(RequestContext<()>) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}` | note: required by a bound in `ApiEndpoint::::new` --> src/api_description.rs diff --git a/dropshot/tests/fail/bad_endpoint9.stderr b/dropshot/tests/fail/bad_endpoint9.stderr index 9317957d8..072b25588 100644 --- a/dropshot/tests/fail/bad_endpoint9.stderr +++ b/dropshot/tests/fail/bad_endpoint9.stderr @@ -18,7 +18,7 @@ error[E0277]: the trait bound `dropshot::Query: RequestContextArgum = help: the trait `RequestContextArgument` is implemented for `RequestContext` = note: this error originates in the attribute macro `endpoint` (in Nightly builds, run with -Z macro-backtrace for more info) -error[E0277]: the trait bound `fn(dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied +error[E0277]: the trait bound `fn(dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}: dropshot::handler::HttpHandlerFunc<_, _, _>` is not satisfied --> tests/fail/bad_endpoint9.rs:24:10 | 20 | / #[endpoint { @@ -27,7 +27,7 @@ error[E0277]: the trait bound `fn(dropshot::Query) -> impl Future` is not implemented for fn item `fn(dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}` + | ^^^^^^^^^^^^ the trait `dropshot::handler::HttpHandlerFunc<_, _, _>` is not implemented for fn item `fn(dropshot::Query) -> impl Future, HttpError>> { for ApiEndpoint< as RequestContextArgument>::Context>>::from::bad_endpoint}` | note: required by a bound in `ApiEndpoint::::new` --> src/api_description.rs diff --git a/dropshot/tests/test_openapi.json b/dropshot/tests/test_openapi.json index 9ce08e203..7a4a3cbab 100644 --- a/dropshot/tests/test_openapi.json +++ b/dropshot/tests/test_openapi.json @@ -2,7 +2,7 @@ "openapi": "3.0.3", "info": { "title": "test", - "version": "threeve" + "version": "3.5.0" }, "paths": { "/datagoeshere": { diff --git a/dropshot/tests/test_openapi.rs b/dropshot/tests/test_openapi.rs index d4402a8b5..685a5a174 100644 --- a/dropshot/tests/test_openapi.rs +++ b/dropshot/tests/test_openapi.rs @@ -515,7 +515,8 @@ fn test_openapi() -> anyhow::Result<()> { let api = make_api(None)?; let mut output = Cursor::new(Vec::new()); - let _ = api.openapi("test", "threeve").write(&mut output); + let _ = + api.openapi("test", semver::Version::new(3, 5, 0)).write(&mut output); let actual = from_utf8(output.get_ref()).unwrap(); expectorate::assert_contents("tests/test_openapi.json", actual); @@ -541,7 +542,7 @@ fn test_openapi_fuller() -> anyhow::Result<()> { let mut output = Cursor::new(Vec::new()); let _ = api - .openapi("test", "1985.7") + .openapi("test", semver::Version::new(1985, 7, 0)) .description("gusty winds may exist") .contact_name("old mate") .license_name("CDDL") diff --git a/dropshot/tests/test_openapi_fuller.json b/dropshot/tests/test_openapi_fuller.json index eabb5bd5b..b1a6b5f8e 100644 --- a/dropshot/tests/test_openapi_fuller.json +++ b/dropshot/tests/test_openapi_fuller.json @@ -10,7 +10,7 @@ "license": { "name": "CDDL" }, - "version": "1985.7" + "version": "1985.7.0" }, "paths": { "/datagoeshere": { diff --git a/dropshot/tests/test_pagination_schema.json b/dropshot/tests/test_pagination_schema.json index b70d416e7..e28d0135d 100644 --- a/dropshot/tests/test_pagination_schema.json +++ b/dropshot/tests/test_pagination_schema.json @@ -10,7 +10,7 @@ "license": { "name": "CDDL" }, - "version": "1985.7" + "version": "1985.7.0" }, "paths": { "/super_pages": { diff --git a/dropshot/tests/test_pagination_schema.rs b/dropshot/tests/test_pagination_schema.rs index 8f1f85fb8..70f5386c0 100644 --- a/dropshot/tests/test_pagination_schema.rs +++ b/dropshot/tests/test_pagination_schema.rs @@ -48,7 +48,7 @@ fn test_pagination_schema() -> anyhow::Result<()> { let mut output = Cursor::new(Vec::new()); let _ = api - .openapi("test", "1985.7") + .openapi("test", semver::Version::new(1985, 7, 0)) .description("gusty winds may exist") .contact_name("old mate") .license_name("CDDL") diff --git a/dropshot_endpoint/Cargo.toml b/dropshot_endpoint/Cargo.toml index 3b96696ad..6510fdae6 100644 --- a/dropshot_endpoint/Cargo.toml +++ b/dropshot_endpoint/Cargo.toml @@ -17,6 +17,7 @@ workspace = true heck = "0.5.0" proc-macro2 = "1" quote = "1" +semver = "1.0.23" serde_tokenstream = "0.2.2" [dependencies.serde] diff --git a/dropshot_endpoint/src/api_trait.rs b/dropshot_endpoint/src/api_trait.rs index 789aa4a3f..12d84cfa0 100644 --- a/dropshot_endpoint/src/api_trait.rs +++ b/dropshot_endpoint/src/api_trait.rs @@ -1661,12 +1661,20 @@ mod tests { trait MyTrait { type Context; - #[endpoint { method = GET, path = "/xyz" }] + #[endpoint { + method = GET, + path = "/xyz", + versions = "1.2.3".. + }] async fn handler_xyz( rqctx: RequestContext, ) -> Result, HttpError>; - #[channel { protocol = WEBSOCKETS, path = "/ws" }] + #[channel { + protocol = WEBSOCKETS, + path = "/ws", + versions = .. + }] async fn handler_ws( rqctx: RequestContext, upgraded: WebsocketConnection, diff --git a/dropshot_endpoint/src/channel.rs b/dropshot_endpoint/src/channel.rs index d3a199ce4..53dec666a 100644 --- a/dropshot_endpoint/src/channel.rs +++ b/dropshot_endpoint/src/channel.rs @@ -613,4 +613,33 @@ mod tests { &prettyplease::unparse(&parse_quote! { #item }), ); } + + #[test] + fn test_channel_with_versions() { + let input = quote! { + async fn my_channel( + _rqctx: RequestContext<()>, + _conn: WebsocketConnection, + ) -> WebsocketChannelResult { + Ok(()) + } + }; + + let (item, errors) = do_channel( + quote! { + protocol = WEBSOCKETS, + path = "/my/ws/channel", + versions = .."1.2.3", + }, + input.clone(), + ); + + assert!(errors.is_empty()); + + let file = parse_quote! { #item }; + assert_contents( + "tests/output/channel_with_versions.rs", + &prettyplease::unparse(&file), + ); + } } diff --git a/dropshot_endpoint/src/endpoint.rs b/dropshot_endpoint/src/endpoint.rs index 31361909f..5c1c45b0c 100644 --- a/dropshot_endpoint/src/endpoint.rs +++ b/dropshot_endpoint/src/endpoint.rs @@ -802,6 +802,102 @@ mod tests { ); } + #[test] + fn test_endpoint_with_versions_all() { + let (item, errors) = do_endpoint( + quote! { + method = GET, + path = "/test", + versions = .. + }, + quote! { + async fn handler_xyz( + _: RequestContext<()>, + ) -> Result { + Ok(()) + } + }, + ); + + assert!(errors.is_empty()); + assert_contents( + "tests/output/endpoint_with_versions_all.rs", + &prettyplease::unparse(&parse_quote! { #item }), + ); + } + + #[test] + fn test_endpoint_with_versions_from() { + let (item, errors) = do_endpoint( + quote! { + method = GET, + path = "/test", + versions = "1.2.3".. + }, + quote! { + async fn handler_xyz( + _: RequestContext<()>, + ) -> Result { + Ok(()) + } + }, + ); + + assert!(errors.is_empty()); + assert_contents( + "tests/output/endpoint_with_versions_from.rs", + &prettyplease::unparse(&parse_quote! { #item }), + ); + } + + #[test] + fn test_endpoint_with_versions_until() { + let (item, errors) = do_endpoint( + quote! { + method = GET, + path = "/test", + versions = .."1.2.3" + }, + quote! { + async fn handler_xyz( + _: RequestContext<()>, + ) -> Result { + Ok(()) + } + }, + ); + + assert!(errors.is_empty()); + assert_contents( + "tests/output/endpoint_with_versions_until.rs", + &prettyplease::unparse(&parse_quote! { #item }), + ); + } + + #[test] + fn test_endpoint_with_versions_from_until() { + let (item, errors) = do_endpoint( + quote! { + method = GET, + path = "/test", + versions = "1.2.3".."4.5.6", + }, + quote! { + async fn handler_xyz( + _: RequestContext<()>, + ) -> Result { + Ok(()) + } + }, + ); + + assert!(errors.is_empty()); + assert_contents( + "tests/output/endpoint_with_versions_from_until.rs", + &prettyplease::unparse(&parse_quote! { #item }), + ); + } + #[test] fn test_endpoint_invalid_item() { let (_, errors) = do_endpoint( @@ -920,4 +1016,139 @@ mod tests { Some("endpoint `handler_xyz` must have at least one RequestContext argument".to_string()) ); } + + #[test] + fn test_endpoint_bad_versions() { + let (_, errors) = do_endpoint( + quote! { + method = GET, + path = "/a/b/c", + versions = 1.2.3, + }, + quote! { + pub async fn handler_xyz( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + Ok(()) + } + }, + ); + assert!(!errors.is_empty()); + assert_eq!( + errors.get(0).map(ToString::to_string), + Some("expected string literal".to_string()), + ); + + let (_, errors) = do_endpoint( + quote! { + method = GET, + path = "/a/b/c", + versions = "one dot two dot three", + }, + quote! { + pub async fn handler_xyz( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + Ok(()) + } + }, + ); + assert!(!errors.is_empty()); + assert_eq!( + errors.get(0).map(ToString::to_string), + Some( + "expected semver: unexpected character 'o' while \ + parsing major version number" + .to_string() + ), + ); + + let (_, errors) = do_endpoint( + quote! { + method = GET, + path = "/a/b/c", + versions = "1.2", + }, + quote! { + pub async fn handler_xyz( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + Ok(()) + } + }, + ); + assert!(!errors.is_empty()); + assert_eq!( + errors.get(0).map(ToString::to_string), + Some( + "expected semver: unexpected end of input while parsing \ + minor version number" + .to_string() + ), + ); + + let (_, errors) = do_endpoint( + quote! { + method = GET, + path = "/a/b/c", + versions = "1.2.3-pre", + }, + quote! { + pub async fn handler_xyz( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + Ok(()) + } + }, + ); + assert!(!errors.is_empty()); + assert_eq!( + errors.get(0).map(ToString::to_string), + Some("semver pre-release string is not supported here".to_string()), + ); + + let (_, errors) = do_endpoint( + quote! { + method = GET, + path = "/a/b/c", + versions = "1.2.3+latest", + }, + quote! { + pub async fn handler_xyz( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + Ok(()) + } + }, + ); + assert!(!errors.is_empty()); + assert_eq!( + errors.get(0).map(ToString::to_string), + Some("semver build metadata is not supported here".to_string()), + ); + + let (_, errors) = do_endpoint( + quote! { + method = GET, + path = "/a/b/c", + versions = "1.2.5".."1.2.3", + }, + quote! { + pub async fn handler_xyz( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + Ok(()) + } + }, + ); + assert!(!errors.is_empty()); + assert_eq!( + errors.get(0).map(ToString::to_string), + Some( + "semver range (from ... until ...) has the endpoints \ + out of order" + .to_string() + ), + ); + } } diff --git a/dropshot_endpoint/src/metadata.rs b/dropshot_endpoint/src/metadata.rs index 09614fe8a..18a63ed71 100644 --- a/dropshot_endpoint/src/metadata.rs +++ b/dropshot_endpoint/src/metadata.rs @@ -2,7 +2,7 @@ //! Code to handle metadata associated with an endpoint. -use proc_macro2::TokenStream; +use proc_macro2::{TokenStream, TokenTree}; use quote::{format_ident, quote, quote_spanned, ToTokens}; use serde::Deserialize; use syn::{spanned::Spanned, Error}; @@ -12,6 +12,7 @@ use crate::{ error_store::ErrorSink, util::{get_crate, is_wildcard_path, MacroKind, ValidContentType}, }; +use serde_tokenstream::ParseWrapper; #[allow(non_snake_case)] #[derive(Deserialize, Debug)] @@ -51,6 +52,7 @@ pub(crate) struct EndpointMetadata { pub(crate) deprecated: bool, pub(crate) content_type: Option, pub(crate) _dropshot_crate: Option, + pub(crate) versions: Option>, } impl EndpointMetadata { @@ -81,6 +83,7 @@ impl EndpointMetadata { deprecated, content_type, _dropshot_crate, + versions, } = self; if kind == MacroKind::Trait && _dropshot_crate.is_some() { @@ -136,6 +139,9 @@ impl EndpointMetadata { unpublished, deprecated, content_type, + versions: versions + .map(|h| h.into_inner()) + .unwrap_or(VersionRange::All), }) } else { unreachable!("no validation errors, but content_type is None") @@ -151,6 +157,14 @@ pub(crate) struct ValidatedEndpointMetadata { unpublished: bool, deprecated: bool, content_type: ValidContentType, + versions: VersionRange, +} + +fn semver_parts(x: &semver::Version) -> (u64, u64, u64) { + // This was validated during validation. + assert_eq!(x.pre, semver::Prerelease::EMPTY); + assert_eq!(x.build, semver::BuildMetadata::EMPTY); + (x.major, x.minor, x.patch) } impl ValidatedEndpointMetadata { @@ -188,6 +202,36 @@ impl ValidatedEndpointMetadata { quote! { .deprecated(true) } }); + let versions = match &self.versions { + VersionRange::All => quote! { #dropshot::ApiEndpointVersions::All }, + VersionRange::From(x) => { + let (major, minor, patch) = semver_parts(&x); + quote! { + #dropshot::ApiEndpointVersions::From( + semver::Version::new(#major, #minor, #patch) + ) + } + } + VersionRange::Until(y) => { + let (major, minor, patch) = semver_parts(&y); + quote! { + #dropshot::ApiEndpointVersions::Until( + semver::Version::new(#major, #minor, #patch) + ) + } + } + VersionRange::FromUntil(x, y) => { + let (xmajor, xminor, xpatch) = semver_parts(&x); + let (ymajor, yminor, ypatch) = semver_parts(&y); + quote! { + #dropshot::ApiEndpointVersions::from_until( + semver::Version::new(#xmajor, #xminor, #xpatch), + semver::Version::new(#ymajor, #yminor, #ypatch), + ).unwrap() + } + } + }; + let fn_call = match kind { ApiEndpointKind::Regular(endpoint_fn) => { quote_spanned! {endpoint_fn.span()=> @@ -197,6 +241,7 @@ impl ValidatedEndpointMetadata { #dropshot::Method::#method_ident, #content_type, #path, + #versions, ) } } @@ -216,6 +261,7 @@ impl ValidatedEndpointMetadata { #dropshot::Method::#method_ident, #content_type, #path, + #versions, ) } } @@ -232,6 +278,85 @@ impl ValidatedEndpointMetadata { } } +#[derive(Debug)] +pub(crate) enum VersionRange { + All, + From(semver::Version), + Until(semver::Version), + FromUntil(semver::Version, semver::Version), +} + +fn parse_semver(v: &syn::LitStr) -> syn::Result { + v.value() + .parse::() + .map_err(|e| { + syn::Error::new_spanned(v, format!("expected semver: {}", e)) + }) + .and_then(|s| { + if s.pre == semver::Prerelease::EMPTY { + Ok(s) + } else { + Err(syn::Error::new_spanned( + v, + format!("semver pre-release string is not supported here"), + )) + } + }) + .and_then(|s| { + if s.build == semver::BuildMetadata::EMPTY { + Ok(s) + } else { + Err(syn::Error::new_spanned( + v, + format!("semver build metadata is not supported here"), + )) + } + }) +} + +impl syn::parse::Parse for VersionRange { + fn parse(input: syn::parse::ParseStream) -> syn::Result { + let lookahead = input.lookahead1(); + if lookahead.peek(syn::Token![..]) { + let _ = input.parse::()?; + if input.is_empty() { + Ok(VersionRange::All) + } else { + let latest = input.parse::()?; + let latest_semver = parse_semver(&latest)?; + Ok(VersionRange::Until(latest_semver)) + } + } else { + let earliest = input.parse::()?; + let earliest_semver = parse_semver(&earliest)?; + let _ = input.parse::()?; + let lookahead = input.lookahead1(); + if lookahead.peek(syn::LitStr) { + let latest = input.parse::()?; + let latest_semver = parse_semver(&latest)?; + if latest_semver < earliest_semver { + let span: TokenStream = [ + TokenTree::from(earliest.token()), + TokenTree::from(latest.token()), + ] + .into_iter() + .collect(); + return Err(syn::Error::new_spanned( + span, + format!( + "semver range (from ... until ...) has the \ + endpoints out of order" + ), + )); + } + Ok(VersionRange::FromUntil(earliest_semver, latest_semver)) + } else { + Ok(VersionRange::From(earliest_semver)) + } + } + } +} + #[allow(non_snake_case)] #[derive(Deserialize, Debug)] pub(crate) enum ChannelProtocol { @@ -249,6 +374,7 @@ pub(crate) struct ChannelMetadata { #[serde(default)] pub(crate) deprecated: bool, pub(crate) _dropshot_crate: Option, + pub(crate) versions: Option>, } impl ChannelMetadata { @@ -278,6 +404,7 @@ impl ChannelMetadata { unpublished, deprecated, _dropshot_crate, + versions, } = self; if kind == MacroKind::Trait && _dropshot_crate.is_some() { @@ -313,6 +440,9 @@ impl ChannelMetadata { unpublished, deprecated, content_type: ValidContentType::ApplicationJson, + versions: versions + .map(|h| h.into_inner()) + .unwrap_or(VersionRange::All), }; Some(ValidatedChannelMetadata { inner }) diff --git a/dropshot_endpoint/tests/output/api_trait_basic.rs b/dropshot_endpoint/tests/output/api_trait_basic.rs index 99eada816..d3452f45b 100644 --- a/dropshot_endpoint/tests/output/api_trait_basic.rs +++ b/dropshot_endpoint/tests/output/api_trait_basic.rs @@ -72,6 +72,9 @@ mod my_trait_mod { dropshot::Method::GET, "application/json", "/xyz", + dropshot::ApiEndpointVersions::From( + semver::Version::new(1u64, 2u64, 3u64), + ), ); if let Err(error) = dropshot_api.register(endpoint_handler_xyz) { dropshot_errors.push(error); @@ -86,6 +89,7 @@ mod my_trait_mod { dropshot::Method::GET, "application/json", "/ws", + dropshot::ApiEndpointVersions::All, ); if let Err(error) = dropshot_api.register(endpoint_handler_ws) { dropshot_errors.push(error); @@ -159,6 +163,9 @@ mod my_trait_mod { dropshot::Method::GET, "application/json", "/xyz", + dropshot::ApiEndpointVersions::From( + semver::Version::new(1u64, 2u64, 3u64), + ), ); if let Err(error) = dropshot_api.register(endpoint_handler_xyz) { dropshot_errors.push(error); @@ -182,6 +189,7 @@ mod my_trait_mod { dropshot::Method::GET, "application/json", "/ws", + dropshot::ApiEndpointVersions::All, ); if let Err(error) = dropshot_api.register(endpoint_handler_ws) { dropshot_errors.push(error); diff --git a/dropshot_endpoint/tests/output/api_trait_with_custom_params.rs b/dropshot_endpoint/tests/output/api_trait_with_custom_params.rs index 1997af94c..b2c20103c 100644 --- a/dropshot_endpoint/tests/output/api_trait_with_custom_params.rs +++ b/dropshot_endpoint/tests/output/api_trait_with_custom_params.rs @@ -96,6 +96,7 @@ pub mod my_support_module { topspin::Method::GET, "application/json", "/xyz", + topspin::ApiEndpointVersions::All, ); if let Err(error) = dropshot_api.register(endpoint_handler_xyz) { dropshot_errors.push(error); @@ -105,7 +106,13 @@ pub mod my_support_module { let endpoint_handler_ws = topspin::ApiEndpoint::new_for_types::< (topspin::WebsocketUpgrade,), topspin::WebsocketEndpointResult, - >("handler_ws".to_string(), topspin::Method::GET, "application/json", "/ws"); + >( + "handler_ws".to_string(), + topspin::Method::GET, + "application/json", + "/ws", + topspin::ApiEndpointVersions::All, + ); if let Err(error) = dropshot_api.register(endpoint_handler_ws) { dropshot_errors.push(error); } @@ -202,6 +209,7 @@ pub mod my_support_module { topspin::Method::GET, "application/json", "/xyz", + topspin::ApiEndpointVersions::All, ); if let Err(error) = dropshot_api.register(endpoint_handler_xyz) { dropshot_errors.push(error); @@ -225,6 +233,7 @@ pub mod my_support_module { topspin::Method::GET, "application/json", "/ws", + topspin::ApiEndpointVersions::All, ); if let Err(error) = dropshot_api.register(endpoint_handler_ws) { dropshot_errors.push(error); diff --git a/dropshot_endpoint/tests/output/channel_with_custom_params.rs b/dropshot_endpoint/tests/output/channel_with_custom_params.rs index f3adfe78f..531b42819 100644 --- a/dropshot_endpoint/tests/output/channel_with_custom_params.rs +++ b/dropshot_endpoint/tests/output/channel_with_custom_params.rs @@ -70,6 +70,7 @@ for topspin::ApiEndpoint< topspin::Method::GET, "application/json", "/my/ws/channel", + topspin::ApiEndpointVersions::All, ) } } diff --git a/dropshot_endpoint/tests/output/channel_with_unnamed_params.rs b/dropshot_endpoint/tests/output/channel_with_unnamed_params.rs index d4aece24d..5d18b2ec5 100644 --- a/dropshot_endpoint/tests/output/channel_with_unnamed_params.rs +++ b/dropshot_endpoint/tests/output/channel_with_unnamed_params.rs @@ -80,6 +80,7 @@ for dropshot::ApiEndpoint< dropshot::Method::GET, "application/json", "/my/ws/channel", + dropshot::ApiEndpointVersions::All, ) } } diff --git a/dropshot_endpoint/tests/output/channel_with_versions.rs b/dropshot_endpoint/tests/output/channel_with_versions.rs new file mode 100644 index 000000000..a891febb3 --- /dev/null +++ b/dropshot_endpoint/tests/output/channel_with_versions.rs @@ -0,0 +1,64 @@ +const _: fn() = || { + struct NeedRequestContext( + as dropshot::RequestContextArgument>::Context, + ); +}; +const _: fn() = || { + trait TypeEq { + type This: ?Sized; + } + impl TypeEq for T { + type This = Self; + } + fn validate_websocket_connection_type() + where + T: ?Sized + TypeEq, + {} + validate_websocket_connection_type::(); +}; +#[allow(non_camel_case_types, missing_docs)] +///API Endpoint: my_channel +struct my_channel {} +#[allow(non_upper_case_globals, missing_docs)] +///API Endpoint: my_channel +const my_channel: my_channel = my_channel {}; +impl From +for dropshot::ApiEndpoint< + as dropshot::RequestContextArgument>::Context, +> { + fn from(_: my_channel) -> Self { + #[allow(clippy::unused_async)] + async fn my_channel( + _rqctx: RequestContext<()>, + _conn: WebsocketConnection, + ) -> WebsocketChannelResult { + Ok(()) + } + const _: fn() = || { + fn future_endpoint_must_be_send(_t: T) {} + fn check_future_bounds( + arg0: RequestContext<()>, + __dropshot_websocket: WebsocketConnection, + ) { + future_endpoint_must_be_send(my_channel(arg0, __dropshot_websocket)); + } + }; + async fn my_channel_adapter( + arg0: RequestContext<()>, + __dropshot_websocket: dropshot::WebsocketUpgrade, + ) -> dropshot::WebsocketEndpointResult { + __dropshot_websocket + .handle(move |__dropshot_websocket: WebsocketConnection| async move { + my_channel(arg0, __dropshot_websocket).await + }) + } + dropshot::ApiEndpoint::new( + "my_channel".to_string(), + my_channel_adapter, + dropshot::Method::GET, + "application/json", + "/my/ws/channel", + dropshot::ApiEndpointVersions::Until(semver::Version::new(1u64, 2u64, 3u64)), + ) + } +} diff --git a/dropshot_endpoint/tests/output/endpoint_basic.rs b/dropshot_endpoint/tests/output/endpoint_basic.rs index b967d5597..916d25ebc 100644 --- a/dropshot_endpoint/tests/output/endpoint_basic.rs +++ b/dropshot_endpoint/tests/output/endpoint_basic.rs @@ -59,6 +59,7 @@ for dropshot::ApiEndpoint< dropshot::Method::GET, "application/json", "/a/b/c", + dropshot::ApiEndpointVersions::All, ) } } diff --git a/dropshot_endpoint/tests/output/endpoint_content_type.rs b/dropshot_endpoint/tests/output/endpoint_content_type.rs index 46ccdfe70..97a305d43 100644 --- a/dropshot_endpoint/tests/output/endpoint_content_type.rs +++ b/dropshot_endpoint/tests/output/endpoint_content_type.rs @@ -59,6 +59,7 @@ for dropshot::ApiEndpoint< dropshot::Method::POST, "application/x-www-form-urlencoded", "/a/b/c", + dropshot::ApiEndpointVersions::All, ) } } diff --git a/dropshot_endpoint/tests/output/endpoint_context_fully_qualified_names.rs b/dropshot_endpoint/tests/output/endpoint_context_fully_qualified_names.rs index 7fe68014c..c126c9275 100644 --- a/dropshot_endpoint/tests/output/endpoint_context_fully_qualified_names.rs +++ b/dropshot_endpoint/tests/output/endpoint_context_fully_qualified_names.rs @@ -67,6 +67,7 @@ for dropshot::ApiEndpoint< dropshot::Method::GET, "application/json", "/a/b/c", + dropshot::ApiEndpointVersions::All, ) } } diff --git a/dropshot_endpoint/tests/output/endpoint_pub_crate.rs b/dropshot_endpoint/tests/output/endpoint_pub_crate.rs index 4d5fb1faa..a6cc141ab 100644 --- a/dropshot_endpoint/tests/output/endpoint_pub_crate.rs +++ b/dropshot_endpoint/tests/output/endpoint_pub_crate.rs @@ -67,6 +67,7 @@ for dropshot::ApiEndpoint< dropshot::Method::GET, "application/json", "/a/b/c", + dropshot::ApiEndpointVersions::All, ) } } diff --git a/dropshot_endpoint/tests/output/endpoint_weird_but_ok_arg_types_1.rs b/dropshot_endpoint/tests/output/endpoint_weird_but_ok_arg_types_1.rs index 00bd3d7d9..7a84672dc 100644 --- a/dropshot_endpoint/tests/output/endpoint_weird_but_ok_arg_types_1.rs +++ b/dropshot_endpoint/tests/output/endpoint_weird_but_ok_arg_types_1.rs @@ -84,6 +84,7 @@ for dropshot::ApiEndpoint< dropshot::Method::GET, "application/json", "/a/b/c", + dropshot::ApiEndpointVersions::All, ) .summary("handle \"xyz\" requests") } diff --git a/dropshot_endpoint/tests/output/endpoint_weird_but_ok_arg_types_2.rs b/dropshot_endpoint/tests/output/endpoint_weird_but_ok_arg_types_2.rs index 294fcb198..180943b10 100644 --- a/dropshot_endpoint/tests/output/endpoint_weird_but_ok_arg_types_2.rs +++ b/dropshot_endpoint/tests/output/endpoint_weird_but_ok_arg_types_2.rs @@ -64,6 +64,7 @@ for dropshot::ApiEndpoint< dropshot::Method::GET, "application/json", "/a/b/c", + dropshot::ApiEndpointVersions::All, ) .summary("handle \"xyz\" requests") } diff --git a/dropshot_endpoint/tests/output/endpoint_with_custom_params.rs b/dropshot_endpoint/tests/output/endpoint_with_custom_params.rs index ff0d26c45..31e9fa89c 100644 --- a/dropshot_endpoint/tests/output/endpoint_with_custom_params.rs +++ b/dropshot_endpoint/tests/output/endpoint_with_custom_params.rs @@ -79,6 +79,7 @@ for topspin::ApiEndpoint< topspin::Method::GET, "application/json", "/a/b/c", + topspin::ApiEndpointVersions::All, ) } } diff --git a/dropshot_endpoint/tests/output/endpoint_with_doc.rs b/dropshot_endpoint/tests/output/endpoint_with_doc.rs index febc155e2..a117dd324 100644 --- a/dropshot_endpoint/tests/output/endpoint_with_doc.rs +++ b/dropshot_endpoint/tests/output/endpoint_with_doc.rs @@ -62,6 +62,7 @@ for dropshot::ApiEndpoint< dropshot::Method::GET, "application/json", "/a/b/c", + dropshot::ApiEndpointVersions::All, ) .summary("handle \"xyz\" requests") } diff --git a/dropshot_endpoint/tests/output/endpoint_with_empty_where_clause.rs b/dropshot_endpoint/tests/output/endpoint_with_empty_where_clause.rs index b967d5597..916d25ebc 100644 --- a/dropshot_endpoint/tests/output/endpoint_with_empty_where_clause.rs +++ b/dropshot_endpoint/tests/output/endpoint_with_empty_where_clause.rs @@ -59,6 +59,7 @@ for dropshot::ApiEndpoint< dropshot::Method::GET, "application/json", "/a/b/c", + dropshot::ApiEndpointVersions::All, ) } } diff --git a/dropshot_endpoint/tests/output/endpoint_with_query.rs b/dropshot_endpoint/tests/output/endpoint_with_query.rs index 9c0b6208c..704f00d1e 100644 --- a/dropshot_endpoint/tests/output/endpoint_with_query.rs +++ b/dropshot_endpoint/tests/output/endpoint_with_query.rs @@ -67,6 +67,7 @@ for dropshot::ApiEndpoint< dropshot::Method::GET, "application/json", "/a/b/c", + dropshot::ApiEndpointVersions::All, ) } } diff --git a/dropshot_endpoint/tests/output/endpoint_with_tags.rs b/dropshot_endpoint/tests/output/endpoint_with_tags.rs index cd3c50e09..68c36c1f4 100644 --- a/dropshot_endpoint/tests/output/endpoint_with_tags.rs +++ b/dropshot_endpoint/tests/output/endpoint_with_tags.rs @@ -59,6 +59,7 @@ for dropshot::ApiEndpoint< dropshot::Method::GET, "application/json", "/a/b/c", + dropshot::ApiEndpointVersions::All, ) .tag("stuff") .tag("things") diff --git a/dropshot_endpoint/tests/output/endpoint_with_unnamed_params.rs b/dropshot_endpoint/tests/output/endpoint_with_unnamed_params.rs index d4568b8cd..63419caf3 100644 --- a/dropshot_endpoint/tests/output/endpoint_with_unnamed_params.rs +++ b/dropshot_endpoint/tests/output/endpoint_with_unnamed_params.rs @@ -90,6 +90,7 @@ for dropshot::ApiEndpoint< dropshot::Method::GET, "application/json", "/test", + dropshot::ApiEndpointVersions::All, ) } } diff --git a/dropshot_endpoint/tests/output/endpoint_with_versions_all.rs b/dropshot_endpoint/tests/output/endpoint_with_versions_all.rs new file mode 100644 index 000000000..fe36fd4fd --- /dev/null +++ b/dropshot_endpoint/tests/output/endpoint_with_versions_all.rs @@ -0,0 +1,67 @@ +const _: fn() = || { + struct NeedRequestContext( + as dropshot::RequestContextArgument>::Context, + ); +}; +const _: fn() = || { + trait ResultTrait { + type T; + type E; + } + impl ResultTrait for Result + where + TT: dropshot::HttpResponse, + { + type T = TT; + type E = EE; + } + struct NeedHttpResponse( + as ResultTrait>::T, + ); + trait TypeEq { + type This: ?Sized; + } + impl TypeEq for T { + type This = Self; + } + fn validate_result_error_type() + where + T: ?Sized + TypeEq, + {} + validate_result_error_type::< + as ResultTrait>::E, + >(); +}; +#[allow(non_camel_case_types, missing_docs)] +///API Endpoint: handler_xyz +struct handler_xyz {} +#[allow(non_upper_case_globals, missing_docs)] +///API Endpoint: handler_xyz +const handler_xyz: handler_xyz = handler_xyz {}; +impl From +for dropshot::ApiEndpoint< + as dropshot::RequestContextArgument>::Context, +> { + fn from(_: handler_xyz) -> Self { + #[allow(clippy::unused_async)] + async fn handler_xyz( + _: RequestContext<()>, + ) -> Result { + Ok(()) + } + const _: fn() = || { + fn future_endpoint_must_be_send(_t: T) {} + fn check_future_bounds(arg0: RequestContext<()>) { + future_endpoint_must_be_send(handler_xyz(arg0)); + } + }; + dropshot::ApiEndpoint::new( + "handler_xyz".to_string(), + handler_xyz, + dropshot::Method::GET, + "application/json", + "/test", + dropshot::ApiEndpointVersions::All, + ) + } +} diff --git a/dropshot_endpoint/tests/output/endpoint_with_versions_from.rs b/dropshot_endpoint/tests/output/endpoint_with_versions_from.rs new file mode 100644 index 000000000..79aae9062 --- /dev/null +++ b/dropshot_endpoint/tests/output/endpoint_with_versions_from.rs @@ -0,0 +1,67 @@ +const _: fn() = || { + struct NeedRequestContext( + as dropshot::RequestContextArgument>::Context, + ); +}; +const _: fn() = || { + trait ResultTrait { + type T; + type E; + } + impl ResultTrait for Result + where + TT: dropshot::HttpResponse, + { + type T = TT; + type E = EE; + } + struct NeedHttpResponse( + as ResultTrait>::T, + ); + trait TypeEq { + type This: ?Sized; + } + impl TypeEq for T { + type This = Self; + } + fn validate_result_error_type() + where + T: ?Sized + TypeEq, + {} + validate_result_error_type::< + as ResultTrait>::E, + >(); +}; +#[allow(non_camel_case_types, missing_docs)] +///API Endpoint: handler_xyz +struct handler_xyz {} +#[allow(non_upper_case_globals, missing_docs)] +///API Endpoint: handler_xyz +const handler_xyz: handler_xyz = handler_xyz {}; +impl From +for dropshot::ApiEndpoint< + as dropshot::RequestContextArgument>::Context, +> { + fn from(_: handler_xyz) -> Self { + #[allow(clippy::unused_async)] + async fn handler_xyz( + _: RequestContext<()>, + ) -> Result { + Ok(()) + } + const _: fn() = || { + fn future_endpoint_must_be_send(_t: T) {} + fn check_future_bounds(arg0: RequestContext<()>) { + future_endpoint_must_be_send(handler_xyz(arg0)); + } + }; + dropshot::ApiEndpoint::new( + "handler_xyz".to_string(), + handler_xyz, + dropshot::Method::GET, + "application/json", + "/test", + dropshot::ApiEndpointVersions::From(semver::Version::new(1u64, 2u64, 3u64)), + ) + } +} diff --git a/dropshot_endpoint/tests/output/endpoint_with_versions_from_until.rs b/dropshot_endpoint/tests/output/endpoint_with_versions_from_until.rs new file mode 100644 index 000000000..3c2901560 --- /dev/null +++ b/dropshot_endpoint/tests/output/endpoint_with_versions_from_until.rs @@ -0,0 +1,71 @@ +const _: fn() = || { + struct NeedRequestContext( + as dropshot::RequestContextArgument>::Context, + ); +}; +const _: fn() = || { + trait ResultTrait { + type T; + type E; + } + impl ResultTrait for Result + where + TT: dropshot::HttpResponse, + { + type T = TT; + type E = EE; + } + struct NeedHttpResponse( + as ResultTrait>::T, + ); + trait TypeEq { + type This: ?Sized; + } + impl TypeEq for T { + type This = Self; + } + fn validate_result_error_type() + where + T: ?Sized + TypeEq, + {} + validate_result_error_type::< + as ResultTrait>::E, + >(); +}; +#[allow(non_camel_case_types, missing_docs)] +///API Endpoint: handler_xyz +struct handler_xyz {} +#[allow(non_upper_case_globals, missing_docs)] +///API Endpoint: handler_xyz +const handler_xyz: handler_xyz = handler_xyz {}; +impl From +for dropshot::ApiEndpoint< + as dropshot::RequestContextArgument>::Context, +> { + fn from(_: handler_xyz) -> Self { + #[allow(clippy::unused_async)] + async fn handler_xyz( + _: RequestContext<()>, + ) -> Result { + Ok(()) + } + const _: fn() = || { + fn future_endpoint_must_be_send(_t: T) {} + fn check_future_bounds(arg0: RequestContext<()>) { + future_endpoint_must_be_send(handler_xyz(arg0)); + } + }; + dropshot::ApiEndpoint::new( + "handler_xyz".to_string(), + handler_xyz, + dropshot::Method::GET, + "application/json", + "/test", + dropshot::ApiEndpointVersions::from_until( + semver::Version::new(1u64, 2u64, 3u64), + semver::Version::new(4u64, 5u64, 6u64), + ) + .unwrap(), + ) + } +} diff --git a/dropshot_endpoint/tests/output/endpoint_with_versions_until.rs b/dropshot_endpoint/tests/output/endpoint_with_versions_until.rs new file mode 100644 index 000000000..49b418a55 --- /dev/null +++ b/dropshot_endpoint/tests/output/endpoint_with_versions_until.rs @@ -0,0 +1,67 @@ +const _: fn() = || { + struct NeedRequestContext( + as dropshot::RequestContextArgument>::Context, + ); +}; +const _: fn() = || { + trait ResultTrait { + type T; + type E; + } + impl ResultTrait for Result + where + TT: dropshot::HttpResponse, + { + type T = TT; + type E = EE; + } + struct NeedHttpResponse( + as ResultTrait>::T, + ); + trait TypeEq { + type This: ?Sized; + } + impl TypeEq for T { + type This = Self; + } + fn validate_result_error_type() + where + T: ?Sized + TypeEq, + {} + validate_result_error_type::< + as ResultTrait>::E, + >(); +}; +#[allow(non_camel_case_types, missing_docs)] +///API Endpoint: handler_xyz +struct handler_xyz {} +#[allow(non_upper_case_globals, missing_docs)] +///API Endpoint: handler_xyz +const handler_xyz: handler_xyz = handler_xyz {}; +impl From +for dropshot::ApiEndpoint< + as dropshot::RequestContextArgument>::Context, +> { + fn from(_: handler_xyz) -> Self { + #[allow(clippy::unused_async)] + async fn handler_xyz( + _: RequestContext<()>, + ) -> Result { + Ok(()) + } + const _: fn() = || { + fn future_endpoint_must_be_send(_t: T) {} + fn check_future_bounds(arg0: RequestContext<()>) { + future_endpoint_must_be_send(handler_xyz(arg0)); + } + }; + dropshot::ApiEndpoint::new( + "handler_xyz".to_string(), + handler_xyz, + dropshot::Method::GET, + "application/json", + "/test", + dropshot::ApiEndpointVersions::Until(semver::Version::new(1u64, 2u64, 3u64)), + ) + } +} From 25c46468656b94c142ecaae93f567434a98c874f Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 24 Sep 2024 12:55:10 -0700 Subject: [PATCH 02/23] update CHANGELOG --- CHANGELOG.adoc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 423bc759e..d56ed28b7 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -15,6 +15,13 @@ https://github.com/oxidecomputer/dropshot/compare/v0.11.0\...HEAD[Full list of commits] +* https://github.com/oxidecomputer/dropshot/pull/1115[#1115] Dropshot now supports hosting multiple versions of an API at a single server and routing to the correct version based on the incoming request. See documentation for details. If you don't care about this, you can mostly ignore it, but see "Breaking Changes" below. + +=== Breaking Changes + +* Dropshot now expects that APIs use https://semver.org/[Semver] values for their version string. Concretely, this only means that the `version` argument to `ApiDescription::openapi` (which generates an OpenAPI document) must be a `semver::Version`. Previously, it was `AsRef`. +* If you're invoking `ApiEndpoint::new` directly or constructing one as a literal (both of which are uncommon), you must provide a new `ApiEndpointVersions` value describing which versions this endpoint implements. You can use `ApiEndpointVersions::All` if you don't care about versioning. + == 0.11.0 (released 2024-08-21) https://github.com/oxidecomputer/dropshot/compare/v0.10.1\...v0.11.0[Full list of commits] From 4e5fcd16aea2592f52e08d68a22373caa3c44a15 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 24 Sep 2024 12:57:53 -0700 Subject: [PATCH 03/23] fix up most clippy errors --- dropshot/src/router.rs | 6 +++--- dropshot_endpoint/src/metadata.rs | 10 ++++++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/dropshot/src/router.rs b/dropshot/src/router.rs index c1be0fd56..8ddcb598f 100644 --- a/dropshot/src/router.rs +++ b/dropshot/src/router.rs @@ -615,7 +615,7 @@ where { Box::new(node.methods.iter().flat_map(move |(m, handlers)| { handlers.iter().filter_map(move |h| { - if h.versions.matches(version.clone()) { + if h.versions.matches(version) { Some((m, h)) } else { None @@ -630,7 +630,7 @@ impl<'a, Context: ServerContext> HttpRouterIter<'a, Context> { version: Option<&'a Version>, ) -> Self { HttpRouterIter { - method: iter_handlers_from_node(&router.root, version.clone()), + method: iter_handlers_from_node(&router.root, version), path: vec![( PathSegment::Literal("".to_string()), HttpRouterIter::iter_node(&router.root), @@ -715,7 +715,7 @@ impl<'a, Context: ServerContext> Iterator for HttpRouterIter<'a, Context> { )); self.method = iter_handlers_from_node( &node, - self.version.clone(), + self.version, ); } }, diff --git a/dropshot_endpoint/src/metadata.rs b/dropshot_endpoint/src/metadata.rs index 18a63ed71..fb0f00816 100644 --- a/dropshot_endpoint/src/metadata.rs +++ b/dropshot_endpoint/src/metadata.rs @@ -298,7 +298,9 @@ fn parse_semver(v: &syn::LitStr) -> syn::Result { } else { Err(syn::Error::new_spanned( v, - format!("semver pre-release string is not supported here"), + String::from( + "semver pre-release string is not supported here", + ), )) } }) @@ -308,7 +310,7 @@ fn parse_semver(v: &syn::LitStr) -> syn::Result { } else { Err(syn::Error::new_spanned( v, - format!("semver build metadata is not supported here"), + String::from("semver build metadata is not supported here"), )) } }) @@ -343,9 +345,9 @@ impl syn::parse::Parse for VersionRange { .collect(); return Err(syn::Error::new_spanned( span, - format!( + String::from( "semver range (from ... until ...) has the \ - endpoints out of order" + endpoints out of order", ), )); } From e0d00038e4e6f95b51dd6f28f9a049e34960277b Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 25 Sep 2024 14:31:07 -0700 Subject: [PATCH 04/23] add test for creating unversioned servers with versioned routes --- dropshot/tests/test_config.rs | 46 +++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/dropshot/tests/test_config.rs b/dropshot/tests/test_config.rs index 17c3864e1..a02bff404 100644 --- a/dropshot/tests/test_config.rs +++ b/dropshot/tests/test_config.rs @@ -581,3 +581,49 @@ async fn test_config_handler_task_mode_detached() { logctx.cleanup_successful(); } + +#[tokio::test] +async fn test_unversioned_servers_with_versioned_routes() { + #[dropshot::endpoint { + method = GET, + path = "/handler", + versions = "1.0.1".."1.0.1", + }] + async fn versioned_handler( + _rqctx: RequestContext, + ) -> Result, HttpError> { + Ok(HttpResponseOk(3)) + } + + let logctx = + create_log_context("test_unversioned_servers_with_versioned_routes"); + let config_dropshot = ConfigDropshot::default(); + + // Test both the HTTP and HTTPS code paths because the check is present in + // both. + let (certs, key) = common::generate_tls_key(); + let (serialized_certs, serialized_key) = + common::tls_key_to_buffer(&certs, &key); + let tls = Some(ConfigTls::AsBytes { + certs: serialized_certs.clone(), + key: serialized_key.clone(), + }); + for tls_arg in [None, tls] { + let mut api = dropshot::ApiDescription::new(); + api.register(versioned_handler).unwrap(); + let Err(error) = HttpServerStarter::new_with_tls( + &config_dropshot, + api, + 0, + &logctx.log, + tls_arg, + ) else { + panic!("expected failure to create server"); + }; + println!("{}", error); + assert_eq!( + error.to_string(), + "unversioned servers cannot have endpoints with specific versions" + ); + } +} From ceaf0ad5e550b3977ad1f639ffddf45608b487ed Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 25 Sep 2024 14:37:37 -0700 Subject: [PATCH 05/23] add test for compiler error --- dropshot/tests/fail/bad_version_backwards.rs | 23 +++++++++++++++++++ .../tests/fail/bad_version_backwards.stderr | 5 ++++ 2 files changed, 28 insertions(+) create mode 100644 dropshot/tests/fail/bad_version_backwards.rs create mode 100644 dropshot/tests/fail/bad_version_backwards.stderr diff --git a/dropshot/tests/fail/bad_version_backwards.rs b/dropshot/tests/fail/bad_version_backwards.rs new file mode 100644 index 000000000..0384180e8 --- /dev/null +++ b/dropshot/tests/fail/bad_version_backwards.rs @@ -0,0 +1,23 @@ +// Copyright 2024 Oxide Computer Company + +//! Tests the case where the "versions" field is not parseable +//! +//! We do not bother testing most of the other "bad version" cases because we +//! have exhaustive unit tests for those. + +#![allow(unused_imports)] + +use dropshot::endpoint; +use dropshot::HttpError; +use dropshot::HttpResponseOk; + +#[endpoint { + method = GET, + path = "/test", + versions = "1.2.3".."1.2.2" +}] +async fn bad_version_backwards() -> Result, HttpError> { + Ok(HttpResponseOk(())) +} + +fn main() {} diff --git a/dropshot/tests/fail/bad_version_backwards.stderr b/dropshot/tests/fail/bad_version_backwards.stderr new file mode 100644 index 000000000..2a5b86ab1 --- /dev/null +++ b/dropshot/tests/fail/bad_version_backwards.stderr @@ -0,0 +1,5 @@ +error: semver range (from ... until ...) has the endpoints out of order + --> tests/fail/bad_version_backwards.rs:17:16 + | +17 | versions = "1.2.3".."1.2.2" + | ^^^^^^^ From 4cc1d1dc7cd57863855bc1487e1140130ffb9b29 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 25 Sep 2024 15:51:45 -0700 Subject: [PATCH 06/23] add a real test for versioning --- Cargo.lock | 481 ++++++++++++++++++++++++++++---- dropshot/Cargo.toml | 4 + dropshot/examples/versioning.rs | 38 +-- dropshot/src/lib.rs | 37 +++ dropshot/tests/test_versions.rs | 296 ++++++++++++++++++++ 5 files changed, 759 insertions(+), 97 deletions(-) create mode 100644 dropshot/tests/test_versions.rs diff --git a/Cargo.lock b/Cargo.lock index 7d14b19dd..fa215e01d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -198,7 +198,7 @@ dependencies = [ "iana-time-zone", "num-traits", "serde", - "windows-targets 0.52.0", + "windows-targets 0.52.6", ] [[package]] @@ -363,8 +363,8 @@ dependencies = [ "futures", "hostname 0.4.0", "http 0.2.9", - "hyper", - "hyper-rustls", + "hyper 0.14.27", + "hyper-rustls 0.25.0", "hyper-staticfile", "indexmap", "lazy_static", @@ -376,7 +376,8 @@ dependencies = [ "pem", "percent-encoding", "rcgen", - "rustls", + "reqwest", + "rustls 0.22.4", "rustls-pemfile", "rustls-pki-types", "schemars", @@ -397,7 +398,7 @@ dependencies = [ "tempfile", "thiserror", "tokio", - "tokio-rustls", + "tokio-rustls 0.25.0", "tokio-tungstenite", "toml", "trybuild", @@ -515,6 +516,21 @@ version = "1.0.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" +[[package]] +name = "foreign-types" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f6f339eb8adc052cd2ca78910fda869aefa38d22d5cb648e6485e4d3fc06f3b1" +dependencies = [ + "foreign-types-shared", +] + +[[package]] +name = "foreign-types-shared" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "00b0228411908ca8685dba7fc2cdd70ec9990a6e753e89b6ac91a84c40fbaf4b" + [[package]] name = "form_urlencoded" version = "1.2.1" @@ -676,6 +692,25 @@ dependencies = [ "tracing", ] +[[package]] +name = "h2" +version = "0.4.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "524e8ac6999421f49a846c2d4411f337e53497d8ec55d67753beffa43c5d9205" +dependencies = [ + "atomic-waker", + "bytes", + "fnv", + "futures-core", + "futures-sink", + "http 1.0.0", + "indexmap", + "slab", + "tokio", + "tokio-util", + "tracing", +] + [[package]] name = "hashbrown" version = "0.14.1" @@ -758,6 +793,29 @@ dependencies = [ "pin-project-lite", ] +[[package]] +name = "http-body" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1efedce1fb8e6913f23e0c92de8e62cd5b772a67e7b3946df930a62566c93184" +dependencies = [ + "bytes", + "http 1.0.0", +] + +[[package]] +name = "http-body-util" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "793429d76616a256bcb62c2a2ec2bed781c8307e797e2598c50010f2bee2544f" +dependencies = [ + "bytes", + "futures-util", + "http 1.0.0", + "http-body 1.0.1", + "pin-project-lite", +] + [[package]] name = "http-range" version = "0.1.4" @@ -786,9 +844,9 @@ dependencies = [ "futures-channel", "futures-core", "futures-util", - "h2", + "h2 0.3.26", "http 0.2.9", - "http-body", + "http-body 0.4.3", "httparse", "httpdate", "itoa", @@ -800,6 +858,26 @@ dependencies = [ "want", ] +[[package]] +name = "hyper" +version = "1.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "50dfd22e0e76d0f662d429a5f80fcaf3855009297eab6a0a9f8543834744ba05" +dependencies = [ + "bytes", + "futures-channel", + "futures-util", + "h2 0.4.6", + "http 1.0.0", + "http-body 1.0.1", + "httparse", + "itoa", + "pin-project-lite", + "smallvec", + "tokio", + "want", +] + [[package]] name = "hyper-rustls" version = "0.25.0" @@ -808,13 +886,30 @@ checksum = "399c78f9338483cb7e630c8474b07268983c6bd5acee012e4211f9f7bb21b070" dependencies = [ "futures-util", "http 0.2.9", - "hyper", + "hyper 0.14.27", "log", - "rustls", + "rustls 0.22.4", "rustls-native-certs", "rustls-pki-types", "tokio", - "tokio-rustls", + "tokio-rustls 0.25.0", +] + +[[package]] +name = "hyper-rustls" +version = "0.27.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08afdbb5c31130e3034af566421053ab03787c640246a446327f550d11bcb333" +dependencies = [ + "futures-util", + "http 1.0.0", + "hyper 1.4.1", + "hyper-util", + "rustls 0.23.13", + "rustls-pki-types", + "tokio", + "tokio-rustls 0.26.0", + "tower-service", ] [[package]] @@ -827,7 +922,7 @@ dependencies = [ "http 0.2.9", "http-range", "httpdate", - "hyper", + "hyper 0.14.27", "mime_guess", "percent-encoding", "rand", @@ -836,6 +931,41 @@ dependencies = [ "winapi", ] +[[package]] +name = "hyper-tls" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "70206fc6890eaca9fde8a0bf71caa2ddfc9fe045ac9e5c70df101a7dbde866e0" +dependencies = [ + "bytes", + "http-body-util", + "hyper 1.4.1", + "hyper-util", + "native-tls", + "tokio", + "tokio-native-tls", + "tower-service", +] + +[[package]] +name = "hyper-util" +version = "0.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "41296eb09f183ac68eec06e03cdbea2e759633d4067b2f6552fc2e009bcad08b" +dependencies = [ + "bytes", + "futures-channel", + "futures-util", + "http 1.0.0", + "http-body 1.0.1", + "hyper 1.4.1", + "pin-project-lite", + "socket2 0.5.5", + "tokio", + "tower-service", + "tracing", +] + [[package]] name = "iana-time-zone" version = "0.1.47" @@ -852,11 +982,10 @@ dependencies = [ [[package]] name = "idna" -version = "0.2.3" +version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "418a0a6fab821475f634efe3ccc45c013f742efe03d853e8d3355d5cb850ecf8" +checksum = "634d9b1461af396cad843f47fdba5597a4f9e6ddd4bfb6ff5d85028c25cb12f6" dependencies = [ - "matches", "unicode-bidi", "unicode-normalization", ] @@ -872,6 +1001,12 @@ dependencies = [ "serde", ] +[[package]] +name = "ipnet" +version = "2.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "187674a687eed5fe42285b40c6291f9a01517d415fad1c3cbc6a9f778af7fcd4" + [[package]] name = "is-terminal" version = "0.4.12" @@ -940,12 +1075,6 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ffbee8634e0d45d258acb448e7eaab3fce7a0a467395d4d9f228e3c1f01fb2e4" -[[package]] -name = "matches" -version = "0.1.9" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a3e378b66a060d48947b590737b30a1be76706c8dd7b8ba0f2fe3989c68a853f" - [[package]] name = "memchr" version = "2.6.0" @@ -1015,6 +1144,23 @@ dependencies = [ "version_check", ] +[[package]] +name = "native-tls" +version = "0.2.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a8614eb2c83d59d1c8cc974dd3f920198647674a0a035e1af1fa58707e317466" +dependencies = [ + "libc", + "log", + "openssl", + "openssl-probe", + "openssl-sys", + "schannel", + "security-framework", + "security-framework-sys", + "tempfile", +] + [[package]] name = "newline-converter" version = "0.3.0" @@ -1069,9 +1215,9 @@ dependencies = [ [[package]] name = "once_cell" -version = "1.13.1" +version = "1.19.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "074864da206b4973b84eb91683020dbefd6a8c3f0f38e054d93954e891935e4e" +checksum = "3fdb12b2476b595f9358c5161aa467c2438859caa136dec86c26fdd2efe17b92" [[package]] name = "openapiv3" @@ -1084,12 +1230,50 @@ dependencies = [ "serde_json", ] +[[package]] +name = "openssl" +version = "0.10.66" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9529f4786b70a3e8c61e11179af17ab6188ad8d0ded78c5529441ed39d4bd9c1" +dependencies = [ + "bitflags 2.4.0", + "cfg-if", + "foreign-types", + "libc", + "once_cell", + "openssl-macros", + "openssl-sys", +] + +[[package]] +name = "openssl-macros" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a948666b637a0f465e8564c73e89d4dde00d72d4d473cc972f390fc3dcee7d9c" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.77", +] + [[package]] name = "openssl-probe" version = "0.1.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "28988d872ab76095a6e6ac88d99b54fd267702734fd7ffe610ca27f533ddb95a" +[[package]] +name = "openssl-sys" +version = "0.9.103" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f9e8deee91df40a943c71b917e5874b951d32a802526c85721ce3b776c929d6" +dependencies = [ + "cc", + "libc", + "pkg-config", + "vcpkg", +] + [[package]] name = "parking" version = "2.2.0" @@ -1198,6 +1382,12 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8b870d8c151b6f2fb93e84a13146138f05d02ed11c7e7c54f8826aaaf7c9f184" +[[package]] +name = "pkg-config" +version = "0.3.31" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "953ec861398dccce10c670dfeaf3ec4911ca479e9c02154b3a215178c5f566f2" + [[package]] name = "plain" version = "0.2.3" @@ -1322,6 +1512,49 @@ dependencies = [ "redox_syscall", ] +[[package]] +name = "reqwest" +version = "0.12.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8f4955649ef5c38cc7f9e8aa41761d48fb9677197daea9984dc54f56aad5e63" +dependencies = [ + "base64", + "bytes", + "encoding_rs", + "futures-core", + "futures-util", + "h2 0.4.6", + "http 1.0.0", + "http-body 1.0.1", + "http-body-util", + "hyper 1.4.1", + "hyper-rustls 0.27.3", + "hyper-tls", + "hyper-util", + "ipnet", + "js-sys", + "log", + "mime", + "native-tls", + "once_cell", + "percent-encoding", + "pin-project-lite", + "rustls-pemfile", + "serde", + "serde_json", + "serde_urlencoded", + "sync_wrapper", + "system-configuration", + "tokio", + "tokio-native-tls", + "tower-service", + "url", + "wasm-bindgen", + "wasm-bindgen-futures", + "web-sys", + "windows-registry", +] + [[package]] name = "ring" version = "0.17.7" @@ -1369,6 +1602,19 @@ dependencies = [ "zeroize", ] +[[package]] +name = "rustls" +version = "0.23.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f2dabaac7466917e566adb06783a81ca48944c6898a1b08b9374106dd671f4c8" +dependencies = [ + "once_cell", + "rustls-pki-types", + "rustls-webpki", + "subtle", + "zeroize", +] + [[package]] name = "rustls-native-certs" version = "0.7.0" @@ -1400,9 +1646,9 @@ checksum = "fc0a2ce646f8655401bb81e7927b812614bd5d91dbc968696be50603510fcaf0" [[package]] name = "rustls-webpki" -version = "0.102.1" +version = "0.102.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ef4ca26037c909dedb327b48c3327d0ba91d3dd3c4e05dad328f210ffb68e95b" +checksum = "64ca1bc8749bd4cf37b5ce386cc146580777b4e8572c7b97baf22c83f444bee9" dependencies = [ "ring", "rustls-pki-types", @@ -1728,9 +1974,9 @@ dependencies = [ [[package]] name = "smallvec" -version = "1.7.0" +version = "1.13.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1ecab6c735a6bb4139c0caafd0cc3635748bbb3acf4550e8138122099251f309" +checksum = "3c5e1a9a646d36c3599cd173a41282daf47c44583ad367b8e6837255952e5c67" [[package]] name = "socket2" @@ -1796,6 +2042,36 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "sync_wrapper" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a7065abeca94b6a8a577f9bd45aa0867a2238b74e8eb67cf10d492bc39351394" +dependencies = [ + "futures-core", +] + +[[package]] +name = "system-configuration" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3c879d448e9d986b661742763247d3693ed13609438cf3d006f51f5368a5ba6b" +dependencies = [ + "bitflags 2.4.0", + "core-foundation", + "system-configuration-sys", +] + +[[package]] +name = "system-configuration-sys" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e1d1b10ced5ca923a1fcb8d03e96b8d3268065d724548c0211415ff6ac6bac4" +dependencies = [ + "core-foundation-sys", + "libc", +] + [[package]] name = "take_mut" version = "0.2.2" @@ -1952,13 +2228,34 @@ dependencies = [ "syn 2.0.77", ] +[[package]] +name = "tokio-native-tls" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bbae76ab933c85776efabc971569dd6119c580d8f5d448769dec1764bf796ef2" +dependencies = [ + "native-tls", + "tokio", +] + [[package]] name = "tokio-rustls" version = "0.25.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "775e0c0f0adb3a2f22a00c4745d728b479985fc15ee7ca6a2608388c5569860f" dependencies = [ - "rustls", + "rustls 0.22.4", + "rustls-pki-types", + "tokio", +] + +[[package]] +name = "tokio-rustls" +version = "0.26.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0c7bc40d0e5a97695bb96e27995cd3a08538541b0a846f65bba7a359f36700d4" +dependencies = [ + "rustls 0.23.13", "rustls-pki-types", "tokio", ] @@ -2109,9 +2406,9 @@ dependencies = [ [[package]] name = "unicode-bidi" -version = "0.3.7" +version = "0.3.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1a01404663e3db436ed2746d9fefef640d868edae3cceb81c3b8d5732fda678f" +checksum = "08f95100a766bf4f8f28f90d77e0a5461bbdb219042e7679bebe79004fed8d75" [[package]] name = "unicode-ident" @@ -2121,9 +2418,9 @@ checksum = "3354b9ac3fae1ff6755cb6db53683adb661634f67557942dea4facebec0fee4b" [[package]] name = "unicode-normalization" -version = "0.1.19" +version = "0.1.24" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d54590932941a9e9266f0832deed84ebe1bf2e4c9e4a3554d393d18f5e854bf9" +checksum = "5033c97c4262335cded6d6fc3e5c18ab755e1a3dc96376350f3d8e9f009ad956" dependencies = [ "tinyvec", ] @@ -2148,13 +2445,12 @@ checksum = "8ecb6da28b8a351d773b68d5825ac39017e680750f980f3a1a85cd8dd28a47c1" [[package]] name = "url" -version = "2.2.2" +version = "2.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a507c383b2d33b5fc35d1861e77e6b383d158b2da5e14fe51b83dfedf6fd578c" +checksum = "22784dbdf76fdde8af1aeda5622b546b422b6fc585325248a2bf9f5e41e94d6c" dependencies = [ "form_urlencoded", "idna", - "matches", "percent-encoding", ] @@ -2238,6 +2534,12 @@ dependencies = [ "serde", ] +[[package]] +name = "vcpkg" +version = "0.2.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "accd4ea62f7bb7a82fe23066fb0957d48ef677f6eeb8215f372f52e48bb32426" + [[package]] name = "version_check" version = "0.9.5" @@ -2294,6 +2596,18 @@ dependencies = [ "wasm-bindgen-shared", ] +[[package]] +name = "wasm-bindgen-futures" +version = "0.4.32" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fa76fb221a1f8acddf5b54ace85912606980ad661ac7a503b4570ffd3a624dad" +dependencies = [ + "cfg-if", + "js-sys", + "wasm-bindgen", + "web-sys", +] + [[package]] name = "wasm-bindgen-macro" version = "0.2.82" @@ -2323,6 +2637,16 @@ version = "0.2.82" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6598dd0bd3c7d51095ff6531a5b23e02acdc81804e30d8f07afb77b7215a140a" +[[package]] +name = "web-sys" +version = "0.3.59" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ed055ab27f941423197eb86b2035720b1a3ce40504df082cac2ecc6ed73335a1" +dependencies = [ + "js-sys", + "wasm-bindgen", +] + [[package]] name = "winapi" version = "0.3.9" @@ -2361,7 +2685,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e48a53791691ab099e5e2ad123536d0fff50652600abaf43bbf952894110d0be" dependencies = [ "windows-core", - "windows-targets 0.52.0", + "windows-targets 0.52.6", ] [[package]] @@ -2370,7 +2694,37 @@ version = "0.52.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "33ab640c8d7e35bf8ba19b884ba838ceb4fba93a4e8c65a9059d08afcfc683d9" dependencies = [ - "windows-targets 0.52.0", + "windows-targets 0.52.6", +] + +[[package]] +name = "windows-registry" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e400001bb720a623c1c69032f8e3e4cf09984deec740f007dd2b03ec864804b0" +dependencies = [ + "windows-result", + "windows-strings", + "windows-targets 0.52.6", +] + +[[package]] +name = "windows-result" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1d1043d8214f791817bab27572aaa8af63732e11bf84aa21a45a78d6c317ae0e" +dependencies = [ + "windows-targets 0.52.6", +] + +[[package]] +name = "windows-strings" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4cd9b125c486025df0eabcb585e62173c6c9eddcec5d117d3b6e8c30e2ee4d10" +dependencies = [ + "windows-result", + "windows-targets 0.52.6", ] [[package]] @@ -2412,7 +2766,7 @@ version = "0.52.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "282be5f36a8ce781fad8c8ae18fa3f9beff57ec1b52cb3de0789201425d9a33d" dependencies = [ - "windows-targets 0.52.0", + "windows-targets 0.52.6", ] [[package]] @@ -2447,17 +2801,18 @@ dependencies = [ [[package]] name = "windows-targets" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8a18201040b24831fbb9e4eb208f8892e1f50a37feb53cc7ff887feb8f50e7cd" +checksum = "9b724f72796e036ab90c1021d4780d4d3d648aca59e491e6b98e725b84e99973" dependencies = [ - "windows_aarch64_gnullvm 0.52.0", - "windows_aarch64_msvc 0.52.0", - "windows_i686_gnu 0.52.0", - "windows_i686_msvc 0.52.0", - "windows_x86_64_gnu 0.52.0", - "windows_x86_64_gnullvm 0.52.0", - "windows_x86_64_msvc 0.52.0", + "windows_aarch64_gnullvm 0.52.6", + "windows_aarch64_msvc 0.52.6", + "windows_i686_gnu 0.52.6", + "windows_i686_gnullvm", + "windows_i686_msvc 0.52.6", + "windows_x86_64_gnu 0.52.6", + "windows_x86_64_gnullvm 0.52.6", + "windows_x86_64_msvc 0.52.6", ] [[package]] @@ -2474,9 +2829,9 @@ checksum = "91ae572e1b79dba883e0d315474df7305d12f569b400fcf90581b06062f7e1bc" [[package]] name = "windows_aarch64_gnullvm" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cb7764e35d4db8a7921e09562a0304bf2f93e0a51bfccee0bd0bb0b666b015ea" +checksum = "32a4622180e7a0ec044bb555404c800bc9fd9ec262ec147edd5989ccd0c02cd3" [[package]] name = "windows_aarch64_msvc" @@ -2492,9 +2847,9 @@ checksum = "b2ef27e0d7bdfcfc7b868b317c1d32c641a6fe4629c171b8928c7b08d98d7cf3" [[package]] name = "windows_aarch64_msvc" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bbaa0368d4f1d2aaefc55b6fcfee13f41544ddf36801e793edbbfd7d7df075ef" +checksum = "09ec2a7bb152e2252b53fa7803150007879548bc709c039df7627cabbd05d469" [[package]] name = "windows_i686_gnu" @@ -2510,9 +2865,15 @@ checksum = "622a1962a7db830d6fd0a69683c80a18fda201879f0f447f065a3b7467daa241" [[package]] name = "windows_i686_gnu" -version = "0.52.0" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e9b5ad5ab802e97eb8e295ac6720e509ee4c243f69d781394014ebfe8bbfa0b" + +[[package]] +name = "windows_i686_gnullvm" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a28637cb1fa3560a16915793afb20081aba2c92ee8af57b4d5f28e4b3e7df313" +checksum = "0eee52d38c090b3caa76c563b86c3a4bd71ef1a819287c19d586d7334ae8ed66" [[package]] name = "windows_i686_msvc" @@ -2528,9 +2889,9 @@ checksum = "4542c6e364ce21bf45d69fdd2a8e455fa38d316158cfd43b3ac1c5b1b19f8e00" [[package]] name = "windows_i686_msvc" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ffe5e8e31046ce6230cc7215707b816e339ff4d4d67c65dffa206fd0f7aa7b9a" +checksum = "240948bc05c5e7c6dabba28bf89d89ffce3e303022809e73deaefe4f6ec56c66" [[package]] name = "windows_x86_64_gnu" @@ -2546,9 +2907,9 @@ checksum = "ca2b8a661f7628cbd23440e50b05d705db3686f894fc9580820623656af974b1" [[package]] name = "windows_x86_64_gnu" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3d6fa32db2bc4a2f5abeacf2b69f7992cd09dca97498da74a151a3132c26befd" +checksum = "147a5c80aabfbf0c7d901cb5895d1de30ef2907eb21fbbab29ca94c5b08b1a78" [[package]] name = "windows_x86_64_gnullvm" @@ -2564,9 +2925,9 @@ checksum = "7896dbc1f41e08872e9d5e8f8baa8fdd2677f29468c4e156210174edc7f7b953" [[package]] name = "windows_x86_64_gnullvm" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1a657e1e9d3f514745a572a6846d3c7aa7dbe1658c056ed9c3344c4109a6949e" +checksum = "24d5b23dc417412679681396f2b49f3de8c1473deb516bd34410872eff51ed0d" [[package]] name = "windows_x86_64_msvc" @@ -2582,9 +2943,9 @@ checksum = "1a515f5799fe4961cb532f983ce2b23082366b898e52ffbce459c86f67c8378a" [[package]] name = "windows_x86_64_msvc" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dff9641d1cd4be8d1a070daf9e3773c5f67e78b4d9d42263020c057706765c04" +checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" [[package]] name = "winnow" diff --git a/dropshot/Cargo.toml b/dropshot/Cargo.toml index aa27be0eb..9970e7d84 100644 --- a/dropshot/Cargo.toml +++ b/dropshot/Cargo.toml @@ -104,6 +104,10 @@ rcgen = "0.13.1" # Used in a doc-test demonstrating the WebsocketUpgrade extractor. tokio-tungstenite = "0.23.1" +[dev-dependencies.reqwest] +version = "0.12.7" +features = [ "json" ] + [dev-dependencies.rustls-pki-types] version = "1.8.0" # Needed for CertificateDer::into_owned diff --git a/dropshot/examples/versioning.rs b/dropshot/examples/versioning.rs index 2ed68c59e..934af8873 100644 --- a/dropshot/examples/versioning.rs +++ b/dropshot/examples/versioning.rs @@ -19,7 +19,6 @@ use schemars::JsonSchema; use semver::Version; use serde::Serialize; use slog::Logger; -use std::str::FromStr; #[tokio::main] async fn main() -> Result<(), String> { @@ -60,45 +59,10 @@ impl DynamicVersionPolicy for BasicVersionPolicy { request: &Request, _log: &Logger, ) -> Result { - parse_header(request.headers(), "dropshot-demo-version") + dropshot::parse_header(request.headers(), "dropshot-demo-version") } } -/// Parses a required header out of a request (producing useful error messages -/// for all failure modes) -fn parse_header( - headers: &http::HeaderMap, - header_name: &str, -) -> Result -where - T: FromStr, - ::Err: std::fmt::Display, -{ - let v_value = headers.get(header_name).ok_or_else(|| { - HttpError::for_bad_request( - None, - format!("missing expected header {:?}", header_name), - ) - })?; - - let v_str = v_value.to_str().map_err(|_| { - HttpError::for_bad_request( - None, - format!( - "bad value for header {:?}: not ASCII: {:?}", - header_name, v_value - ), - ) - })?; - - v_str.parse::().map_err(|e| { - HttpError::for_bad_request( - None, - format!("bad value for header {:?}: {}: {}", header_name, e, v_str), - ) - }) -} - // HTTP API interface // // This API defines several different versions: diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 78049aa77..e6a7e5614 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -855,3 +855,40 @@ extern crate dropshot_endpoint; pub use dropshot_endpoint::api_description; pub use dropshot_endpoint::channel; pub use dropshot_endpoint::endpoint; + +use std::str::FromStr; + +/// Parses a required header out of a request (producing useful error messages +/// for all failure modes) +pub fn parse_header( + headers: &http::HeaderMap, + header_name: &str, +) -> Result +where + T: FromStr, + ::Err: std::fmt::Display, +{ + let v_value = headers.get(header_name).ok_or_else(|| { + HttpError::for_bad_request( + None, + format!("missing expected header {:?}", header_name), + ) + })?; + + let v_str = v_value.to_str().map_err(|_| { + HttpError::for_bad_request( + None, + format!( + "bad value for header {:?}: not ASCII: {:?}", + header_name, v_value + ), + ) + })?; + + v_str.parse::().map_err(|e| { + HttpError::for_bad_request( + None, + format!("bad value for header {:?}: {}: {}", header_name, e, v_str), + ) + }) +} diff --git a/dropshot/tests/test_versions.rs b/dropshot/tests/test_versions.rs new file mode 100644 index 000000000..c8c0e628c --- /dev/null +++ b/dropshot/tests/test_versions.rs @@ -0,0 +1,296 @@ +// Copyright 2024 Oxide Computer Company + +//! Exercise the API versioning behavior + +use dropshot::endpoint; +use dropshot::ApiDescription; +use dropshot::ConfigDropshot; +use dropshot::HttpError; +use dropshot::HttpErrorResponseBody; +use dropshot::HttpResponseOk; +use dropshot::HttpServerStarter; +use dropshot::RequestContext; +use dropshot::VersionPolicy; +use reqwest::Method; +use reqwest::StatusCode; +use semver::Version; + +pub mod common; + +const VERSION_HEADER_NAME: &str = "dropshot-test-version"; +const HANDLER1_MSG: &str = "handler1"; +const HANDLER2_MSG: &str = "handler2"; +const HANDLER3_MSG: &str = "handler3"; +const HANDLER4_MSG: &str = "handler3"; +const FORBIDDEN_MSG: &str = "THAT VALUE IS FORBIDDEN!!!"; + +#[tokio::test] +async fn test_versions() { + let mut api = ApiDescription::<()>::new(); + api.register(handler1).unwrap(); + api.register(handler2).unwrap(); + api.register(handler3).unwrap(); + api.register(handler4).unwrap(); + + let logctx = common::create_log_context("test_versions"); + let server = HttpServerStarter::new_with_versioning( + &ConfigDropshot::default(), + api, + (), + &logctx.log, + None, + VersionPolicy::Dynamic(Box::new(TestVersionPolicy {})), + ) + .unwrap() + .start(); + + let server_addr = server.local_addr(); + let mkurl = |path: &str| format!("http://{}{}", server_addr, path); + let client = reqwest::Client::new(); + + #[derive(Debug)] + struct TestCase<'a> { + method: Method, + path: &'a str, + header: Option<&'a str>, + expected_status: StatusCode, + body_contents: String, + } + + impl<'a> TestCase<'a> { + fn new( + method: Method, + path: &'a str, + header: Option<&'a str>, + expected_status: StatusCode, + body_contents: S, + ) -> TestCase<'a> + where + String: From, + { + TestCase { + method, + path, + header, + expected_status, + body_contents: String::from(body_contents), + } + } + } + + let test_cases = [ + // Test that errors produced by the version policy get propagated + // through to the client. + TestCase::new( + Method::GET, + "/demo", + None, + StatusCode::BAD_REQUEST, + format!("missing expected header {:?}", VERSION_HEADER_NAME), + ), + TestCase::new( + Method::GET, + "/demo", + Some("not-a-semver"), + StatusCode::BAD_REQUEST, + format!( + "bad value for header {:?}: unexpected character 'n' while \ + parsing major version number: not-a-semver", + VERSION_HEADER_NAME + ), + ), + // Versions prior to and including 1.0.0 get "handler1". + TestCase::new( + Method::GET, + "/demo", + Some("0.9.0"), + StatusCode::OK, + HANDLER1_MSG, + ), + TestCase::new( + Method::GET, + "/demo", + Some("1.0.0"), + StatusCode::OK, + HANDLER1_MSG, + ), + // Versions between 1.0.0 and 1.1.0 (non-inclusive) do not exist. + // + // Because there's a PUT handler for all versions, the expected error is + // 405 ("Method Not Allowed"), not 404 ("Not Found"). + TestCase::new( + Method::GET, + "/demo", + Some("1.0.1"), + StatusCode::METHOD_NOT_ALLOWED, + StatusCode::METHOD_NOT_ALLOWED.canonical_reason().unwrap(), + ), + TestCase::new( + Method::GET, + "/demo", + Some("1.0.99"), + StatusCode::METHOD_NOT_ALLOWED, + StatusCode::METHOD_NOT_ALLOWED.canonical_reason().unwrap(), + ), + // Versions between 1.1.0 and 1.3.0 (inclusive) get "handler2". + TestCase::new( + Method::GET, + "/demo", + Some("1.1.0"), + StatusCode::OK, + HANDLER2_MSG, + ), + TestCase::new( + Method::GET, + "/demo", + Some("1.3.0"), + StatusCode::OK, + HANDLER2_MSG, + ), + // Versions between 1.3.0 and 1.4.0 (exclusive) do not exist. + // See above for why this is a 405. + TestCase::new( + Method::GET, + "/demo", + Some("1.3.1"), + StatusCode::METHOD_NOT_ALLOWED, + StatusCode::METHOD_NOT_ALLOWED.canonical_reason().unwrap(), + ), + TestCase::new( + Method::GET, + "/demo", + Some("1.3.99"), + StatusCode::METHOD_NOT_ALLOWED, + StatusCode::METHOD_NOT_ALLOWED.canonical_reason().unwrap(), + ), + // Versions after 1.4.0 (inclusive) get "handler3". + TestCase::new( + Method::GET, + "/demo", + Some("1.4.0"), + StatusCode::OK, + HANDLER3_MSG, + ), + // For all of these versions, a PUT should get handler4. + TestCase::new( + Method::PUT, + "/demo", + Some("0.9.0"), + StatusCode::OK, + HANDLER4_MSG, + ), + TestCase::new( + Method::PUT, + "/demo", + Some("1.0.0"), + StatusCode::OK, + HANDLER4_MSG, + ), + TestCase::new( + Method::PUT, + "/demo", + Some("1.1.0"), + StatusCode::OK, + HANDLER4_MSG, + ), + TestCase::new( + Method::PUT, + "/demo", + Some("1.3.1"), + StatusCode::OK, + HANDLER4_MSG, + ), + TestCase::new( + Method::PUT, + "/demo", + Some("1.4.0"), + StatusCode::OK, + HANDLER4_MSG, + ), + ]; + + for t in test_cases { + println!("test case: {:?}", t); + let mut request = client.request(t.method, mkurl(t.path)); + if let Some(h) = t.header { + request = request.header(VERSION_HEADER_NAME, h); + } + let response = request.send().await.unwrap(); + assert_eq!(response.status(), t.expected_status); + + if !t.expected_status.is_success() { + let error: HttpErrorResponseBody = response.json().await.unwrap(); + assert_eq!(error.message, t.body_contents); + } else { + let body: String = response.json().await.unwrap(); + assert_eq!(body, t.body_contents); + } + } +} + +#[endpoint { + method = GET, + path = "/demo", + versions = .."1.0.0", +}] +async fn handler1( + _rqctx: RequestContext<()>, +) -> Result, HttpError> { + Ok(HttpResponseOk(HANDLER1_MSG)) +} + +#[endpoint { + method = GET, + path = "/demo", + versions = "1.1.0".."1.3.0", +}] +async fn handler2( + _rqctx: RequestContext<()>, +) -> Result, HttpError> { + Ok(HttpResponseOk(HANDLER2_MSG)) +} + +#[endpoint { + method = GET, + path = "/demo", + versions = "1.4.0".., +}] +async fn handler3( + _rqctx: RequestContext<()>, +) -> Result, HttpError> { + Ok(HttpResponseOk(HANDLER3_MSG)) +} + +#[endpoint { + method = PUT, + path = "/demo", + versions = .. +}] +async fn handler4( + _rqctx: RequestContext<()>, +) -> Result, HttpError> { + Ok(HttpResponseOk(HANDLER4_MSG)) +} + +#[derive(Debug)] +struct TestVersionPolicy; +impl dropshot::DynamicVersionPolicy for TestVersionPolicy { + fn request_extract_version( + &self, + request: &http::Request, + _log: &slog::Logger, + ) -> Result { + dropshot::parse_header(request.headers(), VERSION_HEADER_NAME).and_then( + |v: semver::Version| { + if v.major == 4 && v.minor == 3 && v.patch == 2 { + Err(HttpError::for_bad_request( + None, + String::from(FORBIDDEN_MSG), + )) + } else { + Ok(v) + } + }, + ) + } +} From 8d4535ef38d9c0a9cad433b0092b2b5a2a33d180 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 26 Sep 2024 09:59:20 -0700 Subject: [PATCH 07/23] some refactoring; add a common impl; much documentation --- dropshot/examples/versioning.rs | 180 ++++++++++++++++++++++++++------ dropshot/src/lib.rs | 142 ++++++++++++++++++------- dropshot/src/router.rs | 2 +- dropshot/src/server.rs | 84 +++++++-------- dropshot/src/versioning.rs | 174 ++++++++++++++++++++++++++++++ dropshot/tests/test_versions.rs | 29 +++-- 6 files changed, 475 insertions(+), 136 deletions(-) create mode 100644 dropshot/src/versioning.rs diff --git a/dropshot/examples/versioning.rs b/dropshot/examples/versioning.rs index 934af8873..3c407dacb 100644 --- a/dropshot/examples/versioning.rs +++ b/dropshot/examples/versioning.rs @@ -1,29 +1,121 @@ // Copyright 2024 Oxide Computer Company //! Example using API versioning +//! +//! This example defines a bunch of API versions: +//! +//! - Versions 1.0.0 through 1.0.3 contain an endpoint `GET /` that returns a +//! `Value` type with just one field: `s`. +//! - Versions 1.0.5 and later contain an endpoint `GET /` that returns a +//! `Value` type with two fields: `s` and `number`. +//! +//! The client chooses which version they want to use by specifying the +//! `dropshot-demo-version` header with their request. +//! +//! ## Generating OpenAPI specs +//! +//! You can generate the OpenAPI spec for 1.0.0 using: +//! +//! ```text +//! $ cargo run --example=versioning -- openapi 1.0.0 +//! ``` +//! +//! You'll see that the this spec contains one operation, it produces `Value`, +//! and that the `Value` type only has the one field `s`. +//! +//! You can generate the OpenAPI spec for 1.0.5 and see that the corresponding +//! `Value` type has the extra field `number`, as expected. +//! +//! You can generate the OpenAPI spec for any other version. You'll see that +//! 0.9.0, for example, has no operations and no `Value` type. +//! +//! ## Running the server +//! +//! Start the Dropshot HTTP server with: +//! +//! ```text +//! $ cargo run --example=versioning -- run +//! ``` +//! +//! The server will listen on 127.0.0.1:12345. You can use `curl` to make +//! requests. If we don't specify a version, we get an error: +//! +//! ```text +//! $ curl http://127.0.0.1:12345 +//! { +//! "request_id": "73f62e8a-b363-488a-b662-662814e306ee", +//! "message": "missing expected header \"dropshot-demo-version\"" +//! } +//! ``` +//! +//! You can customize this behavior for your Dropshot server, but this one +//! requires that the client specify a version. +//! +//! If we provide a bogus one, we'll also get an error: +//! +//! ```text +//! $ curl -H 'dropshot-demo-version: threeve' http://127.0.0.1:12345 +//! { +//! "request_id": "18c1964e-88c6-4122-8287-1f2f399871bd", +//! "message": "bad value for header \"dropshot-demo-version\": unexpected character 't' while parsing major version number: threeve" +//! } +//! ``` +//! +//! If we provide version 0.9.0 (or 1.0.4), there is no endpoint at `/`, so we +//! get a 404: +//! +//! ```text +//! $ curl -i -H 'dropshot-demo-version: 0.9.0' http://127.0.0.1:12345 +//! HTTP/1.1 404 Not Found +//! content-type: application/json +//! x-request-id: 0d3d25b8-4c48-43b2-a417-018ebce68870 +//! content-length: 84 +//! date: Thu, 26 Sep 2024 16:55:20 GMT +//! +//! { +//! "request_id": "0d3d25b8-4c48-43b2-a417-018ebce68870", +//! "message": "Not Found" +//! } +//! ``` +//! +//! If we provide version 1.0.0, we get the v1 handler we defined: +//! +//! ```text +//! $ curl -H 'dropshot-demo-version: 1.0.0' http://127.0.0.1:12345 +//! {"s":"hello from an early v1"} +//! ``` +//! +//! If we provide version 1.0.5, we get the later version that we defined, with +//! a different response body type: +//! +//! ```text +//! $ curl -H 'dropshot-demo-version: 1.0.5' http://127.0.0.1:12345 +//! {"s":"hello from a LATE v1","number":12} +//! ``` + use dropshot::endpoint; use dropshot::ApiDescription; +use dropshot::ClientSpecifiesVersionInHeader; use dropshot::ConfigDropshot; use dropshot::ConfigLogging; use dropshot::ConfigLoggingLevel; -use dropshot::DynamicVersionPolicy; use dropshot::HttpError; use dropshot::HttpResponseOk; use dropshot::HttpServerStarter; use dropshot::RequestContext; use dropshot::VersionPolicy; -use hyper::Body; -use hyper::Request; +use http::HeaderName; use schemars::JsonSchema; -use semver::Version; use serde::Serialize; -use slog::Logger; #[tokio::main] async fn main() -> Result<(), String> { // See dropshot/examples/basic.rs for more details on these pieces. - let config_dropshot: ConfigDropshot = Default::default(); + let config_dropshot = ConfigDropshot { + bind_address: "127.0.0.1:12345".parse().unwrap(), + ..Default::default() + }; let config_logging = ConfigLogging::StderrTerminal { level: ConfigLoggingLevel::Debug }; let log = config_logging @@ -34,32 +126,56 @@ async fn main() -> Result<(), String> { api.register(v1::versioned_get).unwrap(); api.register(v2::versioned_get).unwrap(); - let api_context = (); - let server = HttpServerStarter::new_with_versioning( - &config_dropshot, - api, - api_context, - &log, - None, - VersionPolicy::Dynamic(Box::new(BasicVersionPolicy {})), - ) - .map_err(|error| format!("failed to create server: {}", error))? - .start(); - - server.await -} - -/// This impl of `DynamicVersionPolicy` tells Dropshot how to determine the -/// appropriate version number for a request. -#[derive(Debug)] -struct BasicVersionPolicy {} -impl DynamicVersionPolicy for BasicVersionPolicy { - fn request_extract_version( - &self, - request: &Request, - _log: &Logger, - ) -> Result { - dropshot::parse_header(request.headers(), "dropshot-demo-version") + // Determine if we're generating the OpenAPI spec or starting the server. + // Skip the first argument because that's just the name of the program. + let args: Vec<_> = std::env::args().skip(1).collect(); + if args.is_empty() { + Err(String::from("expected subcommand: \"run\" or \"openapi\"")) + } else if args[0] == "openapi" { + if args.len() != 2 { + return Err(String::from( + "subcommand \"openapi\": expected exactly one argument", + )); + } + + // Write an OpenAPI spec for the requested version. + let version: semver::Version = + args[1].parse().map_err(|e| format!("expected semver: {}", e))?; + let _ = api + .openapi("Example API with versioning", version) + .write(&mut std::io::stdout()); + Ok(()) + } else if args[0] == "run" { + // Run a versioned server. + let api_context = (); + + // When using API versioning, you must provide a `VersionPolicy` that + // tells Dropshot how to determine what API version is being used for + // each incoming request. + // + // For this example, we will say that the client always specifies the + // version using the "dropshot-demo-version" header. You can provide + // your own impl of `DynamicVersionPolicy` that does this differently + // (e.g., filling in a default if the client doesn't provide one). + let header_name = "dropshot-demo-version" + .parse::() + .map_err(|_| String::from("demo header name was not valid"))?; + let version_impl = ClientSpecifiesVersionInHeader::new(header_name); + let version_policy = VersionPolicy::Dynamic(Box::new(version_impl)); + let server = HttpServerStarter::new_with_versioning( + &config_dropshot, + api, + api_context, + &log, + None, + version_policy, + ) + .map_err(|error| format!("failed to create server: {}", error))? + .start(); + + server.await + } else { + Err(String::from("unknown subcommand")) } } diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index e6a7e5614..2aafd0087 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -316,6 +316,7 @@ //! //! // Optional fields //! tags = [ "all", "your", "OpenAPI", "tags" ], +//! versions = .. //! }] //! ``` //! @@ -326,6 +327,9 @@ //! The tags field is used to categorize API endpoints and only impacts the //! OpenAPI spec output. //! +//! The versions field controls which versions of the API this endpoint appears +//! in. See [`API Versioning`] for more on this. +//! //! //! ### Function parameters //! @@ -697,6 +701,101 @@ //! parameters that are mandatory if `page_token` is not specified (when //! fetching the first page of data). //! +//! ## API Versioning +//! +//! Dropshot servers can host multiple versions of an API. See +//! dropshot/examples/versioning.rs for a complete, working, commented example +//! that uses a client-provided header to determine which API version to use for +//! each incoming request. +//! +//! API versioning basically works like this: +//! +//! 1. When using the `endpoint` macro to define an endpoint, you specify a +//! `versions` field as a range of [semver](https://semver.org/) version +//! strings. This identifies what versions of the API this endpoint +//! implementation appears in. Examples: +//! +//! ```text +//! // introduced in 1.0.0, present in all subsequent versions +//! versions = "1.0.0".. +//! +//! // removed *after* 2.0.0, present in all previous versions +//! versions = "2.0.0"... +//! +//! // introduced in 1.0.0, removed after 2.0.0 +//! // (present only in versions 1.0.0 through 2.0.0, inclusive) +//! versions = "1.0.0".."2.0.0" +//! +//! // present in all versions (the default) +//! versions = .. +//! ``` +//! +//! 2. When constructing the server, you provide [`VersionPolicy::Dynamic`] with +//! your own impl of [`DynamicVersionPolicy`] that tells Dropshot how to +//! determine which API version to use for each request. +//! +//! 3. When a request arrives for a server using `VersionPolicy::Dynamic`, +//! Dropshot uses the provided impl to determine the appropriate API version. +//! Then it routes requests by HTTP method and path (like usual) but only +//! considers endpoints whose version range matches the requested API +//! version. +//! +//! 4. When generating an OpenAPI document for your `ApiDescription`, you must +//! provide a specific version to generate it _for_. It will only include +//! endpoints present in that version and types referenced by those +//! endpoints. +//! +//! It is illegal to register multiple endpoints for the same HTTP method and +//! path with overlapping version ranges. +//! +//! All versioning-related configuration is optional. You can ignore it +//! altogether by simply not specifying `versions` for each endpoint and not +//! providing a `VersionPolicy` for the server (or, equivalently, providing +//! `VersionPolicy::Unversioned`). In this case, the server does not try to +//! determine a version for incoming requests. It routes requests to handlers +//! without considering API versions. +//! +//! It's maybe surprising that this mechanism only talks about versioning +//! endpoints, but usually when we think about API versioning we think about +//! types, especially the input and output types. This works because the +//! endpoint implementation itself specifies the input and output types. Let's +//! look at an example. +//! +//! Suppose you have version 1.0.0 of an API with an endpoint `my_endpoint` with +//! a body parameter `TypedBody`. You want to make a breaking change to +//! the API, creating version 2.0.0 where `MyArg` has a new required field. You +//! still want to support API version 1.0.0. Here's one clean way to do this: +//! +//! 1. Mark the existing `my_endpoint` as removed after 1.0.0: +//! 1. Move the `my_endpoint` function _and_ its input type `MyArg` to a +//! new module called `v1`. (You'd also move its output type here if +//! that's changing.) +//! 2. Change the `endpoint` macro invocation on `my_endpoint` to say +//! `versions = ..1.0.0`. This says that it was removed after 1.0.0. +//! 2. Create a new endpoint that appears in 2.0.0. +//! 1. Create a new module called `v2`. +//! 2. In `v2`, create a new type `MyArg` that looks the way you want it to +//! appear in 2.0.0. (You'd also create new versions of the output +//! types, if those are changing, too). +//! 3. Also in `v2`, create a new `my_endpoint` function that accepts and +//! returns the `v2` new versions of the types. Its `endpoint` macro +//! will say `versions = 2.0.0`. +//! +//! As mentioned above, you will also need to create your server with +//! `VersionPolicy::Dynamic` and specify how Dropshot should determine which +//! version to use for each request. But that's it! Having done this: +//! +//! * If you generate an OpenAPI doc for version 1.0.0, Dropshot will include +//! `v1::my_endpoint` and its types. +//! * If you generate an OpenAPI doc for version 2.0.0, Dropshot will include +//! `v2::my_endpoint` and its types. +//! * If a request comes in for version 1.0.0, Dropshot will route it to +//! `v1::my_endpoint` and so parse the body as `v1::MyArg`. +//! * If a request comes in for version 2.0.0, Dropshot will route it to +//! `v2::my_endpoint` and so parse the body as `v2::MyArg`. +//! +//! To see a completed example of this, see dropshot/examples/versioning.rs. +//! //! ## DTrace probes //! //! Dropshot optionally exposes two DTrace probes, `request_start` and @@ -764,6 +863,7 @@ mod schema_util; mod server; mod to_map; mod type_util; +mod versioning; mod websocket; pub mod test_util; @@ -836,11 +936,12 @@ pub use pagination::PaginationOrder; pub use pagination::PaginationParams; pub use pagination::ResultsPage; pub use pagination::WhichPage; -pub use server::DynamicVersionPolicy; pub use server::ServerContext; pub use server::ShutdownWaitFuture; -pub use server::VersionPolicy; pub use server::{HttpServer, HttpServerStarter}; +pub use versioning::ClientSpecifiesVersionInHeader; +pub use versioning::DynamicVersionPolicy; +pub use versioning::VersionPolicy; pub use websocket::WebsocketChannelResult; pub use websocket::WebsocketConnection; pub use websocket::WebsocketConnectionRaw; @@ -855,40 +956,3 @@ extern crate dropshot_endpoint; pub use dropshot_endpoint::api_description; pub use dropshot_endpoint::channel; pub use dropshot_endpoint::endpoint; - -use std::str::FromStr; - -/// Parses a required header out of a request (producing useful error messages -/// for all failure modes) -pub fn parse_header( - headers: &http::HeaderMap, - header_name: &str, -) -> Result -where - T: FromStr, - ::Err: std::fmt::Display, -{ - let v_value = headers.get(header_name).ok_or_else(|| { - HttpError::for_bad_request( - None, - format!("missing expected header {:?}", header_name), - ) - })?; - - let v_str = v_value.to_str().map_err(|_| { - HttpError::for_bad_request( - None, - format!( - "bad value for header {:?}: not ASCII: {:?}", - header_name, v_value - ), - ) - })?; - - v_str.parse::().map_err(|e| { - HttpError::for_bad_request( - None, - format!("bad value for header {:?}: {}: {}", header_name, e, v_str), - ) - }) -} diff --git a/dropshot/src/router.rs b/dropshot/src/router.rs index 8ddcb598f..3d0cb3942 100644 --- a/dropshot/src/router.rs +++ b/dropshot/src/router.rs @@ -8,12 +8,12 @@ use crate::api_description::ApiEndpointVersions; use crate::from_map::MapError; use crate::from_map::MapValue; use crate::server::ServerContext; -use crate::server::Version; use crate::ApiEndpoint; use crate::ApiEndpointBodyContentType; use http::Method; use http::StatusCode; use percent_encoding::percent_decode_str; +use semver::Version; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::sync::Arc; diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index af6f5d761..81d90baa6 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -47,6 +47,7 @@ use waitgroup::WaitGroup; use crate::config::HandlerTaskMode; use crate::RequestInfo; use slog::Logger; +use crate::versioning::VersionPolicy; // TODO Replace this with something else? type GenericError = Box; @@ -89,55 +90,6 @@ impl DropshotState { } } -pub type Version = semver::Version; - -#[derive(Debug)] -pub enum VersionPolicy { - Unversioned, - Dynamic(Box), -} - -impl VersionPolicy { - fn request_version( - &self, - request: &Request, - request_log: &Logger, - ) -> Result, HttpError> { - match self { - VersionPolicy::Unversioned => Ok(None), - VersionPolicy::Dynamic(vers_impl) => { - let result = - vers_impl.request_extract_version(request, request_log); - - match &result { - Ok(version) => { - debug!(request_log, "determined request API version"; - "version" => %version, - ); - } - Err(error) => { - error!( - request_log, - "failed to determine request API version"; - "error" => ?error, - ); - } - } - - result.map(Some) - } - } - } -} - -pub trait DynamicVersionPolicy: std::fmt::Debug + Send + Sync { - fn request_extract_version( - &self, - request: &Request, - log: &Logger, - ) -> Result; -} - /// Stores static configuration associated with the server /// TODO-cleanup merge with ConfigDropshot #[derive(Debug)] @@ -169,6 +121,14 @@ pub struct HttpServerStarter { } impl HttpServerStarter { + /// Construct a new starter with TLS disabled and version policy + /// `VersionPolicy::Unversioned` + /// + /// ## Errors + /// + /// This fails for any of the reasons that + /// [`HttpServerStarter::new_with_tls()`] can fail when the `tls` argument + /// is `None`. pub fn new( config: &ConfigDropshot, api: ApiDescription, @@ -178,6 +138,19 @@ impl HttpServerStarter { Self::new_with_tls(config, api, private, log, None) } + /// Construct a new starter with version policy + /// `VersionPolicy::Unversioned` + /// + /// ## Errors + /// + /// This fails if: + /// + /// * we could not bind to the requested address and port + /// * `tls` is `Some` but the provided configuration was not valid + /// * `api` (the `ApiDescription`) contains any endpoints that are + /// version-restricted (i.e., have "versions" to anything other than + /// `ApiEndpointVersions::All). Versioned routes are not supported with + /// unversioned servers. pub fn new_with_tls( config: &ConfigDropshot, api: ApiDescription, @@ -195,6 +168,19 @@ impl HttpServerStarter { ) } + /// Construct a new starter + /// + /// ## Errors + /// + /// This fails if: + /// + /// * we could not bind to the requested address and port + /// * `tls` is `Some` but the provided configuration was not valid + /// * `version_policy` is `VersionPolicy::Unversioned` and `api` (the + /// `ApiDescription`) contains any endpoints that are version-restricted + /// (i.e., have "versions" to anything other than + /// `ApiEndpointVersions::All). Versioned routes are not supported with + /// unversioned servers. pub fn new_with_versioning( config: &ConfigDropshot, api: ApiDescription, diff --git a/dropshot/src/versioning.rs b/dropshot/src/versioning.rs new file mode 100644 index 000000000..1d1ac45d2 --- /dev/null +++ b/dropshot/src/versioning.rs @@ -0,0 +1,174 @@ +// Copyright 2024 Oxide Computer Company + +//! Support for API versioning + +use crate::HttpError; +use http::HeaderName; +use hyper::Body; +use hyper::Request; +use semver::Version; +use slog::Logger; +use std::str::FromStr; + +/// Specifies how a server handles API versioning +#[derive(Debug)] +pub enum VersionPolicy { + /// This server does not use API versioning. + /// + /// All endpoints registered with this server must be specified with + /// versions = `ApiEndpointVersions::All` (the default). Dropshot will not + /// attempt to determine a version for each request. It will route requests + /// without considering versions at all. + Unversioned, + + /// This server uses API versioning and the provided + /// [`DynamicVersionPolicy`] specifies how to determine the API version to + /// use for each incoming request. + /// + /// With this policy, when a request arrives, Dropshot uses the provided + /// `DynamicVersionPolicy` impl to determine what API version to use when + /// handling the request. Then it routes the request to a handler based on + /// the HTTP method and path (as usual), filtering out handlers whose + /// associated `versions` does not include the requested version. + Dynamic(Box), +} + +impl VersionPolicy { + /// Given an incoming request, determine the version constraint (if any) to + /// use when routing the request to a handler + pub(crate) fn request_version( + &self, + request: &Request, + request_log: &Logger, + ) -> Result, HttpError> { + match self { + // If the server is unversioned, then we can ignore versioning + // altogether when routing. The result is still ambiguous because + // we never allow multiple endpoints to have the same HTTP method + // and path and overlapping version ranges, and unversioned servers + // only support endpoints whose version range is `All`. + VersionPolicy::Unversioned => Ok(None), + + // If the server is versioned, use the client-provided impl to + // determine which version to use. In this case the impl must + // return a value or an error -- it's not allowed to decline to + // provide a version. + VersionPolicy::Dynamic(vers_impl) => { + let result = + vers_impl.request_extract_version(request, request_log); + + match &result { + Ok(version) => { + debug!(request_log, "determined request API version"; + "version" => %version, + ); + } + Err(error) => { + error!( + request_log, + "failed to determine request API version"; + "error" => ?error, + ); + } + } + + result.map(Some) + } + } + } +} + +/// Determines the API version to use for an incoming request +/// +/// See [`ClientSpecifiesVersionInHeader`] for a basic implementation that, as +/// the name suggests, requires that the client specify the exact version they +/// want to use in a header and then always uses whatever they provide. +/// +/// This trait gives you freedom to implement a very wide range of behavior. +/// For example, you could: +/// +/// * Require that the client specify a particular version and always use that +/// * Require that the client specify a particular version but require that it +/// come from a fixed set of supported versions +/// * Allow clients to specify a specific version but supply a default if they +/// don't +/// * Allow clients to specify something else (e.g., a version range, like +/// ">1.0.0") that you then map to a specific version based on the API +/// versions that you know about +/// +/// This does mean that if you care about restricting this in any way (e.g., +/// restricting the allowed API versions to a fixed set), you must implement +/// that yourself by impl'ing this trait. +pub trait DynamicVersionPolicy: std::fmt::Debug + Send + Sync { + /// Given a request, determine the API version to use to route the request + /// to the appropriate handler + /// + /// This is expected to be a quick, synchronous operation. Most commonly, + /// you might parse a semver out of a particular header, maybe match it + /// against some supported set of versions, and maybe supply a default if + /// you don't find the header at all. + fn request_extract_version( + &self, + request: &Request, + log: &Logger, + ) -> Result; +} + +/// Implementation of `DynamicVersionPolicy` where the client must specify a +/// specific semver in a specific header and we always use whatever they +/// requested +#[derive(Debug)] +pub struct ClientSpecifiesVersionInHeader { + name: HeaderName, +} + +impl ClientSpecifiesVersionInHeader { + pub fn new(name: HeaderName) -> ClientSpecifiesVersionInHeader { + ClientSpecifiesVersionInHeader { name } + } +} + +impl DynamicVersionPolicy for ClientSpecifiesVersionInHeader { + fn request_extract_version( + &self, + request: &Request, + _log: &Logger, + ) -> Result { + parse_header(request.headers(), &self.name) + } +} + +/// Parses a required header out of a request (producing useful error messages +/// for all failure modes) +fn parse_header( + headers: &http::HeaderMap, + header_name: &HeaderName, +) -> Result +where + T: FromStr, + ::Err: std::fmt::Display, +{ + let v_value = headers.get(header_name).ok_or_else(|| { + HttpError::for_bad_request( + None, + format!("missing expected header {:?}", header_name), + ) + })?; + + let v_str = v_value.to_str().map_err(|_| { + HttpError::for_bad_request( + None, + format!( + "bad value for header {:?}: not ASCII: {:?}", + header_name, v_value + ), + ) + })?; + + v_str.parse::().map_err(|e| { + HttpError::for_bad_request( + None, + format!("bad value for header {:?}: {}: {}", header_name, e, v_str), + ) + }) +} diff --git a/dropshot/tests/test_versions.rs b/dropshot/tests/test_versions.rs index c8c0e628c..80000bd5e 100644 --- a/dropshot/tests/test_versions.rs +++ b/dropshot/tests/test_versions.rs @@ -4,6 +4,7 @@ use dropshot::endpoint; use dropshot::ApiDescription; +use dropshot::ClientSpecifiesVersionInHeader; use dropshot::ConfigDropshot; use dropshot::HttpError; use dropshot::HttpErrorResponseBody; @@ -39,7 +40,11 @@ async fn test_versions() { (), &logctx.log, None, - VersionPolicy::Dynamic(Box::new(TestVersionPolicy {})), + VersionPolicy::Dynamic(Box::new(TestVersionPolicy( + ClientSpecifiesVersionInHeader::new( + VERSION_HEADER_NAME.parse().unwrap(), + ), + ))), ) .unwrap() .start(); @@ -273,24 +278,18 @@ async fn handler4( } #[derive(Debug)] -struct TestVersionPolicy; +struct TestVersionPolicy(ClientSpecifiesVersionInHeader); impl dropshot::DynamicVersionPolicy for TestVersionPolicy { fn request_extract_version( &self, request: &http::Request, - _log: &slog::Logger, + log: &slog::Logger, ) -> Result { - dropshot::parse_header(request.headers(), VERSION_HEADER_NAME).and_then( - |v: semver::Version| { - if v.major == 4 && v.minor == 3 && v.patch == 2 { - Err(HttpError::for_bad_request( - None, - String::from(FORBIDDEN_MSG), - )) - } else { - Ok(v) - } - }, - ) + let v = self.0.request_extract_version(request, log)?; + if v.major == 4 && v.minor == 3 && v.patch == 2 { + Err(HttpError::for_bad_request(None, String::from(FORBIDDEN_MSG))) + } else { + Ok(v) + } } } From 115e74ad6f81308b0c413d78d8390424848fe81b Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 2 Oct 2024 13:31:55 -0700 Subject: [PATCH 08/23] allow overriding operation_id --- dropshot/tests/test_openapi.json | 45 ++++ dropshot/tests/test_openapi.rs | 33 ++- dropshot/tests/test_openapi_fuller.json | 45 ++++ dropshot_endpoint/src/api_trait.rs | 37 ++++ dropshot_endpoint/src/channel.rs | 25 +++ dropshot_endpoint/src/endpoint.rs | 24 +++ dropshot_endpoint/src/metadata.rs | 17 +- .../tests/output/api_trait_operation_id.rs | 193 ++++++++++++++++++ .../tests/output/channel_operation_id.rs | 63 ++++++ .../tests/output/endpoint_operation_id.rs | 64 ++++++ 10 files changed, 541 insertions(+), 5 deletions(-) create mode 100644 dropshot_endpoint/tests/output/api_trait_operation_id.rs create mode 100644 dropshot_endpoint/tests/output/channel_operation_id.rs create mode 100644 dropshot_endpoint/tests/output/endpoint_operation_id.rs diff --git a/dropshot/tests/test_openapi.json b/dropshot/tests/test_openapi.json index 9ce08e203..79ff0c495 100644 --- a/dropshot/tests/test_openapi.json +++ b/dropshot/tests/test_openapi.json @@ -200,6 +200,32 @@ } } }, + "/first_thing": { + "get": { + "tags": [ + "it" + ], + "operationId": "vzeroupper", + "responses": { + "201": { + "description": "successful creation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Response" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/impairment": { "get": { "tags": [ @@ -269,6 +295,25 @@ } } }, + "/other_thing": { + "get": { + "tags": [ + "it" + ], + "operationId": "vzerolower", + "responses": { + "default": { + "description": "", + "content": { + "*/*": { + "schema": {} + } + } + } + }, + "x-dropshot-websocket": {} + } + }, "/playing/a/bit/nicer": { "get": { "tags": [ diff --git a/dropshot/tests/test_openapi.rs b/dropshot/tests/test_openapi.rs index 039fd7e31..7fbec5ac5 100644 --- a/dropshot/tests/test_openapi.rs +++ b/dropshot/tests/test_openapi.rs @@ -1,8 +1,7 @@ // Copyright 2023 Oxide Computer Company -use dropshot::Body; use dropshot::{ - endpoint, http_response_found, http_response_see_other, + channel, endpoint, http_response_found, http_response_see_other, http_response_temporary_redirect, ApiDescription, ApiDescriptionRegisterError, FreeformBody, HttpError, HttpResponseAccepted, HttpResponseCreated, HttpResponseDeleted, HttpResponseFound, @@ -11,6 +10,7 @@ use dropshot::{ PaginationParams, Path, Query, RequestContext, ResultsPage, TagConfig, TagDetails, TypedBody, UntypedBody, }; +use dropshot::{Body, WebsocketConnection}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use std::{collections::HashMap, io::Cursor, str::from_utf8}; @@ -473,6 +473,33 @@ async fn handler25( Ok(HttpResponseCreated(Response {})) } +// test: Overridden operation id +#[endpoint { + operation_id = "vzeroupper", + method = GET, + path = "/first_thing", + tags = ["it"] +}] +async fn handler26( + _rqctx: RequestContext<()>, +) -> Result, HttpError> { + Ok(HttpResponseCreated(Response {})) +} + +// test: websocket using overriden operation id +#[channel { + protocol = WEBSOCKETS, + operation_id = "vzerolower", + path = "/other_thing", + tags = ["it"] +}] +async fn handler27( + _rqctx: RequestContext<()>, + _: WebsocketConnection, +) -> dropshot::WebsocketChannelResult { + Ok(()) +} + fn make_api( maybe_tag_config: Option, ) -> Result, ApiDescriptionRegisterError> { @@ -507,6 +534,8 @@ fn make_api( api.register(handler23)?; api.register(handler24)?; api.register(handler25)?; + api.register(handler26)?; + api.register(handler27)?; Ok(api) } diff --git a/dropshot/tests/test_openapi_fuller.json b/dropshot/tests/test_openapi_fuller.json index eabb5bd5b..3a4f31240 100644 --- a/dropshot/tests/test_openapi_fuller.json +++ b/dropshot/tests/test_openapi_fuller.json @@ -208,6 +208,32 @@ } } }, + "/first_thing": { + "get": { + "tags": [ + "it" + ], + "operationId": "vzeroupper", + "responses": { + "201": { + "description": "successful creation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Response" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/impairment": { "get": { "tags": [ @@ -277,6 +303,25 @@ } } }, + "/other_thing": { + "get": { + "tags": [ + "it" + ], + "operationId": "vzerolower", + "responses": { + "default": { + "description": "", + "content": { + "*/*": { + "schema": {} + } + } + } + }, + "x-dropshot-websocket": {} + } + }, "/playing/a/bit/nicer": { "get": { "tags": [ diff --git a/dropshot_endpoint/src/api_trait.rs b/dropshot_endpoint/src/api_trait.rs index 2a87e4b7f..1f09db5bc 100644 --- a/dropshot_endpoint/src/api_trait.rs +++ b/dropshot_endpoint/src/api_trait.rs @@ -1759,4 +1759,41 @@ mod tests { &prettyplease::unparse(&parse_quote! { #item }), ); } + + #[test] + fn test_api_trait_operation_id() { + let (item, errors) = do_trait( + quote! {}, + quote! { + trait MyTrait { + type Context; + + #[endpoint { + operation_id = "vzerolower", + method = GET, + path = "/xyz" + }] + async fn handler_xyz( + rqctx: RequestContext, + ) -> Result, HttpError>; + + #[channel { + protocol = WEBSOCKETS, + path = "/ws", + operation_id = "vzeroupper", + }] + async fn handler_ws( + rqctx: RequestContext, + upgraded: WebsocketConnection, + ) -> WebsocketChannelResult; + } + }, + ); + + assert!(errors.is_empty()); + assert_contents( + "tests/output/api_trait_operation_id.rs", + &prettyplease::unparse(&parse_quote! { #item }), + ); + } } diff --git a/dropshot_endpoint/src/channel.rs b/dropshot_endpoint/src/channel.rs index d3a199ce4..d6440cf65 100644 --- a/dropshot_endpoint/src/channel.rs +++ b/dropshot_endpoint/src/channel.rs @@ -613,4 +613,29 @@ mod tests { &prettyplease::unparse(&parse_quote! { #item }), ); } + + #[test] + fn test_channel_operation_id() { + let (item, errors) = do_channel( + quote! { + protocol = WEBSOCKETS, + path = "/my/ws/channel", + operation_id = "vzeroupper" + }, + quote! { + async fn handler_xyz( + _rqctx: RequestContext<()>, + _ws: WebsocketConnection, + ) -> Result, HttpError> { + Ok(()) + } + }, + ); + + assert!(errors.is_empty()); + assert_contents( + "tests/output/channel_operation_id.rs", + &prettyplease::unparse(&parse_quote! { #item }), + ); + } } diff --git a/dropshot_endpoint/src/endpoint.rs b/dropshot_endpoint/src/endpoint.rs index 31361909f..d2601085f 100644 --- a/dropshot_endpoint/src/endpoint.rs +++ b/dropshot_endpoint/src/endpoint.rs @@ -920,4 +920,28 @@ mod tests { Some("endpoint `handler_xyz` must have at least one RequestContext argument".to_string()) ); } + + #[test] + fn test_operation_id() { + let (item, errors) = do_endpoint( + quote! { + method = GET, + path = "/a/b/c", + operation_id = "vzeroupper" + }, + quote! { + pub async fn handler_xyz( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + Ok(()) + } + }, + ); + + assert!(errors.is_empty()); + assert_contents( + "tests/output/endpoint_operation_id.rs", + &prettyplease::unparse(&parse_quote! { #item }), + ); + } } diff --git a/dropshot_endpoint/src/metadata.rs b/dropshot_endpoint/src/metadata.rs index 09614fe8a..ff8663c51 100644 --- a/dropshot_endpoint/src/metadata.rs +++ b/dropshot_endpoint/src/metadata.rs @@ -41,6 +41,8 @@ impl MethodType { #[derive(Deserialize, Debug)] pub(crate) struct EndpointMetadata { + #[serde(default)] + pub(crate) operation_id: Option, pub(crate) method: MethodType, pub(crate) path: String, #[serde(default)] @@ -59,7 +61,7 @@ impl EndpointMetadata { get_crate(self._dropshot_crate.as_deref()) } - /// Validates metadata, returning an `EndpointMetadata` if valid. + /// Validates metadata, returning a `ValidatedEndpointMetadata` if valid. /// /// Note: the only reason we pass in attr here is to provide a span for /// error reporting. As of Rust 1.76, just passing in `attr.span()` produces @@ -74,6 +76,7 @@ impl EndpointMetadata { let errors = errors.new(); let EndpointMetadata { + operation_id, method, path, tags, @@ -130,6 +133,7 @@ impl EndpointMetadata { None } else if let Some(content_type) = content_type { Some(ValidatedEndpointMetadata { + operation_id, method, path, tags, @@ -145,6 +149,7 @@ impl EndpointMetadata { /// A validated form of endpoint metadata. pub(crate) struct ValidatedEndpointMetadata { + operation_id: Option, method: MethodType, path: String, tags: Vec, @@ -163,6 +168,8 @@ impl ValidatedEndpointMetadata { ) -> TokenStream { let path = &self.path; let content_type = self.content_type; + let operation_id = + self.operation_id.as_deref().unwrap_or(endpoint_name); let method_ident = format_ident!("{}", self.method.as_str()); let summary = doc.summary.as_ref().map(|summary| { @@ -192,7 +199,7 @@ impl ValidatedEndpointMetadata { ApiEndpointKind::Regular(endpoint_fn) => { quote_spanned! {endpoint_fn.span()=> #dropshot::ApiEndpoint::new( - #endpoint_name.to_string(), + #operation_id.to_string(), #endpoint_fn, #dropshot::Method::#method_ident, #content_type, @@ -212,7 +219,7 @@ impl ValidatedEndpointMetadata { // Seems pretty unobjectionable. quote_spanned! {attr.pound_token.span()=> #dropshot::ApiEndpoint::new_for_types::<(#(#extractor_types,)*), #ret_ty>( - #endpoint_name.to_string(), + #operation_id.to_string(), #dropshot::Method::#method_ident, #content_type, #path, @@ -241,6 +248,8 @@ pub(crate) enum ChannelProtocol { #[derive(Deserialize, Debug)] pub(crate) struct ChannelMetadata { pub(crate) protocol: ChannelProtocol, + #[serde(default)] + pub(crate) operation_id: Option, pub(crate) path: String, #[serde(default)] pub(crate) tags: Vec, @@ -273,6 +282,7 @@ impl ChannelMetadata { let ChannelMetadata { protocol: ChannelProtocol::WEBSOCKETS, + operation_id, path, tags, unpublished, @@ -307,6 +317,7 @@ impl ChannelMetadata { // Validating channel metadata also validates the corresponding // endpoint metadata. let inner = ValidatedEndpointMetadata { + operation_id, method: MethodType::GET, path, tags, diff --git a/dropshot_endpoint/tests/output/api_trait_operation_id.rs b/dropshot_endpoint/tests/output/api_trait_operation_id.rs new file mode 100644 index 000000000..52a2fc873 --- /dev/null +++ b/dropshot_endpoint/tests/output/api_trait_operation_id.rs @@ -0,0 +1,193 @@ +trait MyTrait: 'static { + type Context: dropshot::ServerContext; + fn handler_xyz( + rqctx: RequestContext, + ) -> impl ::core::future::Future< + Output = Result, HttpError>, + > + Send + 'static; + fn handler_ws( + rqctx: RequestContext, + upgraded: WebsocketConnection, + ) -> impl ::core::future::Future + Send + 'static; +} +/// Support module for the Dropshot API trait [`MyTrait`](MyTrait). +#[automatically_derived] +mod my_trait_mod { + use super::*; + const _: fn() = || { + trait TypeEq { + type This: ?Sized; + } + impl TypeEq for T { + type This = Self; + } + fn validate_websocket_connection_type() + where + T: ?Sized + TypeEq, + {} + validate_websocket_connection_type::(); + }; + /// Generate a _stub_ API description for [`MyTrait`], meant for OpenAPI + /// generation. + /// + /// Unlike [`api_description`], this function does not require an implementation + /// of [`MyTrait`] to be available, instead generating handlers that panic. + /// The return value is of type [`ApiDescription`]`<`[`StubContext`]`>`. + /// + /// The main use of this function is in cases where [`MyTrait`] is defined + /// in a separate crate from its implementation. The OpenAPI spec can then be + /// generated directly from the stub API description. + /// + /// ## Example + /// + /// A function that prints the OpenAPI spec to standard output: + /// + /// ```rust,ignore + /// fn print_openapi_spec() { + /// let stub = my_trait_mod::stub_api_description().unwrap(); + /// + /// // Generate OpenAPI spec from `stub`. + /// let spec = stub.openapi("MyTrait", "0.1.0"); + /// spec.write(&mut std::io::stdout()).unwrap(); + /// } + /// ``` + /// + /// [`MyTrait`]: MyTrait + /// [`api_description`]: my_trait_mod::api_description + /// [`ApiDescription`]: dropshot::ApiDescription + /// [`StubContext`]: dropshot::StubContext + #[automatically_derived] + pub fn stub_api_description() -> ::std::result::Result< + dropshot::ApiDescription, + dropshot::ApiDescriptionBuildErrors, + > { + let mut dropshot_api = dropshot::ApiDescription::new(); + let mut dropshot_errors: Vec = Vec::new(); + { + let endpoint_handler_xyz = dropshot::ApiEndpoint::new_for_types::< + (), + Result, HttpError>, + >( + "vzerolower".to_string(), + dropshot::Method::GET, + "application/json", + "/xyz", + ); + if let Err(error) = dropshot_api.register(endpoint_handler_xyz) { + dropshot_errors.push(error); + } + } + { + let endpoint_handler_ws = dropshot::ApiEndpoint::new_for_types::< + (dropshot::WebsocketUpgrade,), + dropshot::WebsocketEndpointResult, + >( + "vzeroupper".to_string(), + dropshot::Method::GET, + "application/json", + "/ws", + ); + if let Err(error) = dropshot_api.register(endpoint_handler_ws) { + dropshot_errors.push(error); + } + } + if !dropshot_errors.is_empty() { + Err(dropshot::ApiDescriptionBuildErrors::new(dropshot_errors)) + } else { + Ok(dropshot_api) + } + } + /// Given an implementation of [`MyTrait`], generate an API description. + /// + /// This function accepts a single type argument `ServerImpl`, turning it into a + /// Dropshot [`ApiDescription`]``. + /// The returned `ApiDescription` can then be turned into a Dropshot server that + /// accepts a concrete `Context`. + /// + /// ## Example + /// + /// ```rust,ignore + /// /// A type used to define the concrete implementation for `MyTrait`. + /// /// + /// /// This type is never constructed -- it is just a place to define your + /// /// implementation of `MyTrait`. + /// enum MyTraitImpl {} + /// + /// impl MyTrait for MyTraitImpl { + /// type Context = /* context type */; + /// + /// // ... trait methods + /// } + /// + /// #[tokio::main] + /// async fn main() { + /// // Generate the description for `MyTraitImpl`. + /// let description = my_trait_mod::api_description::().unwrap(); + /// + /// // Create a value of the concrete context type. + /// let context = /* some value of type `MyTraitImpl::Context` */; + /// + /// // Create a Dropshot server from the description. + /// let log = /* ... */; + /// let server = dropshot::ServerBuilder::new(description, context, log) + /// .start() + /// .unwrap(); + /// + /// // Run the server. + /// server.await + /// } + /// ``` + /// + /// [`ApiDescription`]: dropshot::ApiDescription + /// [`MyTrait`]: MyTrait + /// [`Context`]: MyTrait::Context + #[automatically_derived] + pub fn api_description() -> ::std::result::Result< + dropshot::ApiDescription<::Context>, + dropshot::ApiDescriptionBuildErrors, + > { + let mut dropshot_api = dropshot::ApiDescription::new(); + let mut dropshot_errors: Vec = Vec::new(); + { + let endpoint_handler_xyz = dropshot::ApiEndpoint::new( + "vzerolower".to_string(), + ::handler_xyz, + dropshot::Method::GET, + "application/json", + "/xyz", + ); + if let Err(error) = dropshot_api.register(endpoint_handler_xyz) { + dropshot_errors.push(error); + } + } + { + async fn handler_ws_adapter( + arg0: RequestContext<::Context>, + __dropshot_websocket: dropshot::WebsocketUpgrade, + ) -> dropshot::WebsocketEndpointResult { + __dropshot_websocket + .handle(move |__dropshot_websocket: WebsocketConnection| async move { + ::handler_ws(arg0, __dropshot_websocket) + .await + }) + } + { + let endpoint_handler_ws = dropshot::ApiEndpoint::new( + "vzeroupper".to_string(), + handler_ws_adapter::, + dropshot::Method::GET, + "application/json", + "/ws", + ); + if let Err(error) = dropshot_api.register(endpoint_handler_ws) { + dropshot_errors.push(error); + } + } + } + if !dropshot_errors.is_empty() { + Err(dropshot::ApiDescriptionBuildErrors::new(dropshot_errors)) + } else { + Ok(dropshot_api) + } + } +} diff --git a/dropshot_endpoint/tests/output/channel_operation_id.rs b/dropshot_endpoint/tests/output/channel_operation_id.rs new file mode 100644 index 000000000..16b95b6ed --- /dev/null +++ b/dropshot_endpoint/tests/output/channel_operation_id.rs @@ -0,0 +1,63 @@ +const _: fn() = || { + struct NeedRequestContext( + as dropshot::RequestContextArgument>::Context, + ); +}; +const _: fn() = || { + trait TypeEq { + type This: ?Sized; + } + impl TypeEq for T { + type This = Self; + } + fn validate_websocket_connection_type() + where + T: ?Sized + TypeEq, + {} + validate_websocket_connection_type::(); +}; +#[allow(non_camel_case_types, missing_docs)] +///API Endpoint: handler_xyz +struct handler_xyz {} +#[allow(non_upper_case_globals, missing_docs)] +///API Endpoint: handler_xyz +const handler_xyz: handler_xyz = handler_xyz {}; +impl From +for dropshot::ApiEndpoint< + as dropshot::RequestContextArgument>::Context, +> { + fn from(_: handler_xyz) -> Self { + #[allow(clippy::unused_async)] + async fn handler_xyz( + _rqctx: RequestContext<()>, + _ws: WebsocketConnection, + ) -> Result, HttpError> { + Ok(()) + } + const _: fn() = || { + fn future_endpoint_must_be_send(_t: T) {} + fn check_future_bounds( + arg0: RequestContext<()>, + __dropshot_websocket: WebsocketConnection, + ) { + future_endpoint_must_be_send(handler_xyz(arg0, __dropshot_websocket)); + } + }; + async fn handler_xyz_adapter( + arg0: RequestContext<()>, + __dropshot_websocket: dropshot::WebsocketUpgrade, + ) -> dropshot::WebsocketEndpointResult { + __dropshot_websocket + .handle(move |__dropshot_websocket: WebsocketConnection| async move { + handler_xyz(arg0, __dropshot_websocket).await + }) + } + dropshot::ApiEndpoint::new( + "vzeroupper".to_string(), + handler_xyz_adapter, + dropshot::Method::GET, + "application/json", + "/my/ws/channel", + ) + } +} diff --git a/dropshot_endpoint/tests/output/endpoint_operation_id.rs b/dropshot_endpoint/tests/output/endpoint_operation_id.rs new file mode 100644 index 000000000..586eb3b30 --- /dev/null +++ b/dropshot_endpoint/tests/output/endpoint_operation_id.rs @@ -0,0 +1,64 @@ +const _: fn() = || { + struct NeedRequestContext( + as dropshot::RequestContextArgument>::Context, + ); +}; +const _: fn() = || { + trait ResultTrait { + type T; + type E; + } + impl ResultTrait for Result + where + TT: dropshot::HttpResponse, + { + type T = TT; + type E = EE; + } + struct NeedHttpResponse(, HttpError> as ResultTrait>::T); + trait TypeEq { + type This: ?Sized; + } + impl TypeEq for T { + type This = Self; + } + fn validate_result_error_type() + where + T: ?Sized + TypeEq, + {} + validate_result_error_type::< + , HttpError> as ResultTrait>::E, + >(); +}; +#[allow(non_camel_case_types, missing_docs)] +///API Endpoint: handler_xyz +pub struct handler_xyz {} +#[allow(non_upper_case_globals, missing_docs)] +///API Endpoint: handler_xyz +pub const handler_xyz: handler_xyz = handler_xyz {}; +impl From +for dropshot::ApiEndpoint< + as dropshot::RequestContextArgument>::Context, +> { + fn from(_: handler_xyz) -> Self { + #[allow(clippy::unused_async)] + pub async fn handler_xyz( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + Ok(()) + } + const _: fn() = || { + fn future_endpoint_must_be_send(_t: T) {} + fn check_future_bounds(arg0: RequestContext<()>) { + future_endpoint_must_be_send(handler_xyz(arg0)); + } + }; + dropshot::ApiEndpoint::new( + "vzeroupper".to_string(), + handler_xyz, + dropshot::Method::GET, + "application/json", + "/a/b/c", + ) + } +} From d30fc789116e5510ae253dbf7f6eb22322bb34e0 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 2 Oct 2024 13:35:57 -0700 Subject: [PATCH 09/23] mention in the docs --- dropshot/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 10f8d967b..96aa20d9f 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -304,6 +304,7 @@ //! path = "/path/name/with/{named}/{variables}", //! //! // Optional fields +//! operation_id = "my_operation" // (default: name of the function) //! tags = [ "all", "your", "OpenAPI", "tags" ], //! }] //! ``` From 10113745b4fbedda9bd2957450c452e0775e7362 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 2 Oct 2024 15:00:34 -0700 Subject: [PATCH 10/23] add tests for version-specific OpenAPI specs --- dropshot/tests/test_openapi_v0.9.0.json | 100 ++++++++++++++++++++++++ dropshot/tests/test_openapi_v1.0.0.json | 100 ++++++++++++++++++++++++ dropshot/tests/test_openapi_v1.1.0.json | 90 +++++++++++++++++++++ dropshot/tests/test_openapi_v1.3.1.json | 68 ++++++++++++++++ dropshot/tests/test_openapi_v1.4.0.json | 90 +++++++++++++++++++++ dropshot/tests/test_versions.rs | 40 +++++++++- 6 files changed, 484 insertions(+), 4 deletions(-) create mode 100644 dropshot/tests/test_openapi_v0.9.0.json create mode 100644 dropshot/tests/test_openapi_v1.0.0.json create mode 100644 dropshot/tests/test_openapi_v1.1.0.json create mode 100644 dropshot/tests/test_openapi_v1.3.1.json create mode 100644 dropshot/tests/test_openapi_v1.4.0.json diff --git a/dropshot/tests/test_openapi_v0.9.0.json b/dropshot/tests/test_openapi_v0.9.0.json new file mode 100644 index 000000000..df17f64d0 --- /dev/null +++ b/dropshot/tests/test_openapi_v0.9.0.json @@ -0,0 +1,100 @@ +{ + "openapi": "3.0.3", + "info": { + "title": "Evolving API", + "version": "0.9.0" + }, + "paths": { + "/demo": { + "get": { + "operationId": "handler1", + "parameters": [ + { + "in": "query", + "name": "v", + "schema": { + "nullable": true, + "type": "string" + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "String", + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "put": { + "operationId": "handler4", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "String", + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + } + }, + "components": { + "schemas": { + "Error": { + "description": "Error information from a response.", + "type": "object", + "properties": { + "error_code": { + "type": "string" + }, + "message": { + "type": "string" + }, + "request_id": { + "type": "string" + } + }, + "required": [ + "message", + "request_id" + ] + } + }, + "responses": { + "Error": { + "description": "Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + } + } + } +} diff --git a/dropshot/tests/test_openapi_v1.0.0.json b/dropshot/tests/test_openapi_v1.0.0.json new file mode 100644 index 000000000..01a04cf31 --- /dev/null +++ b/dropshot/tests/test_openapi_v1.0.0.json @@ -0,0 +1,100 @@ +{ + "openapi": "3.0.3", + "info": { + "title": "Evolving API", + "version": "1.0.0" + }, + "paths": { + "/demo": { + "get": { + "operationId": "handler1", + "parameters": [ + { + "in": "query", + "name": "v", + "schema": { + "nullable": true, + "type": "string" + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "String", + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "put": { + "operationId": "handler4", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "String", + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + } + }, + "components": { + "schemas": { + "Error": { + "description": "Error information from a response.", + "type": "object", + "properties": { + "error_code": { + "type": "string" + }, + "message": { + "type": "string" + }, + "request_id": { + "type": "string" + } + }, + "required": [ + "message", + "request_id" + ] + } + }, + "responses": { + "Error": { + "description": "Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + } + } + } +} diff --git a/dropshot/tests/test_openapi_v1.1.0.json b/dropshot/tests/test_openapi_v1.1.0.json new file mode 100644 index 000000000..720ed5a56 --- /dev/null +++ b/dropshot/tests/test_openapi_v1.1.0.json @@ -0,0 +1,90 @@ +{ + "openapi": "3.0.3", + "info": { + "title": "Evolving API", + "version": "1.1.0" + }, + "paths": { + "/demo": { + "get": { + "operationId": "handler2", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "String", + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "put": { + "operationId": "handler4", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "String", + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + } + }, + "components": { + "schemas": { + "Error": { + "description": "Error information from a response.", + "type": "object", + "properties": { + "error_code": { + "type": "string" + }, + "message": { + "type": "string" + }, + "request_id": { + "type": "string" + } + }, + "required": [ + "message", + "request_id" + ] + } + }, + "responses": { + "Error": { + "description": "Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + } + } + } +} diff --git a/dropshot/tests/test_openapi_v1.3.1.json b/dropshot/tests/test_openapi_v1.3.1.json new file mode 100644 index 000000000..a83df0348 --- /dev/null +++ b/dropshot/tests/test_openapi_v1.3.1.json @@ -0,0 +1,68 @@ +{ + "openapi": "3.0.3", + "info": { + "title": "Evolving API", + "version": "1.3.1" + }, + "paths": { + "/demo": { + "put": { + "operationId": "handler4", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "String", + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + } + }, + "components": { + "schemas": { + "Error": { + "description": "Error information from a response.", + "type": "object", + "properties": { + "error_code": { + "type": "string" + }, + "message": { + "type": "string" + }, + "request_id": { + "type": "string" + } + }, + "required": [ + "message", + "request_id" + ] + } + }, + "responses": { + "Error": { + "description": "Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + } + } + } +} diff --git a/dropshot/tests/test_openapi_v1.4.0.json b/dropshot/tests/test_openapi_v1.4.0.json new file mode 100644 index 000000000..f421be3e4 --- /dev/null +++ b/dropshot/tests/test_openapi_v1.4.0.json @@ -0,0 +1,90 @@ +{ + "openapi": "3.0.3", + "info": { + "title": "Evolving API", + "version": "1.4.0" + }, + "paths": { + "/demo": { + "get": { + "operationId": "handler3", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "String", + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "put": { + "operationId": "handler4", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "String", + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + } + }, + "components": { + "schemas": { + "Error": { + "description": "Error information from a response.", + "type": "object", + "properties": { + "error_code": { + "type": "string" + }, + "message": { + "type": "string" + }, + "request_id": { + "type": "string" + } + }, + "required": [ + "message", + "request_id" + ] + } + }, + "responses": { + "Error": { + "description": "Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + } + } + } +} diff --git a/dropshot/tests/test_versions.rs b/dropshot/tests/test_versions.rs index 7a1ba82bb..d7e74f3d0 100644 --- a/dropshot/tests/test_versions.rs +++ b/dropshot/tests/test_versions.rs @@ -8,12 +8,16 @@ use dropshot::ClientSpecifiesVersionInHeader; use dropshot::HttpError; use dropshot::HttpErrorResponseBody; use dropshot::HttpResponseOk; +use dropshot::Query; use dropshot::RequestContext; use dropshot::ServerBuilder; use dropshot::VersionPolicy; use reqwest::Method; use reqwest::StatusCode; +use schemars::JsonSchema; use semver::Version; +use serde::Deserialize; +use std::io::Cursor; pub mod common; @@ -24,16 +28,19 @@ const HANDLER3_MSG: &str = "handler3"; const HANDLER4_MSG: &str = "handler3"; const FORBIDDEN_MSG: &str = "THAT VALUE IS FORBIDDEN!!!"; -#[tokio::test] -async fn test_versions() { - let mut api = ApiDescription::<()>::new(); +fn api() -> ApiDescription<()> { + let mut api = ApiDescription::new(); api.register(handler1).unwrap(); api.register(handler2).unwrap(); api.register(handler3).unwrap(); api.register(handler4).unwrap(); + api +} +#[tokio::test] +async fn test_versions() { let logctx = common::create_log_context("test_versions"); - let server = ServerBuilder::new(api, (), logctx.log.clone()) + let server = ServerBuilder::new(api(), (), logctx.log.clone()) .version_policy(VersionPolicy::Dynamic(Box::new(TestVersionPolicy( ClientSpecifiesVersionInHeader::new( VERSION_HEADER_NAME.parse().unwrap(), @@ -226,6 +233,30 @@ async fn test_versions() { } } +#[test] +fn test_versions_openapi() { + let api = api(); + + for version in ["0.9.0", "1.0.0", "1.1.0", "1.3.1", "1.4.0"] { + let semver: semver::Version = version.parse().unwrap(); + let mut found = Cursor::new(Vec::new()); + api.openapi("Evolving API", semver).write(&mut found).unwrap(); + let actual = std::str::from_utf8(found.get_ref()).unwrap(); + expectorate::assert_contents( + &format!("tests/test_openapi_v{}.json", version), + actual, + ); + } +} + +// This is just here so that we can tell that types are included in the spec iff +// they are referenced by endpoints in that version of the spec. +#[derive(JsonSchema, Deserialize)] +struct EarlyOptionalParams { + #[allow(dead_code)] + v: Option, +} + #[endpoint { method = GET, path = "/demo", @@ -233,6 +264,7 @@ async fn test_versions() { }] async fn handler1( _rqctx: RequestContext<()>, + _query: Query, ) -> Result, HttpError> { Ok(HttpResponseOk(HANDLER1_MSG)) } From 67c135f65445e0109ffe4b025561b07224c25485 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 2 Oct 2024 16:32:31 -0700 Subject: [PATCH 11/23] add test that uses three different approaches to naming operations and types in OpenAPI spec --- dropshot/tests/test_openapi_overrides_v1.json | 78 +++++++ dropshot/tests/test_openapi_overrides_v2.json | 78 +++++++ dropshot/tests/test_versions.rs | 198 +++++++++++++++++- 3 files changed, 353 insertions(+), 1 deletion(-) create mode 100644 dropshot/tests/test_openapi_overrides_v1.json create mode 100644 dropshot/tests/test_openapi_overrides_v2.json diff --git a/dropshot/tests/test_openapi_overrides_v1.json b/dropshot/tests/test_openapi_overrides_v1.json new file mode 100644 index 000000000..6046c9922 --- /dev/null +++ b/dropshot/tests/test_openapi_overrides_v1.json @@ -0,0 +1,78 @@ +{ + "openapi": "3.0.3", + "info": { + "title": "An API", + "version": "1.0.0" + }, + "paths": { + "/demo": { + "get": { + "operationId": "the_operation", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/MyReturn" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + } + }, + "components": { + "schemas": { + "Error": { + "description": "Error information from a response.", + "type": "object", + "properties": { + "error_code": { + "type": "string" + }, + "message": { + "type": "string" + }, + "request_id": { + "type": "string" + } + }, + "required": [ + "message", + "request_id" + ] + }, + "MyReturn": { + "type": "object", + "properties": { + "q": { + "type": "string" + } + }, + "required": [ + "q" + ] + } + }, + "responses": { + "Error": { + "description": "Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + } + } + } +} diff --git a/dropshot/tests/test_openapi_overrides_v2.json b/dropshot/tests/test_openapi_overrides_v2.json new file mode 100644 index 000000000..f483c8056 --- /dev/null +++ b/dropshot/tests/test_openapi_overrides_v2.json @@ -0,0 +1,78 @@ +{ + "openapi": "3.0.3", + "info": { + "title": "An API", + "version": "2.0.0" + }, + "paths": { + "/demo": { + "get": { + "operationId": "the_operation", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/MyReturn" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + } + }, + "components": { + "schemas": { + "Error": { + "description": "Error information from a response.", + "type": "object", + "properties": { + "error_code": { + "type": "string" + }, + "message": { + "type": "string" + }, + "request_id": { + "type": "string" + } + }, + "required": [ + "message", + "request_id" + ] + }, + "MyReturn": { + "type": "object", + "properties": { + "r": { + "type": "string" + } + }, + "required": [ + "r" + ] + } + }, + "responses": { + "Error": { + "description": "Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/Error" + } + } + } + } + } + } +} diff --git a/dropshot/tests/test_versions.rs b/dropshot/tests/test_versions.rs index d7e74f3d0..a2247e484 100644 --- a/dropshot/tests/test_versions.rs +++ b/dropshot/tests/test_versions.rs @@ -16,7 +16,7 @@ use reqwest::Method; use reqwest::StatusCode; use schemars::JsonSchema; use semver::Version; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use std::io::Cursor; pub mod common; @@ -37,6 +37,16 @@ fn api() -> ApiDescription<()> { api } +fn api_to_openapi_string( + api: &ApiDescription, + name: &str, + version: &semver::Version, +) -> String { + let mut contents = Cursor::new(Vec::new()); + api.openapi(name, version.clone()).write(&mut contents).unwrap(); + String::from_utf8(contents.get_ref().to_vec()).unwrap() +} + #[tokio::test] async fn test_versions() { let logctx = common::create_log_context("test_versions"); @@ -233,6 +243,8 @@ async fn test_versions() { } } +/// Test that the generated OpenAPI spec only refers to handlers in that version +/// and types that are used by those handlers. #[test] fn test_versions_openapi() { let api = api(); @@ -249,6 +261,190 @@ fn test_versions_openapi() { } } +// The contents of this module logically belongs inside +// test_versions_openapi_same_names(). It can't go there due to +// oxidecomputer/dropshot#1128. +mod trait_based { + use super::*; + + #[derive(JsonSchema, Serialize)] + #[schemars(rename = "MyReturn")] + pub struct MyReturnV1 { + #[allow(dead_code)] + q: String, + } + + #[derive(JsonSchema, Serialize)] + #[schemars(rename = "MyReturn")] + pub struct MyReturnV2 { + #[allow(dead_code)] + r: String, + } + + #[dropshot::api_description] + // This `allow(dead_code)` works around oxidecomputer/dropshot#1129. + #[allow(dead_code)] + pub trait MyApi { + type Context; + + #[endpoint { + method = GET, + path = "/demo", + versions = .."1.0.0", + operation_id = "the_operation", + }] + async fn the_operation_v1( + _rqctx: RequestContext, + ) -> Result, HttpError>; + + #[endpoint { + method = GET, + path = "/demo", + versions = "2.0.0".., + operation_id = "the_operation" + }] + async fn the_operation_v2( + _rqctx: RequestContext, + ) -> Result, HttpError>; + } +} + +/// Test three different ways to define the same operation in two versions +/// (using different handlers). These should all produce the same pair of +/// specs. +#[test] +fn test_versions_openapi_same_names() { + // This approach uses freestanding functions in separate modules. + let api_function_modules = { + mod v1 { + use super::*; + + #[derive(JsonSchema, Serialize)] + pub struct MyReturn { + #[allow(dead_code)] + q: String, + } + + #[endpoint { + method = GET, + path = "/demo", + versions = .."1.0.0" + }] + pub async fn the_operation( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + unimplemented!(); + } + } + + mod v2 { + use super::*; + + #[derive(JsonSchema, Serialize)] + pub struct MyReturn { + #[allow(dead_code)] + r: String, + } + + #[endpoint { + method = GET, + path = "/demo", + versions = "2.0.0".. + }] + pub async fn the_operation( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + unimplemented!(); + } + } + + let mut api = ApiDescription::new(); + api.register(v1::the_operation).unwrap(); + api.register(v2::the_operation).unwrap(); + api + }; + + // This approach uses freestanding functions and types all in one module. + // This requires applying overrides to the names in order to have them show + // up with the same name in each version. + let api_function_overrides = { + #[derive(JsonSchema, Serialize)] + #[schemars(rename = "MyReturn")] + struct MyReturnV1 { + #[allow(dead_code)] + q: String, + } + + #[endpoint { + method = GET, + path = "/demo", + versions = .."1.0.0", + operation_id = "the_operation", + }] + async fn the_operation_v1( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + unimplemented!(); + } + + #[derive(JsonSchema, Serialize)] + #[schemars(rename = "MyReturn")] + struct MyReturnV2 { + #[allow(dead_code)] + r: String, + } + + #[endpoint { + method = GET, + path = "/demo", + versions = "2.0.0".., + operation_id = "the_operation" + }] + async fn the_operation_v2( + _rqctx: RequestContext<()>, + ) -> Result, HttpError> { + unimplemented!(); + } + + let mut api = ApiDescription::new(); + api.register(the_operation_v1).unwrap(); + api.register(the_operation_v2).unwrap(); + api + }; + + // This approach uses the trait-based interface, which requires using + // `operation_id` to override the operation name if you want the name to be + // the same across versions. + let api_trait_overrides = + trait_based::my_api_mod::stub_api_description().unwrap(); + + const NAME: &str = "An API"; + let v1 = semver::Version::new(1, 0, 0); + let v2 = semver::Version::new(2, 0, 0); + let func_mods_v1 = api_to_openapi_string(&api_function_modules, NAME, &v1); + let func_mods_v2 = api_to_openapi_string(&api_function_modules, NAME, &v2); + let func_overrides_v1 = + api_to_openapi_string(&api_function_overrides, NAME, &v1); + let func_overrides_v2 = + api_to_openapi_string(&api_function_overrides, NAME, &v2); + let traits_v1 = api_to_openapi_string(&api_trait_overrides, NAME, &v1); + let traits_v2 = api_to_openapi_string(&api_trait_overrides, NAME, &v2); + + expectorate::assert_contents( + "tests/test_openapi_overrides_v1.json", + &func_overrides_v1, + ); + expectorate::assert_contents( + "tests/test_openapi_overrides_v2.json", + &func_overrides_v2, + ); + + assert_eq!(func_mods_v1, func_overrides_v1); + assert_eq!(func_mods_v1, traits_v1); + assert_eq!(func_mods_v2, func_overrides_v2); + assert_eq!(func_mods_v2, traits_v2); +} + // This is just here so that we can tell that types are included in the spec iff // they are referenced by endpoints in that version of the spec. #[derive(JsonSchema, Deserialize)] From 6344e3c36de2f90108252ab089dfbc36b5bc1f5b Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 2 Oct 2024 16:39:16 -0700 Subject: [PATCH 12/23] fix test to actually test that types appear in the correct versions --- dropshot/tests/test_openapi_v0.9.0.json | 24 +++++++++---------- dropshot/tests/test_openapi_v1.0.0.json | 24 +++++++++---------- dropshot/tests/test_versions.rs | 31 +++++++++++++++++-------- 3 files changed, 45 insertions(+), 34 deletions(-) diff --git a/dropshot/tests/test_openapi_v0.9.0.json b/dropshot/tests/test_openapi_v0.9.0.json index df17f64d0..93dc53e2b 100644 --- a/dropshot/tests/test_openapi_v0.9.0.json +++ b/dropshot/tests/test_openapi_v0.9.0.json @@ -8,24 +8,13 @@ "/demo": { "get": { "operationId": "handler1", - "parameters": [ - { - "in": "query", - "name": "v", - "schema": { - "nullable": true, - "type": "string" - } - } - ], "responses": { "200": { "description": "successful operation", "content": { "application/json": { "schema": { - "title": "String", - "type": "string" + "$ref": "#/components/schemas/EarlyReturn" } } } @@ -64,6 +53,17 @@ }, "components": { "schemas": { + "EarlyReturn": { + "type": "object", + "properties": { + "real_message": { + "type": "string" + } + }, + "required": [ + "real_message" + ] + }, "Error": { "description": "Error information from a response.", "type": "object", diff --git a/dropshot/tests/test_openapi_v1.0.0.json b/dropshot/tests/test_openapi_v1.0.0.json index 01a04cf31..c88ae8d2e 100644 --- a/dropshot/tests/test_openapi_v1.0.0.json +++ b/dropshot/tests/test_openapi_v1.0.0.json @@ -8,24 +8,13 @@ "/demo": { "get": { "operationId": "handler1", - "parameters": [ - { - "in": "query", - "name": "v", - "schema": { - "nullable": true, - "type": "string" - } - } - ], "responses": { "200": { "description": "successful operation", "content": { "application/json": { "schema": { - "title": "String", - "type": "string" + "$ref": "#/components/schemas/EarlyReturn" } } } @@ -64,6 +53,17 @@ }, "components": { "schemas": { + "EarlyReturn": { + "type": "object", + "properties": { + "real_message": { + "type": "string" + } + }, + "required": [ + "real_message" + ] + }, "Error": { "description": "Error information from a response.", "type": "object", diff --git a/dropshot/tests/test_versions.rs b/dropshot/tests/test_versions.rs index a2247e484..a3b0e5276 100644 --- a/dropshot/tests/test_versions.rs +++ b/dropshot/tests/test_versions.rs @@ -8,7 +8,6 @@ use dropshot::ClientSpecifiesVersionInHeader; use dropshot::HttpError; use dropshot::HttpErrorResponseBody; use dropshot::HttpResponseOk; -use dropshot::Query; use dropshot::RequestContext; use dropshot::ServerBuilder; use dropshot::VersionPolicy; @@ -16,7 +15,8 @@ use reqwest::Method; use reqwest::StatusCode; use schemars::JsonSchema; use semver::Version; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; +use serde::Serialize; use std::io::Cursor; pub mod common; @@ -237,7 +237,20 @@ async fn test_versions() { let error: HttpErrorResponseBody = response.json().await.unwrap(); assert_eq!(error.message, t.body_contents); } else { - let body: String = response.json().await.unwrap(); + // This is ugly. But it's concise! + // + // We want to use a different type for `handler1` just so that we + // can check in test_versions_openapi() that it appears in the + // OpenAPI spec only in the appropriate versions. So if we're + // expecting to get something back from handler1, then parse the + // type a little differently. + let body: String = if t.body_contents == HANDLER1_MSG { + let body: EarlyReturn = response.json().await.unwrap(); + body.real_message + } else { + response.json().await.unwrap() + }; + assert_eq!(body, t.body_contents); } } @@ -447,10 +460,9 @@ fn test_versions_openapi_same_names() { // This is just here so that we can tell that types are included in the spec iff // they are referenced by endpoints in that version of the spec. -#[derive(JsonSchema, Deserialize)] -struct EarlyOptionalParams { - #[allow(dead_code)] - v: Option, +#[derive(Deserialize, JsonSchema, Serialize)] +struct EarlyReturn { + real_message: String, } #[endpoint { @@ -460,9 +472,8 @@ struct EarlyOptionalParams { }] async fn handler1( _rqctx: RequestContext<()>, - _query: Query, -) -> Result, HttpError> { - Ok(HttpResponseOk(HANDLER1_MSG)) +) -> Result, HttpError> { + Ok(HttpResponseOk(EarlyReturn { real_message: HANDLER1_MSG.to_string() })) } #[endpoint { From 7a8683df1ac5c9cd936114f83f8bb33a80e0f477 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 2 Oct 2024 16:42:14 -0700 Subject: [PATCH 13/23] fix whitespace --- dropshot_endpoint/src/api_trait.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dropshot_endpoint/src/api_trait.rs b/dropshot_endpoint/src/api_trait.rs index 1f09db5bc..9fffac944 100644 --- a/dropshot_endpoint/src/api_trait.rs +++ b/dropshot_endpoint/src/api_trait.rs @@ -1780,7 +1780,7 @@ mod tests { #[channel { protocol = WEBSOCKETS, path = "/ws", - operation_id = "vzeroupper", + operation_id = "vzeroupper", }] async fn handler_ws( rqctx: RequestContext, From 368c36623b84bd7e1cb440fb2a0bea20205f10fb Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 2 Oct 2024 16:51:02 -0700 Subject: [PATCH 14/23] fix changelog mismerge --- CHANGELOG.adoc | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 42017db9c..36be9ad1e 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -63,6 +63,14 @@ starter.start() + We'd like to remove the `HttpServerStarter` altogether, so let us know if you're still using it for some reason. +* https://github.com/oxidecomputer/dropshot/pull/1115[#1115] Dropshot now supports hosting multiple versions of an API at a single server and routing to the correct version based on the incoming request. See documentation for details. If you don't care about this, you can mostly ignore it, but see "Breaking Changes" below. + +=== Breaking Changes + +* Dropshot now expects that APIs use https://semver.org/[Semver] values for their version string. Concretely, this only means that the `version` argument to `ApiDescription::openapi` (which generates an OpenAPI document) must be a `semver::Version`. Previously, it was `AsRef`. +* If you're invoking `ApiEndpoint::new` directly or constructing one as a literal (both of which are uncommon), you must provide a new `ApiEndpointVersions` value describing which versions this endpoint implements. You can use `ApiEndpointVersions::All` if you don't care about versioning. + + == 0.12.0 (released 2024-09-26) https://github.com/oxidecomputer/dropshot/compare/v0.11.0\...v0.12.0[Full list of commits] @@ -79,13 +87,6 @@ https://github.com/oxidecomputer/dropshot/compare/v0.11.0\...v0.12.0[Full list o There are no other known breaking changes in these crates that affect Dropshot. If you have any trouble with this upgrade, please let us know by filing an issue. -* https://github.com/oxidecomputer/dropshot/pull/1115[#1115] Dropshot now supports hosting multiple versions of an API at a single server and routing to the correct version based on the incoming request. See documentation for details. If you don't care about this, you can mostly ignore it, but see "Breaking Changes" below. - -=== Breaking Changes - -* Dropshot now expects that APIs use https://semver.org/[Semver] values for their version string. Concretely, this only means that the `version` argument to `ApiDescription::openapi` (which generates an OpenAPI document) must be a `semver::Version`. Previously, it was `AsRef`. -* If you're invoking `ApiEndpoint::new` directly or constructing one as a literal (both of which are uncommon), you must provide a new `ApiEndpointVersions` value describing which versions this endpoint implements. You can use `ApiEndpointVersions::All` if you don't care about versioning. - == 0.11.0 (released 2024-08-21) https://github.com/oxidecomputer/dropshot/compare/v0.10.1\...v0.11.0[Full list of commits] From d19f3e0552d241862a6b99ecd0f676e3d284a770 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 2 Oct 2024 16:58:15 -0700 Subject: [PATCH 15/23] self-review nits --- dropshot/src/lib.rs | 2 +- dropshot/tests/test_versions.rs | 225 +++++++++++++++++--------------- 2 files changed, 118 insertions(+), 109 deletions(-) diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index cd9a68a54..179ae5a2c 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -318,7 +318,7 @@ //! OpenAPI spec output. //! //! The versions field controls which versions of the API this endpoint appears -//! in. See [`API Versioning`] for more on this. +//! in. See "API Versioning" for more on this. //! //! //! ### Function parameters diff --git a/dropshot/tests/test_versions.rs b/dropshot/tests/test_versions.rs index a3b0e5276..18af6d019 100644 --- a/dropshot/tests/test_versions.rs +++ b/dropshot/tests/test_versions.rs @@ -47,6 +47,77 @@ fn api_to_openapi_string( String::from_utf8(contents.get_ref().to_vec()).unwrap() } +// This is just here so that we can tell that types are included in the spec iff +// they are referenced by endpoints in that version of the spec. +#[derive(Deserialize, JsonSchema, Serialize)] +struct EarlyReturn { + real_message: String, +} + +#[endpoint { + method = GET, + path = "/demo", + versions = .."1.0.0", +}] +async fn handler1( + _rqctx: RequestContext<()>, +) -> Result, HttpError> { + Ok(HttpResponseOk(EarlyReturn { real_message: HANDLER1_MSG.to_string() })) +} + +#[endpoint { + method = GET, + path = "/demo", + versions = "1.1.0".."1.3.0", +}] +async fn handler2( + _rqctx: RequestContext<()>, +) -> Result, HttpError> { + Ok(HttpResponseOk(HANDLER2_MSG)) +} + +#[endpoint { + method = GET, + path = "/demo", + versions = "1.4.0".., +}] +async fn handler3( + _rqctx: RequestContext<()>, +) -> Result, HttpError> { + Ok(HttpResponseOk(HANDLER3_MSG)) +} + +#[endpoint { + method = PUT, + path = "/demo", + versions = .. +}] +async fn handler4( + _rqctx: RequestContext<()>, +) -> Result, HttpError> { + Ok(HttpResponseOk(HANDLER4_MSG)) +} + +#[derive(Debug)] +struct TestVersionPolicy(ClientSpecifiesVersionInHeader); +impl dropshot::DynamicVersionPolicy for TestVersionPolicy { + fn request_extract_version( + &self, + request: &http::Request, + log: &slog::Logger, + ) -> Result { + let v = self.0.request_extract_version(request, log)?; + if v.major == 4 && v.minor == 3 && v.patch == 2 { + Err(HttpError::for_bad_request(None, String::from(FORBIDDEN_MSG))) + } else { + Ok(v) + } + } +} + +/// Define an API with different versions and run through an exhaustive battery +/// of tests showing that we use the correct handler for each incoming request +/// based on the version requested. #[tokio::test] async fn test_versions() { let logctx = common::create_log_context("test_versions"); @@ -65,10 +136,16 @@ async fn test_versions() { #[derive(Debug)] struct TestCase<'a> { + /// HTTP method for the request method: Method, + /// HTTP path for the request path: &'a str, + /// Value to pass for our requested version + /// (string supports providing bad (non-semver) input) header: Option<&'a str>, + /// expected HTTP response status code expected_status: StatusCode, + /// expected HTTP response body contents body_contents: String, } @@ -274,54 +351,6 @@ fn test_versions_openapi() { } } -// The contents of this module logically belongs inside -// test_versions_openapi_same_names(). It can't go there due to -// oxidecomputer/dropshot#1128. -mod trait_based { - use super::*; - - #[derive(JsonSchema, Serialize)] - #[schemars(rename = "MyReturn")] - pub struct MyReturnV1 { - #[allow(dead_code)] - q: String, - } - - #[derive(JsonSchema, Serialize)] - #[schemars(rename = "MyReturn")] - pub struct MyReturnV2 { - #[allow(dead_code)] - r: String, - } - - #[dropshot::api_description] - // This `allow(dead_code)` works around oxidecomputer/dropshot#1129. - #[allow(dead_code)] - pub trait MyApi { - type Context; - - #[endpoint { - method = GET, - path = "/demo", - versions = .."1.0.0", - operation_id = "the_operation", - }] - async fn the_operation_v1( - _rqctx: RequestContext, - ) -> Result, HttpError>; - - #[endpoint { - method = GET, - path = "/demo", - versions = "2.0.0".., - operation_id = "the_operation" - }] - async fn the_operation_v2( - _rqctx: RequestContext, - ) -> Result, HttpError>; - } -} - /// Test three different ways to define the same operation in two versions /// (using different handlers). These should all produce the same pair of /// specs. @@ -458,70 +487,50 @@ fn test_versions_openapi_same_names() { assert_eq!(func_mods_v2, traits_v2); } -// This is just here so that we can tell that types are included in the spec iff -// they are referenced by endpoints in that version of the spec. -#[derive(Deserialize, JsonSchema, Serialize)] -struct EarlyReturn { - real_message: String, -} +// The contents of this module logically belongs inside +// test_versions_openapi_same_names(). It can't go there due to +// oxidecomputer/dropshot#1128. +mod trait_based { + use super::*; -#[endpoint { - method = GET, - path = "/demo", - versions = .."1.0.0", -}] -async fn handler1( - _rqctx: RequestContext<()>, -) -> Result, HttpError> { - Ok(HttpResponseOk(EarlyReturn { real_message: HANDLER1_MSG.to_string() })) -} + #[derive(JsonSchema, Serialize)] + #[schemars(rename = "MyReturn")] + pub struct MyReturnV1 { + #[allow(dead_code)] + q: String, + } -#[endpoint { - method = GET, - path = "/demo", - versions = "1.1.0".."1.3.0", -}] -async fn handler2( - _rqctx: RequestContext<()>, -) -> Result, HttpError> { - Ok(HttpResponseOk(HANDLER2_MSG)) -} + #[derive(JsonSchema, Serialize)] + #[schemars(rename = "MyReturn")] + pub struct MyReturnV2 { + #[allow(dead_code)] + r: String, + } -#[endpoint { - method = GET, - path = "/demo", - versions = "1.4.0".., -}] -async fn handler3( - _rqctx: RequestContext<()>, -) -> Result, HttpError> { - Ok(HttpResponseOk(HANDLER3_MSG)) -} + #[dropshot::api_description] + // This `allow(dead_code)` works around oxidecomputer/dropshot#1129. + #[allow(dead_code)] + pub trait MyApi { + type Context; -#[endpoint { - method = PUT, - path = "/demo", - versions = .. -}] -async fn handler4( - _rqctx: RequestContext<()>, -) -> Result, HttpError> { - Ok(HttpResponseOk(HANDLER4_MSG)) -} + #[endpoint { + method = GET, + path = "/demo", + versions = .."1.0.0", + operation_id = "the_operation", + }] + async fn the_operation_v1( + _rqctx: RequestContext, + ) -> Result, HttpError>; -#[derive(Debug)] -struct TestVersionPolicy(ClientSpecifiesVersionInHeader); -impl dropshot::DynamicVersionPolicy for TestVersionPolicy { - fn request_extract_version( - &self, - request: &http::Request, - log: &slog::Logger, - ) -> Result { - let v = self.0.request_extract_version(request, log)?; - if v.major == 4 && v.minor == 3 && v.patch == 2 { - Err(HttpError::for_bad_request(None, String::from(FORBIDDEN_MSG))) - } else { - Ok(v) - } + #[endpoint { + method = GET, + path = "/demo", + versions = "2.0.0".., + operation_id = "the_operation" + }] + async fn the_operation_v2( + _rqctx: RequestContext, + ) -> Result, HttpError>; } } From 3d2addd3b847012d9ea2f4578e148d0ab3bc944b Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 3 Oct 2024 14:30:19 -0700 Subject: [PATCH 16/23] cleaner logging for endpoint versions --- dropshot/src/api_description.rs | 28 ++++++++++++++++++++++++++++ dropshot/src/server.rs | 2 +- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/dropshot/src/api_description.rs b/dropshot/src/api_description.rs index a81837a77..895918a5d 100644 --- a/dropshot/src/api_description.rs +++ b/dropshot/src/api_description.rs @@ -1227,6 +1227,34 @@ impl ApiEndpointVersions { } } +impl slog::Value for ApiEndpointVersions { + fn serialize( + &self, + _record: &slog::Record, + key: slog::Key, + serializer: &mut dyn slog::Serializer, + ) -> slog::Result { + match self { + ApiEndpointVersions::All => serializer.emit_str(key, "all"), + ApiEndpointVersions::From(from) => serializer.emit_arguments( + key, + &format_args!("all starting from {}", from), + ), + ApiEndpointVersions::FromUntil(OrderedVersionPair { + earliest, + latest, + }) => serializer.emit_arguments( + key, + &format_args!("from {} to {}", earliest, latest), + ), + ApiEndpointVersions::Until(until) => serializer.emit_arguments( + key, + &format_args!("all ending with {}", until), + ), + } + } +} + /// Returns true iff the schema represents the void schema that matches no data. fn is_empty(schema: &schemars::schema::Schema) -> bool { if let schemars::schema::Schema::Bool(false) = schema { diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index 7c82238f5..bb077ed93 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -221,7 +221,7 @@ impl HttpServerStarter { debug!(&log, "registered endpoint"; "method" => &method, "path" => &path, - "versions" => ?endpoint.versions, + "versions" => &endpoint.versions, ); } From 0f1bdacc3215fdbff5fbfa1616b071aeb9ff0c18 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 3 Oct 2024 15:16:57 -0700 Subject: [PATCH 17/23] make the end of the versions range NON-inclusive --- dropshot/src/api_description.rs | 83 +++++++++++++------------ dropshot/src/lib.rs | 9 +-- dropshot/src/router.rs | 18 +++--- dropshot/tests/test_openapi_v1.0.0.json | 32 ---------- dropshot/tests/test_openapi_v1.3.1.json | 22 +++++++ dropshot/tests/test_versions.rs | 41 ++++++------ 6 files changed, 95 insertions(+), 110 deletions(-) diff --git a/dropshot/src/api_description.rs b/dropshot/src/api_description.rs index 895918a5d..f7f7893a1 100644 --- a/dropshot/src/api_description.rs +++ b/dropshot/src/api_description.rs @@ -1079,7 +1079,7 @@ pub enum ApiEndpointVersions { // We use an extra level of indirection to enforce that the versions here // are provided in order. FromUntil(OrderedVersionPair), - /// this endpoint was present in all versions up to (and including) this + /// this endpoint was present in all versions up to (but not including) this /// specific version Until(semver::Version), } @@ -1087,7 +1087,7 @@ pub enum ApiEndpointVersions { #[derive(Debug, Eq, PartialEq)] pub struct OrderedVersionPair { earliest: semver::Version, - latest: semver::Version, + until: semver::Version, } impl ApiEndpointVersions { @@ -1105,9 +1105,9 @@ impl ApiEndpointVersions { pub fn from_until( earliest: semver::Version, - latest: semver::Version, + until: semver::Version, ) -> Result { - if latest < earliest { + if until < earliest { return Err( "versions in a from-until version range must be provided \ in order", @@ -1116,7 +1116,7 @@ impl ApiEndpointVersions { Ok(ApiEndpointVersions::FromUntil(OrderedVersionPair { earliest, - latest, + until, })) } @@ -1140,9 +1140,13 @@ impl ApiEndpointVersions { ApiEndpointVersions::From(earliest) => version >= earliest, ApiEndpointVersions::FromUntil(OrderedVersionPair { earliest, - latest, - }) => version >= earliest && version <= latest, - ApiEndpointVersions::Until(latest) => version <= latest, + until, + }) => { + version >= earliest + && (version < until + || (version == until && earliest == until)) + } + ApiEndpointVersions::Until(until) => version < until, } } @@ -1173,56 +1177,53 @@ impl ApiEndpointVersions { // more complicated cases ( ApiEndpointVersions::From(earliest), - ApiEndpointVersions::Until(latest), - ) => latest >= earliest, + u @ ApiEndpointVersions::Until(_), + ) => u.matches(Some(&earliest)), ( - ApiEndpointVersions::Until(latest), + u @ ApiEndpointVersions::Until(_), ApiEndpointVersions::From(earliest), - ) => latest >= earliest, + ) => u.matches(Some(&earliest)), ( ApiEndpointVersions::From(earliest), ApiEndpointVersions::FromUntil(OrderedVersionPair { earliest: _, - latest, + until, }), - ) => earliest <= latest, + ) => earliest < until, ( ApiEndpointVersions::FromUntil(OrderedVersionPair { earliest: _, - latest, + until, }), ApiEndpointVersions::From(earliest), - ) => earliest <= latest, + ) => earliest < until, ( - ApiEndpointVersions::Until(latest), + u @ ApiEndpointVersions::Until(_), ApiEndpointVersions::FromUntil(OrderedVersionPair { earliest, - latest: _, + until: _, }), - ) => earliest <= latest, + ) => u.matches(Some(&earliest)), ( ApiEndpointVersions::FromUntil(OrderedVersionPair { earliest, - latest: _, + until: _, }), - ApiEndpointVersions::Until(latest), - ) => earliest <= latest, + u @ ApiEndpointVersions::Until(_), + ) => u.matches(Some(&earliest)), ( - ApiEndpointVersions::FromUntil(OrderedVersionPair { + r1 @ ApiEndpointVersions::FromUntil(OrderedVersionPair { earliest: earliest1, - latest: latest1, + until: _, }), - ApiEndpointVersions::FromUntil(OrderedVersionPair { + r2 @ ApiEndpointVersions::FromUntil(OrderedVersionPair { earliest: earliest2, - latest: latest2, + until: _, }), - ) => { - (earliest1 <= earliest2 && earliest2 <= latest1) - || (earliest2 <= earliest1 && earliest1 <= latest2) - } + ) => r1.matches(Some(&earliest2)) || r2.matches(Some(&earliest1)), } } } @@ -1242,7 +1243,7 @@ impl slog::Value for ApiEndpointVersions { ), ApiEndpointVersions::FromUntil(OrderedVersionPair { earliest, - latest, + until: latest, }) => serializer.emit_arguments( key, &format_args!("from {} to {}", earliest, latest), @@ -1856,14 +1857,14 @@ mod test { TestCase::new(&v_from, Version::new(5, 0, 0), true), TestCase::new(&v_until, Version::new(0, 0, 0), true), TestCase::new(&v_until, Version::new(4, 5, 5), true), - TestCase::new(&v_until, Version::new(4, 5, 6), true), + TestCase::new(&v_until, Version::new(4, 5, 6), false), TestCase::new(&v_until, Version::new(4, 5, 7), false), TestCase::new(&v_until, Version::new(37, 0, 0), false), TestCase::new(&v_fromuntil, Version::new(0, 0, 0), false), TestCase::new(&v_fromuntil, Version::new(1, 2, 2), false), TestCase::new(&v_fromuntil, Version::new(1, 2, 3), true), TestCase::new(&v_fromuntil, Version::new(4, 5, 5), true), - TestCase::new(&v_fromuntil, Version::new(4, 5, 6), true), + TestCase::new(&v_fromuntil, Version::new(4, 5, 6), false), TestCase::new(&v_fromuntil, Version::new(4, 5, 7), false), TestCase::new(&v_fromuntil, Version::new(12, 0, 0), false), TestCase::new(&v_oneversion, Version::new(1, 2, 2), false), @@ -1954,23 +1955,23 @@ mod test { // "from" + "until": overlap is exactly one point TestCase::new( &v_from, - &ApiEndpointVersions::until(Version::new(1, 2, 3)), + &ApiEndpointVersions::until(Version::new(1, 2, 4)), true, ), - // "from" + "until": no overlap + // "from" + "until": no overlap (right on the edge) TestCase::new( &v_from, - &ApiEndpointVersions::until(Version::new(1, 2, 2)), + &ApiEndpointVersions::until(Version::new(1, 2, 3)), false, ), - // "from" from "from-until": overlap + // "from" + "from-until": overlap TestCase::new(&v_from, &v_fromuntil, true), // "from" + "from-until": no overlap TestCase::new( &v_from, &ApiEndpointVersions::from_until( Version::new(1, 2, 0), - Version::new(1, 2, 2), + Version::new(1, 2, 3), ) .unwrap(), false, @@ -1990,7 +1991,7 @@ mod test { TestCase::new( &v_until, &ApiEndpointVersions::from_until( - Version::new(5, 0, 0), + Version::new(4, 5, 6), Version::new(6, 0, 0), ) .unwrap(), @@ -2005,7 +2006,7 @@ mod test { &v_fromuntil, &ApiEndpointVersions::from_until( Version::new(0, 0, 1), - Version::new(1, 2, 2), + Version::new(1, 2, 3), ) .unwrap(), false, @@ -2015,7 +2016,7 @@ mod test { &v_fromuntil, &ApiEndpointVersions::from_until( Version::new(0, 0, 1), - Version::new(1, 2, 3), + Version::new(1, 2, 4), ) .unwrap(), true, diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 179ae5a2c..265e226cd 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -709,11 +709,12 @@ //! // introduced in 1.0.0, present in all subsequent versions //! versions = "1.0.0".. //! -//! // removed *after* 2.0.0, present in all previous versions -//! versions = "2.0.0"... +//! // removed in 2.0.0, present in all previous versions +//! // (not present in 2.0.0 itself) +//! versions = .."2.0.0" //! -//! // introduced in 1.0.0, removed after 2.0.0 -//! // (present only in versions 1.0.0 through 2.0.0, inclusive) +//! // introduced in 1.0.0, removed in 2.0.0 +//! // (present only in all 1.x versions, NOT 2.0.0 or later) //! versions = "1.0.0".."2.0.0" //! //! // present in all versions (the default) diff --git a/dropshot/src/router.rs b/dropshot/src/router.rs index 96546eaf9..8f43e2bb0 100644 --- a/dropshot/src/router.rs +++ b/dropshot/src/router.rs @@ -1315,6 +1315,9 @@ mod test { fn test_router_versioned() { // Install handlers for a particular route for a bunch of different // versions. + // + // This is not exhaustive because the matching logic is tested + // exhaustively elsewhere. let method = Method::GET; let path: super::InputPath<'static> = "/foo".into(); let mut router = HttpRouter::new(); @@ -1346,13 +1349,8 @@ mod test { .lookup_route(&method, path, Some(&Version::new(1, 2, 2))) .unwrap(); assert_eq!(result.handler.label(), "h1"); - let result = router - .lookup_route(&method, path, Some(&Version::new(1, 2, 3))) - .unwrap(); - assert_eq!(result.handler.label(), "h1"); - let error = router - .lookup_route(&method, path, Some(&Version::new(1, 2, 4))) + .lookup_route(&method, path, Some(&Version::new(1, 2, 3))) .unwrap_err(); assert_eq!(error.status_code, StatusCode::NOT_FOUND); @@ -1364,11 +1362,11 @@ mod test { .lookup_route(&method, path, Some(&Version::new(2, 1, 0))) .unwrap(); assert_eq!(result.handler.label(), "h2"); - let result = router - .lookup_route(&method, path, Some(&Version::new(3, 0, 0))) - .unwrap(); - assert_eq!(result.handler.label(), "h2"); + let error = router + .lookup_route(&method, path, Some(&Version::new(3, 0, 0))) + .unwrap_err(); + assert_eq!(error.status_code, StatusCode::NOT_FOUND); let error = router .lookup_route(&method, path, Some(&Version::new(3, 0, 1))) .unwrap_err(); diff --git a/dropshot/tests/test_openapi_v1.0.0.json b/dropshot/tests/test_openapi_v1.0.0.json index c88ae8d2e..524fd7f62 100644 --- a/dropshot/tests/test_openapi_v1.0.0.json +++ b/dropshot/tests/test_openapi_v1.0.0.json @@ -6,27 +6,6 @@ }, "paths": { "/demo": { - "get": { - "operationId": "handler1", - "responses": { - "200": { - "description": "successful operation", - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/EarlyReturn" - } - } - } - }, - "4XX": { - "$ref": "#/components/responses/Error" - }, - "5XX": { - "$ref": "#/components/responses/Error" - } - } - }, "put": { "operationId": "handler4", "responses": { @@ -53,17 +32,6 @@ }, "components": { "schemas": { - "EarlyReturn": { - "type": "object", - "properties": { - "real_message": { - "type": "string" - } - }, - "required": [ - "real_message" - ] - }, "Error": { "description": "Error information from a response.", "type": "object", diff --git a/dropshot/tests/test_openapi_v1.3.1.json b/dropshot/tests/test_openapi_v1.3.1.json index a83df0348..31111ea47 100644 --- a/dropshot/tests/test_openapi_v1.3.1.json +++ b/dropshot/tests/test_openapi_v1.3.1.json @@ -6,6 +6,28 @@ }, "paths": { "/demo": { + "get": { + "operationId": "handler3", + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "title": "String", + "type": "string" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, "put": { "operationId": "handler4", "responses": { diff --git a/dropshot/tests/test_versions.rs b/dropshot/tests/test_versions.rs index 18af6d019..c29e4c59c 100644 --- a/dropshot/tests/test_versions.rs +++ b/dropshot/tests/test_versions.rs @@ -68,7 +68,7 @@ async fn handler1( #[endpoint { method = GET, path = "/demo", - versions = "1.1.0".."1.3.0", + versions = "1.1.0".."1.2.0", }] async fn handler2( _rqctx: RequestContext<()>, @@ -79,7 +79,7 @@ async fn handler2( #[endpoint { method = GET, path = "/demo", - versions = "1.4.0".., + versions = "1.3.0".., }] async fn handler3( _rqctx: RequestContext<()>, @@ -191,7 +191,7 @@ async fn test_versions() { VERSION_HEADER_NAME ), ), - // Versions prior to and including 1.0.0 get "handler1". + // Versions prior to (not including) 1.0.0 get "handler1". TestCase::new( Method::GET, "/demo", @@ -199,21 +199,15 @@ async fn test_versions() { StatusCode::OK, HANDLER1_MSG, ), - TestCase::new( - Method::GET, - "/demo", - Some("1.0.0"), - StatusCode::OK, - HANDLER1_MSG, - ), - // Versions between 1.0.0 and 1.1.0 (non-inclusive) do not exist. + // Versions between 1.0.0 (inclusive) and 1.1.0 (exclusive) do not + // exist. // // Because there's a PUT handler for all versions, the expected error is // 405 ("Method Not Allowed"), not 404 ("Not Found"). TestCase::new( Method::GET, "/demo", - Some("1.0.1"), + Some("1.0.0"), StatusCode::METHOD_NOT_ALLOWED, StatusCode::METHOD_NOT_ALLOWED.canonical_reason().unwrap(), ), @@ -224,7 +218,8 @@ async fn test_versions() { StatusCode::METHOD_NOT_ALLOWED, StatusCode::METHOD_NOT_ALLOWED.canonical_reason().unwrap(), ), - // Versions between 1.1.0 and 1.3.0 (inclusive) get "handler2". + // Versions between 1.1.0 (inclusive) and 1.2.0 (exclusive) get + // "handler2". TestCase::new( Method::GET, "/demo", @@ -235,31 +230,31 @@ async fn test_versions() { TestCase::new( Method::GET, "/demo", - Some("1.3.0"), + Some("1.1.99"), StatusCode::OK, HANDLER2_MSG, ), - // Versions between 1.3.0 and 1.4.0 (exclusive) do not exist. - // See above for why this is a 405. + // Versions between 1.2.0 (inclusive) and 1.3.0 (exclusive) do not + // exist. See above for why this is a 405. TestCase::new( Method::GET, "/demo", - Some("1.3.1"), + Some("1.2.0"), StatusCode::METHOD_NOT_ALLOWED, StatusCode::METHOD_NOT_ALLOWED.canonical_reason().unwrap(), ), TestCase::new( Method::GET, "/demo", - Some("1.3.99"), + Some("1.2.99"), StatusCode::METHOD_NOT_ALLOWED, StatusCode::METHOD_NOT_ALLOWED.canonical_reason().unwrap(), ), - // Versions after 1.4.0 (inclusive) get "handler3". + // Versions after 1.3.0 (inclusive) get "handler3". TestCase::new( Method::GET, "/demo", - Some("1.4.0"), + Some("1.3.0"), StatusCode::OK, HANDLER3_MSG, ), @@ -370,7 +365,7 @@ fn test_versions_openapi_same_names() { #[endpoint { method = GET, path = "/demo", - versions = .."1.0.0" + versions = .."2.0.0" }] pub async fn the_operation( _rqctx: RequestContext<()>, @@ -420,7 +415,7 @@ fn test_versions_openapi_same_names() { #[endpoint { method = GET, path = "/demo", - versions = .."1.0.0", + versions = .."2.0.0", operation_id = "the_operation", }] async fn the_operation_v1( @@ -516,7 +511,7 @@ mod trait_based { #[endpoint { method = GET, path = "/demo", - versions = .."1.0.0", + versions = .."2.0.0", operation_id = "the_operation", }] async fn the_operation_v1( From 2a73353b2ac743e813237fe0ce90c04f438a79b2 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 3 Oct 2024 15:18:08 -0700 Subject: [PATCH 18/23] update versioning example for clarity --- dropshot/examples/versioning.rs | 112 +++++++++++++++++++------------- 1 file changed, 68 insertions(+), 44 deletions(-) diff --git a/dropshot/examples/versioning.rs b/dropshot/examples/versioning.rs index 1e36a5f09..940663f82 100644 --- a/dropshot/examples/versioning.rs +++ b/dropshot/examples/versioning.rs @@ -4,10 +4,10 @@ //! //! This example defines a bunch of API versions: //! -//! - Versions 1.0.0 through 1.0.3 contain an endpoint `GET /` that returns a -//! `Value` type with just one field: `s`. -//! - Versions 1.0.5 and later contain an endpoint `GET /` that returns a -//! `Value` type with two fields: `s` and `number`. +//! - Versions 1.x contain an endpoint `GET /` that returns a `Thing1` type with +//! just one field: `thing1_early`. +//! - Versions 2.x and later contain an endpoint `GET /` that returns a `Thing1` +//! type with one field: `thing1_late`. //! //! The client chooses which version they want to use by specifying the //! `dropshot-demo-version` header with their request. @@ -20,14 +20,16 @@ //! $ cargo run --example=versioning -- openapi 1.0.0 //! ``` //! -//! You'll see that the this spec contains one operation, it produces `Value`, -//! and that the `Value` type only has the one field `s`. +//! You'll see that the this spec contains one operation, it produces `Thing1`, +//! and that the `Thing1` type only has the one field `thing1_early`. It also +//! contains an operation that produces a `Thing2`. //! -//! You can generate the OpenAPI spec for 1.0.5 and see that the corresponding -//! `Value` type has the extra field `number`, as expected. +//! You can generate the OpenAPI spec for 2.0.0 and see that the corresponding +//! `Thing1` type has a different field, `thing1_late`, as expected. `Thing2` +//! is also present and unchanged. //! //! You can generate the OpenAPI spec for any other version. You'll see that -//! 0.9.0, for example, has no operations and no `Value` type. +//! 0.9.0, for example, has only the `Thing2` type and its associated getter. //! //! ## Running the server //! @@ -41,7 +43,7 @@ //! requests. If we don't specify a version, we get an error: //! //! ```text -//! $ curl http://127.0.0.1:12345 +//! $ curl http://127.0.0.1:12345/thing1 //! { //! "request_id": "73f62e8a-b363-488a-b662-662814e306ee", //! "message": "missing expected header \"dropshot-demo-version\"" @@ -54,18 +56,18 @@ //! If we provide a bogus one, we'll also get an error: //! //! ```text -//! $ curl -H 'dropshot-demo-version: threeve' http://127.0.0.1:12345 +//! $ curl -H 'dropshot-demo-version: threeve' http://127.0.0.1:12345/thing1 //! { //! "request_id": "18c1964e-88c6-4122-8287-1f2f399871bd", //! "message": "bad value for header \"dropshot-demo-version\": unexpected character 't' while parsing major version number: threeve" //! } //! ``` //! -//! If we provide version 0.9.0 (or 1.0.4), there is no endpoint at `/`, so we -//! get a 404: +//! If we provide version 0.9.0, there is no endpoint at `/thing1`, so we get a +//! 404: //! //! ```text -//! $ curl -i -H 'dropshot-demo-version: 0.9.0' http://127.0.0.1:12345 +//! $ curl -i -H 'dropshot-demo-version: 0.9.0' http://127.0.0.1:12345/thing1 //! HTTP/1.1 404 Not Found //! content-type: application/json //! x-request-id: 0d3d25b8-4c48-43b2-a417-018ebce68870 @@ -81,16 +83,16 @@ //! If we provide version 1.0.0, we get the v1 handler we defined: //! //! ```text -//! $ curl -H 'dropshot-demo-version: 1.0.0' http://127.0.0.1:12345 -//! {"s":"hello from an early v1"} +//! $ curl -H 'dropshot-demo-version: 1.0.0' http://127.0.0.1:12345/thing1 +//! {"thing1_early":"hello from an early v1"} //! ``` //! -//! If we provide version 1.0.5, we get the later version that we defined, with +//! If we provide version 2.0.0, we get the later version that we defined, with //! a different response body type: //! //! ```text -//! $ curl -H 'dropshot-demo-version: 1.0.5' http://127.0.0.1:12345 -//! {"s":"hello from a LATE v1","number":12} +//! $ curl -H 'dropshot-demo-version: 2.0.0' http://127.0.0.1:12345/thing1 +//! {"thing1_late":"hello from a LATE v1"} //! ``` use dropshot::endpoint; @@ -122,8 +124,9 @@ async fn main() -> Result<(), String> { .map_err(|error| format!("failed to create logger: {}", error))?; let mut api = ApiDescription::new(); - api.register(v1::versioned_get).unwrap(); - api.register(v2::versioned_get).unwrap(); + api.register(v1::get_thing1).unwrap(); + api.register(v2::get_thing1).unwrap(); + api.register(get_thing2).unwrap(); // Determine if we're generating the OpenAPI spec or starting the server. // Skip the first argument because that's just the name of the program. @@ -177,55 +180,76 @@ async fn main() -> Result<(), String> { // // This API defines several different versions: // -// - versions 1.0.0 through 1.0.3 use `v1::versioned_get` -// - versions 1.0.5 and later use `v2::versioned_get` -// - versions prior to 1.0.0 and version 1.0.4 do not exist +// - versions prior to 1.0.0 do not contain a `get_thing1` endpoint +// - versions 1.0.0 through 2.0.0 (exclusive) use `v1::get_thing1` +// - versions 2.0.0 and later use `v2::get_thing1` +// +// `get_thing2` appears in all versions. mod v1 { - // The contents of this module define endpoints and types used in v1.0.0 - // through v1.0.3. + // The contents of this module define endpoints and types used in all v1.x + // versions. use super::*; #[derive(Serialize, JsonSchema)] - struct Value { - s: &'static str, + struct Thing1 { + thing1_early: &'static str, } - /// Fetch the current value of the counter. + /// Fetch `thing1` #[endpoint { method = GET, - path = "/", - versions = "1.0.0".."1.0.3" + path = "/thing1", + versions = "1.0.0".."2.0.0" }] - pub async fn versioned_get( + pub async fn get_thing1( _rqctx: RequestContext<()>, - ) -> Result, HttpError> { - Ok(HttpResponseOk(Value { s: "hello from an early v1" })) + ) -> Result, HttpError> { + Ok(HttpResponseOk(Thing1 { thing1_early: "hello from an early v1" })) } } mod v2 { - // The contents of this module define endpoints and types used in v1.0.5 and + // The contents of this module define endpoints and types used in v2.x and // later. use super::*; #[derive(Serialize, JsonSchema)] - struct Value { - s: &'static str, - number: u32, + struct Thing1 { + thing1_late: &'static str, } - /// Fetch the current value of the counter. + /// Fetch `thing1` #[endpoint { method = GET, - path = "/", - versions = "1.0.5".. + path = "/thing1", + versions = "2.0.0".. }] - pub async fn versioned_get( + pub async fn get_thing1( _rqctx: RequestContext<()>, - ) -> Result, HttpError> { - Ok(HttpResponseOk(Value { s: "hello from a LATE v1", number: 12 })) + ) -> Result, HttpError> { + Ok(HttpResponseOk(Thing1 { thing1_late: "hello from a LATE v1" })) } } + +// The following are used in all API versions. The delta for each version is +// proportional to what actually changed in each version. i.e., you don't have +// to repeat the code for all the unchanged endpoints. + +#[derive(Serialize, JsonSchema)] +struct Thing2 { + thing2: &'static str, +} + +/// Fetch `thing2` +#[endpoint { + method = GET, + path = "/thing2", +}] +pub async fn get_thing2( + _rqctx: RequestContext<()>, +) -> Result, HttpError> { + Ok(HttpResponseOk(Thing2 { thing2: "hello from any version" })) +} From 9a86f2a26eb00209bd06d05fad05003fe5296964 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 7 Oct 2024 14:03:51 -0700 Subject: [PATCH 19/23] ClientSpecifiesVersionInHeader should accept a max version --- dropshot/examples/versioning.rs | 17 ++++++++++----- dropshot/src/versioning.rs | 38 ++++++++++++++++++++++++++++++--- dropshot/tests/test_versions.rs | 30 +++++++++----------------- 3 files changed, 57 insertions(+), 28 deletions(-) diff --git a/dropshot/examples/versioning.rs b/dropshot/examples/versioning.rs index 940663f82..61ce0473c 100644 --- a/dropshot/examples/versioning.rs +++ b/dropshot/examples/versioning.rs @@ -155,14 +155,21 @@ async fn main() -> Result<(), String> { // tells Dropshot how to determine what API version is being used for // each incoming request. // - // For this example, we will say that the client always specifies the - // version using the "dropshot-demo-version" header. You can provide - // your own impl of `DynamicVersionPolicy` that does this differently - // (e.g., filling in a default if the client doesn't provide one). + // For this example, we use `ClientSpecifiesVersionInHeader` to tell + // Dropshot that, as the name suggests, the client always specifies the + // version using the "dropshot-demo-version" header. We specify a max + // API version of "2.0.0". This is the "current" (latest) API that this + // example server is intended to support. + // + // You can provide your own impl of `DynamicVersionPolicy` that does + // this differently (e.g., filling in a default if the client doesn't + // provide one). See `DynamicVersionPolicy` for details. let header_name = "dropshot-demo-version" .parse::() .map_err(|_| String::from("demo header name was not valid"))?; - let version_impl = ClientSpecifiesVersionInHeader::new(header_name); + let max_version = semver::Version::new(2, 0, 0); + let version_impl = + ClientSpecifiesVersionInHeader::new(header_name, max_version); let version_policy = VersionPolicy::Dynamic(Box::new(version_impl)); let server = ServerBuilder::new(api, api_context, log) .config(config_dropshot) diff --git a/dropshot/src/versioning.rs b/dropshot/src/versioning.rs index d4b2c467b..96a6025db 100644 --- a/dropshot/src/versioning.rs +++ b/dropshot/src/versioning.rs @@ -117,14 +117,38 @@ pub trait DynamicVersionPolicy: std::fmt::Debug + Send + Sync { /// Implementation of `DynamicVersionPolicy` where the client must specify a /// specific semver in a specific header and we always use whatever they /// requested +/// +/// An incoming request will be rejected with a 400-level error if: +/// +/// - the header value cannot be parsed as a semver, or +/// - the requested version is newer than `max_version` (see +/// [`ClientSpecifiesVersionInHeader::new()`], which implies that the client +/// is trying to use a newer version of the API than this server supports. +/// +/// If you need anything more flexible (e.g., validating the provided version +/// against a fixed set of supported versions), you'll want to impl +/// `DynamicVersionPolicy` yourself. #[derive(Debug)] pub struct ClientSpecifiesVersionInHeader { name: HeaderName, + max_version: Version, } impl ClientSpecifiesVersionInHeader { - pub fn new(name: HeaderName) -> ClientSpecifiesVersionInHeader { - ClientSpecifiesVersionInHeader { name } + /// Make a new `ClientSpecifiesVersionInHeader` policy + /// + /// Arguments: + /// + /// * `name`: name of the header that the client will use to specify the + /// version + /// * `max_version`: the maximum version of the API that this server + /// supports. Requests for a version newer than this will be rejected + /// with a 400-level error. + pub fn new( + name: HeaderName, + max_version: Version, + ) -> ClientSpecifiesVersionInHeader { + ClientSpecifiesVersionInHeader { name, max_version } } } @@ -134,7 +158,15 @@ impl DynamicVersionPolicy for ClientSpecifiesVersionInHeader { request: &Request, _log: &Logger, ) -> Result { - parse_header(request.headers(), &self.name) + let v = parse_header(request.headers(), &self.name)?; + if v <= self.max_version { + Ok(v) + } else { + Err(HttpError::for_bad_request( + None, + format!("server does not support this API version: {}", v), + )) + } } } diff --git a/dropshot/tests/test_versions.rs b/dropshot/tests/test_versions.rs index c29e4c59c..006c60e25 100644 --- a/dropshot/tests/test_versions.rs +++ b/dropshot/tests/test_versions.rs @@ -26,7 +26,6 @@ const HANDLER1_MSG: &str = "handler1"; const HANDLER2_MSG: &str = "handler2"; const HANDLER3_MSG: &str = "handler3"; const HANDLER4_MSG: &str = "handler3"; -const FORBIDDEN_MSG: &str = "THAT VALUE IS FORBIDDEN!!!"; fn api() -> ApiDescription<()> { let mut api = ApiDescription::new(); @@ -98,23 +97,6 @@ async fn handler4( Ok(HttpResponseOk(HANDLER4_MSG)) } -#[derive(Debug)] -struct TestVersionPolicy(ClientSpecifiesVersionInHeader); -impl dropshot::DynamicVersionPolicy for TestVersionPolicy { - fn request_extract_version( - &self, - request: &http::Request, - log: &slog::Logger, - ) -> Result { - let v = self.0.request_extract_version(request, log)?; - if v.major == 4 && v.minor == 3 && v.patch == 2 { - Err(HttpError::for_bad_request(None, String::from(FORBIDDEN_MSG))) - } else { - Ok(v) - } - } -} - /// Define an API with different versions and run through an exhaustive battery /// of tests showing that we use the correct handler for each incoming request /// based on the version requested. @@ -122,11 +104,12 @@ impl dropshot::DynamicVersionPolicy for TestVersionPolicy { async fn test_versions() { let logctx = common::create_log_context("test_versions"); let server = ServerBuilder::new(api(), (), logctx.log.clone()) - .version_policy(VersionPolicy::Dynamic(Box::new(TestVersionPolicy( + .version_policy(VersionPolicy::Dynamic(Box::new( ClientSpecifiesVersionInHeader::new( VERSION_HEADER_NAME.parse().unwrap(), + Version::new(1, 4, 0), ), - )))) + ))) .start() .unwrap(); @@ -294,6 +277,13 @@ async fn test_versions() { StatusCode::OK, HANDLER4_MSG, ), + TestCase::new( + Method::PUT, + "/demo", + Some("1.5.0"), + StatusCode::BAD_REQUEST, + "server does not support this API version: 1.5.0", + ), ]; for t in test_cases { From e747169e4f2b834c0a03cd7411e38cf9fad64b38 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 29 Oct 2024 12:57:55 -0700 Subject: [PATCH 20/23] review feedback (rename test output files) --- dropshot/tests/test_versions.rs | 2 +- .../{test_openapi_v0.9.0.json => test_versions_v0.9.0.json} | 0 .../{test_openapi_v1.0.0.json => test_versions_v1.0.0.json} | 0 .../{test_openapi_v1.1.0.json => test_versions_v1.1.0.json} | 0 .../{test_openapi_v1.3.1.json => test_versions_v1.3.1.json} | 0 .../{test_openapi_v1.4.0.json => test_versions_v1.4.0.json} | 0 6 files changed, 1 insertion(+), 1 deletion(-) rename dropshot/tests/{test_openapi_v0.9.0.json => test_versions_v0.9.0.json} (100%) rename dropshot/tests/{test_openapi_v1.0.0.json => test_versions_v1.0.0.json} (100%) rename dropshot/tests/{test_openapi_v1.1.0.json => test_versions_v1.1.0.json} (100%) rename dropshot/tests/{test_openapi_v1.3.1.json => test_versions_v1.3.1.json} (100%) rename dropshot/tests/{test_openapi_v1.4.0.json => test_versions_v1.4.0.json} (100%) diff --git a/dropshot/tests/test_versions.rs b/dropshot/tests/test_versions.rs index 006c60e25..6b03af4d5 100644 --- a/dropshot/tests/test_versions.rs +++ b/dropshot/tests/test_versions.rs @@ -330,7 +330,7 @@ fn test_versions_openapi() { api.openapi("Evolving API", semver).write(&mut found).unwrap(); let actual = std::str::from_utf8(found.get_ref()).unwrap(); expectorate::assert_contents( - &format!("tests/test_openapi_v{}.json", version), + &format!("tests/test_versions_v{}.json", version), actual, ); } diff --git a/dropshot/tests/test_openapi_v0.9.0.json b/dropshot/tests/test_versions_v0.9.0.json similarity index 100% rename from dropshot/tests/test_openapi_v0.9.0.json rename to dropshot/tests/test_versions_v0.9.0.json diff --git a/dropshot/tests/test_openapi_v1.0.0.json b/dropshot/tests/test_versions_v1.0.0.json similarity index 100% rename from dropshot/tests/test_openapi_v1.0.0.json rename to dropshot/tests/test_versions_v1.0.0.json diff --git a/dropshot/tests/test_openapi_v1.1.0.json b/dropshot/tests/test_versions_v1.1.0.json similarity index 100% rename from dropshot/tests/test_openapi_v1.1.0.json rename to dropshot/tests/test_versions_v1.1.0.json diff --git a/dropshot/tests/test_openapi_v1.3.1.json b/dropshot/tests/test_versions_v1.3.1.json similarity index 100% rename from dropshot/tests/test_openapi_v1.3.1.json rename to dropshot/tests/test_versions_v1.3.1.json diff --git a/dropshot/tests/test_openapi_v1.4.0.json b/dropshot/tests/test_versions_v1.4.0.json similarity index 100% rename from dropshot/tests/test_openapi_v1.4.0.json rename to dropshot/tests/test_versions_v1.4.0.json From 2843d1320f2f312799893c5c393967ae7dbcba7e Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 30 Oct 2024 11:55:52 -0700 Subject: [PATCH 21/23] review feedback: update error message --- dropshot/tests/fail/bad_version_backwards.stderr | 2 +- dropshot_endpoint/src/endpoint.rs | 4 ++-- dropshot_endpoint/src/metadata.rs | 7 ++++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/dropshot/tests/fail/bad_version_backwards.stderr b/dropshot/tests/fail/bad_version_backwards.stderr index 2a5b86ab1..f00435387 100644 --- a/dropshot/tests/fail/bad_version_backwards.stderr +++ b/dropshot/tests/fail/bad_version_backwards.stderr @@ -1,4 +1,4 @@ -error: semver range (from ... until ...) has the endpoints out of order +error: "from" version (1.2.3) must be earlier than "until" version (1.2.2) --> tests/fail/bad_version_backwards.rs:17:16 | 17 | versions = "1.2.3".."1.2.2" diff --git a/dropshot_endpoint/src/endpoint.rs b/dropshot_endpoint/src/endpoint.rs index 5b4767808..b9ea7d069 100644 --- a/dropshot_endpoint/src/endpoint.rs +++ b/dropshot_endpoint/src/endpoint.rs @@ -1169,8 +1169,8 @@ mod tests { assert_eq!( errors.get(0).map(ToString::to_string), Some( - "semver range (from ... until ...) has the endpoints \ - out of order" + "\"from\" version (1.2.5) must be earlier \ + than \"until\" version (1.2.3)" .to_string() ), ); diff --git a/dropshot_endpoint/src/metadata.rs b/dropshot_endpoint/src/metadata.rs index e3d3a4843..c96092021 100644 --- a/dropshot_endpoint/src/metadata.rs +++ b/dropshot_endpoint/src/metadata.rs @@ -352,9 +352,10 @@ impl syn::parse::Parse for VersionRange { .collect(); return Err(syn::Error::new_spanned( span, - String::from( - "semver range (from ... until ...) has the \ - endpoints out of order", + format!( + "\"from\" version ({}) must be earlier than \ + \"until\" version ({})", + earliest_semver, latest_semver, ), )); } From 93e02c12d52e1af7f01bb4d4726314d33b70c648 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 12 Nov 2024 16:19:43 -0800 Subject: [PATCH 22/23] update CHANGELOG to reflect being somewhat experimental --- CHANGELOG.adoc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 36be9ad1e..d5f3182cb 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -63,7 +63,9 @@ starter.start() + We'd like to remove the `HttpServerStarter` altogether, so let us know if you're still using it for some reason. -* https://github.com/oxidecomputer/dropshot/pull/1115[#1115] Dropshot now supports hosting multiple versions of an API at a single server and routing to the correct version based on the incoming request. See documentation for details. If you don't care about this, you can mostly ignore it, but see "Breaking Changes" below. +* https://github.com/oxidecomputer/dropshot/pull/1115[#1115] Dropshot now includes **experimental** support for hosting multiple versions of an API at a single server and routing to the correct version based on the incoming request. See documentation for details. If you don't care about this, you can mostly ignore it, but see "Breaking Changes" below. ++ +By "experimental" we only mean that the API may change in upcoming releases. === Breaking Changes From 8a949a767c47955becbc40f4e7190c56f6c2f9b5 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 12 Nov 2024 16:24:10 -0800 Subject: [PATCH 23/23] mark experimental (cont) --- dropshot/src/server.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index bb077ed93..c47e8a8f8 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -1154,6 +1154,9 @@ impl ServerBuilder { /// Specifies whether and how this server determines the API version to use /// for incoming requests + /// + /// All the interfaces related to [`VersionPolicy`] are considered + /// experimental and may change in an upcoming release. pub fn version_policy(mut self, version_policy: VersionPolicy) -> Self { self.version_policy = version_policy; self