return option item from Extensions::insert (#1904)

actix-http/CHANGES.md

@@ -12,6 +12,7 @@
   `mime` types. [#1894]
 * Renamed `IntoHeaderValue::{try_into => try_into_value}` to avoid ambiguity with std
   `TryInto` trait. [#1894]
+* `Extensions::insert` returns Option of replaced item. [#1904]
 
 ### Removed
 * `ResponseBuilder::set`; use `ResponseBuilder::insert_header`. [#1869]
@@ -22,6 +23,7 @@
 
 [#1869]: https://github.com/actix/actix-web/pull/1869
 [#1894]: https://github.com/actix/actix-web/pull/1894
+[#1904]: https://github.com/actix/actix-web/pull/1904
 
 
 ## 3.0.0-beta.1 - 2021-01-07

actix-http/src/extensions.rs

@@ -5,7 +5,9 @@ use std::{
 
 use ahash::AHashMap;
 
-/// A type map of request extensions.
+/// A type map for request extensions.
+///
+/// All entries into this map must be owned types (or static references).
 #[derive(Default)]
 pub struct Extensions {
     /// Use FxHasher with a std HashMap with for faster
@@ -14,7 +16,7 @@ pub struct Extensions {
 }
 
 impl Extensions {
-    /// Create an empty `Extensions`.
+    /// Creates an empty `Extensions`.
     #[inline]
     pub fn new() -> Extensions {
         Extensions {
@@ -22,43 +24,96 @@ impl Extensions {
         }
     }
 
-    /// Insert a type into this `Extensions`.
+    /// Insert an item into the map.
     ///
-    /// If a extension of this type already existed, it will
+    /// If an item of this type was already stored, it will be replaced and returned.
-    /// be returned.
+    ///
-    pub fn insert<T: 'static>(&mut self, val: T) {
+    /// ```
-        self.map.insert(TypeId::of::<T>(), Box::new(val));
+    /// # use actix_http::Extensions;
+    /// let mut map = Extensions::new();
+    /// assert_eq!(map.insert(""), None);
+    /// assert_eq!(map.insert(1u32), None);
+    /// assert_eq!(map.insert(2u32), Some(1u32));
+    /// assert_eq!(*map.get::<u32>().unwrap(), 2u32);
+    /// ```
+    pub fn insert<T: 'static>(&mut self, val: T) -> Option<T> {
+        self.map
+            .insert(TypeId::of::<T>(), Box::new(val))
+            .and_then(downcast_owned)
     }
 
-    /// Check if container contains entry
+    /// Check if map contains an item of a given type.
+    ///
+    /// ```
+    /// # use actix_http::Extensions;
+    /// let mut map = Extensions::new();
+    /// assert!(!map.contains::<u32>());
+    ///
+    /// assert_eq!(map.insert(1u32), None);
+    /// assert!(map.contains::<u32>());
+    /// ```
     pub fn contains<T: 'static>(&self) -> bool {
         self.map.contains_key(&TypeId::of::<T>())
     }
 
-    /// Get a reference to a type previously inserted on this `Extensions`.
+    /// Get a reference to an item of a given type.
+    ///
+    /// ```
+    /// # use actix_http::Extensions;
+    /// let mut map = Extensions::new();
+    /// map.insert(1u32);
+    /// assert_eq!(map.get::<u32>(), Some(&1u32));
+    /// ```
     pub fn get<T: 'static>(&self) -> Option<&T> {
         self.map
             .get(&TypeId::of::<T>())
             .and_then(|boxed| boxed.downcast_ref())
     }
 
-    /// Get a mutable reference to a type previously inserted on this `Extensions`.
+    /// Get a mutable reference to an item of a given type.
+    ///
+    /// ```
+    /// # use actix_http::Extensions;
+    /// let mut map = Extensions::new();
+    /// map.insert(1u32);
+    /// assert_eq!(map.get_mut::<u32>(), Some(&mut 1u32));
+    /// ```
     pub fn get_mut<T: 'static>(&mut self) -> Option<&mut T> {
         self.map
             .get_mut(&TypeId::of::<T>())
             .and_then(|boxed| boxed.downcast_mut())
     }
 
-    /// Remove a type from this `Extensions`.
+    /// Remove an item from the map of a given type.
     ///
-    /// If a extension of this type existed, it will be returned.
+    /// If an item of this type was already stored, it will be returned.
+    ///
+    /// ```
+    /// # use actix_http::Extensions;
+    /// let mut map = Extensions::new();
+    ///
+    /// map.insert(1u32);
+    /// assert_eq!(map.get::<u32>(), Some(&1u32));
+    ///
+    /// assert_eq!(map.remove::<u32>(), Some(1u32));
+    /// assert!(!map.contains::<u32>());
+    /// ```
     pub fn remove<T: 'static>(&mut self) -> Option<T> {
-        self.map
+        self.map.remove(&TypeId::of::<T>()).and_then(downcast_owned)
-            .remove(&TypeId::of::<T>())
-            .and_then(|boxed| boxed.downcast().ok().map(|boxed| *boxed))
     }
 
     /// Clear the `Extensions` of all inserted extensions.
+    ///
+    /// ```
+    /// # use actix_http::Extensions;
+    /// let mut map = Extensions::new();
+    ///
+    /// map.insert(1u32);
+    /// assert!(map.contains::<u32>());
+    ///
+    /// map.clear();
+    /// assert!(!map.contains::<u32>());
+    /// ```
     #[inline]
     pub fn clear(&mut self) {
         self.map.clear();
@@ -81,6 +136,10 @@ impl fmt::Debug for Extensions {
     }
 }
 
+fn downcast_owned<T: 'static>(boxed: Box<dyn Any>) -> Option<T> {
+    boxed.downcast().ok().map(|boxed| *boxed)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

actix-http/tests/test_openssl.rs

@@ -408,7 +408,9 @@ async fn test_h2_service_error() {
 async fn test_h2_on_connect() {
     let srv = test_server(move || {
         HttpService::build()
-            .on_connect_ext(|_, data| data.insert(20isize))
+            .on_connect_ext(|_, data| {
+                data.insert(20isize);
+            })
             .h2(|req: Request| {
                 assert!(req.extensions().contains::<isize>());
                 ok::<_, ()>(Response::Ok().finish())

actix-http/tests/test_server.rs

@@ -662,7 +662,9 @@ async fn test_h1_service_error() {
 async fn test_h1_on_connect() {
     let srv = test_server(|| {
         HttpService::build()
-            .on_connect_ext(|_, data| data.insert(20isize))
+            .on_connect_ext(|_, data| {
+                data.insert(20isize);
+            })
             .h1(|req: Request| {
                 assert!(req.extensions().contains::<isize>());
                 future::ok::<_, ()>(Response::Ok().finish())