Support URLRouter with include

[jjjkkkjjj]

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]

Would you mind fixing the lint errors?

[jjjkkkjjj]

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]

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]

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.

[jjjkkkjjj]

@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

[jjjkkkjjj]

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.

[jjjkkkjjj]

@bigfootjon

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

Please check them again!

[jjjkkkjjj]

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]

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]

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

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]

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

@carltongibson OK! Wait for a moment.

[jjjkkkjjj]

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

[carltongibson]

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

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

[jjjkkkjjj]

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

[carltongibson]

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

[jjjkkkjjj]

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

[carltongibson]

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

[jjjkkkjjj]

@bigfootjon @carltongibson Any progress?