Support URLRouter with include #2110

jjjkkkjjj · 2024-06-23T13:11:29Z

I re-created a new PR (#2037)

I tried to implement to support URLRouter with include.
This will be helpful project structure organized well.

Usage:

In parent's routings.py;

urlpatterns = [
    path('chats/', include('pj.chats.routings'), name='chats'),
]

In child's routings.py;

app_name = 'chats'

urlpatterns = [
    re_path(r"/(?P<room_name>\w+)/chat/$", ChatConsumer.as_asgi()),
]

Also, I've implemented the unittest for it in test_url_router_nesting_by_include in tests/test_routing.py.

bigfootjon · 2024-07-10T01:09:02Z

Would you mind fixing the lint errors?

jjjkkkjjj · 2024-07-10T14:10:19Z

I've modified codes by ruff. Unfortunately, I coundn't run linter (tox -e qa) on my windows PC due to NameError: name 'ssl' is not defined.
Could you check my modified codes again?

bigfootjon · 2024-07-11T03:39:09Z

Hey @jjjkkkjjj. I've done some more reading and thinking on this topic and I'm not sure this is correct.

Per the docs:

Note that you cannot use the Django include function inside of the URLRouter as it assumes a bit too much about what it is given as its left-hand side and will terminate your regular expression/URL pattern wrongly.

https://channels.readthedocs.io/en/latest/releases/2.1.0.html#nested-url-routing

And per this stack overflow question there is a clean (and channels native) way to do this:

https://stackoverflow.com/questions/56239070/how-can-i-nest-urlrouter-in-django-channels

I think that we won't move forward with this PR since an alternative, preferred technique exists.

jjjkkkjjj · 2024-07-11T07:28:34Z

it is given as its left-hand side and will terminate your regular expression/URL pattern wrongly.

Yes, that's why I modified it in this PR. Actually, the unit test has passed.

I think the workaround solution is valid too, but my PR has a merit that can use reverse function. It is desirable that channels' core routing function conforms to django as possible, I think.

But I don't understand django's url routing system completely. When you judge this PR can't be accepted, I'll close this PR.

bigfootjon

but my PR has a merit that can use reverse function.

Oh interesting, I didn't realize the current solution doesn't allow reverse. Good call out!

Alright, I think this is a good idea then.

Can you add some test cases for reverse?

And it looks like there are still lint errors: https://github.com/django/channels/actions/runs/9875832061/job/27301864730?pr=2110

(and see my other inline questions/requests, this PR is looking pretty reasonable. We just need to tighten up a few things)

bigfootjon · 2024-07-21T00:55:21Z

channels/routing.py

+                        x.pattern
+                        for x in [route.pattern.regex, url_pattern.pattern.regex]
+                    )
+                    regex = re.sub(r"(/)\1+", r"\1", regex)


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 /?

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

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

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! ]

I removed it for now by 758a205

channels/routing.py

tests/test_routing.py

channels/routing.py

jjjkkkjjj · 2024-08-05T04:35:38Z

@bigfootjon
Sorry for late response... (I had no time to work on it)

But, I could fix some bugs and implemented reverse function!
When you accept this PR, you can do like this as if you can use routing system like original django's one.

src/routings.py (root)

...

urlpatterns = [
    path("universe/", include("src.universe.routings"), name="universe"),
    path("moon/", test_app, name="moon"),
    re_path(r"mars/(\d+)/$", test_app, name="mars"),
]

outer_router = URLRouter(urlpatterns)

src/universe/routings.py

...

app_name = "universe"

urlpatterns = [
    re_path(r"book/(?P<book>[\w\-]+)/page/(?P<page>\d+)/$", test_app),     
    re_path(r"test/(\d+)/$", test_app),
    path("/home/", test_app),
]

Note: Django's routing system parses the urlpatterns as global variable from root directory specified by urlconf argument in reverse function.

And then, you can use original reverse function or the wrapper reverse function I implemented like this.

from django.urls import reverse as django_reverse
from channels.routing import reverse

...

django_reverse("mars", urlconf=root_urlconf, args=(5,)) # "/mars/5/"
reverse("mars", args=(5,)) # "/mars/5/"

django_reverse(
    "universe:book",
    urlconf=root_urlconf,
    kwargs=dict(book="channels-guide", page=10),
) # "/universe/book/channels-guide/page/10/"
reverse("universe:book", kwargs=dict(book="channels-guide", page=10)) # "/universe/book/channels-guide/page/10/"

This has been confirmed by test_url_router_nesting_by_include in unittest

bigfootjon

Getting closer!

Please see my comments and fix the lint warnings

channels/routing.py

bigfootjon · 2024-08-09T03:15:00Z

channels/routing.py

+def reverse(*args, urlconf=None, **kwargs):
+    """reverse wrapper for django's reverse function
+
+    Parameters
+    ----------
+    urlconf : str, optional
+        The root path of the routings, by default None
+
+    See the django's [reverse](https://docs.djangoproject.com/en/5.0/ref/urlresolvers/#reverse)
+    for more details of the other arguments
+
+    Returns
+    -------
+    str
+        The reversed url
+    """
+    if urlconf is None:
+        urlconf = settings.ROOT_WEBSOCKET_URLCONF
+    return django_reverse(*args, urlconf=urlconf, **kwargs)


I think this is a change that deserves its own discussion. I don't think we should introduce a new ROOT_WEBSOCKET_URLCONF setting. Please revert the addition of this function.

I want to discuss this.
When you don't set the ROOT_WEBSOCKET_URLCONF, this reverse function will be limited. Users must specify the urlconf as the parent routings.py every time.

Why every time?

settings.ROOT_URLCONF should be configured in the original django's routing system. And actually, django's reverse function refers to ROOT_URLCONF here

https://github.com/django/django/blob/f16a9a556fb4225f9094048614f4fcec3db7e067/django/urls/base.py#L30

def resolve(path, urlconf=None): if urlconf is None: urlconf = get_urlconf() return get_resolver(urlconf).resolve(path) def reverse(viewname, urlconf=None, args=None, kwargs=None, current_app=None): if urlconf is None: urlconf = get_urlconf() resolver = get_resolver(urlconf)

get_resolver function is below;

https://github.com/django/django/blob/f16a9a556fb4225f9094048614f4fcec3db7e067/django/urls/resolvers.py#L108

def get_resolver(urlconf=None): if urlconf is None: urlconf = settings.ROOT_URLCONF return _get_cached_resolver(urlconf)

This means original django's routing system parses all configured urlpatterns from ROOT_URLCONF. That's why we can use reverse function.

Example;

src ├── app1 │ ├── urls.py │ └── routings.py ├── urls.py (root) └── routings.py (root)

In settings.py

ROOT_URLCONF = 'src.urls' # this is entrypoint to parse all urls

That's why I imitated this implementation to use reverse. If we want to use reverse for channels, we should configure the "entrypoint" to parse all urls of channels.

I totally see where you're coming from, but I don't want to mix in those new changes into this PR. I see this as 2 separate issues:

Support nested urlpatterns

Enable reverse

This PR is (1). (2) requires more discussion and design, so I'd like to make that a separate PR. Do you mind splitting them up so we can discuss (2) in greater depth?

jjjkkkjjj · 2024-08-09T15:39:20Z

Thanks! I left the comments. Please check them!

And I'll fix linter errors from now.

Also, I summarize that this PR's merit again. This will be helpful for the documentation.

more clear routing system in channels
Allow to use include, reverse (channels wraper)
Future's features

more clear routing system in channels (goodbye nested URLRouter)

When you configure ROOT_WEBSOCKET_URLCONF , channels users can implement almost same as the original django's routing system.

src
├── app1
│   ├── urls.py
│   └── routings.py
├── urls.py (root)
└── routings.py (root)

In settings.py

ROOT_URLCONF = 'src.urls'
ROOT_WEBSOCKET_URLCONF = 'src.routings'

In parent urls.py

urlpatterns = [
    path("app1/", include("src.app1.urls"), name="app1"),
    path("home/", test_app, name="home"),
]

In parent routings.py

urlpatterns = [
    path("app1/", include("src.app1.routings"), name="app1"),
    path("home-ws/", test_app, name="home-ws"),
]

def get_websocket_application():
     return URLRouter(urlpatterns)

Almost same!

In child app1/urls.py

app_name = 'app1'

urlpatterns = [
    re_path(r"news/(\d+)/$", test_app, name="news"),
]

In child app1/routings.py

app_name = 'app1'

urlpatterns = [
    re_path(r"chats/(\d+)/$", test_app, name="chats"),
]

Almost same! We don't need many URLRouter like this.

And then we can reverse

Allow to use `include`, `reverse` (channels wraper)

from django.urls import reverse as django_reverse
from channels.routing import reverse

django_reverse("app1:news", args=(5,)) # "/app1/news/5/"
reverse("app1:chats", args=(5,)) # "/app1/chats/5/"

Future's features

This is generally speaking. If the channels' code conforms to the original django, it will be easy to implement some features.

bigfootjon

All I see left is:

Removing reverse from this PR (submitting another PR is fine, I just don't want this PR to grow too much)
Fixing lints (I think that's just running black on the listed file)

jjjkkkjjj · 2024-08-10T02:54:54Z

@bigfootjon

I removed new reverse function for now
(Maybe) I fixed linter error

Please check them again!

jjjkkkjjj · 2024-08-10T03:27:01Z

I sorted modules.
Please check them again...

Memo:
I could check the formatting by the below command directly instead of tox -e qa

flake8 channels tests
black --check channels tests
isort --check-only --diff channels tests

sevdog · 2024-08-16T09:05:35Z

Just a simple question about the tests: what it the advantage of altering the sys.modules property instead of having a file which contains such code?

Django unittests for routing does not use such pattern. IMO having files would be easier to read and will not encourage a potentially dangarous pattern (altering sys.modules).

jjjkkkjjj · 2024-08-18T10:56:50Z

@sevdog
Actually I avoid creating many new files for this test only. This advantage is mocking the routing pattern in the test code instead of creating the actual file
I agree with your opinion that this is dangerous

I implemented the rollback function in tests/conftest.py to avoid this danger for now.

bigfootjon · 2024-08-24T17:49:16Z

Sorry for the delay, yeah I agree with @sevdog that we should use files instead for the tests here.

Also, @carltongibson when you get a chance could you look over this PR and give your thoughts?

carltongibson · 2024-08-27T07:04:36Z

@jjjkkkjjj I've begun looking at this. Can I ask you to add a draft at least for a release note and docs changes that would go with this please? (I see what you're proposing, but seeing docs clarify that. Thanks)

(Also, I share the scepticism about the sys.modules munging. An actual routing file is going to be easier to read no...)

jjjkkkjjj · 2024-08-29T00:18:04Z

@carltongibson
OK! Wait for a moment.

jjjkkkjjj · 2024-08-29T00:19:31Z

And then, should I use files instead of sys.modules?

carltongibson · 2024-08-29T07:17:54Z

@jjjkkkjjj hold off on that last. If you could add the drafts for the docs (don't have to be perfect) I can think it through properly

jjjkkkjjj · 2024-09-06T09:45:20Z

I'll work on adding the drafts by next week!
Sorry for late response and inconvenience.

jjjkkkjjj · 2024-09-19T13:28:20Z

@carltongibson @bigfootjon
Sorry for late response. I added the drafts in this commit (a303572). Please take a look!

carltongibson · 2024-09-19T13:51:07Z

Thanks @jjjkkkjjj, that's great. Let me have a look 👀

carltongibson

Hey @jjjkkkjjj.

I managed to get a look at it. 🤹 — I don't dislike it 🙂

I left some initial comments. Could you address, and then I take another look?
(Specifically, the autouse on the fixture bothers me, as I can't see what's going on. If you can remove that, it'll be clearer.)

docs/topics/routing.rst

carltongibson · 2024-10-03T15:31:59Z

tests/conftest.py

@@ -38,3 +40,14 @@ def samesite(request, settings):
 def samesite_invalid(settings):
    """Set samesite flag to strict."""
    settings.SESSION_COOKIE_SAMESITE = "Hello"
+
+
+@pytest.fixture(autouse=True)


I'm not keen on the autouse. I can't see just from the code whether or when it's in play.

This one is for rolling back the changes by sys.module.

And then, should I use files instead of sys.modules?

@jjjkkkjjj hold off on that last. If you could add the drafts for the docs (don't have to be perfect) I can think it through properly

I ask you again.
Should I create the test files instead of using sys.module?

channels/routing.py

carltongibson · 2024-10-03T15:33:30Z

channels/routing.py

+
+    Parameters
+    ----------
+    child_url_pattern : URLResolver |


Why the trailing |?

Ah, I forgot to append to Any.
This function receives the url_pattern returned by path, re_path (Any) and include (URLResolver)

jjjkkkjjj · 2024-10-08T13:15:59Z

@carltongibson
I modified them on your reviews!
Could you check what should I do (Create files or use sys.module) in test again?

carltongibson · 2024-10-08T13:24:56Z

@jjjkkkjjj Great thanks. Let me have another look. I will dig into those tests properly now. 👍

jjjkkkjjj and others added 5 commits August 8, 2023 01:58

support URLRouter with include

cc4a598

Merge branch 'django:main' into main

98de0a2

fixed indentation error

5309e68

remove test_invalid_routes

b9b0c76

added test_url_router_nesting_by_include in test_rouiting.py and passed

436d38a

fixed lint errors

4b47981

Merge branch 'django:main' into main

1890cd8

bigfootjon requested changes Jul 21, 2024

View reviewed changes

jjjkkkjjj and others added 9 commits August 3, 2024 12:45

Merge branch 'django:main' into main

9d4feeb

modified URLPattern's arguments (django#2110)

504bd3c

removed type hint (django#2110)

76260f7

add comment for regex (django#2110)

7ca0037

add path in test (django#2110)

4df1d5f

formatting (django#2110)

cdc8c6c

support deeper include (django#2110)

e35c2d5

add usual case with include (django#2110)

e3e411f

add reverse (django#2110)

3376bb2

bigfootjon requested changes Aug 9, 2024

View reviewed changes

jjjkkkjjj added 3 commits August 10, 2024 00:43

fixed linter errors (django#2110)

88ddd77

removed handiling the sequential / (django#2110)

758a205

removed not route.callback (django#2110)

dd4b1aa

bigfootjon requested changes Aug 10, 2024

View reviewed changes

jjjkkkjjj added 2 commits August 10, 2024 11:51

fix linter error (django#2110)

29c7402

removed reverse function (django#2110)

74e5ee2

jjjkkkjjj added 2 commits August 10, 2024 11:57

removed root_urlconf in conftest (django#2110)

05a869a

sorted by isort (django#2110)

6773674

add drafts for docs django#2110

a303572

carltongibson reviewed Oct 3, 2024

View reviewed changes

modified routing.rst (django#2110)

c2b5ee0

jjjkkkjjj pushed a commit to jjjkkkjjj/channels that referenced this pull request Oct 8, 2024

modified routing.py (django#2110)

08eb7b6

jjjkkkjjj force-pushed the main branch from 08eb7b6 to 67d5240 Compare October 8, 2024 13:05

modified routing.py (django#2110)

67d5240

carltongibson self-requested a review October 8, 2024 13:25

Support URLRouter with include #2110

Are you sure you want to change the base?

Support URLRouter with include #2110

Conversation

jjjkkkjjj commented Jun 23, 2024

bigfootjon commented Jul 10, 2024

jjjkkkjjj commented Jul 10, 2024 • edited Loading

bigfootjon commented Jul 11, 2024

jjjkkkjjj commented Jul 11, 2024 • edited Loading

bigfootjon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjjkkkjjj Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjjkkkjjj commented Aug 5, 2024 • edited Loading

bigfootjon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjjkkkjjj Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjjkkkjjj commented Aug 9, 2024

more clear routing system in channels (goodbye nested URLRouter)

Allow to use include, reverse (channels wraper)

Future's features

bigfootjon left a comment

Choose a reason for hiding this comment

jjjkkkjjj commented Aug 10, 2024

jjjkkkjjj commented Aug 10, 2024 • edited Loading

sevdog commented Aug 16, 2024

jjjkkkjjj commented Aug 18, 2024 • edited Loading

bigfootjon commented Aug 24, 2024

carltongibson commented Aug 27, 2024 • edited Loading

jjjkkkjjj commented Aug 29, 2024

jjjkkkjjj commented Aug 29, 2024

carltongibson commented Aug 29, 2024

jjjkkkjjj commented Sep 6, 2024

jjjkkkjjj commented Sep 19, 2024

carltongibson commented Sep 19, 2024

carltongibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjjkkkjjj Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

jjjkkkjjj commented Oct 8, 2024

carltongibson commented Oct 8, 2024

jjjkkkjjj commented Jul 10, 2024 •

edited

Loading

jjjkkkjjj commented Jul 11, 2024 •

edited

Loading

jjjkkkjjj Aug 3, 2024 •

edited

Loading

jjjkkkjjj commented Aug 5, 2024 •

edited

Loading

jjjkkkjjj Aug 9, 2024 •

edited

Loading

Allow to use `include`, `reverse` (channels wraper)

jjjkkkjjj commented Aug 10, 2024 •

edited

Loading

jjjkkkjjj commented Aug 18, 2024 •

edited

Loading

carltongibson commented Aug 27, 2024 •

edited

Loading

jjjkkkjjj Oct 8, 2024 •

edited

Loading