From 299976659580932832318c562c228e366a3c8e24 Mon Sep 17 00:00:00 2001 From: ysaito1001 Date: Fri, 14 Jul 2023 12:36:17 -0500 Subject: [PATCH] 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 --- CHANGELOG.next.toml | 12 ++++ aws/rust-runtime/aws-http/external-types.toml | 3 - aws/rust-runtime/aws-http/src/user_agent.rs | 33 ++++++----- .../aws-smithy-async/external-types.toml | 3 - .../aws-smithy-async/src/future/rendezvous.rs | 35 +++++++++++- .../aws-smithy-xml/external-types.toml | 3 - rust-runtime/aws-smithy-xml/src/decode.rs | 55 ++++++++++--------- 7 files changed, 93 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index d603759359..7b44b6203d 100644 --- 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` 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 index 33a2ed14aa..d2e362f515 100644 --- 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 index 59edb2a5c3..27abba7bba 100644 --- 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` 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 for UserAgentStageError { } } -impl From 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::() .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 index dee6e8bb30..424f7dc1db 100644 --- 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 index b501efc85b..16456f123e 100644 --- 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() -> (Sender, Receiver) { ) } +/// 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 { + source: TokioSendError, + } + + impl SendError { + pub(crate) fn tokio_send_error(source: TokioSendError) -> Self { + Self { source } + } + } + + impl fmt::Display for SendError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "failed to send value to the receiver") + } + } + + impl std::error::Error for SendError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(&self.source) + } + } +} + #[derive(Debug)] /// Sender-half of a channel pub struct Sender { @@ -50,7 +79,7 @@ impl Sender { /// /// 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> { + pub async fn send(&self, item: T) -> Result<(), error::SendError> { 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 Sender { .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 index b4b49878e5..ff30ccf5ad 100644 --- 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 index 522c5b12e3..a81c9dbe5f 100644 --- 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 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) -> 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> { /// <- endel depth 0 /// ``` impl<'inp> Iterator for Document<'inp> { - type Item = Result<(Token<'inp>, Depth), XmlDecodeError>; - fn next<'a>(&'a mut self) -> Option, Depth), XmlDecodeError>> { + type Item = Result<(XmlToken<'inp>, Depth), XmlDecodeError>; + fn next<'a>(&'a mut self) -> Option, 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 { 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, Depth), XmlDecodeError>>, + tokens: &'a mut impl Iterator, Depth), XmlDecodeError>>, ) -> Option> { 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 ``) an error /// will be returned pub fn try_data<'a, 'inp>( - tokens: &'a mut impl Iterator, Depth), XmlDecodeError>>, + tokens: &'a mut impl Iterator, Depth), XmlDecodeError>>, ) -> Result, 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