Code

Opened 6 months ago

Closed 2 months ago

#21247 closed Bug (fixed)

django.utils.method_decorator doesn't honour method binding properly

Reported by: grahamd Owned by: nobody
Component: Utilities Version: 1.5
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The decorators created using django.utils.method_decorator() do not honour method binding properly.

This becomes a problem when such a decorator is used in conjunction with decorators that are implemented as descriptors, implement the __get__() method and expect binding to be performed.

Specifically, if a method decorator created using django.utils.method_decorator() is applied on top of a decorator or wrapper which is implemented as a descriptor then things will fail.

def my_wrapper_1(wrapped):
    def _wrapper(arg1, arg2):
        print('_wrapper', arg1, arg2)
        return wrapped(arg1, arg2)
    return _wrapper

my_method_wrapper_1 = method_decorator(my_wrapper_1)

class my_bound_wrapper_2(object):
    def __init__(self, wrapped):
        self.wrapped = wrapped
        self.__name__ = wrapped.__name__
    def __call__(self, arg1, arg2):
        print('my_bound_wrapper_2.__call__', arg1, arg2)
        return self.wrapped(arg1, arg2)
    def __get__(self, instance, owner):
        print('my_bound_wrapper_2.__get__', instance, owner)
        return self

class my_desc_wrapper_2(object):
    def __init__(self, wrapped):
        self.wrapped = wrapped
        self.__name__ = wrapped.__name__
    def __get__(self, instance, owner):
        print('my_desc_wrapper_2.__get__', instance, owner)
        return my_bound_wrapper_2(self.wrapped.__get__(instance, owner))

class Class(object):

    # Works
    @my_method_wrapper_1
    def method_1(self, arg1, arg2):
        print('Class.method_1', self, arg1, arg2)
        return arg1, arg2

    # Works
    @my_desc_wrapper_2
    def method_2(self, arg1, arg2):
        print('Class.method_2', arg1, arg2)
        return arg1, arg2

    # Fails
    @my_method_wrapper_1
    @my_desc_wrapper_2
    def method_3(self, arg1, arg2):
        print('Class.method_3', arg1, arg2)
        return arg1, arg2

c = Class()

c.method_1(1, 2) # Works.
c.method_2(1, 2) # Works
c.method_3(1, 2) # Fails.

The error one will see in this case is:

Traceback (most recent call last):
  File "runtest-1.py", line 90, in <module>
    c.method_3(1, 2)
  File "runtest-1.py", line 17, in _wrapper
    return bound_func(*args, **kwargs)
  File "runtest-1.py", line 37, in _wrapper
    return wrapped(arg1, arg2)
  File "runtest-1.py", line 13, in bound_func
    return func(self, *args2, **kwargs2)
TypeError: 'my_desc_wrapper_2' object is not callable

This is because the decorator implemented as a decorator doesn't have a __call__() method. It instead expects the method to have been bound correctly to the instance. This would normally mean that __get__() is called to do the binding, which would see an instance of the bound wrapper returned. The __call__() of the bound wrapper would then normally be called.

The django.utils.method_decorator() way of creating method decorators breaks this aspect of how instance method calling is meant to work for Python.

The problem is the code in django.utils.method_decorator() of:

        def _wrapper(self, *args, **kwargs):
            @decorator
            def bound_func(*args2, **kwargs2):
                return func(self, *args2, **kwargs2)
            # bound_func has the signature that 'decorator' expects i.e.  no
            # 'self' argument, but it is a closure over self so it can call
            # 'func' correctly.
            return bound_func(*args, **kwargs)

Although there are better ways of implementing decorators that can be used on both functions and instance methods, a fix for the way things are being done here is to use:

        def _wrapper(self, *args, **kwargs):
            @decorator
            def bound_func(*args2, **kwargs2):
                #return func(self, *args2, **kwargs2)
                return func.__get__(self, type(self))(*args2, **kwargs2)
            # bound_func has the signature that 'decorator' expects i.e.  no
            # 'self' argument, but it is a closure over self so it can call
            # 'func' correctly.
            return bound_func(*args, **kwargs)

That is, instead of simply calling the wrapped function (which is unbound at that point) and pass in the self argument explicitly:

                return func(self, *args2, **kwargs2)

it should bind the function to the instance prior to calling it.

                return func.__get__(self, type(self))(*args2, **kwargs2)

This results in it honouring correctly the binding requirements of instance methods and so it will not break in situations where the decorator is wrapped around another decorator which is implemented as a descriptor.

Attached are runtest-1.py, which fails, and runtest-2.py which has the change and runs correctly.

Attachments (2)

runtest-1.py (3.0 KB) - added by grahamd 6 months ago.
Test with original code and which fails.
runtest-2.py (3.1 KB) - added by grahamd 6 months ago.
Test with modified code and which passes.

Download all attachments as: .zip

Change History (4)

Changed 6 months ago by grahamd

Test with original code and which fails.

Changed 6 months ago by grahamd

Test with modified code and which passes.

comment:1 Changed 6 months ago by timo

  • Component changed from Uncategorized to Utilities
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

comment:2 Changed 2 months ago by Marc Tamlyn <marc.tamlyn@…>

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

In 80cd64ee17191ffb235c5850a8fa60cae96c1b89:

Fixed #21247 -- Made method_decorator play nicely with descriptors

When a method decorator was used in conjunction with a decorator
implemented as a descriptor, method_decorator did not correctly respect
the method binding.

Thanks for Graham Dumpleton for the report and initial patch.

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.