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.

Sign up for GitHub

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

Jump to bottom

Allow server decorators to inject methods on config #3111

Merged
merged 8 commits into from
Oct 31, 2023
Merged

Allow server decorators to inject methods on config #3111

merged 8 commits into from
Oct 31, 2023

Conversation

david-perez
Copy link
Contributor

PR #3095 added a code-generated ${serviceName}Config object on which
users can register layers and plugins. For example:

let config = PokemonServiceConfig::builder()
    .layer(layers)
    .http_plugin(authn_plugin)
    .model_plugin(authz_plugin)
    .build();

This PR makes it so that server decorators can inject methods on this
config builder object. These methods can apply arbitrary layers, HTTP
plugins, and/or model plugins. Moreover, the decorator can configure
whether invoking such method is required or not.

For example, a decorator can inject an aws_auth method that configures
some plugins using its input arguments. Missing invocation of this method
will result in the config failing to build:

let _: SimpleServiceConfig<
    // No layers have been applied.
    tower::layer::util::Identity,
    // One HTTP plugin has been applied.
    PluginStack<IdentityPlugin, IdentityPlugin>,
    // One model plugin has been applied.
    PluginStack<IdentityPlugin, IdentityPlugin>,
> = SimpleServiceConfig::builder()
    // This method has been injected in the config builder!
    .aws_auth("a".repeat(69).to_owned(), 69)
    // The method will apply one HTTP plugin and one model plugin,
    // configuring them with the input arguments. Configuration can be
    // declared to be fallible, in which case we get a `Result` we unwrap
    // here.
    .expect("failed to configure aws_auth")
    .build()
    // Since `aws_auth` has been marked as required, if the user misses
    // invoking it, this would panic here.
    .unwrap();

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

@david-perez
Allow server decorators to inject methods on config
5445dbf 
PR #3095 added a code-generated `${serviceName}Config` object on which
users can register layers and plugins. For example:

```rust
let config = PokemonServiceConfig::builder()
    .layer(layers)
    .http_plugin(authn_plugin)
    .model_plugin(authz_plugin)
    .build();
```

This PR makes it so that server decorators can inject methods on this
config builder object. These methods can apply arbitrary layers, HTTP
plugins, and/or model plugins. Moreover, the decorator can configure
whether invoking such method is required or not.

For example, a decorator can inject an `aws_auth` method that configures
some plugins using its input arguments. Missing invocation of this method
will result in the config failing to build:

```rust
let _: SimpleServiceConfig<
    // No layers have been applied.
    tower::layer::util::Identity,
    // One HTTP plugin has been applied.
    PluginStack<IdentityPlugin, IdentityPlugin>,
    // One model plugin has been applied.
    PluginStack<IdentityPlugin, IdentityPlugin>,
> = SimpleServiceConfig::builder()
    // This method has been injected in the config builder!
    .aws_auth("a".repeat(69).to_owned(), 69)
    // The method will apply one HTTP plugin and one model plugin,
    // configuring them with the input arguments. Configuration can be
    // declared to be fallible, in which case we get a `Result` we unwrap
    // here.
    .expect("failed to configure aws_auth")
    .build()
    // Since `aws_auth` has been marked as required, if the user misses
    // invoking it, this would panic here.
    .unwrap();
```
@david-perez david-perez added the server Rust server SDK label Oct 30, 2023
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@david-perez david-perez marked this pull request as ready for review October 31, 2023 11:30
@david-perez david-perez requested review from a team as code owners October 31, 2023 11:30
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

drganjoo
drganjoo reviewed Oct 31, 2023
View reviewed changes
...n/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerRootGenerator.kt Show resolved Hide resolved
...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerServiceGenerator.kt Show resolved Hide resolved
...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServiceConfigGenerator.kt Show resolved Hide resolved
...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServiceConfigGenerator.kt Show resolved Hide resolved
...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServiceConfigGenerator.kt Show resolved Hide resolved
...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServiceConfigGenerator.kt Show resolved Hide resolved
...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServiceConfigGenerator.kt Show resolved Hide resolved
...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServiceConfigGenerator.kt Outdated
)
}

private fun isBuilderFallible() = configMethods.isBuilderFallible()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Make this a private val isBuilderFallible = configMethods.isBuilderFallible()?

...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServiceConfigGenerator.kt Outdated
rust(
"""
if !self.${it.requiredBuilderFlagName()} {
return Err(${serviceName}ConfigError::${it.requiredErrorVariant()});
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use #{Err} instead of using Err directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should!

...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServiceConfigGenerator.kt Outdated
writable {
rust(
"""
##[error("service is not fully configured; invoke `${it.requiredBuilderFlagName()}` on the config builder")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't ${it.requiredBuilderFlagName()} output the name of the flag itself, rather than the method name that the user is expected to call on Config::builder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

drganjoo
drganjoo approved these changes Oct 31, 2023
View reviewed changes
...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServiceConfigGenerator.kt Outdated
)
}
}
if (isBuilderFallible()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: keep this before val variants on line 292 as we don't need to compute that if builder is not fallible.

...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServiceConfigGenerator.kt
private fun injectedMethods() = configMethods.map {
writable {
val paramBindings = it.params.map { binding ->
writable { rustTemplate("${binding.name}: #{BindingTy},", "BindingTy" to binding.ty) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this automatically include any dependencies required by BindingTy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

...otlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServiceConfigGenerator.kt
// - "T" is the generic type variable name used in the enclosing impl block.
fun List<Binding>.stackReturnType(genericTypeVarName: String, stackType: RuntimeType): Writable =
this.fold(writable { rust(genericTypeVarName) }) { acc, next ->
writable {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: So, if I return a listOf(A, B, C), does it get transformed into Stack<C, Stack<B, Stack<A, T>>>, where T is genericTypeVarName? If yes, how about we use this.foldRight so that it becomes Stack<A, Stack<B, Stack<C, T>>>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The inner middleware is the left type parameter of the stack: https://docs.rs/tower/latest/tower/layer/util/struct.Stack.html

So we do want Stack<C, Stack<B, Stack<A, T>>>. A will be executed first, then B, then C.

We should document this though.

@github-actions GitHub Actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions GitHub Actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@david-perez david-perez added this pull request to the merge queue Oct 31, 2023
Merged via the queue into main with commit f0929e7 Oct 31, 2023
40 of 41 checks passed
@david-perez david-perez deleted the davidpz/service-config-inject-methods branch October 31, 2023 16:37
rcoh pushed a commit that referenced this pull request Nov 1, 2023
@david-perez @rcoh
Allow server decorators to inject methods on config (#3111)
2a6af3b 
PR #3095 added a code-generated `${serviceName}Config` object on which
users can register layers and plugins. For example:

```rust
let config = PokemonServiceConfig::builder()
    .layer(layers)
    .http_plugin(authn_plugin)
    .model_plugin(authz_plugin)
    .build();
```

This PR makes it so that server decorators can inject methods on this
config builder object. These methods can apply arbitrary layers, HTTP
plugins, and/or model plugins. Moreover, the decorator can configure
whether invoking such method is required or not.

For example, a decorator can inject an `aws_auth` method that configures
some plugins using its input arguments. Missing invocation of this
method
will result in the config failing to build:

```rust
let _: SimpleServiceConfig<
    // No layers have been applied.
    tower::layer::util::Identity,
    // One HTTP plugin has been applied.
    PluginStack<IdentityPlugin, IdentityPlugin>,
    // One model plugin has been applied.
    PluginStack<IdentityPlugin, IdentityPlugin>,
> = SimpleServiceConfig::builder()
    // This method has been injected in the config builder!
    .aws_auth("a".repeat(69).to_owned(), 69)
    // The method will apply one HTTP plugin and one model plugin,
    // configuring them with the input arguments. Configuration can be
    // declared to be fallible, in which case we get a `Result` we unwrap
    // here.
    .expect("failed to configure aws_auth")
    .build()
    // Since `aws_auth` has been marked as required, if the user misses
    // invoking it, this would panic here.
    .unwrap();
```

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
david-perez added a commit that referenced this pull request Nov 30, 2023
@david-perez
Allow injecting methods with generic type parameters in the config ob…
efba260 
…ject

This is a follow-up to #3111. Currently, the injected methods are
limited to taking in concrete types. This PR allows for these methods to
take in generic type parameters as well.

```rust
impl<L, H, M> SimpleServiceConfigBuilder<L, H, M> {
    pub fn aws_auth<C>(config: C) {
        ...
    }
}
```
@david-perez david-perez mentioned this pull request Nov 30, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Dec 19, 2023
@david-perez
Allow injecting methods with generic type parameters in the config ob…
a1ec8b8 
…ject (#3274)

This is a follow-up to #3111. Currently, the injected methods are
limited to taking in concrete types. This PR allows for these methods to
take in generic type parameters as well.

```rust
impl<L, H, M> SimpleServiceConfigBuilder<L, H, M> {
    pub fn aws_auth<C>(config: C) {
        ...
    }
}
```

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
@david-perez david-perez mentioned this pull request Dec 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drganjoo drganjoo drganjoo approved these changes

Assignees
No one assigned
Labels
server Rust server SDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@david-perez @drganjoo