From 99068f5a4b309d0966777eb6e5a8ce924f204e6d Mon Sep 17 00:00:00 2001 From: Yann Simon Date: Mon, 6 Apr 2026 18:47:42 +0200 Subject: [PATCH] Revert "Fix `IntoResponse` for tuples overriding error response codes (#3603)" This reverts commit 0e961504c27d3b43af0147023d8644cce116b025. --- Cargo.lock | 1 - axum-core/Cargo.toml | 1 - axum-core/src/response/into_response.rs | 87 ++---- axum-core/src/response/into_response_parts.rs | 20 +- axum-core/src/response/mod.rs | 88 ------ axum-extra/src/protobuf.rs | 9 +- axum-extra/src/response/erased_json.rs | 9 +- axum/src/form.rs | 9 +- axum/src/json.rs | 3 +- axum/src/response/mod.rs | 292 +----------------- 10 files changed, 39 insertions(+), 480 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3b996773..08fa9e53 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -352,7 +352,6 @@ dependencies = [ "hyper 1.7.0", "mime", "pin-project-lite", - "serde", "sync_wrapper 1.0.2", "tokio", "tower-http 0.6.8", diff --git a/axum-core/Cargo.toml b/axum-core/Cargo.toml index b8b0195b..7667a3fc 100644 --- a/axum-core/Cargo.toml +++ b/axum-core/Cargo.toml @@ -38,7 +38,6 @@ axum = { path = "../axum", features = ["__private"] } axum-extra = { path = "../axum-extra", features = ["typed-header"] } axum-macros = { path = "../axum-macros", features = ["__private"] } hyper = "1.0.0" -serde = { version = "1.0.200", features = ["derive"] } tokio = { version = "1.25.0", features = ["macros"] } tower-http = { version = "0.6.0", features = ["limit"] } diff --git a/axum-core/src/response/into_response.rs b/axum-core/src/response/into_response.rs index a311851c..915b55ef 100644 --- a/axum-core/src/response/into_response.rs +++ b/axum-core/src/response/into_response.rs @@ -1,4 +1,4 @@ -use super::{ForceStatusCode, IntoResponseFailed, IntoResponseParts, Response, ResponseParts}; +use super::{IntoResponseParts, Response, ResponseParts}; use crate::{body::Body, BoxError}; use bytes::{buf::Chain, Buf, Bytes, BytesMut}; use http::{ @@ -329,9 +329,7 @@ where { fn into_response(self) -> Response { let mut res = self.1.into_response(); - if res.extensions().get::().is_none() { - *res.status_mut() = self.0; - } + *res.status_mut() = self.0; res } } @@ -407,16 +405,18 @@ macro_rules! impl_into_response { let ($($ty),*, res) = self; let res = res.into_response(); - if res.extensions().get::().is_none() { - let parts = ResponseParts { res }; - let parts = match ($($ty,)*).into_response_parts(parts) { + let parts = ResponseParts { res }; + + $( + let parts = match $ty.into_response_parts(parts) { Ok(parts) => parts, - Err(err) => return err.into_response(), + Err(err) => { + return err.into_response(); + } }; - parts.res - } else { - res - } + )* + + parts.res } } @@ -429,41 +429,17 @@ macro_rules! impl_into_response { fn into_response(self) -> Response { let (status, $($ty),*, res) = self; - let res = res.into_response(); - if res.extensions().get::().is_none() { - let parts = ResponseParts { res }; - let mut parts = match ($($ty,)*).into_response_parts(parts) { - Ok(parts) => parts, - Err(err) => return err.into_response(), - }; - - // Don't call `(status, parts.res).into_response()` since that checks for - // `IntoResponseFailed` and skips setting the status. We've already done that - // check here so overriding the status is required if returning - // `(IntoResponseFailed, StatusCode::INTERNAL_SERVER_ERROR)` - *parts.res.status_mut() = status; - parts.res - } else { - res - } - } - } - - #[allow(non_snake_case)] - impl IntoResponse for (ForceStatusCode, $($ty),*, R) - where - $( $ty: IntoResponseParts, )* - R: IntoResponse, - { - fn into_response(self) -> Response { - let (status, $($ty),*, res) = self; - let res = res.into_response(); let parts = ResponseParts { res }; - let parts = match ($($ty,)*).into_response_parts(parts) { - Ok(parts) => parts, - Err(err) => return err.into_response(), - }; + + $( + let parts = match $ty.into_response_parts(parts) { + Ok(parts) => parts, + Err(err) => { + return err.into_response(); + } + }; + )* (status, parts.res).into_response() } @@ -479,22 +455,17 @@ macro_rules! impl_into_response { let (outer_parts, $($ty),*, res) = self; let res = res.into_response(); - if res.extensions().get::().is_none() { - let parts = ResponseParts { res }; - let mut parts = match ($($ty,)*).into_response_parts(parts) { + let parts = ResponseParts { res }; + $( + let parts = match $ty.into_response_parts(parts) { Ok(parts) => parts, - Err(err) => return err.into_response(), + Err(err) => { + return err.into_response(); + } }; + )* - // Don't call `(outer_parts, parts.res).into_response()` for the same reason we - // don't call `(status, parts.res).into_response()` in the above impl. - *parts.res.status_mut() = outer_parts.status; - parts.res.headers_mut().extend(outer_parts.headers); - parts.res.extensions_mut().extend(outer_parts.extensions); - parts.res - } else { - res - } + (outer_parts, parts.res).into_response() } } diff --git a/axum-core/src/response/into_response_parts.rs b/axum-core/src/response/into_response_parts.rs index a142b50b..95564823 100644 --- a/axum-core/src/response/into_response_parts.rs +++ b/axum-core/src/response/into_response_parts.rs @@ -241,9 +241,7 @@ macro_rules! impl_into_response_parts { let res = match $ty.into_response_parts(res) { Ok(res) => res, Err(err) => { - let mut err_res = err.into_response(); - err_res.extensions_mut().insert(super::IntoResponseFailed); - return Err(err_res); + return Err(err.into_response()); } }; )* @@ -272,19 +270,3 @@ impl IntoResponseParts for () { Ok(res) } } - -#[cfg(test)] -mod tests { - use http::StatusCode; - - use crate::response::IntoResponse; - - #[test] - fn failed_into_response_parts() { - let response = (StatusCode::CREATED, [("\n", "\n")]).into_response(); - assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR); - - let response = (StatusCode::CREATED, [("\n", "\n")], ()).into_response(); - assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR); - } -} diff --git a/axum-core/src/response/mod.rs b/axum-core/src/response/mod.rs index b00bc85e..b40b2529 100644 --- a/axum-core/src/response/mod.rs +++ b/axum-core/src/response/mod.rs @@ -4,10 +4,6 @@ //! //! [`axum::response`]: https://docs.rs/axum/0.8/axum/response/index.html -use std::convert::Infallible; - -use http::StatusCode; - use crate::body::Body; mod append_headers; @@ -132,87 +128,3 @@ where Self(value.into_response()) } } - -/// Response part that stops status code overrides. -/// -/// This type should be used by types implementing [`IntoResponseParts`] or -/// [`IntoResponse`] when they fail to produce the response usually expected of -/// them and return some sort of error response instead. -/// -/// It is checked used by the tuple impls of [`IntoResponse`] that have a -/// [`StatusCode`] as their first element to ignore that status code. -/// Consider the following example: -/// -/// ```no_run -/// # use axum::Json; -/// # use http::StatusCode; -/// # #[derive(serde::Serialize)] -/// # struct CreatedResponse { } -/// fn my_handler(/* ... */) -> (StatusCode, Json) { -/// // This response type's serialization may fail -/// let response = CreatedResponse { /* ... */ }; -/// (StatusCode::CREATED, Json(response)) -/// } -/// ``` -/// -/// When `response` serialization succeeds, the server responds with a status -/// code of 201 Created (overwriting `Json`s default status code of 200 OK), -/// and the expected JSON payload. -/// -/// When `response` serialization fails hoewever, `impl IntoResponse for Json` -/// return a response with status code 500 Internal Server Error, and -/// `IntoResponseFailed` as a response extension, and the 201 Created override -/// is ignored. -/// -/// This is a behavior introduced with axum 0.9.\ -/// To force a status code override even when an inner [`IntoResponseParts`] / -/// [`IntoResponse`] failed, use [`ForceStatusCode`]. -#[derive(Copy, Clone, Debug)] -pub struct IntoResponseFailed; - -impl IntoResponseParts for IntoResponseFailed { - type Error = Infallible; - - fn into_response_parts(self, mut res: ResponseParts) -> Result { - res.extensions_mut().insert(self); - Ok(res) - } -} - -/// Not sure it makes sense to return `IntoResponseFailed` as the whole response. You should -/// probably at least combine it with a status code. -/// -/// ```compile_fail -/// fn foo() -/// where -/// axum_core::response::IntoResponseFailed: axum_core::response::IntoResponse, -/// {} -/// ``` -#[allow(dead_code)] -fn into_response_failed_doesnt_impl_into_response() {} - -/// Set the status code regardless of whether [`IntoResponseFailed`] is used or not. -/// -/// See the docs for [`IntoResponseFailed`] for more details. -#[derive(Debug, Copy, Clone, Default)] -pub struct ForceStatusCode(pub StatusCode); - -impl IntoResponse for ForceStatusCode { - fn into_response(self) -> Response { - let mut res = ().into_response(); - *res.status_mut() = self.0; - res - } -} - -impl IntoResponse for (ForceStatusCode, R) -where - R: IntoResponse, -{ - fn into_response(self) -> Response { - let (ForceStatusCode(status), res) = self; - let mut res = res.into_response(); - *res.status_mut() = status; - res - } -} diff --git a/axum-extra/src/protobuf.rs b/axum-extra/src/protobuf.rs index aa8991ce..cc68af2d 100644 --- a/axum-extra/src/protobuf.rs +++ b/axum-extra/src/protobuf.rs @@ -4,7 +4,7 @@ use axum_core::__composite_rejection as composite_rejection; use axum_core::__define_rejection as define_rejection; use axum_core::{ extract::{rejection::BytesRejection, FromRequest, Request}, - response::{IntoResponse, IntoResponseFailed, Response}, + response::{IntoResponse, Response}, RequestExt, }; use bytes::BytesMut; @@ -131,12 +131,7 @@ where let mut buf = BytesMut::with_capacity(self.0.encoded_len()); match &self.0.encode(&mut buf) { Ok(()) => buf.into_response(), - Err(err) => ( - StatusCode::INTERNAL_SERVER_ERROR, - IntoResponseFailed, - err.to_string(), - ) - .into_response(), + Err(err) => (StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response(), } } } diff --git a/axum-extra/src/response/erased_json.rs b/axum-extra/src/response/erased_json.rs index def3d2c1..17d8967b 100644 --- a/axum-extra/src/response/erased_json.rs +++ b/axum-extra/src/response/erased_json.rs @@ -1,6 +1,6 @@ use std::sync::Arc; -use axum_core::response::{IntoResponse, IntoResponseFailed, Response}; +use axum_core::response::{IntoResponse, Response}; use bytes::{BufMut, Bytes, BytesMut}; use http::{header, HeaderValue, StatusCode}; use serde_core::Serialize; @@ -78,12 +78,7 @@ impl IntoResponse for ErasedJson { bytes, ) .into_response(), - Err(err) => ( - StatusCode::INTERNAL_SERVER_ERROR, - IntoResponseFailed, - err.to_string(), - ) - .into_response(), + Err(err) => (StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response(), } } } diff --git a/axum/src/form.rs b/axum/src/form.rs index 50a532e4..ab692a64 100644 --- a/axum/src/form.rs +++ b/axum/src/form.rs @@ -1,6 +1,6 @@ use crate::extract::Request; use crate::extract::{rejection::*, FromRequest, RawForm}; -use axum_core::response::{IntoResponse, IntoResponseFailed, Response}; +use axum_core::response::{IntoResponse, Response}; use axum_core::RequestExt; use http::header::CONTENT_TYPE; use http::StatusCode; @@ -117,12 +117,7 @@ where body, ) .into_response(), - Err(err) => ( - StatusCode::INTERNAL_SERVER_ERROR, - IntoResponseFailed, - err.to_string(), - ) - .into_response(), + Err(err) => (StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response(), } } diff --git a/axum/src/json.rs b/axum/src/json.rs index 90ea13ae..59f2c859 100644 --- a/axum/src/json.rs +++ b/axum/src/json.rs @@ -1,7 +1,7 @@ use crate::extract::Request; use crate::extract::{rejection::*, FromRequest}; use axum_core::extract::OptionalFromRequest; -use axum_core::response::{IntoResponse, IntoResponseFailed, Response}; +use axum_core::response::{IntoResponse, Response}; use bytes::{BufMut, Bytes, BytesMut}; use http::{ header::{self, HeaderMap, HeaderValue}, @@ -224,7 +224,6 @@ where header::CONTENT_TYPE, HeaderValue::from_static(mime::TEXT_PLAIN_UTF_8.as_ref()), )], - IntoResponseFailed, err.to_string(), ) .into_response(), diff --git a/axum/src/response/mod.rs b/axum/src/response/mod.rs index 6307610f..70be7452 100644 --- a/axum/src/response/mod.rs +++ b/axum/src/response/mod.rs @@ -19,8 +19,7 @@ pub use crate::Extension; #[doc(inline)] pub use axum_core::response::{ - AppendHeaders, ErrorResponse, IntoResponse, IntoResponseFailed, IntoResponseParts, Response, - ResponseParts, Result, + AppendHeaders, ErrorResponse, IntoResponse, IntoResponseParts, Response, ResponseParts, Result, }; #[doc(inline)] @@ -86,16 +85,10 @@ impl IntoResponse for NoContent { #[cfg(test)] mod tests { use crate::extract::Extension; - use crate::test_helpers::*; - use crate::Json; use crate::{routing::get, Router}; - use axum_core::response::ForceStatusCode; - use axum_core::response::{ - IntoResponse, IntoResponseFailed, IntoResponseParts, Response, ResponseParts, - }; + use axum_core::response::IntoResponse; use http::HeaderMap; use http::{StatusCode, Uri}; - use std::collections::HashMap; // just needs to compile #[allow(dead_code)] @@ -254,287 +247,6 @@ mod tests { .route("/", get(header_array_extension_mixed_body)); } - #[test] - fn status_code_tuple_doesnt_override_error() { - // sanity check where there is just one status code - assert_eq!( - StatusCode::INTERNAL_SERVER_ERROR.into_response().status(), - StatusCode::INTERNAL_SERVER_ERROR - ); - assert_eq!( - (StatusCode::INTERNAL_SERVER_ERROR,) - .into_response() - .status(), - StatusCode::INTERNAL_SERVER_ERROR - ); - - // non-5xx status should be changed - assert_eq!( - (StatusCode::SEE_OTHER, StatusCode::NO_CONTENT) - .into_response() - .status(), - StatusCode::SEE_OTHER - ); - let res = ( - StatusCode::SEE_OTHER, - [("location", "foo")], - StatusCode::NO_CONTENT, - ) - .into_response(); - assert_eq!(res.status(), StatusCode::SEE_OTHER); - assert_eq!(res.headers()["location"], "foo"); - - // 5xx status codes are also changed - assert_eq!( - (StatusCode::SEE_OTHER, StatusCode::INTERNAL_SERVER_ERROR) - .into_response() - .status(), - StatusCode::SEE_OTHER - ); - let res = ( - StatusCode::SEE_OTHER, - [("location", "foo")], - StatusCode::INTERNAL_SERVER_ERROR, - ) - .into_response(); - assert_eq!(res.status(), StatusCode::SEE_OTHER); - assert_eq!(res.headers()["location"], "foo"); - - // the status is not changed if `IntoResponseFailed` is used - assert_eq!( - ( - StatusCode::SEE_OTHER, - (IntoResponseFailed, StatusCode::INTERNAL_SERVER_ERROR) - ) - .into_response() - .status(), - StatusCode::INTERNAL_SERVER_ERROR - ); - let res = ( - StatusCode::SEE_OTHER, - [("location", "foo")], - (IntoResponseFailed, StatusCode::INTERNAL_SERVER_ERROR), - ) - .into_response(); - assert_eq!(res.status(), StatusCode::INTERNAL_SERVER_ERROR); - assert!(res.headers().get("location").is_none()); - - // response parts from the inner response do run - let res = ( - // with status override - StatusCode::SEE_OTHER, - [("location", "foo")], - ( - [("x-bar", "bar")], - IntoResponseFailed, - [("x-foo", "foo")], - StatusCode::INTERNAL_SERVER_ERROR, - ), - ) - .into_response(); - assert_eq!(res.status(), StatusCode::INTERNAL_SERVER_ERROR); - assert!(res.headers().get("location").is_none()); - assert_eq!(res.headers()["x-foo"], "foo"); - assert_eq!(res.headers()["x-bar"], "bar"); - - let res = ( - // without status override - [("location", "foo")], - ( - [("x-bar", "bar")], - IntoResponseFailed, - [("x-foo", "foo")], - StatusCode::INTERNAL_SERVER_ERROR, - ), - ) - .into_response(); - assert_eq!(res.status(), StatusCode::INTERNAL_SERVER_ERROR); - assert!(res.headers().get("location").is_none()); - assert_eq!(res.headers()["x-foo"], "foo"); - assert_eq!(res.headers()["x-bar"], "bar"); - - // (Parts, ...) - let res = ( - Response::new(()).into_parts().0, - [("location", "foo")], - ( - [("x-bar", "bar")], - IntoResponseFailed, - [("x-foo", "foo")], - StatusCode::INTERNAL_SERVER_ERROR, - ), - ) - .into_response(); - assert_eq!(res.status(), StatusCode::INTERNAL_SERVER_ERROR); - assert!(res.headers().get("location").is_none()); - assert_eq!(res.headers()["x-foo"], "foo"); - assert_eq!(res.headers()["x-bar"], "bar"); - - // (Response<()>, ...) - let res = ( - Response::new(()), - [("location", "foo")], - ( - [("x-bar", "bar")], - IntoResponseFailed, - [("x-foo", "foo")], - StatusCode::INTERNAL_SERVER_ERROR, - ), - ) - .into_response(); - assert_eq!(res.status(), StatusCode::INTERNAL_SERVER_ERROR); - assert!(res.headers().get("location").is_none()); - assert_eq!(res.headers()["x-foo"], "foo"); - assert_eq!(res.headers()["x-bar"], "bar"); - } - - #[test] - fn into_response_parts_failing_sets_extension() { - struct Fail; - - impl IntoResponseParts for Fail { - type Error = (); - - fn into_response_parts( - self, - _res: ResponseParts, - ) -> Result { - Err(()) - } - } - - impl IntoResponse for Fail { - fn into_response(self) -> Response { - (self, ()).into_response() - } - } - - assert!(Fail - .into_response() - .extensions() - .get::() - .is_some()); - - assert!((StatusCode::INTERNAL_SERVER_ERROR, Fail, ()) - .into_response() - .extensions() - .get::() - .is_some()); - - assert!((Response::new(()).into_parts().0, Fail, ()) - .into_response() - .extensions() - .get::() - .is_some()); - - assert!((Response::new(()), Fail, ()) - .into_response() - .extensions() - .get::() - .is_some()); - } - - #[test] - fn doenst_override_status_code_when_using_into_response_failed_at_same_level() { - assert_eq!( - (StatusCode::INTERNAL_SERVER_ERROR, IntoResponseFailed, ()) - .into_response() - .status(), - StatusCode::INTERNAL_SERVER_ERROR, - ); - - #[derive(Clone)] - struct Thing; - - let res = ( - Response::builder() - .status(StatusCode::INTERNAL_SERVER_ERROR) - .header("x-foo", "foo") - .extension(Thing) - .body(()) - .unwrap() - .into_parts() - .0, - IntoResponseFailed, - (), - ) - .into_response(); - assert_eq!(res.status(), StatusCode::INTERNAL_SERVER_ERROR); - assert_eq!(res.headers()["x-foo"], "foo"); - assert!(res.extensions().get::().is_some()); - - // just a sanity check - assert_eq!( - (IntoResponseFailed, ()).into_response().status(), - StatusCode::OK, - ); - } - - #[test] - fn force_overriding_status_code() { - assert_eq!( - ForceStatusCode(StatusCode::IM_A_TEAPOT) - .into_response() - .status(), - StatusCode::IM_A_TEAPOT - ); - - assert_eq!( - (ForceStatusCode(StatusCode::IM_A_TEAPOT),) - .into_response() - .status(), - StatusCode::IM_A_TEAPOT - ); - - assert_eq!( - (ForceStatusCode(StatusCode::IM_A_TEAPOT), ()) - .into_response() - .status(), - StatusCode::IM_A_TEAPOT - ); - - assert_eq!( - ( - ForceStatusCode(StatusCode::IM_A_TEAPOT), - IntoResponseFailed, - StatusCode::INTERNAL_SERVER_ERROR, - ) - .into_response() - .status(), - StatusCode::IM_A_TEAPOT - ); - } - - #[crate::test] - async fn status_code_tuple_doesnt_override_error_json() { - let app = Router::new() - .route( - "/", - get(|| async { - let not_json_compatible = HashMap::from([(Vec::from([1, 2, 3]), 123)]); - (StatusCode::IM_A_TEAPOT, Json(not_json_compatible)) - }), - ) - .route( - "/two", - get(|| async { - let not_json_compatible = HashMap::from([(Vec::from([1, 2, 3]), 123)]); - ( - ForceStatusCode(StatusCode::IM_A_TEAPOT), - Json(not_json_compatible), - ) - }), - ); - - let client = TestClient::new(app); - - let res = client.get("/").await; - assert_eq!(res.status(), StatusCode::INTERNAL_SERVER_ERROR); - - let res = client.get("/two").await; - assert_eq!(res.status(), StatusCode::IM_A_TEAPOT); - } - #[test] fn no_content() { assert_eq!(