From 8aa3a74af9110e852a2bf695c1d7069d76cd9fcb Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Wed, 4 Aug 2021 17:31:48 -0700 Subject: [PATCH] Remove generic body from `ParseHttpResponse` (#626) * Remove generic body from `ParseHttpResponse` * Update CHANGELOG --- CHANGELOG.md | 15 ++++++++- aws/rust-runtime/aws-hyper/tests/e2e_test.rs | 9 ++---- aws/sdk/integration-tests/dynamodb/Cargo.toml | 3 +- .../dynamodb/benches/deserialization_bench.rs | 3 +- .../generators/HttpProtocolTestGenerator.kt | 3 +- .../protocols/HttpBoundProtocolGenerator.kt | 2 +- .../HttpProtocolTestGeneratorTest.kt | 2 +- rust-runtime/smithy-client/src/bounds.rs | 4 +-- .../smithy-client/src/static_tests.rs | 2 +- .../smithy-http-tower/src/parse_response.rs | 2 +- rust-runtime/smithy-http/src/middleware.rs | 2 +- rust-runtime/smithy-http/src/response.rs | 32 ++++++++----------- 12 files changed, 41 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c59537bc17..89bf4331dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,23 @@ vNext (Month Day Year) ---------------------- +**Breaking changes** + +- (#626) `ParseHttpResponse` no longer has a generic argument for the body type, but instead, always uses `SdkBody`. This may cause compilation failures for you if you are using Smithy generated types to parse JSON or XML without using a client to request data from a service. The fix should be as simple as removing `` in the example below: + + Before: + ```rust + let output = >::parse_loaded(&parser, &response).unwrap(); + ``` + + After: + ```rust + let output = ::parse_loaded(&parser, &response).unwrap(); + ``` + **New This Week** - (When complete) Add profile file provider for region (#594, #xyz) - v0.19 (August 3rd, 2021) ------------------------ diff --git a/aws/rust-runtime/aws-hyper/tests/e2e_test.rs b/aws/rust-runtime/aws-hyper/tests/e2e_test.rs index 039adc2456..00dd0f97a6 100644 --- a/aws/rust-runtime/aws-hyper/tests/e2e_test.rs +++ b/aws/rust-runtime/aws-hyper/tests/e2e_test.rs @@ -54,14 +54,11 @@ impl ProvideErrorKind for OperationError { } } -impl ParseHttpResponse for TestOperationParser -where - B: http_body::Body, -{ +impl ParseHttpResponse for TestOperationParser { type Output = Result; - fn parse_unloaded(&self, _response: &mut Response) -> Option { - if _response.status().is_success() { + fn parse_unloaded(&self, response: &mut Response) -> Option { + if response.status().is_success() { Some(Ok("Hello!".to_string())) } else { Some(Err(OperationError)) diff --git a/aws/sdk/integration-tests/dynamodb/Cargo.toml b/aws/sdk/integration-tests/dynamodb/Cargo.toml index c7995ca687..8be40e6b4c 100644 --- a/aws/sdk/integration-tests/dynamodb/Cargo.toml +++ b/aws/sdk/integration-tests/dynamodb/Cargo.toml @@ -10,12 +10,13 @@ edition = "2018" [dependencies] aws-auth = { path = "../../build/aws-sdk/aws-auth" } aws-http = { path = "../../build/aws-sdk/aws-http" } -aws-hyper = { path = "../../build/aws-sdk/aws-hyper", features = ["test-util"] } +aws-hyper = { path = "../../build/aws-sdk/aws-hyper" } aws-sdk-dynamodb = { path = "../../build/aws-sdk/dynamodb" } bytes = "1" criterion = { version = "0.3.4" } http = "0.2.4" serde_json = "1" +smithy-client = { path = "../../build/aws-sdk/smithy-client", features = ["test-util"] } smithy-http = { path = "../../build/aws-sdk/smithy-http" } smithy-types = { path = "../../build/aws-sdk/smithy-types" } tokio = { version = "1", features = ["full", "test-util"]} diff --git a/aws/sdk/integration-tests/dynamodb/benches/deserialization_bench.rs b/aws/sdk/integration-tests/dynamodb/benches/deserialization_bench.rs index c12d3c2cd8..411f526273 100644 --- a/aws/sdk/integration-tests/dynamodb/benches/deserialization_bench.rs +++ b/aws/sdk/integration-tests/dynamodb/benches/deserialization_bench.rs @@ -6,7 +6,6 @@ use aws_sdk_dynamodb::operation::Query; use bytes::Bytes; use criterion::{criterion_group, criterion_main, Criterion}; -use smithy_http::body::SdkBody; use smithy_http::response::ParseHttpResponse; fn do_bench() { @@ -23,7 +22,7 @@ fn do_bench() { .unwrap(); let parser = Query::new(); - let output = >::parse_loaded(&parser, &response).unwrap(); + let output = ::parse_loaded(&parser, &response).unwrap(); assert_eq!(2, output.count); } diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/HttpProtocolTestGenerator.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/HttpProtocolTestGenerator.kt index a49c371774..95d7178fe4 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/HttpProtocolTestGenerator.kt +++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/HttpProtocolTestGenerator.kt @@ -277,14 +277,13 @@ class HttpProtocolTestGenerator( let parsed = parser.parse_unloaded(&mut http_response); let parsed = parsed.unwrap_or_else(|| { let http_response = http_response.map(|body|#{bytes}::copy_from_slice(body.bytes().unwrap())); - <#{op} as #{parse_http_response}<#{sdk_body}>>::parse_loaded(&parser, &http_response) + <#{op} as #{parse_http_response}>::parse_loaded(&parser, &http_response) }); """, "op" to operationSymbol, "bytes" to RuntimeType.Bytes, "parse_http_response" to CargoDependency.SmithyHttp(protocolConfig.runtimeConfig).asType() .member("response::ParseHttpResponse"), - "sdk_body" to RuntimeType.sdkBody(runtimeConfig = protocolConfig.runtimeConfig) ) if (expectedShape.hasTrait()) { val errorSymbol = operationShape.errorSymbol(protocolConfig.symbolProvider) diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/HttpBoundProtocolGenerator.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/HttpBoundProtocolGenerator.kt index dd631c188e..50faad534e 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/HttpBoundProtocolGenerator.kt +++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/HttpBoundProtocolGenerator.kt @@ -245,7 +245,7 @@ class HttpBoundProtocolGenerator( val successCode = httpBindingResolver.httpTrait(operationShape).code rustTemplate( """ - impl #{ParseResponse}<#{SdkBody}> for $operationName { + impl #{ParseResponse} for $operationName { type Output = std::result::Result<#{O}, #{E}>; fn parse_unloaded(&self, response: &mut http::Response<#{SdkBody}>) -> Option { // This is an error, defer to the non-streaming parser diff --git a/codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/HttpProtocolTestGeneratorTest.kt b/codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/HttpProtocolTestGeneratorTest.kt index 4d3fb9ac4c..bc49ca1b75 100644 --- a/codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/HttpProtocolTestGeneratorTest.kt +++ b/codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/HttpProtocolTestGeneratorTest.kt @@ -185,6 +185,7 @@ class HttpProtocolTestGeneratorTest { } ) visitor.execute() + println("file:///$testDir/src/operation.rs") return testDir } @@ -241,7 +242,6 @@ class HttpProtocolTestGeneratorTest { @Test fun `test invalid url parameter`() { - // Hard coded implementation for this 1 test val path = generateService( """ diff --git a/rust-runtime/smithy-client/src/bounds.rs b/rust-runtime/smithy-client/src/bounds.rs index 71026e090d..1b424e3b8a 100644 --- a/rust-runtime/smithy-client/src/bounds.rs +++ b/rust-runtime/smithy-client/src/bounds.rs @@ -119,7 +119,7 @@ pub trait SmithyRetryPolicy: /// Forwarding type to `O` for bound inference. /// /// See module-level docs for details. - type O: ParseHttpResponse> + Send + Sync + Clone + 'static; + type O: ParseHttpResponse> + Send + Sync + Clone + 'static; /// Forwarding type to `E` for bound inference. /// /// See module-level docs for details. @@ -134,7 +134,7 @@ pub trait SmithyRetryPolicy: impl SmithyRetryPolicy for R where R: tower::retry::Policy, SdkSuccess, SdkError> + Clone, - O: ParseHttpResponse> + Send + Sync + Clone + 'static, + O: ParseHttpResponse> + Send + Sync + Clone + 'static, E: Error + ProvideErrorKind, Retry: ClassifyResponse, SdkError>, { diff --git a/rust-runtime/smithy-client/src/static_tests.rs b/rust-runtime/smithy-client/src/static_tests.rs index ebad8c062c..e6cc6c95d2 100644 --- a/rust-runtime/smithy-client/src/static_tests.rs +++ b/rust-runtime/smithy-client/src/static_tests.rs @@ -24,7 +24,7 @@ impl ProvideErrorKind for TestOperationError { #[derive(Clone)] #[non_exhaustive] pub struct TestOperation; -impl ParseHttpResponse for TestOperation { +impl ParseHttpResponse for TestOperation { type Output = Result<(), TestOperationError>; fn parse_unloaded(&self, _: &mut http::Response) -> Option { diff --git a/rust-runtime/smithy-http-tower/src/parse_response.rs b/rust-runtime/smithy-http-tower/src/parse_response.rs index 52dc2025d3..6bc04f87ae 100644 --- a/rust-runtime/smithy-http-tower/src/parse_response.rs +++ b/rust-runtime/smithy-http-tower/src/parse_response.rs @@ -73,7 +73,7 @@ impl tower::Service> for ParseResponse where S: Service, Error = SendOperationError>, S::Future: Send + 'static, - O: ParseHttpResponse> + Send + Sync + 'static, + O: ParseHttpResponse> + Send + Sync + 'static, E: Error, { type Response = smithy_http::result::SdkSuccess; diff --git a/rust-runtime/smithy-http/src/middleware.rs b/rust-runtime/smithy-http/src/middleware.rs index 4416c97aba..56489e2f09 100644 --- a/rust-runtime/smithy-http/src/middleware.rs +++ b/rust-runtime/smithy-http/src/middleware.rs @@ -90,7 +90,7 @@ pub async fn load_response( handler: &O, ) -> Result, SdkError> where - O: ParseHttpResponse>, + O: ParseHttpResponse>, { if let Some(parsed_response) = handler.parse_unloaded(&mut response) { trace!(response = ?response); diff --git a/rust-runtime/smithy-http/src/response.rs b/rust-runtime/smithy-http/src/response.rs index b0585b22b3..920ee026b2 100644 --- a/rust-runtime/smithy-http/src/response.rs +++ b/rust-runtime/smithy-http/src/response.rs @@ -3,6 +3,7 @@ * SPDX-License-Identifier: Apache-2.0. */ +use crate::body::SdkBody; use bytes::Bytes; use http::Response; @@ -18,7 +19,7 @@ use http::Response; /// /// It also enables this critical and core trait to avoid being async, and it makes code that uses /// the trait easier to test. -pub trait ParseHttpResponse { +pub trait ParseHttpResponse { /// Output type of the HttpResponse. /// /// For request/response style operations, this is typically something like: @@ -44,7 +45,7 @@ pub trait ParseHttpResponse { /// the streaming body with an empty body as long as the body implements default. /// /// We should consider if this is too limiting & if this should take an owned response instead. - fn parse_unloaded(&self, response: &mut http::Response) -> Option; + fn parse_unloaded(&self, response: &mut http::Response) -> Option; /// Parse an HTTP request from a fully loaded body. This is for standard request/response style /// APIs like AwsJson 1.0/1.1 and the error path of most streaming APIs @@ -69,13 +70,10 @@ pub trait ParseStrictResponse { fn parse(&self, response: &Response) -> Self::Output; } -impl ParseHttpResponse for T -where - T: ParseStrictResponse, -{ +impl ParseHttpResponse for T { type Output = T::Output; - fn parse_unloaded(&self, _response: &mut Response) -> Option { + fn parse_unloaded(&self, _response: &mut Response) -> Option { None } @@ -86,30 +84,26 @@ where #[cfg(test)] mod test { + use crate::body::SdkBody; use crate::response::ParseHttpResponse; use bytes::Bytes; use http::Response; - use http_body::Body; use std::mem; #[test] fn supports_streaming_body() { - pub struct S3GetObject { - pub body: B, + pub struct S3GetObject { + pub body: SdkBody, } struct S3GetObjectParser; - impl ParseHttpResponse for S3GetObjectParser - where - B: Default + Body, - { - type Output = S3GetObject; + impl ParseHttpResponse for S3GetObjectParser { + type Output = S3GetObject; - fn parse_unloaded(&self, response: &mut Response) -> Option { - // For responses that pass on the body, use mem::take to leave behind an empty - // body - let body = mem::take(response.body_mut()); + fn parse_unloaded(&self, response: &mut Response) -> Option { + // For responses that pass on the body, use mem::take to leave behind an empty body + let body = mem::replace(response.body_mut(), SdkBody::taken()); Some(S3GetObject { body }) }