actix-web icon indicating copy to clipboard operation
actix-web copied to clipboard

Make it easier to consistently output JSON object from all endpoints

Open Ploppz opened this issue 5 years ago • 7 comments

I think it's currently possible, but it should optimally be easier, and I don't know if I can make sure that all holes are covered. The goal is that all endpoints always output a JSON object (perhaps except in the case of 204 No Content). Let's say that in the case of any error, we want

{
    "code": <http status code>
    "message": <any error message (string)>
}

The first thing I tried to this end was to simple create one single middleware that should cover all cases: https://gist.github.com/Ploppz/54e65eea580f17764c7560a3e7190141 This one simply checks if the status code of a response is not success, then it puts the whole body into the "message" field of the above JSON structure. This mostly works because

  1. actix-web simply returns the error as string
  2. we impl ResponseError like this
impl ResponseError for MyError {
    fn error_response(&self) -> HttpResponse {
        use Error::*;
        let status = match self {
            //omitted code
        };
        HttpResponse::with_body(status, Body::from(&self.to_string()))
    }
}

However, in some cases I found it desirable to return a status like 404 or 503 while also already returning JSON in the request handler. This does not work well in this solution, because it will put the JSON into the "message" field of the above JSON structure. This solution assumes that when a request handler returns Result<T, E>, T always means JSON with 2xx HTTP code and E always means an error that is only string.

So I think this is the way to go to cover most cases right now:

  • impl ResponseError for MyError where we return Result<_, MyError> from all endpoints; this implementation will serialize said JSON format with self.to_string() in the "message" json field.
  • Use JsonConfig to cover most actix-generated 400 errors.
  • Iirc there's something to set custom 404 response

It took me a lot of effort to just get to this point to make a consistent API, and I'm not even sure if it will always work well - I don't know all possible cases in which actix-web can throw an error before invoking a request handler.

I wish there was a way to tell actix-web to transform absolutely all error responses in the same way, rather than e.g. only configuring the Json extraction error handling.

Otherwise, is it possible to adapt the ErrorHandler middleware I provided to only transform errors that stem from actix-web? As far as I can see, it's not possible to see whether an error is generated by a request handler or by actix-web in a middleware.

Ploppz avatar Jul 14 '20 14:07 Ploppz

This is how I did it: WebResult

Would something like this help you?

mocsy avatar Jul 21 '20 15:07 mocsy

Thanks. Indeed something like that is one way to make sure that the types you return result in the correct response. Then the question is only how to cover all the 'holes' of actix_web. Starting anew with

#[derive(Serialize, Deserialize, Debug, Clone)]
pub struct ErrorResponse {
    pub code: u16,
    pub message: String,
}
fn actix_handle_err<T: std::error::Error + 'static>(err: T) -> actix_web::error::Error {
    let response = HttpResponse::BadRequest().json(ErrorResponse {
        code: 400,
        message: err.to_string(),
    });
    actix_web::error::InternalError::from_response(err, response).into()
}
// ....
            .app_data(
                web::JsonConfig::default()
                    .limit(1024 * 1024 * 10)
                    .error_handler(|err, _req| actix_handle_err(err)),
            )
            .app_data(web::QueryConfig::default().error_handler(|err, _req| actix_handle_err(err)))
            .app_data(web::PathConfig::default().error_handler(|err, _req| actix_handle_err(err)))

I didn't find a way to customize the default 404 response. And I wonder, will actix-web throw 500 sometimes? If a handler panics I think it will not respond with anything at all, but maybe there would be other causes of a 500 from actix-web? Any other error conditions in actix-web that need to be covered?

Ploppz avatar Aug 11 '20 09:08 Ploppz

@Ploppz

The actix official way: https://docs.rs/actix-web/3.3.2/actix_web/middleware/errhandlers/struct.ErrorHandlers.html

fn more_handler<B>(mut res: ServiceResponse<B>) -> Result<ErrorHandlerResponse<B>> {
    let status = res.status();
    res = res.map_body::<_, B>(|_, _| {
        ResponseBody::Other(
            format!(
                r#"{{"code":{},"message":"{}"}}"#,
                status.as_u16(),
                status.canonical_reason().unwrap_or("")
            )
            .into(),
        )
    });
    Ok(ErrorHandlerResponse::Response(res))
}
// ...skip...
           .wrap(
                ErrorHandlers::new()
                    .handler(http::StatusCode::NOT_FOUND, more_handler)
                    .handler(http::StatusCode::INTERNAL_SERVER_ERROR, more_handler)
                    // umm...
            )

up9cloud avatar Dec 15 '20 09:12 up9cloud

I have a way on 3, but can't figure it in 4. I think the defaults here are "wrong". The normal (in websites) is to have an error page for all errors and one for 404. So the ability to pinpoint a exact code is nice, but is overkill to specify all of it.

This is what I done in 3 (Note how using if res.status().is_client_error() || res.status().is_server_error() cover all major cases):

#![allow(clippy::redundant_allocation)]
use std::rc::Rc;
use std::task::{Context, Poll};

use actix_service::{Service, Transform};
use actix_web::dev::{ServiceRequest, ServiceResponse};
use actix_web::http::HeaderMap;
use actix_web::Error;
use actix_web::Result;
use futures::future::{ok, FutureExt, LocalBoxFuture, Ready};

/// Error handler response
pub enum ErrorHandlerResponse<B> {
    /// New http response got generated
    Response(ServiceResponse<B>),
    /// Result is a future that resolves to a new http response
    Future(LocalBoxFuture<'static, Result<ServiceResponse<B>, Error>>),
}

pub type ErrorHandler<B> =
    dyn Fn(ServiceResponse<B>, &HeaderMap) -> Result<ErrorHandlerResponse<B>>;

pub struct ErrorHandlers<B> {
    handler: Rc<Box<ErrorHandler<B>>>,
}

impl<B> ErrorHandlers<B> {
    /// Construct new `ErrorHandlers` instance
    pub fn new(handler: Box<ErrorHandler<B>>) -> Self {
        let handler = Rc::new(handler);
        ErrorHandlers { handler }
    }
}

impl<S, B> Transform<S> for ErrorHandlers<B>
where
    S: Service<Request = ServiceRequest, Response = ServiceResponse<B>, Error = Error>,
    S::Future: 'static,
    B: 'static,
{
    type Request = ServiceRequest;
    type Response = ServiceResponse<B>;
    type Error = Error;
    type Transform = ErrorHandlersMiddleware<S, B>;
    type InitError = ();
    type Future = Ready<Result<Self::Transform, Self::InitError>>;

    fn new_transform(&self, service: S) -> Self::Future {
        ok(ErrorHandlersMiddleware {
            service,
            handler: self.handler.clone(),
        })
    }
}

#[doc(hidden)]
pub struct ErrorHandlersMiddleware<S, B> {
    service: S,
    handler: Rc<Box<ErrorHandler<B>>>,
}

impl<S, B> Service for ErrorHandlersMiddleware<S, B>
where
    S: Service<Request = ServiceRequest, Response = ServiceResponse<B>, Error = Error>,
    S::Future: 'static,
    B: 'static,
{
    type Request = ServiceRequest;
    type Response = ServiceResponse<B>;
    type Error = Error;
    type Future = LocalBoxFuture<'static, Result<Self::Response, Self::Error>>;

    fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
        self.service.poll_ready(cx)
    }

    fn call(&mut self, req: ServiceRequest) -> Self::Future {
        let headers = req.headers().clone();
        let handler = self.handler.clone();
        let fut = self.service.call(req);
        async move {
            let res = fut.await?;

            if res.status().is_client_error() || res.status().is_server_error() {
                match handler(res, &headers) {
                    Ok(ErrorHandlerResponse::Response(res)) => Ok(res),
                    Ok(ErrorHandlerResponse::Future(fut)) => fut.await,
                    Err(e) => Err(e),
                }
            } else {
                Ok(res)
            }
        }
        .boxed_local()
    }
}

mamcx avatar Feb 23 '22 20:02 mamcx

One thing I found is that if you are using ErrorHandler middleware to convert 500 errors to JSON, then it will only work if all the middlewares further up the chain return OK:

https://github.com/actix/actix-web/blob/master/actix-web/src/middleware/err_handlers.rs#L157

Which is how middleware should work but it is easy to miss this when writing your own.

jacob-pro avatar Apr 18 '22 16:04 jacob-pro

The other places that you need to cover JSON are for 404s and 405s.

As far as I can see there are two options:

  1. You make sure all you web::scope and web::resource have default services, e.g.
fn api_scope(path: &str) -> actix_web::Scope {
    web::scope(path).default_service(web::route().to(|| async {
        // JSON not found response
    }))
}
  1. You use an error handler, but you need to think about how it will effect any 404s you create yourself, e.g.
fn handle_404<B>(res: ServiceResponse<B>) -> Result<ErrorHandlerResponse<B>, actix_web::Error> {
    if res.response().error().is_some() {
        // This is an error returned from a route, we therefore assume it is already in JSON format
        Ok(ErrorHandlerResponse::Response(res.map_into_left_body()))
    } else {
        // When actix couldn't match the path to any route at all
        Ok(ErrorHandlerResponse::Response(
            res.error_response(
                // 404 JSON here
            )
                .map_into_right_body(),
        ))
    }
}

jacob-pro avatar Apr 18 '22 16:04 jacob-pro