[Django] #29775: custom url converters are not picked up on reverse when part of included patterns with namespace
Django
patterns with namespace
-------------------------------------+-------------------------------------
Reporter: | Owner: nobody
ericbrandwein |
Type: Bug | Status: new
Component: Core | Version: 2.1
(URLs) | Keywords: converter,
Severity: Normal | namespace, reverse, include
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29775>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
patterns with namespace
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (URLs) | Version: 2.1
Severity: Normal | Resolution:
Keywords: converter, | Triage Stage:
namespace, reverse, include | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
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.
*
[https://github.com/django/django/pull/9965/commits/39283c8edbc5991b589d48a8e17152042193f2df
#diff-f9842aa823757ce784641bd27abaf2deR14 Namepaced pattern].
*
[https://github.com/django/django/pull/9965/commits/39283c8edbc5991b589d48a8e17152042193f2df
#diff-055d5f3389bf8b35e12c627956f4a33cR17 Test case data using namepace].
*
[https://github.com/django/django/pull/9965/commits/39283c8edbc5991b589d48a8e17152042193f2df
#diff-055d5f3389bf8b35e12c627956f4a33cR70 Check that it reverses
correctly].
Are you able to adjust the test case there to show your issue in action?
Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/29775#comment:1>
Django
patterns with namespace
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Core (URLs) | Version: 2.1
Keywords: converter, | Triage Stage:
namespace, reverse, include | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => needsinfo
--
Ticket URL: <https://code.djangoproject.com/ticket/29775#comment:2>
Django
patterns with namespace
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: Core (URLs) | Version: 2.1
Severity: Normal | Resolution: needsinfo
Keywords: converter, | Triage Stage:
namespace, reverse, include | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Eric Brandwein):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29775#comment:3>
Django
patterns with namespace
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (URLs) | Version: 2.1
Severity: Normal | Resolution:
namespace, reverse, include | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: needsinfo =>
Comment:
Super. Good follow-up! Will review today. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/29775#comment:4>
Django
patterns with namespace
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converter, | Triage Stage: Accepted
namespace, reverse, include |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 2.1 => master
* stage: Unreviewed => Accepted
Comment:
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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29775#comment:5>
Django
patterns with namespace
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converter, | Triage Stage: Accepted
namespace, reverse, include |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Eric Brandwein):
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 :/
--
Ticket URL: <https://code.djangoproject.com/ticket/29775#comment:6>
Django
patterns with namespace
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converter, | Triage Stage: Accepted
namespace, reverse, include |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Herbert Fortes (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29775#comment:7>
Django
patterns with namespace
-------------------------------------+-------------------------------------
| Brandwein
Type: Bug | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converter, | Triage Stage: Accepted
namespace, reverse, include |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Eric Brandwein
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29775#comment:8>
Django
patterns with namespace
-------------------------------------+-------------------------------------
| Brandwein
Type: Bug | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converter, | Triage Stage: Accepted
namespace, reverse, include |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/10475 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29775#comment:9>
Django
patterns with namespace
-------------------------------------+-------------------------------------
| Brandwein
Component: Core (URLs) | Version: master
Keywords: converter, | Triage Stage: Accepted
namespace, reverse, include |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"b0b4aac555711ae9116f9b54c24ec7e43a0971e9" b0b4aac5]:
{{{
#!CommitTicketReference repository=""
revision="b0b4aac555711ae9116f9b54c24ec7e43a0971e9"
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().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29775#comment:10>