Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make OperationExtension store the absolute shape ID #2276

Merged
merged 6 commits into from
Feb 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
45 changes: 38 additions & 7 deletions rust-runtime/aws-smithy-http-server/src/extension.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Expand All @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
}