Port customizable operation to orchestrator #2706
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds two interceptors, `RequestMapInterceptor` and `RequestMutationInterceptor`, both of which will be used to implement `map_request` and `mutate_request` on `CustomizableOperation` in the orchestrator.
This commit adds `CustomizableOperation` to the orchestrator. It is constructed when the `customize` method is called on a fluent builder. Technically speaking, the `customize` method in the orchestrator can be a synchronous method, but is currently marked `async` due to the counterpart in the middleware.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
jdisanti
reviewed
May 17, 2023
...amazon/smithy/rust/codegen/client/smithy/generators/client/CustomizableOperationGenerator.kt
Outdated
Show resolved
Hide resolved
...software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerator.kt
Outdated
Show resolved
Hide resolved
| ##[doc(hidden)] | ||
| // TODO(enableNewSmithyRuntime): Remove `async` once we switch to orchestrator | ||
| pub async fn customize_orchestrator(self) -> #{CustomizableOperation} { | ||
| #{CustomizableOperation} { fluent_builder: self, config_override: None, interceptors: vec![] } |
There was a problem hiding this comment.
Will the initial config override be lost if I do something like the following?
client.some_operation()
.config_override(some_config)
.customize()
.await
.add_interceptor(some_interceptor)
.send()There was a problem hiding this comment.
Good question. The given snippet no longer complies after this PR, since there is no config_override on a fluent builder anymore. config_override is only accessible after you call .customize. Besides regular parameters for an operation input, I wanted to shove everything that's related to configuring a single operation invocation under CustomizableOperation. That being said, I'm curious to know what you think.
…egen/client/smithy/generators/client/CustomizableOperationGenerator.kt Co-authored-by: John DiSanti <jdisanti@amazon.com>
…egen/client/smithy/generators/client/FluentClientGenerator.kt Co-authored-by: John DiSanti <jdisanti@amazon.com>
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
This commit addresses #2706 (comment) #2706 (comment)
…ttps://github.com/awslabs/smithy-rs into ysaito/port-customizable-operation-to-orchestrator
This commit makes the `customize` method in the orchestrator return a `Result` so its signature is fully compatible with the counterpart in the middleware.
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
jdisanti
approved these changes
May 17, 2023
david-perez
pushed a commit
that referenced
this pull request
May 18, 2023
## Motivation and Context Port [Customizable Operation](#1647) to orchestrator ## Description This PR implements `CustomizableOperation` in the orchestrator. Just like the counterpart in the middleware, it is created when the `customize` method (in the orchestrator mode) on a fluent builder is called. The `customize` method in the orchestrator could technically be made a synchronous method because there is no need to create an operation, which requires `async`, therefore making the `customize` method in the middleware `async`. However, during the transition from the middleware to the orchestrator, the integration tests ([example](https://github.com/awslabs/smithy-rs/blob/31c152d9af53afb9a5e6edf9df3def57931b9c1e/aws/sdk/integration-tests/s3/tests/signing-it.rs#L36)) need to be able to run in both modes. For this reason, the `customize` method in the orchestrator is temporarily marked as `async`. Regarding methods defined on the new `CustomizableOperation`, they include `mutate_request` and `map_request` from the counterpart in the middleware. However, it did not port `map_operation` because there is no operation to map on. Most use cases for `map_operation` is put things in a property bag. The new `CustomizableOperation` provides an `interceptor` method to accomplish the same, i.e putting things in a config bag. Finally, for integration tests to run in both modes, the code gen emits the implementation of the `customize` method differently depending on the active Smithy runtime mode, similar to what the implementation of `send` method does. ## Testing Added one `sra-test` for mutating a request. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --------- Co-authored-by: Yuki Saito <awsaito@amazon.com> Co-authored-by: John DiSanti <jdisanti@amazon.com>
david-perez
pushed a commit
that referenced
this pull request
May 22, 2023
## Motivation and Context Port [Customizable Operation](#1647) to orchestrator ## Description This PR implements `CustomizableOperation` in the orchestrator. Just like the counterpart in the middleware, it is created when the `customize` method (in the orchestrator mode) on a fluent builder is called. The `customize` method in the orchestrator could technically be made a synchronous method because there is no need to create an operation, which requires `async`, therefore making the `customize` method in the middleware `async`. However, during the transition from the middleware to the orchestrator, the integration tests ([example](https://github.com/awslabs/smithy-rs/blob/31c152d9af53afb9a5e6edf9df3def57931b9c1e/aws/sdk/integration-tests/s3/tests/signing-it.rs#L36)) need to be able to run in both modes. For this reason, the `customize` method in the orchestrator is temporarily marked as `async`. Regarding methods defined on the new `CustomizableOperation`, they include `mutate_request` and `map_request` from the counterpart in the middleware. However, it did not port `map_operation` because there is no operation to map on. Most use cases for `map_operation` is put things in a property bag. The new `CustomizableOperation` provides an `interceptor` method to accomplish the same, i.e putting things in a config bag. Finally, for integration tests to run in both modes, the code gen emits the implementation of the `customize` method differently depending on the active Smithy runtime mode, similar to what the implementation of `send` method does. ## Testing Added one `sra-test` for mutating a request. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --------- Co-authored-by: Yuki Saito <awsaito@amazon.com> Co-authored-by: John DiSanti <jdisanti@amazon.com>
david-perez
pushed a commit
that referenced
this pull request
May 22, 2023
## Motivation and Context Port [Customizable Operation](#1647) to orchestrator ## Description This PR implements `CustomizableOperation` in the orchestrator. Just like the counterpart in the middleware, it is created when the `customize` method (in the orchestrator mode) on a fluent builder is called. The `customize` method in the orchestrator could technically be made a synchronous method because there is no need to create an operation, which requires `async`, therefore making the `customize` method in the middleware `async`. However, during the transition from the middleware to the orchestrator, the integration tests ([example](https://github.com/awslabs/smithy-rs/blob/31c152d9af53afb9a5e6edf9df3def57931b9c1e/aws/sdk/integration-tests/s3/tests/signing-it.rs#L36)) need to be able to run in both modes. For this reason, the `customize` method in the orchestrator is temporarily marked as `async`. Regarding methods defined on the new `CustomizableOperation`, they include `mutate_request` and `map_request` from the counterpart in the middleware. However, it did not port `map_operation` because there is no operation to map on. Most use cases for `map_operation` is put things in a property bag. The new `CustomizableOperation` provides an `interceptor` method to accomplish the same, i.e putting things in a config bag. Finally, for integration tests to run in both modes, the code gen emits the implementation of the `customize` method differently depending on the active Smithy runtime mode, similar to what the implementation of `send` method does. ## Testing Added one `sra-test` for mutating a request. ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --------- Co-authored-by: Yuki Saito <awsaito@amazon.com> Co-authored-by: John DiSanti <jdisanti@amazon.com>
ysaito1001
added a commit
that referenced
this pull request
May 26, 2023
## Motivation and Context Render `CustomizableOperation` once in the `customizable` module instead of rendering it in every operation's `builders` module. ## Description This PR is a follow-up on #2706. The previous PR ported `CustomizableOperation` in the middleware to the orchestrator but it had a flaw in that the type was rendered per every operation. This was clearly a codegen regression because the code for `CustomizableOperation` is repeated many times and is rendered even if no customization is requested for an operation. This PR attempts to fix that problem. Just like `CustomizableOperation` in the middleware, we render the new `CustomizableOperation` once in the `customize` module per crate. To accomplish that, the new `CustomizableOperation` uses a closure, called `customizable_send`, to encapsulate a call to a fluent builder's `send` method and invokes it when its own `send` method is called. ~~As a side note, this PR will not take care of the following. We have [a separate PR](#2723) to fix them and once we've merged it to this PR, they should be addressed automatically (there will be merge conflicts, though):~~ - ~~Removing `send_orchestrator_with_plugin` (which is why a type parameter `R` is present in the code changes, but it'll soon go away)~~ - ~~Incorrect signature of a closure in orchestrator's `CustomizableOperaton::map_request`~~ ## Testing No new tests added as it's purely refactoring. Confirmed `sra-test` passed (except for the one that's known to fail). ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --------- Co-authored-by: Yuki Saito <awsaito@amazon.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Port Customizable Operation to orchestrator
Description
This PR implements
CustomizableOperationin the orchestrator. Just like the counterpart in the middleware, it is created when thecustomizemethod (in the orchestrator mode) on a fluent builder is called. Thecustomizemethod in the orchestrator could technically be made a synchronous method because there is no need to create an operation, which requiresasync, therefore making thecustomizemethod in the middlewareasync. However, during the transition from the middleware to the orchestrator, the integration tests (example) need to be able to run in both modes. For this reason, thecustomizemethod in the orchestrator is temporarily marked asasync.Regarding methods defined on the new
CustomizableOperation, they includemutate_requestandmap_requestfrom the counterpart in the middleware. However, it did not portmap_operationbecause there is no operation to map on. Most use cases formap_operationis put things in a property bag. The newCustomizableOperationprovides aninterceptormethod to accomplish the same, i.e putting things in a config bag.Finally, for integration tests to run in both modes, the code gen emits the implementation of the
customizemethod differently depending on the active Smithy runtime mode, similar to what the implementation ofsendmethod does.Testing
Added one
sra-testfor mutating a request.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.