Opened 6 years ago

Closed 5 years ago

#29775 closed Bug (fixed)

custom url converters are not picked up on reverse when part of included patterns with namespace

Reported by: Eric Brandwein Owned by: Eric Brandwein
Component: Core (URLs) Version: dev
Severity: Normal Keywords: converter, namespace, reverse, include
Cc: Herbert Fortes Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Under normal circumstances, url converters to_url methods are called when reversing a url, however, this behavior is inconsistent when used with included sub-patterns, when these sub-patterns have themselves included sub-patterns with a namespace assigned.

To demonstrate:

from django.urls import path, include, register_converter
from django.urls.converters import StringConverter


class ReverseStringConverter(StringConverter):
    """More complex use cases are possible, but this simple case of reversing the string already shows it in effect"""
    def to_python(self, value):
        return value[::-1]

    def to_url(self, value):
        return value[::-1]


# We just want to test the reverse()
def noop(request, **kwargs):
    pass


register_converter(ReverseStringConverter, 'reverse')

second_layer_urlpatterns = [
    path('', noop, name='failure-view'),
]

first_layer_urlpatterns = [
    path('', include(
        (second_layer_urlpatterns, 'app_name'), namespace='ns')),
    path('', include(second_layer_urlpatterns)),
    path('', noop, name='success-view'),
]

urlpatterns = [
    path('<reverse:param>/', include(first_layer_urlpatterns)),
]

When running the following test, reverse works fine when reversing failure-view or success-view, but not when reversing ns:failure-view:

>>> from django.urls import reverse
>>> reverse('failure-view', args=['foo'])
'/oof/'
>>> reverse('ns:failure-view', args=['foo'])
'/foo/'
>>> reverse('success-view', args=['foo'])
'/oof/'

It probably is very similar to #29415. I tested it in version 2.1.1.

Change History (10)

comment:1 by Carlton Gibson, 6 years ago

Hi Eric. Can I ask for a bit more info here?

Maybe I'm just missing it, but it looks to me as if 39283c8edbc5991b589d48a8e17152042193f2df (the fix for #29415) added a test for exactly this case.

Not attempting any reuse of the above test, this diff passes:

diff --git a/tests/urlpatterns/path_base64_urls.py b/tests/urlpatterns/path_base64_urls.py
index 9b69f929fe..52a4045281 100644
--- a/tests/urlpatterns/path_base64_urls.py
+++ b/tests/urlpatterns/path_base64_urls.py
@@ -12,4 +12,5 @@ urlpatterns = [
     path('base64/<base64:value>/', views.empty_view, name='base64'),
     path('base64/<base64:base>/subpatterns/', include(subpatterns)),
     path('base64/<base64:base>/namespaced/', include((subpatterns, 'namespaced-base64'))),
+    path('base64/<base64:base>/namespaced/', include((subpatterns, 'namespaced-base64'), namespace='instance-ns-base64')),
 ]
diff --git a/tests/urlpatterns/tests.py b/tests/urlpatterns/tests.py
index b3d97ec5b9..f108fdfb55 100644
--- a/tests/urlpatterns/tests.py
+++ b/tests/urlpatterns/tests.py
@@ -70,6 +70,16 @@ class SimplifiedURLTests(SimpleTestCase):
                 url = reverse(url_name, kwargs=kwargs)
                 self.assertEqual(url, expected)
 
+    @override_settings(ROOT_URLCONF='urlpatterns.path_base64_urls')
+    def test_converter_reverse_with_instance_namespace(self):
+        expected = '/base64/aGVsbG8=/namespaced/d29ybGQ=/'
+        url_name = 'subpattern-base64'
+        instance_ns = 'instance-ns-base64'
+        kwargs = included_kwargs
+        url_name = '%s:%s' % (instance_ns, url_name)
+        url = reverse(url_name, kwargs=kwargs)
+        self.assertEqual(url, expected)
+
     def test_path_inclusion_is_matchable(self):
         match = resolve('/included_urls/extra/something/')
         self.assertEqual(match.url_name, 'inner-extra')

Are you able to adjust the test case there to show your issue in action?

Thanks!

Version 1, edited 6 years ago by Carlton Gibson (previous) (next) (diff)

comment:2 by Carlton Gibson, 6 years ago

Resolution: needsinfo
Status: newclosed

comment:3 by Eric Brandwein, 6 years ago

Hi Carlton, the issue happens when you have some URL with a custom converter, in which you are including other URLs without passing a namespace to include(). These other URLs, in turn, include another list of URLs, this time with a namespace. The tests you mention seem to just test namespacing from the first level of URLs, instead of from just the second, like in the example I gave. It would seem that when using instance namespaces, the converters are only searched from one include() upwards, instead of from all the include() "tree".

I made a test for it here: https://github.com/ericbrandwein/django/commit/de60d4f200ef08573f92942f0269a4b22a878290. The diff is:

diff --git a/tests/urlpatterns/path_base64_urls.py b/tests/urlpatterns/path_base64_urls.py
index 9b69f929fe..c9b4f6f9ca 100644
--- a/tests/urlpatterns/path_base64_urls.py
+++ b/tests/urlpatterns/path_base64_urls.py
@@ -4,8 +4,14 @@ from . import converters, views
 
 register_converter(converters.Base64Converter, 'base64')
 
+subsubpatterns = [
+    path('<base64:last_value>/', views.empty_view, name='subsubpattern-base64'),
+]
+
 subpatterns = [
     path('<base64:value>/', views.empty_view, name='subpattern-base64'),
+    path('<base64:value>/',
+        include((subsubpatterns, 'second-layer-namespaced-base64'), 'instance-ns-base64')),
 ]
 
 urlpatterns = [
diff --git a/tests/urlpatterns/tests.py b/tests/urlpatterns/tests.py
index b3d97ec5b9..8483d225d5 100644
--- a/tests/urlpatterns/tests.py
+++ b/tests/urlpatterns/tests.py
@@ -70,6 +70,17 @@ class SimplifiedURLTests(SimpleTestCase):
                 url = reverse(url_name, kwargs=kwargs)
                 self.assertEqual(url, expected)
 
+    @override_settings(ROOT_URLCONF='urlpatterns.path_base64_urls')
+    def test_converter_reverse_with_second_layer_instance_namespace(self):
+        expected = '/base64/aGVsbG8=/namespaced/d29ybGQ=/d29ybGQ=/'
+        url_name = 'subsubpattern-base64'
+        instance_ns = 'instance-ns-base64'
+        kwargs = included_kwargs
+        kwargs['last_value'] = b'world'
+        url_name = '%s:%s' % (instance_ns, url_name)
+        url = reverse(url_name, kwargs=kwargs)
+        self.assertEqual(url, expected)
+
     def test_path_inclusion_is_matchable(self):
         match = resolve('/included_urls/extra/something/')
         self.assertEqual(match.url_name, 'inner-extra')

Maybe this test is somewhat ugly, but it shows the bug. It fails with this message:

django.urls.exceptions.NoReverseMatch: Reverse for 'subsubpattern-base64' with keyword arguments '{'base': b'hello', 'value': b'world', 'last_value': b'world'}' not found. 1 pattern(s) tried: ['base64/(?P<base>[a-zA-Z0-9+/]*={0,2})/subpatterns/(?P<value>[a-zA-Z0-9+/]*={0,2})/(?P<last_value>[a-zA-Z0-9+/]*={0,2})/$']

Which means that the converter's regex is being used, but not its to_url function.

comment:4 by Carlton Gibson, 6 years ago

Resolution: needsinfo
Status: closednew

Super. Good follow-up! Will review today. Thanks.

comment:5 by Carlton Gibson, 6 years ago

Triage Stage: UnreviewedAccepted
Version: 2.1master

OK, yes. Good. The test case reproduces — thank you for that!

(Adding the nested instance namespace 'instance-ns-base64' for a nested include is a little strange no? But taking it out doesn't affect the test: removing it and reversing second-layer-namespaced-base64:subsubpattern-base64 has exactly the same error.)

comment:6 by Eric Brandwein, 6 years ago

Great!

I personally was using the nested instance namespace to be able to use the subsubpatterns in two different includes. In my case, the application namespace of these was declared in another file, along with the subsubpatterns. So I think using it that way wasn't as weird as this one.

BTW, I'm sorry for any grammar mistakes I may have had; English is not my first language :/

comment:7 by Herbert Fortes, 6 years ago

Cc: Herbert Fortes added

comment:8 by Eric Brandwein, 5 years ago

Owner: changed from nobody to Eric Brandwein
Status: newassigned

comment:9 by Tim Graham, 5 years ago

Has patch: set

comment:10 by Tim Graham <timograham@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In b0b4aac5:

Fixed #29775 -- Fixed URL converters in a nested namespaced path.

When using include() without namespaces of some urlpatterns that
have an include() with namespace, the converters of the parent
include() weren't being used to convert the arguments of reverse().

Note: See TracTickets for help on using tickets.
Back to Top