From 5d5c5e3cb2c97216a73a5cb578a531e9e96d97e1 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Mon, 14 Nov 2022 16:09:23 -0800 Subject: [PATCH] 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 | 122 +++++++++++++----- .../aws-smithy-http/src/endpoint/error.rs | 54 +++++++- 2 files changed, 138 insertions(+), 38 deletions(-) diff --git a/rust-runtime/aws-smithy-http/src/endpoint.rs b/rust-runtime/aws-smithy-http/src/endpoint.rs index 6aed737e1d7..71a80d39abf 100644 --- a/rust-runtime/aws-smithy-http/src/endpoint.rs +++ b/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) -> std::result::Result { + pub fn new(prefix: impl Into) -> StdResult { 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 { + 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) -> StdResult { + 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 { + 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) -> StdResult { + 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 { + 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 + }) + )); + } } diff --git a/rust-runtime/aws-smithy-http/src/endpoint/error.rs b/rust-runtime/aws-smithy-http/src/endpoint/error.rs index fd850bfe307..72104ead4f7 100644 --- a/rust-runtime/aws-smithy-http/src/endpoint/error.rs +++ b/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, + }, + FailedToConstructUri { + source: Box, + }, } #[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>, + ) -> Self { + Self { + kind: InvalidEndpointErrorKind::FailedToConstructAuthority { + source: source.into(), + }, + } + } + + pub(super) fn failed_to_construct_uri( + source: impl Into>, + ) -> Self { + Self { + kind: InvalidEndpointErrorKind::FailedToConstructUri { + source: source.into(), + }, + } + } } impl From 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, + } + } +}