Create the HTTP `Response` wrapper types #3148

jdisanti · 2023-11-07T22:00:27Z

For the same reason that the request wrapper types were created in #3059, this PR establishes the response wrapper types and refactors existing code to use them.

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-08T20:07:23Z

A new generated diff is ready to view.

A new doc preview is ready to view.

rcoh · 2023-11-08T15:44:09Z

...ftware/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ProtocolTestGenerator.kt

@@ -277,15 +277,20 @@ class DefaultProtocolTestGenerator(
        writeInline("let expected_output =")
        instantiator.render(this, expectedShape, testCase.params)
        write(";")
-        write("let mut http_response = #T::new()", RT.HttpResponseBuilder)
+        rustTemplate(
+            "let mut http_response = #{Response}::try_from(#{HttpResponseBuilder}::new()",


I'd probably just do try_into at the bottom..but I guess this is a wide-open block anyway

rcoh · 2023-11-08T15:46:39Z

rust-runtime/aws-smithy-http/src/header.rs

@@ -428,7 +439,14 @@ mod test {
            .body(())
            .unwrap();
        let read = |name: &str, format: Format| {


no worries either way but this is probably a smaller change with .try_into() on all the requests instead

github-actions · 2023-11-08T22:56:36Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2023-11-09T00:57:58Z

A new generated diff is ready to view.

A new doc preview is ready to view.

drganjoo

I've noticed the use of expect and unwraps in several places. While this might not pose a significant issue on the client side, it could be catastrophic if such code paths are executed on the server side. Are these instances confined to handling UTF-8 related headers only?

Additionally, what is our strategy for when http reaches version 1.x? Will we revert to using the types defined by http directly and convert our new Request/Response types into type aliases? Or do we plan on introducing into_http1x?

drganjoo · 2023-11-09T18:03:39Z

rust-runtime/aws-smithy-http-server/src/protocol/mod.rs

+        let content_type = content_type.to_str().map_err(MissingContentTypeReason::ToStrError)?;
+        content_type_header_classifier(content_type, expected_content_type)
+    } else {
+        Ok(())


nit: I'm curious as to why we don't generate an error when this function is called with expected_content_type set to Some(...) and yet the http::header::CONTENT_TYPE is missing from the headers.

Good question. I don't know the answer to this.

drganjoo · 2023-11-09T18:18:29Z

aws/rust-runtime/aws-config/src/imds/client.rs

+                .body(SdkBody::from(token))
+                .unwrap(),
+        )
+        .unwrap()


nit: Could there be a scenario where HttpResponse::try_from fails? If failure isn't possible, why did we implement try_from rather than from?

try_from fails when there a header value is not ASCII or when a header value is invalid UTF-8. My intention was for all these try_from(...).unwrap() conversions to only take place in unit tests. In the actual code paths, we should be doing proper error handling for it.

drganjoo · 2023-11-09T18:22:18Z

...amazon/smithy/rust/codegen/client/smithy/generators/client/CustomizableOperationGenerator.kt

                .resolve("client::orchestrator::HttpRequest"),
-            "HttpResponse" to RuntimeType.smithyRuntimeApi(runtimeConfig)
+            "HttpResponse" to RuntimeType.smithyRuntimeApiClient(runtimeConfig)


nit: Considering that HttpResponse is used on the server side as well, wouldn't it be more appropriate to place it in smithyRuntimeApi instead of smithyRuntimeApiClient?

It appears like the types are also available without the client feature flag, under the http module. I wonder then why we don't use them directly instead of going through the client module?

Yeah, this makes sense to me. We could probably even get rid of the HttpRequest/HttpResponse types from th eorchestrator module at this point. I think it's not so high priority though, and we can always deprecate them later.

drganjoo · 2023-11-09T19:43:40Z

examples/pokemon-service-client-usage/examples/retry-classifier.rs

@@ -53,7 +53,7 @@ impl ClassifyRetry for SampleRetryClassifier {
            .and_then(|err| err.downcast_ref::<GetServerStatisticsError>())
        {
            if let Some(response) = ctx.response() {
-                if response.status() == StatusCode::SERVICE_UNAVAILABLE {
+                if response.status().as_u16() == StatusCode::SERVICE_UNAVAILABLE.as_u16() {


nit: It would be nice to have an impl From<http::StatusCode> for the response.status() that we return. This would enable direct conversion from http::StatusCode to the type of response.status().

Good call. Added in 08f60b0.

drganjoo · 2023-11-09T19:45:48Z

rust-runtime/aws-smithy-http-server/src/protocol/aws_json/rejection.rs

@@ -26,6 +27,10 @@ pub enum RequestRejection {
    JsonDeserialize(#[from] aws_smithy_json::deserialize::error::DeserializeError),
    #[error("request does not adhere to modeled constraints: {0}")]
    ConstraintViolation(String),
+
+    /// Typically happens when the request has headers that are not valid UTF-8.


I'm curious, since this issue occurred previously during deserialization, and now it will simply occur earlier in the lifecycle, what was the error we returned at that time?

We returned a HeaderParse rejection:

https://github.com/awslabs/smithy-rs/blob/d4208b2c8bba1c88c0fd3c7fc8f09f10c9346647/rust-runtime/aws-smithy-http-server/src/protocol/rest_json_1/rejection.rs#L129-L132

Which encompasses other failure scenarios, like expecting a header to be present and it isn't.

drganjoo · 2023-11-09T19:58:26Z

rust-runtime/aws-smithy-http-server/src/protocol/aws_json/rejection.rs

+
+    /// Typically happens when the request has headers that are not valid UTF-8.
+    #[error("failed to convert request: {0}")]
+    HttpConversion(#[from] HttpError),


nit: Do we have a test that checks for this new variant?

Currently no test.

drganjoo · 2023-11-09T20:09:23Z

rust-runtime/aws-smithy-runtime-api/src/http/response.rs

+    ///
+    /// Depending on the internal storage type, this operation may be free or it may have an internal
+    /// cost.
+    pub fn into_http02x(self) -> Result<http0::Response<B>, HttpError> {


nit: Wondering if we should call this try_into_http02x?

Actually, do we ever return a HttpError?

For responses, the error isn't ever returned, but we keep the error as future proofing in the unlikely event that a http1 response can't easily be converted into a http02 response. I do like the idea of adding a try_ prefix to it. I will change that for the request and the response.

Renamed in bc10bcc.

drganjoo · 2023-11-09T20:16:25Z

rust-runtime/aws-smithy-runtime-api/src/http/response.rs

+    extensions: Extensions,
+}
+
+impl<B> Response<B> {


I'm uncertain about our handling of streaming responses; however, I presume this change won't impact that, correct?

This doesn't make any changes to streaming.

drganjoo · 2023-11-09T20:35:46Z

rust-runtime/aws-smithy-runtime-api/src/http/response.rs

+
+/// An HTTP Response Type
+#[derive(Debug)]
+pub struct Response<B = SdkBody> {


Should we implement Default for this, or do you believe there is no need?

david-perez · 2023-11-10T13:11:58Z

...amazon/smithy/rust/codegen/client/smithy/generators/client/CustomizableOperationGenerator.kt

                .resolve("client::orchestrator::HttpRequest"),
-            "HttpResponse" to RuntimeType.smithyRuntimeApi(runtimeConfig)
+            "HttpResponse" to RuntimeType.smithyRuntimeApiClient(runtimeConfig)


It appears like the types are also available without the client feature flag, under the http module. I wonder then why we don't use them directly instead of going through the client module?

david-perez · 2023-11-10T13:28:59Z

rust-runtime/aws-smithy-http-server/src/protocol/aws_json/rejection.rs

@@ -26,6 +27,10 @@ pub enum RequestRejection {
    JsonDeserialize(#[from] aws_smithy_json::deserialize::error::DeserializeError),
    #[error("request does not adhere to modeled constraints: {0}")]
    ConstraintViolation(String),
+
+    /// Typically happens when the request has headers that are not valid UTF-8.


We returned a HeaderParse rejection:

https://github.com/awslabs/smithy-rs/blob/d4208b2c8bba1c88c0fd3c7fc8f09f10c9346647/rust-runtime/aws-smithy-http-server/src/protocol/rest_json_1/rejection.rs#L129-L132

Which encompasses other failure scenarios, like expecting a header to be present and it isn't.

david-perez · 2023-11-10T13:31:23Z

rust-runtime/aws-smithy-http/src/header.rs

@@ -132,8 +130,8 @@ fn read_many<T>(
 /// Read exactly one or none from a headers iterator
 ///
 /// This function does not perform comma splitting like `read_many`
-pub fn one_or_none<T: FromStr>(
-    mut values: ValueIter<'_, HeaderValue>,
+pub fn one_or_none<'a, T: FromStr>(


(can't leave a comment on line 144). Shouldn't we now no longer need the std::str::from_utf8 call?

In general this file contains several .as_bytes calls, and code that works with &[u8] when I think it could directly work with &str all the way through?

Oh, line 144 looks pretty bad. I'll fix that one at the very least. It'll take a good bit of effort to fix the rest of the file though, it looks like.

Touched up in 85af035.

david-perez · 2023-11-10T13:35:38Z

...are/amazon/smithy/rust/codegen/client/smithy/generators/http/ResponseBindingGeneratorTest.kt

@@ -95,21 +96,28 @@ class ResponseBindingGeneratorTest {
        val testProject = TestWorkspace.testProject(symbolProvider)
        testProject.renderOperation()
        testProject.withModule(symbolProvider.moduleForShape(outputShape)) {
-            unitTest(


Are we forgoing the unitTest style of writing unit tests?

At least in my opinion, the unitTest function doesn't really buy me anything when I have a predetermined test name.

david-perez · 2023-11-10T13:40:29Z

...tware/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt

+            """
+            let #{RequestParts} { uri, headers, body, .. } = #{Request}::try_from(request)?.into_parts();
+            """,
+            *preludeScope,


Do we need preludeScope here? We should add it to codegenScope instead.

Better yet, I want rustTemplate to have the prelude scope baked in, but that's a separate PR.

yes I want this too!

I also want this, but I'll keep it out of this PR. Looks like I can remove the prelude from that rustTemplate call.

jdisanti · 2023-11-10T20:51:56Z

Additionally, what is our strategy for when http reaches version 1.x? Will we revert to using the types defined by http directly and convert our new Request/Response types into type aliases? Or do we plan on introducing into_http1x?

These wrapper types are forever, unfortunately. We will just change the insides of them to use http 1 at that time. You can read more in the RFC: https://github.com/smithy-lang/smithy-rs/blob/main/design/src/rfcs/rfc0037_http_wrapper.md

github-actions · 2023-11-11T00:08:14Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2023-11-11T00:46:19Z

A new generated diff is ready to view.

A new doc preview is ready to view.

jdisanti added 2 commits November 7, 2023 13:57

Create the HttpResponse wrapper types

f242939

Refactor existing code to use the response wrapper

7e473d1

jdisanti added the breaking-change This will require a breaking change label Nov 7, 2023

jdisanti changed the title ~~Create the HttpResponse wrapper types~~ Create the HTTP Response wrapper types Nov 7, 2023

Several CI fixes

e487344

smithy-lang deleted a comment from github-actions bot Nov 8, 2023

jdisanti added 2 commits November 7, 2023 17:29

Fix server protocol tests

a35ceff

Fix external type checks

56390b8

jdisanti added needs-sdk-review needs-server-review labels Nov 8, 2023

jdisanti marked this pull request as ready for review November 8, 2023 01:40

jdisanti requested review from a team as code owners November 8, 2023 01:40

jdisanti added 4 commits November 8, 2023 10:58

More CI fixes

cc84e5b

Update changelog

10763d7

Even more CI fixes

177a013

Merge remote-tracking branch 'origin/main' into jdisanti-http-response

7224f8c

rcoh approved these changes Nov 8, 2023

View reviewed changes

Fix no-default-features build failure

416320b

jdisanti removed the needs-sdk-review label Nov 8, 2023

Fix more external type checks

d4208b2

smithy-lang deleted a comment from github-actions bot Nov 9, 2023

ysaito1001 approved these changes Nov 9, 2023

View reviewed changes

drganjoo reviewed Nov 9, 2023

View reviewed changes

drganjoo approved these changes Nov 9, 2023

View reviewed changes

david-perez reviewed Nov 10, 2023

View reviewed changes

jdisanti added 4 commits November 10, 2023 15:20

Merge remote-tracking branch 'origin/main' into jdisanti-http-response

b2c2e7e

Add From impl for StatusCode

08f60b0

Rename into_http02x to try_into_http02x

bc10bcc

Remove needless UTF-8 conversion

85af035

jdisanti requested review from a team as code owners November 10, 2023 23:48

Merge remote-tracking branch 'origin/main' into jdisanti-http-response

550c71f

CI fix

3e4809e

jdisanti removed the needs-server-review label Nov 11, 2023

jdisanti merged commit 7eb008c into main Nov 11, 2023
40 of 41 checks passed

jdisanti deleted the jdisanti-http-response branch November 11, 2023 00:56

Create the HTTP Response wrapper types #3148

Create the HTTP Response wrapper types #3148

Conversation

jdisanti commented Nov 7, 2023 • edited Loading

github-actions bot commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 9, 2023

drganjoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdisanti Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdisanti commented Nov 10, 2023

github-actions bot commented Nov 11, 2023

github-actions bot commented Nov 11, 2023

Create the HTTP `Response` wrapper types #3148

Create the HTTP `Response` wrapper types #3148

jdisanti commented Nov 7, 2023 •

edited

Loading

jdisanti Nov 10, 2023 •

edited

Loading