Add middleware & update design docs #175
Conversation
|
|
||
| Middleware is defined as minimally as possible, then adapted into the middleware system used by the IO layer. Tower is the de facto standard for HTTP middleware in Rust—we will probably use it. But we also want to make our middleware usable for users who aren't using Tower (or if we decide to not use Tower in the long run). | ||
|
|
||
| Because of this, rather than implementing all our middleware as "Tower Middleware", we implement it narrowly (eg. as a function that operates on `operation::Request`), then define optional adapters to make our middleware tower compatible. |
|
|
||
| type BoxError = Box<dyn Error + Send + Sync>; | ||
|
|
||
| /// `MapRequest` defines a synchronous middleware that transforms an `operation::Request`. |
There was a problem hiding this comment.
Might want to look into intra-doc linking https://doc.rust-lang.org/rustdoc/linking-to-items-by-name.html
| /// # use std::convert::Infallible; | ||
| /// # use smithy_http::operation; | ||
| /// # use http::header::HeaderName; | ||
| /// struct AddHeader(&'static str, &'static str); |
There was a problem hiding this comment.
Nit: Might want to have users actually use HeaderName and HeaderValue since they are reference counted under the hood and will be cheaper. With its current setup it would need to allocate. My main proposal is to just push people to good defaults rather than "simple" ones like &'static str.
| /// # use http::header::HeaderName; | ||
| /// struct AddHeader(&'static str, &'static str); | ||
| /// impl MapRequest for AddHeader { | ||
| /// type Error = &'static str; |
There was a problem hiding this comment.
Same here I would push people to use a structured error instead of a string since that is quite an anti-pattern I find.
| /// } | ||
| /// ``` | ||
| pub trait MapRequest { | ||
| type Error: Into<BoxError>; |
| /// This function is intended to be used on the response side of a middleware chain. | ||
| /// | ||
| /// Success and failure will be split and mapped into `SdkSuccess` and `SdkError`. | ||
| pub async fn load_response<B, T, E, O>( |
There was a problem hiding this comment.
Beautiful generics kung fu! 🥳
I might suggest even documenting what each of these are. I know we don't do this often but might be a good thing to get into a habit of. (I am to blame for not doing it too!!).
|
|
||
| async fn read_body<B: http_body::Body>(body: B) -> Result<Vec<u8>, B::Error> { | ||
| let mut output = Vec::new(); | ||
| pin_utils::pin_mut!(body); |
There was a problem hiding this comment.
tokio::pin! works here, don't think pin_utils is maintained and really pin_mut! is quite old and you could vendor it quite easily to remove an extra dependency.
| Ok(output) | ||
| } | ||
|
|
||
| fn sdk_result<T, E, B: Debug + 'static>( |
There was a problem hiding this comment.
Again, I think Debug here is an anti-pattern imo.
| pub type Body = Box<dyn Debug>; | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct SdkSuccess<O> { |
There was a problem hiding this comment.
I would impl Display here.
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub enum SdkError<E> { |
There was a problem hiding this comment.
and I would impl Display and Error on this.
|
|
||
| /// Body type when a response is returned. Currently, the only use case is introspecting errors | ||
| /// so it is simply `Debug`. This is an area of potential design iteration. | ||
| pub type Body = Box<dyn Debug>; |
There was a problem hiding this comment.
Yeah this doesn't really track for me but we can punt on it for now.
| where | ||
| B: http_body::Body + Unpin, | ||
| B: From<Bytes> + 'static, | ||
| B::Error: Error + Send + Sync + 'static, |
There was a problem hiding this comment.
Do we want to make this also Into<BoxError>?
Issue #, if available: #152
Description of changes: This diff codifies two pieces of middleware that we will require to dispatch and parse response:
MapRequestStage, a pipeline-style middleware to mutate a request based on the contents of the property bag andasync fn parse_response, a function to drive theParseHttpResponsetrait.Both of these are intended to be used via tower-middleware to implement a complete request stack. (See #107)
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.