clarify resource/scope app data overriding (#1476)

* relocate FnDataFactory

* clarify app data overriding in Scope and Resource

Co-authored-by: Yuki Okushi <huyuumi.dev@gmail.com>

actix-http/src/extensions.rs

@@ -67,108 +67,113 @@ impl fmt::Debug for Extensions {
     }
 }
 
-#[test]
-fn test_remove() {
-    let mut map = Extensions::new();
+#[cfg(test)]
+mod tests {
+    use super::*;
+    
+    #[test]
+    fn test_remove() {
+        let mut map = Extensions::new();
 
-    map.insert::<i8>(123);
-    assert!(map.get::<i8>().is_some());
+        map.insert::<i8>(123);
+        assert!(map.get::<i8>().is_some());
 
-    map.remove::<i8>();
-    assert!(map.get::<i8>().is_none());
-}
-
-#[test]
-fn test_clear() {
-    let mut map = Extensions::new();
-
-    map.insert::<i8>(8);
-    map.insert::<i16>(16);
-    map.insert::<i32>(32);
-
-    assert!(map.contains::<i8>());
-    assert!(map.contains::<i16>());
-    assert!(map.contains::<i32>());
-
-    map.clear();
-
-    assert!(!map.contains::<i8>());
-    assert!(!map.contains::<i16>());
-    assert!(!map.contains::<i32>());
-
-    map.insert::<i8>(10);
-    assert_eq!(*map.get::<i8>().unwrap(), 10);
-}
-
-#[test]
-fn test_integers() {
-    let mut map = Extensions::new();
-
-    map.insert::<i8>(8);
-    map.insert::<i16>(16);
-    map.insert::<i32>(32);
-    map.insert::<i64>(64);
-    map.insert::<i128>(128);
-    map.insert::<u8>(8);
-    map.insert::<u16>(16);
-    map.insert::<u32>(32);
-    map.insert::<u64>(64);
-    map.insert::<u128>(128);
-    assert!(map.get::<i8>().is_some());
-    assert!(map.get::<i16>().is_some());
-    assert!(map.get::<i32>().is_some());
-    assert!(map.get::<i64>().is_some());
-    assert!(map.get::<i128>().is_some());
-    assert!(map.get::<u8>().is_some());
-    assert!(map.get::<u16>().is_some());
-    assert!(map.get::<u32>().is_some());
-    assert!(map.get::<u64>().is_some());
-    assert!(map.get::<u128>().is_some());
-}
-
-#[test]
-fn test_composition() {
-    struct Magi<T>(pub T);
-
-    struct Madoka {
-        pub god: bool,
+        map.remove::<i8>();
+        assert!(map.get::<i8>().is_none());
     }
 
-    struct Homura {
-        pub attempts: usize,
+    #[test]
+    fn test_clear() {
+        let mut map = Extensions::new();
+
+        map.insert::<i8>(8);
+        map.insert::<i16>(16);
+        map.insert::<i32>(32);
+
+        assert!(map.contains::<i8>());
+        assert!(map.contains::<i16>());
+        assert!(map.contains::<i32>());
+
+        map.clear();
+
+        assert!(!map.contains::<i8>());
+        assert!(!map.contains::<i16>());
+        assert!(!map.contains::<i32>());
+
+        map.insert::<i8>(10);
+        assert_eq!(*map.get::<i8>().unwrap(), 10);
     }
 
-    struct Mami {
-        pub guns: usize,
+    #[test]
+    fn test_integers() {
+        let mut map = Extensions::new();
+
+        map.insert::<i8>(8);
+        map.insert::<i16>(16);
+        map.insert::<i32>(32);
+        map.insert::<i64>(64);
+        map.insert::<i128>(128);
+        map.insert::<u8>(8);
+        map.insert::<u16>(16);
+        map.insert::<u32>(32);
+        map.insert::<u64>(64);
+        map.insert::<u128>(128);
+        assert!(map.get::<i8>().is_some());
+        assert!(map.get::<i16>().is_some());
+        assert!(map.get::<i32>().is_some());
+        assert!(map.get::<i64>().is_some());
+        assert!(map.get::<i128>().is_some());
+        assert!(map.get::<u8>().is_some());
+        assert!(map.get::<u16>().is_some());
+        assert!(map.get::<u32>().is_some());
+        assert!(map.get::<u64>().is_some());
+        assert!(map.get::<u128>().is_some());
     }
 
-    let mut map = Extensions::new();
+    #[test]
+    fn test_composition() {
+        struct Magi<T>(pub T);
 
-    map.insert(Magi(Madoka { god: false }));
-    map.insert(Magi(Homura { attempts: 0 }));
-    map.insert(Magi(Mami { guns: 999 }));
+        struct Madoka {
+            pub god: bool,
+        }
 
-    assert!(!map.get::<Magi<Madoka>>().unwrap().0.god);
-    assert_eq!(0, map.get::<Magi<Homura>>().unwrap().0.attempts);
-    assert_eq!(999, map.get::<Magi<Mami>>().unwrap().0.guns);
-}
-
-#[test]
-fn test_extensions() {
-    #[derive(Debug, PartialEq)]
-    struct MyType(i32);
-
-    let mut extensions = Extensions::new();
-
-    extensions.insert(5i32);
-    extensions.insert(MyType(10));
-
-    assert_eq!(extensions.get(), Some(&5i32));
-    assert_eq!(extensions.get_mut(), Some(&mut 5i32));
-
-    assert_eq!(extensions.remove::<i32>(), Some(5i32));
-    assert!(extensions.get::<i32>().is_none());
-
-    assert_eq!(extensions.get::<bool>(), None);
-    assert_eq!(extensions.get(), Some(&MyType(10)));
+        struct Homura {
+            pub attempts: usize,
+        }
+
+        struct Mami {
+            pub guns: usize,
+        }
+
+        let mut map = Extensions::new();
+
+        map.insert(Magi(Madoka { god: false }));
+        map.insert(Magi(Homura { attempts: 0 }));
+        map.insert(Magi(Mami { guns: 999 }));
+
+        assert!(!map.get::<Magi<Madoka>>().unwrap().0.god);
+        assert_eq!(0, map.get::<Magi<Homura>>().unwrap().0.attempts);
+        assert_eq!(999, map.get::<Magi<Mami>>().unwrap().0.guns);
+    }
+
+    #[test]
+    fn test_extensions() {
+        #[derive(Debug, PartialEq)]
+        struct MyType(i32);
+
+        let mut extensions = Extensions::new();
+
+        extensions.insert(5i32);
+        extensions.insert(MyType(10));
+
+        assert_eq!(extensions.get(), Some(&5i32));
+        assert_eq!(extensions.get_mut(), Some(&mut 5i32));
+
+        assert_eq!(extensions.remove::<i32>(), Some(5i32));
+        assert!(extensions.get::<i32>().is_none());
+
+        assert_eq!(extensions.get::<bool>(), None);
+        assert_eq!(extensions.get(), Some(&MyType(10)));
+    }
 }

src/app.rs

@@ -10,11 +10,11 @@ use actix_service::boxed::{self, BoxServiceFactory};
 use actix_service::{
     apply, apply_fn_factory, IntoServiceFactory, ServiceFactory, Transform,
 };
-use futures::future::{FutureExt, LocalBoxFuture};
+use futures::future::FutureExt;
 
 use crate::app_service::{AppEntry, AppInit, AppRoutingFactory};
 use crate::config::ServiceConfig;
-use crate::data::{Data, DataFactory};
+use crate::data::{Data, DataFactory, FnDataFactory};
 use crate::dev::ResourceDef;
 use crate::error::Error;
 use crate::resource::Resource;
@@ -25,8 +25,6 @@ use crate::service::{
 };
 
 type HttpNewService = BoxServiceFactory<(), ServiceRequest, ServiceResponse, Error, ()>;
-type FnDataFactory =
-    Box<dyn Fn() -> LocalBoxFuture<'static, Result<Box<dyn DataFactory>, ()>>>;
 
 /// Application builder - structure that follows the builder pattern
 /// for building application instances.

src/app_service.rs

@@ -12,7 +12,7 @@ use actix_service::{fn_service, Service, ServiceFactory};
 use futures::future::{join_all, ok, FutureExt, LocalBoxFuture};
 
 use crate::config::{AppConfig, AppService};
-use crate::data::DataFactory;
+use crate::data::{FnDataFactory, DataFactory};
 use crate::error::Error;
 use crate::guard::Guard;
 use crate::request::{HttpRequest, HttpRequestPool};
@@ -23,8 +23,6 @@ type Guards = Vec<Box<dyn Guard>>;
 type HttpService = BoxService<ServiceRequest, ServiceResponse, Error>;
 type HttpNewService = BoxServiceFactory<(), ServiceRequest, ServiceResponse, Error, ()>;
 type BoxResponse = LocalBoxFuture<'static, Result<ServiceResponse, Error>>;
-type FnDataFactory =
-    Box<dyn Fn() -> LocalBoxFuture<'static, Result<Box<dyn DataFactory>, ()>>>;
 
 /// Service factory to convert `Request` to a `ServiceRequest<S>`.
 /// It also executes data factories.

src/data.rs

@@ -3,7 +3,7 @@ use std::sync::Arc;
 
 use actix_http::error::{Error, ErrorInternalServerError};
 use actix_http::Extensions;
-use futures::future::{err, ok, Ready};
+use futures::future::{err, ok, LocalBoxFuture, Ready};
 
 use crate::dev::Payload;
 use crate::extract::FromRequest;
@@ -14,6 +14,9 @@ pub(crate) trait DataFactory {
     fn create(&self, extensions: &mut Extensions) -> bool;
 }
 
+pub(crate) type FnDataFactory =
+    Box<dyn Fn() -> LocalBoxFuture<'static, Result<Box<dyn DataFactory>, ()>>>;
+
 /// Application data.
 ///
 /// Application data is an arbitrary data attached to the app.

src/resource.rs

@@ -196,9 +196,11 @@ where
         self.app_data(Data::new(data))
     }
 
-    /// Set or override application data.
+    /// Add resource data.
     ///
-    /// This method overrides data stored with [`App::app_data()`](#method.app_data)
+    /// If used, this method will create a new data context used for extracting
+    /// from requests. Data added here is *not* merged with data added on App 
+    /// or containing scopes.
     pub fn app_data<U: 'static>(mut self, data: U) -> Self {
         if self.data.is_none() {
             self.data = Some(Extensions::new());
@@ -393,6 +395,7 @@ where
         if let Some(ref mut ext) = self.data {
             config.set_service_data(ext);
         }
+
         config.register_service(rdef, guards, self, None)
     }
 }
@@ -587,13 +590,14 @@ mod tests {
 
     use actix_rt::time::delay_for;
     use actix_service::Service;
+    use bytes::Bytes;
     use futures::future::ok;
 
     use crate::http::{header, HeaderValue, Method, StatusCode};
     use crate::middleware::DefaultHeaders;
     use crate::service::ServiceRequest;
-    use crate::test::{call_service, init_service, TestRequest};
-    use crate::{guard, web, App, Error, HttpResponse};
+    use crate::test::{call_service, init_service, read_body, TestRequest};
+    use crate::{guard, web, App, Error, HttpRequest, HttpResponse};
 
     #[actix_rt::test]
     async fn test_middleware() {
@@ -619,6 +623,79 @@ mod tests {
         );
     }
 
+    #[actix_rt::test]
+    async fn test_overwritten_data() {
+        #[allow(dead_code)]
+        fn echo_usize(req: HttpRequest) -> HttpResponse {
+            let num = req.app_data::<usize>().unwrap();
+            HttpResponse::Ok().body(format!("{}", num))
+        }
+
+        #[allow(dead_code)]
+        fn echo_u32(req: HttpRequest) -> HttpResponse {
+            let num = req.app_data::<u32>().unwrap();
+            HttpResponse::Ok().body(format!("{}", num))
+        }
+
+        #[allow(dead_code)]
+        fn echo_both(req: HttpRequest) -> HttpResponse {
+            let num = req.app_data::<usize>().unwrap();
+            let num2 = req.app_data::<u32>().unwrap();
+            HttpResponse::Ok().body(format!("{}-{}", num, num2))
+        }
+
+        let mut srv = init_service(
+            App::new()
+                .app_data(88usize)
+                .service(web::resource("/").route(web::get().to(echo_usize)))
+                .service(
+                    web::resource("/one")
+                        .app_data(1usize)
+                        .route(web::get().to(echo_usize)),
+                )
+                .service(
+                    web::resource("/two")
+                        .app_data(2usize)
+                        .route(web::get().to(echo_usize)),
+                )
+                .service(
+                    web::resource("/three")
+                        .app_data(3u32)
+                        // this doesnt work because app_data "overrides" the
+                        // entire data field potentially passed down
+                        // .route(web::get().to(echo_both)),
+                        .route(web::get().to(echo_u32)),
+                )
+                .service(web::resource("/eight").route(web::get().to(echo_usize))),
+        )
+        .await;
+
+        let req = TestRequest::get().uri("/").to_request();
+        let resp = srv.call(req).await.unwrap();
+        let body = read_body(resp).await;
+        assert_eq!(body, Bytes::from_static(b"88"));
+
+        let req = TestRequest::get().uri("/one").to_request();
+        let resp = srv.call(req).await.unwrap();
+        let body = read_body(resp).await;
+        assert_eq!(body, Bytes::from_static(b"1"));
+
+        let req = TestRequest::get().uri("/two").to_request();
+        let resp = srv.call(req).await.unwrap();
+        let body = read_body(resp).await;
+        assert_eq!(body, Bytes::from_static(b"2"));
+
+        // let req = TestRequest::get().uri("/three").to_request();
+        // let resp = srv.call(req).await.unwrap();
+        // let body = read_body(resp).await;
+        // assert_eq!(body, Bytes::from_static(b"88-3"));
+
+        let req = TestRequest::get().uri("/eight").to_request();
+        let resp = srv.call(req).await.unwrap();
+        let body = read_body(resp).await;
+        assert_eq!(body, Bytes::from_static(b"88"));
+    }
+
     #[actix_rt::test]
     async fn test_middleware_fn() {
         let mut srv = init_service(

src/scope.rs

@@ -151,9 +151,11 @@ where
         self.app_data(Data::new(data))
     }
 
-    /// Set or override application data.
+    /// Add scope data.
     ///
-    /// This method overrides data stored with [`App::app_data()`](#method.app_data)
+    /// If used, this method will create a new data context used for extracting
+    /// from requests. Data added here is *not* merged with data added on App
+    /// or containing scopes.
     pub fn app_data<U: 'static>(mut self, data: U) -> Self {
         if self.data.is_none() {
             self.data = Some(Extensions::new());