Warning: Can't synchronize with repository "(default)" (/home/git/ome.git does not appear to be a Git repository.). Look in the Trac log for more information.
Notice: In order to edit this ticket you need to be either: a Product Owner, The owner or the reporter of the ticket, or, in case of a Task not yet assigned, a team_member"

Task #11732 (closed)

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

@login_required __name__

Reported by: wmoore Owned by: drussell-x
Priority: major Milestone: 5.0.0-rc1
Component: Web Version: n.a.
Keywords: n.a. Cc: cxallan, atarkowska, jamoore, cneves, douglas.russell@…
Resources: n.a. Referenced By: n.a.
References: n.a. Remaining Time: n.a.
Sprint: n.a.

Description

If you want to use the class-based login_required decorator to decorate class-based views using Django's @method_decorator() https://docs.djangoproject.com/en/1.5/topics/class-based-views/intro/#decorating-the-class

like

    @method_decorator(login_required())
    def dispatch(self, *args, **kwargs):

Then this fails with:

Traceback:
File "/Users/dpwrussell/Checkout/openmicroscopy/dist/lib/python/django/core/handlers/base.py" in get_response
  101.                             request.path_info)
File "/Users/dpwrussell/Checkout/openmicroscopy/dist/lib/python/django/core/urlresolvers.py" in resolve
  252.                     sub_match = pattern.resolve(new_path)
File "/Users/dpwrussell/Checkout/openmicroscopy/dist/lib/python/django/core/urlresolvers.py" in resolve
  250.             for pattern in self.url_patterns:
File "/Users/dpwrussell/Checkout/openmicroscopy/dist/lib/python/django/core/urlresolvers.py" in _get_url_patterns
  279.         patterns = getattr(self.urlconf_module, "urlpatterns", self.urlconf_module)
File "/Users/dpwrussell/Checkout/openmicroscopy/dist/lib/python/django/core/urlresolvers.py" in _get_urlconf_module
  274.             self._urlconf_module = import_module(self.urlconf_name)
File "/Users/dpwrussell/Checkout/openmicroscopy/dist/lib/python/django/utils/importlib.py" in import_module
  35.     __import__(name)
File "/Users/dpwrussell/Checkout/openmicroscopy/components/tools/OmeroWeb/omeroweb/../omeroweb/webadmin/urls.py" in <module>
  31. from omeroweb.webadmin import views
File "/Users/dpwrussell/Checkout/openmicroscopy/components/tools/OmeroWeb/omeroweb/../omeroweb/webadmin/views.py" in <module>
  821. class AnnounceView(FormView):
File "/Users/dpwrussell/Checkout/openmicroscopy/components/tools/OmeroWeb/omeroweb/../omeroweb/webadmin/views.py" in AnnounceView
  845.     @method_decorator(login_required())
File "/Users/dpwrussell/Checkout/openmicroscopy/dist/lib/python/django/utils/decorators.py" in method_decorator
  40.     update_wrapper(_dec, decorator)
File "/usr/local/Cellar/python/2.7.6/Frameworks/Python.framework/Versions/2.7/lib/python2.7/functools.py" in update_wrapper
  33.         setattr(wrapper, attr, getattr(wrapped, attr))

Exception Type: AttributeError at /webadmin/announce/
Exception Value: 'login_required' object has no attribute '__name__'

But works with

lr = login_required()
lr.__name__ = "login_required"
@method_decorator(lr)
def dispatch(self, *args, **kwargs):

So, we simply need to add the name attribute to the login_required class.

Change History (10)

comment:1 Changed 10 years ago by wmoore

From Douglas

ok, this is what you do
>>> import functools
>>> class memoized(object):
    """Decorator that caches a function's return value each time it is called.
    If called later with the same arguments, the cached value is returned, and
    not re-evaluated.
    """
    def __init__(self, func):
        self.func = func
        self.cache = {}
        functools.update_wrapper(self, func)  ## TA-DA! ##
    def __call__(self, *args):
        pass  # Not needed for this demo.

but I think you can use functools there to make the correction
in your __call__

At the same time, you might want to update the imports
to use functools
as the omero ones are importing a legacy import: from django.utils.functional import wraps

comment:2 Changed 10 years ago by drussell-x

I couldn't understand why this error wasn't being hit by anyone else, and I think I've figured out why. Initially we were looking at why the function returned by login_required did not have the expected name method. It does. The problem is that the login_required is initialised, but not called at that point.

When wrapping the object, the functools update_wrapper is trying to copy all the important stuff from the decorator it is passed. Which is login_required(). When this decorator is used, it will execute call and correctly return the true wrapper. But when constructing the class, the method_decorator will do this:

https://github.com/django/django/blob/master/django/utils/decorators.py#L39

which will be looking at login_required, not the method defined when called. Thus it is missing name.

Test cases here: https://gist.github.com/dpwrussell/7573003 (test1 is non-working, test2 has a bodge to make it work)

Still experimenting with the best way to solve this.

comment:3 Changed 10 years ago by wmoore

Hi Douglas,

After playing with your test2 a little bit, I think that your 'bodge' is the only real way to solve it. The reason that the other examples you've seen are working OK is because the class itself, that you're passing to method_decorator() does have a name:

MyDecorator.__name__

but in our case, the instance of the class doesn't (by default) have a name

MyDecorator().__name__

so we have to provide one.
In neither case does the name match the name of the function we're actually wrapping - so I don't think it matters too much.
Using update_wrapper(wrapped, f) as you've done seems like a good idea too, although it's not related to the error we're seeing.

I would just go with your solution in test2, with the name matching the class name, E.g. "MyDecorator?" or "login_required". Works fine for me.

comment:4 Changed 10 years ago by drussell-x

Alternatively, try this:

https://github.com/dpwrussell/openmicroscopy/tree/admin_utils

I haven't replace login_required, I've added login_required_dpwr so both can be used side by side to test this out. I tried it on my CBV and on a function, seems to work fine in both cases as either:

@login_required_dpwr
def load_drivespace(request, conn=None, **kwargs):

or

class AnnounceView(FormView):
# [...]
    @method_decorator(login_required_dpwr)
    def dispatch(self, *args, **kwargs):
        return super(AnnounceView, self).dispatch(*args, **kwargs)

I can't see how it is functionality any different from what is happening at the moment, except that it works with @method_decorator and simplifies the login_required decorator a bit.

comment:5 Changed 10 years ago by wmoore

With the alternative approach above, you can't configure the decorator differently for each view method.
Your init is

def __init__(self, func, useragent='OMERO.web', isAdmin=False,
                   isGroupOwner=False, doConnectionCleanup=True, omero_group='-1',
                   allowPublic=None):

but how are you going to specify the optional parameters, E.g. isAdmin = True

E.g. in webclient views.py we have:

@login_required(setGroupContext=True)
@render_response()
def load_data(....):
    ....


@login_required(doConnectionCleanup=False)
def get_original_file(request, fileId, conn=None, **kwargs):
    ....


@login_required(ignore_login_fail=True)
def keepalive_ping(request, conn=None, **kwargs):
   ....


@login_required(login_redirect='webindex')
def logout(request, conn=None, **kwargs):
    ....

comment:6 Changed 10 years ago by drussell-x

Ok, I finally get it. I hadn't recognised that decorators with arguments work in a fundamentally different way than ones without. This post is an excellent explanation: http://www.artima.com/weblogs/viewpost.jsp?thread=240845

I think my initial bodge is the way to go for now, but I will take this up with django as I think the method_decorator should be able to handle this case automatically.

comment:7 Changed 10 years ago by drussell-x

comment:8 Changed 10 years ago by drussell-x

  • Owner changed from wmoore to drussell-x
  • Status changed from new to accepted

comment:9 Changed 10 years ago by drussell-x

  • Resolution set to fixed
  • Status changed from accepted to closed
Note: See TracTickets for help on using tickets. You may also have a look at Agilo extensions to the ticket.

1.3.13-PRO © 2008-2011 Agilo Software all rights reserved (this page was served in: 0.65806 sec.)

We're Hiring!