Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove third party types from public APIs #2845

Merged
merged 8 commits into from
Jul 14, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
3 changes: 0 additions & 3 deletions aws/rust-runtime/aws-http/external-types.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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",
]
33 changes: 20 additions & 13 deletions aws/rust-runtime/aws-http/src/user_agent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -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");

Expand All @@ -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)
})
}
Expand Down
3 changes: 0 additions & 3 deletions rust-runtime/aws-smithy-async/external-types.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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",
]
34 changes: 31 additions & 3 deletions rust-runtime/aws-smithy-async/src/future/rendezvous.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -38,6 +37,35 @@ pub fn channel<T>() -> (Sender<T>, Receiver<T>) {
)
}

/// Errors for rendezvous channel
pub mod error {
use std::fmt;

/// Error when [crate::future::rendezvous::Sender] fails to send a value to the associated `Receiver`
#[derive(Debug)]
pub struct SendError<T> {
source: tokio::sync::mpsc::error::SendError<T>,
ysaito1001 marked this conversation as resolved.
Show resolved Hide resolved
}

impl<T> SendError<T> {
pub(crate) fn tokio_send_error(source: tokio::sync::mpsc::error::SendError<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> {
Expand All @@ -50,7 +78,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() {
Expand All @@ -61,7 +89,7 @@ impl<T> Sender<T> {
.expect("semaphore is never closed")
.forget();
}
result
result.map_err(error::SendError::tokio_send_error)
}
}

Expand Down
3 changes: 0 additions & 3 deletions rust-runtime/aws-smithy-xml/external-types.toml
Original file line number Diff line number Diff line change
@@ -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",
]
87 changes: 56 additions & 31 deletions rust-runtime/aws-smithy-xml/src/decode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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() },
Expand Down Expand Up @@ -256,6 +254,13 @@ 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: Token<'inp>,
}
ysaito1001 marked this conversation as resolved.
Show resolved Hide resolved

/// Depth tracking iterator
///
/// ```xml
Expand All @@ -267,11 +272,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
Expand All @@ -290,11 +295,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 { token: t }, self.depth - 1)));
}
_ => {}
}
Some(Ok((tok, self.depth)))
Some(Ok((XmlToken { token: tok }, self.depth)))
}
}

Expand Down Expand Up @@ -351,7 +356,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 {
Expand All @@ -365,7 +370,7 @@ impl<'inp, 'a> Iterator for ScopedDecoder<'inp, 'a> {
other => return other,
};

match tok {
match tok.token {
Token::ElementEnd { end, .. } if self.start_el.end_el(end, depth) => {
self.terminated = true;
return None;
Expand All @@ -378,22 +383,30 @@ 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: Token::ElementStart { local, prefix, .. },
},
depth,
)) => {
out.name.local = local.as_str();
out.name.prefix = prefix.as_str();
out.depth = depth;
}
Ok((
Token::Attribute {
prefix,
local,
value,
..
XmlToken {
token:
Token::Attribute {
prefix,
local,
value,
..
},
},
_,
)) => out.attributes.push(Attr {
Expand All @@ -404,16 +417,22 @@ fn next_start_element<'a, 'inp>(
value: unescape(value.as_str()).ok()?,
}),
Ok((
Token::ElementEnd {
end: ElementEnd::Open,
..
XmlToken {
token:
Token::ElementEnd {
end: ElementEnd::Open,
..
},
},
_,
)) => break,
Ok((
Token::ElementEnd {
end: ElementEnd::Empty,
..
XmlToken {
token:
Token::ElementEnd {
end: ElementEnd::Empty,
..
},
},
_,
)) => {
Expand All @@ -431,13 +450,19 @@ 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: Token::Text { text },
})) => return unescape(text.as_str()),
Some(Ok(
e @ XmlToken {
token: Token::ElementStart { .. },
},
)) => {
return Err(XmlDecodeError::custom(format!(
"looking for a data element, found: {:?}",
e
Expand Down