Skip to content

Commit

Permalink
The type checking for parameters could be much more stringent and more
Browse files Browse the repository at this point in the history 
informative. We'd ideally like to raise issues at compile time, but we
don't actually have the data (e.g. in proc macro context) we'd need to
identify type errors. We definitely do not want to fail after the server
has been running when a particular combination of paramters fails to
deserialize into the given structs. As a middle ground, we fail as the
ApiDescription is built up, before the server is servicing requests.
  • Loading branch information
@ahl
ahl committed Jul 20, 2021
1 parent 15acd1a commit 3902899
Show file tree
Hide file tree
Showing 5 changed files with 294 additions and 153 deletions.
145 changes: 140 additions & 5 deletions dropshot/src/api_description.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -232,6 +235,23 @@ impl<Context: ServerContext> ApiDescription<Context> {
{
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<Context>,
) -> 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)
Expand Down Expand Up @@ -262,7 +282,7 @@ impl<Context: ServerContext> ApiDescription<Context> {
let vv =
v.iter().map(|s| s.as_str()).collect::<Vec<&str>>().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,
Expand All @@ -278,9 +298,19 @@ impl<Context: ServerContext> ApiDescription<Context> {
"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<Context>,
) -> Result<(), String> {
// Explicitly disallow any attempt to consume the body twice.
let nbodyextractors = e
.parameters
Expand All @@ -298,7 +328,81 @@ impl<Context: ServerContext> ApiDescription<Context> {
));
}

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<Context>,
) -> 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::<BTreeMap<_, _>>();

for param in &e.parameters {
/* Skip anything that's not a path or query parameter (i.e. body) */
match &param.metadata {
ApiEndpointParameterMetadata::Path(_)
| ApiEndpointParameterMetadata::Query(_) => (),
_ => continue,
}
/* Only body parameters should have unresolved schemas */
let (schema, dependencies) = match &param.schema {
ApiSchemaGenerator::Static {
schema,
dependencies,
} => (schema, dependencies),
_ => unreachable!(),
};

match &param.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(())
}
Expand Down Expand Up @@ -374,8 +478,6 @@ impl<Context: ServerContext> ApiDescription<Context> {
* 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();

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<RequestContext<()>>,
_: Query<TheThing>,
_: Path<TheThing>,
) -> Result<Response<Body>, 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)]
Expand Down
151 changes: 4 additions & 147 deletions dropshot/src/handler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -755,8 +755,8 @@ fn schema2parameters(
) -> Vec<ApiEndpointParameter> {
/*
* 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. */
Expand All @@ -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(_)),
Expand All @@ -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-
Expand Down Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions dropshot/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,7 @@ mod logging;
mod pagination;
mod router;
mod server;
mod type_util;

pub mod test_util;

Expand Down
Loading

0 comments on commit 3902899

Please sign in to comment.