From 9c7dbdbc728c34611fdd9328a8daa942c2c68cb8 Mon Sep 17 00:00:00 2001 From: Elijah Ahianyo Date: Fri, 3 May 2024 12:15:40 -0700 Subject: [PATCH] [REF-2643] Throw Errors for duplicate Routes (#3155) --- reflex/app.py | 61 +++++++++++++++++++++++++++------------ reflex/constants/route.py | 4 +++ reflex/route.py | 39 +++++++++++++++++++++++++ tests/test_app.py | 33 +++++++++++++++++++++ tests/test_route.py | 51 ++++++++++++++++++++++++++++++++ 5 files changed, 169 insertions(+), 19 deletions(-) diff --git a/reflex/app.py b/reflex/app.py index 677f84aa7..754541545 100644 --- a/reflex/app.py +++ b/reflex/app.py @@ -63,9 +63,8 @@ from reflex.page import ( DECORATED_PAGES, ) from reflex.route import ( - catchall_in_route, - catchall_prefix, get_route_args, + replace_brackets_with_keywords, verify_route_validity, ) from reflex.state import ( @@ -456,6 +455,9 @@ class App(Base): 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: @@ -470,6 +472,23 @@ class App(Base): # 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 @@ -561,27 +580,31 @@ class App(Base): Args: new_route: the route being newly added. """ - newroute_catchall = catchall_in_route(new_route) - if not newroute_catchall: + if "[" not in new_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: - 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}')" - ) - - route_catchall = catchall_in_route(route) - if ( - route_catchall - and newroute_catchall - and catchall_prefix(route) == catchall_prefix(new_route) + 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, diff --git a/reflex/constants/route.py b/reflex/constants/route.py index aafea9c49..2af2f33c6 100644 --- a/reflex/constants/route.py +++ b/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): diff --git a/reflex/route.py b/reflex/route.py index 3f50eb96e..0b7172824 100644 --- a/reflex/route.py +++ b/reflex/route.py @@ -101,3 +101,42 @@ def catchall_prefix(route: str) -> str: """ pattern = catchall_in_route(route) return route.replace(pattern, "") if pattern else "" + + +def replace_brackets_with_keywords(input_string): + """Replace brackets and everything inside it in a string with a keyword. + + Args: + input_string: String to replace. + + Returns: + new string containing keywords. + """ + # /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 [[...]] with __DOUBLE_CATCHALL_SEGMENT__ + output_string = re.sub( + r"\[\[\.\.\..+?\]\]", + constants.RouteRegex.DOUBLE_CATCHALL_SEGMENT, + input_string, + ) + # Replace [...] with __SINGLE_CATCHALL_SEGMENT__ + output_string = re.sub( + r"\[\.\.\..+?\]", + constants.RouteRegex.SINGLE_CATCHALL_SEGMENT, + output_string, + ) + # Replace [[]] with __DOUBLE_SEGMENT__ + output_string = re.sub( + r"\[\[.+?\]\]", constants.RouteRegex.DOUBLE_SEGMENT, output_string + ) + # Replace [] with __SINGLE_SEGMENT__ + output_string = re.sub( + r"\[.+?\]", constants.RouteRegex.SINGLE_SEGMENT, output_string + ) + return output_string diff --git a/tests/test_app.py b/tests/test_app.py index 6e8939059..0e204c7d5 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -310,6 +310,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. diff --git a/tests/test_route.py b/tests/test_route.py index 782e54218..851c9cf35 100644 --- a/tests/test_route.py +++ b/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)