From f3d205a7fba6f2ef886200ca7af457d6dec50dbe Mon Sep 17 00:00:00 2001 From: Harry Barber Date: Thu, 2 Feb 2023 11:23:07 +0000 Subject: [PATCH 1/4] Do not alter Operation shape ID --- rust-runtime/aws-smithy-http-server/src/extension.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/rust-runtime/aws-smithy-http-server/src/extension.rs b/rust-runtime/aws-smithy-http-server/src/extension.rs index 33baa890b1..e7a0042052 100644 --- a/rust-runtime/aws-smithy-http-server/src/extension.rs +++ b/rust-runtime/aws-smithy-http-server/src/extension.rs @@ -35,8 +35,6 @@ pub use crate::request::extension::{Extension, MissingExtension}; /// This extension type is inserted, via the [`OperationExtensionPlugin`], whenever it has been correctly determined /// that the request should be routed to a particular operation. The operation handler might not even get invoked /// because the request fails to deserialize into the modeled operation input. -/// -/// The format given must be the absolute shape ID with `#` replaced with a `.`. #[derive(Debug, Clone)] pub struct OperationExtension { absolute: &'static str, @@ -49,16 +47,16 @@ pub struct OperationExtension { #[derive(Debug, Clone, Error, PartialEq, Eq)] #[non_exhaustive] pub enum ParseError { - #[error(". was not found - missing namespace")] + #[error("# was not found - missing namespace")] MissingNamespace, } #[allow(deprecated)] impl OperationExtension { - /// Creates a new [`OperationExtension`] from the absolute shape ID of the operation with `#` symbol replaced with a `.`. + /// Creates a new [`OperationExtension`] from the absolute shape ID. pub fn new(absolute_operation_id: &'static str) -> Result { let (namespace, name) = absolute_operation_id - .rsplit_once('.') + .rsplit_once('#') .ok_or(ParseError::MissingNamespace)?; Ok(Self { absolute: absolute_operation_id, @@ -240,7 +238,7 @@ mod tests { #[test] fn ext_accept() { - let value = "com.amazonaws.ebs.CompleteSnapshot"; + let value = "com.amazonaws.ebs#CompleteSnapshot"; let ext = OperationExtension::new(value).unwrap(); assert_eq!(ext.absolute(), value); From fda7ffd1bd92f341657197daed76b2d6bdf05246 Mon Sep 17 00:00:00 2001 From: Harry Barber Date: Thu, 2 Feb 2023 13:12:03 +0000 Subject: [PATCH 2/4] Add OperationExtensionExt test --- .../aws-smithy-http-server/src/extension.rs | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/rust-runtime/aws-smithy-http-server/src/extension.rs b/rust-runtime/aws-smithy-http-server/src/extension.rs index e7a0042052..2a8b664cdd 100644 --- a/rust-runtime/aws-smithy-http-server/src/extension.rs +++ b/rust-runtime/aws-smithy-http-server/src/extension.rs @@ -35,7 +35,7 @@ pub use crate::request::extension::{Extension, MissingExtension}; /// This extension type is inserted, via the [`OperationExtensionPlugin`], whenever it has been correctly determined /// that the request should be routed to a particular operation. The operation handler might not even get invoked /// because the request fails to deserialize into the modeled operation input. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct OperationExtension { absolute: &'static str, @@ -234,6 +234,10 @@ impl Deref for RuntimeErrorExtension { #[cfg(test)] mod tests { + use tower::{service_fn, ServiceExt}; + + use crate::{operation::OperationShapeExt, proto::rest_json_1::RestJson1}; + use super::*; #[test] @@ -254,4 +258,33 @@ mod tests { ParseError::MissingNamespace ) } + + #[tokio::test] + async fn plugin() { + struct DummyOp; + + impl OperationShape for DummyOp { + const NAME: &'static str = "com.amazonaws.ebs#CompleteSnapshot"; + + type Input = (); + type Output = (); + type Error = (); + } + + // Apply `Plugin`. + let operation = DummyOp::from_handler(|_| async { Ok(()) }); + let plugins = PluginPipeline::new().insert_operation_extension(); + let op = Plugin::::map(&plugins, operation); + + // Apply `Plugin`s `Layer`. + let layer = op.layer; + let svc = service_fn(|_: http::Request<()>| async { Ok::<_, ()>(http::Response::new(())) }); + let svc = layer.layer(svc); + + // Check for `OperationExtension`. + let response = svc.oneshot(http::Request::new(())).await.unwrap(); + let expected = OperationExtension::new(DummyOp::NAME).unwrap(); + let actual = response.extensions().get::().unwrap(); + assert_eq!(*actual, expected); + } } From a4672f8fa3b5381d353aa86185fcd54d3336d783 Mon Sep 17 00:00:00 2001 From: Harry Barber Date: Thu, 2 Feb 2023 13:51:37 +0000 Subject: [PATCH 3/4] Add CHANGELOG.next.toml entry --- CHANGELOG.next.toml | 43 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index fc4c4c2578..f57c453a72 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -9,4 +9,45 @@ # message = "Fix typos in module documentation for generated crates" # references = ["smithy-rs#920"] # meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"} -# author = "rcoh" \ No newline at end of file +# author = "rcoh" +[[smithy-rs]] +message = """ +Fix `name` and `absolute` methods on `OperationExtension`. + +The older, [now removed](https://github.com/awslabs/smithy-rs/pull/2161), service builder would insert `OperationExtension` into the `http::Response` containing the [absolute shape ID](https://smithy.io/2.0/spec/model.html#grammar-token-smithy-AbsoluteRootShapeId) with the `#` symbol replaced with a `.`. When [reintroduced](https://github.com/awslabs/smithy-rs/pull/2157) into the new service builder machinery the behavior was changed - we now do _not_ perform the replace. This change fixes the documentation and `name`/`abosolute` methods of the `OperationExtension` API to match this new behavior. + +In the old service builder, `OperationExtension` was initialized, by the framework, and then used as follows: + +```rust +let ext = OperationExtension::new("com.amazonaws.CompleteSnapshot"); + +// This is expected +let name = ext.name(); // "CompleteSnapshot" +let namespace = ext.namespace(); // = "com.amazonaws"; +``` + +On reintroduction, `OperationExtension` was iniialized, by the `Plugin`, and then used as follows: + +```rust +let ext = OperationExtension::new("com.amazonaws#CompleteSnapshot"); + +// This is the bug +let name = ext.name(); // "amazonaws#CompleteSnapshot" +let namespace = ext.namespace(); // = "com"; +``` + +The intended behavior is now restored: + +```rust +let ext = OperationExtension::new("com.amazonaws#CompleteSnapshot"); + +// This is expected +let name = ext.name(); // "CompleteSnapshot" +let namespace = ext.namespace(); // = "com.amazonaws"; +``` + +The rationale behind this change is that the previous design was tailored towards a specific internal use case and shouldn't be enforced on all customers. +""" +references = ["smithy-rs#2276"] +meta = { "breaking" = true, "tada" = false, "bug" = true, "target" = "server"} +author = "hlbarber" From 4fcd966c70718581e58ff23f63512948c0d60337 Mon Sep 17 00:00:00 2001 From: Harry Barber <106155934+hlbarber@users.noreply.github.com> Date: Thu, 2 Feb 2023 16:04:07 +0000 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Luca Palmieri <20745048+LukeMathWalker@users.noreply.github.com> --- CHANGELOG.next.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index f57c453a72..956448aea1 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -14,7 +14,7 @@ message = """ Fix `name` and `absolute` methods on `OperationExtension`. -The older, [now removed](https://github.com/awslabs/smithy-rs/pull/2161), service builder would insert `OperationExtension` into the `http::Response` containing the [absolute shape ID](https://smithy.io/2.0/spec/model.html#grammar-token-smithy-AbsoluteRootShapeId) with the `#` symbol replaced with a `.`. When [reintroduced](https://github.com/awslabs/smithy-rs/pull/2157) into the new service builder machinery the behavior was changed - we now do _not_ perform the replace. This change fixes the documentation and `name`/`abosolute` methods of the `OperationExtension` API to match this new behavior. +The older, [now removed](https://github.com/awslabs/smithy-rs/pull/2161), service builder would insert `OperationExtension` into the `http::Response` containing the [absolute shape ID](https://smithy.io/2.0/spec/model.html#grammar-token-smithy-AbsoluteRootShapeId) with the `#` symbol replaced with a `.`. When [reintroduced](https://github.com/awslabs/smithy-rs/pull/2157) into the new service builder machinery the behavior was changed - we now do _not_ perform the replace. This change fixes the documentation and `name`/`absolute` methods of the `OperationExtension` API to match this new behavior. In the old service builder, `OperationExtension` was initialized, by the framework, and then used as follows: @@ -26,7 +26,7 @@ let name = ext.name(); // "CompleteSnapshot" let namespace = ext.namespace(); // = "com.amazonaws"; ``` -On reintroduction, `OperationExtension` was iniialized, by the `Plugin`, and then used as follows: +When reintroduced, `OperationExtension` was initialized by the `Plugin` and then used as follows: ```rust let ext = OperationExtension::new("com.amazonaws#CompleteSnapshot");