Fixed #12804 - regression with decorating admin views.

This is a BACKWARDS INCOMPATIBLE change, because it removes the flawed
'auto_adapt_to_methods' decorator, and replaces it with 'method_decorator'
which must be applied manually when necessary, as described in the 1.2
release notes.

For users of 1.1 and 1.0, this affects the decorators:

 * login_required
 * permission_required
 * user_passes_test

For those following trunk, this also affects:

 * csrf_protect
 * anything created with decorator_from_middleware 

If a decorator does not depend on the signature of the function it is
supposed to decorate (for example if it only does post-processing of the
result), it will not be affected.
 



git-svn-id: http://code.djangoproject.com/svn/django/trunk@12399 bcc190cf-cafb-0310-a4f2-bffc1f526a37

django/contrib/admin/options.py

@@ -13,6 +13,7 @@ from django.db import models, transaction
 from django.db.models.fields import BLANK_CHOICE_DASH
 from django.http import Http404, HttpResponse, HttpResponseRedirect
 from django.shortcuts import get_object_or_404, render_to_response
+from django.utils.decorators import method_decorator
 from django.utils.datastructures import SortedDict
 from django.utils.functional import update_wrapper
 from django.utils.html import escape
@@ -53,6 +54,7 @@ FORMFIELD_FOR_DBFIELD_DEFAULTS = {
     models.FileField:       {'widget': widgets.AdminFileWidget},
 }
 
+csrf_protect_m = method_decorator(csrf_protect)
 
 class BaseModelAdmin(object):
     """Functionality common to both ModelAdmin and InlineAdmin."""
@@ -754,7 +756,7 @@ class ModelAdmin(BaseModelAdmin):
             msg = _("No action selected.")
             self.message_user(request, msg)
 
-    @csrf_protect
+    @csrf_protect_m
     @transaction.commit_on_success
     def add_view(self, request, form_url='', extra_context=None):
         "The 'add' admin view for this model."
@@ -844,7 +846,7 @@ class ModelAdmin(BaseModelAdmin):
         context.update(extra_context or {})
         return self.render_change_form(request, context, form_url=form_url, add=True)
 
-    @csrf_protect
+    @csrf_protect_m
     @transaction.commit_on_success
     def change_view(self, request, object_id, extra_context=None):
         "The 'change' admin view for this model."
@@ -936,7 +938,7 @@ class ModelAdmin(BaseModelAdmin):
         context.update(extra_context or {})
         return self.render_change_form(request, context, change=True, obj=obj)
 
-    @csrf_protect
+    @csrf_protect_m
     def changelist_view(self, request, extra_context=None):
         "The 'change list' admin view for this model."
         from django.contrib.admin.views.main import ERROR_FLAG
@@ -1057,7 +1059,7 @@ class ModelAdmin(BaseModelAdmin):
             'admin/change_list.html'
         ], context, context_instance=context_instance)
 
-    @csrf_protect
+    @csrf_protect_m
     def delete_view(self, request, object_id, extra_context=None):
         "The 'delete' admin view for this model."
         opts = self.model._meta

django/contrib/auth/admin.py

@@ -10,9 +10,12 @@ from django.http import HttpResponseRedirect, Http404
 from django.shortcuts import render_to_response, get_object_or_404
 from django.template import RequestContext
 from django.utils.html import escape
+from django.utils.decorators import method_decorator
 from django.utils.translation import ugettext, ugettext_lazy as _
 from django.views.decorators.csrf import csrf_protect
 
+csrf_protect_m = method_decorator(csrf_protect)
+
 class GroupAdmin(admin.ModelAdmin):
     search_fields = ('name',)
     ordering = ('name',)
@@ -76,7 +79,7 @@ class UserAdmin(admin.ModelAdmin):
             (r'^(\d+)/password/$', self.admin_site.admin_view(self.user_change_password))
         ) + super(UserAdmin, self).get_urls()
 
-    @csrf_protect
+    @csrf_protect_m
     @transaction.commit_on_success
     def add_view(self, request, form_url='', extra_context=None):
         # It's an error for a user to have add permission but NOT change

django/contrib/auth/decorators.py

@@ -6,7 +6,7 @@ except ImportError:
 from django.contrib.auth import REDIRECT_FIELD_NAME
 from django.http import HttpResponseRedirect
 from django.utils.http import urlquote
-from django.utils.decorators import auto_adapt_to_methods
+
 
 def user_passes_test(test_func, login_url=None, redirect_field_name=REDIRECT_FIELD_NAME):
     """
@@ -26,7 +26,8 @@ def user_passes_test(test_func, login_url=None, redirect_field_name=REDIRECT_FIE
             tup = login_url, redirect_field_name, path
             return HttpResponseRedirect('%s?%s=%s' % tup)
         return wraps(view_func)(_wrapped_view)
-    return auto_adapt_to_methods(decorator)
+    return decorator
+
 
 def login_required(function=None, redirect_field_name=REDIRECT_FIELD_NAME):
     """
@@ -41,6 +42,7 @@ def login_required(function=None, redirect_field_name=REDIRECT_FIELD_NAME):
         return actual_decorator(function)
     return actual_decorator
 
+
 def permission_required(perm, login_url=None):
     """
     Decorator for views that checks whether a user has a particular permission

django/contrib/formtools/wizard.py

@@ -14,8 +14,10 @@ from django.template.context import RequestContext
 from django.utils.hashcompat import md5_constructor
 from django.utils.translation import ugettext_lazy as _
 from django.contrib.formtools.utils import security_hash
+from django.utils.decorators import method_decorator
 from django.views.decorators.csrf import csrf_protect
 
+
 class FormWizard(object):
     # Dictionary of extra template context variables.
     extra_context = {}
@@ -45,7 +47,7 @@ class FormWizard(object):
         # hook methods might alter self.form_list.
         return len(self.form_list)
 
-    @csrf_protect
+    @method_decorator(csrf_protect)
     def __call__(self, request, *args, **kwargs):
         """
         Main method that does all the hard work, conforming to the Django view

django/utils/decorators.py

@@ -7,44 +7,24 @@ except ImportError:
     from django.utils.functional import wraps, update_wrapper  # Python 2.3, 2.4 fallback.
 
 
-# Licence for MethodDecoratorAdaptor and auto_adapt_to_methods
+def method_decorator(decorator):
-#
+    """
-# This code is taken from stackoverflow.com [1], the code being supplied by
+    Converts a function decorator into a method decorator
-# users 'Ants Aasma' [2] and 'Silent Ghost' [3] with modifications.  It is
+    """
-# legally included here under the terms of the Creative Commons
+    def _dec(func):
-# Attribution-Share Alike 2.5 Generic Licence [4]
+        def _wrapper(self, *args, **kwargs):
-#
+            def bound_func(*args2, **kwargs2):
-# [1] http://stackoverflow.com/questions/1288498/using-the-same-decorator-with-arguments-with-functions-and-methods
+                return func(self, *args2, **kwargs2)
-# [2] http://stackoverflow.com/users/107366/ants-aasma
+            # bound_func has the signature that 'decorator' expects i.e.  no
-# [3] http://stackoverflow.com/users/12855/silentghost
+            # 'self' argument, but it is a closure over self so it can call
-# [4] http://creativecommons.org/licenses/by-sa/2.5/
+            # 'func' correctly.
+            return decorator(bound_func)(*args, **kwargs)
+        return wraps(func)(_wrapper)
+    update_wrapper(_dec, decorator)
+    # Change the name to aid debugging.
+    _dec.__name__ = 'method_dec(%s)' % decorator.__name__
+    return _dec
 
-class MethodDecoratorAdaptor(object):
-    """
-    Generic way of creating decorators that adapt to being
-    used on methods
-    """
-    def __init__(self, decorator, func):
-        update_wrapper(self, func)
-        # NB: update the __dict__ first, *then* set
-        # our own .func and .decorator, in case 'func' is actually
-        # another MethodDecoratorAdaptor object, which has its
-        # 'func' and 'decorator' attributes in its own __dict__
-        self.decorator = decorator
-        self.func = func
-    def __call__(self, *args, **kwargs):
-        return self.decorator(self.func)(*args, **kwargs)
-    def __get__(self, instance, owner):
-        return self.decorator(self.func.__get__(instance, owner))
-
-def auto_adapt_to_methods(decorator):
-    """
-    Takes a decorator function, and returns a decorator-like callable that can
-    be used on methods as well as functions.
-    """
-    def adapt(func):
-        return MethodDecoratorAdaptor(decorator, func)
-    return wraps(decorator)(adapt)
 
 def decorator_from_middleware_with_args(middleware_class):
     """
@@ -61,6 +41,7 @@ def decorator_from_middleware_with_args(middleware_class):
     """
     return make_middleware_decorator(middleware_class)
 
+
 def decorator_from_middleware(middleware_class):
     """
     Given a middleware class (not an instance), returns a view decorator. This
@@ -69,6 +50,7 @@ def decorator_from_middleware(middleware_class):
     """
     return make_middleware_decorator(middleware_class)()
 
+
 def make_middleware_decorator(middleware_class):
     def _make_decorator(*m_args, **m_kwargs):
         middleware = middleware_class(*m_args, **m_kwargs)
@@ -96,5 +78,5 @@ def make_middleware_decorator(middleware_class):
                         return result
                 return response
             return wraps(view_func)(_wrapped_view)
-        return auto_adapt_to_methods(_decorator)
+        return _decorator
     return _make_decorator

django/views/decorators/cache.py

@@ -16,10 +16,11 @@ try:
 except ImportError:
     from django.utils.functional import wraps  # Python 2.3, 2.4 fallback.
 
-from django.utils.decorators import decorator_from_middleware_with_args, auto_adapt_to_methods
+from django.utils.decorators import decorator_from_middleware_with_args
 from django.utils.cache import patch_cache_control, add_never_cache_headers
 from django.middleware.cache import CacheMiddleware
 
+
 def cache_page(*args, **kwargs):
     # We need backwards compatibility with code which spells it this way:
     #   def my_view(): pass
@@ -48,18 +49,16 @@ def cache_page(*args, **kwargs):
     else:
         return decorator_from_middleware_with_args(CacheMiddleware)(cache_timeout=args[0], key_prefix=key_prefix)
 
+
 def cache_control(**kwargs):
-
     def _cache_controller(viewfunc):
-
         def _cache_controlled(request, *args, **kw):
             response = viewfunc(request, *args, **kw)
             patch_cache_control(response, **kwargs)
             return response
-
         return wraps(viewfunc)(_cache_controlled)
+    return _cache_controller
 
-    return auto_adapt_to_methods(_cache_controller)
 
 def never_cache(view_func):
     """
@@ -71,4 +70,3 @@ def never_cache(view_func):
         add_never_cache_headers(response)
         return response
     return wraps(view_func)(_wrapped_view_func)
-never_cache = auto_adapt_to_methods(never_cache)

docs/releases/1.2.txt

@@ -251,6 +251,55 @@ incompatibilities, especially if you are storing comma or semi-colon in
 cookies and have javascript code that parses and manipulates cookie values
 client-side.
 
+``user_passes_test``, ``login_required`` and ``permission_required``
+--------------------------------------------------------------------
+
+``django.contrib.auth.decorators`` provides the decorators ``login_required``,
+``permission_required`` and ``user_passes_test``. Previously it was possible to
+use these decorators both on functions (where the first argument is 'request')
+and on methods (where the first argument is 'self', and the second argument is
+'request'). However, we have found that the trick which enabled this is
+flawed. It only works in limited circumstances, and produces errors that are
+very difficult to debug when it does not work.
+
+For this reason, the 'auto adapt' behaviour has been removed, and if you are
+using these decorators on methods, you will need to manually apply
+:func:`django.utils.decorators.method_decorator` to convert the decorator to one
+that works with methods. You would change code from this::
+
+    class MyClass(object):
+
+        @login_required
+        def my_view(self, request):
+            pass
+
+to this::
+
+    from django.utils.decorators import method_decorator
+
+    class MyClass(object):
+
+        @method_decorator(login_required)
+        def my_view(self, request):
+            pass
+
+or::
+
+    from django.utils.decorators import method_decorator
+
+    login_required_m = method_decorator(login_required)
+
+    class MyClass(object):
+
+        @login_required_m
+        def my_view(self, request):
+            pass
+
+For those following trunk, this change also applies to other decorators
+introduced since 1.1, including ``csrf_protect``, ``cache_control`` and anything
+created using ``decorator_from_middleware``.
+
+
 .. _deprecated-features-1.2:
 
 Features deprecated in 1.2

tests/modeltests/test_client/views.py

@@ -7,6 +7,7 @@ from django.contrib.auth.decorators import login_required, permission_required
 from django.forms.forms import Form
 from django.forms import fields
 from django.shortcuts import render_to_response
+from django.utils.decorators import method_decorator
 
 def get_view(request):
     "A simple view that expects a GET request, and returns a rendered template"
@@ -147,14 +148,15 @@ def permission_protected_view(request):
 permission_protected_view = permission_required('modeltests.test_perm')(permission_protected_view)
 
 class _ViewManager(object):
+    @method_decorator(login_required)
     def login_protected_view(self, request):
         t = Template('This is a login protected test using a method. '
                      'Username is {{ user.username }}.',
                      name='Login Method Template')
         c = Context({'user': request.user})
         return HttpResponse(t.render(c))
-    login_protected_view = login_required(login_protected_view)
 
+    @method_decorator(permission_required('modeltests.test_perm'))
     def permission_protected_view(self, request):
         t = Template('This is a permission protected test using a method. '
                      'Username is {{ user.username }}. '
@@ -162,7 +164,6 @@ class _ViewManager(object):
                      name='Permissions Template')
         c = Context({'user': request.user})
         return HttpResponse(t.render(c))
-    permission_protected_view = permission_required('modeltests.test_perm')(permission_protected_view)
 
 _view_manager = _ViewManager()
 login_protected_method_view = _view_manager.login_protected_view

tests/regressiontests/decorators/tests.py

@@ -10,7 +10,7 @@ from django.utils.functional import allow_lazy, lazy, memoize
 from django.views.decorators.http import require_http_methods, require_GET, require_POST
 from django.views.decorators.vary import vary_on_headers, vary_on_cookie
 from django.views.decorators.cache import cache_page, never_cache, cache_control
-from django.utils.decorators import auto_adapt_to_methods
+from django.utils.decorators import method_decorator
 from django.contrib.auth.decorators import login_required, permission_required, user_passes_test
 from django.contrib.admin.views.decorators import staff_member_required
 
@@ -47,6 +47,7 @@ fully_decorated = memoize(fully_decorated, {}, 1)
 fully_decorated = allow_lazy(fully_decorated)
 fully_decorated = lazy(fully_decorated)
 
+
 class DecoratorsTest(TestCase):
 
     def test_attributes(self):
@@ -112,42 +113,25 @@ class DecoratorsTest(TestCase):
         my_view_cached2 = cache_page(my_view, 123, key_prefix="test")
         self.assertEqual(my_view_cached2(HttpRequest()), "response")
 
-class MethodDecoratorAdapterTests(TestCase):
+
-    def test_auto_adapt_to_methods(self):
+# For testing method_decorator, a decorator that assumes a single argument.
+# We will get type arguments if there is a mismatch in the number of arguments.
+def simple_dec(func):
+    def wrapper(arg):
+        return func("test:" + arg)
+    return wraps(func)(wrapper)
+
+simple_dec_m = method_decorator(simple_dec)
+
+
+class MethodDecoratorTests(TestCase):
     """
-        Test that auto_adapt_to_methods actually works.
+    Tests for method_decorator
     """
-        # Need 2 decorators with auto_adapt_to_methods,
+    def test_method_decorator(self):
-        # to check it plays nicely with composing itself.
+        class Test(object):
+            @simple_dec_m
+            def say(self, arg):
+                return arg
 
-        def my_decorator(func):
+        self.assertEqual("test:hello", Test().say("hello"))
-            def wrapped(*args, **kwargs):
-                # need to ensure that the first arg isn't 'self'
-                self.assertEqual(args[0], "test")
-                return "my_decorator:" + func(*args, **kwargs)
-            wrapped.my_decorator_custom_attribute = True
-            return wraps(func)(wrapped)
-        my_decorator = auto_adapt_to_methods(my_decorator)
-
-        def my_decorator2(func):
-            def wrapped(*args, **kwargs):
-                # need to ensure that the first arg isn't 'self'
-                self.assertEqual(args[0], "test")
-                return "my_decorator2:" + func(*args, **kwargs)
-            wrapped.my_decorator2_custom_attribute = True
-            return wraps(func)(wrapped)
-        my_decorator2 = auto_adapt_to_methods(my_decorator2)
-
-        class MyClass(object):
-            def my_method(self, *args, **kwargs):
-                return "my_method:%r %r" % (args, kwargs)
-            my_method = my_decorator2(my_decorator(my_method))
-
-        obj = MyClass()
-        self.assertEqual(obj.my_method("test", 123, name='foo'),
-                         "my_decorator2:my_decorator:my_method:('test', 123) {'name': 'foo'}")
-        self.assertEqual(obj.my_method.__name__, 'my_method')
-        self.assertEqual(getattr(obj.my_method, 'my_decorator_custom_attribute', False),
-                         True)
-        self.assertEqual(getattr(obj.my_method, 'my_decorator2_custom_attribute', False),
-                         True)