Behaviour change in URL patterns.

[Carlton Gibson]

There's a behaviour change on main pre-5.1 in 5dfcf343cd414d3f7a33dabb763b4478fa081d72 for #35250.

commit 5dfcf343cd414d3f7a33dabb763b4478fa081d72
Author: Adam Johnson <me@adamj.eu>
Date:   Sat Feb 24 19:14:22 2024 +0000

    Refs #35250 -- Avoided double conversion in RoutePattern.

 django/urls/resolvers.py | 58 ++++++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 22 deletions(-)

Since this commit, there are two failures in the Channels test suite.

$ pytest -k test_routing.py

...

=========================== short test summary info ============================
FAILED tests/test_routing.py::test_url_router_nesting_path - ValueError: No r...
FAILED tests/test_routing.py::test_path_remaining - ValueError: No route foun...
================== 2 failed, 6 passed, 55 deselected in 0.07s ==================

See CI run here: ​https://github.com/django/channels/actions/runs/8467612542/job/23198786518#step:5:342

Tracking issue on django/channels: ​https://github.com/django/channels/issues/2084

[Natalia Bidart]

Hello Carlton! Thank you for the report and details.

Would you have a failing regression test for the Django test suite? Otherwise I'll try to come up with one :-)

[Carlton Gibson]

Not yet. It only showed up this afternoon and I didn’t have a chance to sit down with it yet.

The route.pattern.match() is returning None, where before it was matching. Haven’t yet been able to look at why.

[Carlton Gibson]

OK, here's the difference. The Channels URLRouter essentially does this.

Before the commit:

>>> from django.urls import path
>>> p = path("test/", lambda x: x)
>>> p.pattern._is_endpoint = False
>>> p.pattern.regex
re.compile('^test/')

After the commit:

>>> from django.urls import path
>>> p = path("test/", lambda x: x)
>>> p.pattern._is_endpoint = False
>>> p.pattern.regex
re.compile('^test/\\Z')

The change breaks the ability to have non-endpoint usages of path(), because the regex is compiled on instantiation before _is_endpoint can be updated.

That's clearly a private attribute, but Channel's ability to have nested URL routing depends on it, and it's a likely explanation of why the _route_to_regex() helper was previously called twice, each time discarding half the result.

It would be good to find a way to defer the _route_to_regex()[0] call until needed, to not change the behaviour here, whilst also keeping the system check performance gain.

[Adam Johnson]

a likely explanation of why the _route_to_regex() helper was previously called twice, each time discarding half the result.

It was called twice since introduction in df41b5a05d4e00e80e73afe629072e37873e767a. Channels only started touching _is_endpoint two years later in ​984e9dc9cece66f7520bc9c1fd0583e9c60b1022.

IMO the reason it was called twice was refactoring during development to support translated URLs, without consideration for keeping the fast path for non-translated ones.

The change breaks the ability to have non-endpoint usages of path(), because the regex is compiled on instantiation before _is_endpoint can be updated.

Well, yes, but from Django’s perspective that shouldn’t happen. path() is an endpoint, include() is for a non-endpoint: ​https://github.com/django/django/blob/5f3cdf219de71021cc5b965695dbe0c881c0a4f1/django/urls/conf.py#L62-L96 .

whilst also keeping the system check performance gain.

Just to be clear, removing double execution of _route_to_regex() doesn’t affect only system checks, but every Django process that loads the URLconf - runserver, production servers, some management commands, ...

---

IMO the fix should be made in Channels here, since it’s using private attributes. I have made a PR there: ​https://github.com/django/channels/pull/2085 .

(Ideally, Channels should have its own path() / re_path(), because it does the routing itself anyway. But that boat has sailed.)

If we don’t want that fix, the only thing I can think of is making RegexPattern and RoutePattern bend over backwards to support _is_endpoint changing with something like this:

diff --git django/urls/resolvers.py django/urls/resolvers.py
index c667d7f268..6c5770cb61 100644
--- django/urls/resolvers.py
+++ django/urls/resolvers.py
@@ -316,11 +316,19 @@ class RoutePattern(CheckURLMixin):
 
     def __init__(self, route, name=None, is_endpoint=False):
         self._route = route
-        self._regex, self.converters = _route_to_regex(str(route), is_endpoint)
         self._regex_dict = {}
         self._is_endpoint = is_endpoint
         self.name = name
 
+    @property
+    def _is_endpoint(self):
+        return self.__is_endpoint
+
+    @_is_endpoint.setter
+    def _is_endpoint(self, value):
+        self.__is_endpoint = value
+        self._regex, self.converters = _route_to_regex(str(self._route), value)
+
     def match(self, path):
         match = self.regex.search(path)
         if match:

[Carlton Gibson]

IMO the fix should be made in Channels here, since it’s using private attributes. I have made a PR there: ​​https://github.com/django/channels/pull/2085 .

Agreed. That looks like the right fix. Thanks Adam.