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

Feature: Customizable Operations #1647

Merged
merged 17 commits into from
Sep 2, 2022
Merged

Conversation

Velfi
Copy link
Contributor

@Velfi Velfi commented Aug 18, 2022

Motivation and Context

#1112, aws-sdk-rust#537

Description

SDK customers occasionally need to add additional HTTP headers to requests, and currently,
the SDK has previously had no easy way to accomplish this. This PR adds a feature that provides an easier way to augment requests that is compatible with the fluent client.

Before:

let input = SomeOperationInput::builder().some_value(5).build()?;

let operation = {
    let op = input.make_operation(&service_config).await?;
    let (request, response) = op.into_request_response();

    let request = request.augment(|req, _props| {
        req.headers_mut().insert(
            HeaderName::from_static("x-some-header"),
            HeaderValue::from_static("some-value")
        );
        Result::<_, Infallible>::Ok(req)
    })?;

    Operation::from_parts(request, response)
};

let response = smithy_client.call(operation).await?;

After:

let response = client.some_operation()
    .some_value(5)
    .customize()
    .await?
    .mutate_request(|mut req| {
        req.headers_mut().insert(
            HeaderName::from_static("x-some-header"),
            HeaderValue::from_static("some-value")
        );
    })
    .send()
    .await?;

Major credit to @jdisanti who wrote the original RFC for this feature.

Testing

I wrote an integration test

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.

@Velfi Velfi requested a review from a team as a code owner August 18, 2022 20:17
@Velfi
Copy link
Contributor Author

Velfi commented Aug 18, 2022

We didn't previously have an inlineables module for smithy stuff so I created one as part of this PR. In the process, I copied the current AWS inlineables implementation closely and I may have done things in a strange way. I look forward to y'alls comments and criticism.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@Velfi
Copy link
Contributor Author

Velfi commented Aug 19, 2022

The test failures are due to the feature being defined in an inlineable. I'll have to move it into codegen unless we change how we handle the smithy client generics. I'm going to ask around internally to get a feel for what people think about that.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

Exciting!

CHANGELOG.next.toml Outdated Show resolved Hide resolved
rust-runtime/aws-smithy-inlineable/Cargo.toml Outdated Show resolved Hide resolved
rust-runtime/build.gradle.kts Outdated Show resolved Hide resolved
remove: unnecessary build.gradle.kts change
add: missing copyright headers
Copy link
Contributor Author

@Velfi Velfi left a comment

Choose a reason for hiding this comment

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

Added some comments to delineate the new fluent client codegen from the old fluent client codegen

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

This looks good to me! Just a question and some simple fixes.

@github-actions
Copy link

github-actions bot commented Sep 1, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

update: add IntelliJ idea folder to .gitignore
update: use nullable type instead of optional in GenericsGenerator
update: use clearer naming and types in GenericsGenerator
add: fancy formatter for generic type parameters
update: use new type param formatter for customizable op generics
@github-actions
Copy link

github-actions bot commented Sep 1, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Sep 1, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@@ -45,3 +45,6 @@ gradle-app.setting

# Rust build artifacts
target/

# IDEs
.idea/
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline.

Comment on lines +79 to +81
override fun toGenericsGenerator(): GenericsGenerator {
return GenericsGenerator()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
override fun toGenericsGenerator(): GenericsGenerator {
return GenericsGenerator()
}
override fun toGenericsGenerator() = GenericsGenerator()

Comment on lines 20 to 24
private val typeArgs: MutableList<GenericTypeArg>

init {
typeArgs = genericTypeArgs.toMutableList()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply:

Suggested change
private val typeArgs: MutableList<GenericTypeArg>
init {
typeArgs = genericTypeArgs.toMutableList()
}
private val typeArgs = genericTypeArgs.toMutableList()

However, do we need mutability here? I don't see the add method being used.

val bound: RuntimeType? = null,
)

class GenericsGenerator(vararg genericTypeArgs: GenericTypeArg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for creating this generator. We will reuse it for the server, we have a slew of generics and bounds in the operation registry.

nit: would be nice to have some docs on usage and unit tests.

update: use Unit instead of "()"
update: RustSymbolFormatter to handle RustTypes
add: tests for rustTypeParameters
@github-actions
Copy link

github-actions bot commented Sep 1, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

add: tests for GenericsGenerator
update: tests for rustTypeParameters
add: docs for GenericsGenerator
refactor: rustInline to be rustInlineTemplate. Now, it can template, and won't trim input
update: code per PR comments
@Velfi Velfi changed the title Feature/customizable operations Feature: Customizable Operations Sep 2, 2022
@github-actions
Copy link

github-actions bot commented Sep 2, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

github-actions bot commented Sep 2, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@Velfi Velfi merged commit 50d88a5 into main Sep 2, 2022
@Velfi Velfi deleted the feature/customizable-operations branch September 2, 2022 22:47
@Velfi
Copy link
Contributor Author

Velfi commented Sep 26, 2022

released as part of v0.49.0

ysaito1001 added a commit that referenced this pull request May 17, 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 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>
aws-sdk-rust-ci pushed a commit to awslabs/aws-sdk-rust that referenced this pull request May 24, 2023
## Motivation and Context
Port [Customizable
Operation](smithy-lang/smithy-rs#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>
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.

None yet

3 participants