docs(example)/custom future with multiple bodies - #711
Conversation
|
Thanks so much for iterating on this! ngl im still learning so custom futures and tower trait bounds are still kinda wizardry to me lol. your version looks way cleaner and more idiomatic. Should i just close my pr so we can use yours? |
| bytes = "1.12.0" | ||
| http = "1.4.2" | ||
| http-body-util = "0.1.3" | ||
| hyper = "1.10.1" |
|
|
||
| let req_bad = Request::builder().body(Full::<Bytes>::default()).unwrap(); | ||
| let res_bad = service.ready().await?.call(req_bad).await?; | ||
| assert_eq!(res_bad.status(), StatusCode::BAD_REQUEST); |
There was a problem hiding this comment.
Probably these assertions should be in test modules? That matches the other examples.
I'd also probably add some println!s so that cargo run -p custom-future-multiple-bodies prints something, given the README guides the user to run it.
|
|
||
| impl<S, ReqBody, ResBody> Service<Request<ReqBody>> for RequireHeader<S> | ||
| where | ||
| S: Service<Request<ReqBody>, Response = Response<ResBody>>, |
There was a problem hiding this comment.
I would probably add ResBody: http_body::Body<Data = Bytes> here, yeah, at the cost of taking the http-body = "1" dependency. That moves any error out to the middleware layer, which is clearer.
| @@ -0,0 +1,14 @@ | |||
| # Custom future with multiple bodies | |||
There was a problem hiding this comment.
Should we mention that the Either is convenient but loses the concrete error type, and that you can refer to the tower_http::limit wrapper to preserve the type?
| pin_project! { | ||
| #[project = EnumProj] | ||
| pub enum RequireHeaderFuture<F> { | ||
| Ok{ #[pin] fut: F }, |
There was a problem hiding this comment.
nit: Future to match the repo conventions?
|
|
||
| pin_project! { | ||
| #[project = EnumProj] | ||
| pub enum RequireHeaderFuture<F> { |
There was a problem hiding this comment.
Would it be worth a doc comment pointing to how eg limit/future.rs encapsulate these types for semver safety, if used in a library? This seems fine for application usage.
There was a problem hiding this comment.
I don't particularly mind demonstrating the encapsulated pattern in general, it's not particularly more verbose, up to you.
|
|
||
| fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> { | ||
| match self.project() { | ||
| EnumProj::Ok { fut } => { |
There was a problem hiding this comment.
More readable as:
fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
let res = match self.project() {
// `?` propagates an inner-service error. to turn it into a response
// instead, replace `?` with
// .unwrap_or_else(|_| rejection(StatusCode::BAD_GATEWAY, "..."))
// to convert every error, or `.or_else(..)` to convert only some and
// keep propagating the rest.
ResFutProj::Future { future } => ready!(future.poll(cx))?.map(Either::Left),
ResFutProj::MissingHeader => rejection(StatusCode::BAD_REQUEST, "missing header"),
};
Poll::Ready(Ok(res))
| } | ||
|
|
||
| #[tokio::main] | ||
| async fn main() -> Result<(), Box<dyn Error>> { |
There was a problem hiding this comment.
We need the unwrap here since we don't have Box<dyn Error>: From<Box<dyn Error + Send + Sync>>.
I'd suggest instead widening the signature to Result<(), Box<dyn Error + Send + Sync>> (or just Result<(), tower::BoxError>, which is equivalent) to avoid both the unwrap and the need to coerce explicitly to work around the missing From.
| resp.map(|body| Either::Left(body)) | ||
| } | ||
|
|
||
| fn new_err_resp<B>(status: StatusCode, body: &'static str) -> Response<ResponseBody<B>> { |
| type ResponseBody<B> = Either<B, Full<Bytes>>; | ||
|
|
||
| // some helper to go along with it | ||
| fn map_resp<B>(resp: Response<B>) -> Response<ResponseBody<B>> { |
There was a problem hiding this comment.
I don't think this really carries its weight as a helper
3 participants
This PR builds on this PR, which explores idomatic solutions to dealing with multiple HTTP bodies in tower services
Motivation
@Isvane did a great job with his proposal, so i wanted to iterate on it by bringing it closer to how i solved this problem in my own project. Primarily, by introducing a custom future.
Changes
BoxBodyaliasservicetrait boundsPin Box Futurewith custom future.main()Discussion
While i think this is a decently elegant solution, it could be argued its not idiomatic if you compare how this problem is solved in
tower-http/limit.A potential foot gun im wondering about is the trait bound on
Eitherwhich states that both arms need to have the same underlyingDatatype. Right now the example leaves the response body of the inner service entirely generic, but this could lead to a nasty compile error down the road should theDatatypes mismatch.tower-http/limitimposes aBody<Data = Bytes>bound on itsBodyimplementation for it's wrapper type and im wondering whether this example should include it too, and if so, where.