error on prefix-suffix conflicts

Author: ibraheemdev

README.md

@@ -29,62 +29,91 @@ The router supports dynamic route segments. These can either be named or catch-a
 Named parameters like `/{id}` match anything until the next static segment or the end of the path.
 
 ```rust,ignore
-let mut m = Router::new();
-m.insert("/users/{id}", true)?;
+let mut router = Router::new();
+router.insert("/users/{id}", 42)?;
 
-assert_eq!(m.at("/users/1")?.params.get("id"), Some("1"));
-assert_eq!(m.at("/users/23")?.params.get("id"), Some("23"));
-assert!(m.at("/users").is_err());
+let matched = router.at("/users/1")?;
+assert_eq!(matched.params.get("id"), Some("1"));
+
+let matched = router.at("/users/23")?;
+assert_eq!(matched.params.get("id"), Some("23"));
+
+assert!(router.at("/users").is_err());
 ```
 
 Prefixes and suffixes within a segment are also supported. However, there may only be a single named parameter per route segment.
 ```rust,ignore
-let mut m = Router::new();
-m.insert("/images/img{id}.png", true)?;
+let mut router = Router::new();
+router.insert("/images/img-{id}.png", true)?;
+
+let matched = router.at("/images/img-1.png")?;
+assert_eq!(matched.params.get("id"), Some("1"));
 
-assert_eq!(m.at("/images/img1.png")?.params.get("id"), Some("1"));
-assert!(m.at("/images/img1.jpg").is_err());
+assert!(router.at("/images/img-1.jpg").is_err());
 ```
 
-Catch-all parameters start with `*` and match anything until the end of the path. They must always be at the **end** of the route.
+Catch-all parameters start with a `*` and match anything until the end of the path. They must always be at the *end* of the route.
 
 ```rust,ignore
-let mut m = Router::new();
-m.insert("/{*p}", true)?;
+let mut router = Router::new();
+router.insert("/{*rest}", true)?;
 
-assert_eq!(m.at("/foo.js")?.params.get("p"), Some("foo.js"));
-assert_eq!(m.at("/c/bar.css")?.params.get("p"), Some("c/bar.css"));
+let matched = router.at("/foo.html")?;
+assert_eq!(matched.params.get("rest"), Some("foo.html"));
 
-// Note that this would lead to an empty parameter.
-assert!(m.at("/").is_err());
+let matched = router.at("/static/bar.css")?;
+assert_eq!(matched.params.get("rest"), Some("static/bar.css"));
+
+// Note that this would lead to an empty parameter value.
+assert!(router.at("/").is_err());
 ```
 
 The literal characters `{` and `}` may be included in a static route by escaping them with the same character.
-For example, the `{` character is escaped with `{{` and the `}` character is escaped with `}}`.
+For example, the `{` character is escaped with `{{`, and the `}` character is escaped with `}}`.
 
 ```rust,ignore
-let mut m = Router::new();
-m.insert("/{{hello}}", true)?;
-m.insert("/{hello}", true)?;
+let mut router = Router::new();
+router.insert("/{{hello}}", true)?;
+router.insert("/{hello}", true)?;
 
 // Match the static route.
-assert!(m.at("/{hello}")?.value);
+let matched = router.at("/{hello}")?;
+assert!(matched.params.is_empty());
 
 // Match the dynamic route.
-assert_eq!(m.at("/hello")?.params.get("hello"), Some("hello"));
+let matched = router.at("/hello")?;
+assert_eq!(matched.params.get("hello"), Some("hello"));
 ```
 
-## Routing Priority
+## Conflict Rules
 
 Static and dynamic route segments are allowed to overlap. If they do, static segments will be given higher priority:
 
 ```rust,ignore
-let mut m = Router::new();
-m.insert("/", "Welcome!").unwrap();      // Priority: 1
-m.insert("/about", "About Me").unwrap(); // Priority: 1
-m.insert("/{*filepath}", "...").unwrap();  // Priority: 2
+let mut router = Router::new();
+router.insert("/", "Welcome!").unwrap();       // Priority: 1
+router.insert("/about", "About Me").unwrap();  // Priority: 1
+router.insert("/{*filepath}", "...").unwrap();  // Priority: 2
 ```
 
+Formally, a route consists of a list of segments separated by `/`, with an optional leading and trailing slash: `(/)<segment_1>/.../<segment_n>(/)`.
+
+Given set of routes, their overlapping segments may include, in order of priority:
+
+- Any number of static segments (`/a`, `/b`, ...).
+- *One* of the following:
+  - Any number of route parameters with a suffix (`/{x}a`, `/{x}b`, ...), prioritizing the longest suffix.
+  - Any number of route parameters with a prefix (`/a{x}`, `/b{x}`, ...), prioritizing the longest prefix.
+  - A single route parameter with both a prefix and a suffix (`/a{x}b`).
+- *One* of the following;
+  - A single standalone parameter (`/{x}`).
+  - A single standalone catch-all parameter (`/{*rest}`).
+
+Any other combination of route segments is considered ambiguous, and attempting to insert such a route will result in an error.
+
+The one exception to the above set of rules is that catch-all parameters are always considered to conflict with suffixed route parameters, i.e. that `/{*rest}`
+and `/{x}suffix` are overlapping. This is due to an implementation detail of the routing tree that may be relaxed in the future.
+
 ## How does it work?
 
 The router takes advantage of the fact that URL routes generally follow a hierarchical structure.

src/error.rs

@@ -79,7 +79,7 @@ impl InsertError {
             route.append(&current.prefix);
         }
 
-        // Add the prefixes of any conflicting children.
+        // Add the prefixes of the first conflicting child.
         let mut child = current.children.first();
         while let Some(node) = child {
             route.append(&node.prefix);

src/lib.rs

@@ -26,12 +26,16 @@ Named parameters like `/{id}` match anything until the next static segment or th
 ```rust
 # use matchit::Router;
 # fn main() -> Result<(), Box<dyn std::error::Error>> {
-let mut m = Router::new();
-m.insert("/users/{id}", true)?;
+let mut router = Router::new();
+router.insert("/users/{id}", 42)?;
 
-assert_eq!(m.at("/users/1")?.params.get("id"), Some("1"));
-assert_eq!(m.at("/users/23")?.params.get("id"), Some("23"));
-assert!(m.at("/users").is_err());
+let matched = router.at("/users/1")?;
+assert_eq!(matched.params.get("id"), Some("1"));
+
+let matched = router.at("/users/23")?;
+assert_eq!(matched.params.get("id"), Some("23"));
+
+assert!(router.at("/users").is_err());
 # Ok(())
 # }
 ```
@@ -40,65 +44,90 @@ Prefixes and suffixes within a segment are also supported. However, there may on
 ```rust
 # use matchit::Router;
 # fn main() -> Result<(), Box<dyn std::error::Error>> {
-let mut m = Router::new();
-m.insert("/images/img{id}.png", true)?;
+let mut router = Router::new();
+router.insert("/images/img-{id}.png", true)?;
+
+let matched = router.at("/images/img-1.png")?;
+assert_eq!(matched.params.get("id"), Some("1"));
 
-assert_eq!(m.at("/images/img1.png")?.params.get("id"), Some("1"));
-assert!(m.at("/images/img1.jpg").is_err());
+assert!(router.at("/images/img-1.jpg").is_err());
 # Ok(())
 # }
 ```
 
-Catch-all parameters start with `*` and match anything until the end of the path. They must always be at the **end** of the route.
+Catch-all parameters start with a `*` and match anything until the end of the path. They must always be at the *end* of the route.
 
 ```rust
 # use matchit::Router;
 # fn main() -> Result<(), Box<dyn std::error::Error>> {
-let mut m = Router::new();
-m.insert("/{*p}", true)?;
+let mut router = Router::new();
+router.insert("/{*rest}", true)?;
 
-assert_eq!(m.at("/foo.js")?.params.get("p"), Some("foo.js"));
-assert_eq!(m.at("/c/bar.css")?.params.get("p"), Some("c/bar.css"));
+let matched = router.at("/foo.html")?;
+assert_eq!(matched.params.get("rest"), Some("foo.html"));
 
-// Note that this would lead to an empty parameter.
-assert!(m.at("/").is_err());
+let matched = router.at("/static/bar.css")?;
+assert_eq!(matched.params.get("rest"), Some("static/bar.css"));
+
+// Note that this would lead to an empty parameter value.
+assert!(router.at("/").is_err());
 # Ok(())
 # }
 ```
 
-The literal characters `{` and `}` may be included in a static route by escaping them with the same character. For example, the `{` character is escaped with `{{` and the `}` character is escaped with `}}`.
+The literal characters `{` and `}` may be included in a static route by escaping them with the same character. For example, the `{` character is escaped with `{{`, and the `}` character is escaped with `}}`.
 
 ```rust
 # use matchit::Router;
 # fn main() -> Result<(), Box<dyn std::error::Error>> {
-let mut m = Router::new();
-m.insert("/{{hello}}", true)?;
-m.insert("/{hello}", true)?;
+let mut router = Router::new();
+router.insert("/{{hello}}", true)?;
+router.insert("/{hello}", true)?;
 
 // Match the static route.
-assert!(m.at("/{hello}")?.value);
+let matched = router.at("/{hello}")?;
+assert!(matched.params.is_empty());
 
 // Match the dynamic route.
-assert_eq!(m.at("/hello")?.params.get("hello"), Some("hello"));
+let matched = router.at("/hello")?;
+assert_eq!(matched.params.get("hello"), Some("hello"));
 # Ok(())
 # }
 ```
 
-# Routing Priority
+# Conflict Rules
 
 Static and dynamic route segments are allowed to overlap. If they do, static segments will be given higher priority:
 
 ```rust
 # use matchit::Router;
 # fn main() -> Result<(), Box<dyn std::error::Error>> {
-let mut m = Router::new();
-m.insert("/", "Welcome!").unwrap();      // Priority: 1
-m.insert("/about", "About Me").unwrap(); // Priority: 1
-m.insert("/{*filepath}", "...").unwrap();  // Priority: 2
+let mut router = Router::new();
+router.insert("/", "Welcome!").unwrap();       // Priority: 1
+router.insert("/about", "About Me").unwrap();  // Priority: 1
+router.insert("/{*filepath}", "...").unwrap();  // Priority: 2
 # Ok(())
 # }
 ```
 
+Formally, a route consists of a list of segments separated by `/`, with an optional leading and trailing slash: `(/)<segment_1>/.../<segment_n>(/)`.
+
+Given set of routes, their overlapping segments may include, in order of priority:
+
+- Any number of static segments (`/a`, `/b`, ...).
+- *One* of the following:
+  - Any number of route parameters with a suffix (`/{x}a`, `/{x}b`, ...), prioritizing the longest suffix.
+  - Any number of route parameters with a prefix (`/a{x}`, `/b{x}`, ...), prioritizing the longest prefix.
+  - A single route parameter with both a prefix and a suffix (`/a{x}b`).
+- *One* of the following;
+  - A single standalone parameter (`/{x}`).
+  - A single standalone catch-all parameter (`/{*rest}`).
+
+Any other combination of route segments is considered ambiguous, and attempting to insert such a route will result in an error.
+
+The one exception to the above set of rules is that catch-all parameters are always considered to conflict with suffixed route parameters, i.e. that `/{*rest}`
+and `/{x}suffix` are overlapping. This is due to an implementation detail of the routing tree that may be relaxed in the future.
+
 # How does it work?
 
 The router takes advantage of the fact that URL routes generally follow a hierarchical structure. Routes are stored them in a radix trie that makes heavy use of common prefixes.

src/router.rs

@@ -125,7 +125,7 @@ impl<T> Router<T> {
     ///
     /// router.insert("/home/{id}/", "Hello!");
     /// // Invalid route.
-    /// assert_eq!(router.remove("/home/{id"), None);
+    /// assert_eq!(router.remove("/home/{id}"), None);
     /// assert_eq!(router.remove("/home/{id}/"), Some("Hello!"));
     /// ```
     pub fn remove(&mut self, path: impl Into<String>) -> Option<T> {