From da69bb4d12c35d170f85273eeda190ae86551d35 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Fri, 15 Jan 2021 23:37:33 +0000 Subject: [PATCH] implement `App::data` as `App::app_data(Data::new(T)))` (#1906) --- CHANGES.md | 7 ++++ src/app.rs | 25 ++++---------- src/app_service.rs | 17 +++------- src/config.rs | 82 ++++++++++++++++++++++------------------------ src/data.rs | 29 ++++++++++++---- src/resource.rs | 17 +++++----- src/scope.rs | 24 +++----------- src/service.rs | 6 ++-- 8 files changed, 99 insertions(+), 108 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 12caa2df..a6bcd56c 100644 --- a/CHANGES.md +++ b/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 diff --git a/src/app.rs b/src/app.rs index fcb491a2..9c5b806e 100644 --- a/src/app.rs +++ b/src/app.rs @@ -34,7 +34,6 @@ pub struct App { services: Vec>, default: Option>, factory_ref: Rc>>, - data: Vec>, data_factories: Vec, external: Vec, extensions: Extensions, @@ -48,7 +47,6 @@ impl App { 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(mut self, data: U) -> Self { - self.data.push(Box::new(Data::new(data))); - self + pub fn data(self, data: U) -> Self { + self.app_data(Data::new(data)) } /// 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, middleware, App, HttpResponse}; + /// use actix_web::{web, App, HttpResponse}; /// /// // this function could be located in different module /// fn config(cfg: &mut web::ServiceConfig) { @@ -168,12 +164,9 @@ where /// ); /// } /// - /// fn main() { - /// let app = App::new() - /// .wrap(middleware::Logger::default()) - /// .configure(config) // <- register resources - /// .route("/index.html", web::get().to(|| HttpResponse::Ok())); - /// } + /// App::new() + /// .configure(config) // <- register resources + /// .route("/index.html", web::get().to(|| HttpResponse::Ok())); /// ``` pub fn configure(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 { 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)), diff --git a/src/app_service.rs b/src/app_service.rs index 4c099d4a..8a00f59e 100644 --- a/src/app_service.rs +++ b/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>, - pub(crate) data_factories: Rc<[Box]>, pub(crate) async_data_factories: Rc<[FnDataFactory]>, pub(crate) services: Rc>>>, pub(crate) default: Option>, @@ -71,8 +70,7 @@ where }); // App config - let mut config = - AppService::new(config, default.clone(), self.data_factories.clone()); + let mut config = AppService::new(config, default.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 - .iter() - .chain(&async_data_factories) - .for_each(|factory| { - factory.create(&mut app_data); - }); + async_data_factories.iter().for_each(|factory| { + factory.create(&mut app_data); + }); Ok(AppInitService { service, diff --git a/src/config.rs b/src/config.rs index 2b93ae89..8e22dc90 100644 --- a/src/config.rs +++ b/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, Option>, )>, - service_data: Rc<[Box]>, } impl AppService { - /// Crate server settings instance - pub(crate) fn new( - config: AppConfig, - default: Rc, - service_data: Rc<[Box]>, - ) -> Self { + /// Crate server settings instance. + pub(crate) fn new(config: AppConfig, default: Rc) -> 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 - 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 + /// Register HTTP service. pub fn register_service( &mut self, rdef: ResourceDef, @@ -168,47 +153,60 @@ impl Default for AppConfig { } } -/// Service config is used for external configuration. -/// Part of application configuration could be offloaded -/// to set of external methods. This could help with -/// modularization of big application configuration. +/// Enables parts of app configuration to be declared separately from the app itself. Helpful for +/// modularizing large applications. +/// +/// 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>, - pub(crate) data: Vec>, pub(crate) external: Vec, - 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 - /// by using `Data` extractor where `T` is data type. + /// Add shared app data item. /// - /// This is same as `App::data()` method. - pub fn data(&mut self, data: S) -> &mut Self { - self.data.push(Box::new(Data::new(data))); + /// Counterpart to [`App::data()`](crate::App::data). + pub fn data(&mut self, data: U) -> &mut Self { + 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(&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(&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 - /// and are never considered for matching at request time. Calls to - /// `HttpRequest::url_for()` will work as expected. + /// External resources are useful for URL generation purposes only and are never considered for + /// matching at request time. Calls to [`HttpRequest::url_for()`](crate::HttpRequest::url_for) + /// 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(&mut self, name: N, url: U) -> &mut Self where N: AsRef, diff --git a/src/data.rs b/src/data.rs index 19c258ff..dd0fbed0 100644 --- a/src/data.rs +++ b/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 FromRequest for Data { impl DataFactory for Data { fn create(&self, extensions: &mut Extensions) -> bool { - if !extensions.contains::>() { - extensions.insert(Data(self.0.clone())); - true - } else { - false - } + extensions.insert(Data(self.0.clone())); + true } } @@ -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, req: HttpRequest| { + // in each case, the latter insertion should be preserved + assert_eq!(*req.app_data::().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] diff --git a/src/resource.rs b/src/resource.rs index 84323707..3844b429 100644 --- a/src/resource.rs +++ b/src/resource.rs @@ -203,10 +203,10 @@ where /// /// Data of different types from parent contexts will still be accessible. pub fn app_data(mut self, data: U) -> Self { - if self.app_data.is_none() { - self.app_data = Some(Extensions::new()); - } - self.app_data.as_mut().unwrap().insert(data); + self.app_data + .get_or_insert_with(Extensions::new) + .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 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) } } diff --git a/src/scope.rs b/src/scope.rs index 2da4f554..290a35eb 100644 --- a/src/scope.rs +++ b/src/scope.rs @@ -155,10 +155,10 @@ where /// /// Data of different types from parent contexts will still be accessible. pub fn app_data(mut self, data: U) -> Self { - if self.app_data.is_none() { - self.app_data = Some(Extensions::new()); - } - self.app_data.as_mut().unwrap().insert(data); + self.app_data + .get_or_insert_with(Extensions::new) + .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), diff --git a/src/service.rs b/src/service.rs index d4fa4acc..596eedd7 100644 --- a/src/service.rs +++ b/src/service.rs @@ -231,8 +231,10 @@ impl ServiceRequest { self.payload = payload; } - #[doc(hidden)] - /// Add app data container to request's resolution set. + /// Add 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) { Rc::get_mut(&mut (self.req).inner) .unwrap()