diff --git a/dropshot/src/api_description.rs b/dropshot/src/api_description.rs index 855c222dc..b2b169baf 100644 --- a/dropshot/src/api_description.rs +++ b/dropshot/src/api_description.rs @@ -11,12 +11,15 @@ use crate::router::route_path_to_segments; use crate::router::HttpRouter; use crate::router::PathSegment; use crate::server::ServerContext; +use crate::type_util::type_is_scalar; +use crate::type_util::type_is_string_enum; use crate::Extractor; use crate::CONTENT_TYPE_JSON; use crate::CONTENT_TYPE_OCTET_STREAM; use http::Method; use http::StatusCode; +use std::collections::BTreeMap; use std::collections::HashSet; /** @@ -232,6 +235,23 @@ impl ApiDescription { { let e = endpoint.into(); + self.validate_path_parameters(&e)?; + self.validate_body_parameters(&e)?; + self.validate_named_parameters(&e)?; + + self.router.insert(e); + + Ok(()) + } + + /** + * Validate that the parameters specified in the path match the parameters + * specified by the path parameter arguments to the handler function. + */ + fn validate_path_parameters( + &self, + e: &ApiEndpoint, + ) -> Result<(), String> { // Gather up the path parameters and the path variable components, and // make sure they're identical. let path = route_path_to_segments(&e.path) @@ -262,7 +282,7 @@ impl ApiDescription { let vv = v.iter().map(|s| s.as_str()).collect::>().join(","); - return match (p.is_empty(), v.is_empty()) { + match (p.is_empty(), v.is_empty()) { (false, true) => Err(format!( "{} ({})", "path parameters are not consumed", pp, @@ -278,9 +298,19 @@ impl ApiDescription { "specified parameters do not appear in the path", vv, )), - }; + }?; } + Ok(()) + } + + /** + * Validate that we have a single body parameter. + */ + fn validate_body_parameters( + &self, + e: &ApiEndpoint, + ) -> Result<(), String> { // Explicitly disallow any attempt to consume the body twice. let nbodyextractors = e .parameters @@ -298,7 +328,81 @@ impl ApiDescription { )); } - self.router.insert(e); + Ok(()) + } + + /** + * Validate that named parameters have appropriate types and their aren't + * duplicates. Parameters must have scalar types except in the case of the + * received for a wildcard path which must be an array of String. + */ + fn validate_named_parameters( + &self, + e: &ApiEndpoint, + ) -> Result<(), String> { + enum SegmentOrWildcard { + Segment, + Wildcard, + } + let path_segments = route_path_to_segments(&e.path) + .iter() + .filter_map(|segment| { + let seg = PathSegment::from(segment); + match seg { + PathSegment::VarnameSegment(v) => { + Some((v, SegmentOrWildcard::Segment)) + } + PathSegment::VarnameWildcard(v) => { + Some((v, SegmentOrWildcard::Wildcard)) + } + PathSegment::Literal(_) => None, + } + }) + .collect::>(); + + for param in &e.parameters { + /* Skip anything that's not a path or query parameter (i.e. body) */ + match ¶m.metadata { + ApiEndpointParameterMetadata::Path(_) + | ApiEndpointParameterMetadata::Query(_) => (), + _ => continue, + } + /* Only body parameters should have unresolved schemas */ + let (schema, dependencies) = match ¶m.schema { + ApiSchemaGenerator::Static { + schema, + dependencies, + } => (schema, dependencies), + _ => unreachable!(), + }; + + match ¶m.metadata { + ApiEndpointParameterMetadata::Path(ref name) => { + match path_segments.get(name) { + Some(SegmentOrWildcard::Segment) => { + type_is_scalar(name, schema, dependencies)?; + } + Some(SegmentOrWildcard::Wildcard) => { + type_is_string_enum(name, schema, dependencies)?; + } + None => { + panic!("all path variables should be accounted for") + } + } + } + ApiEndpointParameterMetadata::Query(ref name) => { + if path_segments.get(name).is_some() { + return Err(format!( + "the parameter '{}' is specified for both query \ + and path parameters", + name + )); + } + type_is_scalar(name, schema, dependencies)?; + } + _ => (), + } + } Ok(()) } @@ -374,8 +478,6 @@ impl ApiDescription { * Internal routine for constructing the OpenAPI definition describing this * API in its JSON form. */ - // TODO: There's a bunch of error handling we need here such as checking - // for duplicate parameter names. fn gen_openapi(&self, info: openapiv3::Info) -> openapiv3::OpenAPI { let mut openapi = openapiv3::OpenAPI::default(); @@ -1181,6 +1283,7 @@ mod test { use super::ApiEndpoint; use crate as dropshot; /* for "endpoint" macro */ use crate::endpoint; + use crate::Query; use crate::TypedBody; use crate::UntypedBody; use http::Method; @@ -1349,6 +1452,38 @@ mod test { ); } + #[test] + fn test_dup_names() { + #[derive(Deserialize, JsonSchema)] + struct AStruct {} + + #[allow(dead_code)] + #[derive(Deserialize, JsonSchema)] + struct TheThing { + thing: String, + } + + #[endpoint { + method = PUT, + path = "/testing/{thing}" + }] + async fn test_dup_names_handler( + _: Arc>, + _: Query, + _: Path, + ) -> Result, HttpError> { + unimplemented!(); + } + + let mut api = ApiDescription::new(); + let error = api.register(test_dup_names_handler).unwrap_err(); + assert_eq!( + error, + "the parameter 'thing' is specified for both query and path \ + parameters", + ); + } + #[test] fn test_additional_properties() { #[allow(dead_code)] diff --git a/dropshot/src/handler.rs b/dropshot/src/handler.rs index 1e36c7866..511ae44ef 100644 --- a/dropshot/src/handler.rs +++ b/dropshot/src/handler.rs @@ -755,8 +755,8 @@ fn schema2parameters( ) -> Vec { /* * We ignore schema.metadata, which includes things like doc comments, and - * schema.extensions. We call these out explicitly rather than .. since we - * match all other fields in the structure. + * schema.extensions. We call these out explicitly rather than eliding them + * as .. since we match all other fields in the structure. */ match schema { /* We expect references to be on their own. */ @@ -779,7 +779,8 @@ fn schema2parameters( generator, required, ), - // Match objects and subschemas. + + /* Match objects and subschemas. */ schemars::schema::Schema::Object(schemars::schema::SchemaObject { metadata: _, instance_type: Some(schemars::schema::SingleOrVec::Single(_)), @@ -801,8 +802,6 @@ fn schema2parameters( if let Some(object) = object { parameters.extend(object.properties.iter().map( |(name, schema)| { - validate_parameter_schema(loc, name, schema, generator); - // We won't often see referenced schemas here, but we may // in the case of enumerated strings. To handle this, we // package up the dependencies to include in the top- @@ -880,148 +879,6 @@ fn schema2parameters( } } -/** - * Confirm that the parameter schema type is either an atomic value (i.e. - * string, number, boolean) or an array of strings (to accommodate wild card - * paths). - */ -fn validate_parameter_schema( - loc: &ApiEndpointParameterLocation, - name: &String, - schema: &schemars::schema::Schema, - generator: &schemars::gen::SchemaGenerator, -) { - match schema { - /* - * We expect references to be on their own. - */ - schemars::schema::Schema::Object(schemars::schema::SchemaObject { - metadata: _, - instance_type: None, - format: None, - enum_values: None, - const_value: None, - subschemas: None, - number: None, - string: None, - array: None, - object: None, - reference: Some(_), - extensions: _, - }) => validate_parameter_schema( - loc, - name, - generator.dereference(schema).expect("invalid reference"), - generator, - ), - - /* - * Match atomic value types. - */ - schemars::schema::Schema::Object(schemars::schema::SchemaObject { - instance_type: - Some(schemars::schema::SingleOrVec::Single(instance_type)), - subschemas: None, - array: None, - object: None, - reference: None, - .. - }) if match instance_type.as_ref() { - InstanceType::Boolean - | InstanceType::Number - | InstanceType::String - | InstanceType::Integer => true, - _ => false, - } => - { /* All good. */ } - - /* - * For path parameters only, match array types and validate that their - * items are strings. - */ - schemars::schema::Schema::Object(schemars::schema::SchemaObject { - metadata: _, - instance_type: - Some(schemars::schema::SingleOrVec::Single(instance_type)), - format: None, - enum_values: None, - const_value: None, - subschemas: None, - number: None, - string: None, - array: Some(array_validation), - object: None, - reference: None, - extensions: _, - }) if matches!(loc, ApiEndpointParameterLocation::Path) - && matches!(instance_type.as_ref(), InstanceType::Array) => - { - match array_validation.as_ref() { - schemars::schema::ArrayValidation { - items: - Some(schemars::schema::SingleOrVec::Single(item_schema)), - additional_items: None, - .. - } => validate_string_schema(name, item_schema, generator), - _ => panic!("parameter {} has an invalid array type", name), - } - } - - _ => panic!("parameter {} has an invalid type", name), - } -} - -/** - * Validate that the given schema is for a simple string type. - */ -fn validate_string_schema( - name: &String, - schema: &schemars::schema::Schema, - generator: &schemars::gen::SchemaGenerator, -) { - match schema { - /* - * We expect references to be on their own. - */ - schemars::schema::Schema::Object(schemars::schema::SchemaObject { - metadata: _, - instance_type: None, - format: None, - enum_values: None, - const_value: None, - subschemas: None, - number: None, - string: None, - array: None, - object: None, - reference: Some(_), - extensions: _, - }) => validate_string_schema( - name, - generator.dereference(schema).expect("invalid reference"), - generator, - ), - - /* - * Match string value types. - */ - schemars::schema::Schema::Object(schemars::schema::SchemaObject { - instance_type: - Some(schemars::schema::SingleOrVec::Single(instance_type)), - subschemas: None, - array: None, - object: None, - reference: None, - .. - }) if matches!(instance_type.as_ref(), InstanceType::String) => { /* All good. */ - } - - _ => { - panic!("parameter {} is an array of a type other than string", name) - } - } -} - /* * TypedBody: body extractor for formats that can be deserialized to a specific * type. Only JSON is currently supported. diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index ad14f1daa..66c516e42 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -515,6 +515,7 @@ mod logging; mod pagination; mod router; mod server; +mod type_util; pub mod test_util; diff --git a/dropshot/src/type_util.rs b/dropshot/src/type_util.rs new file mode 100644 index 000000000..d25301927 --- /dev/null +++ b/dropshot/src/type_util.rs @@ -0,0 +1,148 @@ +// Copyright 2021 Oxide Computer Company + +/*! + * Utility functions for working with JsonSchema types. + */ + +use indexmap::IndexMap; +use schemars::schema::{InstanceType, Schema, SchemaObject, SingleOrVec}; + +/** + * Returns true iff the input schema is a boolean, floating-point number, + * string or integer. + */ +pub fn type_is_scalar( + name: &String, + schema: &Schema, + dependencies: &IndexMap, +) -> Result<(), String> { + type_is_scalar_common(name, schema, dependencies, |instance_type| { + matches!( + instance_type, + InstanceType::Boolean + | InstanceType::Number + | InstanceType::String + | InstanceType::Integer + ) + }) +} + +/** + * Returns true iff the input schema is a string. + */ +pub fn type_is_string( + name: &String, + schema: &Schema, + dependencies: &IndexMap, +) -> Result<(), String> { + type_is_scalar_common(name, schema, dependencies, |instance_type| { + matches!(instance_type, InstanceType::String) + }) +} + +/** + * Helper function for scalar types. + */ +fn type_is_scalar_common( + name: &String, + schema: &Schema, + dependencies: &IndexMap, + type_check: fn(&InstanceType) -> bool, +) -> Result<(), String> { + /* Make sure we're examining a type and not a reference */ + let schema = type_resolve(schema, dependencies); + + /* + * We're looking for types that have no subschemas, are not arrays, are not + * objects, are not references, and whose instance type matches the limited + * set of scalar types. + */ + match schema { + Schema::Object(SchemaObject { + instance_type: Some(SingleOrVec::Single(instance_type)), + subschemas: None, + array: None, + object: None, + reference: None, + .. + }) if type_check(instance_type.as_ref()) => Ok(()), + _ => Err(format!("the parameter '{}' must have a scalar type", name)), + } +} + +pub fn type_is_string_enum( + name: &String, + schema: &Schema, + dependencies: &IndexMap, +) -> Result<(), String> { + /* Make sure we're examining a type and not a reference */ + let schema = type_resolve(schema, dependencies); + + match schema { + Schema::Object(SchemaObject { + metadata: _, + instance_type: + Some(schemars::schema::SingleOrVec::Single(instance_type)), + format: None, + enum_values: None, + const_value: None, + subschemas: None, + number: None, + string: None, + array: Some(array_validation), + object: None, + reference: None, + extensions: _, + }) if matches!(instance_type.as_ref(), InstanceType::Array) => { + match array_validation.as_ref() { + schemars::schema::ArrayValidation { + items: + Some(schemars::schema::SingleOrVec::Single(item_schema)), + additional_items: None, + .. + } => type_is_string(name, item_schema, dependencies).map_err( + |_| { + format!( + "the parameter '{}' must be an array of strings", + name + ) + }, + ), + _ => { + panic!("the parameter '{}' has an invalid array type", name) + } + } + } + _ => { + Err(format!("the parameter '{}' must be an array of strings", name)) + } + } +} + +pub fn type_resolve<'a>( + mut schema: &'a Schema, + dependencies: &'a IndexMap, +) -> &'a Schema { + while let Schema::Object(SchemaObject { + metadata: _, + instance_type: None, + format: None, + enum_values: None, + const_value: None, + subschemas: None, + number: None, + string: None, + array: None, + object: None, + reference: Some(ref_schema), + extensions: _, + }) = schema + { + const PREFIX: &str = "#/components/schemas/"; + assert!(ref_schema.starts_with(PREFIX)); + schema = dependencies + .get(&ref_schema[PREFIX.len()..]) + .unwrap_or_else(|| panic!("invalid reference {}", ref_schema)); + } + schema +} diff --git a/dropshot/tests/test_openapi.rs b/dropshot/tests/test_openapi.rs index 2d2a2d70e..0a1a98b49 100644 --- a/dropshot/tests/test_openapi.rs +++ b/dropshot/tests/test_openapi.rs @@ -251,7 +251,7 @@ async fn handler13( #[allow(dead_code)] #[derive(JsonSchema, Deserialize)] struct AllPath { - path: String, + path: Vec, } #[endpoint {