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.

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

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

[jjjkkkjjj]

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

[bigfootjon]

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

[jjjkkkjjj]

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

[jjjkkkjjj]

I removed it for now by 758a205

[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

[bigfootjon]

Getting closer!

Please see my comments and fix the lint warnings

[bigfootjon]

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.

[jjjkkkjjj]

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.

[bigfootjon]

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:

1. Support nested urlpatterns
2. 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]

OK!

[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.

[bigfootjon]

All I see left is:

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

[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 (a303572). Please take a look!

[carltongibson]

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.)

[carltongibson]

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

[jjjkkkjjj]

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?

[carltongibson]

Why the trailing |?

[jjjkkkjjj]

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

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