Avoid exposing `aws_smithy_http::event_stream::receiver::Receiver` in SDK's public API #3114

ysaito1001 · 2023-10-30T21:49:48Z

Motivation and Context

Implements #3100

Description

Currently, we expose aws_smithy_http::event_stream::Receiver in generated SDKs, as shown in the following S3's example (see a generated diff for tmp-codegen-diff/aws-sdk/sdk/s3/src/operation/select_object_content/_select_object_content_output.rs):

pub struct SelectObjectContentOutput {
    <p>The array of results.</p>
    pub payload: ::aws_smithy_http::event_stream::Receiver<
        crate::types::SelectObjectContentEventStream,
        crate::types::error::SelectObjectContentEventStreamError,
    >,
...

This PR wraps Receiver in a new-type, called EventReceiver, which then supports pub async fn recv method whose signature is the same as aws_smithy_http::event_stream::Receiver::recv.

Testing

Relied on existing tests (e.g. s3 and transcribestreaming integration tests cover uses cases affected by this change).

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.

github-actions · 2023-10-30T22:08:34Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

rust-runtime/inlineable/src/event_stream_receiver.rs

...tlin/software/amazon/smithy/rust/codegen/core/smithy/generators/http/HttpBindingGenerator.kt

jdisanti · 2023-10-30T23:59:54Z

Do you think we should also wrap the EventStreamSender? Is there an advantage to that?

It seems like one of the advantages to wrapping the receiver is that we get to properly hide methods such as try_recv_initial. Wrapping the sender would allow for adding such methods in the future.

github-actions · 2023-10-31T01:49:09Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

This commit addresses #3114 (comment)

github-actions · 2023-10-31T02:53:50Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

ysaito1001 · 2023-10-31T15:45:58Z

Do you think we should also wrap the EventStreamSender? Is there an advantage to that?

It seems like one of the advantages to wrapping the receiver is that we get to properly hide methods such as try_recv_initial. Wrapping the sender would allow for adding such methods in the future.

Yeah, I can see value in doing that, benefiting the event sender the same way it benefited the event receiver. Two things:

It will be MessageStreamAdapter that'll be wrapped in a new-type EventSender, and EventSender will probably have a method pub async send whose implementation likely re-uses today's MessageStreamAdapter::poll_next.
Since we currently do not expose MessageStreamAdapter in public API within a stable crate, it can be done later outside this PR in a non-breaking fashion.

So all in all, I think it's worth tracking it via an issue. ~~Will create one.~~ #3118 created.

github-actions · 2023-10-31T15:55:50Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

github-actions · 2023-10-31T18:25:15Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

…thy-eventstream` to `aws-smithy-types` (#3139) ## Motivation and Context Fixes the following statement in #3114 > A notable breaking change is the error type of the new recv method. Previously, it was [SdkError<E, RawMessage>>](https://docs.rs/aws-smithy-http/0.57.0/aws_smithy_http/event_stream/struct.Receiver.html#method.recv) but it is now a BoxError. This means that upon an event receive error, customers can only show what the error is, but not match on it and take some actions. so that the `recv` method on a new-type wrapper `aws_smithy_http::event_stream::Receiver` can expose `SdkError<E, RawMessage>>` in public API. It is the responsibility of the above PR to move [RawMessage](https://github.com/awslabs/smithy-rs/blob/c7f0d5dc33937d65841d74b5b828cc77ed1d2db4/rust-runtime/aws-smithy-http/src/event_stream/receiver.rs#L92-L98) from `aws-smithy-http` to `aws-smithy-types` but doing so first requires `Message` to be moved to `aws-smithy-types`, hence this PR. ## Description Moving `Message` to `aws-smithy-types` also requires moving the types it depends upon, namely `Header`, `HederValue`, and `StrBytes`. However, their serialization/deserialization methods (`read_from` and `write_to`) remain in `aws-smithy-eventstream` and they are converted to free functions, with the aim of moving things as minimally as possible. `aws-smithy-eventstream` has fuzz testing. It previously implemented the `Arbitrary` trait for moved types, but since they are now in `aws-smithy-types`, we need to have newtypes for them to enable the trait due to the orphan rules. A newly added module `aws-smithy-eventstream::arbitrary` is exactly for that purpose. ## Testing Relied on the existing tests. ## Checklist  - [x] 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._

github-actions · 2023-11-06T23:06:53Z

A new generated diff is ready to view.

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

A new doc preview is ready to view.

…rom `aws-smithy-eventstream` to `aws-smithy-types` (#3139) ## Motivation and Context Fixes the following statement in smithy-lang/smithy-rs#3114 > A notable breaking change is the error type of the new recv method. Previously, it was [SdkError<E, RawMessage>>](https://docs.rs/aws-smithy-http/0.57.0/aws_smithy_http/event_stream/struct.Receiver.html#method.recv) but it is now a BoxError. This means that upon an event receive error, customers can only show what the error is, but not match on it and take some actions. so that the `recv` method on a new-type wrapper `aws_smithy_http::event_stream::Receiver` can expose `SdkError<E, RawMessage>>` in public API. It is the responsibility of the above PR to move [RawMessage](https://github.com/awslabs/smithy-rs/blob/c7f0d5dc33937d65841d74b5b828cc77ed1d2db4/rust-runtime/aws-smithy-http/src/event_stream/receiver.rs#L92-L98) from `aws-smithy-http` to `aws-smithy-types` but doing so first requires `Message` to be moved to `aws-smithy-types`, hence this PR. ## Description Moving `Message` to `aws-smithy-types` also requires moving the types it depends upon, namely `Header`, `HederValue`, and `StrBytes`. However, their serialization/deserialization methods (`read_from` and `write_to`) remain in `aws-smithy-eventstream` and they are converted to free functions, with the aim of moving things as minimally as possible. `aws-smithy-eventstream` has fuzz testing. It previously implemented the `Arbitrary` trait for moved types, but since they are now in `aws-smithy-types`, we need to have newtypes for them to enable the trait due to the orphan rules. A newly added module `aws-smithy-eventstream::arbitrary` is exactly for that purpose. ## Testing Relied on the existing tests. ## Checklist  - [x] 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._

## Motivation and Context Adds re-export for `EventReceiver` in a generated client crate ## Description `EventReceiver` was added in #3114 but the struct itself could not be referenced as `pub`. Our SDK integration tests did not break in the said PR because they only called the `next` method on it and never referred to that struct name. This PR adds a re-export for `EventReceiver` in case customers need to refer to the name in their code. ## Testing Added a unit test and edited a `transcribestreaming` integration test to verify the re-export. ## Checklist  - [x] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or runtime crates - [x] 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._ --------- Co-authored-by: AWS SDK Rust Bot <aws-sdk-rust-primary@amazon.com>

ysaito1001 added 3 commits October 30, 2023 15:06

Add inlined wrapper for event-stream receiver

86e554d

Update client codegen to use event-stream receiver wrapper

44f9639

Merge branch 'main' into ysaito/new-type-event-stream-receiver

0ff01d1

jdisanti reviewed Oct 30, 2023

View reviewed changes

Bifurcate receiver type based on codegen target

58f466a

ysaito1001 added 2 commits October 30, 2023 21:32

Rename EventStreamReceiver to EventReceiver

b3f4b86

This commit addresses #3114 (comment)

Remove unnecessary #[non_exhaustive]

ad8bdb8

ysaito1001 added 3 commits October 31, 2023 10:21

Merge branch 'main' into ysaito/new-type-event-stream-receiver

023e5eb

Update transcribestreaming test to use BoxError

db20c62

Update transcribe canary to use BoxError

1aa6a8d

ysaito1001 mentioned this pull request Oct 31, 2023

Generate an inlined new-type struct with an async .send() method for aws_smithy_http::event_stream::receiver::Sender #3118

Open

Update CHANGELOG.next.toml

7c08c59

ysaito1001 marked this pull request as ready for review October 31, 2023 18:05

ysaito1001 requested review from a team as code owners October 31, 2023 18:05

ysaito1001 added breaking-change This will require a breaking change needs-sdk-review labels Oct 31, 2023

Merge branch 'main' into ysaito/new-type-event-stream-receiver

7d6ba47

ysaito1001 mentioned this pull request Nov 2, 2023

Move Message, Header, HeaderValue, and StrBytes from aws-smithy-eventstream to aws-smithy-types #3139

Merged

1 task

ysaito1001 added 3 commits November 6, 2023 15:45

Merge branch 'main' into ysaito/new-type-event-stream-receiver

c605390

Make .recv return SdkError instead of BoxError

1a1f226

Do not mention a type from aws-smithy-runtime-api

85163ef

Avoid exposing unstable SegmentedBuf in `smithy-types

b87ddbd

ysaito1001 requested a review from jdisanti November 6, 2023 23:26

jdisanti approved these changes Nov 7, 2023

View reviewed changes

ysaito1001 added this pull request to the merge queue Nov 7, 2023

Merged via the queue into main with commit f9c0526 Nov 7, 2023
41 checks passed

ysaito1001 deleted the ysaito/new-type-event-stream-receiver branch November 7, 2023 03:09

ysaito1001 mentioned this pull request Dec 11, 2023

Reexport EventReceiver in a generated client crate #3305

Merged

2 tasks

Velfi removed the needs-sdk-review label Feb 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid exposing `aws_smithy_http::event_stream::receiver::Receiver` in SDK's public API #3114

Avoid exposing `aws_smithy_http::event_stream::receiver::Receiver` in SDK's public API #3114

ysaito1001 commented Oct 30, 2023 •

edited

Loading

github-actions bot commented Oct 30, 2023

jdisanti commented Oct 30, 2023

github-actions bot commented Oct 31, 2023

github-actions bot commented Oct 31, 2023

ysaito1001 commented Oct 31, 2023 •

edited

Loading

github-actions bot commented Oct 31, 2023

github-actions bot commented Oct 31, 2023

github-actions bot commented Nov 6, 2023

Avoid exposing aws_smithy_http::event_stream::receiver::Receiver in SDK's public API #3114

Avoid exposing aws_smithy_http::event_stream::receiver::Receiver in SDK's public API #3114

Conversation

ysaito1001 commented Oct 30, 2023 • edited Loading

Motivation and Context

Description

Testing

Checklist

github-actions bot commented Oct 30, 2023

jdisanti commented Oct 30, 2023

github-actions bot commented Oct 31, 2023

github-actions bot commented Oct 31, 2023

ysaito1001 commented Oct 31, 2023 • edited Loading

github-actions bot commented Oct 31, 2023

github-actions bot commented Oct 31, 2023

github-actions bot commented Nov 6, 2023

Avoid exposing `aws_smithy_http::event_stream::receiver::Receiver` in SDK's public API #3114

Avoid exposing `aws_smithy_http::event_stream::receiver::Receiver` in SDK's public API #3114

ysaito1001 commented Oct 30, 2023 •

edited

Loading

ysaito1001 commented Oct 31, 2023 •

edited

Loading