Opened 8 years ago

Closed 3 years ago

#7571 closed Bug (fixed)

named groups in the regexes of include() urls break reverse()

Reported by: Trevor Caira Owned by: afuna
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: flavio.curella@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

# In project urls.py:
from django.conf.urls.defaults import *

urlpatterns = patterns('',
    (r'^(?P<foo>\w+)/(?P<bar>\w+)/', include('asdf.urls')),
)

# In asdf/urls.py:
from django.conf.urls.defaults import *

urlpatterns = patterns('',
    url(r'^(?P<bat>\w+)/$', lambda: None, name='test'),
)

# Running this in ./manage.py shell...
reverse('test', args=['foo', 'bar', 'bat'])
# returns:
'/foo/bar/foo/'
# instead of:
'/foo/bar/bat/'

Change History (14)

comment:1 Changed 8 years ago by Matt McClanahan

Resolution: worksforme
Status: newclosed

Named groups are populated with keyword arguments, not positional arguments.

reverse('test', kwargs={'foo': 'FOO', 'bar': 'BAR', 'bat': 'BAT'}
'/FOO/BAR/BAT/'

comment:2 Changed 8 years ago by Trevor Caira

Resolution: worksforme
Status: closedreopened

If args is this incorrect way to use reverse with include()'d URLs, it should raise an exception, not give surprising results, and this should be documented. There is no mention of this behavior in [1]. Also, there is no way to use kwargs with the {% url %} template tag.

[1] http://www.djangoproject.com/documentation/url_dispatch/#including-other-urlconfs

comment:3 Changed 8 years ago by Matt McClanahan

Triage Stage: UnreviewedDesign decision needed

Keyword arguments with the url template tag are documented. http://www.djangoproject.com/documentation/templates/#url

Leaving open so someone else can address if an error should be raised here.

comment:4 Changed 8 years ago by James Bennett

Resolution: worksforme
Status: reopenedclosed

The URL dispatch documentation explains quite clearly the difference between named and non-named groups, and the fact that they result in keyword and positional arguments, respectively. Meanwhile, the documentation for the 'url' tag quite clearly shows how to use keyword arguments.

Please do not reopen this ticket unless you can demonstrate a genuine bug.

comment:5 Changed 8 years ago by Trevor Caira

Then why does reverse() work at all with args if only named groups have been given in the URL regex? In fact, they work perfectly well until someone tries to use include() in this way.

I suspect many people might be relying on this undocumented feature, and may find it surprising when they try to use include() like this. Perhaps the best solution would be to fully support matching named groups with positional parameters (as partial support already exists), and provide documentation of it.

Otherwise a developer might be lured into the trap of depending on this feature and be very surprised to learn that in order to factor out the matching of parts of the url above the application, they have to fundamentally alter the way those URLs are resolved.

comment:6 Changed 8 years ago by Trevor Caira

Resolution: worksforme
Status: closedreopened

Upon further testing, this issue doesn't seem to be related to named URL groups in particular. Witness:

# In project urls.py:
from django.conf.urls.defaults import *

urlpatterns = patterns('',
    (r'^(\w+)/', include('asdf.urls')),
)

# In asdf/urls.py:
from django.conf.urls.defaults import *

urlpatterns = patterns('',
    url(r'^(\w+)/$', lambda: 0, name='asdf'),
)

# ./manage.py shell:
from django.core.urlresolvers import reverse
reverse('asdf', args=['foo', 'bar'])
# gives:
'/foo/foo/'
# and
from django.core.urlresolvers import RegexURLResolver
from django.conf import settings
r = RegexURLResolver('^/', settings.ROOT_URLCONF)
r.resolve('/foo/bar/')
(<function <lambda> at 0x87a317c>, ('bar',), {})

comment:7 Changed 8 years ago by Flavio Curella

Cc: flavio.curella@… added

comment:8 Changed 8 years ago by Flavio Curella

possibly a dupe of #9496

comment:9 Changed 6 years ago by Luke Plant

Severity: Normal
Type: Bug

comment:10 Changed 5 years ago by Alex Gaynor

Easy pickings: unset
Triage Stage: Design decision neededAccepted
UI/UX: unset

trevor's comment demonstrates a genuine bug.

comment:11 Changed 4 years ago by micfan

it seems okay now:

reverse('test', args=['foo', 'bar', 'bat'])
# gives:
'/foo/bar/bat/'

comment:12 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:13 Changed 3 years ago by afuna

Owner: changed from nobody to afuna
Status: newassigned

So, it looks like this is actually two things.

The original issue as described has been fixed. This is a case of outer regex + included regex, both using named groups. There's a test case in urlpatterns_reverse/tests.py that matches and confirms:

('inner-extra', '/outer/42/extra/inner/', ['42', 'inner'], {}), in test_data

where the relevant url patterns are: url(r'^outer/(?P<outer>\d+)/', include('urlpatterns_reverse.included_urls')), and url(r'^extra/(?P<extra>\w+)/$', empty_view, name="inner-extra"),

comment:6 is a different thing and that one is still broken. That is a case of both the outer and inner regex using positional parameters, and the positions overlapping when the inner regex is appended to the outer regex. So for example:

outer regex: outer/(\d+)/
inner regx: inner/(\d+)/
arguments: 123, 456

resolves to /outer/123/inner/123/

PR: https://github.com/django/django/pull/2375

comment:14 Changed 3 years ago by Baptiste Mispelon <bmispelon@…>

Resolution: fixed
Status: assignedclosed

In 5d568bcfa66916e3de61e0090c724c899debd981:

Fixed #7571 -- Fixed parameter matching in include()'d urlpattern

Fixed URL resolving in the case where an outer regex includes an inner
regex and both regexes use positional parameters instead of named
groups, causing the outer regex's parameters to override the inner
regex's.

Modified the regex url resolver so that it will concatenates and then
normalizes, instead of normalizing and then concatenating.

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