[REF-2643]Throw Errors for duplicate Routes

reflex/app.py

@@ -11,6 +11,7 @@
 import multiprocessing
 import os
 import platform
+import re
 from typing import (
     Any,
     AsyncIterator,
@@ -63,8 +64,6 @@
     DECORATED_PAGES,
 )
 from reflex.route import (
-    catchall_in_route,
-    catchall_prefix,
     get_route_args,
     verify_route_validity,
 )
@@ -432,6 +431,9 @@ def add_page(
             on_load: The event handler(s) that will be called each time the page load.
             meta: The metadata of the page.
             script_tags: List of script tags to be added to component
+
+        Raises:
+            ValueError: When the specified route name already exists.
         """
         # If the route is not set, get it from the callable.
         if route is None:
@@ -446,6 +448,23 @@ def add_page(
         # Check if the route given is valid
         verify_route_validity(route)
 
+        if route in self.pages and os.getenv(constants.RELOAD_CONFIG):
+            # when the app is reloaded(typically for app harness tests), we should maintain
+            # the latest render function of a route.This applies typically to decorated pages
+            # since they are only added when app._compile is called.
+            self.pages.pop(route)
+
+        if route in self.pages:
+            route_name = (
+                f"`{route}` or `/`"
+                if route == constants.PageNames.INDEX_ROUTE
+                else f"`{route}`"
+            )
+            raise ValueError(
+                f"Duplicate page route {route_name} already exists. Make sure you do not have two"
+                f" pages with the same route"
+            )
+
         # Setup dynamic args for the route.
         # this state assignment is only required for tests using the deprecated state kwarg for App
         state = self.state if self.state else State
@@ -537,27 +556,67 @@ def _check_routes_conflict(self, new_route: str):
         Args:
             new_route: the route being newly added.
         """
-        newroute_catchall = catchall_in_route(new_route)
-        if not newroute_catchall:
-            return
-
-        for route in self.pages:
-            route = "" if route == "index" else route
 
-            if new_route.startswith(f"{route}/[[..."):
-                raise ValueError(
-                    f"You cannot define a route with the same specificity as a optional catch-all route ('{route}' and '{new_route}')"
-                )
+        def replace_brackets_with_keywords(input_string):
+            # /posts -> /post
+            # /posts/[slug] -> /posts/__SINGLE_SEGMENT__
+            # /posts/[slug]/comments -> /posts/__SINGLE_SEGMENT__/comments
+            # /posts/[[slug]] -> /posts/__DOUBLE_SEGMENT__
+            # / posts/[[...slug2]]-> /posts/__DOUBLE_CATCHALL_SEGMENT__
+            # /posts/[...slug3]-> /posts/__SINGLE_CATCHALL_SEGMENT__
+
+            # Replace [[...<slug>]] with __DOUBLE_CATCHALL_SEGMENT__
+            output_string = re.sub(
+                r"\[\[\.\.\..+?\]\]",
+                constants.RouteRegex.DOUBLE_CATCHALL_SEGMENT,
+                input_string,
+            )
+            # Replace [...<slug>] with __SINGLE_CATCHALL_SEGMENT__
+            output_string = re.sub(
+                r"\[\.\.\..+?\]",
+                constants.RouteRegex.SINGLE_CATCHALL_SEGMENT,
+                output_string,
+            )
+            # Replace [[<slug>]] with __DOUBLE_SEGMENT__
+            output_string = re.sub(
+                r"\[\[.+?\]\]", constants.RouteRegex.DOUBLE_SEGMENT, output_string
+            )
+            # Replace [<slug>] with __SINGLE_SEGMENT__
+            output_string = re.sub(
+                r"\[.+?\]", constants.RouteRegex.SINGLE_SEGMENT, output_string
+            )
+            return output_string
 
-            route_catchall = catchall_in_route(route)
-            if (
-                route_catchall
-                and newroute_catchall
-                and catchall_prefix(route) == catchall_prefix(new_route)
+        new_replaced_route = replace_brackets_with_keywords(new_route)
+        if (
+            constants.RouteRegex.SINGLE_SEGMENT not in new_replaced_route
+            and constants.RouteRegex.DOUBLE_SEGMENT not in new_replaced_route
+            and constants.RouteRegex.SINGLE_CATCHALL_SEGMENT not in new_replaced_route
+            and constants.RouteRegex.DOUBLE_CATCHALL_SEGMENT not in new_replaced_route
+        ):
+            return
+        segments = (
+            constants.RouteRegex.SINGLE_SEGMENT,
+            constants.RouteRegex.DOUBLE_SEGMENT,
+            constants.RouteRegex.SINGLE_CATCHALL_SEGMENT,
+            constants.RouteRegex.DOUBLE_CATCHALL_SEGMENT,
+        )
+        for route in self.pages:
+            replaced_route = replace_brackets_with_keywords(route)
+            for rw, r, nr in zip(
+                replaced_route.split("/"), route.split("/"), new_route.split("/")
             ):
-                raise ValueError(
-                    f"You cannot use multiple catchall for the same dynamic route ({route} !== {new_route})"
-                )
+                if rw in segments and r != nr:
+                    # If the slugs in the segments of both routes are not the same, then the route is invalid
+                    raise ValueError(
+                        f"You cannot use different slug names for the same dynamic path in  {route} and {new_route} ('{r}' != '{nr}')"
+                    )
+                elif rw not in segments and r != nr:
+                    # if the section being compared in both routes is not a dynamic segment(i.e not wrapped in brackets)
+                    # then we are guaranteed that the route is valid and there's no need checking the rest.
+                    # eg. /posts/[id]/info/[slug1] and /posts/[id]/info1/[slug1] is always going to be valid since
+                    # info1 will break away into its own tree.
+                    break
 
     def add_custom_404_page(
         self,

reflex/constants/route.py

@@ -44,6 +44,10 @@ class RouteRegex(SimpleNamespace):
     STRICT_CATCHALL = re.compile(r"\[\.{3}([a-zA-Z_][\w]*)\]")
     # group return the arg name (i.e. "slug") (optional arg can be empty)
     OPT_CATCHALL = re.compile(r"\[\[\.{3}([a-zA-Z_][\w]*)\]\]")
+    SINGLE_SEGMENT = "__SINGLE_SEGMENT__"
+    DOUBLE_SEGMENT = "__DOUBLE_SEGMENT__"
+    SINGLE_CATCHALL_SEGMENT = "__SINGLE_CATCHALL_SEGMENT__"
+    DOUBLE_CATCHALL_SEGMENT = "__DOUBLE_CATCHALL_SEGMENT__"
 
 
 class DefaultPage(SimpleNamespace):

tests/test_app.py

@@ -309,6 +309,39 @@ def test_add_page_invalid_api_route(app: App, index_page):
     app.add_page(index_page, route="/foo/api")
 
 
+def page1():
+    return rx.fragment()
+
+
+def page2():
+    return rx.fragment()
+
+
+def index():
+    return rx.fragment()
+
+
+@pytest.mark.parametrize(
+    "first_page,second_page, route",
+    [
+        (lambda: rx.fragment(), lambda: rx.fragment(rx.text("second")), "/"),
+        (rx.fragment(rx.text("first")), rx.fragment(rx.text("second")), "/page1"),
+        (
+            lambda: rx.fragment(rx.text("first")),
+            rx.fragment(rx.text("second")),
+            "page3",
+        ),
+        (page1, page2, "page1"),
+        (index, index, None),
+        (page1, page1, None),
+    ],
+)
+def test_add_duplicate_page_route_error(app, first_page, second_page, route):
+    app.add_page(first_page, route=route)
+    with pytest.raises(ValueError):
+        app.add_page(second_page, route="/" + route.strip("/") if route else None)
+
+
 def test_initialize_with_admin_dashboard(test_model):
     """Test setting the admin dashboard of an app.
 

tests/test_route.py

@@ -1,6 +1,7 @@
 import pytest
 
 from reflex import constants
+from reflex.app import App
 from reflex.route import catchall_in_route, get_route_args, verify_route_validity
 
 
@@ -69,3 +70,53 @@ def test_verify_valid_routes(route_name):
 def test_verify_invalid_routes(route_name):
     with pytest.raises(ValueError):
         verify_route_validity(route_name)
+
+
+@pytest.fixture()
+def app():
+    return App()
+
+
+@pytest.mark.parametrize(
+    "route1,route2",
+    [
+        ("/posts/[slug]", "/posts/[slug1]"),
+        ("/posts/[slug]/info", "/posts/[slug1]/info1"),
+        ("/posts/[slug]/info/[[slug1]]", "/posts/[slug1]/info1/[[slug2]]"),
+        ("/posts/[slug]/info/[[slug1]]", "/posts/[slug]/info/[[slug2]]"),
+        ("/posts/[slug]/info/[[...slug1]]", "/posts/[slug1]/info/[[...slug2]]"),
+        ("/posts/[slug]/info/[[...slug1]]", "/posts/[slug]/info/[[...slug2]]"),
+    ],
+)
+def test_check_routes_conflict_invalid(mocker, app, route1, route2):
+    mocker.patch.object(app, "pages", {route1: []})
+    with pytest.raises(ValueError):
+        app._check_routes_conflict(route2)
+
+
+@pytest.mark.parametrize(
+    "route1,route2",
+    [
+        ("/posts/[slug]", "/post/[slug1]"),
+        ("/posts/[slug]", "/post/[slug]"),
+        ("/posts/[slug]/info", "/posts/[slug]/info1"),
+        ("/posts/[slug]/info/[[slug1]]", "/posts/[slug]/info1/[[slug1]]"),
+        ("/posts/[slug]/info/[[slug1]]", "/posts/[slug]/info1/[[slug2]]"),
+        (
+            "/posts/[slug]/info/[slug2]/[[slug1]]",
+            "/posts/[slug]/info1/[slug2]/[[slug1]]",
+        ),
+        (
+            "/posts/[slug]/info/[slug1]/random1/[slug2]/x",
+            "/posts/[slug]/info/[slug1]/random/[slug4]/x1",
+        ),
+        ("/posts/[slug]/info/[[...slug1]]", "/posts/[slug]/info1/[[...slug1]]"),
+        ("/posts/[slug]/info/[[...slug1]]", "/posts/[slug]/info1/[[...slug2]]"),
+        ("/posts/[slug]/info/[...slug1]", "/posts/[slug]/info1/[...slug1]"),
+        ("/posts/[slug]/info/[...slug1]", "/posts/[slug]/info1/[...slug2]"),
+    ],
+)
+def test_check_routes_conflict_valid(mocker, app, route1, route2):
+    mocker.patch.object(app, "pages", {route1: []})
+    # test that running this does not throw an error.
+    app._check_routes_conflict(route2)