Code

Opened 6 years ago

Closed 3 years ago

Last modified 2 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: SmileyChris
Component: Core (Other) Version: master
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 SmileyChris 4 years ago.
5925.2.diff (3.3 KB) - added by prestontimmons 3 years ago.
5925.2.2.diff (3.3 KB) - added by prestontimmons 3 years ago.
5925.reverse_lazy.diff (4.1 KB) - added by julien 3 years ago.
5925.reverse_lazy.2.diff (5.8 KB) - added by julien 3 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 Changed 6 years ago by yml

  • Cc yann.malet@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

  • Keywords reverse,decorator,url,permalink added; reverse,decorator removed

comment:3 Changed 6 years ago by anonymous

  • Keywords urlresolver,reverse,decorator,url,permalink added; reverse,decorator,url,permalink removed
  • Summary changed from Unable to use url reverse feature in redirect argument of decorator user_passes_test to Unable to use urlresolvers (url,reverse ) feature in redirect argument of decorator user_passes_test

comment:4 Changed 6 years ago by Eratosfen

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

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

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

  • Cc hv@… added
  • Triage Stage changed from Unreviewed to Accepted

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

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

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

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

  • Component changed from Uncategorized to Core framework

Changed 4 years ago by SmileyChris

comment:11 Changed 4 years ago by SmileyChris

  • Owner changed from nobody to SmileyChris
  • Status changed from new to assigned

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

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

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

  • Cc clouserw@… added

comment:15 Changed 3 years ago by ericholscher

  • milestone set to 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 Changed 3 years ago by seanbrant

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

(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 Changed 3 years ago by SmileyChris

  • Patch needs improvement set
  • Triage Stage changed from Accepted to Design 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 Changed 3 years ago by charettes

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

comment:20 Changed 3 years ago by prestontimmons

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

Changed 3 years ago by prestontimmons

Changed 3 years ago by prestontimmons

comment:21 Changed 3 years ago by prestontimmons

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

comment:22 Changed 3 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Cleanup/optimization

Changed 3 years ago by julien

comment:23 Changed 3 years ago by julien

  • Easy pickings unset
  • Triage Stage changed from Design decision needed to Accepted

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.

Changed 3 years ago by julien

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

comment:24 Changed 3 years ago by jezdez

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

In [16121]:

Fixed #5925 -- Added new lazily evaluated version of django.core.urlresolvers.reverse. Thanks, SmileyChris, Preston Timmons and Julien Phalip.

comment:25 Changed 3 years ago by jezdez

In [16122]:

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

comment:26 follow-up: Changed 3 years ago by tomv2

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

comment:27 in reply to: ↑ 26 Changed 3 years ago by julien

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

In [16217]:

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

comment:29 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:30 Changed 2 years ago by guettli

  • Cc hv@… removed
  • UI/UX unset

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.