From f7b7d282bfe89247f1c9a4032d3256c9c6b36d62 Mon Sep 17 00:00:00 2001 From: Nikolay Kim Date: Mon, 4 Jun 2018 13:57:54 -0700 Subject: [PATCH] Middleware::response is not invoked if error result was returned by another Middleware::start #255 --- CHANGES.md | 2 + src/pipeline.rs | 6 +- src/route.rs | 57 +++++------ src/scope.rs | 89 +++++++++-------- tests/test_middleware.rs | 205 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 283 insertions(+), 76 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index ff1b21833..5a2f0cdcf 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -8,6 +8,8 @@ * `HttpRequest::url_for()` for a named route with no variables segments #265 +* `Middleware::response()` is not invoked if error result was returned by another `Middleware::start()` #255 + ## [0.6.10] - 2018-05-24 diff --git a/src/pipeline.rs b/src/pipeline.rs index f5c338e6b..f2ffd32a3 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -262,7 +262,7 @@ impl> StartMiddlewares { _s: PhantomData, }) } - Err(err) => return ProcessResponse::init(err.into()), + Err(err) => return RunMiddlewares::init(info, err.into()), } } } @@ -294,13 +294,13 @@ impl> StartMiddlewares { continue 'outer; } Err(err) => { - return Some(ProcessResponse::init(err.into())) + return Some(RunMiddlewares::init(info, err.into())) } } } } } - Err(err) => return Some(ProcessResponse::init(err.into())), + Err(err) => return Some(RunMiddlewares::init(info, err.into())), } } } diff --git a/src/route.rs b/src/route.rs index ff19db802..8581f3688 100644 --- a/src/route.rs +++ b/src/route.rs @@ -66,13 +66,12 @@ impl Route { /// # extern crate actix_web; /// # use actix_web::*; /// # fn main() { - /// App::new() - /// .resource("/path", |r| - /// r.route() - /// .filter(pred::Get()) - /// .filter(pred::Header("content-type", "text/plain")) - /// .f(|req| HttpResponse::Ok()) - /// ) + /// App::new().resource("/path", |r| { + /// r.route() + /// .filter(pred::Get()) + /// .filter(pred::Header("content-type", "text/plain")) + /// .f(|req| HttpResponse::Ok()) + /// }) /// # .finish(); /// # } /// ``` @@ -115,7 +114,7 @@ impl Route { /// # extern crate actix_web; /// # extern crate futures; /// #[macro_use] extern crate serde_derive; - /// use actix_web::{App, Path, Result, http}; + /// use actix_web::{http, App, Path, Result}; /// /// #[derive(Deserialize)] /// struct Info { @@ -129,8 +128,9 @@ impl Route { /// /// fn main() { /// let app = App::new().resource( - /// "/{username}/index.html", // <- define path parameters - /// |r| r.method(http::Method::GET).with(index)); // <- use `with` extractor + /// "/{username}/index.html", // <- define path parameters + /// |r| r.method(http::Method::GET).with(index), + /// ); // <- use `with` extractor /// } /// ``` /// @@ -143,7 +143,7 @@ impl Route { /// # extern crate futures; /// #[macro_use] extern crate serde_derive; /// # use std::collections::HashMap; - /// use actix_web::{http, App, Query, Path, Result, Json}; + /// use actix_web::{http, App, Json, Path, Query, Result}; /// /// #[derive(Deserialize)] /// struct Info { @@ -151,14 +151,17 @@ impl Route { /// } /// /// /// extract path info using serde - /// fn index(info: (Path, Query>, Json)) -> Result { + /// fn index( + /// info: (Path, Query>, Json), + /// ) -> Result { /// Ok(format!("Welcome {}!", info.0.username)) /// } /// /// fn main() { /// let app = App::new().resource( - /// "/{username}/index.html", // <- define path parameters - /// |r| r.method(http::Method::GET).with(index)); // <- use `with` extractor + /// "/{username}/index.html", // <- define path parameters + /// |r| r.method(http::Method::GET).with(index), + /// ); // <- use `with` extractor /// } /// ``` pub fn with(&mut self, handler: F) -> ExtractorConfig @@ -181,7 +184,7 @@ impl Route { /// # extern crate actix_web; /// # extern crate futures; /// #[macro_use] extern crate serde_derive; - /// use actix_web::{App, Path, Error, http}; + /// use actix_web::{http, App, Error, Path}; /// use futures::Future; /// /// #[derive(Deserialize)] @@ -190,15 +193,15 @@ impl Route { /// } /// /// /// extract path info using serde - /// fn index(info: Path) -> Box> { + /// fn index(info: Path) -> Box> { /// unimplemented!() /// } /// /// fn main() { /// let app = App::new().resource( - /// "/{username}/index.html", // <- define path parameters - /// |r| r.method(http::Method::GET) - /// .with_async(index)); // <- use `with` extractor + /// "/{username}/index.html", // <- define path parameters + /// |r| r.method(http::Method::GET).with_async(index), + /// ); // <- use `with` extractor /// } /// ``` pub fn with_async(&mut self, handler: F) -> ExtractorConfig @@ -222,7 +225,7 @@ impl Route { /// # extern crate actix_web; /// # extern crate futures; /// #[macro_use] extern crate serde_derive; - /// use actix_web::{App, Query, Path, Result, http}; + /// use actix_web::{http, App, Path, Query, Result}; /// /// #[derive(Deserialize)] /// struct PParam { @@ -241,8 +244,9 @@ impl Route { /// /// fn main() { /// let app = App::new().resource( - /// "/{username}/index.html", // <- define path parameters - /// |r| r.method(http::Method::GET).with2(index)); // <- use `with` extractor + /// "/{username}/index.html", // <- define path parameters + /// |r| r.method(http::Method::GET).with2(index), + /// ); // <- use `with` extractor /// } /// ``` pub fn with2( @@ -424,7 +428,7 @@ impl StartMiddlewares { _s: PhantomData, }) } - Err(err) => return FinishingMiddlewares::init(info, err.into()), + Err(err) => return RunMiddlewares::init(info, err.into()), } } } @@ -455,16 +459,13 @@ impl StartMiddlewares { continue 'outer; } Err(err) => { - return Some(FinishingMiddlewares::init( - info, - err.into(), - )) + return Some(RunMiddlewares::init(info, err.into())) } } } } } - Err(err) => return Some(FinishingMiddlewares::init(info, err.into())), + Err(err) => return Some(RunMiddlewares::init(info, err.into())), } } } diff --git a/src/scope.rs b/src/scope.rs index dba490b2f..437d3fdc3 100644 --- a/src/scope.rs +++ b/src/scope.rs @@ -38,12 +38,12 @@ type NestedInfo = (Resource, Route, Vec>>); /// use actix_web::{http, App, HttpRequest, HttpResponse}; /// /// fn main() { -/// let app = App::new() -/// .scope("/{project_id}/", |scope| { -/// scope.resource("/path1", |r| r.f(|_| HttpResponse::Ok())) -/// .resource("/path2", |r| r.f(|_| HttpResponse::Ok())) -/// .resource("/path3", |r| r.f(|_| HttpResponse::MethodNotAllowed())) -/// }); +/// let app = App::new().scope("/{project_id}/", |scope| { +/// scope +/// .resource("/path1", |r| r.f(|_| HttpResponse::Ok())) +/// .resource("/path2", |r| r.f(|_| HttpResponse::Ok())) +/// .resource("/path3", |r| r.f(|_| HttpResponse::MethodNotAllowed())) +/// }); /// } /// ``` /// @@ -89,13 +89,14 @@ impl Scope { /// } /// /// fn main() { - /// let app = App::new() - /// .scope("/app", |scope| { - /// scope.filter(pred::Header("content-type", "text/plain")) - /// .route("/test1", http::Method::GET, index) - /// .route("/test2", http::Method::POST, - /// |_: HttpRequest| HttpResponse::MethodNotAllowed()) - /// }); + /// let app = App::new().scope("/app", |scope| { + /// scope + /// .filter(pred::Header("content-type", "text/plain")) + /// .route("/test1", http::Method::GET, index) + /// .route("/test2", http::Method::POST, |_: HttpRequest| { + /// HttpResponse::MethodNotAllowed() + /// }) + /// }); /// } /// ``` pub fn filter + 'static>(mut self, p: T) -> Self { @@ -116,12 +117,11 @@ impl Scope { /// } /// /// fn main() { - /// let app = App::new() - /// .scope("/app", |scope| { - /// scope.with_state("/state2", AppState, |scope| { - /// scope.resource("/test1", |r| r.f(index)) - /// }) - /// }); + /// let app = App::new().scope("/app", |scope| { + /// scope.with_state("/state2", AppState, |scope| { + /// scope.resource("/test1", |r| r.f(index)) + /// }) + /// }); /// } /// ``` pub fn with_state(mut self, path: &str, state: T, f: F) -> Scope @@ -162,12 +162,9 @@ impl Scope { /// } /// /// fn main() { - /// let app = App::with_state(AppState) - /// .scope("/app", |scope| { - /// scope.nested("/v1", |scope| { - /// scope.resource("/test1", |r| r.f(index)) - /// }) - /// }); + /// let app = App::with_state(AppState).scope("/app", |scope| { + /// scope.nested("/v1", |scope| scope.resource("/test1", |r| r.f(index))) + /// }); /// } /// ``` pub fn nested(mut self, path: &str, f: F) -> Scope @@ -211,12 +208,13 @@ impl Scope { /// } /// /// fn main() { - /// let app = App::new() - /// .scope("/app", |scope| { - /// scope.route("/test1", http::Method::GET, index) - /// .route("/test2", http::Method::POST, - /// |_: HttpRequest| HttpResponse::MethodNotAllowed()) - /// }); + /// let app = App::new().scope("/app", |scope| { + /// scope.route("/test1", http::Method::GET, index).route( + /// "/test2", + /// http::Method::POST, + /// |_: HttpRequest| HttpResponse::MethodNotAllowed(), + /// ) + /// }); /// } /// ``` pub fn route(mut self, path: &str, method: Method, f: F) -> Scope @@ -261,17 +259,16 @@ impl Scope { /// use actix_web::*; /// /// fn main() { - /// let app = App::new() - /// .scope("/api", |scope| { - /// scope.resource("/users/{userid}/{friend}", |r| { - /// r.get().f(|_| HttpResponse::Ok()); - /// r.head().f(|_| HttpResponse::MethodNotAllowed()); - /// r.route() - /// .filter(pred::Any(pred::Get()).or(pred::Put())) - /// .filter(pred::Header("Content-Type", "text/plain")) - /// .f(|_| HttpResponse::Ok()) - /// }) - /// }); + /// let app = App::new().scope("/api", |scope| { + /// scope.resource("/users/{userid}/{friend}", |r| { + /// r.get().f(|_| HttpResponse::Ok()); + /// r.head().f(|_| HttpResponse::MethodNotAllowed()); + /// r.route() + /// .filter(pred::Any(pred::Get()).or(pred::Put())) + /// .filter(pred::Header("Content-Type", "text/plain")) + /// .f(|_| HttpResponse::Ok()) + /// }) + /// }); /// } /// ``` pub fn resource(mut self, path: &str, f: F) -> Scope @@ -518,7 +515,7 @@ impl StartMiddlewares { _s: PhantomData, }) } - Err(err) => return Response::init(err.into()), + Err(err) => return RunMiddlewares::init(info, err.into()), } } } @@ -554,12 +551,14 @@ impl StartMiddlewares { self.fut = Some(fut); continue 'outer; } - Err(err) => return Some(Response::init(err.into())), + Err(err) => { + return Some(RunMiddlewares::init(info, err.into())) + } } } } } - Err(err) => return Some(Response::init(err.into())), + Err(err) => return Some(RunMiddlewares::init(info, err.into())), } } } diff --git a/tests/test_middleware.rs b/tests/test_middleware.rs index 8435e7464..ab00f2f0b 100644 --- a/tests/test_middleware.rs +++ b/tests/test_middleware.rs @@ -9,6 +9,7 @@ use std::thread; use std::time::Duration; use actix::*; +use actix_web::error::{Error, ErrorInternalServerError}; use actix_web::*; use futures::{future, Future}; use tokio_core::reactor::Timeout; @@ -792,3 +793,207 @@ fn test_async_sync_resource_middleware_multiple() { thread::sleep(Duration::from_millis(40)); assert_eq!(num3.load(Ordering::Relaxed), 2); } + +struct MiddlewareWithErr; + +impl middleware::Middleware for MiddlewareWithErr { + fn start(&self, _req: &mut HttpRequest) -> Result { + Err(ErrorInternalServerError("middleware error")) + } +} + +struct MiddlewareAsyncWithErr; + +impl middleware::Middleware for MiddlewareAsyncWithErr { + fn start(&self, _req: &mut HttpRequest) -> Result { + Ok(middleware::Started::Future(Box::new(future::err( + ErrorInternalServerError("middleware error"), + )))) + } +} + +#[test] +fn test_middleware_chain_with_error() { + let num1 = Arc::new(AtomicUsize::new(0)); + let num2 = Arc::new(AtomicUsize::new(0)); + let num3 = Arc::new(AtomicUsize::new(0)); + + let act_num1 = Arc::clone(&num1); + let act_num2 = Arc::clone(&num2); + let act_num3 = Arc::clone(&num3); + + let mut srv = test::TestServer::with_factory(move || { + let mw1 = MiddlewareTest { + start: Arc::clone(&act_num1), + response: Arc::clone(&act_num2), + finish: Arc::clone(&act_num3), + }; + App::new() + .middleware(mw1) + .middleware(MiddlewareWithErr) + .resource("/test", |r| r.h(|_| HttpResponse::Ok())) + }); + + let request = srv.get().uri(srv.url("/test")).finish().unwrap(); + let response = srv.execute(request.send()).unwrap(); + + assert_eq!(num1.load(Ordering::Relaxed), 1); + assert_eq!(num2.load(Ordering::Relaxed), 1); + assert_eq!(num3.load(Ordering::Relaxed), 1); +} + +#[test] +fn test_middleware_async_chain_with_error() { + let num1 = Arc::new(AtomicUsize::new(0)); + let num2 = Arc::new(AtomicUsize::new(0)); + let num3 = Arc::new(AtomicUsize::new(0)); + + let act_num1 = Arc::clone(&num1); + let act_num2 = Arc::clone(&num2); + let act_num3 = Arc::clone(&num3); + + let mut srv = test::TestServer::with_factory(move || { + let mw1 = MiddlewareTest { + start: Arc::clone(&act_num1), + response: Arc::clone(&act_num2), + finish: Arc::clone(&act_num3), + }; + App::new() + .middleware(mw1) + .middleware(MiddlewareAsyncWithErr) + .resource("/test", |r| r.h(|_| HttpResponse::Ok())) + }); + + let request = srv.get().uri(srv.url("/test")).finish().unwrap(); + let response = srv.execute(request.send()).unwrap(); + + assert_eq!(num1.load(Ordering::Relaxed), 1); + assert_eq!(num2.load(Ordering::Relaxed), 1); + assert_eq!(num3.load(Ordering::Relaxed), 1); +} + +#[test] +fn test_scope_middleware_chain_with_error() { + let num1 = Arc::new(AtomicUsize::new(0)); + let num2 = Arc::new(AtomicUsize::new(0)); + let num3 = Arc::new(AtomicUsize::new(0)); + + let act_num1 = Arc::clone(&num1); + let act_num2 = Arc::clone(&num2); + let act_num3 = Arc::clone(&num3); + + let mut srv = test::TestServer::with_factory(move || { + let mw1 = MiddlewareTest { + start: Arc::clone(&act_num1), + response: Arc::clone(&act_num2), + finish: Arc::clone(&act_num3), + }; + App::new().scope("/scope", |scope| { + scope + .middleware(mw1) + .middleware(MiddlewareWithErr) + .resource("/test", |r| r.h(|_| HttpResponse::Ok())) + }) + }); + + let request = srv.get().uri(srv.url("/scope/test")).finish().unwrap(); + let response = srv.execute(request.send()).unwrap(); + + assert_eq!(num1.load(Ordering::Relaxed), 1); + assert_eq!(num2.load(Ordering::Relaxed), 1); + assert_eq!(num3.load(Ordering::Relaxed), 1); +} + +#[test] +fn test_scope_middleware_async_chain_with_error() { + let num1 = Arc::new(AtomicUsize::new(0)); + let num2 = Arc::new(AtomicUsize::new(0)); + let num3 = Arc::new(AtomicUsize::new(0)); + + let act_num1 = Arc::clone(&num1); + let act_num2 = Arc::clone(&num2); + let act_num3 = Arc::clone(&num3); + + let mut srv = test::TestServer::with_factory(move || { + let mw1 = MiddlewareTest { + start: Arc::clone(&act_num1), + response: Arc::clone(&act_num2), + finish: Arc::clone(&act_num3), + }; + App::new().scope("/scope", |scope| { + scope + .middleware(mw1) + .middleware(MiddlewareAsyncWithErr) + .resource("/test", |r| r.h(|_| HttpResponse::Ok())) + }) + }); + + let request = srv.get().uri(srv.url("/scope/test")).finish().unwrap(); + let response = srv.execute(request.send()).unwrap(); + + assert_eq!(num1.load(Ordering::Relaxed), 1); + assert_eq!(num2.load(Ordering::Relaxed), 1); + assert_eq!(num3.load(Ordering::Relaxed), 1); +} + +#[test] +fn test_resource_middleware_chain_with_error() { + let num1 = Arc::new(AtomicUsize::new(0)); + let num2 = Arc::new(AtomicUsize::new(0)); + let num3 = Arc::new(AtomicUsize::new(0)); + + let act_num1 = Arc::clone(&num1); + let act_num2 = Arc::clone(&num2); + let act_num3 = Arc::clone(&num3); + + let mut srv = test::TestServer::with_factory(move || { + let mw1 = MiddlewareTest { + start: Arc::clone(&act_num1), + response: Arc::clone(&act_num2), + finish: Arc::clone(&act_num3), + }; + App::new().resource("/test", move |r| { + r.middleware(mw1); + r.middleware(MiddlewareWithErr); + r.h(|_| HttpResponse::Ok()); + }) + }); + + let request = srv.get().uri(srv.url("/test")).finish().unwrap(); + let response = srv.execute(request.send()).unwrap(); + + assert_eq!(num1.load(Ordering::Relaxed), 1); + assert_eq!(num2.load(Ordering::Relaxed), 1); + assert_eq!(num3.load(Ordering::Relaxed), 1); +} + +#[test] +fn test_resource_middleware_async_chain_with_error() { + let num1 = Arc::new(AtomicUsize::new(0)); + let num2 = Arc::new(AtomicUsize::new(0)); + let num3 = Arc::new(AtomicUsize::new(0)); + + let act_num1 = Arc::clone(&num1); + let act_num2 = Arc::clone(&num2); + let act_num3 = Arc::clone(&num3); + + let mut srv = test::TestServer::with_factory(move || { + let mw1 = MiddlewareTest { + start: Arc::clone(&act_num1), + response: Arc::clone(&act_num2), + finish: Arc::clone(&act_num3), + }; + App::new().resource("/test", move |r| { + r.middleware(mw1); + r.middleware(MiddlewareAsyncWithErr); + r.h(|_| HttpResponse::Ok()); + }) + }); + + let request = srv.get().uri(srv.url("/test")).finish().unwrap(); + let response = srv.execute(request.send()).unwrap(); + + assert_eq!(num1.load(Ordering::Relaxed), 1); + assert_eq!(num2.load(Ordering::Relaxed), 1); + assert_eq!(num3.load(Ordering::Relaxed), 1); +}