Opened 3 years ago

Last modified 2 months ago

#17914 new Bug

reverse() does not support namespaced view references

Reported by: Bradley Ayers <bradley.ayers@…> Owned by: nobody
Component: Core (URLs) Version: master
Severity: Normal Keywords:
Cc: bradley.ayers@…, amirouche.boubekki@…, gwahl@…, apollo13 Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

It's not possible to reverse a URL by passing a function reference to reverse()
that's hooked into the URLconf under a namespace.

Consider the trivial Django project (that doesn't work):

# urls.py
from django.conf.urls.defaults import patterns, include
from django.core.urlresolvers import reverse
from django.http import HttpResponse

def view(request):
    # Return the URL to this view
    return HttpResponse(reverse(view))

level2 = patterns('',
    (r'^2/', view)
)

urlpatterns = patterns('',
    (r'^1/', include(level2, namespace="foo"))
)

Removing , namespace="foo" will make it work.

My understanding of why this happens, is that reverse() traverses RegexURLResolvers
*only* for strings. Function references are passed directly to the root RegexURLResolver.reverse
method.:

# from django/core/urlresolvers.py ~ line 432

if not isinstance(viewname, basestring):
    view = viewname
else:
    # <snip> -- resolver traversal happens here to honor namespaces
    # `resolver` is reassigned to last RegexURLResolver in namespace chain

return iri_to_uri(resolver._reverse_with_prefix(view, prefix, *args, **kwargs))

Obviously traversing namespaces isn't possible for a function reference,
so what's needed is to add *all* descendent view references into each RegexURLResolver's
reverse_dict dictionary.

Populating reverse_dict is lazy and happens on first access, the work is
performed in RegexURLResolver._populate:

# django/core/urlresolvers.py ~ line 237
def _populate(self):
    # <snip>
    for pattern in reversed(self.url_patterns):
        # <snip>
        if isinstance(pattern, RegexURLResolver):
            if pattern.namespace:
                # <snip> -- tldr `self.reverse_dict` isn't populated, `pattern` (the
                #           RegexURLResolver) is instead stored in `self.namespace_dict`
            else:
                # <snip> -- tldr `self.reverse_dict` *is* populated with the keys
                #           being the name of each URL (or the function reference)
                #           defined in this level. (i.e. only children, not all descendants)
        else:
            # <snip> -- tldr `self.reverse_dict` is again populated

This combination of behaviour by RegexURLResolver._populate and reverse()
leads to the problem.

Attachments (2)

17914-test.patch (2.5 KB) - added by Bradley Ayers <bradley.ayers@…> 3 years ago.
test to expose bug
17914.patch (8.0 KB) - added by Bradley Ayers <bradley.ayers@…> 3 years ago.
fix + tests

Download all attachments as: .zip

Change History (16)

Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

test to expose bug

Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

fix + tests

comment:1 Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

  • Cc bradley.ayers@… added

comment:3 Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

  • Version changed from 1.3 to SVN

comment:4 Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

It should be noted that this patch is technically backwards incompatible. After applying this patch, it's no longer possible to use recursive URL patterns, e.g. consider a project named 'foo', and it had a urls.py that contained:

urlpatterns = patterns('', (r'^abc/', include('foo.urls'))

comment:5 Changed 3 years ago by SmileyChris

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Accepting.

Like we talked about on IRC, let's just avoid circular imports when doing your traversal rather than dropping that functionality.

comment:6 Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

I've updated the patch to maintain support for recursive urlconfs. https://github.com/django/django/pull/174

comment:7 Changed 3 years ago by mtredinnick

Doing a quick scan of URL reversing during a sprint, the last git code looks reasonable. Not ready to commit yet -- I haven't fully digested it, but my intuition is that it's the right kind of thing.

Except(!) that using function objects in reverse() situations is kind of a "don't do that" kind of situation. We permitted it initially for smooth integration, but it's kind of a lucky happenstance. My point being, that a patch of this size is a reasonable to fix this ticket. If it grows to twice this size, we should maybe just add a "don't do that" note to the documentation -- the extra complexity isn't worth it.

For now, charge ahead!

comment:8 Changed 2 years ago by abki

  • Cc amirouche.boubekki@… added

comment:9 Changed 2 years ago by gwahl@…

  • Cc gwahl@… added

comment:10 Changed 15 months ago by bpeschier

Closed #21899 as a duplicate

comment:11 Changed 15 months ago by Tonnzor

Any news here?

Since reverse now accepts app_name -- as well as include -- we could easily find corresponding namespace. E.g.:

from django.conf.urls import *

urlpatterns = patterns('',
    url(r'^payment/', include('payment.urls', namespace='payment', app_name='payment')),
)

reverse(view, current_app='payment') # try with current_app given

As alternative solution -- we could require passing namespace when using with function. The we could convert function to its name, add namespace+":" and use existing machinery.

comment:12 Changed 15 months ago by mrmachine

#21899 was closed a duplicate of this ticket, but I'm not sure they are the same thing. Will fixing this ticket also fix #21899? This ticket seems to be about reversing raw callable view functions, for which we have no way to specify a namespace. The other ticket was about reversing a dotted path reference to a view function, specified as a string with a namespace.

comment:13 Changed 5 months ago by apollo13

  • Cc apollo13 added

Can someone present a usecase for using function references in reverse? IMO this should just get closed as wontfix and names should be used throughout instead.

comment:14 Changed 2 months ago by bpeschier

This feels like using args and kwargs on reverses. Using either function reverses or namespaces is fine, just don't mix them.

Maybe we should enforce it a bit more like we do with reversing with args and kwargs?

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