From 9f5dc45914e6dddd0893810ea2f41a43690a6a7f Mon Sep 17 00:00:00 2001 From: David Pedersen Date: Thu, 13 Jan 2022 09:49:55 +0100 Subject: [PATCH] Remove `RequestParts::take_extensions` (#699) * Remove `RequestParts::take_extensions` * fix out of date docs * Remove RequestAlreadyExtracted and replace it with BodyAlreadyExtracted * fix docs * fix test * Update axum-core/src/extract/mod.rs Co-authored-by: Jonas Platte * Remove macro only used once Co-authored-by: Jonas Platte --- axum-core/CHANGELOG.md | 14 +++++ axum-core/src/extract/mod.rs | 47 ++++------------ axum-core/src/extract/rejection.rs | 60 +++++++++----------- axum-core/src/extract/request_parts.rs | 25 ++------- axum-core/src/macros.rs | 35 ------------ axum-extra/CHANGELOG.md | 3 + axum-extra/src/extract/cached.rs | 76 ++------------------------ axum-extra/src/extract/mod.rs | 6 -- axum/CHANGELOG.md | 19 +++++++ axum/src/extract/extension.rs | 1 - axum/src/extract/matched_path.rs | 7 +-- axum/src/extract/path/mod.rs | 12 ++-- axum/src/extract/rejection.rs | 10 ++-- axum/src/extract/ws.rs | 11 +--- 14 files changed, 98 insertions(+), 228 deletions(-) diff --git a/axum-core/CHANGELOG.md b/axum-core/CHANGELOG.md index 24d5962193..b13680812a 100644 --- a/axum-core/CHANGELOG.md +++ b/axum-core/CHANGELOG.md @@ -21,8 +21,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `RequestAlreadyExtracted` - `RequestPartsAlreadyExtracted` - `>::Error` has been changed to `std::convert::Infallible`. +- **breaking:** `axum::http::Extensions` is no longer an extractor (ie it + doesn't implement `FromRequest`). The `axum::extract::Extension` extractor is + _not_ impacted by this and works the same. This change makes it harder to + accidentally remove all extensions which would result in confusing errors + elsewhere ([#699]) + This includes these breaking changes: + - `RequestParts::take_extensions` has been removed. + - `RequestParts::extensions` returns `&Extensions`. + - `RequestParts::extensions_mut` returns `&mut Extensions`. + - `RequestAlreadyExtracted` has been removed. + - `::Error` is now `BodyAlreadyExtracted`. + - `::Error` is now `Infallible`. + - `ExtensionsAlreadyExtracted` has been removed. [#698]: https://github.com/tokio-rs/axum/pull/698 +[#699]: https://github.com/tokio-rs/axum/pull/699 # 0.1.1 (06. December, 2021) diff --git a/axum-core/src/extract/mod.rs b/axum-core/src/extract/mod.rs index 5d7271fc10..1a85da81c9 100644 --- a/axum-core/src/extract/mod.rs +++ b/axum-core/src/extract/mod.rs @@ -78,7 +78,7 @@ pub struct RequestParts { uri: Uri, version: Version, headers: HeaderMap, - extensions: Option, + extensions: Extensions, body: Option, } @@ -108,52 +108,38 @@ impl RequestParts { uri, version, headers, - extensions: Some(extensions), + extensions, body: Some(body), } } /// Convert this `RequestParts` back into a [`Request`]. /// - /// Fails if + /// Fails if The request body has been extracted, that is [`take_body`] has + /// been called. /// - /// - The full [`Extensions`] has been extracted, that is - /// [`take_extensions`] have been called. - /// - The request body has been extracted, that is [`take_body`] have been - /// called. - /// - /// [`take_extensions`]: RequestParts::take_extensions /// [`take_body`]: RequestParts::take_body - pub fn try_into_request(self) -> Result, RequestAlreadyExtracted> { + pub fn try_into_request(self) -> Result, BodyAlreadyExtracted> { let Self { method, uri, version, headers, - mut extensions, + extensions, mut body, } = self; let mut req = if let Some(body) = body.take() { Request::new(body) } else { - return Err(RequestAlreadyExtracted::BodyAlreadyExtracted( - BodyAlreadyExtracted, - )); + return Err(BodyAlreadyExtracted); }; *req.method_mut() = method; *req.uri_mut() = uri; *req.version_mut() = version; *req.headers_mut() = headers; - - if let Some(extensions) = extensions.take() { - *req.extensions_mut() = extensions; - } else { - return Err(RequestAlreadyExtracted::ExtensionsAlreadyExtracted( - ExtensionsAlreadyExtracted, - )); - } + *req.extensions_mut() = extensions; Ok(req) } @@ -199,22 +185,13 @@ impl RequestParts { } /// Gets a reference to the request extensions. - /// - /// Returns `None` if the extensions has been taken by another extractor. - pub fn extensions(&self) -> Option<&Extensions> { - self.extensions.as_ref() + pub fn extensions(&self) -> &Extensions { + &self.extensions } /// Gets a mutable reference to the request extensions. - /// - /// Returns `None` if the extensions has been taken by another extractor. - pub fn extensions_mut(&mut self) -> Option<&mut Extensions> { - self.extensions.as_mut() - } - - /// Takes the extensions out of the request, leaving a `None` in its place. - pub fn take_extensions(&mut self) -> Option { - self.extensions.take() + pub fn extensions_mut(&mut self) -> &mut Extensions { + &mut self.extensions } /// Gets a reference to the request body. diff --git a/axum-core/src/extract/rejection.rs b/axum-core/src/extract/rejection.rs index b04aa9f9c5..10440e49b4 100644 --- a/axum-core/src/extract/rejection.rs +++ b/axum-core/src/extract/rejection.rs @@ -1,21 +1,36 @@ //! Rejection response types. -define_rejection! { - #[status = INTERNAL_SERVER_ERROR] - #[body = "Cannot have two request body extractors for a single handler"] - /// Rejection type used if you try and extract the request body more than - /// once. - pub struct BodyAlreadyExtracted; +use crate::body; +use http::{Response, StatusCode}; +use http_body::Full; +use std::fmt; + +/// Rejection type used if you try and extract the request body more than +/// once. +#[derive(Debug, Default)] +#[non_exhaustive] +pub struct BodyAlreadyExtracted; + +impl BodyAlreadyExtracted { + const BODY: &'static str = "Cannot have two request body extractors for a single handler"; } -define_rejection! { - #[status = INTERNAL_SERVER_ERROR] - #[body = "Extensions taken by other extractor"] - /// Rejection used if the request extension has been taken by another - /// extractor. - pub struct ExtensionsAlreadyExtracted; +impl crate::response::IntoResponse for BodyAlreadyExtracted { + fn into_response(self) -> crate::response::Response { + let mut res = Response::new(body::boxed(Full::from(Self::BODY))); + *res.status_mut() = StatusCode::INTERNAL_SERVER_ERROR; + res + } } +impl fmt::Display for BodyAlreadyExtracted { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", Self::BODY) + } +} + +impl std::error::Error for BodyAlreadyExtracted {} + define_rejection! { #[status = BAD_REQUEST] #[body = "Failed to buffer the request body"] @@ -32,18 +47,6 @@ define_rejection! { pub struct InvalidUtf8(Error); } -composite_rejection! { - /// Rejection used for [`Request<_>`]. - /// - /// Contains one variant for each way the [`Request<_>`] extractor can fail. - /// - /// [`Request<_>`]: http::Request - pub enum RequestAlreadyExtracted { - BodyAlreadyExtracted, - ExtensionsAlreadyExtracted, - } -} - composite_rejection! { /// Rejection used for [`Bytes`](bytes::Bytes). /// @@ -65,12 +68,3 @@ composite_rejection! { InvalidUtf8, } } - -composite_rejection! { - /// Rejection used for [`http::request::Parts`]. - /// - /// Contains one variant for each way the [`http::request::Parts`] extractor can fail. - pub enum RequestPartsAlreadyExtracted { - ExtensionsAlreadyExtracted, - } -} diff --git a/axum-core/src/extract/request_parts.rs b/axum-core/src/extract/request_parts.rs index 9fc4bac95f..33383a7d8d 100644 --- a/axum-core/src/extract/request_parts.rs +++ b/axum-core/src/extract/request_parts.rs @@ -10,7 +10,7 @@ impl FromRequest for Request where B: Send, { - type Rejection = RequestAlreadyExtracted; + type Rejection = BodyAlreadyExtracted; async fn from_request(req: &mut RequestParts) -> Result { let req = std::mem::replace( @@ -20,7 +20,7 @@ where version: req.version, uri: req.uri.clone(), headers: HeaderMap::new(), - extensions: None, + extensions: Extensions::default(), body: None, }, ); @@ -82,18 +82,6 @@ where } } -#[async_trait] -impl FromRequest for Extensions -where - B: Send, -{ - type Rejection = ExtensionsAlreadyExtracted; - - async fn from_request(req: &mut RequestParts) -> Result { - req.take_extensions().ok_or(ExtensionsAlreadyExtracted) - } -} - #[async_trait] impl FromRequest for Bytes where @@ -142,17 +130,14 @@ impl FromRequest for http::request::Parts where B: Send, { - type Rejection = RequestPartsAlreadyExtracted; + type Rejection = Infallible; async fn from_request(req: &mut RequestParts) -> Result { let method = unwrap_infallible(Method::from_request(req).await); let uri = unwrap_infallible(Uri::from_request(req).await); let version = unwrap_infallible(Version::from_request(req).await); - let headers = match HeaderMap::from_request(req).await { - Ok(headers) => headers, - Err(err) => match err {}, - }; - let extensions = Extensions::from_request(req).await?; + let headers = unwrap_infallible(HeaderMap::from_request(req).await); + let extensions = std::mem::take(req.extensions_mut()); let mut temp_request = Request::new(()); *temp_request.method_mut() = method; diff --git a/axum-core/src/macros.rs b/axum-core/src/macros.rs index 77c428882a..723e319f58 100644 --- a/axum-core/src/macros.rs +++ b/axum-core/src/macros.rs @@ -1,39 +1,4 @@ macro_rules! define_rejection { - ( - #[status = $status:ident] - #[body = $body:expr] - $(#[$m:meta])* - pub struct $name:ident; - ) => { - $(#[$m])* - #[derive(Debug)] - #[non_exhaustive] - pub struct $name; - - #[allow(deprecated)] - impl $crate::response::IntoResponse for $name { - fn into_response(self) -> $crate::response::Response { - let mut res = http::Response::new($crate::body::boxed(http_body::Full::from($body))); - *res.status_mut() = http::StatusCode::$status; - res - } - } - - impl std::fmt::Display for $name { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", $body) - } - } - - impl std::error::Error for $name {} - - impl Default for $name { - fn default() -> Self { - Self - } - } - }; - ( #[status = $status:ident] #[body = $body:expr] diff --git a/axum-extra/CHANGELOG.md b/axum-extra/CHANGELOG.md index 080e4526b1..21ac82043f 100644 --- a/axum-extra/CHANGELOG.md +++ b/axum-extra/CHANGELOG.md @@ -8,8 +8,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 # Unreleased - **fix:** Depend on tower with `default_features = false` ([#666]) +- **breaking:** `CachedRejection` has been removed ([#699]) +- **breaking:** ` as FromRequest>::Error` is now `T::Rejection`. ([#699]) [#666]: https://github.com/tokio-rs/axum/pull/666 +[#699]: https://github.com/tokio-rs/axum/pull/699 # 0.1.1 (27. December, 2021) diff --git a/axum-extra/src/extract/cached.rs b/axum-extra/src/extract/cached.rs index 333b64ffce..0ada78a888 100644 --- a/axum-extra/src/extract/cached.rs +++ b/axum-extra/src/extract/cached.rs @@ -1,15 +1,8 @@ use axum::{ async_trait, - extract::{ - rejection::{ExtensionRejection, ExtensionsAlreadyExtracted}, - Extension, FromRequest, RequestParts, - }, - response::{IntoResponse, Response}, -}; -use std::{ - fmt, - ops::{Deref, DerefMut}, + extract::{Extension, FromRequest, RequestParts}, }; +use std::ops::{Deref, DerefMut}; /// Cache results of other extractors. /// @@ -100,25 +93,14 @@ where B: Send, T: FromRequest + Clone + Send + Sync + 'static, { - type Rejection = CachedRejection; + type Rejection = T::Rejection; async fn from_request(req: &mut RequestParts) -> Result { match Extension::>::from_request(req).await { Ok(Extension(CachedEntry(value))) => Ok(Self(value)), - Err(ExtensionRejection::ExtensionsAlreadyExtracted(err)) => { - Err(CachedRejection::ExtensionsAlreadyExtracted(err)) - } Err(_) => { - let value = T::from_request(req).await.map_err(CachedRejection::Inner)?; - - req.extensions_mut() - .ok_or_else(|| { - CachedRejection::ExtensionsAlreadyExtracted( - ExtensionsAlreadyExtracted::default(), - ) - })? - .insert(CachedEntry(value.clone())); - + let value = T::from_request(req).await?; + req.extensions_mut().insert(CachedEntry(value.clone())); Ok(Self(value)) } } @@ -139,54 +121,6 @@ impl DerefMut for Cached { } } -/// Rejection used for [`Cached`]. -/// -/// Contains one variant for each way the [`Cached`] extractor can fail. -#[derive(Debug)] -#[non_exhaustive] -pub enum CachedRejection { - #[allow(missing_docs)] - ExtensionsAlreadyExtracted(ExtensionsAlreadyExtracted), - #[allow(missing_docs)] - Inner(R), -} - -impl IntoResponse for CachedRejection -where - R: IntoResponse, -{ - fn into_response(self) -> Response { - match self { - Self::ExtensionsAlreadyExtracted(inner) => inner.into_response(), - Self::Inner(inner) => inner.into_response(), - } - } -} - -impl fmt::Display for CachedRejection -where - R: fmt::Display, -{ - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::ExtensionsAlreadyExtracted(inner) => write!(f, "{}", inner), - Self::Inner(inner) => write!(f, "{}", inner), - } - } -} - -impl std::error::Error for CachedRejection -where - R: std::error::Error + 'static, -{ - fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - Self::ExtensionsAlreadyExtracted(inner) => Some(inner), - Self::Inner(inner) => Some(inner), - } - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/axum-extra/src/extract/mod.rs b/axum-extra/src/extract/mod.rs index 434fafa543..35c92b3ef1 100644 --- a/axum-extra/src/extract/mod.rs +++ b/axum-extra/src/extract/mod.rs @@ -3,9 +3,3 @@ mod cached; pub use self::cached::Cached; - -pub mod rejection { - //! Rejection response types. - - pub use super::cached::CachedRejection; -} diff --git a/axum/CHANGELOG.md b/axum/CHANGELOG.md index 4c29e8b14c..1650498a60 100644 --- a/axum/CHANGELOG.md +++ b/axum/CHANGELOG.md @@ -31,10 +31,29 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `ContentLengthLimitRejection` - `WebSocketUpgradeRejection` - `>::Error` has been changed to `std::convert::Infallible`. +- **breaking:** `axum::http::Extensions` is no longer an extractor (ie it + doesn't implement `FromRequest`). The `axum::extract::Extension` extractor is + _not_ impacted by this and works the same. This change makes it harder to + accidentally remove all extensions which would result in confusing errors + elsewhere ([#699]) + This includes these breaking changes: + - `RequestParts::take_extensions` has been removed. + - `RequestParts::extensions` returns `&Extensions`. + - `RequestParts::extensions_mut` returns `&mut Extensions`. + - `RequestAlreadyExtracted` has been removed. + - `::Error` is now `BodyAlreadyExtracted`. + - `::Error` is now `Infallible`. + - `ExtensionsAlreadyExtracted` has been removed. + - The `ExtensionsAlreadyExtracted` removed variant has been removed from these rejections: + - `ExtensionRejection` + - `PathRejection` + - `MatchedPathRejection` + - `WebSocketUpgradeRejection` [#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 +[#699]: https://github.com/tokio-rs/axum/pull/699 # 0.4.3 (21. December, 2021) diff --git a/axum/src/extract/extension.rs b/axum/src/extract/extension.rs index d65fb3a3c4..110a23c54d 100644 --- a/axum/src/extract/extension.rs +++ b/axum/src/extract/extension.rs @@ -53,7 +53,6 @@ where async fn from_request(req: &mut RequestParts) -> Result { let value = req .extensions() - .ok_or_else(ExtensionsAlreadyExtracted::default)? .get::() .ok_or_else(|| { MissingExtension::from_err(format!( diff --git a/axum/src/extract/matched_path.rs b/axum/src/extract/matched_path.rs index 9ef519d158..d98e4f439a 100644 --- a/axum/src/extract/matched_path.rs +++ b/axum/src/extract/matched_path.rs @@ -70,11 +70,8 @@ where type Rejection = MatchedPathRejection; async fn from_request(req: &mut RequestParts) -> Result { - let extensions = req.extensions().ok_or_else(|| { - MatchedPathRejection::ExtensionsAlreadyExtracted(ExtensionsAlreadyExtracted::default()) - })?; - - let matched_path = extensions + let matched_path = req + .extensions() .get::() .ok_or(MatchedPathRejection::MatchedPathMissing(MatchedPathMissing))? .clone(); diff --git a/axum/src/extract/path/mod.rs b/axum/src/extract/path/mod.rs index de9af83df7..38a2a7b160 100644 --- a/axum/src/extract/path/mod.rs +++ b/axum/src/extract/path/mod.rs @@ -3,7 +3,6 @@ mod de; -use super::rejection::ExtensionsAlreadyExtracted; use crate::{ body::{boxed, Full}, extract::{rejection::*, FromRequest, RequestParts}, @@ -164,11 +163,7 @@ where type Rejection = PathRejection; async fn from_request(req: &mut RequestParts) -> Result { - let ext = req - .extensions_mut() - .ok_or_else::(|| ExtensionsAlreadyExtracted::default().into())?; - - let params = match ext.get::>() { + let params = match req.extensions_mut().get::>() { Some(Some(UrlParams(Ok(params)))) => Cow::Borrowed(params), Some(Some(UrlParams(Err(InvalidUtf8InPathParam { key })))) => { let err = PathDeserializationError { @@ -519,6 +514,9 @@ mod tests { let res = client.get("/foo").send().await; assert_eq!(res.status(), StatusCode::INTERNAL_SERVER_ERROR); - assert_eq!(res.text().await, "Extensions taken by other extractor"); + assert_eq!( + res.text().await, + "No paths parameters found for matched route. Are you also extracting `Request<_>`?" + ); } } diff --git a/axum/src/extract/rejection.rs b/axum/src/extract/rejection.rs index 627244e342..96002c0815 100644 --- a/axum/src/extract/rejection.rs +++ b/axum/src/extract/rejection.rs @@ -52,9 +52,10 @@ define_rejection! { define_rejection! { #[status = INTERNAL_SERVER_ERROR] - #[body = "No paths parameters found for matched route. This is a bug in axum. Please open an issue"] - /// Rejection type used if axum's internal representation of path parameters is missing. This - /// should never happen and is a bug in axum if it does. + #[body = "No paths parameters found for matched route. Are you also extracting `Request<_>`?"] + /// Rejection type used if axum's internal representation of path parameters + /// is missing. This is commonly caused by extracting `Request<_>`. `Path` + /// must be extracted first. pub struct MissingPathParams; } @@ -148,7 +149,6 @@ composite_rejection! { /// can fail. pub enum ExtensionRejection { MissingExtension, - ExtensionsAlreadyExtracted, } } @@ -160,7 +160,6 @@ composite_rejection! { pub enum PathRejection { FailedToDeserializePathParams, MissingPathParams, - ExtensionsAlreadyExtracted, } } @@ -176,7 +175,6 @@ define_rejection! { composite_rejection! { /// Rejection used for [`MatchedPath`](super::MatchedPath). pub enum MatchedPathRejection { - ExtensionsAlreadyExtracted, MatchedPathMissing, } } diff --git a/axum/src/extract/ws.rs b/axum/src/extract/ws.rs index e07af70aa7..8079be1afd 100644 --- a/axum/src/extract/ws.rs +++ b/axum/src/extract/ws.rs @@ -64,7 +64,7 @@ //! [`StreamExt::split`]: https://docs.rs/futures/0.3.17/futures/stream/trait.StreamExt.html#method.split use self::rejection::*; -use super::{rejection::*, FromRequest, RequestParts}; +use super::{FromRequest, RequestParts}; use crate::{ body::{self, Bytes}, response::Response, @@ -268,11 +268,7 @@ where return Err(WebSocketKeyHeaderMissing.into()); }; - let on_upgrade = req - .extensions_mut() - .ok_or_else(ExtensionsAlreadyExtracted::default)? - .remove::() - .unwrap(); + let on_upgrade = req.extensions_mut().remove::().unwrap(); let sec_websocket_protocol = req.headers().get(header::SEC_WEBSOCKET_PROTOCOL).cloned(); @@ -514,8 +510,6 @@ fn sign(key: &[u8]) -> HeaderValue { pub mod rejection { //! WebSocket specific rejections. - use crate::extract::rejection::*; - define_rejection! { #[status = METHOD_NOT_ALLOWED] #[body = "Request method must be `GET`"] @@ -562,7 +556,6 @@ pub mod rejection { InvalidUpgradeHeader, InvalidWebSocketVersionHeader, WebSocketKeyHeaderMissing, - ExtensionsAlreadyExtracted, } } }