Skip to content

Commit 9c7dbdb

Browse files
ElijahAhianyoElijahAhianyo
authored
[REF-2643] Throw Errors for duplicate Routes (#3155)
1 parent 24d15ac commit 9c7dbdb

5 files changed

Lines changed: 169 additions & 19 deletions

File tree

‎reflex/app.py‎

Lines changed: 42 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,8 @@
6363
DECORATED_PAGES,
6464
)
6565
from reflex.route import (
66-
catchall_in_route,
67-
catchall_prefix,
6866
get_route_args,
67+
replace_brackets_with_keywords,
6968
verify_route_validity,
7069
)
7170
from reflex.state import (
@@ -456,6 +455,9 @@ def add_page(
456455
on_load: The event handler(s) that will be called each time the page load.
457456
meta: The metadata of the page.
458457
script_tags: List of script tags to be added to component
458+
459+
Raises:
460+
ValueError: When the specified route name already exists.
459461
"""
460462
# If the route is not set, get it from the callable.
461463
if route is None:
@@ -470,6 +472,23 @@ def add_page(
470472
# Check if the route given is valid
471473
verify_route_validity(route)
472474

475+
if route in self.pages and os.getenv(constants.RELOAD_CONFIG):
476+
# when the app is reloaded(typically for app harness tests), we should maintain
477+
# the latest render function of a route.This applies typically to decorated pages
478+
# since they are only added when app._compile is called.
479+
self.pages.pop(route)
480+
481+
if route in self.pages:
482+
route_name = (
483+
f"`{route}` or `/`"
484+
if route == constants.PageNames.INDEX_ROUTE
485+
else f"`{route}`"
486+
)
487+
raise ValueError(
488+
f"Duplicate page route {route_name} already exists. Make sure you do not have two"
489+
f" pages with the same route"
490+
)
491+
473492
# Setup dynamic args for the route.
474493
# this state assignment is only required for tests using the deprecated state kwarg for App
475494
state = self.state if self.state else State
@@ -561,27 +580,31 @@ def _check_routes_conflict(self, new_route: str):
561580
Args:
562581
new_route: the route being newly added.
563582
"""
564-
newroute_catchall = catchall_in_route(new_route)
565-
if not newroute_catchall:
583+
if "[" not in new_route:
566584
return
567585

586+
segments = (
587+
constants.RouteRegex.SINGLE_SEGMENT,
588+
constants.RouteRegex.DOUBLE_SEGMENT,
589+
constants.RouteRegex.SINGLE_CATCHALL_SEGMENT,
590+
constants.RouteRegex.DOUBLE_CATCHALL_SEGMENT,
591+
)
568592
for route in self.pages:
569-
route = "" if route == "index" else route
570-
571-
if new_route.startswith(f"{route}/[[..."):
572-
raise ValueError(
573-
f"You cannot define a route with the same specificity as a optional catch-all route ('{route}' and '{new_route}')"
574-
)
575-
576-
route_catchall = catchall_in_route(route)
577-
if (
578-
route_catchall
579-
and newroute_catchall
580-
and catchall_prefix(route) == catchall_prefix(new_route)
593+
replaced_route = replace_brackets_with_keywords(route)
594+
for rw, r, nr in zip(
595+
replaced_route.split("/"), route.split("/"), new_route.split("/")
581596
):
582-
raise ValueError(
583-
f"You cannot use multiple catchall for the same dynamic route ({route} !== {new_route})"
584-
)
597+
if rw in segments and r != nr:
598+
# If the slugs in the segments of both routes are not the same, then the route is invalid
599+
raise ValueError(
600+
f"You cannot use different slug names for the same dynamic path in {route} and {new_route} ('{r}' != '{nr}')"
601+
)
602+
elif rw not in segments and r != nr:
603+
# if the section being compared in both routes is not a dynamic segment(i.e not wrapped in brackets)
604+
# then we are guaranteed that the route is valid and there's no need checking the rest.
605+
# eg. /posts/[id]/info/[slug1] and /posts/[id]/info1/[slug1] is always going to be valid since
606+
# info1 will break away into its own tree.
607+
break
585608

586609
def add_custom_404_page(
587610
self,

‎reflex/constants/route.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,10 @@ class RouteRegex(SimpleNamespace):
4444
STRICT_CATCHALL = re.compile(r"\[\.{3}([a-zA-Z_][\w]*)\]")
4545
# group return the arg name (i.e. "slug") (optional arg can be empty)
4646
OPT_CATCHALL = re.compile(r"\[\[\.{3}([a-zA-Z_][\w]*)\]\]")
47+
SINGLE_SEGMENT = "__SINGLE_SEGMENT__"
48+
DOUBLE_SEGMENT = "__DOUBLE_SEGMENT__"
49+
SINGLE_CATCHALL_SEGMENT = "__SINGLE_CATCHALL_SEGMENT__"
50+
DOUBLE_CATCHALL_SEGMENT = "__DOUBLE_CATCHALL_SEGMENT__"
4751

4852

4953
class DefaultPage(SimpleNamespace):

‎reflex/route.py‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,3 +101,42 @@ def catchall_prefix(route: str) -> str:
101101
"""
102102
pattern = catchall_in_route(route)
103103
return route.replace(pattern, "") if pattern else ""
104+
105+
106+
def replace_brackets_with_keywords(input_string):
107+
"""Replace brackets and everything inside it in a string with a keyword.
108+
109+
Args:
110+
input_string: String to replace.
111+
112+
Returns:
113+
new string containing keywords.
114+
"""
115+
# /posts -> /post
116+
# /posts/[slug] -> /posts/__SINGLE_SEGMENT__
117+
# /posts/[slug]/comments -> /posts/__SINGLE_SEGMENT__/comments
118+
# /posts/[[slug]] -> /posts/__DOUBLE_SEGMENT__
119+
# / posts/[[...slug2]]-> /posts/__DOUBLE_CATCHALL_SEGMENT__
120+
# /posts/[...slug3]-> /posts/__SINGLE_CATCHALL_SEGMENT__
121+
122+
# Replace [[...<slug>]] with __DOUBLE_CATCHALL_SEGMENT__
123+
output_string = re.sub(
124+
r"\[\[\.\.\..+?\]\]",
125+
constants.RouteRegex.DOUBLE_CATCHALL_SEGMENT,
126+
input_string,
127+
)
128+
# Replace [...<slug>] with __SINGLE_CATCHALL_SEGMENT__
129+
output_string = re.sub(
130+
r"\[\.\.\..+?\]",
131+
constants.RouteRegex.SINGLE_CATCHALL_SEGMENT,
132+
output_string,
133+
)
134+
# Replace [[<slug>]] with __DOUBLE_SEGMENT__
135+
output_string = re.sub(
136+
r"\[\[.+?\]\]", constants.RouteRegex.DOUBLE_SEGMENT, output_string
137+
)
138+
# Replace [<slug>] with __SINGLE_SEGMENT__
139+
output_string = re.sub(
140+
r"\[.+?\]", constants.RouteRegex.SINGLE_SEGMENT, output_string
141+
)
142+
return output_string

‎tests/test_app.py‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,39 @@ def test_add_page_invalid_api_route(app: App, index_page):
310310
app.add_page(index_page, route="/foo/api")
311311

312312

313+
def page1():
314+
return rx.fragment()
315+
316+
317+
def page2():
318+
return rx.fragment()
319+
320+
321+
def index():
322+
return rx.fragment()
323+
324+
325+
@pytest.mark.parametrize(
326+
"first_page,second_page, route",
327+
[
328+
(lambda: rx.fragment(), lambda: rx.fragment(rx.text("second")), "/"),
329+
(rx.fragment(rx.text("first")), rx.fragment(rx.text("second")), "/page1"),
330+
(
331+
lambda: rx.fragment(rx.text("first")),
332+
rx.fragment(rx.text("second")),
333+
"page3",
334+
),
335+
(page1, page2, "page1"),
336+
(index, index, None),
337+
(page1, page1, None),
338+
],
339+
)
340+
def test_add_duplicate_page_route_error(app, first_page, second_page, route):
341+
app.add_page(first_page, route=route)
342+
with pytest.raises(ValueError):
343+
app.add_page(second_page, route="/" + route.strip("/") if route else None)
344+
345+
313346
def test_initialize_with_admin_dashboard(test_model):
314347
"""Test setting the admin dashboard of an app.
315348

‎tests/test_route.py‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import pytest
22

33
from reflex import constants
4+
from reflex.app import App
45
from reflex.route import catchall_in_route, get_route_args, verify_route_validity
56

67

@@ -69,3 +70,53 @@ def test_verify_valid_routes(route_name):
6970
def test_verify_invalid_routes(route_name):
7071
with pytest.raises(ValueError):
7172
verify_route_validity(route_name)
73+
74+
75+
@pytest.fixture()
76+
def app():
77+
return App()
78+
79+
80+
@pytest.mark.parametrize(
81+
"route1,route2",
82+
[
83+
("/posts/[slug]", "/posts/[slug1]"),
84+
("/posts/[slug]/info", "/posts/[slug1]/info1"),
85+
("/posts/[slug]/info/[[slug1]]", "/posts/[slug1]/info1/[[slug2]]"),
86+
("/posts/[slug]/info/[[slug1]]", "/posts/[slug]/info/[[slug2]]"),
87+
("/posts/[slug]/info/[[...slug1]]", "/posts/[slug1]/info/[[...slug2]]"),
88+
("/posts/[slug]/info/[[...slug1]]", "/posts/[slug]/info/[[...slug2]]"),
89+
],
90+
)
91+
def test_check_routes_conflict_invalid(mocker, app, route1, route2):
92+
mocker.patch.object(app, "pages", {route1: []})
93+
with pytest.raises(ValueError):
94+
app._check_routes_conflict(route2)
95+
96+
97+
@pytest.mark.parametrize(
98+
"route1,route2",
99+
[
100+
("/posts/[slug]", "/post/[slug1]"),
101+
("/posts/[slug]", "/post/[slug]"),
102+
("/posts/[slug]/info", "/posts/[slug]/info1"),
103+
("/posts/[slug]/info/[[slug1]]", "/posts/[slug]/info1/[[slug1]]"),
104+
("/posts/[slug]/info/[[slug1]]", "/posts/[slug]/info1/[[slug2]]"),
105+
(
106+
"/posts/[slug]/info/[slug2]/[[slug1]]",
107+
"/posts/[slug]/info1/[slug2]/[[slug1]]",
108+
),
109+
(
110+
"/posts/[slug]/info/[slug1]/random1/[slug2]/x",
111+
"/posts/[slug]/info/[slug1]/random/[slug4]/x1",
112+
),
113+
("/posts/[slug]/info/[[...slug1]]", "/posts/[slug]/info1/[[...slug1]]"),
114+
("/posts/[slug]/info/[[...slug1]]", "/posts/[slug]/info1/[[...slug2]]"),
115+
("/posts/[slug]/info/[...slug1]", "/posts/[slug]/info1/[...slug1]"),
116+
("/posts/[slug]/info/[...slug1]", "/posts/[slug]/info1/[...slug2]"),
117+
],
118+
)
119+
def test_check_routes_conflict_valid(mocker, app, route1, route2):
120+
mocker.patch.object(app, "pages", {route1: []})
121+
# test that running this does not throw an error.
122+
app._check_routes_conflict(route2)

0 commit comments

Comments
 (0)