Improve Endpoint panic-safety and ergonomics

- `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`

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

@@ -3,10 +3,11 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-use crate::endpoint::error::{InvalidEndpointError, InvalidEndpointErrorKind};
+use crate::endpoint::error::InvalidEndpointError;
 use crate::operation::error::BuildError;
 use http::uri::{Authority, Uri};
 use std::borrow::Cow;
+use std::result::Result as StdResult;
 use std::str::FromStr;
 
 pub mod error;
@@ -34,7 +35,7 @@ pub struct Endpoint {
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct EndpointPrefix(String);
 impl EndpointPrefix {
-    pub fn new(prefix: impl Into<String>) -> std::result::Result<Self, BuildError> {
+    pub fn new(prefix: impl Into<String>) -> StdResult<Self, BuildError> {
         let prefix = prefix.into();
         match Authority::from_str(&prefix) {
             Ok(_) => Ok(EndpointPrefix(prefix)),
@@ -61,7 +62,7 @@ pub fn apply_endpoint(
     uri: &mut Uri,
     endpoint: &Uri,
     prefix: Option<&EndpointPrefix>,
-) -> std::result::Result<(), InvalidEndpointError> {
+) -> StdResult<(), InvalidEndpointError> {
     let prefix = prefix.map(|p| p.0.as_str()).unwrap_or("");
     let authority = endpoint
         .authority()
@@ -73,17 +74,17 @@ pub fn apply_endpoint(
     } else {
         Authority::from_str(authority)
     }
-    .map_err(|_| InvalidEndpointErrorKind::FailedToConstructAuthority)?;
+    .map_err(InvalidEndpointError::failed_to_construct_authority)?;
     let scheme = *endpoint
         .scheme()
         .as_ref()
-        .ok_or(InvalidEndpointErrorKind::EndpointMustHaveScheme)?;
+        .ok_or_else(InvalidEndpointError::endpoint_must_have_scheme)?;
     let new_uri = Uri::builder()
         .authority(authority)
         .scheme(scheme.clone())
         .path_and_query(Endpoint::merge_paths(endpoint, uri).as_ref())
         .build()
-        .map_err(|_| InvalidEndpointErrorKind::FailedToConstructUri)?;
+        .map_err(InvalidEndpointError::failed_to_construct_uri)?;
     *uri = new_uri;
     Ok(())
 }
@@ -94,11 +95,22 @@ impl Endpoint {
     /// Certain services will augment the endpoint with additional metadata. For example,
     /// S3 can prefix the host with the bucket name. If your endpoint does not support this,
     /// (for example, when communicating with localhost), use [`Endpoint::immutable`].
-    pub fn mutable(uri: Uri) -> Self {
-        Endpoint {
-            uri,
+    pub fn mutable_uri(uri: Uri) -> StdResult<Self, InvalidEndpointError> {
+        Ok(Endpoint {
+            uri: Self::validate_endpoint(uri)?,
             immutable: false,
-        }
+        })
+    }
+
+    /// Create a new endpoint from a URI string
+    ///
+    /// Certain services will augment the endpoint with additional metadata. For example,
+    /// S3 can prefix the host with the bucket name. If your endpoint does not support this,
+    /// (for example, when communicating with localhost), use [`Endpoint::immutable`].
+    pub fn mutable(uri: impl AsRef<str>) -> StdResult<Self, InvalidEndpointError> {
+        Self::mutable_uri(
+            Uri::try_from(uri.as_ref()).map_err(InvalidEndpointError::failed_to_construct_uri)?,
+        )
     }
 
     /// Returns the URI of this endpoint
@@ -111,27 +123,57 @@ impl Endpoint {
     /// ```rust
     /// # use aws_smithy_http::endpoint::Endpoint;
     /// use http::Uri;
-    /// let endpoint = Endpoint::immutable(Uri::from_static("http://localhost:8000"));
+    /// let uri = Uri::from_static("http://localhost:8000");
+    /// let endpoint = Endpoint::immutable_uri(uri);
     /// ```
     ///
     /// Certain services will augment the endpoint with additional metadata. For example,
     /// S3 can prefix the host with the bucket name. This constructor creates an endpoint which will
     /// ignore those mutations. If you want an endpoint which will obey mutation requests, use
     /// [`Endpoint::mutable`] instead.
-    pub fn immutable(uri: Uri) -> Self {
-        Endpoint {
-            uri,
+    pub fn immutable_uri(uri: Uri) -> StdResult<Self, InvalidEndpointError> {
+        Ok(Endpoint {
+            uri: Self::validate_endpoint(uri)?,
             immutable: true,
-        }
+        })
+    }
+
+    /// Create a new immutable endpoint from a URI string
+    ///
+    /// ```rust
+    /// # use aws_smithy_http::endpoint::Endpoint;
+    /// let endpoint = Endpoint::immutable("http://localhost:8000");
+    /// ```
+    ///
+    /// Certain services will augment the endpoint with additional metadata. For example,
+    /// S3 can prefix the host with the bucket name. This constructor creates an endpoint which will
+    /// ignore those mutations. If you want an endpoint which will obey mutation requests, use
+    /// [`Endpoint::mutable`] instead.
+    pub fn immutable(uri: impl AsRef<str>) -> StdResult<Self, InvalidEndpointError> {
+        Self::immutable_uri(
+            Uri::try_from(uri.as_ref()).map_err(InvalidEndpointError::failed_to_construct_uri)?,
+        )
     }
 
     /// Sets the endpoint on `uri`, potentially applying the specified `prefix` in the process.
-    pub fn set_endpoint(&self, uri: &mut http::Uri, prefix: Option<&EndpointPrefix>) {
+    pub fn set_endpoint(
+        &self,
+        uri: &mut http::Uri,
+        prefix: Option<&EndpointPrefix>,
+    ) -> StdResult<(), InvalidEndpointError> {
         let prefix = match self.immutable {
             true => None,
             false => prefix,
         };
-        apply_endpoint(uri, &self.uri, prefix).expect("failed to set endpoint");
+        apply_endpoint(uri, &self.uri, prefix)
+    }
+
+    fn validate_endpoint(endpoint: Uri) -> StdResult<Uri, InvalidEndpointError> {
+        if endpoint.scheme().is_none() {
+            Err(InvalidEndpointError::endpoint_must_have_scheme())
+        } else {
+            Ok(endpoint)
+        }
     }
 
     fn merge_paths<'a>(endpoint: &'a Uri, uri: &'a Uri) -> Cow<'a, str> {
@@ -154,18 +196,19 @@ impl Endpoint {
 
 #[cfg(test)]
 mod test {
-    use http::Uri;
-
+    use crate::endpoint::error::{InvalidEndpointError, InvalidEndpointErrorKind};
     use crate::endpoint::{Endpoint, EndpointPrefix};
+    use http::Uri;
 
     #[test]
     fn prefix_endpoint() {
-        let ep = Endpoint::mutable(Uri::from_static("https://us-east-1.dynamo.amazonaws.com"));
+        let ep = Endpoint::mutable("https://us-east-1.dynamo.amazonaws.com").unwrap();
         let mut uri = Uri::from_static("/list_tables?k=v");
         ep.set_endpoint(
             &mut uri,
             Some(&EndpointPrefix::new("subregion.").expect("valid prefix")),
-        );
+        )
+        .unwrap();
         assert_eq!(
             uri,
             Uri::from_static("https://subregion.us-east-1.dynamo.amazonaws.com/list_tables?k=v")
@@ -174,14 +217,13 @@ mod test {
 
     #[test]
     fn prefix_endpoint_custom_port() {
-        let ep = Endpoint::mutable(Uri::from_static(
-            "https://us-east-1.dynamo.amazonaws.com:6443",
-        ));
+        let ep = Endpoint::mutable("https://us-east-1.dynamo.amazonaws.com:6443").unwrap();
         let mut uri = Uri::from_static("/list_tables?k=v");
         ep.set_endpoint(
             &mut uri,
             Some(&EndpointPrefix::new("subregion.").expect("valid prefix")),
-        );
+        )
+        .unwrap();
         assert_eq!(
             uri,
             Uri::from_static(
@@ -192,12 +234,13 @@ mod test {
 
     #[test]
     fn prefix_immutable_endpoint() {
-        let ep = Endpoint::immutable(Uri::from_static("https://us-east-1.dynamo.amazonaws.com"));
+        let ep = Endpoint::immutable("https://us-east-1.dynamo.amazonaws.com").unwrap();
         let mut uri = Uri::from_static("/list_tables?k=v");
         ep.set_endpoint(
             &mut uri,
             Some(&EndpointPrefix::new("subregion.").expect("valid prefix")),
-        );
+        )
+        .unwrap();
         assert_eq!(
             uri,
             Uri::from_static("https://us-east-1.dynamo.amazonaws.com/list_tables?k=v")
@@ -211,12 +254,13 @@ mod test {
             "https://us-east-1.dynamo.amazonaws.com/private",
             "https://us-east-1.dynamo.amazonaws.com/private/",
         ] {
-            let ep = Endpoint::immutable(Uri::from_static(uri));
+            let ep = Endpoint::immutable(uri).unwrap();
             let mut uri = Uri::from_static("/list_tables?k=v");
             ep.set_endpoint(
                 &mut uri,
                 Some(&EndpointPrefix::new("subregion.").expect("valid prefix")),
-            );
+            )
+            .unwrap();
             assert_eq!(
                 uri,
                 Uri::from_static("https://us-east-1.dynamo.amazonaws.com/private/list_tables?k=v")
@@ -226,9 +270,25 @@ mod test {
 
     #[test]
     fn set_endpoint_empty_path() {
-        let ep = Endpoint::immutable(Uri::from_static("http://localhost:8000"));
+        let ep = Endpoint::immutable("http://localhost:8000").unwrap();
         let mut uri = Uri::from_static("/");
-        ep.set_endpoint(&mut uri, None);
+        ep.set_endpoint(&mut uri, None).unwrap();
         assert_eq!(uri, Uri::from_static("http://localhost:8000/"))
     }
+
+    #[test]
+    fn endpoint_construction_missing_scheme() {
+        assert!(matches!(
+            Endpoint::mutable("localhost:8000"),
+            Err(InvalidEndpointError {
+                kind: InvalidEndpointErrorKind::EndpointMustHaveScheme
+            })
+        ));
+        assert!(matches!(
+            Endpoint::immutable("localhost:8000"),
+            Err(InvalidEndpointError {
+                kind: InvalidEndpointErrorKind::EndpointMustHaveScheme
+            })
+        ));
+    }
 }

rust-runtime/aws-smithy-http/src/endpoint/error.rs

@@ -43,17 +43,48 @@ impl StdError for ResolveEndpointError {
     }
 }
 
-#[non_exhaustive]
 #[derive(Debug)]
 pub(super) enum InvalidEndpointErrorKind {
     EndpointMustHaveScheme,
-    FailedToConstructAuthority,
-    FailedToConstructUri,
+    FailedToConstructAuthority {
+        source: Box<dyn StdError + Send + Sync + 'static>,
+    },
+    FailedToConstructUri {
+        source: Box<dyn StdError + Send + Sync + 'static>,
+    },
 }
 
 #[derive(Debug)]
 pub struct InvalidEndpointError {
-    kind: InvalidEndpointErrorKind,
+    pub(super) kind: InvalidEndpointErrorKind,
+}
+
+impl InvalidEndpointError {
+    pub(super) fn endpoint_must_have_scheme() -> Self {
+        Self {
+            kind: InvalidEndpointErrorKind::EndpointMustHaveScheme,
+        }
+    }
+
+    pub(super) fn failed_to_construct_authority(
+        source: impl Into<Box<dyn StdError + Send + Sync + 'static>>,
+    ) -> Self {
+        Self {
+            kind: InvalidEndpointErrorKind::FailedToConstructAuthority {
+                source: source.into(),
+            },
+        }
+    }
+
+    pub(super) fn failed_to_construct_uri(
+        source: impl Into<Box<dyn StdError + Send + Sync + 'static>>,
+    ) -> Self {
+        Self {
+            kind: InvalidEndpointErrorKind::FailedToConstructUri {
+                source: source.into(),
+            },
+        }
+    }
 }
 
 impl From<InvalidEndpointErrorKind> for InvalidEndpointError {
@@ -67,13 +98,22 @@ impl fmt::Display for InvalidEndpointError {
         use InvalidEndpointErrorKind as ErrorKind;
         match self.kind {
             ErrorKind::EndpointMustHaveScheme => write!(f, "endpoint must contain a valid scheme"),
-            ErrorKind::FailedToConstructAuthority => write!(
+            ErrorKind::FailedToConstructAuthority { .. } => write!(
                 f,
                 "endpoint must contain a valid authority when combined with endpoint prefix"
             ),
-            ErrorKind::FailedToConstructUri => write!(f, "failed to construct URI"),
+            ErrorKind::FailedToConstructUri { .. } => write!(f, "failed to construct URI"),
         }
     }
 }
 
-impl StdError for InvalidEndpointError {}
+impl StdError for InvalidEndpointError {
+    fn source(&self) -> Option<&(dyn StdError + 'static)> {
+        use InvalidEndpointErrorKind as ErrorKind;
+        match &self.kind {
+            ErrorKind::FailedToConstructUri { source }
+            | ErrorKind::FailedToConstructAuthority { source } => Some(source.as_ref()),
+            ErrorKind::EndpointMustHaveScheme => None,
+        }
+    }
+}