Improve the custom rejection experience

[jplatte]

Currently, rejections from extractors in axum and axum-extra turn into responses with plain-text bodies. In some cases this is not a problem but many people are using axum to implement API servers, and at least for rejections that are caused by invalid API usage (rather than server implementation errors), you would most often want a machine-readable error instead.

This means that many extractors such as Json or Query are rarely suitable for production – instead you have to write your own¹. We have examples for this, but it's really unfortunate you can't just do the obvious thing of using the provided ones. There have been many requests for being able to customize the rejections somehow, but we have not found a design that's clearly better than requiring people to create their own extractors.

These are the ideas we had so far, and why they were rejected:

1. Add a defaulted generic const parameter to the extractors, something like

   struct Json<T, const F: fn(serde_json::Error) -> Response = default_error_to_response>(T);

   • Rejected because it isn't possible on stable (and nightly right now, actually, though it will likely become possible at some point), and makes the type rather unwieldy in the documentation
2. Add a defaulted generic type parameter to the extractors, something like

   struct Json<T, R = PlainTextJsonRejection>(T);

   where R has to implement both From<serde_json::Error> and IntoResponse

   • Rejected because it isn't possible with Rust right now, generic type parameters on a struct need to be mentioned in one of the fields, but adding a PhantomData<R> breaks usage of Json(value) as a pattern and constructor
3. Add some config mechanism, the most basic form of which would be

   my_router.layer(Extension(JsonConfig {
     serialize_error: |e: serde_json::Error| /* turn e into axum::response::Response */,
     ..Default::default()
   }))

   • Rejected because it is non-local and has an unnecessary runtime cost (this really seems like something that should be configured at compile-time, not at runtime)

¹ these can sometimes delegate to the provided ones, but not always: for example if you want to include line / column as machine-readable fields of a json rejection, you have to write JSON extractor "from scratch" – though this is hardly more complex than delegating to axum::Json

[simon-amadeus]

Maybe this is a good place to mention, that I have to implement custom rejections for Json and Query.

I am doing this, because the default rejections return error messages that contain internal specifics such as module paths on an public rest-api, e.g.:

Failed to deserialize query string. Expected something of type 'registration_service::routes::registrations::RequestParams'. Error: missing field 'email'.

Nothing serious, but my stomach tells me to avoid exposing internal details of any kind in public environments, at least as a default.

Btw, working with axum has been a pleasure so far and i find it very well documented. Especially the examples have been very helpful and it is great to have a reliable alternative to actix. So thank you @davidpdrsn and all the others for this awesome work!

[davidpdrsn]

Nothing serious, but my stomach tells me to avoid exposing internal details of any kind in public environments, at least as a default.

Yep I agree with that. I considered just logging the errors and return empty responses but that makes it way harder to discover and will likely cause more confusion. And since you'll probably customize the rejections anyway I figured it was fine to include those things directly in the response.

[simon-amadeus]

I see... Well, as this example covers the details, it is easy to implement anyway, but maybe it should be mentioned in the docs somehow?

I just found out about it randomly, when sending a request on the command line with curl. My integration tests missed it, as I did not test for empty response bodies in "failure" cases.

Somehow I did not expect this behavior, but maybe it boils down to writing better tests 😄

[davidpdrsn]

I still think its the right default. I'm certain we're gonna get so many questions about it otherwise. People don't always setup tracing to begin with.

I'd accept a PR that mentions it in the docs though (:

[davidpdrsn]

I've written a crate that explores strategy 2 and 3. You can find it here https://github.com/davidpdrsn/axum-extractor-config

Some learnings

• Doing the conversion to a custom rejection with an async fn would be cool because then you can run extractors. However I cannot get that working with extensions (2). You run into lifetime issues where the future returned from a boxed function needs to borrow from one of the inputs which you cannot do:

  struct JsonConfig {
      rejection_handler: Arc<
        dyn for<'a> Fn(JsonRejection, &'a mut RequestParts<B>) -> BoxFuture<'a, Response>
                                                                            ^^ cannot use 'a here
      >,
  }

  Not sure if this is a showstopper but its a little annoying for sure.

• Doing async stuff is possible with phantom types (3), however you cannot use type aliases as constructors so this doesn't work:

  type Json<T> = axum_extractor_config::Json<T, CustomRejection>;
  
  async fn handler(Json(value, _): Json<Value>) {}
                   ^^^^ cannot use type alias as constructor
  
  // so you have to do this everywhere =(
  async fn handler(Json(value, _): Json<Value, CustomRejection>) {}
  
  // or this which is super weird
  async fn handler(axum_extractor_config::Json(value, _): Json<Value>) {}

  I think this is a showstopper. Having to write Json<Value, CustomRejection> everywhere is pretty annoying.

[jplatte]

you cannot use type aliases as constructors

Have you checked whether there are any bug reports about this on rust-lang/rust? It looks more like a compiler limitation to me rather than an intentional design decision.

[davidpdrsn]

Have you checked whether there are any bug reports about this on rust-lang/rust? It looks more like a compiler limitation to me rather than an intentional design decision.

No I haven't checked but yeah does feel like something that should work.

I've been thinking that we could probably improve things a bit by making it possible to override the rejection in #[derive(FromRequest)] like so:

#[derive(FromRequest)]
#[from_request(
  via(axum::Json),
  rejection(MyRejection),
)]
struct Json<T>(T);

struct MyRejection { ... }

impl From<JsonRejection> for MyRejection {
  ...
}

impl IntoResponse for MyRejection {
  ...
}

While its not an ideal solution it does remove most of the boilerplate. I think we should support this regardless but might be good enough until the necessary features stabilize.

[davidpdrsn]

I've implemented what I posted above in https://github.com/tokio-rs/axum/pull/1256. Still not an ideal solution but better than before.

[Altair-Bueno]

2. Add a defaulted generic type parameter to the extractors, something like

struct Json<T, R = PlainTextJsonRejection>(T);

where R has to implement both From<serde_json::Error> and IntoResponse

• Rejected because it isn't possible with Rust right now, generic type parameters on a struct need to be mentioned in one of the fields, but adding a PhantomData<R> breaks usage of Json(value) as a pattern and constructor

I think I'm a bit late, but there is one other alternative: Instead of breaking the current extractors, add an "extractor of extractors". Something like this:

use std::{
    error::Error, marker::PhantomData,
};

use axum::{
    async_trait,
    extract::{FromRequest, RequestParts},
    http::StatusCode,
    response::IntoResponse,
    routing::get,
    Json, Router, Server,
};
use serde_json::{json, Value};

// Generic extractor
struct ExtractWith<E, S>(pub E, pub PhantomData<S>);

#[async_trait]
impl<B, E, S> FromRequest<B> for ExtractWith<E, S>
where
    B: Send,
    E: FromRequest<B>,
    E::Rejection: Error,
    S: Strategy<E::Rejection>,
{
    type Rejection = S::Response;

    async fn from_request(req: &mut RequestParts<B>) -> Result<Self, Self::Rejection> {
        // Applies extractor. If it fails, applies strategy
        match req.extract::<E>().await {
            Ok(x) => Ok(ExtractWith(x, Default::default())),
            Err(x) => Err(S::apply(x)),
        }
    }
}

pub trait Strategy<E: Error> {
    type Response: IntoResponse;

    fn apply(err: E) -> Self::Response;
}

// User implements its own strategy, Format the error as they like. Different
// endpoints have different error responses.
pub struct MyStrategy;

impl<E: Error> Strategy<E> for MyStrategy {
    type Response = (StatusCode, Json<Value>);

    fn apply(err: E) -> Self::Response {
        let value = json!({
            "err": err.to_string()
        });
        (StatusCode::INTERNAL_SERVER_ERROR, Json(value))
    }
}

async fn handler(
    ExtractWith(Json(json), _): ExtractWith<Json<()>, MyStrategy>,
) -> impl IntoResponse {
    ()
}

#[tokio::main]
async fn main() {
    let app = Router::new().route("/", get(handler));

    Server::bind(&([127, 0, 0, 1], 8080).into())
        .serve(app.into_make_service())
        .await
        .unwrap()
}

This code compiles today, and is far more customisable.

Pros

• Works with almost all already existing extractors (even external libraries!). Most rejections already implement std::error::Error. There is no need to change the ecosystem, no breaking changes
• Easy mix and reformat. If you ever plan on replacing your strategy or combining multiple, you can use type aliases incrementally
• Performant. Compile time monomorphization
• MVC Separation of concerns. Model doesn't get mixed with axum

Cons

• Ugly Phantomdata gets extracted too
• No runtime configuration. What you compile is what you will get
• No internal state. Because we never instantiate the strategy, there is no internal state
• One more trait: Not sure if it could be replace with an std trait
• Requires std::error::Error
• The type gets a bit verbose. Type alias should fix this. type ApiWithStratety<E> = WithStrategy<E,MyRejectStrategy>

[jplatte]

I like that, I think we can easily add it to axum-extra, although I think it we should name things differently, and not impose a std::error::Error constraint (it can still exist on an impl like you showed even if it the trait isn't constrained):

struct WithRejection<E, R>(pub E, pub PhantomData<R>);

impl<E, R> WithRejection<E, R> {
    fn into_inner(self) -> E { self.0 }
}

// impl Deref, DerefMut

#[async_trait]
impl<B, E, R> FromRequest<B> for ExtractWith<E, R>
where
    E: FromRequest<B>,
    E::Rejection: Into<T>,
{
    type Rejection = R;

    async fn from_request(req: &mut RequestParts<B>) -> Result<Self, Self::Rejection> {
        match req.extract::<E>().await {
            Ok(v) => Ok(ExtractWith(v, PhantomData)),
            Err(r) => Err(r.into()),
        }
    }
}

[Altair-Bueno]

I think it we should name things differently

Yea im terrible at naming things haha

and not impose a std::error::Error constraint (it can still exist on an impl like you showed even if it the trait isn't constrained)

Good catch!

[davidpdrsn]

Thats cool! Adding something like that to axum-extra gets a 👍 from me.

[Altair-Bueno]

Is anyone working on this? I can open a pull request with this change, if you are okay with it.

I only have one small concern with @jplatte version, regarding impl Deref and DerefMut. As stated by its docs:

On the other hand, the rules regarding Deref and DerefMut were designed specifically to accommodate smart pointers. Because of this, Deref should only be implemented for smart pointers to avoid confusion. ^1

I think AsRef and AsMut are the correct traits

[davidpdrsn]

Go for it!

I think it should implement Deref and DerefMut, like the other extractors in axum. Afaik that guideline is generally considered not best practice anymore. I believe even std itself violates it in a few places (don't remember where)

[jplatte]

Hehe yeah, the rules for Deref and DerefMut are not really fully settled yet. A while ago (should be fixed now), there was actually a circular definition kind of situation, with some official docs saying that Deref and DerefMut are (only) supposed to be implemented by "smart pointers" without defining that, and a different part of official docs defining smart pointers as types that implement Deref¹ 😄

¹ I find this definition quite absurd, it doesn't even match my intutition for "smart pointer" even for basic examples such as Vec, but Cow and AssertUnwindSafe are worse offenders for not even being pointers IMO (so how can they be smart pointers)

[Altair-Bueno]

Thanks for the explanation! I will look up more info about this topic, looks interesting

[Altair-Bueno]

I've been playing a bit more with WithRejection today and I think i've discovered a nice feature that I believe is worth mentioning somewhere.

Because WithRejection uses From<E::Rejection>, it is possible to use the thiserror crate to transform extractor rejections the same way it is used to transform Err variants. This means that you can now write one single enum and handle all rejections with it!

use axum::http::StatusCode;
use axum::response::IntoResponse;
use axum::routing::post;
use axum::*;
use axum_extra::extract::WithRejection;
use chrono::{DateTime, Utc};
use serde::Serialize;
use serde_json::Value;

async fn handler(
    WithRejection(json, _): WithRejection<Json<Value>, ApiError>,
) -> impl IntoResponse {
    json
}

#[tokio::main]
async fn main() {
    let app = Router::new().route("/", post(handler));

    Server::bind(&([127, 0, 0, 1], 8080).into())
        .serve(app.into_make_service())
        .await
        .unwrap()
}

// Defines all the possible error responses from the API, including extractor responses
#[derive(thiserror::Error, Debug)]
#[non_exhaustive]
enum ApiError {
    #[error(transparent)]
    JsonDeserialization(#[from] axum::extract::rejection::JsonRejection),
    // More extractor errors and API errors
}

#[derive(Debug, Serialize)]
struct ApiErrorPayload {
    message: String,
    timestamp: DateTime<Utc>,
    docs: String,
}

impl IntoResponse for ApiError {
    fn into_response(self) -> response::Response {
        let now = Utc::now();
        let (code, value) = match self {
            // Some sample responses you might see on production REST apis
            Self::JsonDeserialization(x) => (
                StatusCode::BAD_REQUEST,
                ApiErrorPayload {
                    message: x.to_string(),
                    timestamp: now,
                    docs: "https://example.org/errors#JsonDeserialization".into(),
                },
            ),
            _ => (
                StatusCode::INTERNAL_SERVER_ERROR,
                ApiErrorPayload {
                    message: "Unknown error".into(),
                    timestamp: now,
                    docs: "https://example.org/contact".into(),
                },
            ),
        };
        (code, Json(value)).into_response()
    }
}

Cargo.toml for anyone interested on trying this now ```toml [package] name = "sample" version = "0.1.0" edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] axum = { git = "https://github.com/tokio-rs/axum", rev="fb215616" } axum-extra = { git = "https://github.com/tokio-rs/axum", rev="fb215616" } axum-macros = { git = "https://github.com/tokio-rs/axum", rev="fb215616" } tokio = { version = "1.20.1", features = ["full"] } serde = { version = "1.0.143", features = ["derive"] } serde_json = "1.0.83" thiserror = "1.0.32" chrono = { version = "0.4.22", features = ["serde"] } ```

Imho we could close this issue for now, given how ergonomic/powerful combining WithRejection + thiserror is.

Maybe we should include a link to this comment somewhere on the docs so people can find this example easily?

[davidpdrsn]

Yeah thats pretty cool. Takes most of the boilerplate out of it.

Imho we could close this issue for now, given how ergonomic/powerful combining WithRejection + thiserror is.

I think we should keep it open. I don't think having to write in all your handlers WithRejection(Json(person), _): WithRejection<Json<Person>, MyRejection> is ideal. Its quite foreign looking syntax, even though it does work.

Ideally const generics would be more powerful so we could do Json(person): Json<Person, MyRejection> and type Json<T> = axum::Json<T, MyRejection> but thats not possible today, hence the S-blocked label.

[davidpdrsn]

@Altair-Bueno however I think we can probably extend https://github.com/tokio-rs/axum/blob/main/examples/customize-extractor-error/src/main.rs to show the three solutions:

• Manually making a new extractor, i.e. what the example currently shows
• Using #[derive(FromRequest)] like shown here
• Using WithRejection perhaps combined with thiserror.

I think if we do that we can remove the 0.6 milestone from this issue. Not sure we have many other options and I'm okay with these options.

Do you wanna work on that?

[Altair-Bueno]

Do you wanna work on that?

Sure!

however I think we can probably extend main/examples/customize-extractor-error/src/main.rs to show the three solutions:

You mean one crate per solution, right? We are gonna need some good self-explanatory names

While we are at it, we might also refactor some examples to make things easier:

• customize-extractor-error and customize-path-rejection: Choose one of them and rename it to custom-extractor(?). I don't think is worth it to maintain those two at the same time, they are too alike.
• error-handling-and-dependency-injection: Break down this example
  • error-handling(?): Using thiserror, WithRejection and some custom error. Include links to custom-extractor and derive-from-request with small summary highlighting the cons and pros of each approach
  • dependency-injection: Just the dependency injection
• derive-from-request(?): An example showcasing #[derive(FromRequest)], including the error handling feature

[davidpdrsn]

You mean one crate per solution, right? We are gonna need some good self-explanatory names

I don't think we need three separate crates. We can just do three modules in one crate.

[davidpdrsn]

I don't think we should change other examples.

[Altair-Bueno]

I don't think we should change other examples.

Roger, i've created another crate for #1276. I still think there is too much overlapping between examples to my taste, but is something we judge later

[davidpdrsn]

With https://github.com/tokio-rs/axum/pull/1276 merged (thanks @Altair-Bueno) I think we can remove this from the 0.6 milestone.

[heat1q]

I've been following this issue for a while now I must say that the options for implementing custom rejections already improved greatly! In particular WithRejection and #[derive(FromRequest)] make it much easier to use custom API errors as rejections.

What I am still missing (or rather what would be nice to have) is a way to derive a From<Rejection> for my custom API error. It's really cool that you can already do this with thiserror when your error is an enum, but you still have to manually deconstruct the rejection if you want to use a different structure.

Often you simply want to take whatever status and body the axum rejection has and put it in your custom type. The axum rejections already categorizes the different error cases nicely (status code and message) and you might want to keep them as is.

Based on this example the derive signature could look something like

#[derive(FromRejection)]
#[from_rejection(via(JsonRejection))]
struct ApiError {
    #[rejection(status_code)]
    code: StatusCode,
    #[rejection(message)]
    message: String,
}

where you simply tell where the rejection status_code and message (aka body_text) should go. Similar to #[derive(FromRequest)] one could also add #[from_rejection(via(...),...) to specify for which rejection a From implementation should be derived.

I've already implemented a rough draft of this derive macro and found it quite handy. Now I'm wondering if this would also be of use to others and included into axum-macros. What do you think?

[Altair-Bueno]

You don't have to deconstruct the rejection, just use a struct instead of an enum and impl From<RejectionType> for MyCustomRejection

If you still want to go the macro route, i think a declarative macro is better suited as they are more performant and easier to read

Full cargo project

pub async fn handler(
    WithRejection(Json(value), _): WithRejection<Json<Value>, CustomRejection>,
) -> impl IntoResponse {
    Json(dbg!(value))
}

#[derive(Debug)]
pub struct CustomRejection {
    message: String,
    code: StatusCode,
}

// We implement `IntoResponse` so ApiError can be used as a response
impl IntoResponse for CustomRejection {
    fn into_response(self) -> axum::response::Response {
        let payload = json!({
            "message": self.message,
            "origin": "with_rejection"
        });
        (self.code, Json(payload)).into_response()
    }
}

// Manual impl
impl From<JsonRejection> for CustomRejection {
    fn from(value: JsonRejection) -> Self {
        Self {
            code: value.status(),
            message: value.body_text(),
        }
    }
}

// Generate a bunch with a simple declarative macro, cheaper than proc_macro
macro_rules! gen_from_rejection {
    ($from:ty, $rejection:ty ) => {
        impl From<$from> for $rejection {
            fn from(value: $from) -> Self {
                Self {
                    code: value.status(),
                    message: value.body_text(),
                }
            }
        }
    };
}

gen_from_rejection!(FormRejection, CustomRejection);
gen_from_rejection!(BytesRejection, CustomRejection);

Another solution (that would require breaking changes to axum itself) would be to introduce another trait (AxumRejection) that would make the following code possible. This would leverage monomorphization instead of macros

trait AxumRejection {
    fn status(&self) -> StatusCode;
    fn body_text(&self) -> String;
}

// Manual impl
impl<T: AxumRejection> From<T> for CustomRejection {
    fn from(value: T) -> Self {
        Self {
            code: value.status(),
            message: value.body_text(),
        }
    }
}

Note: IntoResponse cannot be used as the compiler yields "conflicting implementations of trait From<CustomRejection> for type CustomRejection"

As a final note, I strongly believe that proc/derive macros shouldn't be used unless you have a really good reason. And even then, it's probably a bad idea.

[heat1q]

Thanks for the answer, really appreciate the feedback. I understand the cost of proc macros, it was just an idea I wanted to explore. I like your suggestion of a simple declarative macro that I can easily apply to my use cases.

[davidpdrsn]

As a final note, I strongly believe that proc/derive macros shouldn't be used unless you have a really good reason. And even then, it's probably a bad idea.

While I don't agree with the sentiment that proc-macros are always a bad idea (#[derive(Serialize, Deserialize)] 👀) I do agree that we probably don't need a proc-macro for this.

[saiintbrisson]

I'm unsure if this has come up yet, but I've added the rejection as a response extension with about 15 lines of code, just changing how the define_rejection macro implements IntoResponse. No clones or copies were required, nor were any checks.

It allows the developer to customize it however it wants if it wants to. My company is switching to Axum and having a global way to handle rejections is a decently important thing for us.

I'll open a PR to gather comments and opinions about it, but here's a general idea:

async fn handle_rejection<B>(req: Request<B>, next: Next<B>) -> Response {
    let resp = next.run(req).await;

    if let Some(rejection) = resp.extensions().get::<JsonRejection>() {
        let payload = json!({
            "message": rejection.body_text(),
            "origin": "response_extension"
        });

        return (resp.status(), axum::Json(payload)).into_response();
    }

    resp
}

[kurtbuilds]

Out of the 3 options for customizing errors in the examples (https://github.com/tokio-rs/axum/blob/main/examples/customize-extractor-error/src/main.rs?rgh-link-date=2022-08-18T12%3A26%3A54Z), I went with #3.

However, the examples appear out of date. (Were they written for axum 0.5?)

Here's the updated example:

pub struct Json<T>(pub T);

// optional, but I find this helpful.
impl<T> std::ops::Deref for Json<T> {
    type Target = T;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

#[async_trait]
impl<T, S, B> FromRequest<S, B> for Json<T>
    where
        T: DeserializeOwned,
        B: HttpBody + Send + 'static,
        B::Data: Send,
        B::Error: Into<BoxError>,
        S: Send + Sync,
{
    type Rejection = ErrorResponse;

    async fn from_request(req: Request<B>, state: &S) -> Result<Self, Self::Rejection> {
        match axum::Json::<T>::from_request(req, state).await {
            Ok(axum::Json(value)) => Ok(Self(value)),
            Err(rejection) => Err(ErrorResponse {
                code: rejection.status(),
                error: "Client Error".to_string(),
                message: rejection.to_string(),
            })
        }
    }
}

[davidpdrsn]

The main branch has breaking changes. Like mention at the top of the readme you have to switch to the 0.6.x branch.

[kurtbuilds]

Got it, yeah I see that now. I was just looking on the link posted earlier in the thread. The breaking changes appear to simplify the code, so I'm excited for those to roll out!

[nbari]

I'm unsure if this has come up yet, but I've added the rejection as a response extension with about 15 lines of code, just changing how the define_rejection macro implements IntoResponse. No clones or copies were required, nor were any checks.

It allows the developer to customize it however it wants if it wants to. My company is switching to Axum and having a global way to handle rejections is a decently important thing for us.

I'll open a PR to gather comments and opinions about it, but here's a general idea:

async fn handle_rejection<B>(req: Request<B>, next: Next<B>) -> Response {
    let resp = next.run(req).await;

    if let Some(rejection) = resp.extensions().get::<JsonRejection>() {
        let payload = json!({
            "message": rejection.body_text(),
            "origin": "response_extension"
        });

        return (resp.status(), axum::Json(payload)).into_response();
    }

    resp
}

Hi, can you please share an example of how are you implementing it

[saiintbrisson]

@nbari this was done in #1932

[nbari]

Hi @saiintbrisson many thanks, I just found out that I could use an Option something like:

async fn foo(payload: Option<Json<Client>>) -> impl IntoResponse {
let data = match payload {
        Some(Json(payload)) =>{
        ...
        },

        None => {
            return Err((
                StatusCode::INTERNAL_SERVER_ERROR,
                "My message .....".to_string(),
            ));
        }
    };
...
}

Any thoughts on it?

[saiintbrisson]

@nbari I'm not sure this will behave as you expect. If I were to guess, I'd say the option will only be None if there's no JSON payload at all. If the payload is present but contains invalid JSON, the response will still be the plaintext one.

Actually, you are right. The downside is having to add the result handling everytime you have a Json param. I'd rather go with the custom extractor.

[mpfaff]

* Doing async stuff is possible with phantom types (3), however you cannot use type aliases as constructors so this doesn't work:
  ```rust
  type Json<T> = axum_extractor_config::Json<T, CustomRejection>;

  async fn handler(Json(value, _): Json<Value>) {}
                   ^^^^ cannot use type alias as constructor

  // so you have to do this everywhere =(
  async fn handler(Json(value, _): Json<Value, CustomRejection>) {}

  // or this which is super weird
  async fn handler(axum_extractor_config::Json(value, _): Json<Value>) {}
  ```

  I think this is a showstopper. Having to write `Json<Value, CustomRejection>` everywhere is pretty annoying.

@davidpdrsn An alternative syntax, in case it makes adding a generic parameter more tolerable:

let handler = |Json::<Value, CustomRejection>(value, _)| async move {}

As far as I know, it only really helps with closures, where you can omit the type and just write the pattern.

If you have a struct you want to destructure, it can look even nicer, depending on your perspective:

let handler = |Json::<_, CustomRejection>(LoginParams { username, password }, _)| async move {}

[Victor-N-Suadicani]

Nothing serious, but my stomach tells me to avoid exposing internal details of any kind in public environments, at least as a default.

Just ran into this as well.

It seems there's a lot of design decisions to make when it comes to custom rejections, but it would be nice if there at least was an option to remove the plain text error messages in release mode. I'm honestly completely fine with returning just an error code and an empty body in many cases 🙂. Often the underlying error can be discerned from the status code without exposing implementation details of the server.

So perhaps while an actual custom rejection feature is implemented (which seems could take a while, considering the age of this issue), it would be nice to at least have an option to disable the exposing of implementation details without having to make custom extractors for all the in-built extractors 😅.

I considered just logging the errors and return empty responses but that makes it way harder to discover and will likely cause more confusion.

I would much prefer this option and you could also make it so it only returns the empty responses in release mode and includes the plain text error messages in debug mode. I feel an error log on the server side is plenty to make it discoverable.

[jplatte]

I would much prefer this option and you could also make it so it only returns the empty responses in release mode and includes the plain text error messages in debug mode. I feel an error log on the server side is plenty to make it discoverable.

As stated long ago:

People don't always setup tracing to begin with.

So it's not a given that people would have logs to see the error in.

I wonder if it would be an option to use a response extension to communicate the fact that one of the builtin extractors failed, such that downstream middleware can then clear or modify the response body based on that knowledge. This would be pretty similar to https://github.com/tokio-rs/axum/pull/2468.

[saiintbrisson]

I wonder if it would be an option to use a response extension to communicate the fact that one of the builtin extractors failed

Isn't this similar to #1932?

EDIT: though I get the points made by David, I still think this is one of the better options. I can re-open the PR if anything changes

[jplatte]

Yes, very similar. I had forgotten about that one. So I guess that option is not really on the table unless David changes his mind.

[Victor-N-Suadicani]

I'm certain we're gonna get so many questions about it otherwise. People don't always setup tracing to begin with.

Are you really sure there are so many people that are not setting up logging? It sounds unlikely to me. The way you say "we're gonna get" suggests that it has not happened yet and it is more of a guess? 🤔

I'm not sure I quite understand the argument - you're saying that an error from the library is too difficult to find even if it's in the logs, because people might just not setup logging? I feel like you could also say that people aren't going to look at the response plain text error message in that case, if we're assuming that people are not looking for the errors in the places they should. Personally logs would be the very first place I would look for errors (in particular I would not assume server errors to be exposed in responses), but maybe I am assuming too much? 🤷

Is this a hypothetical worry or are there actually legitimately many people who do not log errors from their libraries?

[Altair-Bueno]

Are you really sure there are so many people that are not setting up logging? It sounds unlikely to me. The way you say "we're gonna get" suggests that it has not happened yet and it is more of a guess? 🤔

There was a discussion not long ago that all examples on the repo should include some sort of error handling (instead of unwrap or expect) because people just copy and paste code without knowing what they are doing... So yes, I would assume that all developers with little background in Rust are likely to NOT set up tracing.

Personally logs would be the very first place I would look for errors

Contrary to other languages and frameworks, logging is not built in nor automatically configured. So again, someone with little know how is likely to miss those.

---

My take on this issue is that current behavior is okay-ish. Yes, it is not ideal to see implementation details on an error response. However, thats the default behavior on some server frameworks. Off the top of my head, Java EE (now Jakarta EE) + Glassfish and Spring throw error pages with full Java stack traces. Of course, there are configuration options to hide those, but even then, lots of real world applications still have them enabled for some reason.

[Victor-N-Suadicani]

I don't think only having it in logs would be a big problem but maybe I'm wrong - but perhaps we could at least agree on an opt-in feature flag to disable the plain text error bodies (and log them instead)? At least that way I wouldn't have to redefine all the extractors in my own code, I would just enable that flag and that's it.

[Altair-Bueno]

At least that way I wouldn't have to redefine all the extractors in my own code, I would just enable that flag and that's it.

There is WithRejection. I designed it for this purpose. You just need to write either a thiserror enum, or your own error type and From impls. Does that feel like too much boilerplate for you?

PS: sorry if you already mentioned WithRejection. The issue is too large to keep up with.

[Victor-N-Suadicani]

There is WithRejection. I designed it for this purpose. You just need to write either a thiserror enum, or your own error type and From impls. Does that feel like too much boilerplate for you?

That's better than defining my own but it is quite a bit of boilerplate and I need to remember to use it across all of my routes... A feature flag would be much nicer as it would require no code changes.

[Altair-Bueno]

A feature flag would be much nicer as it would require no code changes.

Thats one reason im skeptical about adding said feature flag: people will just turn error reporting off and call it a day. I don't think we should endorse bad API design patterns (e.g. and API that just says 500 Internal Server Error).

Code changes would be required latter down the line (custom extractors, WithRejection,...). I think its better to do it beforehand and be explicit on how your application behaves, before its complexity grows.

[Victor-N-Suadicani]

I would assume the people who would use the feature flag would have logging setup and would hence see the error in the logs - so error reporting would not just be turned off in that sense. Docs about the feature flags should make this clear.

I don't think returning a bare 500 is bad API design honestly. The client (in many cases) has no business knowing what went wrong - it is an internal server error after all. I think exposing implementation details is much worse API design and that's already happening.

[oxalica]

2. Add a defaulted generic type parameter to the extractors, something like

struct Json<T, R = PlainTextJsonRejection>(T);

Rejected because it isn't possible with Rust right now, generic type parameters on a struct need to be mentioned in one of the fields, but adding a PhantomData breaks usage of Json(value) as a pattern and constructor

This reminds me the magic of inventory[^1] which enables for v in inventory::iter::<T>. That's a unused phantom generic parameter, but with a legit though magic workaround from compiler's enforcement. By applying the similar trick, we can do:

// Use the same name from either type and value namespace, then merge.
// So they will not collide.
pub type Json<T, R> = private::Json<T, R>;
#[doc(hidden)]
pub use private::*;

mod use_value {
    // Only pull in the constructor `fn(E) -> Json<E, R>` in value namespace.
    #[doc(hidden)]
    pub use super::private::Json::Json;
}

mod private {
    use std::convert::Infallible;
    use std::marker::PhantomData;

    #[doc(hidden)]
    pub use super::use_value::*;

    pub enum Json<T, R> {
        // Make this variant uninhabitable, see below.
        #[doc(hidden)]
        __Phantom(Infallible, PhantomData<R>),
        #[doc(hidden)]
        Json(T),
    }
}

For users, they could keep the same code:

use axum::Json;

// User could also do the same split-`use`-trick here to create
// an type alias `Json` with a different default rejection type.

// Magic: the type `Json` is an enum with only one variant (`Json`)
// inhabitable, so with `min_exhaustive_patterns`,
// this is actually irrefutable and can be accepted by the compiler.
async fn handler(Json(v): Json<i32>) { // Or `Json<i32, Custom>`
    todo!()
}

The code above works on 1.82.0-beta.5. The only downside is min_exhaustive_patterns is only available in next stable release 1.82, scheduled on the next week 2024-10-17. If the high MSRV requirement is not acceptable, we could gate it under a default-disabled feature: this new Json type should be compatible with the current one IIUC. The same trick can also be applied to axum_extra::WithRejection and others.

[^1]: The trick is further based on ghost, as the comments in inventory say.

[jplatte]

I love it x)

I think it's a bit too experimental to make it into the upcoming release though. Would you be willing to publish copies of our builtin extractors using this technique as a separate crate? Then people could try them with v0.7 / v0.8 of axum (whichever one you want to support) and we can consider importing them for v0.9.