Bug: Body provided in ResponseError implementation is not being honored.

[crmckenzie]

When attempting to serialize a custom error to json as part of the ResponseError implementation, actix-web obliterates the body I provided in favor of the fmt::Display implementation for the same struct. While I appreciate that actix-web wants to coerce my error response to a body on my behalf, I would expect that if I have provided a body that actix-web would not replace it later.

Here is the interesting bit:

#[derive(Fail, Serialize)]
pub struct MyError {
   pub name: &'static str,
   pub message: &'static str
}

impl ResponseError for MyError {
    fn error_response(&self) -> HttpResponse {
        dbg!("ResponseError.error_response(&self) being called.");
        // I'm providing a response body which is not being honored.
        // This is what I expect to see as the response.
        HttpResponse::InternalServerError()
            .json(self)
    }
}

impl fmt::Display for MyError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        // This is being chosen as the response body instead of the serialized response
        dbg!("fmt::Display being called for MyError");
        write!(f, "({}, {})", self.name, self.message)
    }
}

Here is the curl command:

curl http://localhost:8088/error
(test error, If this message is being displayed in the response body there is a problem.)

Here is the logging output. You can see that fmt::Display is being invoked after the ResponseError transformation.

[experimentation/src/main.rs:19] "ResponseError.error_response(&self) being called." = "ResponseError.error_response(&self) being called."
[experimentation/src/main.rs:30] "fmt::Display being called for MyError" = "fmt::Display being called for MyError"
[2019-06-05T20:05:37Z INFO  actix_web::middleware::logger] 127.0.0.1:58038 "GET /error HTTP/1.1" 500 89 "-" "curl/7.58.0" 0.000282

Here is an entire working example.

use std::fmt;
use actix_web::{web, App, Error, HttpResponse, HttpServer, ResponseError};
use actix_web::middleware::{Logger};

use futures::future::{err, Future};

use serde::{Serialize};
use failure::*;

#[derive(Fail, Serialize)]
pub struct MyError {
   pub name: &'static str,
   pub message: &'static str
}

impl ResponseError for MyError {
    fn error_response(&self) -> HttpResponse {
        dbg!("ResponseError.error_response(&self) being called.");
        // I'm providing a response body which is not being honored.
        // This is what I expect to see as the response.
        HttpResponse::InternalServerError()
            .json(self)
    }
}

impl fmt::Display for MyError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        // This is being chosen as the response body instead of the serialized response
        dbg!("fmt::Display being called for MyError");
        write!(f, "({}, {})", self.name, self.message)
    }
}

impl fmt::Debug for MyError {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        dbg!("fmt::Debug being called for MyError");
        write!(f, "({}, {})", self.name, self.message)
    }
}

fn my_error_impl() -> impl Future<Item = (), Error = MyError> {

    err(MyError {
        name: "test error",
        message: "If this message is being displayed in the response body there is a problem."
    })
}

fn my_error() -> impl Future<Item = HttpResponse, Error = Error> {
    my_error_impl()
        .from_err()
        .and_then(|_| {       
            HttpResponse::Ok()
                .body("Nothing interesting happened.  Try again.")
    })
}

fn main() -> std::io::Result<()> {
    std::env::set_var("RUST_LOG", "actix_web=info");
    env_logger::init();

    HttpServer::new(move || {
        App::new()
        .wrap(Logger::default())
        .service(
            web::resource("/error").route(web::get().to_async(my_error)),
        )
    })
    .bind("127.0.0.1:8088")?
    .run()
}

// calling the /error endpoint results in the fmt::Display() body instead of the 
// json formatting
//

/*

curl http://localhost:8088/error -H "content-type: application/json"
(test error, If this message is being displayed in the response body there is a problem.)

*/

[fafhrd91]

you have to implement .render_response()

[crmckenzie]

Thank you for taking the time to review the issue and respond. It's very much appreciated.

I didn't see that ResponseError had a render_response method because the docs say there is only one method on the trait:

ResponseError has a single function called error_response() that returns HttpResponse

Here is my solution that works:

impl ResponseError for MyError {
    fn error_response(&self) -> HttpResponse {
        dbg!("ResponseError.error_response(&self) being called.");
        HttpResponse::InternalServerError()
            .json(self) // I really wish this was all I had to do.
    }

    // this makes the respose come back as a json blob.
    fn render_response(&self) -> Response {
        let resp = self.error_response();
        let json = serde_json::to_string(self);
        match json {
            Ok(json_text) => {
                let new_body = Body::from(json_text);
                resp.set_body(new_body)
            },
            Err(_) => {
                resp
            }
        }
    }
}

This still seems awkward to me since I already specified in error_response that I wanted the result serialized to json. If I can figure out how to do it, would you accept a PR that makes it unnecessary to implement render_response in this scenario?

[fafhrd91]

Pr is always welcomed. Just your idea before

[fafhrd91]

re-open if needed

[rossmacarthur]

So I encountered this exact thing today, and it is was confusing to figure out. @fafhrd91 What is the reason for having both render_response() and error_response() in the trait? I have implemented it as recommended in MIGRATION.md, but I want to know if there is a benefit to only adding the body in the render_response() method?

impl ResponseError for Error {
    fn error_response(&self) -> HttpResponse {
         HttpResponse::InternalServerError().json(self)
    }

    fn render_response(&self) -> HttpResponse {
        self.error_response()
    }
}