add tests and improve docs

CHANGES.md

@@ -1,12 +1,18 @@
 # Changes
 
 ## Unreleased - 2020-xx-xx
-* Implement Logger middleware regex exclude pattern [#1723]
-* Print unconfigured `Data<T>` type when attempting extraction. [#1743]
+### Added
+* Implement `exclude_regex` for Logger middleware. [#1723]
+* Add request-local data extractor `web::ReqData`. [#1729]
 
+### Changed
+* Print non-configured `Data<T>` type when attempting extraction. [#1743]
+
+[#1729]: https://github.com/actix/actix-web/pull/1729
 [#1723]: https://github.com/actix/actix-web/pull/1723
 [#1743]: https://github.com/actix/actix-web/pull/1743
 
+
 ## 3.1.0 - 2020-09-29
 ### Changed
 * Add `TrailingSlash::MergeOnly` behaviour to `NormalizePath`, which allows `NormalizePath`

src/data.rs

@@ -20,22 +20,20 @@ pub(crate) type FnDataFactory =
 
 /// Application data.
 ///
-/// Application data is a piece of arbitrary data attached to the app.
-/// Application data is available to all routes and can be added
-/// during the application configuration process via `App::data()`.
+/// Application level data is a piece of arbitrary data attached to the app, scope, or resource.
+/// Application data is available to all routes and can be added during the application
+/// configuration process via `App::data()`.
 ///
-/// Application data can be accessed by using `Data<T>`
-/// extractor where `T` is data type.
+/// Application data can be accessed by using `Data<T>` extractor where `T` is data type.
 ///
-/// **Note**: http server accepts an application factory rather than
-/// an application instance. Http server constructs an application
-/// instance for each thread, thus application data must be constructed
-/// multiple times. If you want to share data between different
-/// threads, a shareable object should be used, e.g. `Send + Sync`. Application
-/// data does not need to be `Send` or `Sync`. Internally `Data` uses `Arc`.
+/// **Note**: http server accepts an application factory rather than an application instance. HTTP
+/// server constructs an application instance for each thread, thus application data must be
+/// constructed multiple times. If you want to share data between different threads, a shareable
+/// object should be used, e.g. `Send + Sync`. Application data does not need to be `Send`
+/// or `Sync`. Internally `Data` uses `Arc`.
 ///
-/// If route data is not set for a handler, using `Data<T>` extractor would
-/// cause *Internal Server Error* response.
+/// If route data is not set for a handler, using `Data<T>` extractor would cause *Internal
+/// Server Error* response.
 ///
 /// ```rust
 /// use std::sync::Mutex;

src/request_data.rs

@@ -1,19 +1,60 @@
-use std::ops::{Deref, DerefMut};
+use std::{any::type_name, ops::Deref};
 
 use actix_http::error::{Error, ErrorInternalServerError};
 use futures_util::future;
 
 use crate::{dev::Payload, FromRequest, HttpRequest};
 
-/// Request data.
+/// Request-local data.
 ///
-/// Request data is a piece of arbitrary data attached to a request.
+/// Request-local data is arbitrary data attached to an individual request, usually
+/// by middleware. It can be set via `extensions_mut` on [`HttpRequest`](htr_ext_mut)
+/// or [`ServiceRequest`](srv_ext_mut).
 ///
-/// It can be set via [`HttpMessage::extensions_mut`].
+/// Unlike app data, request data is dropped when the request has finished processing. This makes it
+/// useful as a kind of messaging system between middleware and request handlers. It uses the same
+/// types-as-keys storage system as app data.
 ///
-/// [`HttpMessage::extensions_mut`]: crate::HttpMessage::extensions_mut
-#[derive(Clone, Debug)]
-pub struct ReqData<T: Clone + 'static>(pub T);
+/// # Mutating Request Data
+/// Note that since extractors must output owned data, only types that `impl Clone` can use this
+/// extractor. A clone is taken of the required request data and can, therefore, not be directly
+/// mutated in-place. To mutate request data, continue to use [`HttpRequest::extensions_mut`] or
+/// re-insert the cloned data back into the extensions map. A `DerefMut` impl is intentionally not
+/// provided to make this potential foot-gun more obvious.
+///
+/// # Example
+/// ```rust,no_run
+/// # use actix_web::{web, HttpResponse, HttpRequest, Responder};
+///
+/// #[derive(Debug, Clone, PartialEq)]
+/// struct FlagFromMiddleware(String);
+///
+/// /// Use the `Data<T>` extractor to access data in a handler.
+/// async fn handler(
+///     req: HttpRequest,
+///     opt_flag: Option<web::ReqData<FlagFromMiddleware>>,
+/// ) -> impl Responder {
+///     if let Some(flag) = opt_flag {
+///         // using an optional extractor since the middleware may
+///         // not be guaranteed to add our flag to request data
+///         assert_eq!(&flag.into_inner(), req.extensions().get::<FlagFromMiddleware>().unwrap());
+///     }
+///     
+///     HttpResponse::Ok()
+/// }
+/// ```
+///
+/// [`htr_ext_mut`]: crate::HttpRequest::extensions_mut
+/// [`srv_ext_mut`]: crate::ServiceRequest::extensions_mut
+#[derive(Debug, Clone)]
+pub struct ReqData<T: Clone + 'static>(T);
+
+impl<T: Clone + 'static> ReqData<T> {
+    /// Consumes the `ReqData`, returning it's wrapped data.
+    pub fn into_inner(self) -> T {
+        self.0
+    }
+}
 
 impl<T: Clone + 'static> Deref for ReqData<T> {
     type Target = T;
@@ -23,12 +64,6 @@ impl<T: Clone + 'static> Deref for ReqData<T> {
     }
 }
 
-impl<T: Clone + 'static> DerefMut for ReqData<T> {
-    fn deref_mut(&mut self) -> &mut Self::Target {
-        &mut self.0
-    }
-}
-
 impl<T: Clone + 'static> FromRequest for ReqData<T> {
     type Config = ();
     type Error = Error;
@@ -40,8 +75,9 @@ impl<T: Clone + 'static> FromRequest for ReqData<T> {
         } else {
             log::debug!(
                 "Failed to construct App-level ReqData extractor. \
-                 Request path: {:?}",
-                req.path()
+                 Request path: {:?} (type: {})",
+                req.path(),
+                type_name::<T>(),
             );
             future::err(ErrorInternalServerError(
                 "Missing expected request extension data",
@@ -49,3 +85,91 @@ impl<T: Clone + 'static> FromRequest for ReqData<T> {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::{cell::RefCell, rc::Rc};
+
+    use futures_util::TryFutureExt as _;
+
+    use super::*;
+    use crate::{
+        dev::Service,
+        http::{Method, StatusCode},
+        test::{init_service, TestRequest},
+        web, App, HttpMessage, HttpResponse,
+    };
+
+    #[actix_rt::test]
+    async fn req_data_extractor() {
+        let mut srv = init_service(
+            App::new()
+                .wrap_fn(|req, srv| {
+                    if req.method() == Method::POST {
+                        req.extensions_mut().insert(42u32);
+                    }
+
+                    srv.call(req)
+                })
+                .service(web::resource("/test").to(
+                    |req: HttpRequest, data: Option<ReqData<u32>>| {
+                        if req.method() != Method::POST {
+                            assert!(data.is_none());
+                        }
+
+                        if let Some(data) = data {
+                            assert_eq!(*data, 42);
+                            assert_eq!(
+                                Some(data.into_inner()),
+                                req.extensions().get::<u32>().copied()
+                            );
+                        }
+
+                        HttpResponse::Ok()
+                    },
+                )),
+        )
+        .await;
+
+        let req = TestRequest::get().uri("/test").to_request();
+        let resp = srv.call(req).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::OK);
+
+        let req = TestRequest::post().uri("/test").to_request();
+        let resp = srv.call(req).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::OK);
+    }
+
+    #[actix_rt::test]
+    async fn req_data_internal_mutability() {
+        let mut srv = init_service(
+            App::new()
+                .wrap_fn(|req, srv| {
+                    let data_before = Rc::new(RefCell::new(42u32));
+                    req.extensions_mut().insert(data_before);
+
+                    srv.call(req).map_ok(|res| {
+                        {
+                            let ext = res.request().extensions();
+                            let data_after = ext.get::<Rc<RefCell<u32>>>().unwrap();
+                            assert_eq!(*data_after.borrow(), 53u32);
+                        }
+
+                        res
+                    })
+                })
+                .default_service(web::to(|data: ReqData<Rc<RefCell<u32>>>| {
+                    assert_eq!(*data.borrow(), 42);
+                    *data.borrow_mut() += 11;
+                    assert_eq!(*data.borrow(), 53);
+
+                    HttpResponse::Ok()
+                })),
+        )
+        .await;
+
+        let req = TestRequest::get().uri("/test").to_request();
+        let resp = srv.call(req).await.unwrap();
+        assert_eq!(resp.status(), StatusCode::OK);
+    }
+}