add url_for test for conflicting named resources

actix-web-codegen/src/lib.rs

@@ -74,8 +74,10 @@ mod route;
 ///
 /// # Attributes
 /// - `"path"` - Raw literal string with path for which to register handler.
-/// - `name="resource_name"` - Specifies resource name for the handler. If not set, the function name of handler is used.
-/// - `method="HTTP_METHOD"` - Registers HTTP method to provide guard for. Upper-case string, "GET", "POST" for example.
+/// - `name = "resource_name"` - Specifies resource name for the handler. If not set, the function
+///   name of handler is used.
+/// - `method = "HTTP_METHOD"` - Registers HTTP method to provide guard for. Upper-case string,
+///   "GET", "POST" for example.
 /// - `guard = "function_name"` - Registers function as guard using `actix_web::guard::fn_guard`
 /// - `wrap = "Middleware"` - Registers a resource middleware.
 ///

src/config.rs

@@ -102,8 +102,14 @@ impl AppService {
                 InitError = (),
             > + 'static,
     {
-        self.services
-            .push((rdef, boxed::factory(factory.into_factory()), guards, nested));
+        dbg!(rdef.pattern());
+
+        self.services.push((
+            rdef,
+            boxed::factory(factory.into_factory()),
+            guards,
+            dbg!(nested),
+        ));
     }
 }
 

src/request.rs

@@ -508,10 +508,12 @@ mod tests {
     use bytes::Bytes;
 
     use super::*;
-    use crate::dev::{ResourceDef, ResourceMap};
-    use crate::http::{header, StatusCode};
-    use crate::test::{call_service, init_service, read_body, TestRequest};
-    use crate::{web, App, HttpResponse};
+    use crate::{
+        dev::{ResourceDef, ResourceMap},
+        http::{header, StatusCode},
+        test::{self, call_service, init_service, read_body, TestRequest},
+        web, App, HttpResponse,
+    };
 
     #[test]
     fn test_debug() {
@@ -865,4 +867,46 @@ mod tests {
         let res = call_service(&srv, req).await;
         assert_eq!(res.status(), StatusCode::OK);
     }
+
+    #[actix_rt::test]
+    async fn url_for_closest_named_resource() {
+        // we mount the route named 'nested' on 2 different scopes, 'a' and 'b'
+        let srv = test::init_service(
+            App::new()
+                .service(
+                    web::scope("/foo")
+                        .service(web::resource("/nested").name("nested").route(web::get().to(
+                            |req: HttpRequest| {
+                                HttpResponse::Ok()
+                                    .body(format!("{}", req.url_for_static("nested").unwrap()))
+                            },
+                        )))
+                        .service(web::scope("/baz").service(web::resource("deep")))
+                        .service(web::resource("{foo_param}")),
+                )
+                .service(web::scope("/bar").service(
+                    web::resource("/nested").name("nested").route(web::get().to(
+                        |req: HttpRequest| {
+                            HttpResponse::Ok()
+                                .body(format!("{}", req.url_for_static("nested").unwrap()))
+                        },
+                    )),
+                )),
+        )
+        .await;
+
+        let foo_resp =
+            test::call_service(&srv, TestRequest::with_uri("/foo/nested").to_request()).await;
+        assert_eq!(foo_resp.status(), StatusCode::OK);
+        let body = read_body(foo_resp).await;
+        // XXX: body equals http://localhost:8080/bar/nested
+        // because nested from /bar overrides /foo's
+        assert_eq!(body, "http://localhost:8080/bar/nested");
+
+        let bar_resp =
+            test::call_service(&srv, TestRequest::with_uri("/bar/nested").to_request()).await;
+        assert_eq!(bar_resp.status(), StatusCode::OK);
+        let body = read_body(bar_resp).await;
+        assert_eq!(body, "http://localhost:8080/bar/nested");
+    }
 }

src/rmap.rs

@@ -1,6 +1,7 @@
 use std::{
     borrow::Cow,
     cell::RefCell,
+    fmt::Write as _,
     rc::{Rc, Weak},
 };
 
@@ -10,12 +11,14 @@ use url::Url;
 
 use crate::{error::UrlGenerationError, request::HttpRequest};
 
+const AVG_PATH_LEN: usize = 24;
+
 #[derive(Clone, Debug)]
 pub struct ResourceMap {
     pattern: ResourceDef,
 
-    /// Named resources within the tree or, for external resources,
-    /// it points to isolated nodes outside the tree.
+    /// 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>>,
@@ -35,6 +38,35 @@ impl ResourceMap {
         }
     }
 
+    /// Format resource map as tree structure (unfinished).
+    #[allow(dead_code)]
+    pub(crate) fn tree(&self) -> String {
+        let mut buf = String::new();
+        self._tree(&mut buf, 0);
+        buf
+    }
+
+    pub(crate) fn _tree(&self, buf: &mut String, level: usize) {
+        if let Some(children) = &self.nodes {
+            for child in children {
+                writeln!(
+                    buf,
+                    "{}{} {}",
+                    "--".repeat(level),
+                    child.pattern.pattern().unwrap(),
+                    child
+                        .pattern
+                        .name()
+                        .map(|name| format!("({})", name))
+                        .unwrap_or_else(|| "".to_owned())
+                )
+                .unwrap();
+
+                ResourceMap::_tree(child, buf, level + 1);
+            }
+        }
+    }
+
     /// Adds a (possibly nested) resource.
     ///
     /// To add a non-prefix pattern, `nested` must be `None`.
@@ -44,7 +76,11 @@ impl ResourceMap {
         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");
+            debug_assert_eq!(
+                &new_node.pattern, pattern,
+                "`pattern` and `nested` mismatch"
+            );
+            // parents absorb references to the named resources of children
             self.named.extend(new_node.named.clone().into_iter());
             self.nodes.as_mut().unwrap().push(new_node);
         } else {
@@ -64,7 +100,7 @@ impl ResourceMap {
                 None => false,
             };
 
-            // Don't add external resources to the tree
+            // don't add external resources to the tree
             if !is_external {
                 self.nodes.as_mut().unwrap().push(new_node);
             }
@@ -78,7 +114,7 @@ impl ResourceMap {
         }
     }
 
-    /// Generate url for named resource
+    /// Generate URL for named resource.
     ///
     /// Check [`HttpRequest::url_for`] for detailed information.
     pub fn url_for<U, I>(
@@ -97,7 +133,7 @@ impl ResourceMap {
             .named
             .get(name)
             .ok_or(UrlGenerationError::ResourceNotFound)?
-            .root_rmap_fn(String::with_capacity(24), |mut acc, node| {
+            .root_rmap_fn(String::with_capacity(AVG_PATH_LEN), |mut acc, node| {
                 node.pattern
                     .resource_path_from_iter(&mut acc, &mut elements)
                     .then(|| acc)
@@ -128,6 +164,7 @@ impl ResourceMap {
         Ok(url)
     }
 
+    /// Returns true if there is a resource that would match `path`.
     pub fn has_resource(&self, path: &str) -> bool {
         self.find_matching_node(path).is_some()
     }
@@ -142,9 +179,10 @@ impl ResourceMap {
     /// is possible.
     pub fn match_pattern(&self, path: &str) -> Option<String> {
         self.find_matching_node(path)?.root_rmap_fn(
-            String::with_capacity(24),
+            String::with_capacity(AVG_PATH_LEN),
             |mut acc, node| {
-                acc.push_str(node.pattern.pattern()?);
+                let pattern = node.pattern.pattern()?;
+                acc.push_str(pattern);
                 Some(acc)
             },
         )
@@ -490,4 +528,33 @@ mod tests {
             "https://duck.com/abcd"
         );
     }
+
+    #[test]
+    fn url_for_override_within_map() {
+        let mut root = ResourceMap::new(ResourceDef::prefix(""));
+
+        let mut foo_rdef = ResourceDef::prefix("/foo");
+        let mut foo_map = ResourceMap::new(foo_rdef.clone());
+        let mut nested_rdef = ResourceDef::new("/nested");
+        nested_rdef.set_name("nested");
+        foo_map.add(&mut nested_rdef, None);
+        root.add(&mut foo_rdef, Some(Rc::new(foo_map)));
+
+        let mut foo_rdef = ResourceDef::prefix("/bar");
+        let mut foo_map = ResourceMap::new(foo_rdef.clone());
+        let mut nested_rdef = ResourceDef::new("/nested");
+        nested_rdef.set_name("nested");
+        foo_map.add(&mut nested_rdef, None);
+        root.add(&mut foo_rdef, Some(Rc::new(foo_map)));
+
+        let rmap = Rc::new(root);
+        ResourceMap::finish(&rmap);
+
+        let req = crate::test::TestRequest::default().to_http_request();
+
+        let url = rmap.url_for(&req, "nested", &[""; 0]).unwrap().to_string();
+        assert_eq!(url, "http://localhost:8080/bar/nested");
+
+        assert!(rmap.url_for(&req, "missing", &["u123"]).is_err());
+    }
 }

src/service.rs

@@ -831,6 +831,7 @@ mod tests {
                     let req = ServiceRequest::from_parts(req, pl);
                     svc.call(req)
                 })
+                .route("/", web::get().to(|| async { "" }))
                 .service(
                     web::resource("/resource1/{name}/index.html").route(web::get().to(index)),
                 ),