implement App::data as App::app_data(Data::new(T))) (#1906)

CHANGES.md

@@ -12,15 +12,22 @@
   `ServiceRequest::from_request` would not fail and no payload would be generated [#1893]
 * Our `Either` type now uses `Left`/`Right` variants (instead of `A`/`B`) [#1894]
 
+### Fixed
+* Multiple calls `App::data` with the same type now keeps the latest call's data. [#1906]
+
 ### Removed
 * Public field of `web::Path` has been made private. [#1894]
 * Public field of `web::Query` has been made private. [#1894]
 * `TestRequest::with_header`; use `TestRequest::default().insert_header()`. [#1869]
+* `AppService::set_service_data`; for custom HTTP service factories adding application data, use the
+  layered data model by calling `ServiceRequest::add_data_container` when handling
+  requests instead. [#1906]
 
 [#1891]: https://github.com/actix/actix-web/pull/1891
 [#1893]: https://github.com/actix/actix-web/pull/1893
 [#1894]: https://github.com/actix/actix-web/pull/1894
 [#1869]: https://github.com/actix/actix-web/pull/1869
+[#1906]: https://github.com/actix/actix-web/pull/1906
 
 
 ## 4.0.0-beta.1 - 2021-01-07

src/app.rs

@@ -34,7 +34,6 @@ pub struct App<T, B> {
     services: Vec<Box<dyn AppServiceFactory>>,
     default: Option<Rc<HttpNewService>>,
     factory_ref: Rc<RefCell<Option<AppRoutingFactory>>>,
-    data: Vec<Box<dyn DataFactory>>,
     data_factories: Vec<FnDataFactory>,
     external: Vec<ResourceDef>,
     extensions: Extensions,
@@ -48,7 +47,6 @@ impl App<AppEntry, Body> {
         let fref = Rc::new(RefCell::new(None));
         App {
             endpoint: AppEntry::new(fref.clone()),
-            data: Vec::new(),
             data_factories: Vec::new(),
             services: Vec::new(),
             default: None,
@@ -101,9 +99,8 @@ where
     ///         web::resource("/index.html").route(
     ///             web::get().to(index)));
     /// ```
-    pub fn data<U: 'static>(mut self, data: U) -> Self {
+    pub fn data<U: 'static>(self, data: U) -> Self {
-        self.data.push(Box::new(Data::new(data)));
+        self.app_data(Data::new(data))
-        self
     }
 
     /// Set application data factory. This function is
@@ -157,8 +154,7 @@ where
     /// some of the resource's configuration could be moved to different module.
     ///
     /// ```rust
-    /// # extern crate actix_web;
+    /// use actix_web::{web, App, HttpResponse};
-    /// use actix_web::{web, middleware, App, HttpResponse};
     ///
     /// // this function could be located in different module
     /// fn config(cfg: &mut web::ServiceConfig) {
@@ -168,12 +164,9 @@ where
     ///     );
     /// }
     ///
-    /// fn main() {
+    /// App::new()
-    ///     let app = App::new()
+    ///     .configure(config)  // <- register resources
-    ///         .wrap(middleware::Logger::default())
+    ///     .route("/index.html", web::get().to(|| HttpResponse::Ok()));
-    ///         .configure(config)  // <- register resources
-    ///         .route("/index.html", web::get().to(|| HttpResponse::Ok()));
-    /// }
     /// ```
     pub fn configure<F>(mut self, f: F) -> Self
     where
@@ -181,10 +174,9 @@ where
     {
         let mut cfg = ServiceConfig::new();
         f(&mut cfg);
-        self.data.extend(cfg.data);
         self.services.extend(cfg.services);
         self.external.extend(cfg.external);
-        self.extensions.extend(cfg.extensions);
+        self.extensions.extend(cfg.app_data);
         self
     }
 
@@ -374,7 +366,6 @@ where
     {
         App {
             endpoint: apply(mw, self.endpoint),
-            data: self.data,
             data_factories: self.data_factories,
             services: self.services,
             default: self.default,
@@ -436,7 +427,6 @@ where
     {
         App {
             endpoint: apply_fn_factory(self.endpoint, mw),
-            data: self.data,
             data_factories: self.data_factories,
             services: self.services,
             default: self.default,
@@ -462,7 +452,6 @@ where
 {
     fn into_factory(self) -> AppInit<T, B> {
         AppInit {
-            data_factories: self.data.into_boxed_slice().into(),
             async_data_factories: self.data_factories.into_boxed_slice().into(),
             endpoint: self.endpoint,
             services: Rc::new(RefCell::new(self.services)),

src/app_service.rs

@@ -10,7 +10,7 @@ use futures_core::future::LocalBoxFuture;
 use futures_util::future::join_all;
 
 use crate::config::{AppConfig, AppService};
-use crate::data::{DataFactory, FnDataFactory};
+use crate::data::FnDataFactory;
 use crate::error::Error;
 use crate::guard::Guard;
 use crate::request::{HttpRequest, HttpRequestPool};
@@ -35,7 +35,6 @@ where
 {
     pub(crate) endpoint: T,
     pub(crate) extensions: RefCell<Option<Extensions>>,
-    pub(crate) data_factories: Rc<[Box<dyn DataFactory>]>,
     pub(crate) async_data_factories: Rc<[FnDataFactory]>,
     pub(crate) services: Rc<RefCell<Vec<Box<dyn AppServiceFactory>>>>,
     pub(crate) default: Option<Rc<HttpNewService>>,
@@ -71,8 +70,7 @@ where
         });
 
         // App config
-        let mut config =
+        let mut config = AppService::new(config, default.clone());
-            AppService::new(config, default.clone(), self.data_factories.clone());
 
         // register services
         std::mem::take(&mut *self.services.borrow_mut())
@@ -119,8 +117,6 @@ where
             .take()
             .unwrap_or_else(Extensions::new);
 
-        let data_factories = self.data_factories.clone();
-
         Box::pin(async move {
             // async data factories
             let async_data_factories = factory_futs
@@ -133,12 +129,9 @@ where
             let service = endpoint_fut.await?;
 
             // populate app data container from (async) data factories.
-            data_factories
+            async_data_factories.iter().for_each(|factory| {
-                .iter()
+                factory.create(&mut app_data);
-                .chain(&async_data_factories)
+            });
-                .for_each(|factory| {
-                    factory.create(&mut app_data);
-                });
 
             Ok(AppInitService {
                 service,

src/config.rs

@@ -5,7 +5,7 @@ use actix_http::Extensions;
 use actix_router::ResourceDef;
 use actix_service::{boxed, IntoServiceFactory, ServiceFactory};
 
-use crate::data::{Data, DataFactory};
+use crate::data::Data;
 use crate::error::Error;
 use crate::guard::Guard;
 use crate::resource::Resource;
@@ -31,20 +31,14 @@ pub struct AppService {
         Option<Guards>,
         Option<Rc<ResourceMap>>,
     )>,
-    service_data: Rc<[Box<dyn DataFactory>]>,
 }
 
 impl AppService {
-    /// Crate server settings instance
+    /// Crate server settings instance.
-    pub(crate) fn new(
+    pub(crate) fn new(config: AppConfig, default: Rc<HttpNewService>) -> Self {
-        config: AppConfig,
-        default: Rc<HttpNewService>,
-        service_data: Rc<[Box<dyn DataFactory>]>,
-    ) -> Self {
         AppService {
             config,
             default,
-            service_data,
             root: true,
             services: Vec::new(),
         }
@@ -75,7 +69,6 @@ impl AppService {
             default: self.default.clone(),
             services: Vec::new(),
             root: false,
-            service_data: self.service_data.clone(),
         }
     }
 
@@ -89,15 +82,7 @@ impl AppService {
         self.default.clone()
     }
 
-    /// Set global route data
+    /// Register HTTP service.
-    pub fn set_service_data(&self, extensions: &mut Extensions) -> bool {
-        for f in self.service_data.iter() {
-            f.create(extensions);
-        }
-        !self.service_data.is_empty()
-    }
-
-    /// Register http service
     pub fn register_service<F, S>(
         &mut self,
         rdef: ResourceDef,
@@ -168,47 +153,60 @@ impl Default for AppConfig {
     }
 }
 
-/// Service config is used for external configuration.
+/// Enables parts of app configuration to be declared separately from the app itself. Helpful for
-/// Part of application configuration could be offloaded
+/// modularizing large applications.
-/// to set of external methods. This could help with
+///
-/// modularization of big application configuration.
+/// Merge a `ServiceConfig` into an app using [`App::configure`](crate::App::configure). Scope and
+/// resources services have similar methods.
+///
+/// ```
+/// use actix_web::{web, App, HttpResponse};
+///
+/// // this function could be located in different module
+/// fn config(cfg: &mut web::ServiceConfig) {
+///     cfg.service(web::resource("/test")
+///         .route(web::get().to(|| HttpResponse::Ok()))
+///         .route(web::head().to(|| HttpResponse::MethodNotAllowed()))
+///     );
+/// }
+///
+/// // merge `/test` routes from config function to App
+/// App::new().configure(config);
+/// ```
 pub struct ServiceConfig {
     pub(crate) services: Vec<Box<dyn AppServiceFactory>>,
-    pub(crate) data: Vec<Box<dyn DataFactory>>,
     pub(crate) external: Vec<ResourceDef>,
-    pub(crate) extensions: Extensions,
+    pub(crate) app_data: Extensions,
 }
 
 impl ServiceConfig {
     pub(crate) fn new() -> Self {
         Self {
             services: Vec::new(),
-            data: Vec::new(),
             external: Vec::new(),
-            extensions: Extensions::new(),
+            app_data: Extensions::new(),
         }
     }
 
-    /// Set application data. Application data could be accessed
+    /// Add shared app data item.
-    /// by using `Data<T>` extractor where `T` is data type.
     ///
-    /// This is same as `App::data()` method.
+    /// Counterpart to [`App::data()`](crate::App::data).
-    pub fn data<S: 'static>(&mut self, data: S) -> &mut Self {
+    pub fn data<U: 'static>(&mut self, data: U) -> &mut Self {
-        self.data.push(Box::new(Data::new(data)));
+        self.app_data(Data::new(data));
         self
     }
 
-    /// Set arbitrary data item.
+    /// Add arbitrary app data item.
     ///
-    /// This is same as `App::data()` method.
+    /// Counterpart to [`App::app_data()`](crate::App::app_data).
     pub fn app_data<U: 'static>(&mut self, ext: U) -> &mut Self {
-        self.extensions.insert(ext);
+        self.app_data.insert(ext);
         self
     }
 
     /// Configure route for a specific path.
     ///
-    /// This is same as `App::route()` method.
+    /// Counterpart to [`App::route()`](crate::App::route).
     pub fn route(&mut self, path: &str, mut route: Route) -> &mut Self {
         self.service(
             Resource::new(path)
@@ -217,9 +215,9 @@ impl ServiceConfig {
         )
     }
 
-    /// Register http service.
+    /// Register HTTP service factory.
     ///
-    /// This is same as `App::service()` method.
+    /// Counterpart to [`App::service()`](crate::App::service).
     pub fn service<F>(&mut self, factory: F) -> &mut Self
     where
         F: HttpServiceFactory + 'static,
@@ -231,11 +229,11 @@ impl ServiceConfig {
 
     /// Register an external resource.
     ///
-    /// External resources are useful for URL generation purposes only
+    /// External resources are useful for URL generation purposes only and are never considered for
-    /// and are never considered for matching at request time. Calls to
+    /// matching at request time. Calls to [`HttpRequest::url_for()`](crate::HttpRequest::url_for)
-    /// `HttpRequest::url_for()` will work as expected.
+    /// will work as expected.
     ///
-    /// This is same as `App::external_service()` method.
+    /// Counterpart to [`App::external_resource()`](crate::App::external_resource).
     pub fn external_resource<N, U>(&mut self, name: N, url: U) -> &mut Self
     where
         N: AsRef<str>,

src/data.rs

@@ -10,8 +10,9 @@ use crate::dev::Payload;
 use crate::extract::FromRequest;
 use crate::request::HttpRequest;
 
-/// Application data factory
+/// Data factory.
 pub(crate) trait DataFactory {
+    /// Return true if modifications were made to extensions map.
     fn create(&self, extensions: &mut Extensions) -> bool;
 }
 
@@ -126,12 +127,8 @@ impl<T: ?Sized + 'static> FromRequest for Data<T> {
 
 impl<T: ?Sized + 'static> DataFactory for Data<T> {
     fn create(&self, extensions: &mut Extensions) -> bool {
-        if !extensions.contains::<Data<T>>() {
+        extensions.insert(Data(self.0.clone()));
-            extensions.insert(Data(self.0.clone()));
+        true
-            true
-        } else {
-            false
-        }
     }
 }
 
@@ -167,6 +164,24 @@ mod tests {
         let req = TestRequest::default().to_request();
         let resp = srv.call(req).await.unwrap();
         assert_eq!(resp.status(), StatusCode::INTERNAL_SERVER_ERROR);
+
+        let mut srv = init_service(
+            App::new()
+                .data(10u32)
+                .data(13u32)
+                .app_data(12u64)
+                .app_data(15u64)
+                .default_service(web::to(|n: web::Data<u32>, req: HttpRequest| {
+                    // in each case, the latter insertion should be preserved
+                    assert_eq!(*req.app_data::<u64>().unwrap(), 15);
+                    assert_eq!(*n.into_inner(), 13);
+                    HttpResponse::Ok()
+                })),
+        )
+        .await;
+        let req = TestRequest::default().to_request();
+        let resp = srv.call(req).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::OK);
     }
 
     #[actix_rt::test]

src/resource.rs

@@ -203,10 +203,10 @@ where
     ///
     /// Data of different types from parent contexts will still be accessible.
     pub fn app_data<U: 'static>(mut self, data: U) -> Self {
-        if self.app_data.is_none() {
+        self.app_data
-            self.app_data = Some(Extensions::new());
+            .get_or_insert_with(Extensions::new)
-        }
+            .insert(data);
-        self.app_data.as_mut().unwrap().insert(data);
+
         self
     }
 
@@ -382,18 +382,16 @@ where
         } else {
             Some(std::mem::take(&mut self.guards))
         };
+
         let mut rdef = if config.is_root() || !self.rdef.is_empty() {
             ResourceDef::new(insert_slash(self.rdef.clone()))
         } else {
             ResourceDef::new(self.rdef.clone())
         };
+
         if let Some(ref name) = self.name {
             *rdef.name_mut() = name.clone();
         }
-        // custom app data storage
-        if let Some(ref mut ext) = self.app_data {
-            config.set_service_data(ext);
-        }
 
         config.register_service(rdef, guards, self, None)
     }
@@ -479,12 +477,15 @@ impl Service<ServiceRequest> for ResourceService {
                 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)
     }
 }

src/scope.rs

@@ -155,10 +155,10 @@ where
     ///
     /// Data of different types from parent contexts will still be accessible.
     pub fn app_data<U: 'static>(mut self, data: U) -> Self {
-        if self.app_data.is_none() {
+        self.app_data
-            self.app_data = Some(Extensions::new());
+            .get_or_insert_with(Extensions::new)
-        }
+            .insert(data);
-        self.app_data.as_mut().unwrap().insert(data);
+
         self
     }
 
@@ -200,18 +200,9 @@ where
         self.services.extend(cfg.services);
         self.external.extend(cfg.external);
 
-        if !cfg.data.is_empty() {
-            let mut data = self.app_data.unwrap_or_else(Extensions::new);
-
-            for value in cfg.data.iter() {
-                value.create(&mut data);
-            }
-
-            self.app_data = Some(data);
-        }
         self.app_data
             .get_or_insert_with(Extensions::new)
-            .extend(cfg.extensions);
+            .extend(cfg.app_data);
         self
     }
 
@@ -432,11 +423,6 @@ where
             rmap.add(&mut rdef, None);
         }
 
-        // custom app data storage
-        if let Some(ref mut ext) = self.app_data {
-            config.set_service_data(ext);
-        }
-
         // complete scope pipeline creation
         *self.factory_ref.borrow_mut() = Some(ScopeFactory {
             app_data: self.app_data.take().map(Rc::new),

src/service.rs

@@ -231,8 +231,10 @@ impl ServiceRequest {
         self.payload = payload;
     }
 
-    #[doc(hidden)]
+    /// Add data container to request's resolution set.
-    /// Add app data container to request's resolution set.
+    ///
+    /// In middleware, prefer [`extensions_mut`](ServiceRequest::extensions_mut) for request-local
+    /// data since it is assumed that the same app data is presented for every request.
     pub fn add_data_container(&mut self, extensions: Rc<Extensions>) {
         Rc::get_mut(&mut (self.req).inner)
             .unwrap()