rework ResourceMap internals (#2337)

src/app_service.rs

@@ -79,7 +79,7 @@ where
             .into_iter()
             .for_each(|mut srv| srv.register(&mut config));
 
-        let mut rmap = ResourceMap::new(ResourceDef::new(""));
+        let mut rmap = ResourceMap::new(ResourceDef::prefix(""));
 
         let (config, services) = config.into_services();
 
@@ -104,7 +104,7 @@ where
 
         // complete ResourceMap tree creation
         let rmap = Rc::new(rmap);
-        rmap.finish(rmap.clone());
+        ResourceMap::finish(&rmap);
 
         // construct all async data factory futures
         let factory_futs = join_all(self.async_data_factories.iter().map(|f| f()));

src/request.rs

@@ -511,7 +511,7 @@ mod tests {
         let mut res = ResourceDef::new("/user/{name}.{ext}");
         res.set_name("index");
 
-        let mut rmap = ResourceMap::new(ResourceDef::new(""));
+        let mut rmap = ResourceMap::new(ResourceDef::prefix(""));
         rmap.add(&mut res, None);
         assert!(rmap.has_resource("/user/test.html"));
         assert!(!rmap.has_resource("/test/unknown"));
@@ -541,7 +541,7 @@ mod tests {
         let mut rdef = ResourceDef::new("/index.html");
         rdef.set_name("index");
 
-        let mut rmap = ResourceMap::new(ResourceDef::new(""));
+        let mut rmap = ResourceMap::new(ResourceDef::prefix(""));
         rmap.add(&mut rdef, None);
 
         assert!(rmap.has_resource("/index.html"));
@@ -562,7 +562,7 @@ mod tests {
         let mut rdef = ResourceDef::new("/index.html");
         rdef.set_name("index");
 
-        let mut rmap = ResourceMap::new(ResourceDef::new(""));
+        let mut rmap = ResourceMap::new(ResourceDef::prefix(""));
         rmap.add(&mut rdef, None);
 
         assert!(rmap.has_resource("/index.html"));
@@ -581,9 +581,8 @@ mod tests {
 
         rdef.set_name("youtube");
 
-        let mut rmap = ResourceMap::new(ResourceDef::new(""));
+        let mut rmap = ResourceMap::new(ResourceDef::prefix(""));
         rmap.add(&mut rdef, None);
-        assert!(rmap.has_resource("https://youtube.com/watch/unknown"));
 
         let req = TestRequest::default().rmap(rmap).to_http_request();
         let url = req.url_for("youtube", &["oHg5SJYRHA0"]);

src/rmap.rs

@@ -10,43 +10,75 @@ use crate::request::HttpRequest;
 
 #[derive(Clone, Debug)]
 pub struct ResourceMap {
-    root: ResourceDef,
+    pattern: ResourceDef,
+
+    /// Named resources within the tree or, for external resources,
+    /// it points to isolated nodes outside the tree.
+    named: AHashMap<String, Rc<ResourceMap>>,
+
     parent: RefCell<Weak<ResourceMap>>,
-    named: AHashMap<String, ResourceDef>,
-    patterns: Vec<(ResourceDef, Option<Rc<ResourceMap>>)>,
+
+    /// Must be `None` for "edge" nodes.
+    nodes: Option<Vec<Rc<ResourceMap>>>,
 }
 
 impl ResourceMap {
+    /// Creates a _container_ node in the `ResourceMap` tree.
     pub fn new(root: ResourceDef) -> Self {
         ResourceMap {
-            root,
-            parent: RefCell::new(Weak::new()),
+            pattern: root,
             named: AHashMap::default(),
-            patterns: Vec::new(),
+            parent: RefCell::new(Weak::new()),
+            nodes: Some(Vec::new()),
         }
     }
 
+    /// Adds a (possibly nested) resource.
+    ///
+    /// To add a non-prefix pattern, `nested` must be `None`.
+    /// To add external resource, supply a pattern without a leading `/`.
+    /// The root pattern of `nested`, if present, should match `pattern`.
     pub fn add(&mut self, pattern: &mut ResourceDef, nested: Option<Rc<ResourceMap>>) {
-        pattern.set_id(self.patterns.len() as u16);
-        self.patterns.push((pattern.clone(), nested));
+        pattern.set_id(self.nodes.as_ref().unwrap().len() as u16);
+
+        if let Some(new_node) = nested {
+            assert_eq!(&new_node.pattern, pattern, "`patern` and `nested` mismatch");
+            self.named.extend(new_node.named.clone().into_iter());
+            self.nodes.as_mut().unwrap().push(new_node);
+        } else {
+            let new_node = Rc::new(ResourceMap {
+                pattern: pattern.clone(),
+                named: AHashMap::default(),
+                parent: RefCell::new(Weak::new()),
+                nodes: None,
+            });
+
             if let Some(name) = pattern.name() {
-            self.named.insert(name.to_owned(), pattern.clone());
+                self.named.insert(name.to_owned(), Rc::clone(&new_node));
+            }
+
+            let is_external = match pattern.pattern() {
+                Some(p) => !p.is_empty() && !p.starts_with('/'),
+                None => false,
+            };
+
+            // Don't add external resources to the tree
+            if !is_external {
+                self.nodes.as_mut().unwrap().push(new_node);
+            }
         }
     }
 
-    pub(crate) fn finish(&self, current: Rc<ResourceMap>) {
-        for (_, nested) in &self.patterns {
-            if let Some(ref nested) = nested {
-                *nested.parent.borrow_mut() = Rc::downgrade(&current);
-                nested.finish(nested.clone());
-            }
+    pub(crate) fn finish(self: &Rc<Self>) {
+        for node in self.nodes.iter().flatten() {
+            node.parent.replace(Rc::downgrade(self));
+            ResourceMap::finish(node);
         }
     }
 
     /// Generate url for named resource
     ///
-    /// Check [`HttpRequest::url_for()`](../struct.HttpRequest.html#method.
-    /// url_for) for detailed information.
+    /// Check [`HttpRequest::url_for`] for detailed information.
     pub fn url_for<U, I>(
         &self,
         req: &HttpRequest,
@@ -57,10 +89,19 @@ impl ResourceMap {
         U: IntoIterator<Item = I>,
         I: AsRef<str>,
     {
-        let mut path = String::new();
         let mut elements = elements.into_iter();
 
-        if self.patterns_for(name, &mut path, &mut elements)?.is_some() {
+        let path = self
+            .named
+            .get(name)
+            .ok_or(UrlGenerationError::ResourceNotFound)?
+            .root_rmap_fn(String::with_capacity(24), |mut acc, node| {
+                node.pattern
+                    .resource_path_from_iter(&mut acc, &mut elements)
+                    .then(|| acc)
+            })
+            .ok_or(UrlGenerationError::NotEnoughElements)?;
+
         if path.starts_with('/') {
             let conn = req.connection_info();
             Ok(Url::parse(&format!(
@@ -72,182 +113,73 @@ impl ResourceMap {
         } else {
             Ok(Url::parse(&path)?)
         }
-        } else {
-            Err(UrlGenerationError::ResourceNotFound)
-        }
     }
 
     pub fn has_resource(&self, path: &str) -> bool {
-        let path = if path.is_empty() { "/" } else { path };
-
-        for (pattern, rmap) in &self.patterns {
-            if let Some(ref rmap) = rmap {
-                if let Some(pat_len) = pattern.find_match(path) {
-                    return rmap.has_resource(&path[pat_len..]);
-                }
-            } else if pattern.is_match(path) || pattern.pattern() == Some("") && path == "/" {
-                return true;
-            }
-        }
-        false
+        self.find_matching_node(path).is_some()
     }
 
     /// Returns the name of the route that matches the given path or None if no full match
-    /// is possible.
+    /// is possible or the matching resource is not named.
     pub fn match_name(&self, path: &str) -> Option<&str> {
-        let path = if path.is_empty() { "/" } else { path };
-
-        for (pattern, rmap) in &self.patterns {
-            if let Some(ref rmap) = rmap {
-                if let Some(plen) = pattern.find_match(path) {
-                    return rmap.match_name(&path[plen..]);
-                }
-            } else if pattern.is_match(path) {
-                return pattern.name();
-            }
-        }
-
-        None
+        self.find_matching_node(path)?.pattern.name()
     }
 
     /// Returns the full resource pattern matched against a path or None if no full match
     /// is possible.
     pub fn match_pattern(&self, path: &str) -> Option<String> {
-        let path = if path.is_empty() { "/" } else { path };
-
-        // ensure a full match exists
-        if !self.has_resource(path) {
-            return None;
+        self.find_matching_node(path)?.root_rmap_fn(
+            String::with_capacity(24),
+            |mut acc, node| {
+                acc.push_str(node.pattern.pattern()?);
+                Some(acc)
+            },
+        )
     }
 
-        Some(self.traverse_resource_pattern(path))
+    fn find_matching_node(&self, path: &str) -> Option<&ResourceMap> {
+        self._find_matching_node(path).flatten()
     }
 
-    /// Takes remaining path and tries to match it up against a resource definition within the
-    /// current resource map recursively, returning a concatenation of all resource prefixes and
-    /// patterns matched in the tree.
-    ///
-    /// Should only be used after checking the resource exists in the map so that partial match
-    /// patterns are not returned.
-    fn traverse_resource_pattern(&self, remaining: &str) -> String {
-        for (pattern, rmap) in &self.patterns {
-            if let Some(ref rmap) = rmap {
-                if let Some(prefix_len) = pattern.find_match(remaining) {
-                    // TODO: think about unwrap_or
-                    let prefix = pattern.pattern().unwrap_or("").to_owned();
+    /// Returns `None` if root pattern doesn't match;
+    /// `Some(None)` if root pattern matches but there is no matching child pattern.
+    /// Don't search sideways when `Some(none)` is returned.
+    fn _find_matching_node(&self, path: &str) -> Option<Option<&ResourceMap>> {
+        let matched_len = self.pattern.find_match(path)?;
+        let path = &path[matched_len..];
 
-                    return [
-                        prefix,
-                        rmap.traverse_resource_pattern(&remaining[prefix_len..]),
-                    ]
-                    .concat();
-                }
-            } else if pattern.is_match(remaining) {
-                // TODO: think about unwrap_or
-                return pattern.pattern().unwrap_or("").to_owned();
-            }
+        Some(match &self.nodes {
+            // find first sub-node to match remaining path
+            Some(nodes) => nodes
+                .iter()
+                .filter_map(|node| node._find_matching_node(path))
+                .next()
+                .flatten(),
+
+            // only terminate at edge nodes
+            None => Some(self),
+        })
     }
 
-        String::new()
-    }
-
-    fn patterns_for<U, I>(
-        &self,
-        name: &str,
-        path: &mut String,
-        elements: &mut U,
-    ) -> Result<Option<()>, UrlGenerationError>
+    /// Find `self`'s highest ancestor and then run `F`, providing `B`, in that rmap context.
+    fn root_rmap_fn<F, B>(&self, init: B, mut f: F) -> Option<B>
     where
-        U: Iterator<Item = I>,
-        I: AsRef<str>,
+        F: FnMut(B, &ResourceMap) -> Option<B>,
     {
-        if self.pattern_for(name, path, elements)?.is_some() {
-            Ok(Some(()))
-        } else {
-            self.parent_pattern_for(name, path, elements)
-        }
+        self._root_rmap_fn(init, &mut f)
     }
 
-    fn pattern_for<U, I>(
-        &self,
-        name: &str,
-        path: &mut String,
-        elements: &mut U,
-    ) -> Result<Option<()>, UrlGenerationError>
+    /// Run `F`, providing `B`, if `self` is top-level resource map, else recurse to parent map.
+    fn _root_rmap_fn<F, B>(&self, init: B, f: &mut F) -> Option<B>
     where
-        U: Iterator<Item = I>,
-        I: AsRef<str>,
+        F: FnMut(B, &ResourceMap) -> Option<B>,
     {
-        if let Some(pattern) = self.named.get(name) {
-            if pattern
-                .pattern()
-                .map(|pat| pat.starts_with('/'))
-                .unwrap_or(false)
-            {
-                self.fill_root(path, elements)?;
-            }
+        let data = match self.parent.borrow().upgrade() {
+            Some(ref parent) => parent._root_rmap_fn(init, f)?,
+            None => init,
+        };
 
-            if pattern.resource_path_from_iter(path, elements) {
-                Ok(Some(()))
-            } else {
-                Err(UrlGenerationError::NotEnoughElements)
-            }
-        } else {
-            for (_, rmap) in &self.patterns {
-                if let Some(ref rmap) = rmap {
-                    if rmap.pattern_for(name, path, elements)?.is_some() {
-                        return Ok(Some(()));
-                    }
-                }
-            }
-            Ok(None)
-        }
-    }
-
-    fn fill_root<U, I>(
-        &self,
-        path: &mut String,
-        elements: &mut U,
-    ) -> Result<(), UrlGenerationError>
-    where
-        U: Iterator<Item = I>,
-        I: AsRef<str>,
-    {
-        if let Some(ref parent) = self.parent.borrow().upgrade() {
-            parent.fill_root(path, elements)?;
-        }
-
-        if self.root.resource_path_from_iter(path, elements) {
-            Ok(())
-        } else {
-            Err(UrlGenerationError::NotEnoughElements)
-        }
-    }
-
-    fn parent_pattern_for<U, I>(
-        &self,
-        name: &str,
-        path: &mut String,
-        elements: &mut U,
-    ) -> Result<Option<()>, UrlGenerationError>
-    where
-        U: Iterator<Item = I>,
-        I: AsRef<str>,
-    {
-        if let Some(ref parent) = self.parent.borrow().upgrade() {
-            if let Some(pattern) = parent.named.get(name) {
-                self.fill_root(path, elements)?;
-                if pattern.resource_path_from_iter(path, elements) {
-                    Ok(Some(()))
-                } else {
-                    Err(UrlGenerationError::NotEnoughElements)
-                }
-            } else {
-                parent.parent_pattern_for(name, path, elements)
-            }
-        } else {
-            Ok(None)
-        }
+        f(data, self)
     }
 }
 
@@ -259,7 +191,7 @@ mod tests {
     fn extract_matched_pattern() {
         let mut root = ResourceMap::new(ResourceDef::root_prefix(""));
 
-        let mut user_map = ResourceMap::new(ResourceDef::root_prefix(""));
+        let mut user_map = ResourceMap::new(ResourceDef::root_prefix("/user/{id}"));
         user_map.add(&mut ResourceDef::new("/"), None);
         user_map.add(&mut ResourceDef::new("/profile"), None);
         user_map.add(&mut ResourceDef::new("/article/{id}"), None);
@@ -275,9 +207,10 @@ mod tests {
             &mut ResourceDef::root_prefix("/user/{id}"),
             Some(Rc::new(user_map)),
         );
+        root.add(&mut ResourceDef::new("/info"), None);
 
         let root = Rc::new(root);
-        root.finish(Rc::clone(&root));
+        ResourceMap::finish(&root);
 
         // sanity check resource map setup
 
@@ -288,7 +221,7 @@ mod tests {
         assert!(root.has_resource("/v2"));
         assert!(!root.has_resource("/v33"));
 
-        assert!(root.has_resource("/user/22"));
+        assert!(!root.has_resource("/user/22"));
         assert!(root.has_resource("/user/22/"));
         assert!(root.has_resource("/user/22/profile"));
 
@@ -336,7 +269,7 @@ mod tests {
         rdef.set_name("root_info");
         root.add(&mut rdef, None);
 
-        let mut user_map = ResourceMap::new(ResourceDef::root_prefix(""));
+        let mut user_map = ResourceMap::new(ResourceDef::root_prefix("/user/{id}"));
         let mut rdef = ResourceDef::new("/");
         user_map.add(&mut rdef, None);
 
@@ -350,14 +283,14 @@ mod tests {
         );
 
         let root = Rc::new(root);
-        root.finish(Rc::clone(&root));
+        ResourceMap::finish(&root);
 
         // sanity check resource map setup
 
         assert!(root.has_resource("/info"));
         assert!(!root.has_resource("/bar"));
 
-        assert!(root.has_resource("/user/22"));
+        assert!(!root.has_resource("/user/22"));
         assert!(root.has_resource("/user/22/"));
         assert!(root.has_resource("/user/22/post/55"));
 
@@ -377,7 +310,7 @@ mod tests {
         // ref: https://github.com/actix/actix-web/issues/1582
         let mut root = ResourceMap::new(ResourceDef::root_prefix(""));
 
-        let mut user_map = ResourceMap::new(ResourceDef::root_prefix(""));
+        let mut user_map = ResourceMap::new(ResourceDef::root_prefix("/user/{id}"));
         user_map.add(&mut ResourceDef::new("/"), None);
         user_map.add(&mut ResourceDef::new("/profile"), None);
         user_map.add(&mut ResourceDef::new("/article/{id}"), None);
@@ -393,20 +326,119 @@ mod tests {
         );
 
         let root = Rc::new(root);
-        root.finish(Rc::clone(&root));
+        ResourceMap::finish(&root);
 
         // check root has no parent
         assert!(root.parent.borrow().upgrade().is_none());
         // check child has parent reference
-        assert!(root.patterns[0].1.is_some());
+        assert!(root.nodes.as_ref().unwrap()[0]
+            .parent
+            .borrow()
+            .upgrade()
+            .is_some());
         // check child's parent root id matches root's root id
-        assert_eq!(
-            root.patterns[0].1.as_ref().unwrap().root.id(),
-            root.root.id()
-        );
+        assert!(Rc::ptr_eq(
+            &root.nodes.as_ref().unwrap()[0]
+                .parent
+                .borrow()
+                .upgrade()
+                .unwrap(),
+            &root
+        ));
 
         let output = format!("{:?}", root);
         assert!(output.starts_with("ResourceMap {"));
         assert!(output.ends_with(" }"));
     }
+
+    #[test]
+    fn short_circuit() {
+        let mut root = ResourceMap::new(ResourceDef::prefix(""));
+
+        let mut user_root = ResourceDef::prefix("/user");
+        let mut user_map = ResourceMap::new(user_root.clone());
+        user_map.add(&mut ResourceDef::new("/u1"), None);
+        user_map.add(&mut ResourceDef::new("/u2"), None);
+
+        root.add(&mut ResourceDef::new("/user/u3"), None);
+        root.add(&mut user_root, Some(Rc::new(user_map)));
+        root.add(&mut ResourceDef::new("/user/u4"), None);
+
+        let rmap = Rc::new(root);
+        ResourceMap::finish(&rmap);
+
+        assert!(rmap.has_resource("/user/u1"));
+        assert!(rmap.has_resource("/user/u2"));
+        assert!(rmap.has_resource("/user/u3"));
+        assert!(!rmap.has_resource("/user/u4"));
+    }
+
+    #[test]
+    fn url_for() {
+        let mut root = ResourceMap::new(ResourceDef::prefix(""));
+
+        let mut user_scope_rdef = ResourceDef::prefix("/user");
+        let mut user_scope_map = ResourceMap::new(user_scope_rdef.clone());
+
+        let mut user_rdef = ResourceDef::new("/{user_id}");
+        let mut user_map = ResourceMap::new(user_rdef.clone());
+
+        let mut post_rdef = ResourceDef::new("/post/{sub_id}");
+        post_rdef.set_name("post");
+
+        user_map.add(&mut post_rdef, None);
+        user_scope_map.add(&mut user_rdef, Some(Rc::new(user_map)));
+        root.add(&mut user_scope_rdef, Some(Rc::new(user_scope_map)));
+
+        let rmap = Rc::new(root);
+        ResourceMap::finish(&rmap);
+
+        let mut req = crate::test::TestRequest::default();
+        req.set_server_hostname("localhost:8888");
+        let req = req.to_http_request();
+
+        let url = rmap
+            .url_for(&req, "post", &["u123", "foobar"])
+            .unwrap()
+            .to_string();
+        assert_eq!(url, "http://localhost:8888/user/u123/post/foobar");
+
+        assert!(rmap.url_for(&req, "missing", &["u123"]).is_err());
+    }
+
+    #[test]
+    fn external_resource_with_no_name() {
+        let mut root = ResourceMap::new(ResourceDef::prefix(""));
+
+        let mut rdef = ResourceDef::new("https://duck.com/{query}");
+        root.add(&mut rdef, None);
+
+        let rmap = Rc::new(root);
+        ResourceMap::finish(&rmap);
+
+        assert!(!rmap.has_resource("https://duck.com/abc"));
+    }
+
+    #[test]
+    fn external_resource_with_name() {
+        let mut root = ResourceMap::new(ResourceDef::prefix(""));
+
+        let mut rdef = ResourceDef::new("https://duck.com/{query}");
+        rdef.set_name("duck");
+        root.add(&mut rdef, None);
+
+        let rmap = Rc::new(root);
+        ResourceMap::finish(&rmap);
+
+        assert!(!rmap.has_resource("https://duck.com/abc"));
+
+        let mut req = crate::test::TestRequest::default();
+        req.set_server_hostname("localhost:8888");
+        let req = req.to_http_request();
+
+        assert_eq!(
+            rmap.url_for(&req, "duck", &["abcd"]).unwrap().to_string(),
+            "https://duck.com/abcd"
+        );
+    }
 }