Improve Endpoint panic-safety and ergonomics (#1984)

jdisanti · web-flow · commit 06c23f65cc54 · 2022-11-17T00:17:22.000Z
- `Endpoint::set_endpoint` no longer panics when called on an endpoint
  without a scheme
- `Endpoint::mutable` and `Endpoint::immutable` now both return a result
  so that constructing an endpoint without a scheme is an error
- `Endpoint::mutable` and `Endpoint::immutable` both now take a string
  instead of a `Uri` as a convenience
- `Endpoint::mutable_uri` and `Endpoint::immutable_uri` were added
  to construct an endpoint directly from a `Uri`
diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml
@@ -395,3 +395,39 @@ This is forward-compatible because the execution will hit the second last match
 references = ["smithy-rs#1945"]
 meta = { "breaking" = true, "tada" = false, "bug" = false }
 author = "ysaito1001"
+
+[[aws-sdk-rust]]
+message = "Functions on `aws_smithy_http::endpoint::Endpoint` now return a `Result` instead of panicking."
+references = ["smithy-rs#1984", "smithy-rs#1496"]
+meta = { "breaking" = true, "tada" = false, "bug" = false }
+author = "jdisanti"
+
+[[smithy-rs]]
+message = "Functions on `aws_smithy_http::endpoint::Endpoint` now return a `Result` instead of panicking."
+references = ["smithy-rs#1984", "smithy-rs#1496"]
+meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
+author = "jdisanti"
+
+[[aws-sdk-rust]]
+message = "`Endpoint::mutable` now takes `impl AsRef<str>` instead of `Uri`. For the old functionality, use `Endpoint::mutable_uri`."
+references = ["smithy-rs#1984", "smithy-rs#1496"]
+meta = { "breaking" = true, "tada" = false, "bug" = false }
+author = "jdisanti"
+
+[[smithy-rs]]
+message = "`Endpoint::mutable` now takes `impl AsRef<str>` instead of `Uri`. For the old functionality, use `Endpoint::mutable_uri`."
+references = ["smithy-rs#1984", "smithy-rs#1496"]
+meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
+author = "jdisanti"
+
+[[aws-sdk-rust]]
+message = "`Endpoint::immutable` now takes `impl AsRef<str>` instead of `Uri`. For the old functionality, use `Endpoint::immutable_uri`."
+references = ["smithy-rs#1984", "smithy-rs#1496"]
+meta = { "breaking" = true, "tada" = false, "bug" = false }
+author = "jdisanti"
+
+[[smithy-rs]]
+message = "`Endpoint::immutable` now takes `impl AsRef<str>` instead of `Uri`. For the old functionality, use `Endpoint::immutable_uri`."
+references = ["smithy-rs#1984", "smithy-rs#1496"]
+meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
+author = "jdisanti"
diff --git a/aws/rust-runtime/aws-config/external-types.toml b/aws/rust-runtime/aws-config/external-types.toml
@@ -8,11 +8,12 @@ allowed_external_types = [
    "aws_smithy_client::bounds::SmithyConnector",
    "aws_smithy_client::erase::DynConnector",
    "aws_smithy_client::erase::boxclone::BoxCloneService",
-   "aws_smithy_client::http_connector::HttpConnector",
    "aws_smithy_client::http_connector::ConnectorSettings",
+   "aws_smithy_client::http_connector::HttpConnector",
    "aws_smithy_http::body::SdkBody",
-   "aws_smithy_http::result::SdkError",
    "aws_smithy_http::endpoint",
+   "aws_smithy_http::endpoint::error::InvalidEndpointError",
+   "aws_smithy_http::result::SdkError",
    "aws_smithy_types::retry",
    "aws_smithy_types::retry::*",
    "aws_smithy_types::timeout",
diff --git a/aws/rust-runtime/aws-config/src/ecs.rs b/aws/rust-runtime/aws-config/src/ecs.rs
@@ -190,8 +190,11 @@ impl Provider {
                 });
             }
         };
-        let endpoint = Endpoint::immutable(Uri::from_static(BASE_HOST));
-        endpoint.set_endpoint(&mut relative_uri, None);
+        let endpoint =
+            Endpoint::immutable_uri(Uri::from_static(BASE_HOST)).expect("BASE_HOST is valid");
+        endpoint
+            .set_endpoint(&mut relative_uri, None)
+            .expect("appending relative URLs to the ECS endpoint should always succeed");
         Ok(relative_uri)
     }
 }
diff --git a/aws/rust-runtime/aws-config/src/imds/client.rs b/aws/rust-runtime/aws-config/src/imds/client.rs
@@ -8,9 +8,7 @@
 //! Client for direct access to IMDSv2.
 
 use crate::connector::expect_connector;
-use crate::imds::client::error::{
-    BuildError, BuildErrorKind, ImdsError, InnerImdsError, InvalidEndpointMode,
-};
+use crate::imds::client::error::{BuildError, ImdsError, InnerImdsError, InvalidEndpointMode};
 use crate::imds::client::token::TokenMiddleware;
 use crate::provider_config::ProviderConfig;
 use crate::{profile, PKG_VERSION};
@@ -237,7 +235,10 @@ impl Client {
         let mut base_uri: Uri = path.parse().map_err(|_| {
             ImdsError::unexpected("IMDS path was not a valid URI. Hint: does it begin with `/`?")
         })?;
-        self.inner.endpoint.set_endpoint(&mut base_uri, None);
+        self.inner
+            .endpoint
+            .set_endpoint(&mut base_uri, None)
+            .map_err(ImdsError::unexpected)?;
         let request = http::Request::builder()
             .uri(base_uri)
             .body(SdkBody::empty())
@@ -433,7 +434,7 @@ impl Builder {
             .endpoint
             .unwrap_or_else(|| EndpointSource::Env(config.env(), config.fs()));
         let endpoint = endpoint_source.endpoint(self.mode_override).await?;
-        let endpoint = Endpoint::immutable(endpoint);
+        let endpoint = Endpoint::immutable_uri(endpoint)?;
         let retry_config = retry::Config::default()
             .with_max_attempts(self.max_attempts.unwrap_or(DEFAULT_ATTEMPTS));
         let token_loader = token::TokenMiddleware::new(
@@ -496,27 +497,25 @@ impl EndpointSource {
                 // load an endpoint override from the environment
                 let profile = profile::load(fs, env, &Default::default())
                     .await
-                    .map_err(BuildErrorKind::InvalidProfile)?;
+                    .map_err(BuildError::invalid_profile)?;
                 let uri_override = if let Ok(uri) = env.get(env::ENDPOINT) {
                     Some(Cow::Owned(uri))
                 } else {
                     profile.get(profile_keys::ENDPOINT).map(Cow::Borrowed)
                 };
                 if let Some(uri) = uri_override {
-                    return Ok(
-                        Uri::try_from(uri.as_ref()).map_err(BuildErrorKind::InvalidEndpointUri)?
-                    );
+                    return Uri::try_from(uri.as_ref()).map_err(BuildError::invalid_endpoint_uri);
                 }
 
                 // if not, load a endpoint mode from the environment
                 let mode = if let Some(mode) = mode_override {
                     mode
                 } else if let Ok(mode) = env.get(env::ENDPOINT_MODE) {
                     mode.parse::<EndpointMode>()
-                        .map_err(BuildErrorKind::InvalidEndpointMode)?
+                        .map_err(BuildError::invalid_endpoint_mode)?
                 } else if let Some(mode) = profile.get(profile_keys::ENDPOINT_MODE) {
                     mode.parse::<EndpointMode>()
-                        .map_err(BuildErrorKind::InvalidEndpointMode)?
+                        .map_err(BuildError::invalid_endpoint_mode)?
                 } else {
                     EndpointMode::IpV4
                 };
diff --git a/aws/rust-runtime/aws-config/src/imds/client/error.rs b/aws/rust-runtime/aws-config/src/imds/client/error.rs
@@ -8,7 +8,7 @@
 use crate::profile::credentials::ProfileFileError;
 use aws_smithy_client::SdkError;
 use aws_smithy_http::body::SdkBody;
-use http::uri::InvalidUri;
+use aws_smithy_http::endpoint::error::InvalidEndpointError;
 use std::error::Error;
 use std::fmt;
 
@@ -174,15 +174,15 @@ impl Error for InvalidEndpointMode {}
 
 #[derive(Debug)]
 #[allow(clippy::enum_variant_names)]
-pub(super) enum BuildErrorKind {
+enum BuildErrorKind {
     /// The endpoint mode was invalid
     InvalidEndpointMode(InvalidEndpointMode),
 
     /// The AWS Profile (e.g. `~/.aws/config`) was invalid
     InvalidProfile(ProfileFileError),
 
     /// The specified endpoint was not a valid URI
-    InvalidEndpointUri(InvalidUri),
+    InvalidEndpointUri(Box<dyn Error + Send + Sync + 'static>),
 }
 
 /// Error constructing IMDSv2 Client
@@ -191,6 +191,28 @@ pub struct BuildError {
     kind: BuildErrorKind,
 }
 
+impl BuildError {
+    pub(super) fn invalid_endpoint_mode(source: InvalidEndpointMode) -> Self {
+        Self {
+            kind: BuildErrorKind::InvalidEndpointMode(source),
+        }
+    }
+
+    pub(super) fn invalid_profile(source: ProfileFileError) -> Self {
+        Self {
+            kind: BuildErrorKind::InvalidProfile(source),
+        }
+    }
+
+    pub(super) fn invalid_endpoint_uri(
+        source: impl Into<Box<dyn Error + Send + Sync + 'static>>,
+    ) -> Self {
+        Self {
+            kind: BuildErrorKind::InvalidEndpointUri(source.into()),
+        }
+    }
+}
+
 impl fmt::Display for BuildError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::fmt::Result {
         use BuildErrorKind::*;
@@ -209,14 +231,16 @@ impl Error for BuildError {
         match &self.kind {
             InvalidEndpointMode(e) => Some(e),
             InvalidProfile(e) => Some(e),
-            InvalidEndpointUri(e) => Some(e),
+            InvalidEndpointUri(e) => Some(e.as_ref()),
         }
     }
 }
 
-impl From<BuildErrorKind> for BuildError {
-    fn from(kind: BuildErrorKind) -> Self {
-        Self { kind }
+impl From<InvalidEndpointError> for BuildError {
+    fn from(err: InvalidEndpointError) -> Self {
+        Self {
+            kind: BuildErrorKind::InvalidEndpointUri(err.into()),
+        }
     }
 }
 
diff --git a/aws/rust-runtime/aws-config/src/imds/client/token.rs b/aws/rust-runtime/aws-config/src/imds/client/token.rs
@@ -128,7 +128,9 @@ impl TokenMiddleware {
 
     async fn get_token(&self) -> Result<(Token, SystemTime), ImdsError> {
         let mut uri = Uri::from_static("/latest/api/token");
-        self.endpoint.set_endpoint(&mut uri, None);
+        self.endpoint
+            .set_endpoint(&mut uri, None)
+            .map_err(ImdsError::unexpected)?;
         let request = http::Request::builder()
             .header(
                 X_AWS_EC2_METADATA_TOKEN_TTL_SECONDS,
diff --git a/aws/rust-runtime/aws-config/src/lib.rs b/aws/rust-runtime/aws-config/src/lib.rs
@@ -323,13 +323,14 @@ mod loader {
         ///
         /// Use a static endpoint for all services
         /// ```no_run
-        /// # async fn create_config() {
+        /// # async fn create_config() -> Result<(), aws_smithy_http::endpoint::error::InvalidEndpointError> {
         /// use aws_config::endpoint::Endpoint;
         ///
         /// let sdk_config = aws_config::from_env()
-        ///     .endpoint_resolver(Endpoint::immutable("http://localhost:1234".parse().expect("valid URI")))
+        ///     .endpoint_resolver(Endpoint::immutable("http://localhost:1234")?)
         ///     .load()
         ///     .await;
+        /// # Ok(())
         /// # }
         pub fn endpoint_resolver(
             mut self,
diff --git a/aws/rust-runtime/aws-endpoint/src/partition/endpoint.rs b/aws/rust-runtime/aws-endpoint/src/partition/endpoint.rs
@@ -54,7 +54,7 @@ impl ResolveAwsEndpoint for Metadata {
     fn resolve_endpoint(&self, region: &Region) -> Result<AwsEndpoint, BoxError> {
         let uri = self.uri_template.replace("{region}", region.as_ref());
         let uri = format!("{}://{}", self.protocol.as_str(), uri);
-        let endpoint = Endpoint::mutable(uri.parse()?);
+        let endpoint = Endpoint::mutable(uri)?;
         let mut credential_scope = CredentialScope::builder().region(
             self.credential_scope
                 .region()
diff --git a/aws/rust-runtime/aws-endpoint/src/partition/mod.rs b/aws/rust-runtime/aws-endpoint/src/partition/mod.rs
@@ -373,7 +373,7 @@ mod test {
             .resolve_endpoint(&Region::new(test_case.region))
             .expect("valid region");
         let mut test_uri = Uri::from_static("/");
-        endpoint.set_endpoint(&mut test_uri, None);
+        endpoint.set_endpoint(&mut test_uri, None).unwrap();
         assert_eq!(test_uri, Uri::from_static(test_case.uri));
         assert_eq!(
             endpoint.credential_scope().region(),
diff --git a/aws/rust-runtime/aws-types/external-types.toml b/aws/rust-runtime/aws-types/external-types.toml
@@ -4,6 +4,7 @@ allowed_external_types = [
     "aws_smithy_client::http_connector::HttpConnector",
     "aws_smithy_http::endpoint::Endpoint",
     "aws_smithy_http::endpoint::EndpointPrefix",
+    "aws_smithy_http::endpoint::error::InvalidEndpointError",
     "aws_smithy_types::retry::RetryConfig",
     "aws_smithy_types::timeout::TimeoutConfig",
     "http::uri::Uri",
diff --git a/aws/rust-runtime/aws-types/src/endpoint.rs b/aws/rust-runtime/aws-types/src/endpoint.rs
@@ -7,6 +7,7 @@
 
 use crate::region::{Region, SigningRegion};
 use crate::SigningService;
+use aws_smithy_http::endpoint::error::InvalidEndpointError;
 use aws_smithy_http::endpoint::{Endpoint, EndpointPrefix};
 use std::error::Error;
 use std::fmt::Debug;
@@ -43,8 +44,12 @@ impl AwsEndpoint {
     }
 
     /// Sets the endpoint on a given `uri` based on this endpoint
-    pub fn set_endpoint(&self, uri: &mut http::Uri, endpoint_prefix: Option<&EndpointPrefix>) {
-        self.endpoint.set_endpoint(uri, endpoint_prefix);
+    pub fn set_endpoint(
+        &self,
+        uri: &mut http::Uri,
+        endpoint_prefix: Option<&EndpointPrefix>,
+    ) -> Result<(), InvalidEndpointError> {
+        self.endpoint.set_endpoint(uri, endpoint_prefix)
     }
 }
 
@@ -71,12 +76,12 @@ pub type BoxError = Box<dyn Error + Send + Sync + 'static>;
 /// #     }
 /// # }
 /// # }
+/// # fn wrapper() -> Result<(), aws_smithy_http::endpoint::error::InvalidEndpointError> {
 /// use aws_smithy_http::endpoint::Endpoint;
-/// use http::Uri;
 /// let config = dynamodb::Config::builder()
-///     .endpoint(
-///         Endpoint::immutable(Uri::from_static("http://localhost:8080"))
-///     );
+///     .endpoint(Endpoint::immutable("http://localhost:8080")?);
+/// #     Ok(())
+/// # }
 /// ```
 /// Each AWS service generates their own implementation of `ResolveAwsEndpoint`.
 pub trait ResolveAwsEndpoint: Send + Sync + Debug {
diff --git a/aws/rust-runtime/aws-types/src/sdk_config.rs b/aws/rust-runtime/aws-types/src/sdk_config.rs
@@ -90,13 +90,15 @@ impl Builder {
     ///
     /// # Examples
     /// ```
+    /// # fn wrapper() -> Result<(), aws_smithy_http::endpoint::error::InvalidEndpointError> {
     /// use std::sync::Arc;
     /// use aws_types::SdkConfig;
     /// use aws_smithy_http::endpoint::Endpoint;
-    /// use http::Uri;
     /// let config = SdkConfig::builder().endpoint_resolver(
-    ///     Endpoint::immutable(Uri::from_static("http://localhost:8080"))
+    ///     Endpoint::immutable("http://localhost:8080")?
     /// ).build();
+    /// # Ok(())
+    /// # }
     /// ```
     pub fn endpoint_resolver(
         mut self,
diff --git a/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsEndpointDecorator.kt b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsEndpointDecorator.kt
@@ -138,14 +138,16 @@ class EndpointConfigCustomization(
                     ///
                     /// ## Examples
                     /// ```no_run
+                    /// ## fn wrapper() -> Result<(), aws_smithy_http::endpoint::error::InvalidEndpointError> {
                     /// use #{aws_types}::region::Region;
                     /// use $moduleUseName::config::{Builder, Config};
                     /// use $moduleUseName::Endpoint;
                     ///
                     /// let config = $moduleUseName::Config::builder()
-                    ///     .endpoint_resolver(
-                    ///         Endpoint::immutable("http://localhost:8080".parse().expect("valid URI"))
-                    ///     ).build();
+                    ///     .endpoint_resolver(Endpoint::immutable("http://localhost:8080")?)
+                    ///     .build();
+                    /// ## Ok(())
+                    /// ## }
                     /// ```
                     pub fn endpoint_resolver(mut self, endpoint_resolver: impl #{ResolveAwsEndpoint} + 'static) -> Self {
                         self.endpoint_resolver = Some(std::sync::Arc::new(#{EndpointShim}::from_resolver(endpoint_resolver)) as _);
diff --git a/aws/sdk/integration-tests/dynamodb/tests/endpoints.rs b/aws/sdk/integration-tests/dynamodb/tests/endpoints.rs
@@ -14,9 +14,7 @@ async fn endpoints_can_be_overridden_globally() {
     let shared_config = aws_types::SdkConfig::builder()
         .region(Region::new("us-east-4"))
         .http_connector(conn)
-        .endpoint_resolver(Endpoint::immutable(
-            "http://localhost:8000".parse().unwrap(),
-        ))
+        .endpoint_resolver(Endpoint::immutable("http://localhost:8000").expect("valid endpoint"))
         .build();
     let conf = aws_sdk_dynamodb::config::Builder::from(&shared_config)
         .credentials_provider(Credentials::new("asdf", "asdf", None, None, "test"))
@@ -38,9 +36,7 @@ async fn endpoints_can_be_overridden_locally() {
         .build();
     let conf = aws_sdk_dynamodb::config::Builder::from(&shared_config)
         .credentials_provider(Credentials::new("asdf", "asdf", None, None, "test"))
-        .endpoint_resolver(Endpoint::immutable(
-            "http://localhost:8000".parse().unwrap(),
-        ))
+        .endpoint_resolver(Endpoint::immutable("http://localhost:8000").expect("valid endpoint"))
         .build();
     let svc = aws_sdk_dynamodb::Client::from_conf(conf);
     let _ = svc.list_tables().send().await;
diff --git a/aws/sdk/integration-tests/s3/tests/streaming-response.rs b/aws/sdk/integration-tests/s3/tests/streaming-response.rs
@@ -30,9 +30,9 @@ async fn test_streaming_response_fails_when_eof_comes_before_content_length_reac
             "test",
         )))
         .region(Region::new("us-east-1"))
-        .endpoint_resolver(Endpoint::immutable(
-            format!("http://{server_addr}").parse().expect("valid URI"),
-        ))
+        .endpoint_resolver(
+            Endpoint::immutable(format!("http://{server_addr}")).expect("valid endpoint"),
+        )
         .build();
 
     let client = Client::new(&sdk_config);
diff --git a/aws/sdk/integration-tests/s3/tests/timeouts.rs b/aws/sdk/integration-tests/s3/tests/timeouts.rs
diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ProtocolTestGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/ProtocolTestGenerator.kt
diff --git a/rust-runtime/aws-smithy-http/src/endpoint.rs b/rust-runtime/aws-smithy-http/src/endpoint.rs
diff --git a/rust-runtime/aws-smithy-http/src/endpoint/error.rs b/rust-runtime/aws-smithy-http/src/endpoint/error.rs

Original file line number	Diff line number	Diff line change
`@@ -190,8 +190,11 @@ impl Provider {`
`190`	`190`	`});`
`191`	`191`	`}`
`192`	`192`	`};`
`193`		`- let endpoint = Endpoint::immutable(Uri::from_static(BASE_HOST));`
`194`		`- endpoint.set_endpoint(&mut relative_uri, None);`
	`193`	`+ let endpoint =`
	`194`	`+ Endpoint::immutable_uri(Uri::from_static(BASE_HOST)).expect("BASE_HOST is valid");`
	`195`	`+ endpoint`
	`196`	`+ .set_endpoint(&mut relative_uri, None)`
	`197`	`+ .expect("appending relative URLs to the ECS endpoint should always succeed");`
`195`	`198`	`Ok(relative_uri)`
`196`	`199`	`}`
`197`	`200`	`}`