Unverified Commit 3ee5d069 authored by Harry Barber's avatar Harry Barber Committed by GitHub
Browse files

Make `OperationExtension` store the absolute shape ID (#2276) 



* Do not alter Operation shape ID

* Add OperationExtensionExt test

* Add CHANGELOG.next.toml entry

* Apply suggestions from code review

Co-authored-by: default avatarLuca Palmieri <20745048+LukeMathWalker@users.noreply.github.com>

---------

Co-authored-by: default avatarLuca Palmieri <20745048+LukeMathWalker@users.noreply.github.com>
parent 47f6bd86

CHANGELOG.next.toml

+42 −0
Original line number Diff line number Diff line
@@ -82,3 +82,45 @@ message = "Fix broken doc link for `tokio_stream::Stream` that is a re-export of 
references = ["smithy-rs#2271"]

meta = { "breaking" = false, "tada" = false, "bug" = true }

author = "ysaito1001"



[[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`/`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:



```rust

let ext = OperationExtension::new("com.amazonaws.CompleteSnapshot");



// This is expected

let name = ext.name(); // "CompleteSnapshot"

let namespace = ext.namespace(); // = "com.amazonaws";

```



When reintroduced, `OperationExtension` was initialized 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"

rust-runtime/aws-smithy-http-server/src/extension.rs

+38 −7
Original line number Diff line number Diff line
@@ -35,9 +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.

///

/// The format given must be the absolute shape ID with `#` replaced with a `.`.

#[derive(Debug, Clone)]

#[derive(Debug, Clone, PartialEq, Eq)]

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<Self, ParseError> {

        let (namespace, name) = absolute_operation_id

            .rsplit_once('.')

            .rsplit_once('#')

            .ok_or(ParseError::MissingNamespace)?;

        Ok(Self {

            absolute: absolute_operation_id,
@@ -236,11 +234,15 @@ 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]

    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);
@@ -256,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::<RestJson1, DummyOp, _, _>::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::<OperationExtension>().unwrap();

        assert_eq!(*actual, expected);

    }

}