Opened 6 years ago

Closed 4 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: hvdklauw
Component: Core (Other) Version: master
Severity: Normal Keywords: dceu2011
Cc: darkrho, kmike84@…, alexkoshelev, me@…, apollo13, trey@…, t.kaemming+t11559@…, gnublade@…, remco@…, trowbrds@…, chipx86@…, ungenio@…, mvt, 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 6 years ago.
Hack to get parameterised parent namespace urlconfs to reverse (against 1.1)
urlresolvers.reverse.hack.diff (1.9 KB) - added by cwb 6 years ago.
Maybe SVN diff against trunk with .diff extension then.. (Patch not tested on trunk)
11559.patch (3.4 KB) - added by ungenio 4 years ago.
Added some polish to patch from #12950. Includes test.
ticket_11559.diff (3.9 KB) - added by hvdklauw 4 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 Changed 6 years ago by danros

  • milestone set to 1.1
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

  • milestone 1.1 deleted
  • Triage Stage changed from Unreviewed to Someday/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 Changed 6 years ago by kmike

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

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

  • Cc alexkoshelev added

comment:6 Changed 6 years ago by daybreaker

  • Cc me@… added

Changed 6 years ago by cwb

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

comment:7 Changed 6 years ago by cwb

  • 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.

Changed 6 years ago by cwb

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

comment:8 Changed 5 years ago by apollo13

  • Cc apollo13 added

comment:9 Changed 5 years ago by anonymous

  • Cc trey@… added

comment:10 Changed 5 years ago by trey@…

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

  • Cc t.kaemming+t11559@… added

comment:12 Changed 5 years ago by gnublade

  • Cc gnublade@… added

comment:13 Changed 5 years ago by shanx

  • Cc remco@… added

comment:14 Changed 5 years ago by darkrho

  • Cc darkrho added

comment:15 Changed 5 years ago by anonymous

  • Cc trowbrds@… added

comment:16 Changed 5 years ago by chipx86

  • Cc chipx86@… added

comment:17 Changed 5 years ago by chipx86

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 Changed 4 years ago by carljm

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

Changed 4 years ago by ungenio

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

comment:19 Changed 4 years ago by ungenio

  • 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 Changed 4 years ago by ungenio

  • Cc ungenio@… added

comment:21 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to New feature

comment:22 Changed 4 years ago by mvt

  • Cc mvt added

comment:23 Changed 4 years ago by jgelens

  • Cc jeffrey@… added
  • Easy pickings unset

comment:24 Changed 4 years ago by cwb

  • Cc calle.ba@… removed

comment:25 Changed 4 years ago by AndrewIngram

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 Changed 4 years ago by hvdklauw

  • UI/UX unset

patch applies to trunk (r16351) and tests pass

comment:27 Changed 4 years ago by hvdklauw

  • Keywords dceu2011 added
  • Triage Stage changed from Someday/Maybe to Accepted

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

comment:28 Changed 4 years ago by hvdklauw

  • Owner changed from nobody to hvdklauw

Changed 4 years ago by hvdklauw

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

comment:29 Changed 4 years ago by oinopion

  • Triage Stage changed from Accepted to Ready for checkin

comment:30 Changed 4 years ago by anonymous

  • Triage Stage changed from Ready for checkin to Accepted

comment:31 Changed 4 years ago by hvdklauw

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:32 Changed 4 years ago by jezdez

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

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