Opened 3 years ago

Closed 5 weeks ago

#17914 closed Bug (wontfix)

Add support for namespaced view references when passing a callable to reverse()

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: no
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 (3)

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
17914.diff (437 bytes) - added by timgraham 5 weeks ago.

Download all attachments as: .zip

Change History (26)

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 3 years ago by abki

  • Cc amirouche.boubekki@… added

comment:9 Changed 3 years ago by gwahl@…

  • Cc gwahl@… added

comment:10 Changed 19 months ago by bpeschier

Closed #21899 as a duplicate

comment:11 Changed 19 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 18 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 follow-up: Changed 8 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 6 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?

comment:15 in reply to: ↑ 13 ; follow-up: Changed 3 months ago by guettli

Replying to apollo13:

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.

For me the view-name is redundant. We try to avoid it. Reversing by function reference works fine (except this bug). Jumping through the code with an IDE is much more fun if you use function references.

It would be nice if this could be solved. For our team this means: don't use namespaced URLs :-(

comment:16 in reply to: ↑ 15 Changed 3 months ago by apollo13

Replying to guettli:

For me the view-name is redundant. We try to avoid it. Reversing by function reference works fine (except this bug). Jumping through the code with an IDE is much more fun if you use function references.

If I remember correctly we've been over the redundancy point on the ML already, you can always just write your own url function which sets the path of the function as name, not really redundant imo. Also reversing by function reference is something which is ugly in templates etc imo… And redundant is relative anyways, "app:view_name" is imo way nicer than "app.views.view_name" -- especially if you have nested imports.

comment:17 Changed 3 months ago by timgraham

I'm in favor of a "won't fix" of this issue and promoting reversing by URL name as Florian suggested. We could deprecate passing callables to reverse, but this will probably just needlessly annoy people who are happy with it. Instead, we could just remove it from the docs like we did with the @permalink decorator.

comment:18 Changed 2 months ago by timgraham

  • Summary changed from reverse() does not support namespaced view references to reverse() does not support namespaced view references when passed a callable

Changed 5 weeks ago by timgraham

comment:19 Changed 5 weeks ago by timgraham

  • Patch needs improvement unset

Added a documentation patch to discourage reversing by callable view.

comment:20 Changed 5 weeks ago by apollo13

LGTM

comment:21 Changed 5 weeks ago by Tim Graham <timograham@…>

In a6acfc31:

Refs #17914 -- Discouraged using reverese() with callables.

comment:22 Changed 5 weeks ago by Tim Graham <timograham@…>

In f32bb3a:

[1.8.x] Refs #17914 -- Discouraged using reverese() with callables.

Backport of a6acfc31837fd7a9e0e387320d995b2c85cfcfce from master

comment:23 Changed 5 weeks ago by timgraham

  • Resolution set to wontfix
  • Status changed from new to closed
  • Summary changed from reverse() does not support namespaced view references when passed a callable to Add support for namespaced view references when passing a callable to reverse()
Note: See TracTickets for help on using tickets.
Back to Top