Opened 6 years ago

Last modified 14 months ago

#13879 new New feature

method_decorator doesn't supports decorators with arguments

Reported by: Marinho Brandão Owned by:
Component: Utilities Version: 1.2
Severity: Normal Keywords: sprintnov13, decorators
Cc: Łukasz Rekucki, douglas.russell@…, toracle@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I tried use django.utils.decorators.method_decorator to support decorator permission_required and figured out it's not supporting decorators passing arguments like it does:

@permission_required(something_here)

Instead its code waits for you just pass a function as an only one argument, like this:

@login_required

I made my own version of it, that's below:

from functools import wraps, update_wrapper

def method_decorator(decorator):
    """Converts a function decorator into a method decorator.
    
    This works properly for both: decorators with arguments and without them. The Django's version
    of this function just supports decorators with no arguments."""

    # For simple decorators, like @login_required, without arguments
    def _dec(func):
        def _wrapper(self, *args, **kwargs):
            def bound_func(*args2, **kwargs2):
                return func(self, *args2, **kwargs2)
            return decorator(bound_func)(*args, **kwargs)
        return wraps(func)(_wrapper)

    # Called everytime
    def _args(*argsx, **kwargsx):
        # Detect a simple decorator and call _dec for it
        if len(argsx) == 1 and callable(argsx[0]) and not kwargsx:
            return _dec(argsx[0])

        # Used for decorators with arguments, like @permission_required('something')
        def _dec2(func):
            def _wrapper(self, *args, **kwargs):
                def bound_func(*args2, **kwargs2):
                    return func(self, *args2, **kwargs2)
                return decorator(*argsx, **kwargsx)(bound_func)(*args, **kwargs)
            return wraps(func)(_wrapper)
        return _dec2

    update_wrapper(_args, decorator)
    # Change the name to aid debugging.
    _args.__name__ = 'method_decorator(%s)' % decorator.__name__
    return _args

If necessary I can make a patch and attach to this ticket.

Attachments (1)

ticket13879-r16259.diff (3.9 KB) - added by Andy Durdin 6 years ago.
Patch with fix and tests against trunk revision 16259

Download all attachments as: .zip

Change History (20)

comment:1 Changed 6 years ago by Natalia Bidart

Owner: changed from nobody to Natalia Bidart

comment:2 Changed 6 years ago by Natalia Bidart

Keywords: sprintnov13 decorators added
milestone: 1.3
Owner: changed from Natalia Bidart to nobody
Triage Stage: UnreviewedDesign decision needed

I'm not sure about the goal of this request, so I'll leave the design decision to the core devs.

comment:3 Changed 6 years ago by Łukasz Rekucki

Cc: Łukasz Rekucki added

IMHO, the goal here is to be able to write:

m_perm_required = method_decorator(permission_required) # do this once

class A(View):
   @m_perm_required('spam.mores_spam') # use it later many times without repeating method_decorator 
   def get(self, request, *args, **kwrgs):
     #...

The current implementation handles only decorators without arguments, so you can only write:

   @method_decorator(permission_required('spam.more_eggs')
   def method(self, ...): 
       #...

This is similar to diffrence between "decorator_from_middleware" and "decorator_from_middleware_with_args" and I think the right solution here is to provide a "method_decorator_with_args" function.

PS. The OP is wrong that his implementation handles all cases - there is no universal pattern in Python for transforming decorators with and without arguments if you allow the first argument to be a callable (a class for example). It's probably a matter of taste, so you can just disregard this part of comment :)

comment:4 Changed 6 years ago by Luke Plant

Triage Stage: Design decision neededAccepted

Technically, method_decorator works as advertised. permission_required is not actually a decorator. permission_required('spam.more_eggs') is a decorator, and permission_required is a decorator generator (it returns a decorator when you call it with some arguments).

But I agree that the goal set out lrekucki is a good one, and that a method_decorator_with_args (or some better name) would be desirable, and that the OP's solution is not a good idea.

comment:5 Changed 6 years ago by Graham King

Component: UncategorizedCore (Other)
Severity: Normal
Type: New feature

comment:6 Changed 6 years ago by Andy Durdin

Easy pickings: unset
Owner: changed from nobody to Andy Durdin
Status: newassigned

Changed 6 years ago by Andy Durdin

Attachment: ticket13879-r16259.diff added

Patch with fix and tests against trunk revision 16259

comment:7 Changed 6 years ago by Andy Durdin

Has patch: set
Owner: changed from Andy Durdin to nobody
Status: assignednew

Implemented lukeplant's suggestion of method_decorator_with_args.

comment:8 Changed 6 years ago by Jannis Leidel

Patch needs improvement: set

I'm not terribly happy with the ..with_args suffix (same for decorator_from_middleware_with_args). It feels weird to having to remember two different function names that do basically the same. Especially for decorators that take arguments optionally I'd appreciate if the method_decorator function would simply allow me to pass the arguments for the decorator optionally, too. E.g.::

    @method_decorator(permission_required, 'spam.more_eggs', another_arg=True)
    def method(self, whatever):
        # bla
Last edited 6 years ago by Jannis Leidel (previous) (diff)

comment:9 Changed 6 years ago by Jannis Leidel

Triage Stage: AcceptedDesign decision needed

After talking to adurdin on IRC more about it, I realized that this ticket shows why the "generate a method decorator" pattern is aweful. Marking as DDN.

comment:10 Changed 6 years ago by Luke Plant

@jezdez: My experience with decorators tells me that confusing decorators and decorator factories is always a bad idea, despite the temptation of convenience. If you look at the cache_page decorator, you'll find probably some of the nastiest code in Django, and it was caused by this confusion. You always end up with isinstance checks and horrible little edge cases, and it makes it hard to reason about the code, since it can accept/return fundamentally different types of things.

However, in this case, this code:

    @method_decorator(permission_required, 'spam.more_eggs', another_arg=True)
    def method(self, whatever):

is no improvement over this code:

    @method_decorator(permission_required('spam.more_eggs', another_arg=True))
    def method(self, whatever):

in terms of the original motivation of this ticket, and it violates TOOWTDI.

comment:11 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:11 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 Changed 4 years ago by Aymeric Augustin

Component: Core (Other)Utilities

comment:13 Changed 4 years ago by Aymeric Augustin

Triage Stage: Design decision neededAccepted

I grudgingly acknowledge the opportunity of adding method_decorator_with_args, for consistency with decorator_from_middleware_with_args.

comment:14 Changed 3 years ago by Susan Tan

Owner: changed from nobody to Susan Tan
Status: newassigned

comment:15 Changed 3 years ago by Susan Tan

Applied the attached patch and all the tests pass.

comment:16 Changed 3 years ago by douglas.russell@…

Cc: douglas.russell@… added

comment:17 Changed 3 years ago by anonymous

Owner: Susan Tan deleted
Status: assignednew

comment:18 Changed 14 months ago by Jeongsoo, Park

Cc: toracle@… added
Note: See TracTickets for help on using tickets.
Back to Top