Missing field(s) in JSON payload should return a more detailed error message
Closed this issue · 7 comments
Hi,
I don't have time to implement it myself but if it can help, here a snippet on how I did it:
App::with_state(api_state.clone())
// ...
.default_resource(|r| r.f(api::route_404))
.configure(|app| {
Cors::for_app(app)
// ...
.max_age(3600)
.resource("/account/v1/welcome/register", |r| r.method(http::Method::POST).with_config(services::account::api::v1::register_post, api::json_default_config))
// ...
.register()
})pub fn json_default_config(cfg: &mut ((dev::JsonConfig<State>, ()),)) {
(cfg.0).0.error_handler(|err, _req| { // <- create custom error response
Error::BadRequest{error: err.to_string()}.into()
});
}use failure::Fail;
use actix_web::{
error,
HttpResponse,
};
#[derive(Clone, Debug, Fail)]
pub enum Error {
#[fail(display="Internal error")]
Internal,
#[fail(display="{}", error)]
BadRequest{ error: String },
#[fail(display="Timeout")]
Timeout,
#[fail(display="Route not found")]
RouteNotFound,
}
impl error::ResponseError for Error {
fn error_response(&self) -> HttpResponse {
let res: Response<String> = Response::error(self.clone());
match *self {
Error::Internal => HttpResponse::InternalServerError().json(&res),
Error::BadRequest{..} => HttpResponse::BadRequest().json(&res),
Error::Timeout => HttpResponse::RequestTimeout().json(&res),
Error::RouteNotFound => HttpResponse::NotFound().json(&res),
}
}
}Error handling in this app already works like that though -- You can see this via https://github.com/fairingrey/actix-realworld-example-app/blob/master/src/error.rs
What I'm asking for in this particular issue is that errors on extracting fields from a request body be more specific in which fields are missing or erroneous when Serde works its magic. This is a bit more difficult, but it'll make request errors more clear to the client.
On a bad request, the server already sends back a 400 Bad Request via Actix and Serde defaults, so it brings up the question if its' just fine is, or should we elaborate a bit more?
See this function in particular https://docs.rs/actix-web/0.7.18/actix_web/dev/struct.JsonConfig.html#method.error_handler
Yes, you can see it in action in the second snippet. It will send back an error with a precise description of the missing field / bad data.
Then Just add BadRequest to you error enum.
Ah, okay. If you say it works as it does, I'll try checking it out later. Thanks for your help -- although I've been busy so I probably won't have time this week to finish it.
If I don't get to it in due time I would appreciate it if anyone else would like to work on this issue.
Ugh... my update on this.
I decided to try giving it a shot in a separate branch (https://github.com/fairingrey/actix-realworld-example-app/tree/add_json_config) but the ergonomics of writing the error handler(s) for particular configurations are pretty bad. It's very, very annoying to write.
I presume it may be easier for someone that has a very simple server approach, where all provided endpoint handler functions look the same and extract the same content from HTTP requests, but sadly that's not the case given the complexity of this app.
So something like what you've provided here:
pub fn json_default_config(cfg: &mut ((dev::JsonConfig<State>, ()),)) {
(cfg.0).0.error_handler(|err, _req| { // <- create custom error response
Error::BadRequest{error: err.to_string()}.into()
});
}Aside from the function signature looking absolutely horrendous to mentally parse... This, for one, doesn't work over generics -- and it's arguable if it even should, anyway, because then trying to use the cfg would be meaningless since the arity and order of extractors within the endpoint handler can vary. For example, I don't always extract just the Json, I sometimes also extract the path and params from the request depending on the logic I require.
An attempt to convert it to take bounds over a generic type would look something like:
pub fn json_default_config<T>(cfg: &mut T::Config)
where
T: FromRequest<AppState> + 'static,
{
// cfg.0
// .error_handler(|err, _req| Error::BadRequest(json!({ "error": err.to_string() })).into());
}But, the commented out lines won't work because this function has no idea what (cfg.0) is unless explicitly stated as such within the function signature.
@z0mbie42 Thanks for your suggestion, but I'm closing this issue if only because I think it's an absolute crapshoot to really understand unless you're willing to write closures for each and every function, and even so it'd be this convoluted mess of (cfg.0).0 or (cfg.1).0 (depending on the Json extractor's position within the argument of said async endpoint handler(s).
Overall, I would be averse to trying to over-engineer a solution for this issue, since I imagine the ergonomics of the framework's basic usage and ease-of-learning are much more important than trying to shove in an extra feature that currently has a very awful looking approach to it... If in the future there's a better way of doing this (such as through Actix-web 1.0 or the like), then I'll definitely consider it.
But for right now, I just can't. Doing so would complicate the architecture too much, and make the framework look more intimidating than it really is.
Thanks to all who offered their help, but I will be closing this issue until something better suited comes along.
Totally agree!
Unfortunately better ergonomics seems not planned for 1.0 (I've opened an issue actix/actix-web#720)
That's a real shame. Honestly I do like the actix framework a lot, it's just those kinds of pain points exist that make the framework slightly hard to work with.
Thanks for sharing.