Remove third party types from public APIs (#2845)

## Motivation and Context Addresses item 1, 3, and 9 in #2413 ## Description This PR removes the third party types as follows: - removes `InvalidHeaderValue` from public API in `aws-http` - removes `SendError` from public API in `aws-smithy-async` - removes `xmlparser` from public API in `aws-smithy-xml` Those types were exposed to public APIs primarily due to the implementation of traits, e.g. `From` and `Iterator`. Those implementations are used internally (such as XML deserialization and converting an error type) so we should be able to hide them within crates. ## Testing - [x] Passed tests in CI ## Checklist  - [x] I have updated `CHANGELOG.next.toml` if I made changes to the smithy-rs codegen or runtime crates - [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --------- Co-authored-by: ysaito1001 <[email protected]>
smithy-lang · Jul 14, 2023 · 2999766 · 2999766
1 parent 26827f1
commit 2999766
Show file tree

Hide file tree

Showing 7 changed files with 93 additions and 51 deletions.
diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml
@@ -628,3 +628,15 @@ message = "The naming `make_token` for fields and the API of `IdempotencyTokenPr
 references = ["smithy-rs#2783"]
 meta = { "breaking" = true, "tada" = false, "bug" = false }
 author = "ysaito1001"
+
+[[smithy-rs]]
+message = "`aws_smithy_async::future::rendezvous::Sender::send` no longer exposes `tokio::sync::mpsc::error::SendError` for the error of its return type and instead exposes a new-type wrapper called `aws_smithy_async::future::rendezvous::error::SendError`. In addition, the `aws_smithy_xml` crate no longer exposes types from `xmlparser`."
+references = ["smithy-rs#2845"]
+meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
+author = "ysaito1001"
+
+[[aws-sdk-rust]]
+message = "The implementation `From<http::header::value::InvalidHeaderValue>` for `aws_http::user_agent::UserAgentStageError` has been removed."
+references = ["smithy-rs#2845"]
+meta = { "breaking" = true, "tada" = false, "bug" = false }
+author = "ysaito1001"
diff --git a/aws/rust-runtime/aws-http/external-types.toml b/aws/rust-runtime/aws-http/external-types.toml
@@ -5,7 +5,4 @@ allowed_external_types = [
     "aws_types::*",
     "bytes::bytes::Bytes",
     "http_body::Body",
-
-    # TODO(https://github.com/awslabs/smithy-rs/issues/1193): Decide if the following should be exposed
-    "http::header::value::InvalidHeaderValue",
 ]
diff --git a/aws/rust-runtime/aws-http/src/user_agent.rs b/aws/rust-runtime/aws-http/src/user_agent.rs
@@ -550,6 +550,16 @@ pub struct UserAgentStageError {
     kind: UserAgentStageErrorKind,
 }
 
+impl UserAgentStageError {
+    // `pub(crate)` method instead of implementing `From<InvalidHeaderValue>` so that we
+    // don't have to expose `InvalidHeaderValue` in public API.
+    pub(crate) fn from_invalid_header(value: InvalidHeaderValue) -> Self {
+        Self {
+            kind: UserAgentStageErrorKind::InvalidHeader(value),
+        }
+    }
+}
+
 impl Error for UserAgentStageError {
     fn source(&self) -> Option<&(dyn Error + 'static)> {
         use UserAgentStageErrorKind::*;
@@ -578,14 +588,6 @@ impl From<UserAgentStageErrorKind> for UserAgentStageError {
     }
 }
 
-impl From<InvalidHeaderValue> for UserAgentStageError {
-    fn from(value: InvalidHeaderValue) -> Self {
-        Self {
-            kind: UserAgentStageErrorKind::InvalidHeader(value),
-        }
-    }
-}
-
 #[allow(clippy::declare_interior_mutable_const)] // we will never mutate this
 const X_AMZ_USER_AGENT: HeaderName = HeaderName::from_static("x-amz-user-agent");
 
@@ -601,11 +603,16 @@ impl MapRequest for UserAgentStage {
             let ua = conf
                 .get::<AwsUserAgent>()
                 .ok_or(UserAgentStageErrorKind::UserAgentMissing)?;
-            req.headers_mut()
-                .append(USER_AGENT, HeaderValue::try_from(ua.ua_header())?);
-            req.headers_mut()
-                .append(X_AMZ_USER_AGENT, HeaderValue::try_from(ua.aws_ua_header())?);
-
+            req.headers_mut().append(
+                USER_AGENT,
+                HeaderValue::try_from(ua.ua_header())
+                    .map_err(UserAgentStageError::from_invalid_header)?,
+            );
+            req.headers_mut().append(
+                X_AMZ_USER_AGENT,
+                HeaderValue::try_from(ua.aws_ua_header())
+                    .map_err(UserAgentStageError::from_invalid_header)?,
+            );
             Ok(req)
         })
     }

diff --git a/rust-runtime/aws-smithy-async/external-types.toml b/rust-runtime/aws-smithy-async/external-types.toml
@@ -5,7 +5,4 @@ allowed_external_types = [
 
     # TODO(https://github.com/awslabs/smithy-rs/issues/1193): Switch to AsyncIterator once standardized
     "futures_core::stream::Stream",
-
-    # TODO(https://github.com/awslabs/smithy-rs/issues/1193): Don't expose `SendError`
-    "tokio::sync::mpsc::error::SendError",
 ]
diff --git a/rust-runtime/aws-smithy-async/src/future/rendezvous.rs b/rust-runtime/aws-smithy-async/src/future/rendezvous.rs
@@ -14,7 +14,6 @@
 
 use std::sync::Arc;
 use std::task::{Context, Poll};
-use tokio::sync::mpsc::error::SendError;
 use tokio::sync::Semaphore;
 
 /// Create a new rendezvous channel
@@ -38,6 +37,36 @@ pub fn channel<T>() -> (Sender<T>, Receiver<T>) {
     )
 }
 
+/// Errors for rendezvous channel
+pub mod error {
+    use std::fmt;
+    use tokio::sync::mpsc::error::SendError as TokioSendError;
+
+    /// Error when [crate::future::rendezvous::Sender] fails to send a value to the associated `Receiver`
+    #[derive(Debug)]
+    pub struct SendError<T> {
+        source: TokioSendError<T>,
+    }
+
+    impl<T> SendError<T> {
+        pub(crate) fn tokio_send_error(source: TokioSendError<T>) -> Self {
+            Self { source }
+        }
+    }
+
+    impl<T> fmt::Display for SendError<T> {
+        fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+            write!(f, "failed to send value to the receiver")
+        }
+    }
+
+    impl<T: fmt::Debug + 'static> std::error::Error for SendError<T> {
+        fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+            Some(&self.source)
+        }
+    }
+}
+
 #[derive(Debug)]
 /// Sender-half of a channel
 pub struct Sender<T> {
@@ -50,7 +79,7 @@ impl<T> Sender<T> {
     ///
     /// Unlike something like `tokio::sync::mpsc::Channel` where sending a value will be buffered until
     /// demand exists, a rendezvous sender will wait until matching demand exists before this function will return.
-    pub async fn send(&self, item: T) -> Result<(), SendError<T>> {
+    pub async fn send(&self, item: T) -> Result<(), error::SendError<T>> {
         let result = self.chan.send(item).await;
         // If this is an error, the rx half has been dropped. We will never get demand.
         if result.is_ok() {
@@ -61,7 +90,7 @@ impl<T> Sender<T> {
                 .expect("semaphore is never closed")
                 .forget();
         }
-        result
+        result.map_err(error::SendError::tokio_send_error)
     }
 }
 

diff --git a/rust-runtime/aws-smithy-xml/external-types.toml b/rust-runtime/aws-smithy-xml/external-types.toml
@@ -1,5 +1,2 @@
 allowed_external_types = [
-    # TODO(https://github.com/awslabs/smithy-rs/issues/1193): Don't expose `xmlparser` at all
-    "xmlparser::Token",
-    "xmlparser::error::Error",
 ]
diff --git a/rust-runtime/aws-smithy-xml/src/decode.rs b/rust-runtime/aws-smithy-xml/src/decode.rs
@@ -28,14 +28,6 @@ pub struct XmlDecodeError {
     kind: XmlDecodeErrorKind,
 }
 
-impl From<xmlparser::Error> for XmlDecodeError {
-    fn from(error: xmlparser::Error) -> Self {
-        Self {
-            kind: XmlDecodeErrorKind::InvalidXml(error),
-        }
-    }
-}
-
 impl Display for XmlDecodeError {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         match &self.kind {
@@ -58,6 +50,12 @@ impl Error for XmlDecodeError {
 }
 
 impl XmlDecodeError {
+    pub(crate) fn invalid_xml(error: xmlparser::Error) -> Self {
+        Self {
+            kind: XmlDecodeErrorKind::InvalidXml(error),
+        }
+    }
+
     pub(crate) fn invalid_escape(esc: impl Into<String>) -> Self {
         Self {
             kind: XmlDecodeErrorKind::InvalidEscape { esc: esc.into() },
@@ -256,6 +254,11 @@ impl<'inp> Document<'inp> {
     }
 }
 
+/// A new-type wrapper around `Token` to prevent the wrapped third party type from showing up in
+/// public API
+#[derive(Debug)]
+pub struct XmlToken<'inp>(Token<'inp>);
+
 /// Depth tracking iterator
 ///
 /// ```xml
@@ -267,11 +270,11 @@ impl<'inp> Document<'inp> {
 /// </a> <- endel depth 0
 /// ```
 impl<'inp> Iterator for Document<'inp> {
-    type Item = Result<(Token<'inp>, Depth), XmlDecodeError>;
-    fn next<'a>(&'a mut self) -> Option<Result<(Token<'inp>, Depth), XmlDecodeError>> {
+    type Item = Result<(XmlToken<'inp>, Depth), XmlDecodeError>;
+    fn next<'a>(&'a mut self) -> Option<Result<(XmlToken<'inp>, Depth), XmlDecodeError>> {
         let tok = self.tokenizer.next()?;
         let tok = match tok {
-            Err(e) => return Some(Err(e.into())),
+            Err(e) => return Some(Err(XmlDecodeError::invalid_xml(e))),
             Ok(tok) => tok,
         };
         // depth bookkeeping
@@ -290,11 +293,11 @@ impl<'inp> Iterator for Document<'inp> {
                 self.depth += 1;
                 // We want the startel and endel to have the same depth, but after the opener,
                 // the parser will be at depth 1. Return the previous depth:
-                return Some(Ok((t, self.depth - 1)));
+                return Some(Ok((XmlToken(t), self.depth - 1)));
             }
             _ => {}
         }
-        Some(Ok((tok, self.depth)))
+        Some(Ok((XmlToken(tok), self.depth)))
     }
 }
 
@@ -351,7 +354,7 @@ impl<'inp> ScopedDecoder<'inp, '_> {
 }
 
 impl<'inp, 'a> Iterator for ScopedDecoder<'inp, 'a> {
-    type Item = Result<(Token<'inp>, Depth), XmlDecodeError>;
+    type Item = Result<(XmlToken<'inp>, Depth), XmlDecodeError>;
 
     fn next(&mut self) -> Option<Self::Item> {
         if self.start_el.closed {
@@ -365,7 +368,7 @@ impl<'inp, 'a> Iterator for ScopedDecoder<'inp, 'a> {
             other => return other,
         };
 
-        match tok {
+        match tok.0 {
             Token::ElementEnd { end, .. } if self.start_el.end_el(end, depth) => {
                 self.terminated = true;
                 return None;
@@ -378,23 +381,23 @@ impl<'inp, 'a> Iterator for ScopedDecoder<'inp, 'a> {
 
 /// Load the next start element out of a depth-tagged token iterator
 fn next_start_element<'a, 'inp>(
-    tokens: &'a mut impl Iterator<Item = Result<(Token<'inp>, Depth), XmlDecodeError>>,
+    tokens: &'a mut impl Iterator<Item = Result<(XmlToken<'inp>, Depth), XmlDecodeError>>,
 ) -> Option<StartEl<'inp>> {
     let mut out = StartEl::new("", "", 0);
     loop {
         match tokens.next()? {
-            Ok((Token::ElementStart { local, prefix, .. }, depth)) => {
+            Ok((XmlToken(Token::ElementStart { local, prefix, .. }), depth)) => {
                 out.name.local = local.as_str();
                 out.name.prefix = prefix.as_str();
                 out.depth = depth;
             }
             Ok((
-                Token::Attribute {
+                XmlToken(Token::Attribute {
                     prefix,
                     local,
                     value,
                     ..
-                },
+                }),
                 _,
             )) => out.attributes.push(Attr {
                 name: Name {
@@ -404,17 +407,17 @@ fn next_start_element<'a, 'inp>(
                 value: unescape(value.as_str()).ok()?,
             }),
             Ok((
-                Token::ElementEnd {
+                XmlToken(Token::ElementEnd {
                     end: ElementEnd::Open,
                     ..
-                },
+                }),
                 _,
             )) => break,
             Ok((
-                Token::ElementEnd {
+                XmlToken(Token::ElementEnd {
                     end: ElementEnd::Empty,
                     ..
-                },
+                }),
                 _,
             )) => {
                 out.closed = true;
@@ -431,13 +434,13 @@ fn next_start_element<'a, 'inp>(
 /// If the current position is not a data element (and is instead a `<start-element>`) an error
 /// will be returned
 pub fn try_data<'a, 'inp>(
-    tokens: &'a mut impl Iterator<Item = Result<(Token<'inp>, Depth), XmlDecodeError>>,
+    tokens: &'a mut impl Iterator<Item = Result<(XmlToken<'inp>, Depth), XmlDecodeError>>,
 ) -> Result<Cow<'inp, str>, XmlDecodeError> {
     loop {
         match tokens.next().map(|opt| opt.map(|opt| opt.0)) {
             None => return Ok(Cow::Borrowed("")),
-            Some(Ok(Token::Text { text })) => return unescape(text.as_str()),
-            Some(Ok(e @ Token::ElementStart { .. })) => {
+            Some(Ok(XmlToken(Token::Text { text }))) => return unescape(text.as_str()),
+            Some(Ok(e @ XmlToken(Token::ElementStart { .. }))) => {
                 return Err(XmlDecodeError::custom(format!(
                     "looking for a data element, found: {:?}",
                     e