Better distinguish model and HTTP plugins #2827

david-perez · 2023-07-03T11:48:44Z

So far, servers have tacitly worked with the notion that plugins come in
two flavors: "HTTP plugins" and "model plugins":

A HTTP plugin acts on the HTTP request before it is deserialized, and
acts on the HTTP response after it is serialized.
A model plugin acts on the modeled operation input after it is
deserialized, and acts on the modeled operation output or the modeled
operation error before it is serialized.

However, this notion was never reified in the type system. Thus, users
who pass in a model plugin where a HTTP plugin is expected or
viceversa in several APIs:

let pipeline = PluginPipeline::new().push(http_plugin).push(model_plugin);

let app = SimpleService::builder_with_plugins(http_plugins, IdentityPlugin)
    .post_operation(handler)
    /* ... */
    .build()
    .unwrap();

would get the typical Tower service compilation errors, which can get
very confusing:

error[E0631]: type mismatch in function arguments
  --> simple/rust-server-codegen/src/main.rs:47:6
   |
15 | fn new_svc<S, Ext>(inner: S) -> model_plugin::PostOperationService<S, Ext> {
   | -------------------------------------------------------------------------- found signature defined here
...
47 |     .post_operation(post_operation)
   |      ^^^^^^^^^^^^^^ expected due to this
   |
   = note: expected function signature `fn(Upgrade<RestJson1, (PostOperationInput, _), PostOperationService<aws_smithy_http_server::operation::IntoService<simple::operation_shape::PostOperation, _>, _>>) -> _`
              found function signature `fn(aws_smithy_http_server::operation::IntoService<simple::operation_shape::PostOperation, _>) -> _`
   = note: required for `LayerFn<fn(aws_smithy_http_server::operation::IntoService<..., ...>) -> ... {new_svc::<..., ...>}>` to implement `Layer<Upgrade<RestJson1, (PostOperationInput, _), PostOperationService<aws_smithy_http_server::operation::IntoService<simple::operation_shape::PostOperation, _>, _>>>`
   = note: the full type name has been written to '/local/home/davidpz/workplace/smithy-ws/src/SmithyRsSource/target/debug/deps/simple-6577f9f79749ceb9.long-type-4938700695428041031.txt'

This commit introduces the HttpPlugin and ModelPlugin marker traits,
allowing plugins to be marked as an HTTP plugin, a model plugin, or
both. It also removes the primary way of concatenating plugins,
PluginPipeline, in favor of HttpPlugins and ModelPlugins, which
eagerly check that whenever a plugin is pushed, it is of the expected
type. The generated service type in the server SDKs'
builder_with_plugins constructor now takes an HttpPlugin as its
first parameter, and a ModelPlugin as its second parameter.

I think this change perhaps goes counter to the generally accepted
wisdom that trait bounds in Rust should be enforced "at the latest
possible moment", that is, only when the behavior encoded in the trait
implementation is relied upon in the code (citation needed).
However, the result is that exposing the concepts of HTTP plugin and
model plugin in the type system makes for code that is easier to reason
about, and produces more helpful compiler error messages.

Documentation about the plugin system has been expanded, particularly on
how to implement model plugins.

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

So far, servers have tacitly worked with the notion that plugins come in two flavors: "HTTP plugins" and "model plugins": - A HTTP plugin acts on the HTTP request before it is deserialized, and acts on the HTTP response after it is serialized. - A model plugin acts on the modeled operation input after it is deserialized, and acts on the modeled operation output or the modeled operation error before it is serialized. However, this notion was never reified in the type system. Thus, users who pass in a model plugin where a HTTP plugin is expected or viceversa in several APIs: ```rust let pipeline = PluginPipeline::new().push(http_plugin).push(model_plugin); let app = SimpleService::builder_with_plugins(http_plugins, IdentityPlugin) .post_operation(handler) /* ... */ .build() .unwrap(); ``` would get the typical Tower service compilation errors, which can get very confusing: ``` error[E0631]: type mismatch in function arguments --> simple/rust-server-codegen/src/main.rs:47:6 | 15 | fn new_svc<S, Ext>(inner: S) -> model_plugin::PostOperationService<S, Ext> { | -------------------------------------------------------------------------- found signature defined here ... 47 | .post_operation(post_operation) | ^^^^^^^^^^^^^^ expected due to this | = note: expected function signature `fn(Upgrade<RestJson1, (PostOperationInput, _), PostOperationService<aws_smithy_http_server::operation::IntoService<simple::operation_shape::PostOperation, _>, _>>) -> _` found function signature `fn(aws_smithy_http_server::operation::IntoService<simple::operation_shape::PostOperation, _>) -> _` = note: required for `LayerFn<fn(aws_smithy_http_server::operation::IntoService<..., ...>) -> ... {new_svc::<..., ...>}>` to implement `Layer<Upgrade<RestJson1, (PostOperationInput, _), PostOperationService<aws_smithy_http_server::operation::IntoService<simple::operation_shape::PostOperation, _>, _>>>` = note: the full type name has been written to '/local/home/davidpz/workplace/smithy-ws/src/SmithyRsSource/target/debug/deps/simple-6577f9f79749ceb9.long-type-4938700695428041031.txt' ``` This commit introduces the `HttpPlugin` and `ModelPlugin` marker traits, allowing plugins to be marked as an HTTP plugin, a model plugin, or both. It also removes the primary way of concatenating plugins, `PluginPipeline`, in favor of `HttpPlugins` and `ModelPlugins`, which eagerly check that whenever a plugin is `push`ed, it is of the expected type. The generated service type in the server SDKs' `builder_with_plugins` constructor now takes an `HttpPlugin` as its first parameter, and a `ModelPlugin` as its second parameter. I think this change perhaps goes counter to the generally accepted wisdom that trait bounds in Rust should be enforced "at the latest possible moment", that is, only when the behavior encoded in the trait implementation is relied upon in the code (citation needed). However, the result is that exposing the concepts of HTTP plugin and model plugin in the type system makes for code that is easier to reason about, and produces more helpful compiler error messages. Documentation about the plugin system has been expanded, particularly on how to implement model plugins.

david-perez · 2023-07-03T11:50:44Z

rust-runtime/aws-smithy-http-server/src/plugin/layer.rs

@@ -23,6 +23,12 @@ where
    }
 }

+// Without more information about what the layer `L` does, we can't know whether it's appropriate


Not too happy about this. With PluginLayer and LayerPlugin, a user can effectively circumvent the whole point of this PR by converting an e.g. model plugin to a layer and using LayerPlugin to get a HTTP plugin.

Good catch.

To prevent confusion, we could remove the constructor here (pub L) -> (L) and instead require that the user construct LayerPlugin directly through HttpPlugins::layer and ModelPlugins::layer.

It seems a bit heavy-handed to force them to use a collection of plugins to make use of a single plugin that they construct via a layer.

I think it's ok we go ahead with this. I can't think of valid reasons for why someone might want to do a plugin -> layer -> plugin conversion.

github-actions · 2023-07-03T12:14:42Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
Server Test (ignoring whitespace)
Server Test Python (ignoring whitespace)
Server Test Typescript (ignoring whitespace)

A new doc preview is ready to view.

hlbarber · 2023-07-03T12:04:50Z

rust-runtime/aws-smithy-http-server/src/plugin/layer.rs

@@ -23,6 +23,12 @@ where
    }
 }

+// Without more information about what the layer `L` does, we can't know whether it's appropriate


Good catch.

To prevent confusion, we could remove the constructor here (pub L) -> (L) and instead require that the user construct LayerPlugin directly through HttpPlugins::layer and ModelPlugins::layer.

hlbarber · 2023-07-03T13:37:07Z

rust-runtime/aws-smithy-http-server/src/plugin/mod.rs

@@ -188,3 +244,222 @@ where
        <Pl as Plugin<Ser, Op, T>>::apply(self, inner)
    }
 }
+
+/// A HTTP plugin is a plugin that acts on the HTTP request before it is deserialized, and acts on


A HTTP plugin is a plugin

I think this is misleading and why perhaps we should call it HttpMarker or something. This description would make sense if it was a super trait of Plugin.

Why is it misleading?

I like that this conveys the notion that end-users should think of plugins as coming in two distinct flavors. It's a more useful mental model for them to have than to think of a plugin as how it's actually implemented.

Why is it misleading?

If I knew of Plugin and then I wrote a function signature

fn foo<S>(x: impl HttpPlugin, svc: S)

I might except to be able call x.apply (from Plugin::apply).

I see. Ok, will change.

hlbarber · 2023-07-03T13:38:59Z

CHANGELOG.next.toml

@@ -586,3 +586,9 @@ message = "The naming `make_token` for fields and the API of `IdempotencyTokenPr
 references = ["smithy-rs#2783"]
 meta = { "breaking" = true, "tada" = false, "bug" = false }
 author = "ysaito1001"
+
+[[smithy-rs]]
+message = "Server `Plugin`s now need to be marked as a `HttpPlugin`, a `ModelPlugin`, or both. Check out the `plugin` module documentation for more information."


I've got a huge changelog entry about the type refactors above - perhaps it'd be nice to squash this one into that and uphold its narrative?

Addressed in a58ec48.

github-actions · 2023-07-03T17:46:59Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
Server Test (ignoring whitespace)
Server Test Python (ignoring whitespace)
Server Test Typescript (ignoring whitespace)

A new doc preview is ready to view.

github-actions · 2023-07-04T11:02:33Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
No codegen difference in the Client Test
Server Test (ignoring whitespace)
Server Test Python (ignoring whitespace)
Server Test Typescript (ignoring whitespace)

A new doc preview is ready to view.

david-perez added the server Rust server SDK label Jul 3, 2023

david-perez requested review from a team as code owners July 3, 2023 11:48

david-perez commented Jul 3, 2023

View reviewed changes

hlbarber approved these changes Jul 3, 2023

View reviewed changes

hlbarber reviewed Jul 3, 2023

View reviewed changes

david-perez added 3 commits July 3, 2023 17:53

Better changelog

a58ec48

fix cargo doc

8e1af65

fix mdbook

ff2aab6

david-perez added 2 commits July 4, 2023 12:01

HttpPlugin -> HttpMarker

6caec26

ModelPlugin -> ModelMarker

113802d

david-perez added this pull request to the merge queue Jul 4, 2023

Merged via the queue into main with commit ddba460 Jul 4, 2023
40 of 41 checks passed

david-perez deleted the davidpz/better-distinguish-model-and-http-plugins branch July 4, 2023 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better distinguish model and HTTP plugins #2827

Better distinguish model and HTTP plugins #2827

david-perez commented Jul 3, 2023

david-perez Jul 3, 2023 •

edited

Loading

hlbarber Jul 3, 2023

david-perez Jul 3, 2023

github-actions bot commented Jul 3, 2023

hlbarber Jul 3, 2023

hlbarber Jul 3, 2023

david-perez Jul 3, 2023

david-perez Jul 3, 2023

hlbarber Jul 3, 2023 •

edited

Loading

david-perez Jul 3, 2023

hlbarber Jul 3, 2023

david-perez Jul 3, 2023

github-actions bot commented Jul 3, 2023

github-actions bot commented Jul 4, 2023

Better distinguish model and HTTP plugins #2827

Better distinguish model and HTTP plugins #2827

Conversation

david-perez commented Jul 3, 2023

Checklist

david-perez Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlbarber Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 3, 2023

github-actions bot commented Jul 4, 2023

david-perez Jul 3, 2023 •

edited

Loading

hlbarber Jul 3, 2023 •

edited

Loading