carllerche
Improve error handling
Hi @carllerche,
I have a token-based authentication implemented as Extractor. I'd like to return a meaningful error status code – "401 Unauthorized" if there is a broken token.
In order to do this in a right way, shall I add a new error::KindPriv::Unauthorized, make extract::web() public function, and than update the default catch handler?
Or which approach should I take?
I've introduced error::Error::status_code function in order to simplify default catch handler and use error descriptions from http crate. With this change it looks as there is no need in error:Error::is{bad_request,not_found,internal} functions anymore, so I deleted them.
I've started working on handling errors in my resource handler. For the following example, I've expected to see an 403 - Forbidden, but got 500 - Internal Error.
use tower_web::error::{Error, ErrorKind};
fn read(&self) -> Result<http::Response<&'static str>, Error> {
Err(Error::from(ErrorKind::forbidden()))
}
What would be the best practice to handler HTTP errors in resource handlers?
We definitely can do something like that, but it doesn't look idiomatic: we can't return the errors using ? operator, and we duplicate logic of error::Error.
#[derive(Response)]
#[web(status = "200")]
struct SomeResponse {
uri: String,
}
#[derive(Response)]
#[web(status = "403")]
struct Forbidden {
}
#[derive(Response)]
#[web(either)]
enum Response {
Success(SomeResponse),
Failure(Forbidden),
}
@carllerche Could you please take a look? We probably could merge the current PR that aims handling errors in extractors If you are good with the changes. Then, I may start working on a new one for resource handlers, with little help in understanding what the final concept should look like.
Regarding your resource handler, right now you need to do:
fn read(&self) -> Result<Result<http::Response<&'static str>, Error>, ()> {
Err(Error::from(ErrorKind::forbidden()))
}
I agree that it is a bit awkward. We can try to fix that in the next breaking release. Maybe open up a follow up issue for errors and the handler return sig?
@carllerche I've updated the PR, tests have already completed.
As for your example, it fails to compile.
fn read(&self) -> Result<Result<http::Response<&'static str>, Error>, ()> {
Err(Error::from(ErrorKind::forbidden()))
}
error[E0277]: the trait bound `std::result::Result<http::Response<&'static str>, tower_web::Error>: tower_web::response::Response` is not satisfied
--> src/app/mod.rs:59:1
|
59 | / impl_web! {
60 | |
61 | | impl Object {
62 | | #[get("/")]
... |
125 | |
126 | | }
| |_^ the trait `tower_web::response::Response` is not implemented for `std::result::Result<http::Response<&'static str>, tower_web::Error>`
|
= note: required because of the requirements on the impl of `tower_web::response::Response` for `tower_web::util::tuple::Either1<std::result::Result<http::Response<&'static str>, tower_web::Error>>`
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
Ok, well then we need an impl of Response for Result. I remember earlier discussion about this. It must not have happened.
Even if we implement Response for Result we will need to return Ok(Err(Error::from(ErrorKind::forbidden()))). Am I understand it right?
Yes... which isn't great. We need to figure out a better story around that for the next breaking release.
What the reason to return Result<Response, ()>? I mean we don't use () anyway. Can't we just switch to Result<Response, Error>?
The function must return T: IntoFuture... without specialization, I'm not sure what we can do...
@carllerche I've just added implementation of Response trait for Result<Response, ()>, it really looks as the best option for now.
One more question. It would be useful to return a short description (or reason) for an error along with its status code. Shall we implement it as a description property and public method of error::Error or error::ErrorKind? It isn't clear for me what the difference between these types.
The error vs. error kind split was done prematurely I think. I was mirroring io::Error.
What we probably should do in the next breaking change is just require that the handler error type is E: Into<tower_web::Error>. This would prevent nesting Result.
Adding a description to error would be fine I think.
What we probably should do in the next breaking change is just require that the handler error type is
E: Into<tower_web::Error>. This would prevent nestingResult.
I'd love to see that.
Two questions about descriptions:
- Is it a good idea to rename
ErrorKindtoKindand make it public, then use one constructor forerror::Errorinstead of all those functions forErrorKind?
use error::{Error, Kind};
Error::new(Kind::Unauthorized, &"invalid access token")
- Specifying an error description looks like a good practice, and we won't need to deal with
Option. Can we make it required for all errors?
I wonder if ErrorKind should be removed. I initially just copied io::Error but I think that was the wrong decision.
I've implemented Error type based on "RFC7807: Problem Details for HTTP APIs"
A problem details object can have the following members:
o "type" (string) - A URI reference [RFC3986] that identifies the problem type. This specification encourages that, when dereferenced, it provide human-readable documentation for the problem type (e.g., using HTML [W3C.REC-html5-20141028]). When this member is not present, its value is assumed to be "about:blank".
o "title" (string) - A short, human-readable summary of the problem type. It SHOULD NOT change from occurrence to occurrence of the problem, except for purposes of localization (e.g., using proactive content negotiation; see [RFC7231], Section 3.4).
o "status" (number) - The HTTP status code ([RFC7231], Section 6) generated by the origin server for this occurrence of the problem.
o "detail" (string) - A human-readable explanation specific to this occurrence of the problem.
o "instance" (string) - A URI reference that identifies the specific occurrence of the problem. It may or may not yield further information if dereferenced.
https://tools.ietf.org/html/rfc7807#section-3.1
In short, required parameters are only type and title, both are Strings, so they are perfect candidates to implement as properties of Error type. detail is optional and it's assumed that application-specific data type may be used. It looks interesting to use non-string data type to provide detailed information about error, but I don't see how it can be implemented without making Error a generic type. As for now, detail is just String.
I've changed "content-type" to "application/problem+json" from "text/plain". Even though, it's possible to format object as "text/plain" media type, the specification recommends "application/problem+json" or "application/problem+xml", so I choose to follow that. We can make it customizable in the future by passing a media type for error messages through Catch constructor.
All changes is made in a way to make ErrorKind obsolete. I've marked it and its public methods as deprecated.
I've also added an impl From<error::error::Error> for extract::error::Error, because it looks as a better alternative to web method of extract::error::Error.
@carllerche How can we log a response body in the log middleware for error responses? This way errors in the logs will be self-descriptive, with detailed information about what happened.
@manifest Sorry, could you clarify? Are you asking how to the log middleware can conditionally log the body of an HTTP response?
Just an example of how response body can be extracted in log middleware. It seems like type of the body is opaque and do not implement Display.
Well, it is opaque intentionally. In theory, it could be bound by BufStream but that can only be read once. How should streaming large files be handled with the log middleware?
I propose logging only error messages (responses with 400...599 status codes). Errors don't have large bodies. It will make log messages more informative.
At the moment, to get a reason of HTTP error you need to write something like that:
.map_err(|err| {
error!("{}", err);
err
}))
When everything you need represented within error body.
The middleware could be bound w/ T: BufStream then we have a peakable wrapper that buffers up to N bytes.
I wonder if it possible to write entire log message in JSON from log middleware? I mean, in the log middleware we get already serialized error message (response body), if we serialize entire log message into JSON we will end up with JSON in JSON.