-
Notifications
You must be signed in to change notification settings - Fork 190
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
Add http = "1.0" support to the http
request wrapper
#3373
Conversation
- Requests are not creatible if extensions exist avoiding the cross-version extension issue. - Response _were_ creatable. For responses, the wrapper will error if extensions existed and you attempt to cross convert Http versions.
A new generated diff is ready to view.
A new doc preview is ready to view. |
http = "0.2.9" | ||
http1 = { package = "http", version = "1" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these both be optional dependencies enabled by the features?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe eventually? But they're both so small that the cost of having them both in tree is very small. When we refactor to use http 1.0 representations under the hood, we can make the http 0x dependency optional
Ok(Self { | ||
status: StatusCode::try_from(parts.status.as_u16()).expect("validated by http 0.x"), | ||
body, | ||
extensions_0x: http0::Extensions::new(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this allocate anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, internally I think it's an Option or something
@@ -53,7 +53,8 @@ pretty_assertions = "1.4.0" | |||
tokio = { version = "1.25", features = ["macros", "rt", "rt-multi-thread", "test-util", "full"] } | |||
tracing-subscriber = { version = "0.3.16", features = ["env-filter"] } | |||
tracing-test = "0.2.1" | |||
hyper_0_14 = { package = "hyper", version = "0.14.27",features = ["client", "server", "tcp", "http1", "http2"] } | |||
hyper_0_14 = { package = "hyper", version = "0.14.27", features = ["client", "server", "tcp", "http1", "http2"] } | |||
http1 = { package = "http", version = "1" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to name this dependency something like http1x
to make it every so slightly less likely to be confused with the HTTP 1 vs. HTTP 2 protocol versions, although that may not make it much better 🤔
Co-authored-by: John DiSanti <jdisanti@amazon.com>
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
Motivation and Context
Description
Add
try_from
andtry_into
for HTTP 1.x to the HTTP request/response wrapper. This is a stepping stone en route to supporting Hyper 1.0Testing
Checklist
CHANGELOG.next.toml
if I made changes to the smithy-rs codegen or runtime cratesCHANGELOG.next.toml
if I made changes to the AWS SDK, generated SDK code, or SDK runtime cratesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.