diff --git a/CHANGES.md b/CHANGES.md index 876e1c03d..3da742f9d 100644 --- a/CHANGES.md +++ b/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 diff --git a/src/app.rs b/src/app.rs index 677f73805..5cff20568 100644 --- a/src/app.rs +++ b/src/app.rs @@ -43,13 +43,14 @@ impl App { /// 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, diff --git a/src/app_service.rs b/src/app_service.rs index 0e590e2b7..bdb7ec433 100644 --- a/src/app_service.rs +++ b/src/app_service.rs @@ -1,22 +1,22 @@ -use std::cell::RefCell; -use std::rc::Rc; +use std::{cell::RefCell, mem, rc::Rc}; use actix_http::{Extensions, Request}; use actix_router::{Path, ResourceDef, Router, Url}; -use actix_service::boxed::{self, BoxService, BoxServiceFactory}; -use actix_service::{fn_service, Service, ServiceFactory}; +use actix_service::{ + 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>; @@ -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); } diff --git a/src/config.rs b/src/config.rs index 966141193..884128308 100644 --- a/src/config.rs +++ b/src/config.rs @@ -94,9 +94,9 @@ impl AppService { F: IntoServiceFactory, S: ServiceFactory< ServiceRequest, - Config = (), Response = ServiceResponse, Error = Error, + Config = (), InitError = (), > + 'static, { diff --git a/src/resource.rs b/src/resource.rs index 7a4c1248b..9f5cf3cb2 100644 --- a/src/resource.rs +++ b/src/resource.rs @@ -400,34 +400,28 @@ where *rdef.name_mut() = name.clone(); } - config.register_service(rdef, guards, self, None) - } -} - -impl IntoServiceFactory for Resource -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, - app_data: Option>, default: HttpNewService, } @@ -446,8 +440,6 @@ impl ServiceFactory 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 for ResourceFactory { .into_iter() .collect::, _>>()?; - Ok(ResourceService { - routes, - app_data, - default, - }) + Ok(ResourceService { routes, default }) }) } } pub struct ResourceService { routes: Vec, - app_data: Option>, default: HttpService, } @@ -480,18 +467,10 @@ impl Service 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::middleware::DefaultHeaders; - use crate::service::ServiceRequest; - use crate::test::{call_service, init_service, TestRequest}; - use crate::{guard, web, App, Error, HttpResponse}; + use crate::{ + guard, + http::{header, HeaderValue, Method, StatusCode}, + middleware::DefaultHeaders, + 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::(), 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::(), 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); + } } diff --git a/src/scope.rs b/src/scope.rs index 0caf06ee3..aa546c422 100644 --- a/src/scope.rs +++ b/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>, services: Rc<[(ResourceDef, HttpNewService, RefCell>)]>, default: Rc, } @@ -488,8 +497,6 @@ impl ServiceFactory for ScopeFactory { } })); - let app_data = self.app_data.clone(); - Box::pin(async move { let default = default_fut.await?; @@ -505,17 +512,12 @@ impl ServiceFactory for ScopeFactory { }) .finish(); - Ok(ScopeService { - app_data, - router, - default, - }) + Ok(ScopeService { router, default }) }) } } pub struct ScopeService { - app_data: Option>, router: Router>>, default: HttpService, } @@ -539,10 +541,6 @@ impl Service 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::http::{header, HeaderValue, Method, StatusCode}; - use crate::middleware::DefaultHeaders; - use crate::service::ServiceRequest; - use crate::test::{call_service, init_service, read_body, TestRequest}; - use crate::{guard, web, App, HttpRequest, HttpResponse}; + use crate::{ + dev::Body, + guard, + http::{header, HeaderValue, Method, StatusCode}, + middleware::DefaultHeaders, + 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( - web::scope("/app1") - .default_service(web::resource("").to(HttpResponse::BadRequest)), - ) + .service(web::scope("/app1").default_service(web::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::(), 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::(), 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]