Fix matching static after dynamic segment

crates/percy-router/src/route.rs

@@ -121,7 +121,7 @@ impl Route {
             return false;
         }
 
-        // Iterate through all of the segments and verify that they match, or if it's a
+        // Iterate over every segment and verify that they match, or if it's a
         // RouteParam segment verify that we can parse it
         for (index, defined_segment) in defined_segments.iter().enumerate() {
             if defined_segment.len() == 0 {
@@ -140,15 +140,18 @@ impl Route {
                 // tacos
                 let incoming_param_value = incoming_segments[index];
 
-                return (self.route_param_parser)(param_name.as_str(), incoming_param_value)
-                    .is_some();
-            }
-
-            // Compare segments on the same level
-            let incoming_segment = incoming_segments[index];
+                let matches =
+                    (self.route_param_parser)(param_name.as_str(), incoming_param_value).is_some();
+                if !matches {
+                    return false;
+                }
+            } else {
+                // Compare segments on the same level
+                let incoming_segment = incoming_segments[index];
 
-            if defined_segment != &incoming_segment {
-                return false;
+                if defined_segment != &incoming_segment {
+                    return false;
+                }
             }
         }
 
@@ -177,67 +180,80 @@ impl Route {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use percy_dom::prelude::*;
 
-    struct MyView {
-        id: u32,
-    }
+    /// Verify that we can get the route parameter value for a given route parameter.
+    ///
+    /// For a route definition is `/:id`, and a path "/5" we confirm that when we ask for the
+    /// "id" parameter of the path we get back "5".
+    #[test]
+    fn find_route_param() {
+        let route = Route::new(
+            "/:id",
+            Box::new(|param_key, param_val| {
+                if param_key == "id" {
+                    Some(Box::new(u32::from_str_param(param_val).unwrap()));
+                }
 
-    impl View for MyView {
-        fn render(&self) -> VirtualNode {
-            let id = VirtualNode::text(self.id.to_string());
-            html! { <div> {id} </div> }
-        }
+                None
+            }),
+        );
+
+        assert_eq!(route.find_route_param("/5", "id"), Some("5"));
     }
 
+    /// Verify that route parameters such as `/:id` are typed.
+    ///
+    /// For instance, confirm that a route parameter that has an integer type does not match a
+    /// string.
     #[test]
     fn route_type_safety() {
         MatchRouteTestCase {
-            desc: "Typed route parameters",
             route_definition: "/users/:id",
             matches: vec![
-                ("/users/5", true, "5 should match since it is a u32"),
-                (
-                    "/users/foo",
-                    false,
-                    "foo should not match since it is not a u32",
-                ),
+                // Verify that an integer matches a parameter that is defined as an integer.
+                ("/users/5", true),
+                // Verify that a string does not match a parameter that is defined as an integer.
+                ("/users/foo", false),
             ],
         }
-        .test();
+            .test();
     }
 
+    /// Verify that a `/` route definition doesn't capture `/some-route`.
     #[test]
     fn route_cascade() {
         MatchRouteTestCase {
-            desc: "Make sure that `/` route doesn't capture `/other-routes`",
             route_definition: "/",
-            matches: vec![("/foo", false, "routes should not match additional segments")],
+            matches: vec![
+                // Verify that a "/" segment is not a catch-all.
+                // So, "/" should not match "/foo".
+                ("/foo", false),
+            ],
         }
-        .test();
+            .test();
     }
 
+    /// Verify that we correctly match when a static segment comes after a dynamic segment.
+    ///
+    /// This helps ensure that are not ignoring segments that come after a dynamic segment.
     #[test]
-    fn find_route_param() {
-        let route = Route::new(
-            "/:id",
-            Box::new(|param_key, param_val| {
-                if param_key == "id" {
-                    Some(Box::new(u32::from_str_param(param_val).unwrap()));
-                }
-
-                None
-            }),
-        );
-
-        assert_eq!(route.find_route_param("/5", "id"), Some("5"));
+    fn static_segment_after_dynamic_segment() {
+        MatchRouteTestCase {
+            route_definition: "/:id/foo",
+            matches: vec![
+                // Verify that a correct segment after a dynamic segment leads to a match.
+                ("/5/foo", true),
+                // Verify that an incorrect segment after a dynamic segment does not lead to a match.
+                ("/5/bar", false),
+            ],
+        }
+            .test();
     }
 
     struct MatchRouteTestCase {
-        desc: &'static str,
         route_definition: &'static str,
-        // (route ... should it match ... description of test)
-        matches: Vec<(&'static str, bool, &'static str)>,
+        // (path, should it match)
+        matches: Vec<(&'static str, bool)>,
     }
 
     impl MatchRouteTestCase {
@@ -259,9 +275,8 @@ mod tests {
                 assert_eq!(
                     route.matches(match_case.0),
                     match_case.1,
-                    "{}\n{}",
-                    self.desc,
-                    match_case.2
+                    "Test case failed: {}",
+                    match_case.0,
                 );
             }
         }