Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support URLRouter with include #2110

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions channels/routing.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import importlib
import re

from django.conf import settings
from django.core.exceptions import ImproperlyConfigured
from django.urls.exceptions import Resolver404
from django.urls.resolvers import RegexPattern, RoutePattern, URLResolver
from django.urls.resolvers import RegexPattern, RoutePattern, URLResolver, URLPattern

"""
All Routing instances inside this file are also valid ASGI applications - with
Expand Down Expand Up @@ -67,7 +68,32 @@ class URLRouter:
_path_routing = True

def __init__(self, routes):
self.routes = routes
new_routes = []
for route in routes:
if not route.callback and isinstance(route, URLResolver):
for url_pattern in route.url_patterns:
url_pattern: URLPattern
bigfootjon marked this conversation as resolved.
Show resolved Hide resolved
# concatenate parent's url and child's url
regex = "".join(
x.pattern
for x in [route.pattern.regex, url_pattern.pattern.regex]
)
regex = re.sub(r"(/)\1+", r"\1", regex)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help me understand the purpose of this line? Is it intended to handle the case where route.pattern.regex ends with a / AND url_pattern.pattern.regex starts with a /?

Copy link
Author

@jjjkkkjjj jjjkkkjjj Aug 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intended to handle the case where route.pattern.regex ends with a / AND url_pattern.pattern.regex starts with a /?

No, this is intended to remove the sequential '/'.
For example, when the url is '//s///s', the output will be '/s/s'

>>> re.sub(r"(/)\1+", r"\1", '///s//s') 
'/s/s'

I've left the comment about this by 7ca0037

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? Can users just configure their urlpatterns to avoid this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can avoid this when we configure the urlpatterns like this;

Case;

  • parent
urlpatterns = [
        path("universe/", include("__src.universe.routings"), name="universe"),
    ]
  • child
urlpatterns = [
        path("home/", test_app, name="home"),
    ]

This is intended for escaping the unnecessary /. This code allows us to configure the urlpatterns even if we configure them like this;

  • child
urlpatterns = [
        path("/home/", test_app, name="home"),
             ^----- here!
    ]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it for now by 758a205

name = (
f"{route.app_name}:{url_pattern.name}"
if url_pattern.name
else None
)
pattern = RegexPattern(regex, name=name, is_endpoint=True)
new_routes.append(
URLPattern(
pattern, url_pattern.callback, route.default_kwargs, name
bigfootjon marked this conversation as resolved.
Show resolved Hide resolved
)
)
else:
new_routes.append(route)

self.routes = new_routes

for route in self.routes:
# The inner ASGI app wants to do additional routing, route
Expand Down
64 changes: 58 additions & 6 deletions tests/test_routing.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import pytest
from django.core.exceptions import ImproperlyConfigured
from django.urls import path, re_path

from channels.routing import ChannelNameRouter, ProtocolTypeRouter, URLRouter
Expand Down Expand Up @@ -302,13 +301,66 @@ async def test_path_remaining():
)


def test_invalid_routes():
@pytest.mark.asyncio
async def test_url_router_nesting_by_include():
"""
Test URLRouter route validation
Tests that nested URLRouters is constructed by include function.
"""
import sys
from django.urls import include

with pytest.raises(ImproperlyConfigured) as exc:
URLRouter([path("", include([]))])
test_app = MockApplication(return_value=1)

assert "include() is not supported in URLRouter." in str(exc)
# mocking the universe module following the directory structure;
# ├── universe
# │ └── routings.py
# └── routings.py (parent)
#
#
# in routings.py
# ======================
# ...
# urlpatterns = [
# re_path(r"book/(?P<book>[\w\-]+)/page/(?P<page>\d+)/$", test_app),
# re_path(r"test/(\d+)/$", test_app),
# ]
# ======================
module_routings = type(sys)("routings")
module_routings.urlpatterns = [
re_path(r"book/(?P<book>[\w\-]+)/page/(?P<page>\d+)/$", test_app),
re_path(r"test/(\d+)/$", test_app),
bigfootjon marked this conversation as resolved.
Show resolved Hide resolved
]
module = type(sys)("universe")
module.routings = module_routings
sys.modules["universe"] = module
sys.modules["universe.routings"] = module.routings

# parent routings.py
outer_router = URLRouter(
[
path("universe/", include("universe.routings"), name="universe"),
bigfootjon marked this conversation as resolved.
Show resolved Hide resolved
]
)
assert (
await outer_router(
{
"type": "http",
"path": "/universe/book/channels-guide/page/10/",
},
None,
None,
)
== 1
)

assert (
await outer_router(
{
"type": "http",
"path": "/universe/test/10/",
},
None,
None,
)
== 1
)
Loading