From f85925a652f87ff42e25b7c88c94bef13ab578fc Mon Sep 17 00:00:00 2001 From: Nikolay Kim Date: Sun, 22 Oct 2017 09:13:29 -0700 Subject: [PATCH] refactor error handling --- Cargo.toml | 6 ----- examples/multipart/src/main.rs | 8 +++---- examples/websocket-chat/src/main.rs | 27 +++++++++------------- examples/websocket.rs | 16 ++++--------- src/application.rs | 2 +- src/error.rs | 12 ++++------ src/httpresponse.rs | 36 +++++++++++++++++++++++++---- src/lib.rs | 6 +---- src/resource.rs | 33 +++++--------------------- src/route.rs | 10 ++++++-- src/ws.rs | 3 ++- 11 files changed, 73 insertions(+), 86 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 009f38a5..238a2d1a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,12 +20,6 @@ codecov = { repository = "fafhrd91/actix-web", branch = "master", service = "git name = "actix_web" path = "src/lib.rs" -[features] -default = [] - -# Enable nightly features -nightly = [] - [dependencies] log = "0.3" time = "0.1" diff --git a/examples/multipart/src/main.rs b/examples/multipart/src/main.rs index 5b210b2b..1e20aca2 100644 --- a/examples/multipart/src/main.rs +++ b/examples/multipart/src/main.rs @@ -15,13 +15,11 @@ impl Actor for MyRoute { impl Route for MyRoute { type State = (); - fn request(req: HttpRequest, payload: Payload, ctx: &mut HttpContext) -> Reply { + fn request(req: &mut HttpRequest, payload: Payload, + ctx: &mut HttpContext) -> RouteResult { println!("{:?}", req); - let multipart = match req.multipart(payload) { - Ok(mp) => mp, - Err(e) => return e.into(), - }; + let multipart = req.multipart(payload)?; // get Multipart stream WrapStream::::actstream(multipart) diff --git a/examples/websocket-chat/src/main.rs b/examples/websocket-chat/src/main.rs index 252ef6d3..50ccfc77 100644 --- a/examples/websocket-chat/src/main.rs +++ b/examples/websocket-chat/src/main.rs @@ -48,24 +48,19 @@ impl Actor for WsChatSession { impl Route for WsChatSession { type State = WsChatSessionState; - fn request(req: HttpRequest, payload: Payload, ctx: &mut HttpContext) -> Reply + fn request(req: &mut HttpRequest, + payload: Payload, ctx: &mut HttpContext) -> RouteResult { // websocket handshakre, it may fail if request is not websocket request - match ws::handshake(&req) { - Ok(resp) => { - ctx.start(resp); - ctx.add_stream(ws::WsStream::new(payload)); - Reply::async( - WsChatSession { - id: 0, - hb: Instant::now(), - room: "Main".to_owned(), - name: None}) - } - Err(err) => { - Reply::reply(err) - } - } + let resp = ws::handshake(&req)?; + ctx.start(resp); + ctx.add_stream(ws::WsStream::new(payload)); + Reply::async( + WsChatSession { + id: 0, + hb: Instant::now(), + room: "Main".to_owned(), + name: None}) } } diff --git a/examples/websocket.rs b/examples/websocket.rs index 2c63687c..1c12cc22 100644 --- a/examples/websocket.rs +++ b/examples/websocket.rs @@ -17,18 +17,12 @@ impl Route for MyWebSocket { type State = (); fn request(req: &mut HttpRequest, - payload: Payload, ctx: &mut HttpContext) -> Reply + payload: Payload, ctx: &mut HttpContext) -> RouteResult { - match ws::handshake(&req) { - Ok(resp) => { - ctx.start(resp); - ctx.add_stream(ws::WsStream::new(payload)); - Reply::async(MyWebSocket) - } - Err(err) => { - Reply::reply(err) - } - } + let resp = ws::handshake(&req)?; + ctx.start(resp); + ctx.add_stream(ws::WsStream::new(payload)); + Reply::async(MyWebSocket) } } diff --git a/src/application.rs b/src/application.rs index de77317d..b18383e6 100644 --- a/src/application.rs +++ b/src/application.rs @@ -170,7 +170,7 @@ impl ApplicationBuilder where S: 'static { /// /// fn request(req: &mut HttpRequest, /// payload: Payload, - /// ctx: &mut HttpContext) -> Reply { + /// ctx: &mut HttpContext) -> RouteResult { /// Reply::reply(httpcodes::HTTPOk) /// } /// } diff --git a/src/error.rs b/src/error.rs index 34497a8a..d8996776 100644 --- a/src/error.rs +++ b/src/error.rs @@ -110,8 +110,7 @@ impl From for ParseError { /// Return `BadRequest` for `ParseError` impl From for HttpResponse { fn from(err: ParseError) -> Self { - HttpResponse::new(StatusCode::BAD_REQUEST, - Body::Binary(err.description().into())) + HttpResponse::from_error(StatusCode::BAD_REQUEST, err) } } @@ -119,24 +118,21 @@ impl From for HttpResponse { /// Response generation can return `HttpError`, so it is internal error impl From for HttpResponse { fn from(err: HttpError) -> Self { - HttpResponse::new(StatusCode::INTERNAL_SERVER_ERROR, - Body::Binary(err.description().into())) + HttpResponse::from_error(StatusCode::INTERNAL_SERVER_ERROR, err) } } /// Return `BadRequest` for `cookie::ParseError` impl From for HttpResponse { fn from(err: cookie::ParseError) -> Self { - HttpResponse::new(StatusCode::BAD_REQUEST, - Body::Binary(err.description().into())) + HttpResponse::from_error(StatusCode::BAD_REQUEST, err) } } /// Return `BadRequest` for `MultipartError` impl From for HttpResponse { fn from(err: MultipartError) -> Self { - HttpResponse::new(StatusCode::BAD_REQUEST, - Body::Binary(err.description().into())) + HttpResponse::from_error(StatusCode::BAD_REQUEST, err) } } diff --git a/src/httpresponse.rs b/src/httpresponse.rs index bc735827..11c48033 100644 --- a/src/httpresponse.rs +++ b/src/httpresponse.rs @@ -1,10 +1,11 @@ //! Pieces pertaining to the HTTP message protocol. use std::{io, mem, str}; +use std::error::Error as Error; use std::convert::Into; use cookie::CookieJar; use bytes::Bytes; -use http::{StatusCode, Version, HeaderMap, HttpTryFrom, Error}; +use http::{StatusCode, Version, HeaderMap, HttpTryFrom, Error as HttpError}; use http::header::{self, HeaderName, HeaderValue}; use Cookie; @@ -57,6 +58,7 @@ pub struct HttpResponse { body: Body, chunked: bool, connection_type: Option, + error: Option>, } impl HttpResponse { @@ -81,9 +83,34 @@ impl HttpResponse { chunked: false, // compression: None, connection_type: None, + error: None, } } + /// Constructs a response from error + #[inline] + pub fn from_error(status: StatusCode, error: E) -> HttpResponse { + let body = Body::Binary(error.description().into()); + + HttpResponse { + version: None, + headers: Default::default(), + status: status, + reason: None, + body: body, + chunked: false, + // compression: None, + connection_type: None, + error: Some(Box::new(error)), + } + } + + /// The `error` which is responsible for this response + #[inline] + pub fn error(&self) -> Option<&Box> { + self.error.as_ref() + } + /// Get the HTTP version of this response. #[inline] pub fn version(&self) -> Option { @@ -226,7 +253,7 @@ impl Parts { #[derive(Debug)] pub struct HttpResponseBuilder { parts: Option, - err: Option, + err: Option, } impl HttpResponseBuilder { @@ -348,7 +375,7 @@ impl HttpResponseBuilder { } /// Set a body - pub fn body>(&mut self, body: B) -> Result { + pub fn body>(&mut self, body: B) -> Result { let mut parts = self.parts.take().expect("cannot reuse response builder"); if let Some(e) = self.err.take() { return Err(e) @@ -366,11 +393,12 @@ impl HttpResponseBuilder { body: body.into(), chunked: parts.chunked, connection_type: parts.connection_type, + error: None, }) } } -fn parts<'a>(parts: &'a mut Option, err: &Option) -> Option<&'a mut Parts> +fn parts<'a>(parts: &'a mut Option, err: &Option) -> Option<&'a mut Parts> { if err.is_some() { return None diff --git a/src/lib.rs b/src/lib.rs index f1c3d1e8..b75c200b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,9 +1,5 @@ //! Http framework for [Actix](https://github.com/fafhrd91/actix) -#![cfg_attr(feature="nightly", feature( - try_trait, // std::ops::Try #42327 -))] - #[macro_use] extern crate log; extern crate time; @@ -53,7 +49,7 @@ pub use application::{Application, ApplicationBuilder, Middleware}; pub use httprequest::{HttpRequest, UrlEncoded}; pub use httpresponse::{Body, HttpResponse, HttpResponseBuilder}; pub use payload::{Payload, PayloadItem, PayloadError}; -pub use route::{Route, RouteFactory, RouteHandler}; +pub use route::{Route, RouteFactory, RouteHandler, RouteResult}; pub use resource::{Reply, Resource}; pub use recognizer::{Params, RouteRecognizer}; pub use logger::Logger; diff --git a/src/resource.rs b/src/resource.rs index 0ad10220..a69e27ca 100644 --- a/src/resource.rs +++ b/src/resource.rs @@ -8,7 +8,7 @@ use http::Method; use futures::Stream; use task::Task; -use route::{Route, RouteHandler, Frame, FnHandler, StreamHandler}; +use route::{Route, RouteHandler, RouteResult, Frame, FnHandler, StreamHandler}; use payload::Payload; use context::HttpContext; use httprequest::HttpRequest; @@ -141,13 +141,13 @@ pub struct Reply (ReplyItem); impl Reply where A: Actor + Route { /// Create async response - pub fn async(act: A) -> Self { - Reply(ReplyItem::Actor(act)) + pub fn async(act: A) -> RouteResult { + Ok(Reply(ReplyItem::Actor(act))) } /// Send response - pub fn reply>(response: R) -> Self { - Reply(ReplyItem::Message(response.into())) + pub fn reply>(response: R) -> RouteResult { + Ok(Reply(ReplyItem::Message(response.into()))) } pub fn into(self, mut ctx: HttpContext) -> Task where A: Actor> @@ -168,27 +168,6 @@ impl From for Reply where T: Into, A: Actor + Route { fn from(item: T) -> Self { - Reply::reply(item) - } -} - -#[cfg(feature="nightly")] -use std::ops::Try; - -#[cfg(feature="nightly")] -impl Try for Reply where A: Actor + Route { - type Ok = HttpResponse; - type Error = HttpResponse; - - fn into_result(self) -> Result { - panic!("Reply -> Result conversion is not supported") - } - - fn from_error(v: Self::Error) -> Self { - Reply::reply(v) - } - - fn from_ok(v: Self::Ok) -> Self { - Reply::reply(v) + Reply(ReplyItem::Message(item.into())) } } diff --git a/src/route.rs b/src/route.rs index 48946fe8..02163dbb 100644 --- a/src/route.rs +++ b/src/route.rs @@ -33,6 +33,9 @@ pub trait RouteHandler: 'static { fn set_prefix(&mut self, prefix: String) {} } +/// Request handling result. +pub type RouteResult = Result, HttpResponse>; + /// Actors with ability to handle http requests. #[allow(unused_variables)] pub trait Route: Actor { @@ -74,7 +77,7 @@ pub trait Route: Actor { /// In that case `HttpContext::start` and `HttpContext::write` has to be used /// for writing response. fn request(req: &mut HttpRequest, - payload: Payload, ctx: &mut Self::Context) -> Reply; + payload: Payload, ctx: &mut Self::Context) -> RouteResult; /// This method creates `RouteFactory` for this actor. fn factory() -> RouteFactory { @@ -99,7 +102,10 @@ impl RouteHandler for RouteFactory return Task::reply(resp) } } - A::request(req, payload, &mut ctx).into(ctx) + match A::request(req, payload, &mut ctx) { + Ok(reply) => reply.into(ctx), + Err(err) => Task::reply(err), + } } } diff --git a/src/ws.rs b/src/ws.rs index 7dbae44f..64b36f6e 100644 --- a/src/ws.rs +++ b/src/ws.rs @@ -22,7 +22,8 @@ //! impl Route for WsRoute { //! type State = (); //! -//! fn request(req: &mut HttpRequest, payload: Payload, ctx: &mut HttpContext) -> Reply +//! fn request(req: &mut HttpRequest, +//! payload: Payload, ctx: &mut HttpContext) -> RouteResult //! { //! // WebSocket handshake //! match ws::handshake(&req) {