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-gate public use of http-body and hyper within aws-smithy-types #3088

Merged
merged 11 commits into from
Oct 25, 2023

Conversation

ysaito1001
Copy link
Contributor

@ysaito1001 ysaito1001 commented Oct 23, 2023

Motivation and Context

Implements #3033

Description

This PR hides behind cargo features the third-party types from http-body and hyper crates that are used inaws-smithy-types' public API. Customers need to opt-in by enabling a cargo feature http-body-0-4-x in aws-smithy-types to create an SdkBody or ByteStream using those third-party types. For more details, please see the upgrade guide.

As can been seen from code changes, to reduce the surface area where we need to feature-gate things, we have fused the aws_smithy_types::body::Inner::Streaming enum variant into aws_smithy_types::body::Inner::Dyn variant, thereby removing SdkBody::from_dyn.

Testing

Relied on existing tests in CI

Checklist

  • 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
Copy link

@ysaito1001 ysaito1001 marked this pull request as ready for review October 24, 2023 02:59
@ysaito1001 ysaito1001 requested review from a team as code owners October 24, 2023 02:59
@github-actions
Copy link

@github-actions
Copy link

@github-actions
Copy link

@github-actions
Copy link

Copy link
Contributor

@drganjoo drganjoo left a comment

Choose a reason for hiding this comment

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

API stabilization clarifications:

When we say "going 1.x," I understand this indicates a commitment to API stabilization. I'd like to clarify what this means in practical terms:

  1. Does "API stabilization" imply that we will never remove a function from the API? If a removal does happen, would it necessitate the introduction of a new major version?
  2. Are we allowed to add more functions to a crate post stabilization?
  3. Is it possible to deprecate functions, or will we avoid that completely?
  4. Can feature flags be added or removed after stabilization?
  5. Could a current API be placed behind a feature flag in a subsequent minor version after going 1.x?
  6. Are we bound to using the same underlying libraries indefinitely, or do we have the flexibility to switch them out? For example, if we're presently utilizing rustls internally, would we have the liberty to transition to native-tls down the line, as long as there isn't any disruption to the API?

Specifics on this PR:

Is it accurate to say that all functionalities associated with http::Body are now under the http-body-0-4-x flag? Hence, we have renamed functions. For instance, ByteStream::from_path to ByteStream::from_path_0_4 due to the API's reliance on http::Body. Are there elements within ByteStream that function independently of http::Body?

Minor questions regarding renaming:

Am I correct in understanding that the reason for renaming ByteStream::from_body(http::Body) to ByteStream::from_body_0_4(http::Body) is due to concerns about future changes in http::Body? If the name remained unchanged, it would suggest our API can always create a ByteStream from http::Body. Is there a concern that certain features in the http::Body 1.x release might prevent us from maintaining this capability, thus making it impossible to produce a ByteStream from http::Body post 1.x?

  1. If http-body releases a version 0.5, will we rename the function from ::from_body_0_4 to ::from_body_0_5? Alternatively, do we anticipate that http-body will never release a 0.5?
  2. When http-body updates to 1.x, will the http-body-0-4-x feature flag remain in place?

@Velfi
Copy link
Contributor

Velfi commented Oct 25, 2023

API stabilization clarifications:

  1. Does "API stabilization" imply that we will never remove a function from the API? If a removal does happen, would it necessitate the introduction of a new major version?
    

Removing a function would necessitate a new major version.

  1. Are we allowed to add more functions to a crate post stabilization?
    

Yes.

  1. Is it possible to deprecate functions, or will we avoid that completely?
    

It is possible to deprecate functions.

  1. Can feature flags be added or removed after stabilization?
    

They can be added but not removed.

  1. Could a current API be placed behind a feature flag in a subsequent minor version after going 1.x?
    

No, this would break users. Even it we made it a default feature, users with no-default-features would be broken. We could add a #[cfg(any(feature = old-feature, feature = new-feature))] if a feature gate already existed.

  1. Are we bound to using the same underlying libraries indefinitely, or do we have the flexibility to switch them out? For example, if we're presently utilizing rustls internally, would we have the liberty to transition to native-tls down the line, as long as there isn't any disruption to the API?
    

We are only bound in cases where external types are exposed in our public API. Transitioning to new defaults is breaking but we have a proposal from Russell on how to do that in a non-breaking way. Contact him for more info.

Specifics on this PR

Is it accurate to say that all functionalities associated with http::Body are now under the http-body-0-4-x flag?

Yes, based on the current version of http_body used in the runtime crates.

Are there elements within ByteStream that function independently of http::Body?

Yes. Any code that doesn't need to interact with the inner body as a v0.4 HTTP body will not be feature-gated.

Minor questions regarding renaming

  1. If http-body releases a version 0.5, will we rename the function from ::from_body_0_4 to ::from_body_0_5? Alternatively, do we anticipate that http-body will never release a 0.5?

If we decide compatibility with a new version is important, we'll create functions for it. I don't anticipate a v0.5 version of http-body, but if one releases, we'll consider supporting it. Renaming the function would be a breaking change. We are committed indefinitely to supporting any API that exposes a 3rd-party type.

When http-body updates to 1.x, will the http-body-0-4-x feature flag remain in place?

Yes, the feature flag will still exist.

One last thing

The only thing that would cause us to remove the v0.4 stuff is if http-body goes 1.0 before our GA. We can't count on that so we must be prepared to support multiple versions of the API.

@rcoh rcoh mentioned this pull request Oct 25, 2023
9 tasks
@github-actions
Copy link

@ysaito1001 ysaito1001 added this pull request to the merge queue Oct 25, 2023
Merged via the queue into main with commit 2a51e0b Oct 25, 2023
40 of 41 checks passed
@ysaito1001 ysaito1001 deleted the ysaito/feature-gate-http-body-crate branch October 25, 2023 17:11
github-merge-queue bot pushed a commit that referenced this pull request Oct 27, 2023
#3101)

## Motivation and Context
Follow up on #3088

## Description
This PR reverts `ByteStream::read_with_body_0_4_from`,
`ByteStream::from_path_body_0_4`, and `ByteStream::from_file_body_0_4`
to the old names since from a customers' point of view, `FsBuilder`
should not mention anything about an `http-body` version at the API
level.

`FsBuilder` is currently an opt-in feature (with `rt-tokio` enabled) and
we make it tied to `http-body-0-4-x` by requiring `rt-tokio` including a
dependency on `http-body` version 0.4.x.

## Testing
Relied on the existing tests in CI

## 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._
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This will require a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants