Code

Opened 6 years ago

Closed 6 weeks ago

#7571 closed Bug (fixed)

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

Reported by: trevor 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/'

Attachments (0)

Change History (14)

comment:1 Changed 6 years ago by mattmcc

  • Resolution set to worksforme
  • Status changed from new to closed

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 6 years ago by trevor

  • Resolution worksforme deleted
  • Status changed from closed to reopened

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 6 years ago by mattmcc

  • Triage Stage changed from Unreviewed to Design 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 6 years ago by ubernostrum

  • Resolution set to worksforme
  • Status changed from reopened to closed

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 6 years ago by trevor

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 6 years ago by trevor

  • Resolution worksforme deleted
  • Status changed from closed to reopened

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 5 years ago by gogna

  • Cc flavio.curella@… added

comment:8 Changed 5 years ago by gogna

possibly a dupe of #9496

comment:9 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:10 Changed 3 years ago by Alex

  • Easy pickings unset
  • Triage Stage changed from Design decision needed to Accepted
  • UI/UX unset

trevor's comment demonstrates a genuine bug.

comment:11 Changed 14 months ago by micfan

it seems okay now:

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

comment:12 Changed 13 months ago by aaugustin

  • Status changed from reopened to new

comment:13 Changed 7 weeks ago by afuna

  • Owner changed from nobody to afuna
  • Status changed from new to assigned

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 6 weeks ago by Baptiste Mispelon <bmispelon@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.