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

Allow third-party devs to write custom plugins for service builders #1736

Merged
merged 8 commits into from
Sep 16, 2022

Conversation

82marbag
Copy link
Contributor

Motivation and Context

With the new service and service builder types, it is verbose to add layers to add middleware. Third-party developers can't write an ergonomic middleware to add to services.

Description

With this change, third-parties can write middleware that add a function to service builder,
which smithy service owners can incorporate by calling that function.
Having implemented a trait that adds a custom printing facility to every (any) operation, a third-party owner provides a .print() plugin to a service builder:

let app = pokemon_service_server_sdk::service::PokemonService::builder()
    ... all other operations ...
    .empty_operation(empty_operation)
    .print()
    .build();

Multiple plugins can be added to provide some functionality: .print().auth()...

Testing

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK 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.

82marbag and others added 5 commits September 14, 2022 20:46
Third-party developers can add custom functions to a service builder,
through a `BuilderModifier`.
These functionalities are applied to every operation of that service.

An example of a builder modifier can be found in
rust-runtime/aws-smithy-http-server/examples/pokemon-service/lib.rs

Signed-off-by: Harry Barber <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
Co-authored-by: Harry Barber <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
Signed-off-by: Daniele Ahmed <[email protected]>
@82marbag 82marbag requested a review from a team as a code owner September 15, 2022 14:56
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@hlbarber hlbarber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@@ -213,6 +224,17 @@ class ServerServiceGeneratorV2(
}
}
}

impl<$builderGenerics, NewPlugin> #{SmithyHttpServer}::plugin::Pluggable<NewPlugin> for $builderName<$builderGenerics> {
Copy link
Contributor

@hlbarber hlbarber Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a , here in $builderGenerics, NewPlugin might cause problems when $builderGenerics is empty? I'm not entirely sure. It might be worth generating this using .joinToString(", ") so we don't get a comma when $builderGenerics is empty.

Copy link
Contributor

@hlbarber hlbarber Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same below with $builderGenericsNoPlugin, aws_smithy_http_server::plugin::PluginStack<$pluginName, NewPlugin>.

I'm not sure this is worth changing - there might be a ton of other existing instances where the codegen breaks in the case of no operations.

Maybe we should batch it up into single effort and write a standalone PR to check whether it works nicely in this degenerate case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It breaks the operation registry too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: #1369

Signed-off-by: Daniele Ahmed <[email protected]>
@82marbag 82marbag enabled auto-merge (squash) September 16, 2022 08:09
Signed-off-by: Daniele Ahmed <[email protected]>
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@82marbag 82marbag merged commit 0313a3c into main Sep 16, 2022
@82marbag 82marbag deleted the external-layers branch September 16, 2022 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants