From 5a07828ebd3c720725ae2a05b1b6e928fc9ad071 Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 13 Jan 2022 12:26:19 +0100 Subject: [PATCH] `aws-smithy-http-server`: implement basic URI pattern conflict disambiguation (#1036) This commit implements basic URI pattern conflict disambiguation when routing incoming requests to service operations. It does this by introducing a measure of how "important" a `RequestSpec` is. The more specific a `RequestSpec` is, the higher it ranks in importance. Specificity is measured by the number of segments plus the number of query string literals in its URI pattern, so `/{Bucket}/{Key}?query` is more specific than `/{Bucket}/{Key}`, which is more specific than `/{Bucket}`, which is more specific than `/`. Why do we need this? Note that: 1. the Smithy spec does not define how servers should route incoming requests in the case of pattern conflicts; and 2. the Smithy spec even outright rejects conflicting patterns that can be easily disambiguated e.g. `/{a}` and `/{label}/b` cannot coexist. We can't to anything about (2) since the Smithy CLI will refuse to build a model with those kind of conflicts. However, the Smithy CLI does allow _other_ conflicting patterns to coexist, e.g. `/` and `/{label}`. We therefore have to take a stance on (1), since if we route arbitrarily we render basic usage impossible (see issue #1009). So this ranking of routes implements some basic pattern conflict disambiguation with some common sense. It's also the same behavior that the TypeScript sSDK is implementing [0]. This commit also: * makes the `future` module private, * hides documentation for the `request_spec` module; and * makes some fields from some structs in the `request_spec` module private and adds constructors. Closes #1009. [1]: https://github.com/awslabs/smithy-typescript/blob/d263078b81485a6a2013d243639c0c680343ff47/smithy-typescript-ssdk-libs/server-common/src/httpbinding/mux.ts#L59. --- .../ServerOperationRegistryGenerator.kt | 29 ++-- .../aws-smithy-http-server/src/routing/mod.rs | 134 ++++++++++++++---- .../src/routing/request_spec.rs | 64 ++++++++- .../src/routing/route.rs | 4 + 4 files changed, 184 insertions(+), 47 deletions(-) diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerOperationRegistryGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerOperationRegistryGenerator.kt index f0defc964..5eface225 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerOperationRegistryGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerOperationRegistryGenerator.kt @@ -14,6 +14,7 @@ import software.amazon.smithy.rust.codegen.rustlang.rust import software.amazon.smithy.rust.codegen.rustlang.rustBlock import software.amazon.smithy.rust.codegen.rustlang.rustBlockTemplate import software.amazon.smithy.rust.codegen.rustlang.rustTemplate +import software.amazon.smithy.rust.codegen.server.smithy.ServerCargoDependency import software.amazon.smithy.rust.codegen.server.smithy.ServerRuntimeType import software.amazon.smithy.rust.codegen.smithy.CodegenContext import software.amazon.smithy.rust.codegen.smithy.RuntimeType @@ -29,9 +30,6 @@ class ServerOperationRegistryGenerator( private val httpBindingResolver: HttpBindingResolver, private val operations: List, ) { - private val serverCrate = "aws_smithy_http_server" - private val service = codegenContext.serviceShape - private val model = codegenContext.model private val symbolProvider = codegenContext.symbolProvider private val operationNames = operations.map { symbolProvider.toSymbol(it).name.toSnakeCase() } private val runtimeConfig = codegenContext.runtimeConfig @@ -39,6 +37,7 @@ class ServerOperationRegistryGenerator( "Router" to ServerRuntimeType.Router(runtimeConfig), "SmithyHttpServer" to CargoDependency.SmithyHttpServer(runtimeConfig).asType(), "ServerOperationHandler" to ServerRuntimeType.serverOperationHandler(runtimeConfig), + "Tower" to ServerCargoDependency.Tower.asType(), "Phantom" to ServerRuntimeType.Phantom, "StdError" to RuntimeType.StdError ) @@ -216,18 +215,17 @@ class ServerOperationRegistryGenerator( ) { rustBlock("fn from(registry: $operationRegistryNameWithArguments) -> Self") { val requestSpecsVarNames = operationNames.map { "${it}_request_spec" } - val routes = requestSpecsVarNames.zip(operationNames) { requestSpecVarName, operationName -> - ".route($requestSpecVarName, #{ServerOperationHandler}::operation(registry.$operationName))" - }.joinToString(separator = "\n") - val requestSpecs = requestSpecsVarNames.zip(operations) { requestSpecVarName, operation -> "let $requestSpecVarName = ${operation.requestSpec()};" }.joinToString(separator = "\n") + val towerServices = requestSpecsVarNames.zip(operationNames) { requestSpecVarName, operationName -> + "(#{Tower}::util::BoxCloneService::new(#{ServerOperationHandler}::operation(registry.$operationName)), $requestSpecVarName)" + }.joinToString(prefix = "vec![", separator = ",\n", postfix = "]") + rustTemplate( """ $requestSpecs - #{Router}::new() - $routes + #{Router}::from_box_clone_service_iter($towerServices) """.trimIndent(), *codegenScope ) @@ -267,13 +265,12 @@ class ServerOperationRegistryGenerator( return """ $namespace::RequestSpec::new( http::Method::${httpTrait.method}, - $namespace::UriSpec { - host_prefix: None, - path_and_query: $namespace::PathAndQuerySpec { - path_segments: $namespace::PathSpec::from_vector_unchecked(vec![${pathSegments.joinToString()}]), - query_segments: $namespace::QuerySpec::from_vector_unchecked(vec![${querySegments.joinToString()}]) - } - } + $namespace::UriSpec::new( + $namespace::PathAndQuerySpec::new( + $namespace::PathSpec::from_vector_unchecked(vec![${pathSegments.joinToString()}]), + $namespace::QuerySpec::from_vector_unchecked(vec![${querySegments.joinToString()}]) + ) + ) ) """.trimIndent() } diff --git a/rust-runtime/aws-smithy-http-server/src/routing/mod.rs b/rust-runtime/aws-smithy-http-server/src/routing/mod.rs index 5f80442be..f929556e0 100644 --- a/rust-runtime/aws-smithy-http-server/src/routing/mod.rs +++ b/rust-runtime/aws-smithy-http-server/src/routing/mod.rs @@ -20,9 +20,12 @@ use tower::util::ServiceExt; use tower::{Service, ServiceBuilder}; use tower_http::map_response_body::MapResponseBodyLayer; -pub mod future; +mod future; mod into_make_service; + +#[doc(hidden)] pub mod request_spec; + mod route; pub use self::{into_make_service::IntoMakeService, route::Route}; @@ -54,7 +57,9 @@ where B: Send + 'static, { fn default() -> Self { - Self::new() + Self { + routes: Default::default(), + } } } @@ -62,26 +67,30 @@ impl Router where B: Send + 'static, { - /// Create a new `Router`. + /// Create a new `Router` from a vector of pairs of request specs and services. /// - /// Unless you add additional routes this will respond to `404 Not Found` to - /// all requests. - #[doc(hidden)] - pub fn new() -> Self { - Self { - routes: Default::default(), - } - } - - /// Add a route to the router. + /// If the vector is empty the router will respond `404 Not Found` to all requests. #[doc(hidden)] - pub fn route(mut self, request_spec: RequestSpec, svc: T) -> Self + pub fn from_box_clone_service_iter(routes: T) -> Self where - T: Service, Response = Response, Error = Infallible> + Clone + Send + 'static, - T::Future: Send + 'static, + T: IntoIterator< + Item = ( + tower::util::BoxCloneService, Response, Infallible>, + RequestSpec, + ), + >, { - self.routes.push((Route::new(svc), request_spec)); - self + let mut routes: Vec<(Route, RequestSpec)> = routes + .into_iter() + .map(|(svc, request_spec)| (Route::from_box_clone_service(svc), request_spec)) + .collect(); + + // Sort them once by specifity, with the more specific routes sorted before the less + // specific ones, so that when routing a request we can simply iterate through the routes + // and pick the first one that matches. + routes.sort_by_key(|(_route, request_spec)| std::cmp::Reverse(request_spec.rank())); + + Self { routes } } /// Convert this router into a [`MakeService`], that is a [`Service`] whose @@ -225,7 +234,7 @@ mod tests { PathSegment::Label, PathSegment::Label, ], - vec![], + Vec::new(), ), "A", ), @@ -237,14 +246,14 @@ mod tests { PathSegment::Greedy, PathSegment::Literal(String::from("z")), ], - vec![], + Vec::new(), ), "MiddleGreedy", ), ( RequestSpec::from_parts( Method::DELETE, - vec![], + Vec::new(), vec![ QuerySegment::KeyValue(String::from("foo"), String::from("bar")), QuerySegment::Key(String::from("baz")), @@ -262,11 +271,12 @@ mod tests { ), ]; - let mut router = Router::new(); - for (spec, svc_name) in request_specs { - let svc = NamedEchoUriService(String::from(svc_name)); - router = router.route(spec, svc.clone()); - } + let mut router = Router::from_box_clone_service_iter(request_specs.into_iter().map(|(spec, svc_name)| { + ( + tower::util::BoxCloneService::new(NamedEchoUriService(String::from(svc_name))), + spec, + ) + })); let hits = vec![ ("A", Method::GET, "/a/b/c"), @@ -312,4 +322,76 @@ mod tests { assert_eq!(StatusCode::NOT_FOUND, res.status()); } } + + #[tokio::test] + async fn basic_pattern_conflict_avoidance() { + let request_specs: Vec<(RequestSpec, &str)> = vec![ + ( + RequestSpec::from_parts( + Method::GET, + vec![PathSegment::Literal(String::from("a")), PathSegment::Label], + Vec::new(), + ), + "A1", + ), + ( + RequestSpec::from_parts( + Method::GET, + vec![ + PathSegment::Literal(String::from("a")), + PathSegment::Label, + PathSegment::Literal(String::from("a")), + ], + Vec::new(), + ), + "A2", + ), + ( + RequestSpec::from_parts( + Method::GET, + vec![PathSegment::Literal(String::from("b")), PathSegment::Greedy], + Vec::new(), + ), + "B1", + ), + ( + RequestSpec::from_parts( + Method::GET, + vec![PathSegment::Literal(String::from("b")), PathSegment::Greedy], + vec![QuerySegment::Key(String::from("q"))], + ), + "B2", + ), + ( + RequestSpec::from_parts(Method::GET, Vec::new(), Vec::new()), + "ListBuckets", + ), + ( + RequestSpec::from_parts(Method::GET, vec![PathSegment::Label], Vec::new()), + "ListObjects", + ), + ]; + + let mut router = Router::from_box_clone_service_iter(request_specs.into_iter().map(|(spec, svc_name)| { + ( + tower::util::BoxCloneService::new(NamedEchoUriService(String::from(svc_name))), + spec, + ) + })); + + let hits = vec![ + ("A1", Method::GET, "/a/foo"), + ("A2", Method::GET, "/a/foo/a"), + ("B1", Method::GET, "/b/foo/bar/baz"), + ("B2", Method::GET, "/b/foo?q=baz"), + ("ListBuckets", Method::GET, "/"), + ("ListObjects", Method::GET, "/bucket"), + ]; + for (svc_name, method, uri) in &hits { + let mut res = router.call(req(method, uri)).await.unwrap(); + let actual_body = get_body_as_str(&mut res).await; + + assert_eq!(format!("{} :: {}", svc_name, uri), actual_body); + } + } } diff --git a/rust-runtime/aws-smithy-http-server/src/routing/request_spec.rs b/rust-runtime/aws-smithy-http-server/src/routing/request_spec.rs index 9e60740ae..1f53292cf 100644 --- a/rust-runtime/aws-smithy-http-server/src/routing/request_spec.rs +++ b/rust-runtime/aws-smithy-http-server/src/routing/request_spec.rs @@ -45,14 +45,35 @@ impl QuerySpec { #[derive(Debug, Clone, Default)] pub struct PathAndQuerySpec { - pub path_segments: PathSpec, - pub query_segments: QuerySpec, + path_segments: PathSpec, + query_segments: QuerySpec, +} + +impl PathAndQuerySpec { + pub fn new(path_segments: PathSpec, query_segments: QuerySpec) -> Self { + PathAndQuerySpec { + path_segments, + query_segments, + } + } } #[derive(Debug, Clone)] pub struct UriSpec { - pub host_prefix: Option>, - pub path_and_query: PathAndQuerySpec, + host_prefix: Option>, + path_and_query: PathAndQuerySpec, +} + +impl UriSpec { + // TODO When we add support for the endpoint trait, this constructor will take in + // a first argument `host_prefix`. + // https://awslabs.github.io/smithy/1.0/spec/core/endpoint-traits.html#endpoint-trait + pub fn new(path_and_query: PathAndQuerySpec) -> Self { + UriSpec { + host_prefix: None, + path_and_query, + } + } } #[derive(Debug, Clone)] @@ -63,7 +84,7 @@ pub struct RequestSpec { } #[derive(Debug, PartialEq)] -pub enum Match { +pub(super) enum Match { /// The request matches the URI pattern spec. Yes, /// The request matches the URI pattern spec, but the wrong HTTP method was used. `405 Method @@ -107,6 +128,39 @@ impl RequestSpec { } } + /// A measure of how "important" a `RequestSpec` is. The more specific a `RequestSpec` is, the + /// higher it ranks in importance. Specificity is measured by the number of segments plus the + /// number of query string literals in its URI pattern, so `/{Bucket}/{Key}?query` is more + /// specific than `/{Bucket}/{Key}`, which is more specific than `/{Bucket}`, which is more + /// specific than `/`. + /// + /// This rank effectively induces a total order, but we don't implement as `Ord` for + /// `RequestSpec` because it would appear in its public interface. + /// + /// # Why do we need this? + /// + /// Note that: + /// 1. the Smithy spec does not define how servers should route incoming requests in the + /// case of pattern conflicts; and + /// 2. the Smithy spec even outright rejects conflicting patterns that can be easily + /// disambiguated e.g. `/{a}` and `/{label}/b` cannot coexist. + /// + /// We can't to anything about (2) since the Smithy CLI will refuse to build a model with those + /// kind of conflicts. However, the Smithy CLI does allow _other_ conflicting patterns to + /// coexist, e.g. `/` and `/{label}`. We therefore have to take a stance on (1), since if we + /// route arbitrarily [we render basic usage + /// impossible](https://github.com/awslabs/smithy-rs/issues/1009). + /// So this ranking of routes implements some basic pattern conflict disambiguation with some + /// common sense. It's also the same behavior that [the TypeScript sSDK is implementing]. + /// + /// TODO(https://github.com/awslabs/smithy/issues/1029#issuecomment-1002683552): Once Smithy + /// updates the spec to define the behavior, update our implementation. + /// + /// [the TypeScript sSDK is implementing]: https://github.com/awslabs/smithy-typescript/blob/d263078b81485a6a2013d243639c0c680343ff47/smithy-typescript-ssdk-libs/server-common/src/httpbinding/mux.ts#L59. + pub(super) fn rank(&self) -> usize { + self.uri_spec.path_and_query.path_segments.0.len() + self.uri_spec.path_and_query.query_segments.0.len() + } + pub(super) fn matches(&self, req: &Request) -> Match { if let Some(_host_prefix) = &self.uri_spec.host_prefix { todo!("Look at host prefix"); diff --git a/rust-runtime/aws-smithy-http-server/src/routing/route.rs b/rust-runtime/aws-smithy-http-server/src/routing/route.rs index 2b6fe3227..5915a1d83 100644 --- a/rust-runtime/aws-smithy-http-server/src/routing/route.rs +++ b/rust-runtime/aws-smithy-http-server/src/routing/route.rs @@ -61,6 +61,10 @@ impl Route { service: BoxCloneService::new(svc), } } + + pub(super) fn from_box_clone_service(svc: BoxCloneService, Response, Infallible>) -> Self { + Self { service: svc } + } } impl Clone for Route { -- GitLab