fix scope and resource middleware data access (#2288)

CHANGES.md

@@ -11,11 +11,15 @@
 * Change compression algorithm features flags. [#2250]
 * Deprecate `App::data` and `App::data_factory`. [#2271]
 
+### Fixed
+* Scope and Resource middleware can access data items set on their own layer. [#2288]
+
 [#2177]: https://github.com/actix/actix-web/pull/2177
 [#2250]: https://github.com/actix/actix-web/pull/2250
 [#2271]: https://github.com/actix/actix-web/pull/2271
 [#2262]: https://github.com/actix/actix-web/pull/2262
 [#2263]: https://github.com/actix/actix-web/pull/2263
+[#2288]: https://github.com/actix/actix-web/pull/2288
 
 
 ## 4.0.0-beta.7 - 2021-06-17

src/app.rs

@@ -43,13 +43,14 @@ impl App<AppEntry, Body> {
     /// Create application builder. Application can be configured with a builder-like pattern.
     #[allow(clippy::new_without_default)]
     pub fn new() -> Self {
-        let fref = Rc::new(RefCell::new(None));
+        let factory_ref = Rc::new(RefCell::new(None));
+
         App {
-            endpoint: AppEntry::new(fref.clone()),
+            endpoint: AppEntry::new(factory_ref.clone()),
             data_factories: Vec::new(),
             services: Vec::new(),
             default: None,
-            factory_ref: fref,
+            factory_ref,
             external: Vec::new(),
             extensions: Extensions::new(),
             _phantom: PhantomData,

src/app_service.rs

@@ -1,22 +1,22 @@
-use std::cell::RefCell;
+use std::{cell::RefCell, mem, rc::Rc};
-use std::rc::Rc;
 
 use actix_http::{Extensions, Request};
 use actix_router::{Path, ResourceDef, Router, Url};
-use actix_service::boxed::{self, BoxService, BoxServiceFactory};
+use actix_service::{
-use actix_service::{fn_service, Service, ServiceFactory};
+    boxed::{self, BoxService, BoxServiceFactory},
+    fn_service, Service, ServiceFactory,
+};
 use futures_core::future::LocalBoxFuture;
 use futures_util::future::join_all;
 
-use crate::data::FnDataFactory;
-use crate::error::Error;
-use crate::guard::Guard;
-use crate::request::{HttpRequest, HttpRequestPool};
-use crate::rmap::ResourceMap;
-use crate::service::{AppServiceFactory, ServiceRequest, ServiceResponse};
 use crate::{
     config::{AppConfig, AppService},
-    HttpResponse,
+    data::FnDataFactory,
+    guard::Guard,
+    request::{HttpRequest, HttpRequestPool},
+    rmap::ResourceMap,
+    service::{AppServiceFactory, ServiceRequest, ServiceResponse},
+    Error, HttpResponse,
 };
 
 type Guards = Vec<Box<dyn Guard>>;
@@ -75,7 +75,7 @@ where
         let mut config = AppService::new(config, default.clone());
 
         // register services
-        std::mem::take(&mut *self.services.borrow_mut())
+        mem::take(&mut *self.services.borrow_mut())
             .into_iter()
             .for_each(|mut srv| srv.register(&mut config));
 
@@ -98,7 +98,7 @@ where
         });
 
         // external resources
-        for mut rdef in std::mem::take(&mut *self.external.borrow_mut()) {
+        for mut rdef in mem::take(&mut *self.external.borrow_mut()) {
             rmap.add(&mut rdef, None);
         }
 

src/config.rs

@@ -94,9 +94,9 @@ impl AppService {
         F: IntoServiceFactory<S, ServiceRequest>,
         S: ServiceFactory<
                 ServiceRequest,
-                Config = (),
                 Response = ServiceResponse,
                 Error = Error,
+                Config = (),
                 InitError = (),
             > + 'static,
     {

src/resource.rs

@@ -400,34 +400,28 @@ where
             *rdef.name_mut() = name.clone();
         }
 
-        config.register_service(rdef, guards, self, None)
-    }
-}
-
-impl<T> IntoServiceFactory<T, ServiceRequest> for Resource<T>
-where
-    T: ServiceFactory<
-        ServiceRequest,
-        Config = (),
-        Response = ServiceResponse,
-        Error = Error,
-        InitError = (),
-    >,
-{
-    fn into_factory(self) -> T {
         *self.factory_ref.borrow_mut() = Some(ResourceFactory {
             routes: self.routes,
-            app_data: self.app_data.map(Rc::new),
             default: self.default,
         });
 
-        self.endpoint
+        let resource_data = self.app_data.map(Rc::new);
+
+        // wraps endpoint service (including middleware) call and injects app data for this scope
+        let endpoint = apply_fn_factory(self.endpoint, move |mut req: ServiceRequest, srv| {
+            if let Some(ref data) = resource_data {
+                req.add_data_container(Rc::clone(data));
+            }
+
+            srv.call(req)
+        });
+
+        config.register_service(rdef, guards, endpoint, None)
     }
 }
 
 pub struct ResourceFactory {
     routes: Vec<Route>,
-    app_data: Option<Rc<Extensions>>,
     default: HttpNewService,
 }
 
@@ -446,8 +440,6 @@ impl ServiceFactory<ServiceRequest> for ResourceFactory {
         // construct route service factory futures
         let factory_fut = join_all(self.routes.iter().map(|route| route.new_service(())));
 
-        let app_data = self.app_data.clone();
-
         Box::pin(async move {
             let default = default_fut.await?;
             let routes = factory_fut
@@ -455,18 +447,13 @@ impl ServiceFactory<ServiceRequest> for ResourceFactory {
                 .into_iter()
                 .collect::<Result<Vec<_>, _>>()?;
 
-            Ok(ResourceService {
+            Ok(ResourceService { routes, default })
-                routes,
-                app_data,
-                default,
-            })
         })
     }
 }
 
 pub struct ResourceService {
     routes: Vec<RouteService>,
-    app_data: Option<Rc<Extensions>>,
     default: HttpService,
 }
 
@@ -480,18 +467,10 @@ impl Service<ServiceRequest> for ResourceService {
     fn call(&self, mut req: ServiceRequest) -> Self::Future {
         for route in self.routes.iter() {
             if route.check(&mut req) {
-                if let Some(ref app_data) = self.app_data {
-                    req.add_data_container(app_data.clone());
-                }
-
                 return route.call(req);
             }
         }
 
-        if let Some(ref app_data) = self.app_data {
-            req.add_data_container(app_data.clone());
-        }
-
         self.default.call(req)
     }
 }
@@ -528,11 +507,14 @@ mod tests {
     use actix_service::Service;
     use actix_utils::future::ok;
 
-    use crate::http::{header, HeaderValue, Method, StatusCode};
+    use crate::{
-    use crate::middleware::DefaultHeaders;
+        guard,
-    use crate::service::ServiceRequest;
+        http::{header, HeaderValue, Method, StatusCode},
-    use crate::test::{call_service, init_service, TestRequest};
+        middleware::DefaultHeaders,
-    use crate::{guard, web, App, Error, HttpResponse};
+        service::{ServiceRequest, ServiceResponse},
+        test::{call_service, init_service, TestRequest},
+        web, App, Error, HttpMessage, HttpResponse,
+    };
 
     #[actix_rt::test]
     async fn test_middleware() {
@@ -753,4 +735,39 @@ mod tests {
         let resp = call_service(&srv, req).await;
         assert_eq!(resp.status(), StatusCode::OK);
     }
+
+    #[actix_rt::test]
+    async fn test_middleware_app_data() {
+        let srv = init_service(
+            App::new().service(
+                web::resource("test")
+                    .app_data(1usize)
+                    .wrap_fn(|req, srv| {
+                        assert_eq!(req.app_data::<usize>(), Some(&1usize));
+                        req.extensions_mut().insert(1usize);
+                        srv.call(req)
+                    })
+                    .route(web::get().to(HttpResponse::Ok))
+                    .default_service(|req: ServiceRequest| async move {
+                        let (req, _) = req.into_parts();
+
+                        assert_eq!(req.extensions().get::<usize>(), Some(&1));
+
+                        Ok(ServiceResponse::new(
+                            req,
+                            HttpResponse::BadRequest().finish(),
+                        ))
+                    }),
+            ),
+        )
+        .await;
+
+        let req = TestRequest::get().uri("/test").to_request();
+        let resp = call_service(&srv, req).await;
+        assert_eq!(resp.status(), StatusCode::OK);
+
+        let req = TestRequest::post().uri("/test").to_request();
+        let resp = call_service(&srv, req).await;
+        assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
+    }
 }

src/scope.rs

@@ -427,7 +427,6 @@ where
 
         // complete scope pipeline creation
         *self.factory_ref.borrow_mut() = Some(ScopeFactory {
-            app_data: self.app_data.take().map(Rc::new),
             default,
             services: cfg
                 .into_services()
@@ -449,18 +448,28 @@ where
             Some(self.guards)
         };
 
+        let scope_data = self.app_data.map(Rc::new);
+
+        // wraps endpoint service (including middleware) call and injects app data for this scope
+        let endpoint = apply_fn_factory(self.endpoint, move |mut req: ServiceRequest, srv| {
+            if let Some(ref data) = scope_data {
+                req.add_data_container(Rc::clone(data));
+            }
+
+            srv.call(req)
+        });
+
         // register final service
         config.register_service(
             ResourceDef::root_prefix(&self.rdef),
             guards,
-            self.endpoint,
+            endpoint,
             Some(Rc::new(rmap)),
         )
     }
 }
 
 pub struct ScopeFactory {
-    app_data: Option<Rc<Extensions>>,
     services: Rc<[(ResourceDef, HttpNewService, RefCell<Option<Guards>>)]>,
     default: Rc<HttpNewService>,
 }
@@ -488,8 +497,6 @@ impl ServiceFactory<ServiceRequest> for ScopeFactory {
             }
         }));
 
-        let app_data = self.app_data.clone();
-
         Box::pin(async move {
             let default = default_fut.await?;
 
@@ -505,17 +512,12 @@ impl ServiceFactory<ServiceRequest> for ScopeFactory {
                 })
                 .finish();
 
-            Ok(ScopeService {
+            Ok(ScopeService { router, default })
-                app_data,
-                router,
-                default,
-            })
         })
     }
 }
 
 pub struct ScopeService {
-    app_data: Option<Rc<Extensions>>,
     router: Router<HttpService, Vec<Box<dyn Guard>>>,
     default: HttpService,
 }
@@ -539,10 +541,6 @@ impl Service<ServiceRequest> for ScopeService {
             true
         });
 
-        if let Some(ref app_data) = self.app_data {
-            req.add_data_container(app_data.clone());
-        }
-
         if let Some((srv, _info)) = res {
             srv.call(req)
         } else {
@@ -581,12 +579,15 @@ mod tests {
     use actix_utils::future::ok;
     use bytes::Bytes;
 
-    use crate::dev::Body;
+    use crate::{
-    use crate::http::{header, HeaderValue, Method, StatusCode};
+        dev::Body,
-    use crate::middleware::DefaultHeaders;
+        guard,
-    use crate::service::ServiceRequest;
+        http::{header, HeaderValue, Method, StatusCode},
-    use crate::test::{call_service, init_service, read_body, TestRequest};
+        middleware::DefaultHeaders,
-    use crate::{guard, web, App, HttpRequest, HttpResponse};
+        service::{ServiceRequest, ServiceResponse},
+        test::{call_service, init_service, read_body, TestRequest},
+        web, App, HttpMessage, HttpRequest, HttpResponse,
+    };
 
     #[actix_rt::test]
     async fn test_scope() {
@@ -918,10 +919,7 @@ mod tests {
     async fn test_default_resource_propagation() {
         let srv = init_service(
             App::new()
-                .service(
+                .service(web::scope("/app1").default_service(web::to(HttpResponse::BadRequest)))
-                    web::scope("/app1")
-                        .default_service(web::resource("").to(HttpResponse::BadRequest)),
-                )
                 .service(web::scope("/app2"))
                 .default_service(|r: ServiceRequest| {
                     ok(r.into_response(HttpResponse::MethodNotAllowed()))
@@ -993,6 +991,41 @@ mod tests {
         );
     }
 
+    #[actix_rt::test]
+    async fn test_middleware_app_data() {
+        let srv = init_service(
+            App::new().service(
+                web::scope("app")
+                    .app_data(1usize)
+                    .wrap_fn(|req, srv| {
+                        assert_eq!(req.app_data::<usize>(), Some(&1usize));
+                        req.extensions_mut().insert(1usize);
+                        srv.call(req)
+                    })
+                    .route("/test", web::get().to(HttpResponse::Ok))
+                    .default_service(|req: ServiceRequest| async move {
+                        let (req, _) = req.into_parts();
+
+                        assert_eq!(req.extensions().get::<usize>(), Some(&1));
+
+                        Ok(ServiceResponse::new(
+                            req,
+                            HttpResponse::BadRequest().finish(),
+                        ))
+                    }),
+            ),
+        )
+        .await;
+
+        let req = TestRequest::with_uri("/app/test").to_request();
+        let resp = call_service(&srv, req).await;
+        assert_eq!(resp.status(), StatusCode::OK);
+
+        let req = TestRequest::with_uri("/app/default").to_request();
+        let resp = call_service(&srv, req).await;
+        assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
+    }
+
     // allow deprecated {App, Scope}::data
     #[allow(deprecated)]
     #[actix_rt::test]