Opened 15 years ago

Closed 13 years ago

#11559 closed New feature (fixed)

urlresolvers.reverse do not work with namespaced urls and captured parameters in parent urlconf

Reported by: kmike84@… Owned by: Harro
Component: Core (Other) Version: dev
Severity: Normal Keywords: dceu2011
Cc: Rolando Espinoza La fuente, kmike84@…, Alexander Koshelev, me@…, Florian Apolloner, trey@…, t.kaemming+t11559@…, gnublade@…, remco@…, trowbrds@…, chipx86@…, ungenio@…, Michael van Tellingen, jeffrey@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Urlresolvers.reverse (and {% url .. %} template tag) do not work with namespaced urls and captured parameters in parent urlconf.

This works:

# In urls.py
urlpatterns = patterns('',
    (r'^(?P<username>\w+)/blog/', include('foo.urls')),
)

# In foo/urls.py
urlpatterns = patterns('foo.views',
    url(r'^index$', 'blog.index', name='blog_index'),
    url(r'^archive/$', 'blog.archive', name='blog_archive'),
)

#in some view
from django.core.urlresolvers import reverse
url = reverse('blog_index', args=['john'])

And this doesn't:

# In urls.py
urlpatterns = patterns('',
    (r'^(?P<username>\w+)/blog/', include('foo.urls', namespace='user_blogs', app_name='foo')),
)

# In foo/urls.py
urlpatterns = patterns('foo.views',
    url(r'^index$', 'blog.index', name='blog_index'),
    url(r'^archive/$', 'blog.archive', name='blog_archive'),
)

#in some view
from django.core.urlresolvers import reverse
url = reverse('user_blogs:blog_index', args=['john'], current_app='foo')

This works again:

# In urls.py
urlpatterns = patterns('',
    (r'^(?P<username>\w+)/blog/', include('foo.urls', namespace='user_blogs', app_name='foo')),
)

# In foo/urls.py
urlpatterns = patterns('foo.views',
    url(r'^index$', 'blog.index', name='blog_index'),
    url(r'^archive/$', 'blog.archive', name='blog_archive'),
)

#in some view
from django.core.urlresolvers import reverse
url = reverse('user_blogs:blog_index', current_app='foo')

# but `url` variable contains "/(?P<username>\\w+)/blog/index"

Attachments (4)

urlresolvers.reverse.hack.patch (1.5 KB ) - added by cwb 14 years ago.
Hack to get parameterised parent namespace urlconfs to reverse (against 1.1)
urlresolvers.reverse.hack.diff (1.9 KB ) - added by cwb 14 years ago.
Maybe SVN diff against trunk with .diff extension then.. (Patch not tested on trunk)
11559.patch (3.4 KB ) - added by David Bennett 13 years ago.
Added some polish to patch from #12950. Includes test.
ticket_11559.diff (3.9 KB ) - added by Harro 13 years ago.
Split the specific tests to own method, also added some for resolving the subitems

Download all attachments as: .zip

Change History (36)

comment:1 by Dan Ros, 15 years ago

milestone: 1.1

I also have exactly the same issue. Bit of a let-down as I upgraded to 1.1 ahead of time in order to use namespaced urls exactly in this way!

In my example I have a 'username' argument as part of the root urlconf.

reverse('userwikis:wiki-view', kwargs={'slug':'testslug'})

'/users/(?P%3Cusername%3E%5Cw+)/wikitestslug'

If I include the username argument:

reverse('userwikis:wiki-view', kwargs={'username':'bar','slug':'testslug'})

NoReverseMatch: Reverse for 'wiki-view' with arguments '()' and keyword arguments '{'username': 'bar', 'slug': 'testslug'}' not found.

Seems like a fairly nasty bug for 1.1? Especially as url namespaces are one of the headline features of 1.1.

comment:2 by Russell Keith-Magee, 15 years ago

milestone: 1.1
Triage Stage: UnreviewedSomeday/Maybe

It's arguable whether this is a bug or a feature request.

The URL namespace feature was designed to remove ambiguity in the deployment of multiple instances of an application. To that end, the namespace feature works exactly as intended.

What is being proposed here is a desire to parameterize the deployable application itself. In the example provided, the 'blog' application provides an index and archive view, but is being parameterized using the username. By contrast, the admin app is entirely self contained - it has no external parameterization, only a static 'mount point' for the URL space.

The provided URL sample doesn't require namespacing at all, since there is only one instance of the app. You would only require namespacing if there were two instances of the blog application:

/foo/<username>/blog/index
                    /archive
/bar/<username>/blog/index
                    /archive

While I can see how this could be an interesting addition to namespaced URLs, it isn't really the same problem faced by the admin app. It also opens up a bunch of questions regarding the manner in which the parameters are passed to an application - in this case, the blog requires that all views in the app take a username argument, and conversely, that it must be deployed in a tree that provides a captured username parameter. Given the potential for error here, Django would need to provide some measure of validation and protection.

So - if this is a new feature, it isn't a v1.1 blocker. However, if you choose to look at this as a bug, then it's a bug with a known workaround ("don't do that"), that doesn't break any existing code, has no potential for data loss, and with no simple solution. To that end, it's still not a v1.1 blocker at this late stage in the development cycle.

comment:3 by Mikhail Korobov, 15 years ago

If this is a new feature then existing behaviour should be documented.
"Captured parameters" section is right before "Defining URL Namespaces" and the fact that they do not work together as excepted is not mentioned.

I totally agree that fixing this ticket as-is is not a trivial task and it is not in 1.1 scope.

comment:4 by Dan Ros, 15 years ago

I didn't include my urlconfs, which showed multiple deployments of the same app in different areas of the site.

I can confirm however that the workaround mentioned works for me. I have a requirement to include some named groups in the urlconf of one deployment of the app, and another set in another deployment. I had tried to do this by having one named group in the root urls.py and another in the application urls.py. I then multiply-included the application urls.py (with varying named groups) in the root urlconf. This doesn't work with namespaces.

Workaround is to include a different urls.py for the app depending on the named groups required. Not as elegant, but it works.

comment:5 by Alexander Koshelev, 15 years ago

Cc: Alexander Koshelev added

comment:6 by daybreaker, 15 years ago

Cc: me@… added

by cwb, 14 years ago

Hack to get parameterised parent namespace urlconfs to reverse (against 1.1)

comment:7 by cwb, 14 years ago

Cc: calle.ba@… added
Patch needs improvement: set

In case anyone is interested in a hack -- not a proper solution -- I've attached a patch (against 1.1). Playing around in urlresolvers.py left me almost as clueless as before about its mechanics, so I'm convinced I'm missing edge (or non-edge!) cases. I needed this to multiply up a number of app URLs with various (parameterised) prefixes, giving them different and predictable names for reversing, but found later namespaces didn't support that (either) -- something to do with only one urlconf being searched per namespace unless I misunderstood something. Thus, I abandoned them and wrote a custom url include function instead. That means this hack has hardly seen any use.

by cwb, 14 years ago

Maybe SVN diff against trunk with .diff extension then.. (Patch not tested on trunk)

comment:8 by Florian Apolloner, 14 years ago

Cc: Florian Apolloner added

comment:9 by anonymous, 14 years ago

Cc: trey@… added

comment:10 by trey@…, 14 years ago

I would argue in the opposite direction; that this is not a feature but is in fact a bug. Since forward URL resolution works completely in this regard it should be expected that the reverse() method does the same.

Because this is outside of the problem domain of the admin does not mean it's an invalid use case. I think extending the problem to 'provide some measure of validation and protection' is adding more to this issue that necessary. Those of us advanced folks creating modular apps that DRY out a repeated system or task know that the views in this particular app take a parameter or kwargs and per the django norm are expected to write views correctly. Django already just allows python to handle it by raising an exception so in that regard there is no difference.

comment:11 by anonymous, 14 years ago

Cc: t.kaemming+t11559@… added

comment:12 by gnublade, 14 years ago

Cc: gnublade@… added

comment:13 by Remco Wendt, 14 years ago

Cc: remco@… added

comment:14 by Rolando Espinoza La fuente, 14 years ago

Cc: Rolando Espinoza La fuente added

comment:15 by anonymous, 13 years ago

Cc: trowbrds@… added

comment:16 by Christian Hammond, 13 years ago

Cc: chipx86@… added

comment:17 by Christian Hammond, 13 years ago

Curious if there's been any further discussion with this change. I've been trying to deploy an instance of the admin UI with a dynamic prefix (basically, hosting instances of a webapp grouped by some identifier), and have a custom subclass of AdminSite that can take the parameters and do the right thing.

Now, this works, except that reversing admin URLs fails because of the way namespaces and captured arguments in the prefix of the URL works. This behavior has actually delayed our launch by about a week, so I'm eager to find a solution of some sort. I think where things seem confusing is that namespaced URLs just do not work the way standard URLs do.

I played around with a couple patch ideas. One thing off the top of my head would be to have the namespaces dictionary build up the possible matches in place of the raw prefix. In a sense, it'd act like reverse_dict, except keyed off by namespace. Then the right possibilities could be received based on namespace.

The trick at that point would be to pass that information to RegexURLResolver.reverse. Maybe the namespace could be passed to it, or the list of possibilities to use instead of those in reverse_dict.

Or I may be on totally the wrong path. However, from the CC list and my own experiences, it doesn't seem that the current design is really sufficient for more advanced use, and I'd be happy to work on this but want to make sure I'm not about to waste any time by going in a direction you guys don't want to go in.

comment:18 by Carl Meyer, 13 years ago

#13052 and #12950 have both been closed as duplicates of this. The latter has a patch.

by David Bennett, 13 years ago

Attachment: 11559.patch added

Added some polish to patch from #12950. Includes test.

comment:19 by David Bennett, 13 years ago

Has patch: set
Patch needs improvement: unset

Thanks carljm for finding my dupe.

Took me a while to wrap my head around the issue once I noticed it. The patch I've submitted includes a test to demonstrate the problem.

My solution involves essentially copying namespaced resolver into a new root resolver. That way any parameters can be resolved as normal and not interfere with non-namespaced functionality.

This new version uses memoize to reduce whatever performance impact that copying might have.

Going to toggle patch flags, though won't be offended if patch is deemed unfit. :)

comment:20 by David Bennett, 13 years ago

Cc: ungenio@… added

comment:21 by Julien Phalip, 13 years ago

Severity: Normal
Type: New feature

comment:22 by Michael van Tellingen, 13 years ago

Cc: Michael van Tellingen added

comment:23 by Jeffrey Gelens, 13 years ago

Cc: jeffrey@… added
Easy pickings: unset

comment:24 by cwb, 13 years ago

Cc: calle.ba@… removed

comment:25 by AndrewIngram, 13 years ago

I've come across this problem whilst trying to develop a fairly complicated e-commerce platform. I want to use nested URL namespaces because it seems more DRY and elegant, so it would be useful if this could be implemented.

comment:26 by Harro, 13 years ago

UI/UX: unset

patch applies to trunk (r16351) and tests pass

comment:27 by Harro, 13 years ago

Keywords: dceu2011 added
Triage Stage: Someday/MaybeAccepted

Working on adding a few more test cases for this to make it complete.

comment:28 by Harro, 13 years ago

Owner: changed from nobody to Harro

by Harro, 13 years ago

Attachment: ticket_11559.diff added

Split the specific tests to own method, also added some for resolving the subitems

comment:29 by Tomek Paczkowski, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:30 by anonymous, 13 years ago

Triage Stage: Ready for checkinAccepted

comment:31 by Harro, 13 years ago

Triage Stage: AcceptedReady for checkin

Setting it back to Ready for checkin, anonymous.. log in if you have a solid reason :)

comment:32 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: newclosed

In [16608]:

Fixed #11559 -- Fixed the URL resolver to be able to handle captured parameters in parent URLconfs when also using namespaces. Thanks, cwb, ungenio and hvdklauw.

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