From e1d872d97b083ea9f8cb961ab79e39a903ad9855 Mon Sep 17 00:00:00 2001 From: Elijah Date: Wed, 24 Apr 2024 13:36:57 +0000 Subject: [PATCH 1/4] Throw Errors for duplicate Routes --- reflex/app.py | 11 +++++++++++ tests/test_app.py | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/reflex/app.py b/reflex/app.py index c47077d0ef..ed9783e463 100644 --- a/reflex/app.py +++ b/reflex/app.py @@ -446,6 +446,17 @@ def add_page( # Check if the route given is valid verify_route_validity(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 diff --git a/tests/test_app.py b/tests/test_app.py index 1e1df01bff..995ac976bd 100644 --- a/tests/test_app.py +++ b/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. From 03fd350c316fc6a78310fd28682d115348691ead Mon Sep 17 00:00:00 2001 From: Elijah Date: Fri, 26 Apr 2024 12:47:58 +0000 Subject: [PATCH 2/4] add support for dynamic routes --- reflex/app.py | 79 +++++++++++++++++++++++++++++---------- reflex/constants/route.py | 4 ++ tests/test_route.py | 51 +++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 20 deletions(-) diff --git a/reflex/app.py b/reflex/app.py index ed9783e463..15a41062ae 100644 --- a/reflex/app.py +++ b/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, ) @@ -548,27 +547,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 [[...]] 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 - 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, diff --git a/reflex/constants/route.py b/reflex/constants/route.py index aafea9c494..2af2f33c69 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/tests/test_route.py b/tests/test_route.py index 782e54218b..851c9cf359 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) From 209e10c4453a95aecc1b4467d905f015e1799a1e Mon Sep 17 00:00:00 2001 From: Elijah Date: Sun, 28 Apr 2024 15:30:33 +0000 Subject: [PATCH 3/4] darglint error fix --- reflex/app.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/reflex/app.py b/reflex/app.py index 15a41062ae..2544702876 100644 --- a/reflex/app.py +++ b/reflex/app.py @@ -431,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: @@ -445,6 +448,12 @@ 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 `/`" From 5ccf11146207f0ca1c8a8624afe2b0a6f8c57422 Mon Sep 17 00:00:00 2001 From: Elijah Date: Thu, 2 May 2024 14:40:50 +0000 Subject: [PATCH 4/4] address pr comment --- reflex/app.py | 42 +++--------------------------------------- reflex/route.py | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 39 deletions(-) diff --git a/reflex/app.py b/reflex/app.py index 2544702876..ad965341ec 100644 --- a/reflex/app.py +++ b/reflex/app.py @@ -11,7 +11,6 @@ import multiprocessing import os import platform -import re from typing import ( Any, AsyncIterator, @@ -65,6 +64,7 @@ ) from reflex.route import ( get_route_args, + replace_brackets_with_keywords, verify_route_validity, ) from reflex.state import ( @@ -556,45 +556,9 @@ def _check_routes_conflict(self, new_route: str): Args: new_route: the route being newly added. """ - - 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 [[...]] 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 - - 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 - ): + if "[" not in new_route: return + segments = ( constants.RouteRegex.SINGLE_SEGMENT, constants.RouteRegex.DOUBLE_SEGMENT, diff --git a/reflex/route.py b/reflex/route.py index 3f50eb96e3..0b71728248 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