From bf83f3461720274ef6d3178ddcfb84f735a5c646 Mon Sep 17 00:00:00 2001 From: Jonas Platte Date: Sun, 23 Jan 2022 17:46:41 +0100 Subject: [PATCH] Add trait IntoResponseHeaders (#649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Introduce IntoResponseHeaders trait * Implement IntoResponseHeaders for HeaderMap * Add impl IntoResponse for impl IntoResponseHeaders … and update IntoResponse impls that use HeaderMap to be generic instead. * Add impl IntoResponseHeaders for Headers … and remove IntoResponse impls that use it. * axum-debug: Fix grammar in docs * Explain confusing error message in docs --- axum-core/src/response/headers.rs | 106 ++++---------- axum-core/src/response/mod.rs | 138 ++++++++++++++++-- axum-debug/src/lib.rs | 30 +++- .../tests/fail/wrong_return_type.stderr | 5 +- axum/CHANGELOG.md | 3 + axum/src/routing/mod.rs | 2 +- 6 files changed, 190 insertions(+), 94 deletions(-) diff --git a/axum-core/src/response/headers.rs b/axum-core/src/response/headers.rs index 56455481..7176ff98 100644 --- a/axum-core/src/response/headers.rs +++ b/axum-core/src/response/headers.rs @@ -1,11 +1,8 @@ -use super::{IntoResponse, Response}; -use crate::body::boxed; -use bytes::Bytes; +use super::{IntoResponse, IntoResponseHeaders, Response}; use http::{ - header::{HeaderMap, HeaderName, HeaderValue}, + header::{HeaderName, HeaderValue}, StatusCode, }; -use http_body::{Empty, Full}; use std::{convert::TryInto, fmt}; /// A response with headers. @@ -54,38 +51,7 @@ use std::{convert::TryInto, fmt}; #[derive(Clone, Copy, Debug)] pub struct Headers(pub H); -impl Headers { - fn try_into_header_map(self) -> Result - where - H: IntoIterator, - K: TryInto, - K::Error: fmt::Display, - V: TryInto, - V::Error: fmt::Display, - { - self.0 - .into_iter() - .map(|(key, value)| { - let key = key.try_into().map_err(Either::A)?; - let value = value.try_into().map_err(Either::B)?; - Ok((key, value)) - }) - .collect::>() - .map_err(|err| { - let err = match err { - Either::A(err) => err.to_string(), - Either::B(err) => err.to_string(), - }; - - let body = boxed(Full::new(Bytes::copy_from_slice(err.as_bytes()))); - let mut res = Response::new(body); - *res.status_mut() = StatusCode::INTERNAL_SERVER_ERROR; - res - }) - } -} - -impl IntoResponse for Headers +impl IntoResponseHeaders for Headers where H: IntoIterator, K: TryInto, @@ -93,63 +59,45 @@ where V: TryInto, V::Error: fmt::Display, { - fn into_response(self) -> Response { - let headers = self.try_into_header_map(); + type IntoIter = IntoIter; - match headers { - Ok(headers) => { - let mut res = Response::new(boxed(Empty::new())); - *res.headers_mut() = headers; - res - } - Err(err) => err, + fn into_headers(self) -> Self::IntoIter { + IntoIter { + inner: self.0.into_iter(), } } } -impl IntoResponse for (Headers, T) +#[doc(hidden)] +#[derive(Debug)] +pub struct IntoIter { + inner: H, +} + +impl Iterator for IntoIter where - T: IntoResponse, - H: IntoIterator, + H: Iterator, K: TryInto, K::Error: fmt::Display, V: TryInto, V::Error: fmt::Display, { - fn into_response(self) -> Response { - let headers = match self.0.try_into_header_map() { - Ok(headers) => headers, - Err(res) => return res, - }; + type Item = Result<(Option, HeaderValue), Response>; - (headers, self.1).into_response() + fn next(&mut self) -> Option { + self.inner.next().map(|(key, value)| { + let key = key + .try_into() + .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response())?; + let value = value + .try_into() + .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()).into_response())?; + + Ok((Some(key), value)) + }) } } -impl IntoResponse for (StatusCode, Headers, T) -where - T: IntoResponse, - H: IntoIterator, - K: TryInto, - K::Error: fmt::Display, - V: TryInto, - V::Error: fmt::Display, -{ - fn into_response(self) -> Response { - let headers = match self.1.try_into_header_map() { - Ok(headers) => headers, - Err(res) => return res, - }; - - (self.0, headers, self.2).into_response() - } -} - -enum Either { - A(A), - B(B), -} - #[cfg(test)] mod tests { use super::*; diff --git a/axum-core/src/response/mod.rs b/axum-core/src/response/mod.rs index 69c14870..534351c2 100644 --- a/axum-core/src/response/mod.rs +++ b/axum-core/src/response/mod.rs @@ -10,14 +10,14 @@ use crate::{ }; use bytes::Bytes; use http::{ - header::{self, HeaderMap, HeaderValue}, + header::{self, HeaderMap, HeaderName, HeaderValue}, StatusCode, }; use http_body::{ combinators::{MapData, MapErr}, Empty, Full, }; -use std::{borrow::Cow, convert::Infallible}; +use std::{borrow::Cow, convert::Infallible, iter}; mod headers; @@ -148,6 +148,42 @@ pub trait IntoResponse { fn into_response(self) -> Response; } +/// Trait for generating response headers. +/// + +/// **Note: If you see this trait not being implemented in an error message, you are almost +/// certainly being mislead by the compiler¹. Look for the following snippet in the output and +/// check [`IntoResponse`]'s documentation if you find it:** +/// +/// ```text +/// note: required because of the requirements on the impl of `IntoResponse` for `` +/// ``` +/// +/// Any type that implements this trait automatically implements `IntoResponse` as well, but can +/// also be used in a tuple like `(StatusCode, Self)`, `(Self, impl IntoResponseHeaders)`, +/// `(StatusCode, Self, impl IntoResponseHeaders, impl IntoResponse)` and so on. +/// +/// This trait can't currently be implemented outside of axum. +/// +/// ¹ See also [this rustc issue](https://github.com/rust-lang/rust/issues/22590) +pub trait IntoResponseHeaders { + /// The return type of [`.into_headers()`]. + /// + /// The iterator item is a `Result` to allow the implementation to return a server error + /// instead. + /// + /// The header name is optional because `HeaderMap`s iterator doesn't yield it multiple times + /// for headers that have multiple values, to avoid unnecessary copies. + #[doc(hidden)] + type IntoIter: IntoIterator, HeaderValue), Response>>; + + /// Attempt to turn `self` into a list of headers. + /// + /// In practice, only the implementation for `axum::response::Headers` ever returns `Err(_)`. + #[doc(hidden)] + fn into_headers(self) -> Self::IntoIter; +} + impl IntoResponse for () { fn into_response(self) -> Response { Response::new(boxed(Empty::new())) @@ -320,6 +356,21 @@ impl IntoResponse for StatusCode { } } +impl IntoResponse for H +where + H: IntoResponseHeaders, +{ + fn into_response(self) -> Response { + let mut res = Response::new(boxed(Empty::new())); + + if let Err(e) = try_extend_headers(res.headers_mut(), self.into_headers()) { + return e; + } + + res + } +} + impl IntoResponse for (StatusCode, T) where T: IntoResponse, @@ -331,33 +382,98 @@ where } } -impl IntoResponse for (HeaderMap, T) +impl IntoResponse for (H, T) where + H: IntoResponseHeaders, T: IntoResponse, { fn into_response(self) -> Response { let mut res = self.1.into_response(); - res.headers_mut().extend(self.0); + + if let Err(e) = try_extend_headers(res.headers_mut(), self.0.into_headers()) { + return e; + } + res } } -impl IntoResponse for (StatusCode, HeaderMap, T) +impl IntoResponse for (StatusCode, H, T) where + H: IntoResponseHeaders, T: IntoResponse, { fn into_response(self) -> Response { let mut res = self.2.into_response(); *res.status_mut() = self.0; - res.headers_mut().extend(self.1); + + if let Err(e) = try_extend_headers(res.headers_mut(), self.1.into_headers()) { + return e; + } + res } } -impl IntoResponse for HeaderMap { - fn into_response(self) -> Response { - let mut res = Response::new(boxed(Empty::new())); - *res.headers_mut() = self; - res +impl IntoResponseHeaders for HeaderMap { + // FIXME: Use type_alias_impl_trait when available + type IntoIter = iter::Map< + http::header::IntoIter, + fn( + (Option, HeaderValue), + ) -> Result<(Option, HeaderValue), Response>, + >; + + fn into_headers(self) -> Self::IntoIter { + self.into_iter().map(Ok) + } +} + +// Slightly adjusted version of `impl Extend<(Option, T)> for HeaderMap`. +// Accepts an iterator that returns Results and short-circuits on an `Err`. +fn try_extend_headers( + headers: &mut HeaderMap, + iter: impl IntoIterator, HeaderValue), Response>>, +) -> Result<(), Response> { + use http::header::Entry; + + let mut iter = iter.into_iter(); + + // The structure of this is a bit weird, but it is mostly to make the + // borrow checker happy. + let (mut key, mut val) = match iter.next().transpose()? { + Some((Some(key), val)) => (key, val), + Some((None, _)) => panic!("expected a header name, but got None"), + None => return Ok(()), + }; + + 'outer: loop { + let mut entry = match headers.entry(key) { + Entry::Occupied(mut e) => { + // Replace all previous values while maintaining a handle to + // the entry. + e.insert(val); + e + } + Entry::Vacant(e) => e.insert_entry(val), + }; + + // As long as `HeaderName` is none, keep inserting the value into + // the current entry + loop { + match iter.next().transpose()? { + Some((Some(k), v)) => { + key = k; + val = v; + continue 'outer; + } + Some((None, v)) => { + entry.append(v); + } + None => { + return Ok(()); + } + } + } } } diff --git a/axum-debug/src/lib.rs b/axum-debug/src/lib.rs index 2c58365d..e25919fd 100644 --- a/axum-debug/src/lib.rs +++ b/axum-debug/src/lib.rs @@ -21,7 +21,7 @@ //! ``` //! //! You will get a long error message about function not implementing [`Handler`] trait. But why -//! this function does not implement it? To figure it out [`debug_handler`] macro can be used. +//! does this function not implement it? To figure it out, the [`debug_handler`] macro can be used. //! //! ```rust,compile_fail //! # use axum::{routing::get, Router}; @@ -89,6 +89,34 @@ //! async fn handler(request: Request) {} //! ``` //! +//! # Known limitations +//! +//! If your response type doesn't implement `IntoResponse`, you will get a slightly confusing error +//! message: +//! +//! ```text +//! error[E0277]: the trait bound `bool: axum_core::response::IntoResponseHeaders` is not satisfied +//! --> tests/fail/wrong_return_type.rs:4:23 +//! | +//! 4 | async fn handler() -> bool { +//! | ^^^^ the trait `axum_core::response::IntoResponseHeaders` is not implemented for `bool` +//! | +//! = note: required because of the requirements on the impl of `IntoResponse` for `bool` +//! note: required by a bound in `__axum_debug_check_handler_into_response::{closure#0}::check` +//! --> tests/fail/wrong_return_type.rs:4:23 +//! | +//! 4 | async fn handler() -> bool { +//! | ^^^^ required by this bound in `__axum_debug_check_handler_into_response::{closure#0}::check` +//! ``` +//! +//! The main error message when `IntoResponse` isn't implemented will also ways be for a different +//! trait, `IntoResponseHeaders`, not being implemented. This trait is not meant to be implemented +//! for types outside of axum and what you really need to do is change your return type or implement +//! `IntoResponse` for it (if it is your own type that you want to return directly from handlers). +//! +//! This issue is not specific to axum and cannot be fixed by us. For more details, see the +//! [rustc issue about it](https://github.com/rust-lang/rust/issues/22590). +//! //! # Performance //! //! Macros in this crate have no effect when using release profile. (eg. `cargo build --release`) diff --git a/axum-debug/tests/fail/wrong_return_type.stderr b/axum-debug/tests/fail/wrong_return_type.stderr index 596ba1d7..55e5c269 100644 --- a/axum-debug/tests/fail/wrong_return_type.stderr +++ b/axum-debug/tests/fail/wrong_return_type.stderr @@ -1,9 +1,10 @@ -error[E0277]: the trait bound `bool: IntoResponse` is not satisfied +error[E0277]: the trait bound `bool: axum_core::response::IntoResponseHeaders` is not satisfied --> tests/fail/wrong_return_type.rs:4:23 | 4 | async fn handler() -> bool { - | ^^^^ the trait `IntoResponse` is not implemented for `bool` + | ^^^^ the trait `axum_core::response::IntoResponseHeaders` is not implemented for `bool` | + = note: required because of the requirements on the impl of `IntoResponse` for `bool` note: required by a bound in `__axum_debug_check_handler_into_response::{closure#0}::check` --> tests/fail/wrong_return_type.rs:4:23 | diff --git a/axum/CHANGELOG.md b/axum/CHANGELOG.md index b0c19261..69912f3e 100644 --- a/axum/CHANGELOG.md +++ b/axum/CHANGELOG.md @@ -50,6 +50,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `MatchedPathRejection` - `WebSocketUpgradeRejection` +TODO(david): make sure everything from https://github.com/tokio-rs/axum/pull/644 +is mentioned here. + [#644]: https://github.com/tokio-rs/axum/pull/644 [#665]: https://github.com/tokio-rs/axum/pull/665 [#698]: https://github.com/tokio-rs/axum/pull/698 diff --git a/axum/src/routing/mod.rs b/axum/src/routing/mod.rs index 1d44101c..bf55d4f2 100644 --- a/axum/src/routing/mod.rs +++ b/axum/src/routing/mod.rs @@ -4,7 +4,7 @@ use self::{future::RouterFuture, not_found::NotFound}; use crate::{ body::{boxed, Body, Bytes, HttpBody}, extract::connect_info::{Connected, IntoMakeServiceWithConnectInfo}, - response::{Response, Redirect, IntoResponse}, + response::{IntoResponse, Redirect, Response}, routing::strip_prefix::StripPrefix, util::{try_downcast, ByteStr, PercentDecodedByteStr}, BoxError,