From 10a1967f03f7c1045c8eb433faea75a0efe0b7ef Mon Sep 17 00:00:00 2001 From: Yuki Saito Date: Tue, 18 Apr 2023 13:39:38 -0500 Subject: [PATCH 1/5] Avoid using `InterceptorError` in `Interceptor` impl This commit incorporates the following feedback: https://github.com/awslabs/smithy-rs/pull/2577#discussion_r1169362663 https://github.com/awslabs/smithy-rs/pull/2577#discussion_r1169365131 https://github.com/awslabs/smithy-rs/pull/2577#discussion_r1169366987 https://github.com/awslabs/smithy-rs/pull/2577#discussion_r1169367069 --- .../generators/EndpointParamsInterceptorGenerator.kt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/generators/EndpointParamsInterceptorGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/generators/EndpointParamsInterceptorGenerator.kt index 8343bbc9bdd..ff7c57cb623 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/generators/EndpointParamsInterceptorGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/generators/EndpointParamsInterceptorGenerator.kt @@ -83,10 +83,10 @@ class EndpointParamsInterceptorGenerator( let input = context.input()?; let _input = input .downcast_ref::<${operationInput.name}>() - .ok_or_else(|| #{InterceptorError}::invalid_input_access())?; + .ok_or_else(|| "failed to downcast to ${operationInput.name}")?; let params_builder = cfg .get::<#{ParamsBuilder}>() - .ok_or(#{InterceptorError}::read_before_execution("missing endpoint params builder"))? + .ok_or_else(|| "missing endpoint params builder".to_owned())? .clone(); ${"" /* TODO(EndpointResolver): Call setters on `params_builder` to update its fields by using values from `_input` */} cfg.put(params_builder); @@ -111,7 +111,7 @@ class EndpointParamsInterceptorGenerator( ) withBlockTemplate( "let endpoint_prefix = ", - ".map_err(#{InterceptorError}::read_before_execution)?;", + """.map_err(|err| format!("endpoint prefix could not be built: {err:?}"))?;""", *codegenScope, ) { endpointTraitBindings.render( @@ -130,11 +130,11 @@ class EndpointParamsInterceptorGenerator( let _ = context; let params_builder = cfg .get::<#{ParamsBuilder}>() - .ok_or(#{InterceptorError}::read_before_execution("missing endpoint params builder"))? + .ok_or_else(|| "missing endpoint params builder".to_owned())? .clone(); let params = params_builder .build() - .map_err(#{InterceptorError}::read_before_execution)?; + .map_err(|err| format!("endpoint params could not be built: {err:?}"))?; cfg.put( #{EndpointResolverParams}::new(params) ); From 82197857c96866125b9ef3eb7a4fbdc1f23f81eb Mon Sep 17 00:00:00 2001 From: Yuki Saito Date: Tue, 18 Apr 2023 14:54:25 -0500 Subject: [PATCH 2/5] Move concrete endpoint resolver types to `runtime-api` This commit addresses https://github.com/awslabs/smithy-rs/pull/2577#discussion_r1169370073 --- .../aws-sdk-s3/tests/sra_test.rs | 2 +- .../ServiceRuntimePluginGenerator.kt | 2 +- .../aws-smithy-runtime-api/src/client.rs | 3 - .../src/client/endpoints.rs | 110 ------------------ .../src/client/orchestrator.rs | 3 +- .../src/client/orchestrator/endpoints.rs | 106 ++++++++++++++++- 6 files changed, 108 insertions(+), 118 deletions(-) delete mode 100644 rust-runtime/aws-smithy-runtime-api/src/client/endpoints.rs diff --git a/aws/sra-test/integration-tests/aws-sdk-s3/tests/sra_test.rs b/aws/sra-test/integration-tests/aws-sdk-s3/tests/sra_test.rs index da778c3737b..bb799ecf3fd 100644 --- a/aws/sra-test/integration-tests/aws-sdk-s3/tests/sra_test.rs +++ b/aws/sra-test/integration-tests/aws-sdk-s3/tests/sra_test.rs @@ -17,7 +17,7 @@ use aws_sdk_s3::primitives::SdkBody; use aws_smithy_client::erase::DynConnector; use aws_smithy_client::test_connection::TestConnection; use aws_smithy_runtime::client::connections::adapter::DynConnectorAdapter; -use aws_smithy_runtime_api::client::endpoints::DefaultEndpointResolver; +use aws_smithy_runtime::client::orchestrator::endpoints::DefaultEndpointResolver; use aws_smithy_runtime_api::client::interceptors::{ Interceptor, InterceptorContext, InterceptorError, Interceptors, }; diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ServiceRuntimePluginGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ServiceRuntimePluginGenerator.kt index acb93bbfa73..8b70a847859 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ServiceRuntimePluginGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ServiceRuntimePluginGenerator.kt @@ -86,7 +86,7 @@ class ServiceRuntimePluginGenerator( "ConfigBagAccessors" to runtimeApi.resolve("client::orchestrator::ConfigBagAccessors"), "Connection" to runtimeApi.resolve("client::orchestrator::Connection"), "ConnectorSettings" to RuntimeType.smithyClient(rc).resolve("http_connector::ConnectorSettings"), - "DefaultEndpointResolver" to runtimeApi.resolve("client::endpoints::DefaultEndpointResolver"), + "DefaultEndpointResolver" to runtime.resolve("client::orchestrator::endpoints::DefaultEndpointResolver"), "DynConnectorAdapter" to runtime.resolve("client::connections::adapter::DynConnectorAdapter"), "HttpAuthSchemes" to runtimeApi.resolve("client::orchestrator::HttpAuthSchemes"), "IdentityResolvers" to runtimeApi.resolve("client::orchestrator::IdentityResolvers"), diff --git a/rust-runtime/aws-smithy-runtime-api/src/client.rs b/rust-runtime/aws-smithy-runtime-api/src/client.rs index c6724ff251c..192b97a5d4e 100644 --- a/rust-runtime/aws-smithy-runtime-api/src/client.rs +++ b/rust-runtime/aws-smithy-runtime-api/src/client.rs @@ -21,8 +21,5 @@ pub mod retries; /// Runtime plugin type definitions. pub mod runtime_plugin; -/// Smithy endpoint resolution runtime plugins -pub mod endpoints; - /// Smithy auth runtime plugins pub mod auth; diff --git a/rust-runtime/aws-smithy-runtime-api/src/client/endpoints.rs b/rust-runtime/aws-smithy-runtime-api/src/client/endpoints.rs deleted file mode 100644 index b9b4f4a512b..00000000000 --- a/rust-runtime/aws-smithy-runtime-api/src/client/endpoints.rs +++ /dev/null @@ -1,110 +0,0 @@ -/* - * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. - * SPDX-License-Identifier: Apache-2.0 - */ - -use crate::client::orchestrator::{ - BoxError, EndpointResolver, EndpointResolverParams, HttpRequest, -}; -use aws_smithy_http::endpoint::error::ResolveEndpointError; -use aws_smithy_http::endpoint::{ - apply_endpoint, EndpointPrefix, ResolveEndpoint, SharedEndpointResolver, -}; -use http::header::HeaderName; -use http::{HeaderValue, Uri}; -use std::fmt::Debug; -use std::str::FromStr; - -#[derive(Debug, Clone)] -pub struct StaticUriEndpointResolver { - endpoint: Uri, -} - -impl StaticUriEndpointResolver { - pub fn http_localhost(port: u16) -> Self { - Self { - endpoint: Uri::from_str(&format!("http://localhost:{port}")) - .expect("all u16 values are valid ports"), - } - } - - pub fn uri(endpoint: Uri) -> Self { - Self { endpoint } - } -} - -impl EndpointResolver for StaticUriEndpointResolver { - fn resolve_and_apply_endpoint( - &self, - _params: &EndpointResolverParams, - _endpoint_prefix: Option<&EndpointPrefix>, - request: &mut HttpRequest, - ) -> Result<(), BoxError> { - apply_endpoint(request.uri_mut(), &self.endpoint, None)?; - Ok(()) - } -} - -#[derive(Debug, Clone)] -pub struct DefaultEndpointResolver { - inner: SharedEndpointResolver, -} - -impl DefaultEndpointResolver { - pub fn new(resolve_endpoint: SharedEndpointResolver) -> Self { - Self { - inner: resolve_endpoint, - } - } -} - -impl EndpointResolver for DefaultEndpointResolver -where - Params: Debug + Send + Sync + 'static, -{ - fn resolve_and_apply_endpoint( - &self, - params: &EndpointResolverParams, - endpoint_prefix: Option<&EndpointPrefix>, - request: &mut HttpRequest, - ) -> Result<(), BoxError> { - let endpoint = match params.get::() { - Some(params) => self.inner.resolve_endpoint(params)?, - None => { - return Err(Box::new(ResolveEndpointError::message( - "params of expected type was not present", - ))); - } - }; - - let uri: Uri = endpoint.url().parse().map_err(|err| { - ResolveEndpointError::from_source("endpoint did not have a valid uri", err) - })?; - - apply_endpoint(request.uri_mut(), &uri, endpoint_prefix).map_err(|err| { - ResolveEndpointError::message(format!( - "failed to apply endpoint `{:?}` to request `{:?}`", - uri, request, - )) - .with_source(Some(err.into())) - })?; - - for (header_name, header_values) in endpoint.headers() { - request.headers_mut().remove(header_name); - for value in header_values { - request.headers_mut().insert( - HeaderName::from_str(header_name).map_err(|err| { - ResolveEndpointError::message("invalid header name") - .with_source(Some(err.into())) - })?, - HeaderValue::from_str(value).map_err(|err| { - ResolveEndpointError::message("invalid header value") - .with_source(Some(err.into())) - })?, - ); - } - } - - Ok(()) - } -} diff --git a/rust-runtime/aws-smithy-runtime/src/client/orchestrator.rs b/rust-runtime/aws-smithy-runtime/src/client/orchestrator.rs index 217a0eaae7b..dff37ec7b0b 100644 --- a/rust-runtime/aws-smithy-runtime/src/client/orchestrator.rs +++ b/rust-runtime/aws-smithy-runtime/src/client/orchestrator.rs @@ -18,7 +18,8 @@ use aws_smithy_runtime_api::config_bag::ConfigBag; use tracing::{debug_span, Instrument}; mod auth; -mod endpoints; +/// Defines types that implement a trait for endpoint resolution +pub mod endpoints; mod http; pub(self) mod phase; diff --git a/rust-runtime/aws-smithy-runtime/src/client/orchestrator/endpoints.rs b/rust-runtime/aws-smithy-runtime/src/client/orchestrator/endpoints.rs index 5da7bfe5173..981ef3239cc 100644 --- a/rust-runtime/aws-smithy-runtime/src/client/orchestrator/endpoints.rs +++ b/rust-runtime/aws-smithy-runtime/src/client/orchestrator/endpoints.rs @@ -3,12 +3,114 @@ * SPDX-License-Identifier: Apache-2.0 */ -use aws_smithy_http::endpoint::EndpointPrefix; +use aws_smithy_http::endpoint::error::ResolveEndpointError; +use aws_smithy_http::endpoint::{ + apply_endpoint, EndpointPrefix, ResolveEndpoint, SharedEndpointResolver, +}; use aws_smithy_runtime_api::client::interceptors::InterceptorContext; use aws_smithy_runtime_api::client::orchestrator::{ - BoxError, ConfigBagAccessors, HttpRequest, HttpResponse, + BoxError, ConfigBagAccessors, EndpointResolver, EndpointResolverParams, HttpRequest, + HttpResponse, }; use aws_smithy_runtime_api::config_bag::ConfigBag; +use http::header::HeaderName; +use http::{HeaderValue, Uri}; +use std::fmt::Debug; +use std::str::FromStr; + +#[derive(Debug, Clone)] +pub struct StaticUriEndpointResolver { + endpoint: Uri, +} + +impl StaticUriEndpointResolver { + pub fn http_localhost(port: u16) -> Self { + Self { + endpoint: Uri::from_str(&format!("http://localhost:{port}")) + .expect("all u16 values are valid ports"), + } + } + + pub fn uri(endpoint: Uri) -> Self { + Self { endpoint } + } +} + +impl EndpointResolver for StaticUriEndpointResolver { + fn resolve_and_apply_endpoint( + &self, + _params: &EndpointResolverParams, + _endpoint_prefix: Option<&EndpointPrefix>, + request: &mut HttpRequest, + ) -> Result<(), BoxError> { + apply_endpoint(request.uri_mut(), &self.endpoint, None)?; + Ok(()) + } +} + +#[derive(Debug, Clone)] +pub struct DefaultEndpointResolver { + inner: SharedEndpointResolver, +} + +impl DefaultEndpointResolver { + pub fn new(resolve_endpoint: SharedEndpointResolver) -> Self { + Self { + inner: resolve_endpoint, + } + } +} + +impl EndpointResolver for DefaultEndpointResolver +where + Params: Debug + Send + Sync + 'static, +{ + fn resolve_and_apply_endpoint( + &self, + params: &EndpointResolverParams, + endpoint_prefix: Option<&EndpointPrefix>, + request: &mut HttpRequest, + ) -> Result<(), BoxError> { + let endpoint = match params.get::() { + Some(params) => self.inner.resolve_endpoint(params)?, + None => { + return Err(Box::new(ResolveEndpointError::message( + "params of expected type was not present", + ))); + } + }; + + let uri: Uri = endpoint.url().parse().map_err(|err| { + ResolveEndpointError::from_source("endpoint did not have a valid uri", err) + })?; + + apply_endpoint(request.uri_mut(), &uri, endpoint_prefix).map_err(|err| { + ResolveEndpointError::message(format!( + "failed to apply endpoint `{:?}` to request `{:?}`", + uri, request, + )) + .with_source(Some(err.into())) + })?; + + for (header_name, header_values) in endpoint.headers() { + request.headers_mut().remove(header_name); + for value in header_values { + request.headers_mut().insert( + HeaderName::from_str(header_name).map_err(|err| { + ResolveEndpointError::message("invalid header name") + .with_source(Some(err.into())) + })?, + HeaderValue::from_str(value).map_err(|err| { + ResolveEndpointError::message("invalid header value") + .with_source(Some(err.into())) + })?, + ); + } + } + + Ok(()) + } +} pub(super) fn orchestrate_endpoint( ctx: &mut InterceptorContext, From 51026e9df27204cce09351675a70003c1f36aa0d Mon Sep 17 00:00:00 2001 From: Yuki Saito Date: Tue, 18 Apr 2023 20:18:56 -0500 Subject: [PATCH 3/5] Add a convenience error to attach context to existing error This commit incorporates https://github.com/awslabs/smithy-rs/pull/2592#discussion_r1170527208 --- .../EndpointParamsInterceptorGenerator.kt | 9 +++--- .../src/client/interceptors/error.rs | 28 +++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/generators/EndpointParamsInterceptorGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/generators/EndpointParamsInterceptorGenerator.kt index ff7c57cb623..d7fbe5f27db 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/generators/EndpointParamsInterceptorGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/generators/EndpointParamsInterceptorGenerator.kt @@ -32,6 +32,7 @@ class EndpointParamsInterceptorGenerator( arrayOf( "BoxError" to runtimeApi.resolve("client::runtime_plugin::BoxError"), "ConfigBag" to runtimeApi.resolve("config_bag::ConfigBag"), + "ContextAttachedError" to interceptors.resolve("error::ContextAttachedError"), "EndpointResolverParams" to orchestrator.resolve("EndpointResolverParams"), "HttpResponse" to orchestrator.resolve("HttpResponse"), "HttpRequest" to orchestrator.resolve("HttpRequest"), @@ -86,7 +87,7 @@ class EndpointParamsInterceptorGenerator( .ok_or_else(|| "failed to downcast to ${operationInput.name}")?; let params_builder = cfg .get::<#{ParamsBuilder}>() - .ok_or_else(|| "missing endpoint params builder".to_owned())? + .ok_or_else(|| "missing endpoint params builder")? .clone(); ${"" /* TODO(EndpointResolver): Call setters on `params_builder` to update its fields by using values from `_input` */} cfg.put(params_builder); @@ -111,7 +112,7 @@ class EndpointParamsInterceptorGenerator( ) withBlockTemplate( "let endpoint_prefix = ", - """.map_err(|err| format!("endpoint prefix could not be built: {err:?}"))?;""", + """.map_err(|err| #{ContextAttachedError}::new("endpoint prefix could not be built", err.into()))?;""", *codegenScope, ) { endpointTraitBindings.render( @@ -130,11 +131,11 @@ class EndpointParamsInterceptorGenerator( let _ = context; let params_builder = cfg .get::<#{ParamsBuilder}>() - .ok_or_else(|| "missing endpoint params builder".to_owned())? + .ok_or_else(|| "missing endpoint params builder")? .clone(); let params = params_builder .build() - .map_err(|err| format!("endpoint params could not be built: {err:?}"))?; + .map_err(|err| #{ContextAttachedError}::new("endpoint params could not be built", err.into()))?; cfg.put( #{EndpointResolverParams}::new(params) ); diff --git a/rust-runtime/aws-smithy-runtime-api/src/client/interceptors/error.rs b/rust-runtime/aws-smithy-runtime-api/src/client/interceptors/error.rs index e25b797aebb..44eb7665251 100644 --- a/rust-runtime/aws-smithy-runtime-api/src/client/interceptors/error.rs +++ b/rust-runtime/aws-smithy-runtime-api/src/client/interceptors/error.rs @@ -169,3 +169,31 @@ impl std::error::Error for InterceptorError { self.source.as_ref().map(|err| err.as_ref() as _) } } + +/// A convenience error that allows for adding additional `context` to `source` +#[derive(Debug)] +pub struct ContextAttachedError { + context: String, + source: Option, +} + +impl ContextAttachedError { + pub fn new(context: impl Into, source: BoxError) -> Self { + Self { + context: context.into(), + source: Some(source), + } + } +} + +impl fmt::Display for ContextAttachedError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.context) + } +} + +impl std::error::Error for ContextAttachedError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + self.source.as_ref().map(|err| err.as_ref() as _) + } +} From b3c22f380ea7f2d2a3d9544a3a18de9d2f58cad0 Mon Sep 17 00:00:00 2001 From: Yuki Saito Date: Tue, 18 Apr 2023 20:24:15 -0500 Subject: [PATCH 4/5] Update external-types.toml --- rust-runtime/aws-smithy-runtime/external-types.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust-runtime/aws-smithy-runtime/external-types.toml b/rust-runtime/aws-smithy-runtime/external-types.toml index 5ecb3a376bb..a8a47f10d99 100644 --- a/rust-runtime/aws-smithy-runtime/external-types.toml +++ b/rust-runtime/aws-smithy-runtime/external-types.toml @@ -6,5 +6,5 @@ allowed_external_types = [ "http::header::name::HeaderName", "http::request::Request", "http::response::Response", - "uri::Uri", + "http::uri::Uri", ] From 93d044dff09813cbef3d2291c03ae8cb5589d6c7 Mon Sep 17 00:00:00 2001 From: Yuki Saito Date: Wed, 19 Apr 2023 18:32:59 -0500 Subject: [PATCH 5/5] Make call sites of `ContextAttachedError::new` ergonomic This commit addresses https://github.com/awslabs/smithy-rs/pull/2592#discussion_r1171627755 --- .../endpoint/generators/EndpointParamsInterceptorGenerator.kt | 4 ++-- .../aws-smithy-runtime-api/src/client/interceptors/error.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/generators/EndpointParamsInterceptorGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/generators/EndpointParamsInterceptorGenerator.kt index d7fbe5f27db..0299eb21c76 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/generators/EndpointParamsInterceptorGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/generators/EndpointParamsInterceptorGenerator.kt @@ -112,7 +112,7 @@ class EndpointParamsInterceptorGenerator( ) withBlockTemplate( "let endpoint_prefix = ", - """.map_err(|err| #{ContextAttachedError}::new("endpoint prefix could not be built", err.into()))?;""", + """.map_err(|err| #{ContextAttachedError}::new("endpoint prefix could not be built", err))?;""", *codegenScope, ) { endpointTraitBindings.render( @@ -135,7 +135,7 @@ class EndpointParamsInterceptorGenerator( .clone(); let params = params_builder .build() - .map_err(|err| #{ContextAttachedError}::new("endpoint params could not be built", err.into()))?; + .map_err(|err| #{ContextAttachedError}::new("endpoint params could not be built", err))?; cfg.put( #{EndpointResolverParams}::new(params) ); diff --git a/rust-runtime/aws-smithy-runtime-api/src/client/interceptors/error.rs b/rust-runtime/aws-smithy-runtime-api/src/client/interceptors/error.rs index 44eb7665251..8d8b8289715 100644 --- a/rust-runtime/aws-smithy-runtime-api/src/client/interceptors/error.rs +++ b/rust-runtime/aws-smithy-runtime-api/src/client/interceptors/error.rs @@ -178,10 +178,10 @@ pub struct ContextAttachedError { } impl ContextAttachedError { - pub fn new(context: impl Into, source: BoxError) -> Self { + pub fn new(context: impl Into, source: impl Into) -> Self { Self { context: context.into(), - source: Some(source), + source: Some(source.into()), } } }