On Monday 08 February 2010 08:21:27 Florian Apolloner wrote: > thx for your quick response, I'll update the ticket and reopen it > (I hope that's okay in this caseā¦)
That's fine, apologies for the premature closing. Here is my analysis, sorry in advance for the length. Unfortunately, we have backwards incompatibilities which ever way we go, as far as I can see. == flaw in @auto_adapt_to_methods == @auto_adapt_to_methods is flawed. It is supposed to allow the same decorator to work with functions: @does_something_with_arg def foo(arg): # ... ...and methods: class Foo(object): @does_something_with_arg def meth(self, arg): # pass Effectively it tries to cope with two different function signatures. However, when a function is defined, you have no idea whether you are 'inside' a class or not, so the decision is delegated to runtime. This is done by wrapping functions in a MethodDecoratorAdaptor object, which: 1) implements __call__, to handle the case of a simple function 2) implements __get__, to handle the case of a method. This works by delaying the application of the decorator until the method is retrieved, at which point we have the 'self' parameter (passed in to __get__) and can create a 'bound' callable which does not have the self parameter in the function signature, and therefore matches the expected function signature. The second of these fails if you use an additional decorator that is not implemented using @auto_adapt_to_methods. Such a decorator will treat the MethodDecoratorAdaptor object as a simple callable, and the descriptor magic never kicks in. The simplest example is as follows: class Foo(object): @my_decorator @does_something_with_arg def meth(self, arg): pass In this case, we might be able to fix @my_decorator simply by decorating it with @auto_adapt_to_methods. But that isn't always possible - in Florian's case, his my_decorator needs access to 'self'. Example code for all of this is attached in 'auto_adapt_to_methods.py'. I don't think this approach is fixable, but I'd be happy to be proved wrong! This problem applies to everything we use @auto_adapt_to_methods on, which includes csrf_protect, login_required, user_passes_test, never_cache and anything created using decorator_from_middleware. == a fix == Instead of trying an automagic 'adapt to methods', we can have a manual 'adapt to methods' decorator, called 'method_dec' (better names welcome, 'method_decorator' seemed a bit long). So we would have: @csrf_protect def my_view(request): # .... class MyClass(object): @method_dec(csrf_protect) def my_view(self, request): # ... or csrf_protect_m = method_dec(csrf_protect) class MyClass(object): @csrf_protect_m def my_view(self, request): # ... I've implemented this in the attached 'method_dec.py'. It doesn't rely on the descriptor protocol, so it should be robust. == the problem == Unfortunately, we have problems and backwards incompatibilities which ever way we go. Although @auto_adapt_to_methods and MethodDecoratorAdaptor were introduced after 1.1 (so we are allowed to remove them), they replace the _CheckLogin class, which served the same purpose, but was specific to the user_passes_test decorator. It used the same methodology (__call__ and __get__ implementation), and so it will suffer from exactly the same flaw. So, if we do nothing, we have the general bug described above, and Florian's bug. In his case, it appears as a backwards incompatibility, as he can no longer do this: class MyModelAdmin(ModelAdmin): change_view = my_decorator(ModelAdmin.change_view) This is because ModelAdmin.change_view is now decorated with csrf_protect, which has the flawed 'auto adapt' behaviour. If we replace the automatic auto_adapt_to_methods with the manual method_dec, then we face: 1) backwards incompatibilities for those following trunk, which is not ideal but allowed. 2) backwards incompatibilities for anyone who has used login_required or user_passes_test on *methods* since 1.0. The release notes would have something like this: <<< user_passes_test and login_required ----------------------------------- Previously, it was possible to use 'login_required' (and other decorators generated using 'user_passes_test') both on functions (where the first argument was 'request') and on methods (where the first argument was 'self', and the second argument was '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 `django.utils.decorators.method_dec` 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_dec class MyClass(object): @method_dec(login_required) def my_view(self, request): or:: from django.utils.decorators import method_dec login_required_m = method_dec(login_required) class MyClass(object): @login_required_m def my_view(self, request): >>> This is kind of annoying, since the solution we had before seems to work pretty well most of the time. When it doesn't, however, it is extremely difficult to debug, and in some cases very tricky to fix. Florian is right that if we leave it as it is, we should at least document it. But we would then be documenting a bug, and my (mental) attempts to come up with documentation for it just reinforce a feeling of ickiness. There is a halfway house position, which involves adding method_dec while deprecating auto_adapt_to_methods, and removing use of it as much as we can. But this would add a layer of complication, and people will have to update their code eventually anyway. What do people think? Luke -- "Yes, wearily I sit here, pain and misery my only companions. And vast intelligence of course. And infinite sorrow. And..." (Marvin the paranoid android) Luke Plant || http://lukeplant.me.uk/ -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
from functools import wraps, update_wrapper 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): if instance is None: return self 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 striparg(func): """ Decorates functions that take a single string argument, and runs strip() on that arg before calling. """ def wrapper(arg): return func(arg.strip()) return wraps(func)(wrapper) striparg_auto = auto_adapt_to_methods(striparg) def simple_decorator(func): """ No-op decorator, without @auto_adapt_to_methods """ def wrapper(*args): return func(*args) return wrapper # By definition this can't use @auto_adapt_to_methods def sign(method): """ Decorates *methods* that take a single argument and return a string. """ def wrapper(self, arg): return method(self, arg) + " (says " + self.__class__.__name__ + ")" return wraps(method)(wrapper) @striparg_auto def insult(arg): return arg + " sucks!" class Stuff(object): @striparg_auto def deny(self, arg): return "No you can't have " + arg class MoreStuff(Stuff): deny = Stuff.deny deny2 = simple_decorator(Stuff.deny) deny3 = sign(Stuff.deny) deny4 = striparg_auto(Stuff.deny) @simple_decorator def deny5(self, arg): return super(MoreStuff, self).deny(arg) @sign def deny6(self, arg): return super(MoreStuff, self).deny(arg) @simple_decorator @striparg_auto def deny7(self, arg): return super(MoreStuff, self).deny(arg) assert insult(" PHP ") == "PHP sucks!" assert Stuff().deny(" a pony ") == "No you can't have a pony" assert MoreStuff().deny(" a backwards incompatible change ") == "No you can't have a backwards incompatible change" assert MoreStuff().deny2(" some figgy pudding ") == "No you can't have some figgy pudding" # fails assert MoreStuff().deny3(" a velociraptor ") == "No you can't have a velociraptor (says MoreStuff)" # fails assert MoreStuff().deny4(" true freedom on your iPad ") == "No you can't have true freedom on your iPad" assert MoreStuff().deny5(" a design contest ") == "No you can't have a design contest" assert MoreStuff().deny6(" less politically charged example code ") == "No you can't have less politically charged example code (says MoreStuff)" assert MoreStuff().deny7(" an auto_adapt_to_methods function that works ") == "No you can't have an auto_adapt_to_methods function that works" # fails
from functools import wraps, update_wrapper def method_dec(decorator): """ Converts a function decorator into a method decorator """ def _dec(func): def _wrapper(self, *args, **kwargs): 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 decorator(bound_func)(*args, **kwargs) return wraps(func)(_wrapper) update_wrapper(_dec, decorator) # Change the name, to avoid confusion, as this is *not* the same # decorator. _dec.__name__ = 'method_dec(%s)' % decorator.__name__ return _dec def striparg(func): """ Decorates functions that take a single string argument, and runs strip() on that arg before calling. """ def wrapper(arg): return func(arg.strip()) return wraps(func)(wrapper) def simple_decorator(func): """ No-op decorator, without @auto_adapt_to_methods """ def wrapper(*args): return func(*args) return wrapper def sign(method): """ Decorates *methods* that take a single argument and return a string. """ def wrapper(self, arg): return method(self, arg) + " (says " + self.__class__.__name__ + ")" return wraps(method)(wrapper) @striparg def insult(arg): return arg + " sucks!" class Stuff(object): @method_dec(striparg) def deny(self, arg): return "No you can't have " + arg class MoreStuff(Stuff): deny = Stuff.deny deny2 = simple_decorator(Stuff.deny) deny3 = sign(Stuff.deny) deny4 = method_dec(striparg)(Stuff.deny) @simple_decorator def deny5(self, arg): return super(MoreStuff, self).deny(arg) @sign def deny6(self, arg): return super(MoreStuff, self).deny(arg) @simple_decorator @method_dec(striparg) def deny7(self, arg): return super(MoreStuff, self).deny(arg) assert insult(" PHP ") == "PHP sucks!" assert Stuff().deny(" a pony ") == "No you can't have a pony" assert MoreStuff().deny(" a backwards incompatible change ") == "No you can't have a backwards incompatible change" assert MoreStuff().deny2(" some figgy pudding ") == "No you can't have some figgy pudding" assert MoreStuff().deny3(" a velociraptor ") == "No you can't have a velociraptor (says MoreStuff)" assert MoreStuff().deny4(" true freedom on your iPad ") == "No you can't have true freedom on your iPad" assert MoreStuff().deny5(" a design contest ") == "No you can't have a design contest" assert MoreStuff().deny6(" less politically charged example code ") == "No you can't have less politically charged example code (says MoreStuff)" assert MoreStuff().deny7(" an auto_adapt_to_methods function that works ") == "No you can't have an auto_adapt_to_methods function that works"