Middleware::response is not invoked if error result was returned by another Middleware::start #255

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
 

src/pipeline.rs

@@ -262,7 +262,7 @@ impl<S: 'static, H: PipelineHandler<S>> StartMiddlewares<S, H> {
                             _s: PhantomData,
                         })
                     }
-                    Err(err) => return ProcessResponse::init(err.into()),
+                    Err(err) => return RunMiddlewares::init(info, err.into()),
                 }
             }
         }
@@ -294,13 +294,13 @@ impl<S: 'static, H: PipelineHandler<S>> StartMiddlewares<S, H> {
                                     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())),
             }
         }
     }

src/route.rs

@@ -66,13 +66,12 @@ impl<S: 'static> Route<S> {
     /// # extern crate actix_web;
     /// # use actix_web::*;
     /// # fn main() {
-    /// App::new()
+    /// App::new().resource("/path", |r| {
-    ///    .resource("/path", |r|
+    ///     r.route()
-    ///       r.route()
+    ///         .filter(pred::Get())
-    ///          .filter(pred::Get())
+    ///         .filter(pred::Header("content-type", "text/plain"))
-    ///          .filter(pred::Header("content-type", "text/plain"))
+    ///         .f(|req| HttpResponse::Ok())
-    ///          .f(|req| HttpResponse::Ok())
+    /// })
-    ///       )
     /// #      .finish();
     /// # }
     /// ```
@@ -115,7 +114,7 @@ impl<S: 'static> Route<S> {
     /// # 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<S: 'static> Route<S> {
     ///
     /// fn main() {
     ///     let app = App::new().resource(
-    ///        "/{username}/index.html",                     // <- define path parameters
+    ///         "/{username}/index.html", // <- define path parameters
-    ///        |r| r.method(http::Method::GET).with(index)); // <- use `with` extractor
+    ///         |r| r.method(http::Method::GET).with(index),
+    ///     ); // <- use `with` extractor
     /// }
     /// ```
     ///
@@ -143,7 +143,7 @@ impl<S: 'static> Route<S> {
     /// # 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<S: 'static> Route<S> {
     /// }
     ///
     /// /// extract path info using serde
-    /// fn index(info: (Path<Info>, Query<HashMap<String, String>>, Json<Info>)) -> Result<String> {
+    /// fn index(
+    ///     info: (Path<Info>, Query<HashMap<String, String>>, Json<Info>),
+    /// ) -> Result<String> {
     ///     Ok(format!("Welcome {}!", info.0.username))
     /// }
     ///
     /// fn main() {
     ///     let app = App::new().resource(
-    ///        "/{username}/index.html",                     // <- define path parameters
+    ///         "/{username}/index.html", // <- define path parameters
-    ///        |r| r.method(http::Method::GET).with(index)); // <- use `with` extractor
+    ///         |r| r.method(http::Method::GET).with(index),
+    ///     ); // <- use `with` extractor
     /// }
     /// ```
     pub fn with<T, F, R>(&mut self, handler: F) -> ExtractorConfig<S, T>
@@ -181,7 +184,7 @@ impl<S: 'static> Route<S> {
     /// # 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<S: 'static> Route<S> {
     /// }
     ///
     /// /// extract path info using serde
-    /// fn index(info: Path<Info>) -> Box<Future<Item=&'static str, Error=Error>> {
+    /// fn index(info: Path<Info>) -> Box<Future<Item = &'static str, Error = Error>> {
     ///     unimplemented!()
     /// }
     ///
     /// fn main() {
     ///     let app = App::new().resource(
-    ///        "/{username}/index.html",           // <- define path parameters
+    ///         "/{username}/index.html", // <- define path parameters
-    ///        |r| r.method(http::Method::GET)
+    ///         |r| r.method(http::Method::GET).with_async(index),
-    ///               .with_async(index));         // <- use `with` extractor
+    ///     ); // <- use `with` extractor
     /// }
     /// ```
     pub fn with_async<T, F, R, I, E>(&mut self, handler: F) -> ExtractorConfig<S, T>
@@ -222,7 +225,7 @@ impl<S: 'static> Route<S> {
     /// # 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<S: 'static> Route<S> {
     ///
     /// fn main() {
     ///     let app = App::new().resource(
-    ///        "/{username}/index.html",                      // <- define path parameters
+    ///         "/{username}/index.html", // <- define path parameters
-    ///        |r| r.method(http::Method::GET).with2(index)); // <- use `with` extractor
+    ///         |r| r.method(http::Method::GET).with2(index),
+    ///     ); // <- use `with` extractor
     /// }
     /// ```
     pub fn with2<T1, T2, F, R>(
@@ -424,7 +428,7 @@ impl<S: 'static> StartMiddlewares<S> {
                             _s: PhantomData,
                         })
                     }
-                    Err(err) => return FinishingMiddlewares::init(info, err.into()),
+                    Err(err) => return RunMiddlewares::init(info, err.into()),
                 }
             }
         }
@@ -455,16 +459,13 @@ impl<S: 'static> StartMiddlewares<S> {
                                     continue 'outer;
                                 }
                                 Err(err) => {
-                                    return Some(FinishingMiddlewares::init(
+                                    return Some(RunMiddlewares::init(info, err.into()))
-                                        info,
-                                        err.into(),
-                                    ))
                                 }
                             }
                         }
                     }
                 }
-                Err(err) => return Some(FinishingMiddlewares::init(info, err.into())),
+                Err(err) => return Some(RunMiddlewares::init(info, err.into())),
             }
         }
     }

src/scope.rs

@@ -38,12 +38,12 @@ type NestedInfo<S> = (Resource, Route<S>, Vec<Box<Predicate<S>>>);
 /// use actix_web::{http, App, HttpRequest, HttpResponse};
 ///
 /// fn main() {
-///     let app = App::new()
+///     let app = App::new().scope("/{project_id}/", |scope| {
-///         .scope("/{project_id}/", |scope| {
+///         scope
-///              scope.resource("/path1", |r| r.f(|_| HttpResponse::Ok()))
+///             .resource("/path1", |r| r.f(|_| HttpResponse::Ok()))
-///                .resource("/path2", |r| r.f(|_| HttpResponse::Ok()))
+///             .resource("/path2", |r| r.f(|_| HttpResponse::Ok()))
-///                .resource("/path3", |r| r.f(|_| HttpResponse::MethodNotAllowed()))
+///             .resource("/path3", |r| r.f(|_| HttpResponse::MethodNotAllowed()))
-///         });
+///     });
 /// }
 /// ```
 ///
@@ -89,13 +89,14 @@ impl<S: 'static> Scope<S> {
     /// }
     ///
     /// fn main() {
-    ///     let app = App::new()
+    ///     let app = App::new().scope("/app", |scope| {
-    ///         .scope("/app", |scope| {
+    ///         scope
-    ///             scope.filter(pred::Header("content-type", "text/plain"))
+    ///             .filter(pred::Header("content-type", "text/plain"))
-    ///                .route("/test1", http::Method::GET, index)
+    ///             .route("/test1", http::Method::GET, index)
-    ///                .route("/test2", http::Method::POST,
+    ///             .route("/test2", http::Method::POST, |_: HttpRequest| {
-    ///                    |_: HttpRequest| HttpResponse::MethodNotAllowed())
+    ///                 HttpResponse::MethodNotAllowed()
-    ///         });
+    ///             })
+    ///     });
     /// }
     /// ```
     pub fn filter<T: Predicate<S> + 'static>(mut self, p: T) -> Self {
@@ -116,12 +117,11 @@ impl<S: 'static> Scope<S> {
     /// }
     ///
     /// fn main() {
-    ///     let app = App::new()
+    ///     let app = App::new().scope("/app", |scope| {
-    ///         .scope("/app", |scope| {
+    ///         scope.with_state("/state2", AppState, |scope| {
-    ///             scope.with_state("/state2", AppState, |scope| {
+    ///             scope.resource("/test1", |r| r.f(index))
-    ///                scope.resource("/test1", |r| r.f(index))
+    ///         })
-    ///             })
+    ///     });
-    ///         });
     /// }
     /// ```
     pub fn with_state<F, T: 'static>(mut self, path: &str, state: T, f: F) -> Scope<S>
@@ -162,12 +162,9 @@ impl<S: 'static> Scope<S> {
     /// }
     ///
     /// fn main() {
-    ///     let app = App::with_state(AppState)
+    ///     let app = App::with_state(AppState).scope("/app", |scope| {
-    ///         .scope("/app", |scope| {
+    ///         scope.nested("/v1", |scope| scope.resource("/test1", |r| r.f(index)))
-    ///             scope.nested("/v1", |scope| {
+    ///     });
-    ///                scope.resource("/test1", |r| r.f(index))
-    ///             })
-    ///         });
     /// }
     /// ```
     pub fn nested<F>(mut self, path: &str, f: F) -> Scope<S>
@@ -211,12 +208,13 @@ impl<S: 'static> Scope<S> {
     /// }
     ///
     /// fn main() {
-    ///     let app = App::new()
+    ///     let app = App::new().scope("/app", |scope| {
-    ///         .scope("/app", |scope| {
+    ///         scope.route("/test1", http::Method::GET, index).route(
-    ///             scope.route("/test1", http::Method::GET, index)
+    ///             "/test2",
-    ///                .route("/test2", http::Method::POST,
+    ///             http::Method::POST,
-    ///                    |_: HttpRequest| HttpResponse::MethodNotAllowed())
+    ///             |_: HttpRequest| HttpResponse::MethodNotAllowed(),
-    ///         });
+    ///         )
+    ///     });
     /// }
     /// ```
     pub fn route<T, F, R>(mut self, path: &str, method: Method, f: F) -> Scope<S>
@@ -261,17 +259,16 @@ impl<S: 'static> Scope<S> {
     /// use actix_web::*;
     ///
     /// fn main() {
-    ///     let app = App::new()
+    ///     let app = App::new().scope("/api", |scope| {
-    ///         .scope("/api", |scope| {
+    ///         scope.resource("/users/{userid}/{friend}", |r| {
-    ///             scope.resource("/users/{userid}/{friend}", |r| {
+    ///             r.get().f(|_| HttpResponse::Ok());
-    ///                 r.get().f(|_| HttpResponse::Ok());
+    ///             r.head().f(|_| HttpResponse::MethodNotAllowed());
-    ///                 r.head().f(|_| HttpResponse::MethodNotAllowed());
+    ///             r.route()
-    ///                 r.route()
+    ///                 .filter(pred::Any(pred::Get()).or(pred::Put()))
-    ///                    .filter(pred::Any(pred::Get()).or(pred::Put()))
+    ///                 .filter(pred::Header("Content-Type", "text/plain"))
-    ///                    .filter(pred::Header("Content-Type", "text/plain"))
+    ///                 .f(|_| HttpResponse::Ok())
-    ///                    .f(|_| HttpResponse::Ok())
+    ///         })
-    ///             })
+    ///     });
-    ///         });
     /// }
     /// ```
     pub fn resource<F, R>(mut self, path: &str, f: F) -> Scope<S>
@@ -518,7 +515,7 @@ impl<S: 'static> StartMiddlewares<S> {
                             _s: PhantomData,
                         })
                     }
-                    Err(err) => return Response::init(err.into()),
+                    Err(err) => return RunMiddlewares::init(info, err.into()),
                 }
             }
         }
@@ -554,12 +551,14 @@ impl<S: 'static> StartMiddlewares<S> {
                                     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())),
             }
         }
     }

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<S> middleware::Middleware<S> for MiddlewareWithErr {
+    fn start(&self, _req: &mut HttpRequest<S>) -> Result<middleware::Started, Error> {
+        Err(ErrorInternalServerError("middleware error"))
+    }
+}
+
+struct MiddlewareAsyncWithErr;
+
+impl<S> middleware::Middleware<S> for MiddlewareAsyncWithErr {
+    fn start(&self, _req: &mut HttpRequest<S>) -> Result<middleware::Started, Error> {
+        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);
+}