Opened 16 years ago

Closed 13 years ago

Last modified 12 years ago

#5925 closed Cleanup/optimization (fixed)

Unable to use urlresolvers (url,reverse ) feature in redirect argument of decorator user_passes_test

Reported by: anonymous Owned by: Chris Beaven
Component: Core (Other) Version: dev
Severity: Normal Keywords: urlresolver, reverse, decorator, url, permalink
Cc: yann.malet@…, clouserw@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

trying something like :

@user_passes_test(lambda u: my_function(u),login_url = reverse('my_reverse_key'))
def my view(request)
   pass
   ...

yields an error when importing "urls.py" (i.e.really quickly)

Attachments (5)

5925.diff (3.2 KB ) - added by Chris Beaven 14 years ago.
5925.2.diff (3.3 KB ) - added by Preston Timmons 13 years ago.
5925.2.2.diff (3.3 KB ) - added by Preston Timmons 13 years ago.
5925.reverse_lazy.diff (4.1 KB ) - added by Julien Phalip 13 years ago.
5925.reverse_lazy.2.diff (5.8 KB ) - added by Julien Phalip 13 years ago.
Added more tests for the original use case of this ticket (i.e. user_passes_test)

Download all attachments as: .zip

Change History (35)

comment:1 by yml, 16 years ago

Cc: yann.malet@… added

One other side effect of the issue describes above is that it breaks the url resolution for reverse, {%url%}, permalink and company.
I have the case in one of my application where some views are protected by :
"""
@user_passes_test(lambda u: Group.objects.get(name='admin') in u.groups.all(), login_url=reverse("auth_login"))
"""
This lead to some nasty bugs in the url resolution of an other view.

I hope this description will help.

comment:2 by anonymous, 16 years ago

Keywords: url permalink added

comment:3 by anonymous, 16 years ago

Keywords: urlresolver added
Summary: Unable to use url reverse feature in redirect argument of decorator user_passes_testUnable to use urlresolvers (url,reverse ) feature in redirect argument of decorator user_passes_test

comment:4 by Eratosfen, 16 years ago

It looks like problem concerns not only user_passes_test. reverse() can't be used at all decorators of views.
Since the decorators of your views are evaluated during parsing urls.py you have an 'chicken - egg' problem. The method reverse() can't be used since urls.py is not read.

The solution, I see, is to evaluate the reverse() lazy.

comment:5 by yml, 16 years ago

I do not know if this information can help. I have observed that @cache (decorator) does not trigger any error message.

comment:6 by Thomas Güttler <hv@…>, 16 years ago

Cc: hv@… added
Triage Stage: UnreviewedAccepted

I have this problem, too.

Here is my solution:

# file lazy.py
from django.core import urlresolvers

class lazy_string(object):
    def __init__(self, function, *args, **kwargs):
        self.function=function
        self.args=args
        self.kwargs=kwargs
        
    def __str__(self):
        if not hasattr(self, 'str'):
            self.str=self.function(*self.args, **self.kwargs)
        return self.str

def reverse(*args, **kwargs):
    return lazy_string(urlresolvers.reverse, *args, **kwargs)

Just use

from lazy import reverse

instead of

from django.core.urlresovlers import reverse

comment:7 by Malcolm Tredinnick, 16 years ago

Should be possible to solve this using our existing django.utils.functional.lazy() function to create lazy_reverse. Reinventing that wheel is something to avoid. reverse() is a function that returns a unicode object (we should ensure that it does return unicode and not str). So there are a few things to do here: auditing existing reverse, hooking up reverse_lazy, documentation, tests, etc, but that sounds like a plan. At least in my head.

comment:8 by Thomas Güttler, 16 years ago

I think it is better to alter reverse() than to invent a new method lazy_reverse(). The recursive import error results
in very strange Exceptions: Modules are missing methods/attributes that are in the source code. It took me some time
to find out what's wrong, although I am using Python since seven years.

comment:9 by Antti Kaihola, 15 years ago

This urls.py works as expected (redirects from / to /comehere/), so django.utils.functional.lazy() seems to work as mtredinnick suggested:

from django.conf.urls.defaults import *
from django.core.urlresolvers import reverse
from django.utils.functional import lazy
from django.http import HttpResponse

reverse_lazy = lazy(reverse, unicode)

urlpatterns = patterns('',
    url(r'^comehere/', lambda request: HttpResponse('Welcome!'), name='comehere'),
    url(r'^$', 'django.views.generic.simple.redirect_to',
        {'url': reverse_lazy('comehere')}, name='root')
)

comment:10 by Thejaswi Puthraya, 15 years ago

Component: UncategorizedCore framework

by Chris Beaven, 14 years ago

Attachment: 5925.diff added

comment:11 by Chris Beaven, 14 years ago

Owner: changed from nobody to Chris Beaven
Status: newassigned

Patch provides a new reverse_lazy function (with documentation), and changes the user_passes_test decorator function to not use the login_url at runtime.

I'm not quite sure how you'd test this, since it'd require testing it before the URL names map is loaded.

comment:12 by Chris Beaven, 14 years ago

PS: for anyone code reviewing, yes str is the right type instead of unicode, since reverse passes through iri_to_uri which converts it to a string type.

comment:13 by Chris Beaven, 14 years ago

Has patch: set

If anyone needs this functionality currently, here's a stand-alone object I created which works well enough:

from django.core.urlresolvers import reverse
from django.utils.functional import LazyObject


class ReverseLazy(LazyObject):
    """
    A lazily evaluated URL reversing object.

    Takes identical arguments to the ``reverse`` method in
    ``django.core.urlresolvers``.

    """

    def __init__(self, viewname, *args, **kwargs):
        self.__dict__['_reverse_args'] = (viewname,) + args
        self.__dict__['_reverse_kwargs'] = kwargs
        super(ReverseLazy, self).__init__()

    def _setup(self):
        self._wrapped = reverse(*self._reverse_args, **self._reverse_kwargs)

    def __nonzero__(self):
        """
        Always assuming a boolean check passes.

        This avoids one test in the ``user_passes_test`` decorator generator 
        in ``django.contrib.auth``.

        """
        return True

    def __str__(self):
        self._setup()
        return str(self._wrapped)

comment:14 by Wil Clouser, 14 years ago

Cc: clouserw@… added

comment:15 by Eric Holscher, 13 years ago

milestone: 1.3
Needs tests: set

Lets fix this for 1.3, it has a patch from SmileyChris, who's now a core dev, so that should make it a bit easier :)

refs: http://twitter.com/bkmontgomery/status/28421302314

comment:16 by Sean Brant, 13 years ago

I needed something like this for generic pagination templates that take want to use {% url %} but have variable viewnames and args. Probably not the best way to handle it but this is how I did it, not sure if it helps.

Here is the LazyReverse class::

    from django.core.urlresolvers import reverse

    class LazyReverse(object):
        def __init__(self, viewname, urlconf=None, args=None,
                kwargs=None, prefix=None, current_app=None):
            self.viewname = viewname
            self.urlconf = urlconf
            self.args = args or []
            self.kwargs = kwargs or {}
            self.prefix = prefix
            self.current_app = current_app

        def reverse(self, *extra_args, **extra_kwargs):
            args = []
            args.extend(self.args)
            args.extend(extra_args)
            kwargs = {}
            kwargs.update(self.kwargs)
            kwargs.update(extra_kwargs)
            return reverse(self.viewname, self.urlconf, args,
                kwargs, self.prefix, self.current_app)

In your view you would do::

    context['reverse_obj'] = LazyReverse('myview', key=val)

In your template::

    {% lazy_url reverse_obj extra_arg %}

comment:17 by Chris Beaven, 13 years ago

(In [14733]) Fixes #11025 -- ability to specify LOGIN_URL as full qualified absolute URL.

auth.views.login now allows for login redirections for different schemes
with the same host (or no host even, e.g. 'https:///login/')

auth.decorators.login_required can now use lazy urls (refs #5925)

comment:18 by Chris Beaven, 13 years ago

Patch needs improvement: set
Triage Stage: AcceptedDesign decision needed

Well, the "fix" is now as simple as a one line code change and documentation. I'm going to bump to a design decision as to whether or not we should provide the lazy_reverse method.

I think we should, since it may not be obvious that it should be lazy(reverse, str) rather than .., unicode

comment:19 by Simon Charette, 13 years ago

Is this going to make 1.3? It's really useful when extending ModelAdmin and defining named urls within get_urls.

comment:20 by Preston Timmons, 13 years ago

Needs tests: unset
Patch needs improvement: unset

Attached is a patch updated for Django 1.3. We also added tests. We removed the changes that Chris made to the auth decorators, which didn't apply cleanly, and didn't seem to effect this patch.

by Preston Timmons, 13 years ago

Attachment: 5925.2.diff added

by Preston Timmons, 13 years ago

Attachment: 5925.2.2.diff added

comment:21 by Preston Timmons, 13 years ago

The patch 5925.2.diff is corrupt. The second patch was meant to replace it.

comment:22 by Gabriel Hurley, 13 years ago

Severity: Normal
Type: Cleanup/optimization

by Julien Phalip, 13 years ago

Attachment: 5925.reverse_lazy.diff added

comment:23 by Julien Phalip, 13 years ago

Easy pickings: unset
Triage Stage: Design decision neededAccepted

I agree with SmileyChris that reverse_lazy() is the right approach as it is more explicit, thus marking this ticket as accepted. I've updated the patch with tests using class-based views instead of the deprecated "simple" ones, also tweaked the doc to reflect that, and added release notes. I'm hoping this gets checked in as it would help tickets like #15294 move forward. Not marking RFC yet as I'd like to have another pair of eyes review this patch.

by Julien Phalip, 13 years ago

Attachment: 5925.reverse_lazy.2.diff added

Added more tests for the original use case of this ticket (i.e. user_passes_test)

comment:24 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: assignedclosed

In [16121]:

(The changeset message doesn't reference this ticket)

comment:25 by Jannis Leidel, 13 years ago

In [16122]:

Added file forgotten in r16121. Refs #5925. Thanks, Julien.

comment:26 by tomv2, 13 years ago

It's only a name in a test but in [16121] was LazyRedictView intending to be spelt LazyRedirectView?

in reply to:  26 comment:27 by Julien Phalip, 13 years ago

Replying to tomv2:

It's only a name in a test but in [16121] was LazyRedictView intending to be spelt LazyRedirectView?

Yes, this is a minor typo.

comment:28 by Jannis Leidel, 13 years ago

In [16217]:

Fixed minor typo in tests introduced in r16121. Refs #5925.

comment:29 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:30 by Thomas Güttler, 12 years ago

Cc: hv@… removed
UI/UX: unset
Note: See TracTickets for help on using tickets.
Back to Top